fix: implement all 24 review findings (P0-P3)

Phase 1: Foundation & Quick Fixes
- P0-4: Fix test fixture scope (session -> function)
- P0-2: Add secret key validation with production check
- P1-2: Increase connection pool size (5->25, overflow 10->50)
- P2-4: Create constants module for centralized values
- P1-4: Extract generate_slug utility, remove duplication

Phase 2: Multi-Tenancy Enforcement
- P0-1: Make tenant_id required in all UserRepository methods
- Add system-level methods for authentication flows
- P1-7: Create tenant isolation integration tests

Phase 3: Redis & OAuth
- P0-3: Implement Redis caching layer for OAuth state
- P1-8: Refactor oauth_callback into helper functions
- P1-5: Create OAuth integration tests

Phase 4: Performance & Database
- P1-1: Change lazy='selectin' to lazy='raise' for relationships
- P2-2: Add composite indexes (tenant_email, oauth)
- P2-3: Change expires_at from String to DateTime
- P1-3: Add JWT token revocation with jti claim

Phase 5: RBAC & Permissions
- P1-9: Consolidate permission decorators with shared helper
- P2-6: Add superuser bypass audit logging
- P1-6: Create RBAC unit and integration tests

Phase 6: Code Quality & Polish
- P2-1: Fix Mypy type errors in middleware
- P2-5: Add password complexity validation
- P2-7: Remove outdated TODO
- P3-1: Fix user enumeration with generic messages
- P3-2: Improve CORS configuration
- P3-3: Use configurable api_docs_base_url for error URIs
- P3-4: Fix OAuth provider ClassVar inheritance

Author: robertguss

REVIEW_FINDINGS.md

@@ -0,0 +1,160 @@
+"""performance_and_security_fixes
+
+Revision ID: b2c3d4e5f6g7
+Revises: a1b2c3d4e5f6
+Create Date: 2025-12-01 00:02:00.000000
+
+This migration adds:
+- Composite indexes for performance (P2-2)
+- Changes expires_at from String to DateTime (P2-3)
+- Adds revoked_tokens table for JWT revocation (P1-3)
+"""
+
+from typing import Sequence, Union
+
+import sqlalchemy as sa
+from alembic import op
+
+
+# revision identifiers, used by Alembic.
+revision: str = "b2c3d4e5f6g7"
+down_revision: Union[str, None] = "a1b2c3d4e5f6"
+branch_labels: Union[str, Sequence[str], None] = None
+depends_on: Union[str, Sequence[str], None] = None
+
+
+def upgrade() -> None:
+    """Upgrade database schema."""
+    # P2-2: Add composite indexes for users table
+    op.create_index(
+        "ix_users_tenant_email",
+        "users",
+        ["tenant_id", "email"],
+        unique=True,
+    )
+    op.create_index(
+        "ix_users_oauth",
+        "users",
+        ["oauth_provider", "oauth_id"],
+        unique=False,
+    )
+
+    # P2-3: Change expires_at from String to DateTime
+    # First, add a new column
+    op.add_column(
+        "refresh_tokens",
+        sa.Column(
+            "expires_at_new",
+            sa.DateTime(timezone=True),
+            nullable=True,
+        ),
+    )
+
+    # Migrate data from string to datetime
+    op.execute(
+        """
+        UPDATE refresh_tokens
+        SET expires_at_new = expires_at::timestamptz
+        WHERE expires_at IS NOT NULL
+        """
+    )
+
+    # Drop old column and rename new one
+    op.drop_column("refresh_tokens", "expires_at")
+    op.alter_column(
+        "refresh_tokens",
+        "expires_at_new",
+        new_column_name="expires_at",
+        nullable=False,
+    )
+
+    # Add index on expires_at for efficient cleanup queries
+    op.create_index(
+        "ix_refresh_tokens_expires_at",
+        "refresh_tokens",
+        ["expires_at"],
+    )
+
+    # P1-3: Create revoked_tokens table for JWT revocation
+    op.create_table(
+        "revoked_tokens",
+        sa.Column("jti", sa.String(length=64), nullable=False),
+        sa.Column(
+            "expires_at",
+            sa.DateTime(timezone=True),
+            nullable=False,
+        ),
+        sa.Column("id", sa.Uuid(), nullable=False),
+        sa.Column(
+            "created_at",
+            sa.DateTime(timezone=True),
+            server_default=sa.text("now()"),
+            nullable=False,
+        ),
+        sa.Column(
+            "updated_at",
+            sa.DateTime(timezone=True),
+            server_default=sa.text("now()"),
+            nullable=False,
+        ),
+        sa.PrimaryKeyConstraint("id"),
+    )
+    op.create_index(
+        "ix_revoked_tokens_id",
+        "revoked_tokens",
+        ["id"],
+        unique=False,
+    )
+    op.create_index(
+        "ix_revoked_tokens_jti",
+        "revoked_tokens",
+        ["jti"],
+        unique=True,
+    )
+    op.create_index(
+        "ix_revoked_tokens_expires_at",
+        "revoked_tokens",
+        ["expires_at"],
+    )
+
+
+def downgrade() -> None:
+    """Downgrade database schema."""
+    # Drop revoked_tokens table
+    op.drop_index("ix_revoked_tokens_expires_at", table_name="revoked_tokens")
+    op.drop_index("ix_revoked_tokens_jti", table_name="revoked_tokens")
+    op.drop_index("ix_revoked_tokens_id", table_name="revoked_tokens")
+    op.drop_table("revoked_tokens")
+
+    # Revert expires_at back to String
+    op.drop_index("ix_refresh_tokens_expires_at", table_name="refresh_tokens")
+
+    op.add_column(
+        "refresh_tokens",
+        sa.Column(
+            "expires_at_old",
+            sa.String(),
+            nullable=True,
+        ),
+    )
+
+    op.execute(
+        """
+        UPDATE refresh_tokens
+        SET expires_at_old = expires_at::text
+        WHERE expires_at IS NOT NULL
+        """
+    )
+
+    op.drop_column("refresh_tokens", "expires_at")
+    op.alter_column(
+        "refresh_tokens",
+        "expires_at_old",
+        new_column_name="expires_at",
+        nullable=False,
+    )
+
+    # Drop composite indexes from users table
+    op.drop_index("ix_users_oauth", table_name="users")
+    op.drop_index("ix_users_tenant_email", table_name="users")
+

alembic/versions/2025_12_01_0002-performance_and_security_fixes.py

@@ -1,6 +1,5 @@
 """Shared API dependencies."""
 
-from collections.abc import AsyncGenerator
 from typing import Annotated
 
 from fastapi import Depends
@@ -13,14 +12,6 @@
 DBSession = Annotated[AsyncSession, Depends(get_db)]
 
 
-async def get_current_tenant_id() -> AsyncGenerator[str | None, None]:
-    """Get current tenant ID from request context.
-
-    This is a placeholder that will be implemented in Phase 2
-    with proper authentication middleware.
-    """
-    # TODO: Extract tenant_id from authenticated user
-    yield None
-
-
-CurrentTenantId = Annotated[str | None, Depends(get_current_tenant_id)]
+# Note: Tenant context is now handled by TenantContextMiddleware.
+# For authenticated routes, use TenantId from app.core.auth.dependencies
+# which extracts tenant_id from the JWT token.

src/app/api/dependencies.py

@@ -2,9 +2,11 @@
 
 from functools import lru_cache
 
-from pydantic import PostgresDsn, RedisDsn, computed_field
+from pydantic import PostgresDsn, RedisDsn, computed_field, field_validator
 from pydantic_settings import BaseSettings, SettingsConfigDict
 
+from app.core.constants import DEFAULT_INSECURE_SECRET, MIN_SECRET_KEY_LENGTH
+
 
 class Settings(BaseSettings):
     """Application settings loaded from environment variables."""
@@ -20,16 +22,52 @@ class Settings(BaseSettings):
     app_name: str = "Agency Standard"
     debug: bool = False
     environment: str = "development"  # development, staging, production
-    secret_key: str = "change-me-in-production"
+    secret_key: str = DEFAULT_INSECURE_SECRET
 
     # Database
     database_url: PostgresDsn = PostgresDsn(
         "postgresql://postgres:postgres@localhost:5432/agency_standard"
     )
-    database_pool_size: int = 5
-    database_max_overflow: int = 10
+    database_pool_size: int = 25
+    database_max_overflow: int = 50
     database_echo: bool = False
 
+    # CORS
+    cors_origins: list[str] = []
+
+    # API Documentation
+    api_docs_base_url: str = "https://api.example.com"
+
+    @field_validator("secret_key")
+    @classmethod
+    def validate_secret_key(cls, v: str, info) -> str:
+        """Validate that secret_key is secure in production.
+
+        Args:
+            v: The secret key value
+            info: Validation info containing other field values
+
+        Returns:
+            The validated secret key
+
+        Raises:
+            ValueError: If secret key is insecure in production
+        """
+        # Get environment from the data being validated
+        # Note: In pydantic v2, we need to handle this differently
+        # The validation happens before all fields are set
+        # So we check the value itself rather than environment
+        if v == DEFAULT_INSECURE_SECRET:
+            # We'll do a runtime check in is_production instead
+            # to allow development mode to use default
+            pass
+        elif len(v) < MIN_SECRET_KEY_LENGTH:
+            raise ValueError(
+                f"SECRET_KEY must be at least {MIN_SECRET_KEY_LENGTH} characters. "
+                "Generate one with: python -c 'import secrets; print(secrets.token_urlsafe(32))'"
+            )
+        return v
+
     # Redis
     redis_url: RedisDsn = RedisDsn("redis://localhost:6379")
 
@@ -63,8 +101,18 @@ def async_database_url(self) -> str:
     @computed_field  # type: ignore[prop-decorator]
     @property
     def is_production(self) -> bool:
-        """Check if running in production environment."""
-        return self.environment == "production"
+        """Check if running in production environment.
+
+        Raises:
+            ValueError: If using insecure secret key in production
+        """
+        is_prod = self.environment == "production"
+        if is_prod and self.secret_key == DEFAULT_INSECURE_SECRET:
+            raise ValueError(
+                "SECRET_KEY must be set to a secure value in production. "
+                "Generate one with: python -c 'import secrets; print(secrets.token_urlsafe(32))'"
+            )
+        return is_prod
 
     @computed_field  # type: ignore[prop-decorator]
     @property

src/app/config.py

@@ -4,6 +4,7 @@
 - Password hashing with bcrypt
 - JWT token creation and verification
 - Token hashing for storage
+- Token revocation
 """
 
 import hashlib
@@ -17,6 +18,7 @@
 
 from app.config import settings
 from app.core.auth.schemas import TokenData
+from app.core.constants import ACCESS_TOKEN_JTI_LENGTH
 
 
 # Password hashing context using bcrypt
@@ -93,6 +95,7 @@ def create_access_token(
         "exp": expire,
         "type": "access",
         "iat": datetime.now(UTC),
+        "jti": secrets.token_urlsafe(ACCESS_TOKEN_JTI_LENGTH),  # Unique token ID
     }
 
     if additional_claims:
@@ -162,6 +165,7 @@ def decode_token(token: str) -> TokenData | None:
         tenant_id = payload.get("tenant_id")
         exp = payload.get("exp")
         token_type = payload.get("type", "access")
+        jti = payload.get("jti")
 
         if not user_id or not tenant_id:
             return None
@@ -171,6 +175,7 @@ def decode_token(token: str) -> TokenData | None:
             tenant_id=UUID(tenant_id),
             exp=datetime.fromtimestamp(exp, tz=UTC),
             type=token_type,
+            jti=jti,
         )
 
     except (JWTError, ValueError):

src/app/core/auth/backend.py

@@ -77,7 +77,8 @@ async def get_current_user(
     from app.modules.users.repos import UserRepository  # noqa: PLC0415
 
     repo = UserRepository(db)
-    user = await repo.get_by_id(token_data.user_id)
+    # Use tenant-scoped lookup for security
+    user = await repo.get_by_id(token_data.user_id, token_data.tenant_id)
 
     if not user:
         raise UnauthorizedError(
@@ -180,7 +181,8 @@ async def get_optional_user(
     from app.modules.users.repos import UserRepository  # noqa: PLC0415
 
     repo = UserRepository(db)
-    user = await repo.get_by_id(token_data.user_id)
+    # Use tenant-scoped lookup for security
+    user = await repo.get_by_id(token_data.user_id, token_data.tenant_id)
 
     if not user or not user.is_active:
         return None

src/app/core/auth/dependencies.py

@@ -6,16 +6,19 @@
 """
 
 import uuid
-from collections.abc import Callable
-from typing import Any
+from typing import TYPE_CHECKING
 
 import structlog
 from fastapi import Request, Response
-from starlette.middleware.base import BaseHTTPMiddleware
+from starlette.middleware.base import BaseHTTPMiddleware, RequestResponseEndpoint
 
 from app.core.auth.backend import decode_token
 
 
+if TYPE_CHECKING:
+    from starlette.types import ASGIApp
+
+
 logger = structlog.get_logger()
 
 
@@ -31,7 +34,7 @@ class TenantContextMiddleware(BaseHTTPMiddleware):
 
     def __init__(
         self,
-        app: Any,
+        app: "ASGIApp",
         exclude_paths: list[str] | None = None,
     ) -> None:
         super().__init__(app)
@@ -49,7 +52,7 @@ def __init__(
     async def dispatch(
         self,
         request: Request,
-        call_next: Callable[[Request], Any],
+        call_next: RequestResponseEndpoint,
     ) -> Response:
         """Process the request and inject tenant context.
 
@@ -95,7 +98,7 @@ class RequestIdMiddleware(BaseHTTPMiddleware):
     async def dispatch(
         self,
         request: Request,
-        call_next: Callable[[Request], Any],
+        call_next: RequestResponseEndpoint,
     ) -> Response:
         """Process the request and add request ID.