Feature/login #195

kobyfogel · 2021-02-03T13:25:41Z

This is my version for users management system: register, login, and current logged-in user.
This version doesn't use fast-api_users library.
My comment to 1 CR was not answered, so it's not 'resolved' yet.
Instructions for using security dependencies are given in README.md file.

app/routers/login.py

app/config.py.example

README.md

app/internal/security/dependancies.py

app/internal/security/ouath2.py

app/main.py

app/routers/logout.py

app/routers/register.py

This reverts commit 8fbe36a.

kobyfogel · 2021-02-09T23:13:37Z

If i clicked on 'resoved' on an issue, without addressing it, it was just by mistake. In few of the cases, i just thought that the CR is for another line of code, and i thought i did resolve it.

yammesicka

Awesome progress - keep up the good job :)

README.md

yammesicka · 2021-02-10T07:28:06Z

app/internal/security/ouath2.py

+            token, JWT_KEY, algorithms=JWT_ALGORITHM)
+        jwt_username = jwt_payload.get("sub")
+        jwt_hashed_password = jwt_payload.get("hashed_password")
+        db_user = await get_db_user_by_username(db, username=jwt_username)


Sorry, but I still unable to understand why.

Why not having one method to check the username and the password against the database (which happens only if the user asks to log-in), and another method to check if the user is authenticated (have a valid JWT token)?

app/internal/security/ouath2.py

yammesicka · 2021-02-10T07:33:13Z

app/internal/security/ouath2.py

+        jwt_user_id = jwt_payload.get("user_id")
+        db_user = await get_db_user_by_username(db, username=jwt_username)
+        if db_user and db_user.id == jwt_user_id:
+            return CurrentUser(**db_user.__dict__)


What is the difference between CurrentUser and LoginUser?

LoginUser is a pydantic object for internal actions management of the security system.
CurrentUser is the Object that will be returned to the routes from the dependencies functions, after verifying JWT token. It's purpose is to return a User Base Model object to the routed, minus the password column, and keep the workflow with pydantic models.
I'm thinking maybe to re-change it to a Base Model object, as the code is keep on changing by other. And this object has to be maintained in order to keep it's functionality.

What are the downsides of having only one of them? Currently having two different classes for two similar objects is a bit confusing from the developer side

If by 'them' you mean LoginUser and CurrentUser, so they are not the same at all. LoginUser is created from the login form, and verifies that required information was given by the user. It is essential for this action. As CurrentUser supposed to return all the User information to the routes, after jwt verification.
Maybe it will be better to delete CurrentUser, and just return the User Base Object? It's a db object, no confusions there. That will be also much easy to understand just by looking at the type annotations.

I think the major source of confusion is in the object's name. Maybe we should find a better name which describes it better and which highlights the differences between them.

if it ok by you, i will just return db_user. otherwise, every change in User base model will have to be updated in CurrentUser, in order for it to return the new column.

Just be sure to name stuff very indicatively. Otherwise it'll be really PITA to maintain :)

yammesicka · 2021-02-10T08:37:51Z

app/internal/security/ouath2.py

+        jwt_user_id = jwt_payload.get("user_id")
+        db_user = await get_db_user_by_username(db, username=jwt_username)
+        if db_user and db_user.id == jwt_user_id:
+            return CurrentUser(**db_user.__dict__)


What are the downsides of having only one of them? Currently having two different classes for two similar objects is a bit confusing from the developer side

…into feature/login

yammesicka

Great job! Another review to go :)

yammesicka · 2021-02-11T09:29:47Z

app/internal/security/dependancies.py

+    A dependency function protecting routes for only logged in user
+    """
+    await check_jwt_token(db, jwt)
+    return True


Will this function always return True?

this function activates check_jwt_token function. That function will raise an exception if validation fails. so yes..
should i add that to annotations?

So catch the exception and return False.
The function called is_logged_in, the developer who sees it surely expect to get True/False :)

yammesicka · 2021-02-11T09:31:16Z

app/internal/security/dependancies.py

+from starlette.requests import Request
+
+
+async def validate_user(


Hasn't this function should have been called get_user?

app/internal/security/dependancies.py

yammesicka · 2021-02-11T09:33:41Z

app/internal/security/dependancies.py

@@ -0,0 +1,66 @@
+from typing import Union


I believe some problems in this CR could be resolved with better naming.

We should also rethink a bit about the design: there are some functions that it's hard to tell why they're different one from another.

app/internal/security/dependancies.py

app/routers/login.py

app/routers/register.py

app/templates/base.html

yammesicka

Great job and great improvement! Here are some things to improve further :)

yammesicka · 2021-02-12T01:01:51Z

app/internal/security/dependancies.py

+    A dependency function protecting routes for only logged in user
+    """
+    await check_jwt_token(db, jwt)
+    return True


So catch the exception and return False.
The function called is_logged_in, the developer who sees it surely expect to get True/False :)

yammesicka · 2021-02-12T01:02:46Z

app/internal/security/dependancies.py

+
+async def optional_logged_in_user(
+    request: Request,
+        db: Session = Depends(get_db)) -> Union[User, type(None)]:


Optional[User] (instead of Union with NoneType)

Remove the indentation before the db:

Prefix ) in a new line, with comma after the previous line.

Look how the code become much more readable:

async def current_user( request: Request, db: Session = Depends(get_db), ) -> Optional[User]:

yammesicka · 2021-02-12T01:06:55Z

app/internal/security/dependancies.py

+    """
+    A dependency function.
+    Returns logged in User object if exists.
+    None if not.
+    """


Prefer a docstring that matches the guidelines in PEP 257:

"""Return the currently logged in user. Return logged in User object if exists, None if not. A dependency function. """

yammesicka · 2021-02-12T01:13:18Z

app/routers/login.py

+@router.get("/login")
+async def login_user_form(
+        request: Request, message: Optional[str] = "",
+        current_user: Union[User, None] = Depends(optional_logged_in_user)


yammesicka · 2021-02-12T01:14:17Z

app/routers/login.py

+@router.post('/login')
+async def login(
+        request: Request,
+        next: Optional[str] = "/",


The next can be http://attacker.com, for example.

Please read about open redirection attack.

yammesicka · 2021-02-12T01:19:37Z

app/internal/security/dependancies.py

+    return await get_user(db, user_id, username, request.url.path)
+
+
+async def optional_logged_in_user(


Is that another instance of get_user?

No it's not.
It is designed to be a route dependency.
get_user is a function that helps other dependencies, after the jwt token was extracted from cookie, and was checked.
The way it is done varies with the different dependencies.

Unfortunately I can't understand what is the difference even after your explanation.

Please try to look at the issue from the perspective of the programmer who uses your system.

He probably wants to have:

is_logged_in

is_manager

get_user_by_username / _id / _email

login / logout

That's it.

yammesicka · 2021-02-12T01:25:50Z

app/routers/logout.py

+
+
+@router.get('/logout')
+async def logout(request: Request):


Can we somehow check the logout request using some CSRF cookie? Currently it will disconnect you if you'll click a link that I've sent you.

(Don't overthink it if you find it too hard - I do want to finish this ticket ASAP)

I started to read about it. Seems like it's a whole topic of security, which is new to me.
I am always eager to learn more but that will take some time to understand in a proper depth.
If we can leave it as it is for now, I promise to try to improve it after i finish my tickets.
If you have any good suggestions for me regarding what on this subject to put my focus on, i will be more then happy to know.

yammesicka · 2021-02-12T01:28:07Z

app/routers/register.py

+
+
+def get_error_messages_by_fields(errors: [list]) -> Dict[str, str]:
+    '''Getting validation errors by fields from pydantic ValidationError'''


Decide on whether to use ''' or """ (you mixed them inside the file)

app/routers/register.py

yammesicka · 2021-02-12T01:30:06Z

app/routers/register.py

+)
+
+
+async def user_create(


Prefer create_user

kobyfogel · 2021-02-12T10:51:27Z

I tried to answer the best i can, hopefully it clarifies everything that was not understood.
I also tried to fix what i could, and i hope that will be good enough.

yammesicka

Hope we're almost done :)

yammesicka · 2021-02-14T03:32:14Z

app/internal/security/dependancies.py

+    return True
+
+
+async def logged_in_user(


So how is it different from get_current_user?

You must understand you're building an API for other programmers.
All the functions must be easily understood from their names only.

Currently it's really hard to tell what this function (or any other function in this file) do from only looking at it's name.
We must find a better naming that will describe it in a way the other programmers would be able to understand what is does.

yammesicka · 2021-02-14T03:37:38Z

app/internal/security/dependancies.py

+    return await get_user(db, user_id, username, request.url.path)
+
+
+async def optional_logged_in_user(


Unfortunately I can't understand what is the difference even after your explanation.

Please try to look at the issue from the perspective of the programmer who uses your system.

He probably wants to have:

is_logged_in

is_manager

get_user_by_username / _id / _email

login / logout

That's it.

yammesicka · 2021-02-14T03:40:22Z

app/routers/register.py

@@ -0,0 +1,101 @@
+from typing import Dict
+
+from app.internal.security.ouath2 import get_hashed_password


This paragraph should be the 3rd one

kobyfogel added 23 commits January 19, 2021 23:08

WIP register - not finished

43b3f1b

register before updated pull

d211377

register for review

2f0501a

register fixed errors

a5a1a04

register fixed flake-8

5d2c52e

register fixed tests flake8

eafbe37

register fixed tests issues

56d0ca3

register placeholders fix

00d9e7d

register flake8 fix

c9c777e

register flake8 more fixes

bb2b52d

fixed CR suggestions

a32c9c4

first commit

deee638

fixed check_jwt_token

7ca40e1

Not Finshed

1bdbc54

minor fix

eed52e2

starting dependancy

b793e2c

dependancies working

46820e1

redirectin and user messages

68e40fe

async fixing

b7e7e60

exception handler

e3c620a

quary parameters

b6b7441

Documentation added

d1849bb

after pull and fixing conflicts

8d7f135

Gonzom reviewed Feb 3, 2021

View reviewed changes

app/routers/login.py Outdated Show resolved Hide resolved

kobyfogel added 6 commits February 3, 2021 19:32

fixed pep8

84db349

pep8 more fixes

d9ae477

pep8 config fix

d7465b5

pep8 jwt fix

9d735b1

pep8 jwt final fix

4f5a127

pep8 final fix

4392974

kobyfogel added 4 commits February 7, 2021 18:39

updating requirements

b46d679

test added

73f53c1

conflicts fixed

c15a451

flake8 fix

c3cad9d

yammesicka requested changes Feb 9, 2021

View reviewed changes

kobyfogel added 9 commits February 9, 2021 16:11

CR fixing

befd5f9

conflicts fixed

063489f

flake8 fixes

da12351

flake8 fix2

cc4fdae

flake8 fixes3

0ae794a

flake8 fixes4

7a47985

flake8 fixes5

e7bc55a

CR fix

8fbe36a

Revert "CR fix"

f203c38

This reverts commit 8fbe36a.

yammesicka requested changes Feb 10, 2021

View reviewed changes

kobyfogel added 3 commits February 10, 2021 18:53

CR fixes

bbb241b

conflicts fix

b00439d

Merge branch 'develop' of https://github.com/PythonFreeCourse/calendar …

84a6fa7

…into feature/login

yammesicka reviewed Feb 11, 2021

View reviewed changes

kobyfogel added 2 commits February 11, 2021 21:02

many CR fixes

5f3e3e1

conflicts fixed

5d47833

yammesicka reviewed Feb 12, 2021

View reviewed changes

CR fixes

2488679

yammesicka requested changes Feb 14, 2021

View reviewed changes

kobyfogel added 2 commits February 14, 2021 13:08

changed dependencies according to CR

7d5395c

fix conflicts

8f59086

yammesicka merged commit cec69ea into PythonFreeCourse:develop Feb 15, 2021

		from starlette.requests import Request


		async def validate_user(

		return await get_user(db, user_id, username, request.url.path)


		async def optional_logged_in_user(



		def get_error_messages_by_fields(errors: [list]) -> Dict[str, str]:
		'''Getting validation errors by fields from pydantic ValidationError'''

		@@ -0,0 +1,101 @@
		from typing import Dict

		from app.internal.security.ouath2 import get_hashed_password

Feature/login #195

Feature/login #195

Uh oh!

Conversation

kobyfogel commented Feb 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kobyfogel commented Feb 9, 2021

Uh oh!

yammesicka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kobyfogel Feb 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kobyfogel Feb 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yammesicka Feb 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yammesicka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yammesicka left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kobyfogel commented Feb 3, 2021 •

edited

Loading

kobyfogel Feb 10, 2021 •

edited

Loading

kobyfogel Feb 10, 2021 •

edited

Loading

yammesicka Feb 11, 2021 •

edited

Loading

yammesicka left a comment •

edited

Loading