diff --git a/backend/.env.example b/backend/.env.example index cc0bd94..7b3798e 100644 --- a/backend/.env.example +++ b/backend/.env.example @@ -4,10 +4,13 @@ DATABASE_URL=postgresql+asyncpg://agp_user:agp_password@localhost:5432/agp_db # Redis REDIS_URL=redis://localhost:6379/0 -# JWT -SECRET_KEY=change-me-to-a-secure-random-string -ALGORITHM=HS256 -ACCESS_TOKEN_EXPIRE_MINUTES=30 +# JWT / Auth +ALGORITHM=RS256 +ACCESS_TOKEN_EXPIRE_MINUTES=60 +REFRESH_TOKEN_EXPIRE_DAYS=30 +# Directory where the RSA keypair is loaded/generated at runtime. +# Defaults to ~/.agp/keys (outside the repo). Keys are never committed to git. +RSA_KEY_DIR=~/.agp/keys # App APP_NAME=AGP Backend diff --git a/backend/alembic/versions/0003_add_refresh_tokens_table.py b/backend/alembic/versions/0003_add_refresh_tokens_table.py new file mode 100644 index 0000000..e390dd6 --- /dev/null +++ b/backend/alembic/versions/0003_add_refresh_tokens_table.py @@ -0,0 +1,36 @@ +"""add refresh_tokens table + +Revision ID: 0003_add_refresh_tokens_table +Revises: 0002_add_date_of_birth_to_users +Create Date: 2025-07-25 00:00:00.000000 + +""" +from typing import Sequence, Union + +from alembic import op +import sqlalchemy as sa + +# revision identifiers, used by Alembic. +revision: str = "0003_add_refresh_tokens_table" +down_revision: Union[str, None] = "0002_add_date_of_birth_to_users" +branch_labels: Union[str, Sequence[str], None] = None +depends_on: Union[str, Sequence[str], None] = None + + +def upgrade() -> None: + """Create the refresh_tokens table.""" + op.create_table( + "refresh_tokens", + sa.Column("id", sa.String(36), primary_key=True), + sa.Column("user_id", sa.Integer(), sa.ForeignKey("users.id", ondelete="CASCADE"), nullable=False), + sa.Column("token", sa.String(512), unique=True, index=True, nullable=False), + sa.Column("expires_at", sa.DateTime(timezone=True), nullable=False), + sa.Column("is_active", sa.Boolean(), server_default=sa.text("true"), nullable=False), + sa.Column("created_at", sa.DateTime(timezone=True), server_default=sa.func.now(), nullable=False), + sa.Column("updated_at", sa.DateTime(timezone=True), server_default=sa.func.now(), nullable=False), + ) + + +def downgrade() -> None: + """Drop the refresh_tokens table.""" + op.drop_table("refresh_tokens") diff --git a/backend/app/config.py b/backend/app/config.py index 19d89e4..a899e6e 100644 --- a/backend/app/config.py +++ b/backend/app/config.py @@ -1,10 +1,56 @@ """Application configuration via pydantic-settings.""" from functools import lru_cache +from pathlib import Path +from cryptography.hazmat.primitives.asymmetric.rsa import ( + RSAPrivateKey, + RSAPublicKey, + generate_private_key, +) +from cryptography.hazmat.primitives.serialization import ( + Encoding, + NoEncryption, + PrivateFormat, + PublicFormat, + load_pem_private_key, + load_pem_public_key, +) from pydantic_settings import BaseSettings, SettingsConfigDict +def _load_or_generate_rsa_keys(key_dir: Path) -> tuple[bytes, bytes]: + """Load existing RSA keys or generate a new pair. + + Keys are stored as PEM files in *key_dir*. The directory is created + if it does not exist. Files are named ``private.pem`` and + ``public.pem`` and are **not** committed to git. + """ + private_path = key_dir / "private.pem" + public_path = key_dir / "public.pem" + + if private_path.exists() and public_path.exists(): + private_pem = private_path.read_bytes() + public_pem = public_path.read_bytes() + else: + # Generate a new 2048-bit RSA key pair + private_key = generate_private_key(public_exponent=65537, key_size=2048) + private_pem = private_key.private_bytes( + Encoding.PEM, + PrivateFormat.PKCS8, + NoEncryption(), + ) + public_pem = private_key.public_key().public_bytes( + Encoding.PEM, + PublicFormat.SubjectPublicKeyInfo, + ) + key_dir.mkdir(parents=True, exist_ok=True) + private_path.write_bytes(private_pem) + public_path.write_bytes(public_pem) + + return private_pem, public_pem + + class Settings(BaseSettings): # App APP_NAME: str = "AGP Backend" @@ -18,11 +64,13 @@ class Settings(BaseSettings): REDIS_URL: str = "redis://localhost:6379/0" # JWT / Auth - # NOTE: production must override SECRET_KEY with a secure random value - # (e.g. `python -c "import secrets; print(secrets.token_urlsafe(32))"`) - SECRET_KEY: str = "change-me-to-a-secure-random-string" - ALGORITHM: str = "HS256" - ACCESS_TOKEN_EXPIRE_MINUTES: int = 30 + ALGORITHM: str = "RS256" + ACCESS_TOKEN_EXPIRE_MINUTES: int = 60 + REFRESH_TOKEN_EXPIRE_DAYS: int = 30 + + # RS256 key storage. Defaults to a directory outside the repo so the + # generated keypair is never accidentally committed. + RSA_KEY_DIR: str = str(Path.home() / ".agp" / "keys") # CORS CORS_ORIGINS: str = "http://localhost:8080,http://localhost:3000" @@ -42,6 +90,32 @@ class Settings(BaseSettings): def cors_origins_list(self) -> list[str]: return [origin.strip() for origin in self.CORS_ORIGINS.split(",")] + @property + def rsa_key_dir(self) -> Path: + return Path(self.RSA_KEY_DIR) + + @property + def private_pem(self) -> bytes: + """Return the private key PEM bytes (loads/generates on first call).""" + private_pem, _ = _load_or_generate_rsa_keys(self.rsa_key_dir) + return private_pem + + @property + def public_pem(self) -> bytes: + """Return the public key PEM bytes (loads/generates on first call).""" + _, public_pem = _load_or_generate_rsa_keys(self.rsa_key_dir) + return public_pem + + @property + def private_key(self) -> RSAPrivateKey: + """Return the deserialized RSA private key object.""" + return load_pem_private_key(self.private_pem, password=None) + + @property + def public_key(self) -> RSAPublicKey: + """Return the deserialized RSA public key object.""" + return load_pem_public_key(self.public_pem) + @lru_cache() def get_settings() -> Settings: diff --git a/backend/app/dependencies.py b/backend/app/dependencies.py index f388f5d..2f8b597 100644 --- a/backend/app/dependencies.py +++ b/backend/app/dependencies.py @@ -27,7 +27,7 @@ async def get_current_user( ) try: payload = jwt.decode( - token, settings.SECRET_KEY, algorithms=[settings.ALGORITHM] + token, settings.public_pem, algorithms=[settings.ALGORITHM] ) user_id: str | None = payload.get("sub") if user_id is None: diff --git a/backend/app/models/session.py b/backend/app/models/session.py index 7a7d33a..6def90e 100644 --- a/backend/app/models/session.py +++ b/backend/app/models/session.py @@ -3,6 +3,7 @@ import uuid from sqlalchemy import Column, Integer, String, DateTime, ForeignKey, Boolean from sqlalchemy.orm import relationship from sqlalchemy.sql import func + from app.database import Base @@ -18,3 +19,23 @@ class Session(Base): updated_at = Column(DateTime(timezone=True), server_default=func.now(), onupdate=func.now()) user = relationship("User", back_populates="sessions") + + +class RefreshToken(Base): + __tablename__ = "refresh_tokens" + + id = Column(String(36), primary_key=True, default=lambda: str(uuid.uuid4())) + user_id = Column(Integer, ForeignKey("users.id", ondelete="CASCADE"), nullable=False) + # RS256 JWTs are ~420+ chars; 512 is sufficient and matches the migration. + token = Column(String(512), unique=True, index=True, nullable=False) + expires_at = Column(DateTime(timezone=True), nullable=False) + is_active = Column(Boolean, nullable=False, default=True, server_default="true") + created_at = Column(DateTime(timezone=True), nullable=False, server_default=func.now()) + updated_at = Column( + DateTime(timezone=True), + nullable=False, + server_default=func.now(), + onupdate=func.now(), + ) + + user = relationship("User", back_populates="refresh_tokens") diff --git a/backend/app/models/user.py b/backend/app/models/user.py index 8ce7f88..e2197e7 100644 --- a/backend/app/models/user.py +++ b/backend/app/models/user.py @@ -21,6 +21,7 @@ class User(Base): updated_at = Column(DateTime(timezone=True), onupdate=func.now()) sessions = relationship("Session", back_populates="user", cascade="all, delete-orphan") + refresh_tokens = relationship("RefreshToken", back_populates="user", cascade="all, delete-orphan") worlds = relationship("World", back_populates="owner") assets = relationship("Asset", back_populates="owner") diff --git a/backend/app/routers/auth.py b/backend/app/routers/auth.py index 28f5848..260042f 100644 --- a/backend/app/routers/auth.py +++ b/backend/app/routers/auth.py @@ -1,20 +1,21 @@ """Authentication router - login, register, token refresh.""" +import uuid from datetime import date, datetime, timedelta, timezone from fastapi import APIRouter, Depends, HTTPException, status from fastapi.security import OAuth2PasswordRequestForm -from pydantic import BaseModel, EmailStr, field_validator - from jose import jwt from passlib.context import CryptContext +from pydantic import BaseModel, EmailStr, field_validator +from sqlalchemy import select +from sqlalchemy.ext.asyncio import AsyncSession from app.config import settings from app.database import get_db from app.dependencies import CurrentUser +from app.models.session import RefreshToken from app.models.user import User -from sqlalchemy.ext.asyncio import AsyncSession -from sqlalchemy import select pwd_context = CryptContext(schemes=["bcrypt"], deprecated="auto") @@ -25,7 +26,9 @@ router = APIRouter() class TokenResponse(BaseModel): access_token: str + refresh_token: str token_type: str = "bearer" + access_token_expires_minutes: int class RegisterRequest(BaseModel): @@ -59,12 +62,36 @@ class UserResponse(BaseModel): # --- Helpers --- def _create_access_token(data: dict) -> str: + """Create a signed access token using RS256.""" to_encode = data.copy() expire = datetime.now(timezone.utc) + timedelta( minutes=settings.ACCESS_TOKEN_EXPIRE_MINUTES ) to_encode.update({"exp": expire}) - return jwt.encode(to_encode, settings.SECRET_KEY, algorithm=settings.ALGORITHM) + return jwt.encode( + to_encode, + settings.private_pem, + algorithm=settings.ALGORITHM, + ) + + +def _create_refresh_token(data: dict) -> str: + """Create a signed refresh token using RS256. + + Includes a unique ``jti`` claim so that concurrent calls produce + distinct tokens even when issued within the same second. + """ + to_encode = data.copy() + to_encode["jti"] = str(uuid.uuid4()) + expire = datetime.now(timezone.utc) + timedelta( + days=settings.REFRESH_TOKEN_EXPIRE_DAYS + ) + to_encode.update({"exp": expire}) + return jwt.encode( + to_encode, + settings.private_pem, + algorithm=settings.ALGORITHM, + ) def verify_password(plain_password: str, hashed_password: str) -> bool: @@ -97,11 +124,25 @@ def calculate_age(birth_date: date, today: date | None = None) -> int: return age +def _issue_refresh_token(user_id: int, db: AsyncSession) -> str: + """Create and persist a refresh token for the given user.""" + refresh_token_value = _create_refresh_token({"sub": str(user_id)}) + refresh_token_record = RefreshToken( + id=str(uuid.uuid4()), + user_id=user_id, + token=refresh_token_value, + expires_at=datetime.now(timezone.utc) + + timedelta(days=settings.REFRESH_TOKEN_EXPIRE_DAYS), + ) + db.add(refresh_token_record) + return refresh_token_value + + # --- Endpoints --- @router.post("/register", response_model=TokenResponse, status_code=status.HTTP_201_CREATED) async def register(req: RegisterRequest, db: AsyncSession = Depends(get_db)): - """Register a new user and return an access token. + """Register a new user and return access + refresh tokens. Requires date_of_birth and rejects users under 18. """ @@ -138,16 +179,26 @@ async def register(req: RegisterRequest, db: AsyncSession = Depends(get_db)): date_of_birth=dob, ) db.add(user) - await db.commit() + await db.flush() # flush to get user.id before creating refresh token await db.refresh(user) + # Create access token access_token = _create_access_token({"sub": str(user.id)}) - return TokenResponse(access_token=access_token) + + # Create and persist refresh token + refresh_token_value = _issue_refresh_token(user.id, db) + await db.commit() + + return TokenResponse( + access_token=access_token, + refresh_token=refresh_token_value, + access_token_expires_minutes=settings.ACCESS_TOKEN_EXPIRE_MINUTES, + ) @router.post("/login", response_model=TokenResponse) async def login(form: OAuth2PasswordRequestForm = Depends(), db: AsyncSession = Depends(get_db)): - """Authenticate user and return an access token. + """Authenticate user and return access + refresh tokens. Accepts form-encoded credentials (username/password) as expected by OAuth2PasswordBearer, so the Swagger UI "Authorize" flow works correctly. @@ -167,8 +218,18 @@ async def login(form: OAuth2PasswordRequestForm = Depends(), db: AsyncSession = detail="User account is inactive", ) + # Create access token access_token = _create_access_token({"sub": str(user.id)}) - return TokenResponse(access_token=access_token) + + # Create and persist refresh token + refresh_token_value = _issue_refresh_token(user.id, db) + await db.commit() + + return TokenResponse( + access_token=access_token, + refresh_token=refresh_token_value, + access_token_expires_minutes=settings.ACCESS_TOKEN_EXPIRE_MINUTES, + ) @router.get("/me", response_model=UserResponse) diff --git a/backend/tests/test_refresh_token_jti.py b/backend/tests/test_refresh_token_jti.py new file mode 100644 index 0000000..e8b1071 --- /dev/null +++ b/backend/tests/test_refresh_token_jti.py @@ -0,0 +1,79 @@ +"""Tests for refresh token uniqueness (jti claim) and model column length.""" + +import uuid +from datetime import datetime, timedelta, timezone + +import pytest +from jose import jwt + +from app.config import settings +from app.routers.auth import _create_refresh_token +from app.models.session import RefreshToken + + +class TestRefreshTokenJtiUniqueness: + """Verify that _create_refresh_token produces unique tokens even when + called with identical data in rapid succession (issue #1 from review).""" + + def test_two_calls_produce_different_tokens(self): + """Two calls with the same sub produce different JWTs because of jti.""" + token_a = _create_refresh_token({"sub": "42"}) + token_b = _create_refresh_token({"sub": "42"}) + assert token_a != token_b + + def test_jti_is_valid_uuid(self): + """The jti claim inside the refresh token is a valid UUID.""" + token = _create_refresh_token({"sub": "1"}) + payload = jwt.decode( + token, + settings.public_pem, + algorithms=[settings.ALGORITHM], + ) + assert "jti" in payload + # Verify it parses as a UUID (raises ValueError if not) + uuid.UUID(payload["jti"]) + + def test_each_token_has_unique_jti(self): + """Generate 100 tokens for the same user — all jtis must differ.""" + jtis = set() + for _ in range(100): + token = _create_refresh_token({"sub": "999"}) + payload = jwt.decode( + token, + settings.public_pem, + algorithms=[settings.ALGORITHM], + ) + jtis.add(payload["jti"]) + assert len(jtis) == 100 + + def test_jti_survives_decode(self): + """The jti claim is present and correct after decode.""" + token = _create_refresh_token({"sub": "7"}) + payload = jwt.decode( + token, + settings.public_pem, + algorithms=[settings.ALGORITHM], + ) + assert payload["sub"] == "7" + assert "jti" in payload + assert "exp" in payload + + +class TestRefreshTokenModelColumnLength: + """Verify the model's token column length matches the migration (issue #2).""" + + def test_token_column_has_length_512(self): + """RefreshToken.token column should be String(512), not unbounded.""" + from sqlalchemy import inspect + + # Get the column definition from the model's table + column = RefreshToken.__table__.columns["token"] + # The type should be String with length 512 + assert column.type.length == 512 + + def test_token_column_type_is_string(self): + """The token column should be a String type.""" + from sqlalchemy import String as SAString + + column = RefreshToken.__table__.columns["token"] + assert isinstance(column.type, SAString)