Skip to content
Merged
Show file tree
Hide file tree
Changes from 51 commits
Commits
Show all changes
54 commits
Select commit Hold shift + click to select a range
43b3f1b
WIP register - not finished
kobyfogel Jan 19, 2021
d211377
register before updated pull
kobyfogel Jan 22, 2021
2f0501a
register for review
kobyfogel Jan 22, 2021
a5a1a04
register fixed errors
kobyfogel Jan 23, 2021
5d2c52e
register fixed flake-8
kobyfogel Jan 23, 2021
eafbe37
register fixed tests flake8
kobyfogel Jan 23, 2021
56d0ca3
register fixed tests issues
kobyfogel Jan 23, 2021
00d9e7d
register placeholders fix
kobyfogel Jan 23, 2021
c9c777e
register flake8 fix
kobyfogel Jan 23, 2021
bb2b52d
register flake8 more fixes
kobyfogel Jan 23, 2021
a32c9c4
fixed CR suggestions
kobyfogel Jan 24, 2021
deee638
first commit
kobyfogel Jan 24, 2021
7ca40e1
fixed check_jwt_token
kobyfogel Jan 25, 2021
1bdbc54
Not Finshed
kobyfogel Jan 26, 2021
eed52e2
minor fix
kobyfogel Jan 26, 2021
b793e2c
starting dependancy
kobyfogel Jan 31, 2021
46820e1
dependancies working
kobyfogel Feb 1, 2021
68e40fe
redirectin and user messages
kobyfogel Feb 1, 2021
b7e7e60
async fixing
kobyfogel Feb 1, 2021
e3c620a
exception handler
kobyfogel Feb 1, 2021
b6b7441
quary parameters
kobyfogel Feb 2, 2021
d1849bb
Documentation added
kobyfogel Feb 3, 2021
8d7f135
after pull and fixing conflicts
kobyfogel Feb 3, 2021
84db349
fixed pep8
kobyfogel Feb 3, 2021
d9ae477
pep8 more fixes
kobyfogel Feb 3, 2021
d7465b5
pep8 config fix
kobyfogel Feb 3, 2021
9d735b1
pep8 jwt fix
kobyfogel Feb 3, 2021
4f5a127
pep8 jwt final fix
kobyfogel Feb 3, 2021
4392974
pep8 final fix
kobyfogel Feb 3, 2021
b9ee9d3
CR fixes, tests added
kobyfogel Feb 7, 2021
94eb29a
after merge conflicts
kobyfogel Feb 7, 2021
97d8aff
flake8 fixes
kobyfogel Feb 7, 2021
8b54f67
flake8 fixes2
kobyfogel Feb 7, 2021
b46d679
updating requirements
kobyfogel Feb 7, 2021
73f53c1
test added
kobyfogel Feb 7, 2021
c15a451
conflicts fixed
kobyfogel Feb 7, 2021
c3cad9d
flake8 fix
kobyfogel Feb 7, 2021
befd5f9
CR fixing
kobyfogel Feb 9, 2021
063489f
conflicts fixed
kobyfogel Feb 9, 2021
da12351
flake8 fixes
kobyfogel Feb 9, 2021
cc4fdae
flake8 fix2
kobyfogel Feb 9, 2021
0ae794a
flake8 fixes3
kobyfogel Feb 9, 2021
7a47985
flake8 fixes4
kobyfogel Feb 9, 2021
e7bc55a
flake8 fixes5
kobyfogel Feb 9, 2021
8fbe36a
CR fix
kobyfogel Feb 9, 2021
f203c38
Revert "CR fix"
kobyfogel Feb 9, 2021
bbb241b
CR fixes
kobyfogel Feb 10, 2021
b00439d
conflicts fix
kobyfogel Feb 10, 2021
84a6fa7
Merge branch 'develop' of https://github.com/PythonFreeCourse/calenda…
kobyfogel Feb 10, 2021
5f3e3e1
many CR fixes
kobyfogel Feb 11, 2021
5d47833
conflicts fixed
kobyfogel Feb 11, 2021
2488679
CR fixes
kobyfogel Feb 12, 2021
7d5395c
changed dependencies according to CR
kobyfogel Feb 14, 2021
8f59086
fix conflicts
kobyfogel Feb 14, 2021
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,9 @@ dmypy.json
# Pyre type checker
.pyre/


# register stuff
run.txt
# VScode
.vscode/
app/.vscode/
Expand Down
10 changes: 4 additions & 6 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ virtualenv env
pip install -r requirements.txt
# Copy app\config.py.example to app\config.py.
# Edit the variables' values.
# Rendering JWT_KEY:
python -c "import secrets; from pathlib import Path; f = Path('app/config.py'); f.write_text(f.read_text().replace('JWT_KEY_PLACEHOLDER', secrets.token_hex(32), 1));"
uvicorn app.main:app --reload
```

Expand All @@ -45,12 +47,8 @@ source venv/bin/activate
pip install -r requirements.txt
cp app/config.py.example app/config.py
# Edit the variables' values.
uvicorn app.main:app --reload
```
### Running tests
```shell
python -m pytest --cov-report term-missing --cov=app tests
```
# Rendering JWT_KEY:
python -c "import secrets; from pathlib import Path; f = Path('app/config.py'); f.write_text(f.read_text().replace('JWT_KEY_PLACEHOLDER', secrets.token_hex(32), 1));"

## Contributing
View [contributing guidelines](https://github.com/PythonFreeCourse/calendar/blob/master/CONTRIBUTING.md).
5 changes: 5 additions & 0 deletions app/config.py.example
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,11 @@ email_conf = ConnectionConfig(
USE_CREDENTIALS=True,
)


# security
JWT_KEY = "JWT_KEY_PLACEHOLDER"
JWT_ALGORITHM = "HS256"
JWT_MIN_EXP = 60 * 24 * 7
templates = Jinja2Templates(directory=os.path.join("app", "templates"))

# application name
Expand Down
6 changes: 6 additions & 0 deletions app/database/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,12 @@ class User(Base):
def __repr__(self):
return f'<User {self.id}>'

@staticmethod
async def get_by_username(db: Session, username: str) -> User:
"""query database for a user by unique username"""
return db.query(User).filter(
User.username == username).first()


class Event(Base):
__tablename__ = "events"
Expand Down
90 changes: 90 additions & 0 deletions app/database/schemas.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
from typing import Optional, Union
from pydantic import BaseModel, validator, EmailStr, EmailError


EMPTY_FIELD_STRING = 'field is required'
MIN_FIELD_LENGTH = 3
MAX_FIELD_LENGTH = 20


def fields_not_empty(field: Optional[str]) -> Union[ValueError, str]:
"""Global function to validate fields are not empty."""
if not field:
raise ValueError(EMPTY_FIELD_STRING)
return field


class UserBase(BaseModel):
"""
Validating fields types
Returns a User object without sensitive information
"""
username: str
email: str
full_name: str
description: Optional[str] = None

class Config:
orm_mode = True


class UserCreate(UserBase):
"""Validating fields types"""
password: str
confirm_password: str

"""
Calling to field_not_empty validaion function,
for each required field.
"""
_fields_not_empty_username = validator(
'username', allow_reuse=True)(fields_not_empty)
_fields_not_empty_full_name = validator(
'full_name', allow_reuse=True)(fields_not_empty)
_fields_not_empty_password = validator(
'password', allow_reuse=True)(fields_not_empty)
_fields_not_empty_confirm_password = validator(
'confirm_password', allow_reuse=True)(fields_not_empty)
_fields_not_empty_email = validator(
'email', allow_reuse=True)(fields_not_empty)

@validator('confirm_password')
def passwords_match(
cls, confirm_password: str,
values: UserBase) -> Union[ValueError, str]:
"""Validating passwords fields identical."""
if 'password' in values and confirm_password != values['password']:
raise ValueError("doesn't match to password")
return confirm_password

@validator('username')
def username_length(cls, username: str) -> Union[ValueError, str]:
"""Validating username length is legal"""
if not (MIN_FIELD_LENGTH < len(username) < MAX_FIELD_LENGTH):
raise ValueError("must contain between 3 to 20 charactars")
return username

@validator('password')
def password_length(cls, password: str) -> Union[ValueError, str]:
"""Validating username length is legal"""
if not (MIN_FIELD_LENGTH < len(password) < MAX_FIELD_LENGTH):
raise ValueError("must contain between 3 to 20 charactars")
return password

@validator('email')
def confirm_mail(cls, email: str) -> Union[ValueError, str]:
"""Validating email is valid mail address."""
try:
EmailStr.validate(email)
return email
except EmailError:
raise ValueError("address is not valid")


class User(UserBase):
"""
Validating fields types
Returns a User object without sensitive information
"""
id: int
is_active: bool
Empty file.
66 changes: 66 additions & 0 deletions app/internal/security/dependancies.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
from typing import Union
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


from app.dependencies import get_db
from app.database.models import User
from app.internal.security.ouath2 import (
Depends, Session, check_jwt_token,
get_authorization_cookie, HTTP_401_UNAUTHORIZED,
HTTPException)
from starlette.requests import Request


async def get_user(
db: Session, user_id: int, username: str, path: str) -> User:
'''
Helper for dependency functions.
Recives user details from decrypted jwt token,
and check them against the database.
returns User base model instance if succeeded,
raises HTTPException if fails.
'''
db_user = await User.get_by_username(db, username=username)
if db_user and db_user.id == user_id:
return db_user
else:
raise HTTPException(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefer not raising it as an http exception - let the route do it.

In these function you need to return a Pythonic response - which should probably be None, in this case.

Copy link
Contributor Author

@kobyfogel kobyfogel Feb 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image
The same goes for both of the comments:
The whole point of these functions, is to protect a certain route from not logged in user be able to reach it.
In any case which that i don't raise an Exception, but returning a Boolean, that will not do the job. I already checked that, and This is precisely the case with the 'optional_logged_in_user' dependency that i made, which is not protecting the route. You can see a demonstration for how it works in the current login route which deals with GET method. i already used it there, to achieve the opposite outcome - protecting a route from logged-in user.
This is why i configured 'auth_exception_handler' the way i did in the first place:
So i can create a dependency to protect a route, and at the same time render a specific message about what failed, and 'remembering' the user's original request.
Further more: when i did the version for security system using fastapi_Users library, that is exactly what i got when using their dependency. An unauthorize exception, just without all the details that my version returns to the user.
I was able to catch the exception exactly the same way:
https://github.com/kobyfogel/calendar/blob/feature/FastAPI-Users/app/internal/security/security_main.py

status_code=HTTP_401_UNAUTHORIZED,
headers=path,
detail="Your token is incorrect. Please log in again")


async def is_logged_in(
request: Request, db: Session = Depends(get_db),
jwt: str = Depends(get_authorization_cookie)) -> bool:
"""
A dependency function protecting routes for only logged in user
"""
await check_jwt_token(db, jwt)
return True
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this function always return True?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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



async def logged_in_user(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not a verb, so it's hard to understand what the function suppose to do.

Should it be is_logged_in?
How does it different from the current is_logged_in?

Copy link
Contributor Author

@kobyfogel kobyfogel Feb 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The difference is described in the Type Annotations.
They both protecting a route for only logged in user.
The difference is while is_logged_in returns a Boolean, this one returns the User object itself.
I think its extremely useful having this kind of dependency. Then you don't need to query the database in each route when you want to use the current user's different data.
At first i had just this one, but you did not want the database to be queried when the jwt is checked. So i made two different dependencies that preform different actions, thinking about other developers having useful dependencies.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

request: Request,
db: Session = Depends(get_db),
jwt: str = Depends(get_authorization_cookie)) -> User:
"""
A dependency function protecting routes for only logged in user.
Returns logged in User object.
"""
username, user_id = await check_jwt_token(db, jwt)
return await get_user(db, user_id, username, request.url.path)


async def optional_logged_in_user(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that another instance of get_user?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. is_logged_in
  2. is_manager
  3. get_user_by_username / _id / _email
  4. login / logout

That's it.

request: Request,
db: Session = Depends(get_db)) -> Union[User, type(None)]:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Optional[User] (instead of Union with NoneType)
  2. Remove the indentation before the db:
  3. 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]:

"""
A dependency function.
Returns logged in User object if exists.
None if not.
"""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
    """

if 'Authorization' in request.cookies:
jwt = request.cookies['Authorization']
else:
return None
username, user_id = await check_jwt_token(db, jwt)
return await get_user(db, user_id, username, request.url.path)
112 changes: 112 additions & 0 deletions app/internal/security/ouath2.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,112 @@
from datetime import datetime, timedelta
from typing import Union

from app.config import JWT_ALGORITHM, JWT_KEY, JWT_MIN_EXP
from app.database.models import User
from passlib.context import CryptContext
from fastapi import Depends, HTTPException
from fastapi.security import OAuth2PasswordBearer
import jwt
from jwt.exceptions import InvalidSignatureError
from sqlalchemy.orm import Session
from starlette.requests import Request
from starlette.responses import RedirectResponse
from starlette.status import HTTP_401_UNAUTHORIZED
from . import schema


pwd_context = CryptContext(schemes=["bcrypt"])
oauth_schema = OAuth2PasswordBearer(tokenUrl="/login")


def get_hashed_password(password: str) -> str:
"""Hashing user password"""
return pwd_context.hash(password)


def verify_password(plain_password: str, hashed_password: str) -> bool:
"""Verifying password and hashed password are equal"""
return pwd_context.verify(plain_password, hashed_password)


async def authenticate_user(
db: Session, new_user: schema.LoginUser
) -> Union[schema.LoginUser, bool]:
"""Verifying user is in database and password is correct"""
db_user = await User.get_by_username(db=db, username=new_user.username)
if db_user and verify_password(new_user.password, db_user.password):
return schema.LoginUser(
user_id=db_user.id,
username=new_user.username, password=db_user.password)
return False


def create_jwt_token(
user: schema.LoginUser, jwt_min_exp: int = JWT_MIN_EXP,
jwt_key: str = JWT_KEY) -> str:
"""Creating jwt-token out of user unique data"""
expiration = datetime.utcnow() + timedelta(minutes=jwt_min_exp)
jwt_payload = {
"sub": user.username,
"user_id": user.user_id,
"exp": expiration}
jwt_token = jwt.encode(
jwt_payload, jwt_key, algorithm=JWT_ALGORITHM)
return jwt_token


async def check_jwt_token(
db: Session,
token: str = Depends(oauth_schema), path: bool = None) -> User:
"""
Check whether JWT token is correct.
Returns jwt payloads if correct.
Raises HTTPException if fails to decode.
"""
try:
jwt_payload = jwt.decode(
token, JWT_KEY, algorithms=JWT_ALGORITHM)
return jwt_payload.get("sub"), jwt_payload.get("user_id")
except InvalidSignatureError:
raise HTTPException(
status_code=HTTP_401_UNAUTHORIZED,
headers=path,
detail="Your token is incorrect. Please log in again")
except jwt.ExpiredSignatureError:
raise HTTPException(
status_code=HTTP_401_UNAUTHORIZED,
headers=path,
detail="Your token has expired. Please log in again")
except jwt.DecodeError:
raise HTTPException(
status_code=HTTP_401_UNAUTHORIZED,
headers=path,
detail="Your token is incorrect. Please log in again")


async def get_authorization_cookie(request: Request) -> str:
"""
Extracts jwt from HTTPONLY cookie, if exists.
Raises HTTPException if not.
"""
if 'Authorization' in request.cookies:
return request.cookies['Authorization']
raise HTTPException(
status_code=HTTP_401_UNAUTHORIZED,
headers=request.url.path,
detail="Please log in to enter this page")


async def auth_exception_handler(
request: Request,
exc: HTTP_401_UNAUTHORIZED) -> RedirectResponse:
"""
Whenever HTTP_401_UNAUTHORIZED is raised,
redirecting to login route, with original requested url,
and details for why original request failed.
"""
paramas = f"?next={exc.headers}&message={exc.detail}"
url = f"/login{paramas}"
response = RedirectResponse(url=url)
response.delete_cookie('Authorization')
return response
16 changes: 16 additions & 0 deletions app/internal/security/schema.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
from typing import Optional

from pydantic import BaseModel


class LoginUser(BaseModel):
"""
Validating fields types
Returns a User object for signing in.
"""
user_id: Optional[int]
username: str
password: str

class Config:
orm_mode = True
18 changes: 13 additions & 5 deletions app/main.py
Original file line number Diff line number Diff line change
@@ -1,13 +1,14 @@
from fastapi import Depends, FastAPI, Request
from fastapi.staticfiles import StaticFiles
from sqlalchemy.orm import Session

from app import config
from app.database import engine, models
from app.dependencies import get_db, logger, MEDIA_PATH, STATIC_PATH, templates
from app.internal import daily_quotes, json_data_loader
from app.internal.languages import set_ui_language
from app.internal.security.ouath2 import auth_exception_handler
from app.routers.salary import routes as salary
from fastapi import Depends, FastAPI, Request
from fastapi.staticfiles import StaticFiles
from starlette.status import HTTP_401_UNAUTHORIZED
from sqlalchemy.orm import Session


def create_tables(engine, psql_environment):
Expand All @@ -28,12 +29,16 @@ def create_tables(engine, psql_environment):
app.mount("/media", StaticFiles(directory=MEDIA_PATH), name="media")
app.logger = logger

app.add_exception_handler(HTTP_401_UNAUTHORIZED, auth_exception_handler)

json_data_loader.load_to_db(next(get_db()))
# This MUST come before the app.routers imports.
set_ui_language()

from app.routers import ( # noqa: E402
agenda, calendar, categories, celebrity, currency, dayview,
email, event, invitation, profile, search, telegram, whatsapp
email, event, invitation, login, logout, profile, register,
search, telegram, whatsapp
)

json_data_loader.load_to_db(next(get_db()))
Expand All @@ -48,7 +53,10 @@ def create_tables(engine, psql_environment):
email.router,
event.router,
invitation.router,
login.router,
logout.router,
profile.router,
register.router,
salary.router,
search.router,
telegram.router,
Expand Down
Loading