fix: address multiple code quality improvements across backend and frontend
Backend fixes: - Fix JWT sub claim: encode as str(user.id), decode with int() cast (python-jose requirement) - Replace all deprecated datetime.utcnow() with datetime.now(timezone.utc) - Replace deprecated FastAPI @app.on_event() with modern lifespan context manager - Replace deprecated Pydantic class Config with model_config = ConfigDict(...) - Replace deprecated Pydantic .dict() with .model_dump() - Fix overly broad except (GmailInjectionError, Exception) → except Exception - Remove unused GmailInjectionError import - Fix TokenPayload schema sub field type from int to str Frontend: - Create frontend/src/lib/api.ts — API client module with auth, user, mail accounts, processing runs APIs - Add !frontend/src/lib/ to .gitignore negation Tests: - Add 3 new JWT tests (sub string encoding, access token type, refresh token type) - Update test_token_payload_schema for string sub claim - All 128 tests pass Co-authored-by: christianlouis <361235+christianlouis@users.noreply.github.com> Agent-Logs-Url: https://github.com/christianlouis/pop_puller_to_gmail/sessions/e0b13eb0-8de7-4f02-81e4-e202cbba4608
This commit is contained in:
@@ -6,7 +6,7 @@ from fastapi import APIRouter, Depends, HTTPException, status
|
||||
from fastapi.security import OAuth2PasswordRequestForm
|
||||
from sqlalchemy.ext.asyncio import AsyncSession
|
||||
from sqlalchemy import select
|
||||
from datetime import datetime
|
||||
from datetime import datetime, timezone
|
||||
import logging
|
||||
|
||||
from app.core.database import get_db
|
||||
@@ -86,7 +86,7 @@ async def login(
|
||||
)
|
||||
|
||||
# Update last login
|
||||
user.last_login_at = datetime.utcnow() # type: ignore[assignment]
|
||||
user.last_login_at = datetime.now(timezone.utc) # type: ignore[assignment]
|
||||
await db.commit()
|
||||
|
||||
# Create tokens
|
||||
@@ -133,7 +133,7 @@ async def google_oauth(
|
||||
user.oauth_provider = "google" # type: ignore[assignment]
|
||||
|
||||
# Update last login
|
||||
user.last_login_at = datetime.utcnow() # type: ignore[assignment]
|
||||
user.last_login_at = datetime.now(timezone.utc) # type: ignore[assignment]
|
||||
|
||||
logger.info(f"Existing user logged in with Google: {user.email}")
|
||||
else:
|
||||
@@ -145,7 +145,7 @@ async def google_oauth(
|
||||
oauth_provider="google",
|
||||
subscription_tier=SubscriptionTier.FREE,
|
||||
is_active=True,
|
||||
last_login_at=datetime.utcnow(),
|
||||
last_login_at=datetime.now(timezone.utc),
|
||||
)
|
||||
db.add(user)
|
||||
|
||||
|
||||
@@ -143,7 +143,7 @@ async def update_mail_account(
|
||||
)
|
||||
|
||||
# Update fields
|
||||
update_data = account_update.dict(exclude_unset=True)
|
||||
update_data = account_update.model_dump(exclude_unset=True)
|
||||
|
||||
if "password" in update_data:
|
||||
update_data["encrypted_password"] = encrypt_credential(
|
||||
|
||||
@@ -1,6 +1,6 @@
|
||||
"""Provider presets and Gmail credential management endpoints"""
|
||||
|
||||
from datetime import datetime
|
||||
from datetime import datetime, timezone
|
||||
from typing import List
|
||||
from fastapi import APIRouter, Depends, HTTPException, status
|
||||
from sqlalchemy.ext.asyncio import AsyncSession
|
||||
@@ -205,7 +205,7 @@ async def save_gmail_credential(
|
||||
existing.encrypted_access_token = encrypted_access # type: ignore[assignment]
|
||||
existing.encrypted_refresh_token = encrypted_refresh # type: ignore[assignment]
|
||||
existing.is_valid = True # type: ignore[assignment]
|
||||
existing.last_verified_at = datetime.utcnow() # type: ignore[assignment]
|
||||
existing.last_verified_at = datetime.now(timezone.utc) # type: ignore[assignment]
|
||||
await db.commit()
|
||||
await db.refresh(existing)
|
||||
return existing
|
||||
@@ -217,7 +217,7 @@ async def save_gmail_credential(
|
||||
encrypted_access_token=encrypted_access,
|
||||
encrypted_refresh_token=encrypted_refresh,
|
||||
is_valid=True,
|
||||
last_verified_at=datetime.utcnow(),
|
||||
last_verified_at=datetime.now(timezone.utc),
|
||||
)
|
||||
db.add(credential)
|
||||
await db.commit()
|
||||
|
||||
@@ -58,9 +58,18 @@ async def get_current_user(
|
||||
headers={"WWW-Authenticate": "Bearer"},
|
||||
)
|
||||
|
||||
# Get user ID from token
|
||||
user_id: Optional[int] = payload.get("sub")
|
||||
if user_id is None:
|
||||
# Get user ID from token (sub claim is a string per JWT spec)
|
||||
sub: Optional[str] = payload.get("sub")
|
||||
if sub is None:
|
||||
raise HTTPException(
|
||||
status_code=status.HTTP_401_UNAUTHORIZED,
|
||||
detail="Invalid token payload",
|
||||
headers={"WWW-Authenticate": "Bearer"},
|
||||
)
|
||||
|
||||
try:
|
||||
user_id = int(sub)
|
||||
except (ValueError, TypeError):
|
||||
raise HTTPException(
|
||||
status_code=status.HTTP_401_UNAUTHORIZED,
|
||||
detail="Invalid token payload",
|
||||
|
||||
@@ -4,7 +4,7 @@ Security utilities for encryption, hashing, and token generation.
|
||||
|
||||
import hashlib
|
||||
import secrets
|
||||
from datetime import datetime, timedelta
|
||||
from datetime import datetime, timedelta, timezone
|
||||
from typing import Optional, Dict, Any
|
||||
import bcrypt
|
||||
from jose import JWTError, jwt
|
||||
@@ -35,9 +35,9 @@ def create_access_token(
|
||||
to_encode = data.copy()
|
||||
|
||||
if expires_delta:
|
||||
expire = datetime.utcnow() + expires_delta
|
||||
expire = datetime.now(timezone.utc) + expires_delta
|
||||
else:
|
||||
expire = datetime.utcnow() + timedelta(
|
||||
expire = datetime.now(timezone.utc) + timedelta(
|
||||
minutes=settings.ACCESS_TOKEN_EXPIRE_MINUTES
|
||||
)
|
||||
|
||||
@@ -51,7 +51,9 @@ def create_access_token(
|
||||
def create_refresh_token(data: Dict[str, Any]) -> str:
|
||||
"""Create JWT refresh token"""
|
||||
to_encode = data.copy()
|
||||
expire = datetime.utcnow() + timedelta(days=settings.REFRESH_TOKEN_EXPIRE_DAYS)
|
||||
expire = datetime.now(timezone.utc) + timedelta(
|
||||
days=settings.REFRESH_TOKEN_EXPIRE_DAYS
|
||||
)
|
||||
to_encode.update({"exp": expire, "type": "refresh"})
|
||||
encoded_jwt = jwt.encode(
|
||||
to_encode, settings.SECRET_KEY, algorithm=settings.ALGORITHM
|
||||
|
||||
+15
-12
@@ -2,6 +2,8 @@
|
||||
Main FastAPI application.
|
||||
"""
|
||||
|
||||
from contextlib import asynccontextmanager
|
||||
from collections.abc import AsyncIterator
|
||||
from fastapi import FastAPI
|
||||
from fastapi.middleware.cors import CORSMiddleware
|
||||
import logging
|
||||
@@ -19,6 +21,18 @@ logging.basicConfig(
|
||||
logger = logging.getLogger(__name__)
|
||||
|
||||
|
||||
@asynccontextmanager
|
||||
async def lifespan(app: FastAPI) -> AsyncIterator[None]:
|
||||
"""Application lifespan handler for startup and shutdown events."""
|
||||
# Startup
|
||||
logger.info(f"Starting {settings.APP_NAME} v{settings.APP_VERSION}")
|
||||
logger.info(f"Debug mode: {settings.DEBUG}")
|
||||
logger.info("API documentation: /api/docs")
|
||||
yield
|
||||
# Shutdown
|
||||
logger.info("Shutting down application")
|
||||
|
||||
|
||||
def create_application() -> FastAPI:
|
||||
"""Create and configure FastAPI application"""
|
||||
|
||||
@@ -29,6 +43,7 @@ def create_application() -> FastAPI:
|
||||
docs_url="/api/docs",
|
||||
redoc_url="/api/redoc",
|
||||
openapi_url="/api/openapi.json",
|
||||
lifespan=lifespan,
|
||||
)
|
||||
|
||||
# Security middleware (add before CORS)
|
||||
@@ -61,18 +76,6 @@ def create_application() -> FastAPI:
|
||||
"""Health check endpoint for container orchestration"""
|
||||
return {"status": "healthy"}
|
||||
|
||||
@app.on_event("startup")
|
||||
async def startup_event():
|
||||
"""Run on application startup"""
|
||||
logger.info(f"Starting {settings.APP_NAME} v{settings.APP_VERSION}")
|
||||
logger.info(f"Debug mode: {settings.DEBUG}")
|
||||
logger.info("API documentation: /api/docs")
|
||||
|
||||
@app.on_event("shutdown")
|
||||
async def shutdown_event():
|
||||
"""Run on application shutdown"""
|
||||
logger.info("Shutting down application")
|
||||
|
||||
return app
|
||||
|
||||
|
||||
|
||||
@@ -4,7 +4,7 @@ Pydantic schemas for API request/response validation.
|
||||
|
||||
from datetime import datetime
|
||||
from typing import Optional, Dict, Any, List
|
||||
from pydantic import BaseModel, EmailStr, Field
|
||||
from pydantic import BaseModel, ConfigDict, EmailStr, Field
|
||||
from enum import Enum
|
||||
|
||||
|
||||
@@ -65,8 +65,7 @@ class UserResponse(UserBase):
|
||||
subscription_status: str
|
||||
created_at: datetime
|
||||
|
||||
class Config:
|
||||
from_attributes = True
|
||||
model_config = ConfigDict(from_attributes=True)
|
||||
|
||||
|
||||
class UserDetailResponse(UserResponse):
|
||||
@@ -76,8 +75,7 @@ class UserDetailResponse(UserResponse):
|
||||
subscription_expires_at: Optional[datetime] = None
|
||||
last_login_at: Optional[datetime] = None
|
||||
|
||||
class Config:
|
||||
from_attributes = True
|
||||
model_config = ConfigDict(from_attributes=True)
|
||||
|
||||
|
||||
# Authentication Schemas
|
||||
@@ -88,7 +86,7 @@ class Token(BaseModel):
|
||||
|
||||
|
||||
class TokenPayload(BaseModel):
|
||||
sub: Optional[int] = None
|
||||
sub: Optional[str] = None
|
||||
exp: Optional[int] = None
|
||||
type: Optional[str] = None
|
||||
|
||||
@@ -151,8 +149,7 @@ class MailAccountResponse(MailAccountBase):
|
||||
password: str = Field(exclude=True, default="")
|
||||
username: str = Field(exclude=True, default="")
|
||||
|
||||
class Config:
|
||||
from_attributes = True
|
||||
model_config = ConfigDict(from_attributes=True)
|
||||
|
||||
|
||||
class MailAccountTestRequest(BaseModel):
|
||||
@@ -197,8 +194,7 @@ class ProcessingRunResponse(BaseModel):
|
||||
status: str
|
||||
error_message: Optional[str] = None
|
||||
|
||||
class Config:
|
||||
from_attributes = True
|
||||
model_config = ConfigDict(from_attributes=True)
|
||||
|
||||
|
||||
# Processing Log Schemas
|
||||
@@ -211,8 +207,7 @@ class ProcessingLogResponse(BaseModel):
|
||||
email_from: Optional[str] = None
|
||||
success: bool
|
||||
|
||||
class Config:
|
||||
from_attributes = True
|
||||
model_config = ConfigDict(from_attributes=True)
|
||||
|
||||
|
||||
# Notification Config Schemas
|
||||
@@ -243,8 +238,7 @@ class NotificationConfigResponse(NotificationConfigBase):
|
||||
created_at: datetime
|
||||
updated_at: datetime
|
||||
|
||||
class Config:
|
||||
from_attributes = True
|
||||
model_config = ConfigDict(from_attributes=True)
|
||||
|
||||
|
||||
# Subscription Schemas
|
||||
@@ -262,8 +256,7 @@ class SubscriptionPlanResponse(BaseModel):
|
||||
features: Optional[Dict[str, Any]] = None
|
||||
is_active: bool
|
||||
|
||||
class Config:
|
||||
from_attributes = True
|
||||
model_config = ConfigDict(from_attributes=True)
|
||||
|
||||
|
||||
class SubscriptionCheckoutRequest(BaseModel):
|
||||
@@ -311,8 +304,7 @@ class MailServerPresetResponse(BaseModel):
|
||||
configs: Dict[str, Any]
|
||||
is_verified: bool
|
||||
|
||||
class Config:
|
||||
from_attributes = True
|
||||
model_config = ConfigDict(from_attributes=True)
|
||||
|
||||
|
||||
# Gmail Credential Schemas
|
||||
@@ -331,8 +323,7 @@ class GmailCredentialResponse(BaseModel):
|
||||
created_at: datetime
|
||||
updated_at: datetime
|
||||
|
||||
class Config:
|
||||
from_attributes = True
|
||||
model_config = ConfigDict(from_attributes=True)
|
||||
|
||||
|
||||
# Provider Wizard Schemas
|
||||
|
||||
@@ -121,8 +121,8 @@ class OAuthService:
|
||||
Returns:
|
||||
Dict with access_token, refresh_token, and token_type
|
||||
"""
|
||||
access_token = create_access_token(data={"sub": user.id})
|
||||
refresh_token = create_refresh_token(data={"sub": user.id})
|
||||
access_token = create_access_token(data={"sub": str(user.id)})
|
||||
refresh_token = create_refresh_token(data={"sub": str(user.id)})
|
||||
|
||||
return {
|
||||
"access_token": access_token,
|
||||
|
||||
@@ -4,7 +4,7 @@ Celery tasks for background email processing.
|
||||
|
||||
import asyncio
|
||||
import os
|
||||
from datetime import datetime, timedelta
|
||||
from datetime import datetime, timedelta, timezone
|
||||
from celery import Task
|
||||
import logging
|
||||
|
||||
@@ -20,7 +20,7 @@ from app.models.database_models import (
|
||||
GmailCredential,
|
||||
)
|
||||
from app.services.mail_processor import MailProcessor
|
||||
from app.services.gmail_service import GmailService, GmailInjectionError
|
||||
from app.services.gmail_service import GmailService
|
||||
from app.core.config import settings
|
||||
from sqlalchemy import select, and_
|
||||
|
||||
@@ -59,7 +59,7 @@ async def process_mail_account(account_id: int):
|
||||
# Create processing run
|
||||
run = ProcessingRun(
|
||||
mail_account_id=account.id,
|
||||
started_at=datetime.utcnow(),
|
||||
started_at=datetime.now(timezone.utc),
|
||||
status="running",
|
||||
)
|
||||
db.add(run)
|
||||
@@ -156,28 +156,28 @@ async def process_mail_account(account_id: int):
|
||||
else:
|
||||
emails_failed += 1
|
||||
|
||||
except (GmailInjectionError, Exception) as e:
|
||||
except Exception as e:
|
||||
logger.error(f"Error delivering email: {e}")
|
||||
emails_failed += 1
|
||||
|
||||
# Update run
|
||||
run.emails_forwarded = emails_forwarded # type: ignore[assignment]
|
||||
run.emails_failed = emails_failed # type: ignore[assignment]
|
||||
run.completed_at = datetime.utcnow() # type: ignore[assignment]
|
||||
run.completed_at = datetime.now(timezone.utc) # type: ignore[assignment]
|
||||
run.duration_seconds = (run.completed_at - run.started_at).total_seconds()
|
||||
run.status = "completed" if emails_failed == 0 else "partial_failure" # type: ignore[assignment]
|
||||
|
||||
# Update account
|
||||
account.total_emails_processed += emails_forwarded # type: ignore[assignment]
|
||||
account.total_emails_failed += emails_failed # type: ignore[assignment]
|
||||
account.last_check_at = datetime.utcnow() # type: ignore[assignment]
|
||||
account.last_check_at = datetime.now(timezone.utc) # type: ignore[assignment]
|
||||
|
||||
if emails_failed == 0:
|
||||
account.last_successful_check_at = datetime.utcnow() # type: ignore[assignment]
|
||||
account.last_successful_check_at = datetime.now(timezone.utc) # type: ignore[assignment]
|
||||
account.status = AccountStatus.ACTIVE # type: ignore[assignment]
|
||||
else:
|
||||
account.status = AccountStatus.ERROR # type: ignore[assignment]
|
||||
account.last_error_at = datetime.utcnow() # type: ignore[assignment]
|
||||
account.last_error_at = datetime.now(timezone.utc) # type: ignore[assignment]
|
||||
account.last_error_message = f"{emails_failed} emails failed to forward" # type: ignore[assignment]
|
||||
|
||||
await db.commit()
|
||||
@@ -194,7 +194,7 @@ async def process_mail_account(account_id: int):
|
||||
if "run" in locals():
|
||||
run.status = "failed" # type: ignore[assignment]
|
||||
run.error_message = str(e) # type: ignore[assignment]
|
||||
run.completed_at = datetime.utcnow() # type: ignore[assignment]
|
||||
run.completed_at = datetime.now(timezone.utc) # type: ignore[assignment]
|
||||
run.duration_seconds = (
|
||||
run.completed_at - run.started_at
|
||||
).total_seconds()
|
||||
@@ -202,7 +202,7 @@ async def process_mail_account(account_id: int):
|
||||
# Update account error status
|
||||
if "account" in locals() and account is not None:
|
||||
account.status = AccountStatus.ERROR # type: ignore[assignment]
|
||||
account.last_error_at = datetime.utcnow() # type: ignore[assignment]
|
||||
account.last_error_at = datetime.now(timezone.utc) # type: ignore[assignment]
|
||||
account.last_error_message = str(e) # type: ignore[assignment]
|
||||
|
||||
await db.commit()
|
||||
@@ -235,7 +235,9 @@ async def process_all_enabled_accounts():
|
||||
for account in accounts:
|
||||
# Check if it's time to check this account
|
||||
if account.last_check_at:
|
||||
time_since_last_check = datetime.utcnow() - account.last_check_at
|
||||
time_since_last_check = (
|
||||
datetime.now(timezone.utc) - account.last_check_at
|
||||
)
|
||||
if time_since_last_check.total_seconds() < (
|
||||
account.check_interval_minutes * 60
|
||||
):
|
||||
@@ -259,7 +261,7 @@ async def cleanup_old_logs(days_to_keep: int = 30):
|
||||
"""
|
||||
async with async_session_maker() as db:
|
||||
try:
|
||||
cutoff_date = datetime.utcnow() - timedelta(days=days_to_keep)
|
||||
cutoff_date = datetime.now(timezone.utc) - timedelta(days=days_to_keep)
|
||||
|
||||
# Delete old processing runs
|
||||
result = await db.execute(
|
||||
|
||||
@@ -109,8 +109,8 @@ class TestTokenSchemas:
|
||||
|
||||
def test_token_payload_schema(self):
|
||||
"""Test TokenPayload schema"""
|
||||
payload = TokenPayload(sub=42, type="access")
|
||||
assert payload.sub == 42
|
||||
payload = TokenPayload(sub="42", type="access")
|
||||
assert payload.sub == "42"
|
||||
assert payload.type == "access"
|
||||
|
||||
def test_google_auth_request(self):
|
||||
|
||||
@@ -51,6 +51,38 @@ class TestJWT:
|
||||
assert len(token) > 50
|
||||
assert token.count(".") == 2 # JWT has 3 parts
|
||||
|
||||
def test_access_token_sub_claim_is_string(self):
|
||||
"""Test that sub claim must be passed as a string (python-jose requirement)"""
|
||||
from app.core.security import decode_token
|
||||
|
||||
# sub should be a string (e.g. str(user.id)), not an integer
|
||||
token = create_access_token(data={"sub": "42"})
|
||||
payload = decode_token(token)
|
||||
|
||||
assert payload is not None
|
||||
assert payload["sub"] == "42"
|
||||
assert isinstance(payload["sub"], str)
|
||||
|
||||
def test_access_token_contains_type_claim(self):
|
||||
"""Test that access token includes type=access claim"""
|
||||
from app.core.security import decode_token
|
||||
|
||||
token = create_access_token(data={"sub": "1"})
|
||||
payload = decode_token(token)
|
||||
|
||||
assert payload is not None
|
||||
assert payload["type"] == "access"
|
||||
|
||||
def test_refresh_token_contains_type_claim(self):
|
||||
"""Test that refresh token includes type=refresh claim"""
|
||||
from app.core.security import create_refresh_token, decode_token
|
||||
|
||||
token = create_refresh_token(data={"sub": "1"})
|
||||
payload = decode_token(token)
|
||||
|
||||
assert payload is not None
|
||||
assert payload["type"] == "refresh"
|
||||
|
||||
|
||||
class TestEncryption:
|
||||
"""Test credential encryption/decryption"""
|
||||
|
||||
Reference in New Issue
Block a user