diff --git a/src/backend/api/middleware/api_key.py b/src/backend/api/middleware/api_key.py index 3f7e5a4..fb48cb3 100644 --- a/src/backend/api/middleware/api_key.py +++ b/src/backend/api/middleware/api_key.py @@ -1,10 +1,17 @@ """API Key authentication dependency for FastAPI.""" +from datetime import timedelta + from fastapi import Depends, HTTPException, Request, status from sqlalchemy import select from sqlalchemy.ext.asyncio import AsyncSession from src.backend.database import get_db +from src.backend.config import settings +from src.backend.models.orm.api_key import ApiKey from src.backend.models.orm.user import User +from src.backend.services.auth_service import hash_api_key, utcnow + +_RENEW_INTERVAL = timedelta(minutes=5) async def get_current_user( @@ -24,15 +31,25 @@ async def get_current_user( ) result = await db.execute( - select(User).where(User.api_key == api_key, User.active == True) + select(ApiKey, User) + .join(User, User.id == ApiKey.user_id) + .where(ApiKey.key_hash == hash_api_key(api_key), User.active == True) ) - user = result.scalar_one_or_none() + row = result.first() + now = utcnow() - if user is None: + if row is None or row[0].expires_at <= now: raise HTTPException( status_code=status.HTTP_401_UNAUTHORIZED, - detail="Invalid or inactive API key", + detail="Invalid or expired API key", ) + key_row, user = row + + # Sliding TTL, written at most once per _RENEW_INTERVAL to spare the DB. + if key_row.last_used_at is None or now - key_row.last_used_at > _RENEW_INTERVAL: + key_row.last_used_at = now + key_row.expires_at = now + timedelta(hours=settings.api_key_ttl_hours) + await db.flush() # Store user_id in request state for access logging middleware request.state.user_id = user.id diff --git a/src/backend/api/routers/auth.py b/src/backend/api/routers/auth.py index bf65c66..4dd8369 100644 --- a/src/backend/api/routers/auth.py +++ b/src/backend/api/routers/auth.py @@ -1,5 +1,5 @@ """Authentication router - login, logout, profile.""" -from fastapi import APIRouter, Depends, HTTPException, status +from fastapi import APIRouter, Depends, HTTPException, Request, status from sqlalchemy.ext.asyncio import AsyncSession from src.backend.database import get_db @@ -34,15 +34,16 @@ async def login(data: LoginRequest, db: AsyncSession = Depends(get_db)): @router.post("/logout", status_code=status.HTTP_204_NO_CONTENT) async def logout( + request: Request, all_sessions: bool = False, user: User = Depends(get_current_user), db: AsyncSession = Depends(get_db), ): - """Logout. The API key is shared across stations logged in as the same - user, so by default this does not invalidate it - only this station's - session (its Flask session, on the client side) ends. Pass - ?all_sessions=true to invalidate the key everywhere.""" - await logout_user(db, user, all_sessions=all_sessions) + """Logout. Deletes this session's API key only; ?all_sessions=true + deletes every key of the user.""" + await logout_user( + db, user, key=request.headers.get("X-API-Key"), all_sessions=all_sessions + ) @router.get("/me", response_model=UserResponse) diff --git a/src/backend/api/routers/setup.py b/src/backend/api/routers/setup.py index 36ff4d4..5ab3f77 100644 --- a/src/backend/api/routers/setup.py +++ b/src/backend/api/routers/setup.py @@ -5,7 +5,6 @@ NOT protected by API Key auth (standalone access). """ import hmac import random -import secrets from datetime import datetime, timedelta from pathlib import Path @@ -25,7 +24,7 @@ from src.backend.models.orm import ( User, Recipe, RecipeVersion, RecipeTask, RecipeSubtask, Measurement, ) from src.backend.models.orm.station import Station, StationRecipeAssignment -from src.backend.services.auth_service import hash_password +from src.backend.services.auth_service import hash_password, issue_api_key, revoke_all_keys from src.backend.services.measurement_service import calculate_pass_fail router = APIRouter(prefix="/api/setup", tags=["setup"]) @@ -321,8 +320,6 @@ async def create_admin(body: CreateAdminBody): """Create an admin user with all roles.""" _check_password(body.password) - api_key = secrets.token_hex(32) - async with async_session_factory() as session: async with session.begin(): user = User( @@ -332,9 +329,10 @@ async def create_admin(body: CreateAdminBody): display_name=body.admin_display_name, roles=["Maker", "MeasurementTec", "Metrologist"], is_admin=True, - api_key=api_key, ) session.add(user) + await session.flush() + api_key = await issue_api_key(session, user) return { "status": "ok", @@ -404,7 +402,6 @@ async def seed_demo_data(body: PasswordBody): email=u["email"], roles=u["roles"], is_admin=u["is_admin"], - api_key=secrets.token_hex(32), ) session.add(user) created_users.append(user) @@ -652,7 +649,6 @@ async def manage_user(body: ManageUserBody): email=body.email, roles=body.roles, is_admin=body.is_admin, - api_key=secrets.token_hex(32), ) session.add(user) await session.flush() @@ -707,7 +703,7 @@ async def change_user_password(body: ChangeUserPasswordBody): detail=f"User ID {body.user_id} not found", ) user.password_hash = hash_password(body.new_password) - user.api_key = None # Invalidate sessions + await revoke_all_keys(session, user.id) # Invalidate sessions return {"status": "ok", "message": f"Password changed for '{user.username}'"} @@ -730,7 +726,7 @@ async def toggle_user_active(body: ToggleUserActiveBody): ) user.active = not user.active if not user.active: - user.api_key = None # Invalidate sessions on deactivation + await revoke_all_keys(session, user.id) # on deactivation new_status = "active" if user.active else "inactive" return {"status": "ok", "message": f"User '{user.username}' is now {new_status}", "active": user.active} diff --git a/src/backend/api/routers/users.py b/src/backend/api/routers/users.py index 3abc742..94ae584 100644 --- a/src/backend/api/routers/users.py +++ b/src/backend/api/routers/users.py @@ -7,7 +7,7 @@ from src.backend.database import get_db from src.backend.api.middleware.api_key import require_admin_user from src.backend.models.orm.user import User from src.backend.models.api.user import UserCreate, UserPasswordChange, UserResponse, UserUpdate -from src.backend.services.auth_service import create_user, hash_password, regenerate_api_key +from src.backend.services.auth_service import create_user, hash_password, regenerate_api_key, revoke_all_keys router = APIRouter(prefix="/api/users", tags=["users"]) @@ -146,7 +146,7 @@ async def change_user_password( if user is None: raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail="User not found") user.password_hash = hash_password(data.password) - user.api_key = None + await revoke_all_keys(db, user.id) await db.flush() return {"message": f"Password changed for user {user.username}"} @@ -168,7 +168,7 @@ async def deactivate_user( detail="Cannot deactivate yourself", ) user.active = False - user.api_key = None + await revoke_all_keys(db, user.id) await db.flush() @@ -183,5 +183,5 @@ async def regenerate_user_key( user = result.scalar_one_or_none() if user is None: raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail="User not found") - new_key = await regenerate_api_key(db, user_id) + new_key = await regenerate_api_key(db, user) return {"api_key": new_key, "message": f"API key regenerated for user {user.username}"} diff --git a/src/backend/config.py b/src/backend/config.py index b967388..48ea38a 100644 --- a/src/backend/config.py +++ b/src/backend/config.py @@ -38,6 +38,9 @@ class Settings(BaseSettings): # configured limits to keep the cluster-wide effective limit close to what # was configured instead of multiplying it by N. web_concurrency: int = 1 + + # API key session lifetime (hours); renewed on use (env API_KEY_TTL_HOURS). + api_key_ttl_hours: int = 24 # How many reverse proxies between the client and this API are trusted to # have appended their own hop to X-Forwarded-For (Nginx, in the default # deploy). Used by RateLimitMiddleware._client_ip. 0 disables proxy header diff --git a/src/backend/migrations/versions/015_api_key_hash.py b/src/backend/migrations/versions/015_api_key_hash.py new file mode 100644 index 0000000..3ea52df --- /dev/null +++ b/src/backend/migrations/versions/015_api_key_hash.py @@ -0,0 +1,54 @@ +"""API keys as SHA-256 hashes, one per session (#41) + +Replaces the plaintext `users.api_key` column with an `api_keys` table: one +row per login, holding only the SHA-256 hex digest, an expiry and the last +use. Existing keys are invalidated on purpose, not migrated - the plaintext +is exactly what must stop existing, so everybody logs in once again. + +Revision ID: 015_api_key_hash +Revises: 014_vision_fk_ondelete +Create Date: 2026-10-06 + +""" +from typing import Sequence, Union + +from alembic import op +import sqlalchemy as sa + +revision: str = '015_api_key_hash' +down_revision: Union[str, None] = '014_vision_fk_ondelete' +branch_labels: Union[str, Sequence[str], None] = None +depends_on: Union[str, Sequence[str], None] = None + + +def upgrade() -> None: + op.create_table( + 'api_keys', + sa.Column('id', sa.Integer(), primary_key=True, autoincrement=True), + sa.Column('user_id', sa.Integer(), nullable=False), + sa.Column('key_hash', sa.String(64), nullable=False), + sa.Column('created_at', sa.DateTime(), nullable=False), + sa.Column('expires_at', sa.DateTime(), nullable=False), + sa.Column('last_used_at', sa.DateTime(), nullable=True), + sa.ForeignKeyConstraint( + ['user_id'], ['users.id'], name='fk_api_keys_user_id', ondelete='CASCADE' + ), + ) + op.create_index('ix_api_keys_key_hash', 'api_keys', ['key_hash'], unique=True) + op.create_index('ix_api_keys_user_id', 'api_keys', ['user_id']) + + with op.batch_alter_table('users') as batch: + batch.drop_index('ix_users_api_key') + batch.drop_constraint('uq_users_api_key', type_='unique') + batch.drop_column('api_key') + + +def downgrade() -> None: + with op.batch_alter_table('users') as batch: + batch.add_column(sa.Column('api_key', sa.String(64), nullable=True)) + batch.create_unique_constraint('uq_users_api_key', ['api_key']) + batch.create_index('ix_users_api_key', ['api_key']) + + op.drop_index('ix_api_keys_user_id', table_name='api_keys') + op.drop_index('ix_api_keys_key_hash', table_name='api_keys') + op.drop_table('api_keys') diff --git a/src/backend/models/orm/__init__.py b/src/backend/models/orm/__init__.py index 361afd1..1afabdb 100644 --- a/src/backend/models/orm/__init__.py +++ b/src/backend/models/orm/__init__.py @@ -1,5 +1,6 @@ """SQLAlchemy models for TieMeasureFlow.""" from src.backend.models.orm.user import User +from src.backend.models.orm.api_key import ApiKey from src.backend.models.orm.recipe import Recipe, RecipeVersion from src.backend.models.orm.task import RecipeTask, RecipeSubtask from src.backend.models.orm.measurement import Measurement @@ -11,6 +12,7 @@ from src.backend.models.orm.vision import VisionReferenceImage, VisionResult __all__ = [ "User", + "ApiKey", "Recipe", "RecipeVersion", "RecipeTask", diff --git a/src/backend/models/orm/api_key.py b/src/backend/models/orm/api_key.py new file mode 100644 index 0000000..8c9fcd5 --- /dev/null +++ b/src/backend/models/orm/api_key.py @@ -0,0 +1,25 @@ +"""API key sessions: one row per login, stored as a SHA-256 hash.""" +from datetime import datetime +from typing import Optional + +from sqlalchemy import DateTime, ForeignKey, Integer, String +from sqlalchemy.orm import Mapped, mapped_column + +from src.backend.database import Base + + +class ApiKey(Base): + """One API key per login session. + + Only the SHA-256 hex digest is stored, never the key. Dates are naive UTC. + """ + __tablename__ = "api_keys" + + id: Mapped[int] = mapped_column(Integer, primary_key=True, autoincrement=True) + user_id: Mapped[int] = mapped_column( + Integer, ForeignKey("users.id", ondelete="CASCADE"), nullable=False, index=True + ) + key_hash: Mapped[str] = mapped_column(String(64), nullable=False, unique=True, index=True) + created_at: Mapped[datetime] = mapped_column(DateTime, nullable=False) + expires_at: Mapped[datetime] = mapped_column(DateTime, nullable=False) + last_used_at: Mapped[Optional[datetime]] = mapped_column(DateTime, nullable=True) diff --git a/src/backend/models/orm/user.py b/src/backend/models/orm/user.py index 41c28e0..e5ba358 100644 --- a/src/backend/models/orm/user.py +++ b/src/backend/models/orm/user.py @@ -32,7 +32,6 @@ class User(Base): # Status active: Mapped[bool] = mapped_column(Boolean, nullable=False, default=True) - api_key: Mapped[Optional[str]] = mapped_column(String(64), unique=True, nullable=True, index=True) # Timestamps created_at: Mapped[datetime] = mapped_column( diff --git a/src/backend/services/auth_service.py b/src/backend/services/auth_service.py index 283a88f..5b2df80 100644 --- a/src/backend/services/auth_service.py +++ b/src/backend/services/auth_service.py @@ -1,12 +1,15 @@ """Authentication service - password hashing, API key management.""" +import hashlib import secrets -from datetime import datetime +from datetime import datetime, timedelta, timezone import bcrypt from fastapi import HTTPException, status -from sqlalchemy import select, update +from sqlalchemy import delete, select, update from sqlalchemy.ext.asyncio import AsyncSession +from src.backend.config import settings +from src.backend.models.orm.api_key import ApiKey from src.backend.models.orm.user import User @@ -58,8 +61,8 @@ async def authorise_supervisor(db: AsyncSession, username: str, password: str) - of them because it is the same question every time - who is this, and are they allowed to say yes. - Uses authenticate_user rather than a full login: logging in would rotate the - supervisor's API key and knock out whatever session they have open elsewhere. + Uses authenticate_user rather than a full login: logging in would issue a + needless API key for the supervisor. """ supervisor = await authenticate_user(db, username, password) if supervisor is None: @@ -75,39 +78,68 @@ async def authorise_supervisor(db: AsyncSession, username: str, password: str) - return supervisor -async def login_user(db: AsyncSession, user: User) -> str: - """Generate (or reuse) the API key and update last_login for user. +def utcnow() -> datetime: + """Naive UTC now (the whole DB moves to naive UTC, #42).""" + return datetime.now(timezone.utc).replace(tzinfo=None) - Reuses an existing key rather than rotating it on every login: the same - account logged in from two stations at once (a common shift-account - pattern here) must keep both sessions working. Rotating the key would - silently log the other station out. + +def hash_api_key(key: str) -> str: + """SHA-256 hex digest of an API key; this is all the DB ever stores.""" + return hashlib.sha256(key.encode("utf-8")).hexdigest() + + +async def issue_api_key(db: AsyncSession, user: User) -> str: + """Create a new session key for the user and return it in clear (once).""" + key = generate_api_key() + now = utcnow() + db.add( + ApiKey( + user_id=user.id, + key_hash=hash_api_key(key), + created_at=now, + expires_at=now + timedelta(hours=settings.api_key_ttl_hours), + last_used_at=now, + ) + ) + await db.flush() + return key + + +async def revoke_all_keys(db: AsyncSession, user_id: int) -> None: + """Invalidate every session key of a user.""" + await db.execute(delete(ApiKey).where(ApiKey.user_id == user_id)) + await db.flush() + + +async def login_user(db: AsyncSession, user: User) -> str: + """Issue a new API key for this login and update last_login. + + One key per login: the same account used from several stations at once + (#25) keeps every station working, because no login revokes another's key. """ - api_key = user.api_key or generate_api_key() + api_key = await issue_api_key(db, user) await db.execute( - update(User) - .where(User.id == user.id) - .values(api_key=api_key, last_login=datetime.utcnow()) + update(User).where(User.id == user.id).values(last_login=utcnow()) ) await db.flush() return api_key async def logout_user( - db: AsyncSession, user: User, all_sessions: bool = False + db: AsyncSession, user: User, key: str | None = None, all_sessions: bool = False ) -> None: - """Log out. By default a no-op on the shared API key. - - Clearing the key on every logout would kick out any other station still - using the same shift account. `all_sessions=True` is the explicit - override for a supervisor who wants every session logged out. - """ - if not all_sessions: + """Log out. Deletes only the key of this session; all_sessions=True + deletes every key of the user.""" + if all_sessions: + await revoke_all_keys(db, user.id) return - await db.execute( - update(User).where(User.id == user.id).values(api_key=None) - ) - await db.flush() + if key: + await db.execute( + delete(ApiKey).where( + ApiKey.user_id == user.id, ApiKey.key_hash == hash_api_key(key) + ) + ) + await db.flush() async def create_user( @@ -138,11 +170,7 @@ async def create_user( return user -async def regenerate_api_key(db: AsyncSession, user_id: int) -> str: - """Regenerate API key for a user.""" - new_key = generate_api_key() - await db.execute( - update(User).where(User.id == user_id).values(api_key=new_key) - ) - await db.flush() - return new_key +async def regenerate_api_key(db: AsyncSession, user: User) -> str: + """Revoke every key of the user and issue a fresh one.""" + await revoke_all_keys(db, user.id) + return await issue_api_key(db, user) diff --git a/src/backend/tests/conftest.py b/src/backend/tests/conftest.py index d1a36bb..99092fd 100644 --- a/src/backend/tests/conftest.py +++ b/src/backend/tests/conftest.py @@ -40,7 +40,7 @@ from src.backend.models.orm.measurement import Measurement from src.backend.models.orm.access_log import AccessLog from src.backend.models.orm.setting import SystemSetting, RecipeVersionAudit from src.backend.models.orm.station import Station, StationRecipeAssignment -from src.backend.services.auth_service import hash_password, generate_api_key +from src.backend.services.auth_service import hash_password, issue_api_key # --------------------------------------------------------------------------- # In-memory SQLite engine for tests @@ -142,7 +142,6 @@ async def _create_user( active: bool = True, ) -> User: """Insert a user into the test DB and return it with an API key set.""" - api_key = generate_api_key() user = User( username=username, password_hash=hash_password(password), @@ -150,13 +149,14 @@ async def _create_user( roles=roles or [], is_admin=is_admin, active=active, - api_key=api_key, language_pref="en", theme_pref="light", ) session.add(user) await session.flush() await session.refresh(user) + # Clear key of this test session; the DB only keeps its hash. + user.plain_key = await issue_api_key(session, user) return user @@ -278,4 +278,4 @@ async def create_test_recipe( def auth_headers(user: User) -> dict[str, str]: """Return headers dict with the user's API key for authenticated requests.""" - return {"X-API-Key": user.api_key} + return {"X-API-Key": user.plain_key} diff --git a/src/backend/tests/test_auth.py b/src/backend/tests/test_auth.py index 7a72332..fd0ba54 100644 --- a/src/backend/tests/test_auth.py +++ b/src/backend/tests/test_auth.py @@ -78,75 +78,114 @@ class TestLogin: assert len(api_key) >= 32 -class TestLoginKeyReuse: - """login_user reuses an existing api_key instead of rotating it (#25).""" +class TestLoginKeyPerSession: + """Each login issues its own key; none revokes another (#25, #41).""" - async def test_two_logins_same_user_share_api_key( + async def test_two_logins_same_user_get_distinct_valid_keys( self, client: AsyncClient, db_session ): - """Two stations logging in as the same shift account both keep working.""" await _create_user(db_session, username="twostations", password="TwoStations1") - resp1 = await client.post( - "/api/auth/login", - json={"username": "twostations", "password": "TwoStations1"}, - ) - resp2 = await client.post( - "/api/auth/login", - json={"username": "twostations", "password": "TwoStations1"}, - ) - assert resp1.status_code == 200 - assert resp2.status_code == 200 - assert resp1.json()["api_key"] == resp2.json()["api_key"] + creds = {"username": "twostations", "password": "TwoStations1"} + resp1 = await client.post("/api/auth/login", json=creds) + resp2 = await client.post("/api/auth/login", json=creds) + k1, k2 = resp1.json()["api_key"], resp2.json()["api_key"] + assert k1 != k2 + for k in (k1, k2): + resp = await client.get("/api/auth/me", headers={"X-API-Key": k}) + assert resp.status_code == 200 - async def test_login_generates_key_when_user_has_none( + async def test_db_stores_hash_never_clear_key( self, client: AsyncClient, db_session ): - """A user created without an api_key (e.g. via create_user) still gets one.""" - from src.backend.services.auth_service import create_user + from sqlalchemy import select + from src.backend.models.orm.api_key import ApiKey + from src.backend.services.auth_service import hash_api_key - await create_user( - db_session, username="brandnew", password="BrandNew1", - display_name="Brand New", - ) + await _create_user(db_session, username="hashed", password="HashedPass1") resp = await client.post( - "/api/auth/login", - json={"username": "brandnew", "password": "BrandNew1"}, + "/api/auth/login", json={"username": "hashed", "password": "HashedPass1"} ) + key = resp.json()["api_key"] + hashes = (await db_session.execute(select(ApiKey.key_hash))).scalars().all() + assert hash_api_key(key) in hashes + assert key not in hashes + assert all(len(h) == 64 for h in hashes) + + async def test_expired_key_gives_401(self, client: AsyncClient, db_session): + from datetime import timedelta + from sqlalchemy import update + from src.backend.models.orm.api_key import ApiKey + from src.backend.services.auth_service import utcnow + + user = await _create_user(db_session, username="expired", password="ExpiredPass1") + await db_session.execute( + update(ApiKey).values(expires_at=utcnow() - timedelta(minutes=1)) + ) + resp = await client.get("/api/auth/me", headers=auth_headers(user)) + assert resp.status_code == 401 + assert "expired" in resp.json()["detail"].lower() + + async def test_use_renews_expiry_only_after_interval( + self, client: AsyncClient, db_session + ): + from datetime import timedelta + from sqlalchemy import select, update + from src.backend.models.orm.api_key import ApiKey + from src.backend.services.auth_service import utcnow + + user = await _create_user(db_session, username="renew", password="RenewPass1") + headers = auth_headers(user) + old = utcnow() - timedelta(hours=10) + + # Recent use: no write, expiry untouched. + recent = utcnow() - timedelta(minutes=1) + await db_session.execute( + update(ApiKey).values(expires_at=old + timedelta(hours=24), last_used_at=recent) + ) + await client.get("/api/auth/me", headers=headers) + row = (await db_session.execute(select(ApiKey))).scalar_one() + await db_session.refresh(row) + assert row.last_used_at == recent + + # Stale last use: expiry is pushed forward. + await db_session.execute( + update(ApiKey).values(last_used_at=utcnow() - timedelta(minutes=10)) + ) + before = utcnow() + resp = await client.get("/api/auth/me", headers=headers) assert resp.status_code == 200 - assert resp.json()["api_key"] + await db_session.refresh(row) + assert row.expires_at >= before + timedelta(hours=23, minutes=59) class TestLogout: - """POST /api/auth/logout is a no-op on the shared api_key by default (#25).""" + """Logout deletes only this session's key (#41).""" - async def test_logout_default_keeps_api_key_valid( - self, client: AsyncClient, db_session - ): - user = await _create_user( - db_session, username="shared_logout", password="SharedPass1" - ) - headers = auth_headers(user) - resp = await client.post("/api/auth/logout", headers=headers) + async def test_logout_removes_only_own_key(self, client: AsyncClient, db_session): + await _create_user(db_session, username="shared_logout", password="SharedPass1") + creds = {"username": "shared_logout", "password": "SharedPass1"} + k1 = (await client.post("/api/auth/login", json=creds)).json()["api_key"] + k2 = (await client.post("/api/auth/login", json=creds)).json()["api_key"] + resp = await client.post("/api/auth/logout", headers={"X-API-Key": k1}) assert resp.status_code == 204 - # The key is still valid: another station on the same account keeps working. - resp = await client.get("/api/auth/me", headers=headers) - assert resp.status_code == 200 + assert (await client.get("/api/auth/me", headers={"X-API-Key": k1})).status_code == 401 + assert (await client.get("/api/auth/me", headers={"X-API-Key": k2})).status_code == 200 - async def test_logout_all_sessions_invalidates_api_key( + async def test_logout_all_sessions_removes_every_key( self, client: AsyncClient, db_session ): - user = await _create_user( - db_session, username="shared_logout_all", password="SharedPass1" - ) - headers = auth_headers(user) + await _create_user(db_session, username="shared_logout_all", password="SharedPass1") + creds = {"username": "shared_logout_all", "password": "SharedPass1"} + k1 = (await client.post("/api/auth/login", json=creds)).json()["api_key"] + k2 = (await client.post("/api/auth/login", json=creds)).json()["api_key"] resp = await client.post( - "/api/auth/logout", params={"all_sessions": "true"}, headers=headers + "/api/auth/logout", params={"all_sessions": "true"}, + headers={"X-API-Key": k1}, ) assert resp.status_code == 204 - - resp = await client.get("/api/auth/me", headers=headers) - assert resp.status_code == 401 + for k in (k1, k2): + assert (await client.get("/api/auth/me", headers={"X-API-Key": k})).status_code == 401 class TestAuthenticateUserTiming: @@ -185,3 +224,48 @@ class TestHealthAndAuth: resp = await client.get("/api/users") assert resp.status_code == 401 assert "API key" in resp.json()["detail"] or "Missing" in resp.json()["detail"] + + +class TestRevokeAllKeys: + """Password change, deactivation and regenerate invalidate every key (#41).""" + + async def _two_keys(self, client, db_session, name): + user = await _create_user(db_session, username=name, password="KeysPass123") + creds = {"username": name, "password": "KeysPass123"} + keys = [ + (await client.post("/api/auth/login", json=creds)).json()["api_key"] + for _ in range(2) + ] + return user, keys + + async def _all_401(self, client, keys): + for k in keys: + resp = await client.get("/api/auth/me", headers={"X-API-Key": k}) + assert resp.status_code == 401 + + async def test_password_change_invalidates_all(self, client, db_session, admin_user): + user, keys = await self._two_keys(client, db_session, "pwkeys") + resp = await client.put( + f"/api/users/{user.id}/password", + headers=auth_headers(admin_user), json={"password": "NewSecurePass1"}, + ) + assert resp.status_code == 200 + await self._all_401(client, keys) + + async def test_deactivation_invalidates_all(self, client, db_session, admin_user): + user, keys = await self._two_keys(client, db_session, "offkeys") + resp = await client.delete( + f"/api/users/{user.id}", headers=auth_headers(admin_user) + ) + assert resp.status_code == 204 + await self._all_401(client, keys) + + async def test_regenerate_invalidates_old_keys(self, client, db_session, admin_user): + user, keys = await self._two_keys(client, db_session, "regenkeys") + resp = await client.post( + f"/api/users/{user.id}/regenerate-key", headers=auth_headers(admin_user) + ) + assert resp.status_code == 200 + await self._all_401(client, keys) + new = resp.json()["api_key"] + assert (await client.get("/api/auth/me", headers={"X-API-Key": new})).status_code == 200 diff --git a/src/backend/tests/test_migrations.py b/src/backend/tests/test_migrations.py index 4df78a3..9b61d71 100644 --- a/src/backend/tests/test_migrations.py +++ b/src/backend/tests/test_migrations.py @@ -107,3 +107,27 @@ class TestBaseMigrationNoopsOnExistingSchema: assert "image_path" in { c["name"] for c in insp.get_columns("recipes") } + + +class TestApiKeyHashMigration: + """015_api_key_hash: users.api_key -> api_keys table, and back (#41).""" + + def test_015_upgrade_and_downgrade(self, sqlite_database_url): + db_path, _ = sqlite_database_url + cfg = _alembic_config() + command.upgrade(cfg, "head") + + insp = _inspect(db_path) + assert "api_keys" in insp.get_table_names() + assert "api_key" not in {c["name"] for c in insp.get_columns("users")} + assert {c["name"] for c in insp.get_columns("api_keys")} >= { + "user_id", "key_hash", "created_at", "expires_at", "last_used_at", + } + + command.downgrade(cfg, "014_vision_fk_ondelete") + insp = _inspect(db_path) + assert "api_keys" not in insp.get_table_names() + assert "api_key" in {c["name"] for c in insp.get_columns("users")} + + command.upgrade(cfg, "head") # and up again + assert "api_keys" in _inspect(db_path).get_table_names() diff --git a/src/backend/tests/test_user_password_endpoint.py b/src/backend/tests/test_user_password_endpoint.py index a0ff744..2bf09d7 100644 --- a/src/backend/tests/test_user_password_endpoint.py +++ b/src/backend/tests/test_user_password_endpoint.py @@ -17,7 +17,7 @@ class TestChangeUserPassword: the same as setup.py's change-user-password (setup.py:634). """ - old_key = maker_user.api_key + old_key = maker_user.plain_key old_username = maker_user.username assert old_key is not None @@ -31,7 +31,7 @@ class TestChangeUserPassword: # The old key no longer authenticates. Read it before the request # above: the client and app share one session (see conftest), so - # `maker_user.api_key` in this process reflects the mutation too. + # `maker_user.plain_key` in this process reflects the mutation too. resp2 = await client.get("/api/auth/me", headers={"X-API-Key": old_key}) assert resp2.status_code == 401