Skip to content

CODE_REVIEW.md

Latest commit

 

History

History
306 lines (222 loc) · 8.55 KB

CODE_REVIEW.md

File metadata and controls

306 lines (222 loc) · 8.55 KB

Code Simplification Analysis Report

Generated: 2026-01-10 Analyzed: /home/seapagan/data/work/own/fastapi-template

Executive Summary

The codebase generally follows good practices with clear separation of concerns. However, the recently modified authentication code contains significant code duplication that presents a high-priority opportunity for simplification. The recent security enhancements (timing attack protections, metrics, and logging) have introduced repeated patterns across multiple methods.

Priority: HIGH

1. app/managers/auth.py - JWT Token Validation Duplication

Note

Related to SECURITY-REVIEW.md:

  • Section #15 (Code Duplication) - Different duplication (encoding vs validation)
  • Section #7 (Timing Attack in Token Type Checking) - Constant-time comparison duplicated

Issue: Massive code duplication across 4 methods (refresh, verify, reset_password, get_jwt_user). Each method contains identical logic for:

  • Token format validation
  • Constant-time token type comparison
  • User ID parsing with ASCII/digit validation
  • User existence checks with metrics/logging
  • Banned user checks with metrics/logging
  • JWT exception handling with metrics

Locations:

  • Lines 176-188, 198-210, 212-228, 230-240, 242-251, 257-272 (refresh)
  • Lines 280-292, 302-314, 316-332, 334-344, 346-355, 384-399 (verify)
  • Lines 454-466, 476-488, 490-506, 508-518, 520-529, 547-562 (reset_password)
  • Lines 630-643, 653-666, 668-681, 683-695, 697-709, 714-731 (get_jwt_user)

Recommendation: Extract a helper method _validate_and_decode_jwt() to encapsulate the common pattern:

@staticmethod
def _validate_and_decode_jwt(
    token: str,
    expected_type: str,
    session: AsyncSession,
    operation_name: str,
) -> tuple[dict[str, Any], User]:
    """Validate JWT token format, type, and user existence.

    Returns the decoded payload and user object. Raises HTTPException
    with appropriate metrics and logging for any validation failure.
    """
    # Token format validation
    # Token type validation with constant-time comparison
    # User ID validation
    # User lookup and banned/verified checks
    # Returns (payload, user)

Estimated Impact: Reduce ~200 lines of duplicated code to a single 50-line reusable function.

2. app/managers/auth.py:697-709 - Complex Nested Conditions

Issue: Nested ternary operators in get_jwt_user reduce readability.

if bool(user_data.banned) or not bool(user_data.verified):
    user_status = "banned" if user_data.banned else "unverified"
    reason = "banned_user" if user_data.banned else "unverified_user"
    ...

Recommendation: Use explicit if/else blocks for clarity, aligning with project preference for explicit code:

if bool(user_data.banned):
    increment_auth_failure("banned_user", "jwt")
    category_logger.warning(...)
    raise HTTPException(...)

if not bool(user_data.verified):
    increment_auth_failure("unverified_user", "jwt")
    category_logger.warning(...)
    raise HTTPException(...)

3. app/managers/auth.py - Inconsistent Boolean Conversions

Note

Related to SECURITY-REVIEW.md:

  • Section #6 (Timing Attack in Login) - Uses bool() wrapper on lines 206, 216

Issue: Redundant bool() wrappers on lines 243, 347, 521, 697 for database boolean fields (banned, verified).

Recommendation: Verify if these fields are already Boolean type in SQLAlchemy and remove unnecessary conversions. Also appears in user.py lines 227, 239.

Priority: MEDIUM

4. app/managers/auth.py:620-733 - Long Method

Note

Related to SECURITY-REVIEW.md:

  • Section #13 (Missing JWT Format Guards) - Same method, different issue
  • "Existing TODOs Worth Prioritizing" - Explicitly mentions refactoring complexity

Issue: get_jwt_user (~113 lines) has noqa: C901 comment indicating excessive complexity.

Function handles:

  • Token format validation
  • JWT decoding
  • Token type validation
  • User ID validation
  • User lookup
  • Banned/verified checks
  • Multiple exception handlers

Recommendation: Break down into smaller helper functions:

def _validate_token_format(credentials: HTTPAuthorizationCredentials)
def _validate_token_type(payload: dict, expected_type: str)
def _extract_and_validate_user_id(payload: dict) -> int
def _validate_user_status(user_data: User) -> None

5. app/managers/api_key.py:155-248 - Complex Method

Note

Related to SECURITY-REVIEW.md:

  • "Existing TODOs Worth Prioritizing" - Explicitly mentions: "Refactor ApiKeyAuth.__call__() complexity"

Issue: ApiKeyAuth.__call__ (~93 lines) has multiple noqa comments (C901, PLR0911, PLR0912) indicating high complexity.

Recommendation: Extract validation steps into separate methods:

def _validate_api_key_format(raw_key: str) -> str
def _validate_api_key_active(key: ApiKey) -> None
def _validate_user_status_for_api_key(user: User) -> None

This would improve testability and readability.

6. app/resources/auth.py:280-305 - Duplicated Context Building

Issue: The reset_password function builds similar error context dictionaries multiple times:

context = {
    "application": get_settings().api_title,
    "reset_token": code,
    "error": "All fields are required",
}

Recommendation: Extract a helper function:

def _build_reset_password_context(
    code: str, error: str | None = None
) -> dict[str, str | None]:
    return {
        "application": get_settings().api_title,
        "reset_token": code,
        "error": error,
    }

7. app/resources/api_key.py - Fragile Response Pattern

Issue: Multiple locations (lines 46-54, 67, 84, 106) use key.__dict__ for Pydantic model validation:

return ApiKeyResponse.model_validate(key.__dict__)

Recommendation: The __dict__ pattern is fragile. Consider adding a model_dump() method or using Pydantic's from_orm equivalent if available in the version being used.

Priority: LOW

8. app/managers/user.py:187-189 - Unnecessary Formatting

Note

Related to SECURITY-REVIEW.md:

  • Section #6 (Timing Attack in Login) - Same lines, part of timing attack fix

Note

Not fixing: The simplified version exceeds the 80-character line length limit (82 characters). The current formatting with parentheses is the correct approach given the project's line length constraint.

Issue: Extraneous parentheses and line break in simple ternary:

hash_to_verify = (
    str(user_do.password) if user_do else DUMMY_PASSWORD_HASH
)

Recommendation: Simplify to:

hash_to_verify = str(user_do.password) if user_do else DUMMY_PASSWORD_HASH

9. app/managers/email.py:39 - Path Construction

Note

✅ Done: Changed from string-based ".." to explicit Path.parent.parent for better clarity and maintainability.

Issue: Awkward ".." string in path construction:

TEMPLATE_FOLDER=Path(__file__).parent / ".." / "templates/email",

Recommendation: Use Path.parent.parent for clarity:

TEMPLATE_FOLDER=Path(__file__).parent.parent / "templates" / "email",

10. app/cache/decorators.py:52-58 - No-op Decorator

Issue: The no-op decorator when caching is disabled is correct but could be more explicit:

def noop_decorator(func: Callable[..., Any]) -> Callable[..., Any]:
    return func

Recommendation: Consider renaming to _identity or adding a comment explaining why this exists (for consistency with cached code paths).

Summary Statistics

Priority Files Issues Lines Affected (approx.)
HIGH 1 3 ~250
MEDIUM 4 4 ~150
LOW 3 3 ~20
Total 6 10 ~420

Key Recommendations

  1. Immediate Action (HIGH): Refactor JWT validation in auth.py - the duplication is significant and makes security changes error-prone.

  2. Short-term (MEDIUM): Break down overly complex functions (get_jwt_user, ApiKeyAuth.__call__) to improve testability and maintainability.

  3. Long-term (LOW): Minor style improvements for consistency.

The codebase follows the project's documented standards well (ES modules, explicit returns, proper error handling). The main issues stem from recent security additions that weren't refactored into reusable patterns.