Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion .flake8
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ ignore = E203, E266, W503, ANN002, ANN003, ANN101, ANN102, ANN401, N807, N818, V
max-line-length = 119
max-complexity = 18
select = B,C,E,F,W,T4,B9,ANN,Q0,N8,VNE
exclude = .venv
exclude = venv
extend-exclude =
src/tests/*,
src/database/migrations/*
2 changes: 1 addition & 1 deletion docker-compose-local.yml
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ services:
- "5432:5432"
volumes:
- ./init.sql:/docker-entrypoint-initdb.d/init.sql
- postgres_theater_data:/var/lib/postgresql/data/
- postgres_theater_data:/var/lib/postgresql
networks:
- theater_network
healthcheck:
Expand Down
361 changes: 177 additions & 184 deletions poetry.lock

Large diffs are not rendered by default.

2 changes: 2 additions & 0 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ asyncpg = "^0.30.0"
aiosqlite = "^0.21.0"
aioboto3 = "^13.4.0"
pytest-asyncio = "^0.25.3"
psycopg = {extras = ["binary"], version = "^3.3.3"}
greenlet = "^3.3.2"


[build-system]
Expand Down
3 changes: 2 additions & 1 deletion src/database/models/accounts.py
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,8 @@ class UserModel(Base):
profile: Mapped[Optional["UserProfileModel"]] = relationship(
"UserProfileModel",
back_populates="user",
cascade="all, delete-orphan"
cascade="all, delete-orphan",
lazy="selectin"
)

def __repr__(self):
Expand Down
49 changes: 44 additions & 5 deletions src/routes/accounts.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
from datetime import datetime, timezone
from typing import cast

from fastapi import APIRouter, Depends, status, HTTPException
from fastapi import APIRouter, Depends, status, HTTPException, BackgroundTasks, Header, UploadFile, File, Form
from sqlalchemy import select, delete
from sqlalchemy.exc import SQLAlchemyError
from sqlalchemy.ext.asyncio import AsyncSession
Expand Down Expand Up @@ -31,11 +31,21 @@
TokenRefreshRequestSchema,
TokenRefreshResponseSchema
)

from security.interfaces import JWTAuthManagerInterface

router = APIRouter()


api_url = "http://127.0.0.1:8000/api/v1/accounts/"


async def get_user_by_id(id: int, db: AsyncSession):
stmt = select(UserModel).filter_by(id=id)
result = await db.execute(stmt)
return result.scalar_one_or_none()


@router.post(
"/register/",
response_model=UserRegistrationResponseSchema,
Expand Down Expand Up @@ -67,7 +77,9 @@
)
async def register_user(
user_data: UserRegistrationRequestSchema,
background_tasks: BackgroundTasks,
db: AsyncSession = Depends(get_db),
notificator: EmailSenderInterface = Depends(get_accounts_email_notificator)
) -> UserRegistrationResponseSchema:
"""
Endpoint for user registration.
Expand Down Expand Up @@ -117,7 +129,12 @@ async def register_user(

activation_token = ActivationTokenModel(user_id=new_user.id)
db.add(activation_token)

await db.flush()
background_tasks.add_task(
notificator.send_activation_email,
str(user_data.email),
f"{api_url}activate/?token={activation_token.token}"

Choose a reason for hiding this comment

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

The URL generated for the activation email is for a GET request and only includes the token. However, the /activate/ endpoint is a POST endpoint and requires both the email and token in the request body. A user clicking this link won't be able to activate their account, which makes this feature non-functional.

Choose a reason for hiding this comment

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

The URL generated for account activation is not functional. The /activate/ endpoint is a POST method that expects a request body with an email and token. However, a user clicking this link in an email will send a GET request with only the token in the URL, which will fail.

This same problem exists for the password reset link generated on line 295. This was a critical issue from the previous review that needs to be resolved for the feature to be usable.

)
Comment on lines +133 to +137

Choose a reason for hiding this comment

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

The activation URL generated here is a GET link containing only a token. However, the activate_account endpoint is a POST endpoint that expects both an email and a token in the request body. This mismatch will prevent the activation flow from working correctly. Please ensure the generated URL is compatible with the target endpoint's requirements.

await db.commit()
await db.refresh(new_user)
except SQLAlchemyError as e:
Expand Down Expand Up @@ -162,8 +179,10 @@ async def register_user(
},
)
async def activate_account(
activation_data: UserActivationRequestSchema,
db: AsyncSession = Depends(get_db),
background_tasks: BackgroundTasks,
activation_data: UserActivationRequestSchema,
notificator: EmailSenderInterface = Depends(get_accounts_email_notificator),
db: AsyncSession = Depends(get_db),
) -> MessageResponseSchema:
"""
Endpoint to activate a user's account.
Expand All @@ -185,6 +204,7 @@ async def activate_account(
- 400 Bad Request if the activation token is invalid or expired.
- 400 Bad Request if the user account is already active.
"""

stmt = (
select(ActivationTokenModel)
.options(joinedload(ActivationTokenModel.user))
Expand Down Expand Up @@ -216,8 +236,13 @@ async def activate_account(

user.is_active = True
await db.delete(token_record)
await db.flush()
background_tasks.add_task(
notificator.send_activation_complete_email,
str(user.email),
f"{api_url}login/"
)
await db.commit()

return MessageResponseSchema(message="User account activated successfully.")


Expand All @@ -232,7 +257,9 @@ async def activate_account(
status_code=status.HTTP_200_OK,
)
async def request_password_reset_token(
background_tasks: BackgroundTasks,
data: PasswordResetRequestSchema,
notificator: EmailSenderInterface = Depends(get_accounts_email_notificator),
db: AsyncSession = Depends(get_db),
) -> MessageResponseSchema:
"""
Expand Down Expand Up @@ -262,6 +289,11 @@ async def request_password_reset_token(
reset_token = PasswordResetTokenModel(user_id=cast(int, user.id))
db.add(reset_token)
await db.commit()
background_tasks.add_task(
notificator.send_password_reset_email,
str(user.email),
f"{api_url}reset-password/complete/?token={reset_token.token}"
)
Comment on lines 289 to +296

Choose a reason for hiding this comment

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

The password reset link sent to the user is missing the required token. The reset_password endpoint needs this token to proceed. Additionally, the database operations are not ordered correctly to retrieve the token's value before sending the email. Consider using db.flush() to get the generated token, include it in the URL, and then call db.commit() at the end of the block, similar to the implementation in the register_user function.


return MessageResponseSchema(
message="If you are registered, you will receive an email with instructions."
Expand Down Expand Up @@ -313,6 +345,8 @@ async def request_password_reset_token(
)
async def reset_password(
data: PasswordResetCompleteRequestSchema,
background_tasks: BackgroundTasks,
notificator: EmailSenderInterface = Depends(get_accounts_email_notificator),
db: AsyncSession = Depends(get_db),
) -> MessageResponseSchema:
"""
Expand Down Expand Up @@ -376,6 +410,11 @@ async def reset_password(
detail="An error occurred while resetting the password."
)

background_tasks.add_task(
notificator.send_password_reset_complete_email,
str(data.email),
f"{api_url}login/"
)
return MessageResponseSchema(message="Password reset successfully.")


Expand Down
100 changes: 98 additions & 2 deletions src/routes/profiles.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,101 @@
from fastapi import APIRouter
from datetime import date

from PIL import Image
from fastapi import APIRouter, Form, UploadFile, File, Depends, HTTPException
from typing import Annotated

from pydantic import ValidationError
from sqlalchemy.ext.asyncio import AsyncSession

from database import get_db
from config import get_s3_storage_client, get_jwt_auth_manager
from database.models.accounts import UserProfileModel, GenderEnum
from exceptions import TokenExpiredError, InvalidTokenError, S3FileUploadError
from routes.accounts import get_user_by_id
from schemas.profiles import ProfileResponseSchema, ProfileRequestSchema
from security.headers import api_key_header, validate_api_key
from security.interfaces import JWTAuthManagerInterface
from storages import S3StorageInterface

router = APIRouter()

# Write your code here

@router.post(
"/users/{user_id}/profile/",
summary="Create user profile",
description="Creation user profile with adding avatar",
response_model=ProfileResponseSchema,
status_code=201,
dependencies=[Depends(validate_api_key)]
)
async def create_profile(
user_id: int,
first_name: str = Form(),
last_name: str = Form(),
gender: str = Form(),
date_of_birth: date = Form(),
info: str = Form(),
avatar: UploadFile = File(),
access_token: Annotated[str | None, Depends(api_key_header)] = None,
db: AsyncSession = Depends(get_db),
s3_client: S3StorageInterface = Depends(get_s3_storage_client),
jwt_manager: JWTAuthManagerInterface = Depends(get_jwt_auth_manager)
):
try:
ProfileRequestSchema(
first_name=first_name,
last_name=last_name,
gender=gender,
date_of_birth=date_of_birth,
info=info,
avatar=avatar
)
except ValidationError as exc:
raise HTTPException(status_code=422, detail=exc.errors()[0]["msg"])
try:
decoded_auth_header = access_token.split()
user_jwt = jwt_manager.decode_access_token(decoded_auth_header[1])
current_user = await get_user_by_id(user_jwt["user_id"], db)
query_user = await get_user_by_id(user_id, db)

if not query_user or not query_user.is_active:
raise HTTPException(status_code=401, detail="User not found or not active.")

if query_user.profile:

Choose a reason for hiding this comment

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

There's a potential AttributeError here if query_user is None because the user ID doesn't exist. The check for user existence on line 66 should be performed before attempting to access any of the user's attributes, such as .profile.

raise HTTPException(status_code=400, detail="User already has a profile.")

if current_user.id != user_id and current_user.group_id == 1:

Choose a reason for hiding this comment

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

Using a magic number like 1 for the user group ID makes the code less readable and harder to maintain. It's better to use the UserGroupEnum for comparison. To do this without causing extra database queries, you might consider adjusting the get_user_by_id function to eager-load the group relationship.

raise HTTPException(status_code=403, detail="You don't have permission to edit this profile.")

try:
avatar.file.seek(0)
avatar_bytes = avatar.file.read()
image = Image.open(avatar.file)
image_format = image.format.lower()
if image_format == "jpeg":
image_format = "jpg"
file_name = f"avatars/{user_id}_avatar.{image_format}"
profile_db = UserProfileModel(
first_name=first_name.lower(),
last_name=last_name.lower(),
gender=GenderEnum(gender),
date_of_birth=date_of_birth,
info=info,
avatar=file_name,
user=query_user
)
await s3_client.upload_file(file_name, avatar_bytes)
avatar_url = await s3_client.get_file_url(file_name)

db.add(profile_db)
await db.commit()
profile_db.avatar = avatar_url
return profile_db
except S3FileUploadError:
raise HTTPException(status_code=500, detail="Failed to upload avatar. Please try again later.")
except Exception as exc:
raise HTTPException(status_code=500, detail=str(exc))
except TokenExpiredError:
raise HTTPException(status_code=401, detail="Token has expired.")
except InvalidTokenError:
raise HTTPException(status_code=401, detail="Invalid token")
5 changes: 4 additions & 1 deletion src/schemas/accounts.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
from pydantic import BaseModel, EmailStr, field_validator
from dataclasses import Field

from pydantic import BaseModel, EmailStr, field_validator, StringConstraints


from database import accounts_validators

Expand Down
55 changes: 53 additions & 2 deletions src/schemas/profiles.py
Original file line number Diff line number Diff line change
@@ -1,13 +1,64 @@
from datetime import date
from typing import Any

from fastapi import UploadFile, Form, File, HTTPException
from pydantic import BaseModel, field_validator, HttpUrl
from pydantic import BaseModel, field_validator, HttpUrl, ConfigDict
from typing_extensions import Self

from database.models.accounts import GenderEnum
from validation import (
validate_name,
validate_image,
validate_gender,
validate_birth_date
)
from validation.profile import validate_info

# Write your code here

class ProfileBaseSchema(BaseModel):
first_name: str
last_name: str
gender: str
date_of_birth: date
info: str


class ProfileRequestSchema(ProfileBaseSchema):
avatar: UploadFile

@field_validator("first_name", "last_name")
@classmethod
def validate_first_last_name(cls, value):
validate_name(value)
return value

@field_validator("gender")
@classmethod
def validate_gender(cls, value):
validate_gender(value)
return value

@field_validator("date_of_birth")
@classmethod
def validate_date_of_birth(cls, value):
validate_birth_date(value)
return value

@field_validator("info")
@classmethod
def validate_info(cls, value):
validate_info(value)
return value

@field_validator("avatar")
@classmethod
def validate_avatar(cls, value):
validate_image(value)
Comment on lines +29 to +56

Choose a reason for hiding this comment

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

All the @field_validator methods in this schema (validate_first_last_name, validate_gender, validate_date_of_birth, validate_info, and validate_avatar) are missing a return value statement. A Pydantic validator must always return a value; otherwise, the field it's validating will be assigned None, causing data loss.

return value


class ProfileResponseSchema(ProfileBaseSchema):
model_config = ConfigDict(from_attributes=True)
id: int
user_id: int
avatar: str
19 changes: 19 additions & 0 deletions src/security/headers.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
from fastapi import Security, HTTPException
from fastapi.security import APIKeyHeader

api_key_header = APIKeyHeader(name="Authorization", auto_error=False)


async def validate_api_key(api_key: str = Security(api_key_header)):

if not api_key:
raise HTTPException(
status_code=401,
detail="Authorization header is missing"
)

decoded_auth_header = api_key.split()
if decoded_auth_header[0] != "Bearer" or len(decoded_auth_header) != 2:
raise HTTPException(status_code=401, detail="Invalid Authorization header format. Expected 'Bearer <token>'")

return api_key
12 changes: 12 additions & 0 deletions src/security/middleware.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
from starlette.middleware.cors import CORSMiddleware

from main import app


app.add_middleware(
CORSMiddleware,
allow_origins=["*"],
allow_credentials=False,
allow_methods=["*"],
allow_headers=["*"],
)
Loading
Loading