Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
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="joined"

Choose a reason for hiding this comment

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

Using lazy="joined" is a valid performance optimization that avoids the N+1 query problem by fetching the user's profile in the same query as the user. However, this means a JOIN will be performed every time a UserModel is loaded, even in scenarios where the profile data isn't needed (like during login). For frequently accessed one-to-one relationships this is fine, but it's good to be aware of the trade-off. The default lazy="select" would load the profile only when accessed.

)

def __repr__(self):
Expand Down
78 changes: 73 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,11 @@ async def register_user(
},
)
async def activate_account(
activation_data: UserActivationRequestSchema,
db: AsyncSession = Depends(get_db),
# token: str,
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 +205,36 @@ 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)
# .where(ActivationTokenModel.token == token)
# .options(joinedload(ActivationTokenModel.user))
# .join(UserModel)
# )
#
# result = await db.execute(stmt)
# token_record = result.scalar_one_or_none()
# now_utc = datetime.now(timezone.utc)
# if not token_record or cast(datetime, token_record.expires_at).replace(tzinfo=timezone.utc) < now_utc:
# if token_record:
# await db.delete(token_record)
# await db.commit()
# raise HTTPException(
# status_code=status.HTTP_400_BAD_REQUEST,
# detail="Invalid or expired activation token."
# )
#
# user = token_record.user
# if user.is_active:
# raise HTTPException(
# status_code=status.HTTP_400_BAD_REQUEST,
# detail="User account is already active."
# )
#
# user.is_active = True
# await db.delete(token_record)
# await db.commit()

stmt = (
select(ActivationTokenModel)
.options(joinedload(ActivationTokenModel.user))
Expand Down Expand Up @@ -217,7 +267,11 @@ async def activate_account(
user.is_active = True
await db.delete(token_record)
await db.commit()

background_tasks.add_task(
notificator.send_activation_complete_email,
str(user.email),
f"{api_url}login/"
)
return MessageResponseSchema(message="User account activated successfully.")


Expand All @@ -232,7 +286,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 +318,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/"

Choose a reason for hiding this comment

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

The URL for the password reset email is missing the reset token. According to the requirements, the /reset-password/complete/ endpoint needs this token to work. This was a critical issue from the previous review that remains unaddressed. You need to include the generated reset_token.token in this URL.

)
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 +374,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 +439,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
94 changes: 92 additions & 2 deletions src/routes/profiles.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,95 @@
from fastapi import APIRouter
from datetime import date

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

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
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/",
# response_model=TokenRefreshResponseSchema,
Copy link

Choose a reason for hiding this comment

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

remove this comment

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 Exception as exc:

Choose a reason for hiding this comment

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

Catching a generic Exception here is a bit risky because it could hide unexpected errors that are not related to Pydantic validation. It would be safer to catch the specific pydantic.ValidationError to ensure you're only handling validation failures as intended.

Copy link

Choose a reason for hiding this comment

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

Catching a generic Exception here is a bit risky because it could hide unexpected errors that are not related to Pydantic validation. It would be safer to catch the specific pydantic.ValidationError to ensure you're only handling validation failures as intended.

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

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

try:
avatar_bytes = avatar.file.read()
file_name = f"avatars/{user_id}_avatar.jpg"

Choose a reason for hiding this comment

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

The filename for the avatar is hardcoded with a .jpg extension. However, the validation allows for other formats like PNG. This can cause a mismatch between the file's content and its extension. Consider dynamically setting the extension based on the uploaded file's type, for example by using the extension from avatar.filename.

await s3_client.upload_file(file_name, avatar_bytes)

avatar_url = await s3_client.get_file_url(file_name)
profile_db = UserProfileModel(
first_name=first_name.lower(),
last_name=last_name.lower(),
gender=gender,
date_of_birth=date_of_birth,
info=info,
avatar=file_name,
user=query_user
)
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="Token has invalid")

Choose a reason for hiding this comment

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

This error message is grammatically incorrect. Consider changing the detail to "Invalid token" for better clarity.

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
50 changes: 48 additions & 2 deletions src/schemas/profiles.py
Original file line number Diff line number Diff line change
@@ -1,13 +1,59 @@
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)

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

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

@field_validator("info")
@classmethod
def validate_info(cls, value):
validate_info(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.



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
Loading
Loading