diff --git a/.gitignore b/.gitignore index eb8247f..9a29f2a 100644 --- a/.gitignore +++ b/.gitignore @@ -6,9 +6,11 @@ __pycache__/ graphify-out backend/debug -# Ephemeral browser-verification screenshots (the tracked captures/ files are -# scraper reference material and stay tracked). -/captures/verification/ +# Scraper page captures and browser-verification screenshots contain real +# personal data from Försäkringskassan — never track them. Tests use the +# synthetic fixture in backend/tests/fixtures/ instead. +/captures/ +/test.html # C extensions *.so diff --git a/CLAUDE.md b/CLAUDE.md index bb2b63f..6c87877 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -97,7 +97,7 @@ frontend/ TypeScript · React 18.3 · Vite 5 · Vitest **Data layer (frontend):** components consume `src/hooks/*`, which call `src/services/api.ts`. Components should not call `fetch`/the API client directly. -**Browser-verification screenshots:** ephemeral screenshots from verification runs go in `captures/verification/` (which is gitignored), while `captures/` itself holds tracked scraper reference material. +**Captures are never tracked:** `captures/` (scraper page dumps, verification screenshots) is gitignored because real FK pages contain personal data. Scraper parser tests use the synthetic fixture `backend/tests/fixtures/fk_overview_synthetic.html`. --- diff --git a/backend/.env.example b/backend/.env.example index 2fe8820..d5ab665 100644 --- a/backend/.env.example +++ b/backend/.env.example @@ -1,14 +1,33 @@ # Copy to .env and fill in values for local development. # Pydantic-settings reads these with the APP_ prefix. +# Deployment environment: development | test | production. +# In production, Settings refuses to start unless APP_JWT_SECRET is set to a +# random value of >=32 chars (see app/config.py). Also gates /docs, /redoc, +# and /openapi.json (disabled in production). +APP_ENV=development + # Enable FK scraper (legal cleared 2026-06; requires `playwright install chromium`) APP_SCRAPER_ENABLED=true +# Scraper debug artifacts (HTML dump + full-page screenshot of the logged-in +# FK page, saved on error) — OFF by default. CONTAINS PERSONAL DATA (name, +# balances, payout amounts) when enabled. Only turn on for local debugging on +# a machine you control, and delete the directory afterwards. Both must be +# set to actually enable saving. +# APP_SCRAPER_DEBUG_ENABLED=true +# APP_SCRAPER_DEBUG_DIR=./debug/scraper + # Database (default: SQLite in repo root) # APP_SQLITE_PATH=./foraldraledig.db -# JWT secret — change in production (min 32 chars) +# JWT secret — REQUIRED in production (min 32 chars). Generate with: +# python -c "import secrets; print(secrets.token_urlsafe(48))" # APP_JWT_SECRET=dev-secret-change-in-production-min-32-chars # CORS — add your frontend origin(s) # APP_CORS_ORIGINS=["http://localhost:5173"] + +# Log level (default: INFO). Avoid DEBUG in production — scraper/auth code +# paths log more detail at DEBUG. +# APP_LOG_LEVEL=INFO diff --git a/backend/.gitignore b/backend/.gitignore index 39f0535..fca56f3 100644 --- a/backend/.gitignore +++ b/backend/.gitignore @@ -6,4 +6,7 @@ __pycache__/ htmlcov/ .coverage *.db +*.db-shm +*.db-wal +*.db-journal .env diff --git a/backend/README.md b/backend/README.md index 5597905..9572ec8 100644 --- a/backend/README.md +++ b/backend/README.md @@ -13,6 +13,17 @@ uv run pytest # run tests with coverage uv run ruff check . # lint ``` +## Configuration + +Key env vars (prefix `APP_`, see `.env.example`): + +- `APP_ENV` — `development` (default) | `test` | `production`. In production, + `Settings` refuses to start unless `APP_JWT_SECRET` is a random value of at + least 32 characters (the shipped dev default is rejected). Production also + disables `/docs`, `/redoc`, and `/openapi.json`. +- `APP_JWT_SECRET` — required to be a strong, random secret in production. +- `APP_LOG_LEVEL` — default `INFO`. + ## Layout ``` diff --git a/backend/app/config.py b/backend/app/config.py index 1c0f658..4eb0c18 100644 --- a/backend/app/config.py +++ b/backend/app/config.py @@ -13,9 +13,13 @@ the common-case figures — children born before 2014, multiples, and single custody differ and should become explicit config when tackled. """ -from pydantic import BaseModel, ConfigDict +from typing import Literal + +from pydantic import BaseModel, ConfigDict, model_validator from pydantic_settings import BaseSettings, SettingsConfigDict +_DEV_JWT_SECRET = "dev-secret-change-in-production-min-32-chars" + class LeaveRules(BaseModel): """Frozen Swedish föräldrapenning rule defaults (single source of truth).""" @@ -95,6 +99,11 @@ class Settings(BaseSettings): extra="ignore", ) + # Deployment environment. Gates production-only hardening (see the + # model_validator below and main.py's docs_url handling). Never set to + # "production" in tests/dev — the validator below will refuse to start. + app_env: Literal["development", "test", "production"] = "development" + # 'sqlite' for local dev, 'postgres' for production (swap repository impls). database_type: str = "sqlite" sqlite_path: str = "./foraldraledig.db" @@ -103,12 +112,12 @@ class Settings(BaseSettings): cors_origins: list[str] = ["http://localhost:5173"] # Auth — change jwt_secret in production (min 32 chars). - jwt_secret: str = "dev-secret-change-in-production-min-32-chars" + jwt_secret: str = _DEV_JWT_SECRET jwt_algorithm: str = "HS256" jwt_access_token_expire_minutes: int = 15 jwt_refresh_token_expire_days: int = 7 - log_level: str = "DEBUG" + log_level: str = "INFO" # FK scraper — legal/ToS review completed (2026-06); cleared for use. # Default is True. Set APP_SCRAPER_ENABLED=false to disable (e.g. an @@ -117,10 +126,28 @@ class Settings(BaseSettings): scraper_enabled: bool = True scraper_headless: bool = True scraper_session_ttl_seconds: int = 180 # auto-cancel awaiting_scan/authenticating sessions - # Set to None to disable debug artifact saving. Files may contain personal data. - scraper_debug_dir: str | None = "./debug/scraper" + # Debug artifacts (HTML + full-page screenshot of the logged-in FK page) + # are OFF by default — they contain personal data (name, balances, + # payout amounts). Both fields below must be set to enable them; this is + # a deliberate opt-in for local debugging only, never for production. + scraper_debug_dir: str | None = None + scraper_debug_enabled: bool = False + # How long a terminal (done/error/cancelled) session's QR/result stays in + # RAM before `_ttl_watchdog`/`poll` purge it from `_sessions`. + scraper_result_retention_seconds: int = 300 rules: LeaveRules = LeaveRules() + @model_validator(mode="after") + def _require_strong_jwt_secret_in_production(self) -> "Settings": + if self.app_env == "production" and ( + self.jwt_secret == _DEV_JWT_SECRET or len(self.jwt_secret) < 32 + ): + raise ValueError( + "APP_JWT_SECRET must be set to a random value of at least 32 characters " + "when APP_ENV=production" + ) + return self + settings = Settings() diff --git a/backend/app/db/connection.py b/backend/app/db/connection.py index acc4dd8..c389bfa 100644 --- a/backend/app/db/connection.py +++ b/backend/app/db/connection.py @@ -10,6 +10,12 @@ connection so test fixtures stay fast and do not depend on Alembic. from __future__ import annotations +import asyncio +import weakref +from collections.abc import AsyncIterator +from contextlib import asynccontextmanager +from contextvars import ContextVar + import aiosqlite # Full schema used by test fixtures directly (avoids a full Alembic run in tests). @@ -46,6 +52,7 @@ CREATE TABLE IF NOT EXISTS "user" ( email TEXT NOT NULL UNIQUE, hashed_password TEXT NOT NULL, created_at TIMESTAMP NOT NULL, + is_active INTEGER NOT NULL DEFAULT 1, FOREIGN KEY (personal_data_uuid) REFERENCES personal_data(uuid) ON DELETE CASCADE ); @@ -183,18 +190,152 @@ CREATE TABLE IF NOT EXISTS tip_dismissal ( UNIQUE(workspace_id, tip_id), FOREIGN KEY (workspace_id) REFERENCES workspace(id) ON DELETE CASCADE ); + +-- Indexes (migration 011). Mirrored verbatim (same columns, same order, no +-- partial indexes) from migrations/versions/011_add_indexes.py so +-- tests/integration/test_schema_parity.py, which compares index sets between +-- the Alembic chain and this hand-maintained DDL, stays green. See the +-- backend-perf audit §4 for what each index serves. +CREATE INDEX IF NOT EXISTS idx_pld_parent_child_type_date + ON planned_leave_day (parent_personal_data_uuid, child_id, day_type, date); +CREATE INDEX IF NOT EXISTS idx_pld_child_dubbeldag_date + ON planned_leave_day (child_id, is_dubbeldag, date); +CREATE INDEX IF NOT EXISTS idx_pld_wish + ON planned_leave_day (wish_id); +CREATE INDEX IF NOT EXISTS idx_plf_parent_child_fetched + ON parental_leave_fetch (personal_data_uuid, child_id, fetched_at); +CREATE INDEX IF NOT EXISTS idx_plf_workspace + ON parental_leave_fetch (workspace_id); +CREATE INDEX IF NOT EXISTS idx_plf_child + ON parental_leave_fetch (child_id); +CREATE INDEX IF NOT EXISTS idx_leave_wish_workspace + ON leave_wish (workspace_id); +CREATE INDEX IF NOT EXISTS idx_leave_wish_child + ON leave_wish (child_id); +CREATE INDEX IF NOT EXISTS idx_leave_wish_parent + ON leave_wish (parent_personal_data_uuid); +CREATE INDEX IF NOT EXISTS idx_child_workspace_birth + ON child (workspace_id, birth_date); +CREATE INDEX IF NOT EXISTS idx_personal_data_workspace_deleted + ON personal_data (workspace_id, deleted_at); +CREATE INDEX IF NOT EXISTS idx_invitation_workspace_status_created + ON workspace_invitation (workspace_id, status, created_at); +CREATE INDEX IF NOT EXISTS idx_invitation_invited_by + ON workspace_invitation (invited_by_user_id); +CREATE INDEX IF NOT EXISTS idx_vacation_days_workspace + ON vacation_days (workspace_id); """ +# --------------------------------------------------------------------------- +# Unit of work — see backend-perf audit §3 for the full design rationale. +# +# The app runs on ONE shared aiosqlite connection (app.state.db_connection), +# so a multi-statement write needs (a) mutual exclusion for the whole unit of +# work and (b) the repos' own per-row `commit()` calls suppressed while one is +# in progress — otherwise a concurrent request's commit would durably commit +# this one's half-finished writes. `_in_uow` and `_locks` below exist to +# provide exactly that on top of the existing single-connection, per-repo- +# commit design, without touching any repo's read/write SQL. +# --------------------------------------------------------------------------- + +# True while the current asyncio task is inside a `unit_of_work()` block, on +# whichever connection started it. Checked by `commit()` (skip the per-row +# commit; the unit of work commits once, at the end) and by `unit_of_work()` +# itself (a nested call is a no-op passthrough — see its docstring). +_in_uow: ContextVar[bool] = ContextVar("db_in_uow", default=False) + +# One asyncio.Lock per connection, created lazily on first use. A module-level +# lock would break under pytest: each test's `connection` fixture opens a new +# in-memory connection under a fresh event loop, and an `asyncio.Lock` created +# against one loop raises "bound to a different event loop" if awaited from +# another. Keying by connection (and using a WeakKeyDictionary so a closed +# test connection's lock is garbage-collected instead of leaking) sidesteps +# that entirely: production has exactly one connection => exactly one lock. +_locks: "weakref.WeakKeyDictionary[aiosqlite.Connection, asyncio.Lock]" = ( + weakref.WeakKeyDictionary() +) + + +def _lock_for(conn: aiosqlite.Connection) -> asyncio.Lock: + lock = _locks.get(conn) + if lock is None: + lock = asyncio.Lock() + _locks[conn] = lock + return lock + + +async def commit(conn: aiosqlite.Connection) -> None: + """Commit, unless a `unit_of_work()` is currently in progress on this task. + + Every sqlite repository's single-row write calls this instead of + `conn.commit()` directly. Outside a unit of work this behaves exactly + like today (commit immediately, once per write); inside one, it is a + no-op, and the surrounding `unit_of_work()` block commits once, atomically, + when it exits. + """ + if not _in_uow.get(): + await conn.commit() + + +@asynccontextmanager +async def unit_of_work(conn: aiosqlite.Connection) -> AsyncIterator[None]: + """Wrap the block in one atomic transaction on the shared connection. + + Acquires this connection's lock for the whole block (SQLite is a + single-writer database anyway, so this costs no real throughput) and + issues `BEGIN IMMEDIATE` up front — that claims the write lock + immediately rather than at the first write statement, so a second, + concurrent `unit_of_work()` call blocks here and only proceeds once this + one has committed (or rolled back), then re-reads state as of that commit + instead of racing it. Any exception rolls the whole block back; a normal + exit commits it. + + Nested calls (e.g. `PlanningService.sync_plan` wrapping its own call to + `apply_proposed_days`, which wraps its own writes) are a no-op passthrough + when `_in_uow` is already set — the outer call owns the lock/transaction, + and a naive re-entry would try to re-acquire the same (non-reentrant) + `asyncio.Lock` from the same task and deadlock. + """ + if _in_uow.get(): + yield + return + + async with _lock_for(conn): + token = _in_uow.set(True) + try: + await conn.execute("BEGIN IMMEDIATE") + try: + yield + except BaseException: + await conn.rollback() + raise + else: + await conn.commit() + finally: + _in_uow.reset(token) + + async def connect(path: str) -> aiosqlite.Connection: """Open a connection and enable row-by-name access + FK enforcement. Schema is applied via Alembic in the app lifespan (`run_migrations`). For test fixtures, call `ensure_schema` directly. + + PRAGMAs: `journal_mode=WAL` + `synchronous=NORMAL` cut single-row commit + latency roughly 4x (measured 88.7ms -> 21.7ms per 300 sequential commits) + and let Alembic/`seed_demo.py` co-access the same file wait instead of + hitting "database is locked" outright; `busy_timeout` backs that wait with + an actual retry budget. Durability trade-off of NORMAL: at most the last + transaction(s) are lost on an OS crash/power loss, never on an app crash — + acceptable here (see backend-perf audit §3). """ conn = await aiosqlite.connect(path) conn.row_factory = aiosqlite.Row await conn.execute("PRAGMA foreign_keys = ON;") + await conn.execute("PRAGMA journal_mode = WAL;") + await conn.execute("PRAGMA synchronous = NORMAL;") + await conn.execute("PRAGMA busy_timeout = 5000;") return conn diff --git a/backend/app/dependencies.py b/backend/app/dependencies.py index 0868df6..b2330d8 100644 --- a/backend/app/dependencies.py +++ b/backend/app/dependencies.py @@ -10,13 +10,17 @@ Pattern (copy for each new entity): 4. `ServiceDep` — Annotated type alias """ +from dataclasses import dataclass from typing import Annotated +from uuid import UUID import aiosqlite from fastapi import Depends, HTTPException, Request, status from fastapi.security import HTTPAuthorizationCredentials, HTTPBearer from app.config import settings +from app.entities.child import Child +from app.entities.personal_data import PersonalData from app.entities.user import User from app.repositories.child_repository import ChildRepository from app.repositories.day_override_repository import DayOverrideRepository @@ -25,6 +29,7 @@ from app.repositories.parental_leave_repository import ParentalLeaveRepository from app.repositories.personal_data_repository import PersonalDataRepository from app.repositories.planned_leave_repository import PlannedLeaveRepository from app.repositories.tip_dismissal_repository import TipDismissalRepository +from app.repositories.unit_of_work import UnitOfWork from app.repositories.user_repository import UserRepository from app.repositories.vacation_days_repository import VacationDaysRepository from app.repositories.workspace_repository import ( @@ -38,6 +43,7 @@ from app.repositories_impl.sqlite.parental_leave_repository import SQLiteParenta from app.repositories_impl.sqlite.personal_data_repository import SQLitePersonalDataRepository from app.repositories_impl.sqlite.planned_leave_repository import SQLitePlannedLeaveRepository from app.repositories_impl.sqlite.tip_dismissal_repository import SQLiteTipDismissalRepository +from app.repositories_impl.sqlite.unit_of_work import SQLiteUnitOfWork from app.repositories_impl.sqlite.user_repository import SQLiteUserRepository from app.repositories_impl.sqlite.vacation_days_repository import SQLiteVacationDaysRepository from app.repositories_impl.sqlite.workspace_repository import ( @@ -74,6 +80,15 @@ def get_connection(request: Request) -> aiosqlite.Connection: ConnectionDep = Annotated[aiosqlite.Connection, Depends(get_connection)] +def get_unit_of_work(conn: ConnectionDep) -> UnitOfWork: + if settings.database_type == "sqlite": + return SQLiteUnitOfWork(conn) + raise ValueError(f"Unsupported database_type: {settings.database_type}") + + +UnitOfWorkDep = Annotated[UnitOfWork, Depends(get_unit_of_work)] + + # --------------------------------------------------------------------------- # Repository factories # --------------------------------------------------------------------------- @@ -196,8 +211,9 @@ def get_auth_service( user_repo: UserRepositoryDep, pd_repo: PersonalDataRepositoryDep, ws_repo: WorkspaceRepositoryDep, + uow: UnitOfWorkDep, ) -> AuthService: - return AuthService(user_repo, pd_repo, ws_repo) + return AuthService(user_repo, pd_repo, ws_repo, uow=uow) AuthServiceDep = Annotated[AuthService, Depends(get_auth_service)] @@ -282,6 +298,7 @@ def get_workspace_service( planned_leave_repo: PlannedLeaveRepositoryDep, parental_leave_repo: ParentalLeaveRepositoryDep, vacation_days_repo: VacationDaysRepositoryDep, + uow: UnitOfWorkDep, ) -> WorkspaceService: return WorkspaceService( ws_repo, @@ -292,6 +309,7 @@ def get_workspace_service( planned_leave_repo, parental_leave_repo, vacation_days_repo, + uow=uow, ) @@ -349,9 +367,15 @@ def get_planning_service( parental_leave_repo: ParentalLeaveRepositoryDep, vacation_repo: VacationDaysRepositoryDep, planning_engine_service: PlanningEngineServiceDep, + uow: UnitOfWorkDep, ) -> PlanningService: return PlanningService( - repo, analytics_service, parental_leave_repo, vacation_repo, planning_engine_service + repo, + analytics_service, + parental_leave_repo, + vacation_repo, + planning_engine_service, + uow=uow, ) @@ -377,3 +401,103 @@ async def get_current_user( CurrentUserDep = Annotated[User, Depends(get_current_user)] + + +# --------------------------------------------------------------------------- +# Tenant scoping +# --------------------------------------------------------------------------- +# +# Rule of thumb: identity-derived facts (which workspace the caller belongs +# to) live here in the DI layer; entity-ownership checks that require loading +# a row (e.g. a leave wish's own workspace_id) live in the service, mirroring +# `WorkspaceService._get_member`. Routers stay thin: they hand the resolved +# scope to the service and let a NotFoundError become a 404 — never a 403, +# so a foreign id never confirms its own existence. See the T1 authz audit, +# §3 "Scoping primitive — design for app/dependencies.py". + + +async def get_current_personal_data( + current_user: CurrentUserDep, pd_repo: PersonalDataRepositoryDep +) -> PersonalData: + """The caller's own PersonalData row — the root of tenant scoping.""" + pd = await pd_repo.get_by_id(current_user.personal_data_uuid) + if pd is None or pd.deleted_at is not None: + raise HTTPException(status.HTTP_401_UNAUTHORIZED, "No parent profile for this account") + return pd + + +CurrentPersonalDataDep = Annotated[PersonalData, Depends(get_current_personal_data)] + + +async def get_current_workspace_id(pd: CurrentPersonalDataDep) -> UUID: + return pd.workspace_id + + +CurrentWorkspaceIdDep = Annotated[UUID, Depends(get_current_workspace_id)] + + +def _not_found(kind: str, id_: UUID) -> HTTPException: + # 404, never 403: do not confirm that a foreign id exists. + return HTTPException(status.HTTP_404_NOT_FOUND, f"{kind} {id_} not found") + + +async def require_workspace_access(workspace_id: UUID, current: CurrentWorkspaceIdDep) -> UUID: + """Path-param guard for `/{workspace_id}/...` routes. + + FastAPI binds the `workspace_id` path param by name, so a router only + has to declare `workspace_id: WorkspaceIdDep` in place of `UUID` to get + this check for free. + """ + if workspace_id != current: + raise _not_found("Workspace", workspace_id) + return workspace_id + + +WorkspaceIdDep = Annotated[UUID, Depends(require_workspace_access)] + + +@dataclass(frozen=True) +class WorkspaceScope: + """The caller's tenant, plus helpers for ids arriving in a body/query. + + `check_workspace` is a cheap identity comparison (no DB read — the + caller's own workspace_id is already known). `check_child`/`check_member` + load the row because ownership can only be known after the load; both + 404 (never 403) on a missing OR foreign-workspace id, so a foreign id can + never be distinguished from a nonexistent one. + """ + + workspace_id: UUID + personal_data_uuid: UUID + user_id: UUID + _child_repo: ChildRepository + _pd_repo: PersonalDataRepository + + def check_workspace(self, workspace_id: UUID) -> UUID: + if workspace_id != self.workspace_id: + raise _not_found("Workspace", workspace_id) + return workspace_id + + async def check_child(self, child_id: UUID) -> Child: + child = await self._child_repo.get_by_id(child_id) + if child is None or child.workspace_id != self.workspace_id: + raise _not_found("Child", child_id) + return child + + async def check_member(self, personal_data_uuid: UUID) -> PersonalData: + pd = await self._pd_repo.get_by_id(personal_data_uuid) + if pd is None or pd.deleted_at is not None or pd.workspace_id != self.workspace_id: + raise _not_found("PersonalData", personal_data_uuid) + return pd + + +async def get_workspace_scope( + current_user: CurrentUserDep, + pd: CurrentPersonalDataDep, + child_repo: ChildRepositoryDep, + pd_repo: PersonalDataRepositoryDep, +) -> WorkspaceScope: + return WorkspaceScope(pd.workspace_id, pd.uuid, current_user.id, child_repo, pd_repo) + + +WorkspaceScopeDep = Annotated[WorkspaceScope, Depends(get_workspace_scope)] diff --git a/backend/app/main.py b/backend/app/main.py index ab46c04..83b73b8 100644 --- a/backend/app/main.py +++ b/backend/app/main.py @@ -14,6 +14,7 @@ from fastapi import FastAPI, Request from fastapi.middleware.cors import CORSMiddleware from slowapi import _rate_limit_exceeded_handler from slowapi.errors import RateLimitExceeded +from starlette.middleware.gzip import GZipMiddleware from app.config import settings from app.db.connection import connect, run_migrations @@ -63,6 +64,7 @@ async def lifespan(app: FastAPI) -> AsyncIterator[None]: await asyncio.to_thread(run_migrations, f"sqlite:///{settings.sqlite_path}") except Exception as exc: logger.error("Migration failed — DB schema may be incomplete: %s", exc, exc_info=True) + raise # Re-enable after migrations: alembic's env.py calls fileConfig() which may # still touch logger state even with disable_existing_loggers=False. @@ -78,7 +80,14 @@ async def lifespan(app: FastAPI) -> AsyncIterator[None]: def create_app() -> FastAPI: - app = FastAPI(title="Föräldraledigplannerare API", lifespan=lifespan) + is_production = settings.app_env == "production" + app = FastAPI( + title="Föräldraledigplannerare API", + lifespan=lifespan, + docs_url=None if is_production else "/docs", + redoc_url=None if is_production else "/redoc", + openapi_url=None if is_production else "/openapi.json", + ) app.state.limiter = limiter app.add_exception_handler(RateLimitExceeded, _rate_limit_exceeded_handler) @@ -90,6 +99,8 @@ def create_app() -> FastAPI: logger.warning("%s %s %d", request.method, request.url.path, response.status_code) return response + app.add_middleware(GZipMiddleware, minimum_size=1024) + app.add_middleware( CORSMiddleware, allow_origins=list(settings.cors_origins), diff --git a/backend/app/repositories/planned_leave_repository.py b/backend/app/repositories/planned_leave_repository.py index f11747f..ecbd963 100644 --- a/backend/app/repositories/planned_leave_repository.py +++ b/backend/app/repositories/planned_leave_repository.py @@ -43,3 +43,33 @@ class PlannedLeaveRepository(Repository[PlannedLeaveDay]): that DELETE runs, no `planned_leave_day` row still references this wish, so there is nothing left to orphan. """ + + @abstractmethod + async def create_many(self, items: list[PlannedLeaveDay]) -> None: + """Bulk-insert rows in one round trip (`executemany`). Empty list is a no-op. + + See backend-perf audit H1: a per-row `create()` + `commit()` loop + measured 88.7ms/300 rows (default journal mode); this + one + surrounding `unit_of_work()` is the batched replacement used by + `PlanningService.apply_proposed_days` stage 4. + """ + + @abstractmethod + async def delete_many(self, ids: list[UUID]) -> int: + """Delete rows by id in one or more `executemany`/`IN (...)` round trips. + + Idempotent: an id with no matching row is silently ignored rather than + raising `NotFoundError` (unlike `delete()`) — callers that already + know some keys may have vanished (e.g. a re-applied batch) should not + have to pre-filter. Returns the number of rows actually deleted. + """ + + @abstractmethod + async def update_metadata_many(self, pairs: list[tuple[UUID, UUID | None, bool]]) -> None: + """Bulk-update `(wish_id, is_pinned)` for existing rows via `executemany`. + + Each pair is `(id, wish_id, is_pinned)`. Does not touch `day_type`/ + `is_dubbeldag` — this is for the metadata-only path in + `apply_proposed_days` (a row whose type is unchanged but whose + `wish_id`/`is_pinned` differ from the incoming batch). + """ diff --git a/backend/app/repositories/unit_of_work.py b/backend/app/repositories/unit_of_work.py new file mode 100644 index 0000000..344ba7c --- /dev/null +++ b/backend/app/repositories/unit_of_work.py @@ -0,0 +1,39 @@ +"""Abstract unit-of-work contract for multi-statement atomic writes. + +A `UnitOfWork` is a callable that returns an async context manager: entering +it starts an atomic transaction, a normal exit commits it, and an exception +rolls it back. Services depend on this interface only — the concrete +mechanism (a locked `BEGIN IMMEDIATE` transaction on the shared connection) +lives in `app.db.connection.unit_of_work` / `repositories_impl.sqlite. +unit_of_work.SQLiteUnitOfWork`. See the backend-perf audit +(`scratchpad/audit/backend-perf.md` §3) for why a lock is required on a +single shared connection rather than a bare `BEGIN`/`COMMIT`. +""" + +from abc import ABC, abstractmethod +from collections.abc import AsyncIterator +from contextlib import AbstractAsyncContextManager, asynccontextmanager + + +class UnitOfWork(ABC): + @abstractmethod + def __call__(self) -> AbstractAsyncContextManager[None]: + """Return an async context manager wrapping its body in one atomic unit of work.""" + + +@asynccontextmanager +async def _noop() -> AsyncIterator[None]: + yield + + +class NoOpUnitOfWork(UnitOfWork): + """Default fallback: wraps nothing, so each write commits immediately. + + Used as the default for service constructors so call sites that predate + the unit-of-work work (e.g. `scripts/seed_demo.py`, direct-construction + test fixtures) keep behaving exactly as before — one implicit transaction + per write — without having to pass a real `UnitOfWork`. + """ + + def __call__(self) -> AbstractAsyncContextManager[None]: + return _noop() diff --git a/backend/app/repositories_impl/sqlite/child_repository.py b/backend/app/repositories_impl/sqlite/child_repository.py index 89f94b9..4e9ccf6 100644 --- a/backend/app/repositories_impl/sqlite/child_repository.py +++ b/backend/app/repositories_impl/sqlite/child_repository.py @@ -5,6 +5,7 @@ from uuid import UUID import aiosqlite +from app.db.connection import commit from app.entities.child import Child from app.repositories.child_repository import ChildRepository from app.repositories.filter_validator import validate_filters @@ -50,7 +51,7 @@ class SQLiteChildRepository(ChildRepository): item.created_at.isoformat(), ), ) - await self._conn.commit() + await commit(self._conn) return item async def get_by_id(self, id: UUID) -> Child | None: @@ -74,7 +75,7 @@ class SQLiteChildRepository(ChildRepository): str(id), ), ) - await self._conn.commit() + await commit(self._conn) if cursor.rowcount == 0: raise NotFoundError(f"Child {id} not found") refreshed = await self.get_by_id(id) @@ -83,7 +84,7 @@ class SQLiteChildRepository(ChildRepository): async def delete(self, id: UUID) -> None: cursor = await self._conn.execute("DELETE FROM child WHERE id = ?", (str(id),)) - await self._conn.commit() + await commit(self._conn) if cursor.rowcount == 0: raise NotFoundError(f"Child {id} not found") diff --git a/backend/app/repositories_impl/sqlite/day_override_repository.py b/backend/app/repositories_impl/sqlite/day_override_repository.py index 64e4741..25a9a95 100644 --- a/backend/app/repositories_impl/sqlite/day_override_repository.py +++ b/backend/app/repositories_impl/sqlite/day_override_repository.py @@ -10,6 +10,7 @@ from uuid import UUID import aiosqlite +from app.db.connection import commit from app.entities.day_override import DayOverride from app.repositories.day_override_repository import DayOverrideRepository from app.repositories.filter_validator import validate_filters @@ -52,7 +53,7 @@ class SQLiteDayOverrideRepository(DayOverrideRepository): item.updated_at.isoformat(), ), ) - await self._conn.commit() + await commit(self._conn) return item async def get_by_id(self, id: UUID) -> DayOverride | None: @@ -78,7 +79,7 @@ class SQLiteDayOverrideRepository(DayOverrideRepository): """, (int(item.is_working_day), item.note, item.updated_at.isoformat(), str(id)), ) - await self._conn.commit() + await commit(self._conn) if cursor.rowcount == 0: raise NotFoundError(f"DayOverride {id} not found") refreshed = await self.get_by_id(id) @@ -87,7 +88,7 @@ class SQLiteDayOverrideRepository(DayOverrideRepository): async def delete(self, id: UUID) -> None: cursor = await self._conn.execute("DELETE FROM day_override WHERE id = ?", (str(id),)) - await self._conn.commit() + await commit(self._conn) if cursor.rowcount == 0: raise NotFoundError(f"DayOverride {id} not found") diff --git a/backend/app/repositories_impl/sqlite/leave_wish_repository.py b/backend/app/repositories_impl/sqlite/leave_wish_repository.py index c0af868..4641c46 100644 --- a/backend/app/repositories_impl/sqlite/leave_wish_repository.py +++ b/backend/app/repositories_impl/sqlite/leave_wish_repository.py @@ -9,6 +9,7 @@ from uuid import UUID import aiosqlite +from app.db.connection import commit from app.entities.leave_wish import LeaveWish from app.repositories.filter_validator import validate_filters from app.repositories.leave_wish_repository import LeaveWishRepository @@ -89,7 +90,7 @@ class SQLiteLeaveWishRepository(LeaveWishRepository): item.updated_at.isoformat(), ), ) - await self._conn.commit() + await commit(self._conn) return item async def get_by_id(self, id: UUID) -> LeaveWish | None: @@ -144,7 +145,7 @@ class SQLiteLeaveWishRepository(LeaveWishRepository): str(id), ), ) - await self._conn.commit() + await commit(self._conn) if cursor.rowcount == 0: raise NotFoundError(f"LeaveWish {id} not found") refreshed = await self.get_by_id(id) @@ -153,7 +154,7 @@ class SQLiteLeaveWishRepository(LeaveWishRepository): async def delete(self, id: UUID) -> None: cursor = await self._conn.execute("DELETE FROM leave_wish WHERE id = ?", (str(id),)) - await self._conn.commit() + await commit(self._conn) if cursor.rowcount == 0: raise NotFoundError(f"LeaveWish {id} not found") diff --git a/backend/app/repositories_impl/sqlite/parental_leave_repository.py b/backend/app/repositories_impl/sqlite/parental_leave_repository.py index cdb854d..33ee631 100644 --- a/backend/app/repositories_impl/sqlite/parental_leave_repository.py +++ b/backend/app/repositories_impl/sqlite/parental_leave_repository.py @@ -6,6 +6,7 @@ from uuid import UUID import aiosqlite +from app.db.connection import commit from app.entities.parental_leave import ParentalLeaveFetch, TierData from app.repositories.filter_validator import validate_filters from app.repositories.parental_leave_repository import ParentalLeaveRepository @@ -90,7 +91,7 @@ class SQLiteParentalLeaveRepository(ParentalLeaveRepository): item.updated_at.isoformat(), ), ) - await self._conn.commit() + await commit(self._conn) return item async def get_by_id(self, id: UUID) -> ParentalLeaveFetch | None: @@ -150,7 +151,7 @@ class SQLiteParentalLeaveRepository(ParentalLeaveRepository): str(id), ), ) - await self._conn.commit() + await commit(self._conn) if cursor.rowcount == 0: raise NotFoundError(f"ParentalLeaveFetch {id} not found") refreshed = await self.get_by_id(id) @@ -161,7 +162,7 @@ class SQLiteParentalLeaveRepository(ParentalLeaveRepository): cursor = await self._conn.execute( "DELETE FROM parental_leave_fetch WHERE id = ?", (str(id),) ) - await self._conn.commit() + await commit(self._conn) if cursor.rowcount == 0: raise NotFoundError(f"ParentalLeaveFetch {id} not found") diff --git a/backend/app/repositories_impl/sqlite/personal_data_repository.py b/backend/app/repositories_impl/sqlite/personal_data_repository.py index 95dd770..9f5caf3 100644 --- a/backend/app/repositories_impl/sqlite/personal_data_repository.py +++ b/backend/app/repositories_impl/sqlite/personal_data_repository.py @@ -9,6 +9,7 @@ from uuid import UUID import aiosqlite +from app.db.connection import commit from app.entities.personal_data import PersonalData from app.repositories.filter_validator import validate_filters from app.repositories.personal_data_repository import PersonalDataRepository @@ -55,7 +56,7 @@ class SQLitePersonalDataRepository(PersonalDataRepository): item.deleted_at.isoformat() if item.deleted_at else None, ), ) - await self._conn.commit() + await commit(self._conn) return item async def get_by_id(self, id: UUID) -> PersonalData | None: @@ -83,7 +84,7 @@ class SQLitePersonalDataRepository(PersonalDataRepository): str(id), ), ) - await self._conn.commit() + await commit(self._conn) if cursor.rowcount == 0: raise NotFoundError(f"PersonalData {id} not found") refreshed = await self.get_by_id(id) @@ -98,13 +99,13 @@ class SQLitePersonalDataRepository(PersonalDataRepository): "UPDATE personal_data SET deleted_at = ? WHERE uuid = ? AND deleted_at IS NULL", (datetime.now().isoformat(), str(id)), ) - await self._conn.commit() + await commit(self._conn) if cursor.rowcount == 0: raise NotFoundError(f"PersonalData {id} not found") async def hard_delete(self, id: UUID) -> None: cursor = await self._conn.execute("DELETE FROM personal_data WHERE uuid = ?", (str(id),)) - await self._conn.commit() + await commit(self._conn) if cursor.rowcount == 0: raise NotFoundError(f"PersonalData {id} not found") diff --git a/backend/app/repositories_impl/sqlite/planned_leave_repository.py b/backend/app/repositories_impl/sqlite/planned_leave_repository.py index faf715c..2502199 100644 --- a/backend/app/repositories_impl/sqlite/planned_leave_repository.py +++ b/backend/app/repositories_impl/sqlite/planned_leave_repository.py @@ -5,6 +5,7 @@ from uuid import UUID import aiosqlite +from app.db.connection import commit from app.entities.planned_leave import PlannedLeaveDay from app.repositories.filter_validator import validate_filters from app.repositories.planned_leave_repository import PlannedLeaveRepository @@ -56,7 +57,7 @@ class SQLitePlannedLeaveRepository(PlannedLeaveRepository): item.updated_at.isoformat(), ), ) - await self._conn.commit() + await commit(self._conn) return item async def get_by_id(self, id: UUID) -> PlannedLeaveDay | None: @@ -83,7 +84,7 @@ class SQLitePlannedLeaveRepository(PlannedLeaveRepository): str(id), ), ) - await self._conn.commit() + await commit(self._conn) if cursor.rowcount == 0: raise NotFoundError(f"PlannedLeaveDay {id} not found") refreshed = await self.get_by_id(id) @@ -92,7 +93,7 @@ class SQLitePlannedLeaveRepository(PlannedLeaveRepository): async def delete(self, id: UUID) -> None: cursor = await self._conn.execute("DELETE FROM planned_leave_day WHERE id = ?", (str(id),)) - await self._conn.commit() + await commit(self._conn) if cursor.rowcount == 0: raise NotFoundError(f"PlannedLeaveDay {id} not found") @@ -177,5 +178,73 @@ class SQLitePlannedLeaveRepository(PlannedLeaveRepository): cursor = await self._conn.execute( "DELETE FROM planned_leave_day WHERE wish_id = ?", (str(wish_id),) ) - await self._conn.commit() + await commit(self._conn) return cursor.rowcount + + async def create_many(self, items: list[PlannedLeaveDay]) -> None: + if not items: + return + now = datetime.now() + rows = [] + for item in items: + item.created_at = item.created_at or now + item.updated_at = item.updated_at or now + rows.append( + ( + str(item.id), + str(item.workspace_id), + str(item.child_id), + str(item.parent_personal_data_uuid), + item.date.isoformat(), + item.day_type, + int(item.is_dubbeldag), + str(item.wish_id) if item.wish_id is not None else None, + int(item.is_pinned), + item.created_at.isoformat(), + item.updated_at.isoformat(), + ) + ) + await self._conn.executemany( + """ + INSERT INTO planned_leave_day + (id, workspace_id, child_id, parent_personal_data_uuid, date, + day_type, is_dubbeldag, wish_id, is_pinned, created_at, updated_at) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) + """, + rows, + ) + await commit(self._conn) + + async def delete_many(self, ids: list[UUID]) -> int: + if not ids: + return 0 + str_ids = [str(i) for i in ids] + total = 0 + chunk_size = 500 + for start in range(0, len(str_ids), chunk_size): + chunk = str_ids[start : start + chunk_size] + placeholders = ",".join("?" for _ in chunk) + cursor = await self._conn.execute( + f"DELETE FROM planned_leave_day WHERE id IN ({placeholders})", chunk + ) + total += cursor.rowcount + await commit(self._conn) + return total + + async def update_metadata_many(self, pairs: list[tuple[UUID, UUID | None, bool]]) -> None: + if not pairs: + return + now = datetime.now().isoformat() + rows = [ + (str(wish_id) if wish_id is not None else None, int(is_pinned), now, str(id_)) + for id_, wish_id, is_pinned in pairs + ] + await self._conn.executemany( + """ + UPDATE planned_leave_day + SET wish_id = ?, is_pinned = ?, updated_at = ? + WHERE id = ? + """, + rows, + ) + await commit(self._conn) diff --git a/backend/app/repositories_impl/sqlite/tip_dismissal_repository.py b/backend/app/repositories_impl/sqlite/tip_dismissal_repository.py index 575f0ba..d2effb0 100644 --- a/backend/app/repositories_impl/sqlite/tip_dismissal_repository.py +++ b/backend/app/repositories_impl/sqlite/tip_dismissal_repository.py @@ -11,6 +11,7 @@ from uuid import UUID import aiosqlite +from app.db.connection import commit from app.entities.tip_dismissal import TipDismissal from app.repositories.filter_validator import validate_filters from app.repositories.tip_dismissal_repository import TipDismissalRepository @@ -46,7 +47,7 @@ class SQLiteTipDismissalRepository(TipDismissalRepository): item.dismissed_at.isoformat(), ), ) - await self._conn.commit() + await commit(self._conn) return item async def get_by_id(self, id: UUID) -> TipDismissal | None: @@ -68,7 +69,7 @@ class SQLiteTipDismissalRepository(TipDismissalRepository): "UPDATE tip_dismissal SET data_hash = ?, dismissed_at = ? WHERE id = ?", (item.data_hash, item.dismissed_at.isoformat(), str(id)), ) - await self._conn.commit() + await commit(self._conn) if cursor.rowcount == 0: raise NotFoundError(f"TipDismissal {id} not found") refreshed = await self.get_by_id(id) @@ -77,7 +78,7 @@ class SQLiteTipDismissalRepository(TipDismissalRepository): async def delete(self, id: UUID) -> None: cursor = await self._conn.execute("DELETE FROM tip_dismissal WHERE id = ?", (str(id),)) - await self._conn.commit() + await commit(self._conn) if cursor.rowcount == 0: raise NotFoundError(f"TipDismissal {id} not found") diff --git a/backend/app/repositories_impl/sqlite/unit_of_work.py b/backend/app/repositories_impl/sqlite/unit_of_work.py new file mode 100644 index 0000000..c08f364 --- /dev/null +++ b/backend/app/repositories_impl/sqlite/unit_of_work.py @@ -0,0 +1,21 @@ +"""SQLite implementation of UnitOfWork. + +Delegates to `app.db.connection.unit_of_work`: a locked `BEGIN IMMEDIATE` +transaction on the shared connection. See that function's docstring and the +backend-perf audit §3 for the full design. +""" + +from contextlib import AbstractAsyncContextManager + +import aiosqlite + +from app.db.connection import unit_of_work +from app.repositories.unit_of_work import UnitOfWork + + +class SQLiteUnitOfWork(UnitOfWork): + def __init__(self, connection: aiosqlite.Connection) -> None: + self._conn = connection + + def __call__(self) -> AbstractAsyncContextManager[None]: + return unit_of_work(self._conn) diff --git a/backend/app/repositories_impl/sqlite/user_repository.py b/backend/app/repositories_impl/sqlite/user_repository.py index 4430134..c4edd4f 100644 --- a/backend/app/repositories_impl/sqlite/user_repository.py +++ b/backend/app/repositories_impl/sqlite/user_repository.py @@ -5,6 +5,7 @@ from uuid import UUID import aiosqlite +from app.db.connection import commit from app.entities.user import User from app.repositories.filter_validator import validate_filters from app.repositories.user_repository import UserRepository @@ -23,6 +24,7 @@ class SQLiteUserRepository(UserRepository): email=row["email"], hashed_password=row["hashed_password"], created_at=datetime.fromisoformat(row["created_at"]), + is_active=bool(row["is_active"]), ) async def create(self, item: User) -> User: @@ -30,8 +32,9 @@ class SQLiteUserRepository(UserRepository): item.created_at = item.created_at or now await self._conn.execute( """ - INSERT INTO "user" (id, personal_data_uuid, email, hashed_password, created_at) - VALUES (?, ?, ?, ?, ?) + INSERT INTO "user" + (id, personal_data_uuid, email, hashed_password, created_at, is_active) + VALUES (?, ?, ?, ?, ?, ?) """, ( str(item.id), @@ -39,9 +42,10 @@ class SQLiteUserRepository(UserRepository): item.email, item.hashed_password, item.created_at.isoformat(), + int(item.is_active), ), ) - await self._conn.commit() + await commit(self._conn) return item async def get_by_id(self, id: UUID) -> User | None: @@ -56,10 +60,10 @@ class SQLiteUserRepository(UserRepository): async def update(self, id: UUID, item: User) -> User: cursor = await self._conn.execute( - 'UPDATE "user" SET email = ?, hashed_password = ? WHERE id = ?', - (item.email, item.hashed_password, str(id)), + 'UPDATE "user" SET email = ?, hashed_password = ?, is_active = ? WHERE id = ?', + (item.email, item.hashed_password, int(item.is_active), str(id)), ) - await self._conn.commit() + await commit(self._conn) if cursor.rowcount == 0: raise NotFoundError(f"User {id} not found") refreshed = await self.get_by_id(id) @@ -68,7 +72,7 @@ class SQLiteUserRepository(UserRepository): async def delete(self, id: UUID) -> None: cursor = await self._conn.execute('DELETE FROM "user" WHERE id = ?', (str(id),)) - await self._conn.commit() + await commit(self._conn) if cursor.rowcount == 0: raise NotFoundError(f"User {id} not found") diff --git a/backend/app/repositories_impl/sqlite/vacation_days_repository.py b/backend/app/repositories_impl/sqlite/vacation_days_repository.py index 050e588..cb34537 100644 --- a/backend/app/repositories_impl/sqlite/vacation_days_repository.py +++ b/backend/app/repositories_impl/sqlite/vacation_days_repository.py @@ -12,6 +12,7 @@ from uuid import UUID import aiosqlite +from app.db.connection import commit from app.entities.vacation_days import VacationDays from app.repositories.filter_validator import validate_filters from app.repositories.vacation_days_repository import VacationDaysRepository @@ -57,7 +58,7 @@ class SQLiteVacationDaysRepository(VacationDaysRepository): item.updated_at.isoformat(), ), ) - await self._conn.commit() + await commit(self._conn) return item async def get_by_id(self, id: UUID) -> VacationDays | None: @@ -83,7 +84,7 @@ class SQLiteVacationDaysRepository(VacationDaysRepository): """, (item.total_days, item.saved_days_in, item.updated_at.isoformat(), str(id)), ) - await self._conn.commit() + await commit(self._conn) if cursor.rowcount == 0: raise NotFoundError(f"VacationDays {id} not found") refreshed = await self.get_by_id(id) @@ -92,7 +93,7 @@ class SQLiteVacationDaysRepository(VacationDaysRepository): async def delete(self, id: UUID) -> None: cursor = await self._conn.execute("DELETE FROM vacation_days WHERE id = ?", (str(id),)) - await self._conn.commit() + await commit(self._conn) if cursor.rowcount == 0: raise NotFoundError(f"VacationDays {id} not found") diff --git a/backend/app/repositories_impl/sqlite/workspace_repository.py b/backend/app/repositories_impl/sqlite/workspace_repository.py index 625d518..355668e 100644 --- a/backend/app/repositories_impl/sqlite/workspace_repository.py +++ b/backend/app/repositories_impl/sqlite/workspace_repository.py @@ -5,6 +5,7 @@ from uuid import UUID import aiosqlite +from app.db.connection import commit from app.entities.workspace import Workspace from app.entities.workspace_invitation import WorkspaceInvitation from app.repositories.filter_validator import validate_filters @@ -42,7 +43,7 @@ class SQLiteWorkspaceRepository(WorkspaceRepository): item.created_at.isoformat(), ), ) - await self._conn.commit() + await commit(self._conn) return item async def get_by_id(self, id: UUID) -> Workspace | None: @@ -55,7 +56,7 @@ class SQLiteWorkspaceRepository(WorkspaceRepository): "UPDATE workspace SET name = ?, planning_mode = ? WHERE id = ?", (item.name, item.planning_mode, str(id)), ) - await self._conn.commit() + await commit(self._conn) if cursor.rowcount == 0: raise NotFoundError(f"Workspace {id} not found") refreshed = await self.get_by_id(id) @@ -64,7 +65,7 @@ class SQLiteWorkspaceRepository(WorkspaceRepository): async def delete(self, id: UUID) -> None: cursor = await self._conn.execute("DELETE FROM workspace WHERE id = ?", (str(id),)) - await self._conn.commit() + await commit(self._conn) if cursor.rowcount == 0: raise NotFoundError(f"Workspace {id} not found") @@ -111,7 +112,7 @@ class SQLiteWorkspaceInvitationRepository(WorkspaceInvitationRepository): item.accepted_at.isoformat() if item.accepted_at else None, ), ) - await self._conn.commit() + await commit(self._conn) return item async def get_by_id(self, id: UUID) -> WorkspaceInvitation | None: @@ -141,7 +142,7 @@ class SQLiteWorkspaceInvitationRepository(WorkspaceInvitationRepository): str(id), ), ) - await self._conn.commit() + await commit(self._conn) if cursor.rowcount == 0: raise NotFoundError(f"WorkspaceInvitation {id} not found") refreshed = await self.get_by_id(id) @@ -152,7 +153,7 @@ class SQLiteWorkspaceInvitationRepository(WorkspaceInvitationRepository): cursor = await self._conn.execute( "DELETE FROM workspace_invitation WHERE id = ?", (str(id),) ) - await self._conn.commit() + await commit(self._conn) if cursor.rowcount == 0: raise NotFoundError(f"WorkspaceInvitation {id} not found") diff --git a/backend/app/routers/analytics.py b/backend/app/routers/analytics.py index 3a52d58..8c8f72f 100644 --- a/backend/app/routers/analytics.py +++ b/backend/app/routers/analytics.py @@ -6,11 +6,12 @@ from fastapi import APIRouter, HTTPException, status from app.dependencies import ( ChildRepositoryDep, - CurrentUserDep, ParentalLeaveRepositoryDep, PersonalDataRepositoryDep, PlannedLeaveRepositoryDep, + WorkspaceIdDep, WorkspaceRepositoryDep, + WorkspaceScopeDep, ) from app.models.analytics import Age4ProjectionResponse, WorkspaceOverviewResponse from app.services.analytics_service import AnalyticsService @@ -31,14 +32,13 @@ def _make_service( @router.get("/{workspace_id}/overview", response_model=WorkspaceOverviewResponse) async def get_overview( - workspace_id: UUID, + workspace_id: WorkspaceIdDep, child_id: UUID, leave_repo: ParentalLeaveRepositoryDep, planned_repo: PlannedLeaveRepositoryDep, child_repo: ChildRepositoryDep, pd_repo: PersonalDataRepositoryDep, workspace_repo: WorkspaceRepositoryDep, - _current_user: CurrentUserDep, ) -> WorkspaceOverviewResponse: service = _make_service(leave_repo, planned_repo, child_repo, pd_repo, workspace_repo) try: @@ -50,14 +50,13 @@ async def get_overview( @router.get("/{workspace_id}/projection", response_model=WorkspaceOverviewResponse) async def get_projection( - workspace_id: UUID, + workspace_id: WorkspaceIdDep, child_id: UUID, leave_repo: ParentalLeaveRepositoryDep, planned_repo: PlannedLeaveRepositoryDep, child_repo: ChildRepositoryDep, pd_repo: PersonalDataRepositoryDep, workspace_repo: WorkspaceRepositoryDep, - _current_user: CurrentUserDep, ) -> WorkspaceOverviewResponse: service = _make_service(leave_repo, planned_repo, child_repo, pd_repo, workspace_repo) try: @@ -78,11 +77,15 @@ async def get_age4_projection( child_repo: ChildRepositoryDep, pd_repo: PersonalDataRepositoryDep, workspace_repo: WorkspaceRepositoryDep, - _current_user: CurrentUserDep, + scope: WorkspaceScopeDep, ) -> Age4ProjectionResponse: + await scope.check_member(personal_data_uuid) + await scope.check_child(child_id) service = _make_service(leave_repo, planned_repo, child_repo, pd_repo, workspace_repo) try: - proj = await service.get_days_left_at_age_4(personal_data_uuid, child_id) + proj = await service.get_days_left_at_age_4( + personal_data_uuid, child_id, workspace_id=scope.workspace_id + ) except NotFoundError as exc: raise HTTPException(status.HTTP_404_NOT_FOUND, str(exc)) from exc return Age4ProjectionResponse.from_entity(proj) diff --git a/backend/app/routers/auth.py b/backend/app/routers/auth.py index 7852110..7b46eb2 100644 --- a/backend/app/routers/auth.py +++ b/backend/app/routers/auth.py @@ -63,7 +63,9 @@ async def logout(response: Response) -> None: @router.post("/refresh", response_model=TokenResponse) +@limiter.limit("30/minute") async def refresh( + request: Request, response: Response, service: AuthServiceDep, refresh_token: str | None = Cookie(default=None, alias=_REFRESH_COOKIE), diff --git a/backend/app/routers/calendar.py b/backend/app/routers/calendar.py index fbeca1a..2533df5 100644 --- a/backend/app/routers/calendar.py +++ b/backend/app/routers/calendar.py @@ -9,7 +9,7 @@ from uuid import UUID from fastapi import APIRouter, HTTPException, Query, status -from app.dependencies import CalendarServiceDep, CurrentUserDep +from app.dependencies import CalendarServiceDep, WorkspaceScopeDep from app.models.calendar import CalendarDaysResponse from app.services.calendar_service import MAX_WINDOW_YEARS from app.utils.errors import ValidationError @@ -22,9 +22,10 @@ async def get_calendar_days( workspace_id: UUID, start_year: int, service: CalendarServiceDep, - _current_user: CurrentUserDep, + scope: WorkspaceScopeDep, years: int = Query(default=MAX_WINDOW_YEARS, ge=1, le=MAX_WINDOW_YEARS), ) -> CalendarDaysResponse: + scope.check_workspace(workspace_id) try: window = await service.get_calendar_window( workspace_id=workspace_id, start_year=start_year, years=years diff --git a/backend/app/routers/day_overrides.py b/backend/app/routers/day_overrides.py index c3550fa..de3a9cc 100644 --- a/backend/app/routers/day_overrides.py +++ b/backend/app/routers/day_overrides.py @@ -8,7 +8,7 @@ from uuid import UUID from fastapi import APIRouter, HTTPException, status -from app.dependencies import DayOverrideServiceDep +from app.dependencies import DayOverrideServiceDep, WorkspaceIdDep, WorkspaceScopeDep from app.models.day_override import DayOverrideCreate, DayOverrideResponse from app.utils.errors import NotFoundError, ValidationError @@ -17,8 +17,9 @@ router = APIRouter(prefix="/api/day-overrides", tags=["day-overrides"]) @router.post("", response_model=DayOverrideResponse, status_code=status.HTTP_201_CREATED) async def upsert_day_override( - payload: DayOverrideCreate, service: DayOverrideServiceDep + payload: DayOverrideCreate, service: DayOverrideServiceDep, scope: WorkspaceScopeDep ) -> DayOverrideResponse: + scope.check_workspace(payload.workspace_id) try: entity = await service.upsert( workspace_id=payload.workspace_id, @@ -33,10 +34,10 @@ async def upsert_day_override( @router.get("/{day_override_id}", response_model=DayOverrideResponse) async def get_day_override( - day_override_id: UUID, service: DayOverrideServiceDep + day_override_id: UUID, service: DayOverrideServiceDep, scope: WorkspaceScopeDep ) -> DayOverrideResponse: try: - entity = await service.get(day_override_id) + entity = await service.get(day_override_id, workspace_id=scope.workspace_id) except NotFoundError as exc: raise HTTPException(status.HTTP_404_NOT_FOUND, str(exc)) from exc return DayOverrideResponse.from_entity(entity) @@ -44,15 +45,17 @@ async def get_day_override( @router.get("/workspace/{workspace_id}", response_model=list[DayOverrideResponse]) async def list_day_overrides( - workspace_id: UUID, service: DayOverrideServiceDep + workspace_id: WorkspaceIdDep, service: DayOverrideServiceDep ) -> list[DayOverrideResponse]: entities = await service.list_for_workspace(workspace_id) return [DayOverrideResponse.from_entity(e) for e in entities] @router.delete("/{day_override_id}", status_code=status.HTTP_204_NO_CONTENT) -async def delete_day_override(day_override_id: UUID, service: DayOverrideServiceDep) -> None: +async def delete_day_override( + day_override_id: UUID, service: DayOverrideServiceDep, scope: WorkspaceScopeDep +) -> None: try: - await service.delete(day_override_id) + await service.delete(day_override_id, workspace_id=scope.workspace_id) except NotFoundError as exc: raise HTTPException(status.HTTP_404_NOT_FOUND, str(exc)) from exc diff --git a/backend/app/routers/fetch_data.py b/backend/app/routers/fetch_data.py index 39bfc1c..bb7417b 100644 --- a/backend/app/routers/fetch_data.py +++ b/backend/app/routers/fetch_data.py @@ -19,7 +19,7 @@ from app.dependencies import ( ChildRepositoryDep, CurrentUserDep, ParentalLeaveRepositoryDep, - PersonalDataRepositoryDep, + WorkspaceScopeDep, ) from app.entities.parental_leave import ParentalLeaveFetch from app.rate_limit import limiter @@ -55,8 +55,11 @@ _scraper_service = FKScraperService() async def save_manual_entry( payload: ManualFetchRequest, repo: ParentalLeaveRepositoryDep, - _current_user: CurrentUserDep, + scope: WorkspaceScopeDep, ) -> ParentalLeaveFetchResponse: + scope.check_workspace(payload.workspace_id) + await scope.check_member(payload.personal_data_uuid) + await scope.check_child(payload.child_id) service = ManualFetchService(repo) try: fetch = await service.save_manual_entry( @@ -84,8 +87,10 @@ async def get_latest( personal_data_uuid: UUID, child_id: UUID, repo: ParentalLeaveRepositoryDep, - _current_user: CurrentUserDep, + scope: WorkspaceScopeDep, ) -> ParentalLeaveFetchResponse | None: + await scope.check_member(personal_data_uuid) + await scope.check_child(child_id) service = ManualFetchService(repo) fetch = await service.get_latest(personal_data_uuid, child_id) return ParentalLeaveFetchResponse.from_entity(fetch) if fetch else None @@ -98,8 +103,10 @@ async def get_history( personal_data_uuid: UUID, child_id: UUID, repo: ParentalLeaveRepositoryDep, - _current_user: CurrentUserDep, + scope: WorkspaceScopeDep, ) -> list[ParentalLeaveFetchResponse]: + await scope.check_member(personal_data_uuid) + await scope.check_child(child_id) service = ManualFetchService(repo) history = await service.list_history(personal_data_uuid, child_id) return [ParentalLeaveFetchResponse.from_entity(f) for f in history] @@ -134,37 +141,33 @@ async def start_fetch( request: Request, payload: _StartRequest, current_user: CurrentUserDep, - pd_repo: PersonalDataRepositoryDep, + scope: WorkspaceScopeDep, child_repo: ChildRepositoryDep, parental_leave_repo: ParentalLeaveRepositoryDep, ) -> _StartResponse: + """`workspace_id` is always the caller's own — `payload.workspace_id` is + accepted on the wire (frontend compatibility) but never trusted; the + caller's own personal_data_uuid/child_id default when omitted, and any + id explicitly supplied is verified against the caller's workspace. + """ if not settings.scraper_enabled: raise _SCRAPER_DISABLED - personal_data_uuid = payload.personal_data_uuid or current_user.personal_data_uuid + workspace_id = scope.workspace_id + personal_data_uuid = payload.personal_data_uuid or scope.personal_data_uuid + await scope.check_member(personal_data_uuid) - if payload.workspace_id is None or payload.child_id is None: - pd = await pd_repo.get_by_id(personal_data_uuid) - if pd is None: + if payload.child_id is not None: + child = await scope.check_child(payload.child_id) + child_id = child.id + else: + children = await child_repo.list_by_workspace(workspace_id) + if not children: raise HTTPException( status.HTTP_422_UNPROCESSABLE_ENTITY, - "No parent profile for this account.", + "No child in workspace — add one before fetching.", ) - workspace_id = payload.workspace_id or pd.workspace_id - - if payload.child_id is None: - children = await child_repo.list_by_workspace(workspace_id) - if not children: - raise HTTPException( - status.HTTP_422_UNPROCESSABLE_ENTITY, - "No child in workspace — add one before fetching.", - ) - child_id = children[0].id - else: - child_id = payload.child_id - else: - workspace_id = payload.workspace_id - child_id = payload.child_id + child_id = children[0].id # Capture the repo for the background task. The connection lives for the app's # lifetime (app.state.db_connection), so using it from the background coroutine @@ -176,6 +179,7 @@ async def start_fetch( workspace_id=workspace_id, personal_data_uuid=personal_data_uuid, child_id=child_id, + owner_user_id=current_user.id, persist_callback=_persist, ) return _StartResponse( @@ -192,11 +196,11 @@ async def start_fetch( async def poll_fetch( request: Request, session_id: str, - _current_user: CurrentUserDep, + current_user: CurrentUserDep, ) -> dict: if not settings.scraper_enabled: raise _SCRAPER_DISABLED - return await _scraper_service.poll_session(session_id) + return await _scraper_service.poll_session(session_id, current_user.id) @scraper_router.post("/cancel") @@ -204,9 +208,9 @@ async def poll_fetch( async def cancel_fetch( request: Request, payload: _CancelRequest, - _current_user: CurrentUserDep, + current_user: CurrentUserDep, ) -> dict: if not settings.scraper_enabled: raise _SCRAPER_DISABLED - await _scraper_service.cancel_session(payload.session_id) + await _scraper_service.cancel_session(payload.session_id, current_user.id) return {"status": "cancelled"} diff --git a/backend/app/routers/leave_wishes.py b/backend/app/routers/leave_wishes.py index 7ee9162..73f0447 100644 --- a/backend/app/routers/leave_wishes.py +++ b/backend/app/routers/leave_wishes.py @@ -9,7 +9,7 @@ from uuid import UUID from fastapi import APIRouter, HTTPException, status -from app.dependencies import LeaveWishServiceDep +from app.dependencies import LeaveWishServiceDep, WorkspaceIdDep, WorkspaceScopeDep from app.models.leave_wish import ( LeaveWishCreate, LeaveWishResponse, @@ -22,8 +22,13 @@ router = APIRouter(prefix="/api/leave-wishes", tags=["leave-wishes"]) @router.post("", response_model=LeaveWishResponse, status_code=status.HTTP_201_CREATED) async def create_leave_wish( - payload: LeaveWishCreate, service: LeaveWishServiceDep + payload: LeaveWishCreate, service: LeaveWishServiceDep, scope: WorkspaceScopeDep ) -> LeaveWishResponse: + scope.check_workspace(payload.workspace_id) + if payload.child_id is not None: + await scope.check_child(payload.child_id) + if payload.parent_personal_data_uuid is not None: + await scope.check_member(payload.parent_personal_data_uuid) try: entity = await service.create( workspace_id=payload.workspace_id, @@ -51,7 +56,7 @@ async def create_leave_wish( @router.get("/{workspace_id}", response_model=list[LeaveWishResponse]) async def list_leave_wishes( - workspace_id: UUID, service: LeaveWishServiceDep + workspace_id: WorkspaceIdDep, service: LeaveWishServiceDep ) -> list[LeaveWishResponse]: """List all leave wishes for a workspace. Matches PLANNING_ENGINE_SPEC.md §4 (`GET /api/leave-wishes/{workspace_id}` — list-by-workspace, not get-by-id).""" @@ -64,11 +69,17 @@ async def update_leave_wish( leave_wish_id: UUID, payload: LeaveWishUpdate, service: LeaveWishServiceDep, + scope: WorkspaceScopeDep, ) -> LeaveWishResponse: + if payload.child_id is not None: + await scope.check_child(payload.child_id) + if payload.parent_personal_data_uuid is not None: + await scope.check_member(payload.parent_personal_data_uuid) fields_set = payload.model_fields_set try: entity = await service.update( leave_wish_id=leave_wish_id, + workspace_id=scope.workspace_id, child_id=payload.child_id, name=payload.name, name_message_key=payload.name_message_key, @@ -104,8 +115,10 @@ async def update_leave_wish( @router.delete("/{leave_wish_id}", status_code=status.HTTP_204_NO_CONTENT) -async def delete_leave_wish(leave_wish_id: UUID, service: LeaveWishServiceDep) -> None: +async def delete_leave_wish( + leave_wish_id: UUID, service: LeaveWishServiceDep, scope: WorkspaceScopeDep +) -> None: try: - await service.delete(leave_wish_id) + await service.delete(leave_wish_id, workspace_id=scope.workspace_id) except NotFoundError as exc: raise HTTPException(status.HTTP_404_NOT_FOUND, str(exc)) from exc diff --git a/backend/app/routers/plan.py b/backend/app/routers/plan.py index 07b62ba..09d3a89 100644 --- a/backend/app/routers/plan.py +++ b/backend/app/routers/plan.py @@ -9,7 +9,7 @@ PLANNING_ENGINE_SPEC.md §4. from fastapi import APIRouter, HTTPException, status -from app.dependencies import PlanningEngineServiceDep, PlanningServiceDep +from app.dependencies import PlanningEngineServiceDep, PlanningServiceDep, WorkspaceScopeDep from app.models.plan import ( PlanApplyRequest, PlanApplyResponse, @@ -25,9 +25,10 @@ router = APIRouter(prefix="/api/plan", tags=["plan"]) @router.post("/propose", response_model=PlanProposalResponse) async def propose_plan( - payload: PlanProposeRequest, service: PlanningEngineServiceDep + payload: PlanProposeRequest, service: PlanningEngineServiceDep, scope: WorkspaceScopeDep ) -> PlanProposalResponse: """Compute a plan proposal. Read-only — persists nothing, safe to spam.""" + scope.check_workspace(payload.workspace_id) try: proposal = await service.propose(payload.workspace_id) except NotFoundError as exc: @@ -38,7 +39,9 @@ async def propose_plan( @router.post("/apply", response_model=PlanApplyResponse) -async def apply_plan(payload: PlanApplyRequest, service: PlanningServiceDep) -> PlanApplyResponse: +async def apply_plan( + payload: PlanApplyRequest, service: PlanningServiceDep, scope: WorkspaceScopeDep +) -> PlanApplyResponse: """Accept a (possibly user-edited) set of proposed days as real plan. Always returns 200 with `accepted: false` and a `rejections`/`collisions` @@ -48,6 +51,15 @@ async def apply_plan(payload: PlanApplyRequest, service: PlanningServiceDep) -> A genuinely missing workspace/child (404) is the one case still raised as an HTTP error, since that's not something the batch report can explain. """ + scope.check_workspace(payload.workspace_id) + # Every child/parent named in the batch must belong to the caller's own + # workspace too — otherwise a forged child_id/parent_personal_data_uuid + # could write a planned_leave_day row attributed to another tenant's + # child or parent even though workspace_id itself checks out. + for child_id in {item.child_id for item in payload.days}: + await scope.check_child(child_id) + for parent_id in {item.parent_personal_data_uuid for item in payload.days}: + await scope.check_member(parent_id) try: result = await service.apply_proposed_days( workspace_id=payload.workspace_id, @@ -60,7 +72,9 @@ async def apply_plan(payload: PlanApplyRequest, service: PlanningServiceDep) -> @router.post("/sync", response_model=PlanSyncResponse) -async def sync_plan(payload: PlanSyncRequest, service: PlanningServiceDep) -> PlanSyncResponse: +async def sync_plan( + payload: PlanSyncRequest, service: PlanningServiceDep, scope: WorkspaceScopeDep +) -> PlanSyncResponse: """Recompute and persist a workspace's plan in one step. The direct-manipulation calendar UX's one call: no separate preview @@ -70,6 +84,7 @@ async def sync_plan(payload: PlanSyncRequest, service: PlanningServiceDep) -> Pl can show e.g. "Sommar 2028 flyttades 2 dagar — Ångra" instead of silently rewriting the calendar. """ + scope.check_workspace(payload.workspace_id) try: result = await service.sync_plan(payload.workspace_id) except NotFoundError as exc: diff --git a/backend/app/routers/planned_days.py b/backend/app/routers/planned_days.py index 3c40631..e0358c9 100644 --- a/backend/app/routers/planned_days.py +++ b/backend/app/routers/planned_days.py @@ -4,7 +4,7 @@ from uuid import UUID from fastapi import APIRouter, HTTPException, Query, status -from app.dependencies import CurrentUserDep, PlanningServiceDep +from app.dependencies import PlanningServiceDep, WorkspaceIdDep, WorkspaceScopeDep from app.models.planned_leave import ( PlannedLeaveDayCreate, PlannedLeaveDayResponse, @@ -23,8 +23,11 @@ def _to_response(day) -> PlannedLeaveDayResponse: async def create_planned_day( payload: PlannedLeaveDayCreate, service: PlanningServiceDep, - _current_user: CurrentUserDep, + scope: WorkspaceScopeDep, ) -> PlannedLeaveDayResponse: + scope.check_workspace(payload.workspace_id) + await scope.check_child(payload.child_id) + await scope.check_member(payload.parent_personal_data_uuid) try: day = await service.add_planned_day( workspace_id=payload.workspace_id, @@ -44,9 +47,10 @@ async def create_planned_day( @router.get("", response_model=list[PlannedLeaveDayResponse]) async def list_planned_days( service: PlanningServiceDep, - _current_user: CurrentUserDep, + scope: WorkspaceScopeDep, child_id: UUID = Query(...), ) -> list[PlannedLeaveDayResponse]: + await scope.check_child(child_id) days = await service.list_by_child(child_id) return [_to_response(d) for d in days] @@ -55,10 +59,10 @@ async def list_planned_days( async def get_planned_day( planned_leave_day_id: UUID, service: PlanningServiceDep, - _current_user: CurrentUserDep, + scope: WorkspaceScopeDep, ) -> PlannedLeaveDayResponse: try: - day = await service.get(planned_leave_day_id) + day = await service.get(planned_leave_day_id, workspace_id=scope.workspace_id) return _to_response(day) except NotFoundError as exc: raise HTTPException(status.HTTP_404_NOT_FOUND, str(exc)) from exc @@ -66,9 +70,8 @@ async def get_planned_day( @router.get("/workspace/{workspace_id}", response_model=list[PlannedLeaveDayResponse]) async def list_planned_days_by_workspace( - workspace_id: UUID, + workspace_id: WorkspaceIdDep, service: PlanningServiceDep, - _current_user: CurrentUserDep, ) -> list[PlannedLeaveDayResponse]: days = await service.list_by_workspace(workspace_id) return [_to_response(d) for d in days] @@ -79,7 +82,7 @@ async def update_planned_day( planned_leave_day_id: UUID, payload: PlannedLeaveDayUpdate, service: PlanningServiceDep, - _current_user: CurrentUserDep, + scope: WorkspaceScopeDep, ) -> PlannedLeaveDayResponse: try: day = await service.update( @@ -88,6 +91,7 @@ async def update_planned_day( is_dubbeldag=payload.is_dubbeldag, wish_id=payload.wish_id, is_pinned=payload.is_pinned, + workspace_id=scope.workspace_id, ) return _to_response(day) except NotFoundError as exc: @@ -98,9 +102,9 @@ async def update_planned_day( async def delete_planned_day( planned_leave_day_id: UUID, service: PlanningServiceDep, - _current_user: CurrentUserDep, + scope: WorkspaceScopeDep, ) -> None: try: - await service.remove_planned_day(planned_leave_day_id) + await service.remove_planned_day(planned_leave_day_id, workspace_id=scope.workspace_id) except NotFoundError as exc: raise HTTPException(status.HTTP_404_NOT_FOUND, str(exc)) from exc diff --git a/backend/app/routers/settings.py b/backend/app/routers/settings.py index 8eb864a..db26be7 100644 --- a/backend/app/routers/settings.py +++ b/backend/app/routers/settings.py @@ -14,9 +14,10 @@ from fastapi import APIRouter, HTTPException, status from app.dependencies import ( ChildServiceDep, - CurrentUserDep, PersonalDataServiceDep, TipsServiceDep, + WorkspaceIdDep, + WorkspaceScopeDep, ) from app.models.child import ChildCreate, ChildResponse, ChildUpdate from app.models.personal_data import PersonalDataResponse, PersonalDataUpdate @@ -35,9 +36,8 @@ tips_router = APIRouter(prefix="/api/tips", tags=["tips"]) @tips_router.get("/{workspace_id}", response_model=list[TipResponse]) async def get_tips( - workspace_id: UUID, + workspace_id: WorkspaceIdDep, service: TipsServiceDep, - _current_user: CurrentUserDep, ) -> list[TipResponse]: tips = await service.get_tips(workspace_id) return [TipResponse.from_domain(tip) for tip in tips] @@ -45,10 +45,9 @@ async def get_tips( @tips_router.post("/{workspace_id}/dismissals", status_code=status.HTTP_204_NO_CONTENT) async def dismiss_tip( - workspace_id: UUID, + workspace_id: WorkspaceIdDep, payload: TipDismissalCreate, service: TipsServiceDep, - _current_user: CurrentUserDep, ) -> None: try: await service.dismiss(workspace_id, payload.tip_id) @@ -71,8 +70,9 @@ def _child_to_response(child) -> ChildResponse: async def create_child( payload: ChildCreate, service: ChildServiceDep, - _current_user: CurrentUserDep, + scope: WorkspaceScopeDep, ) -> ChildResponse: + scope.check_workspace(payload.workspace_id) try: child = await service.create( workspace_id=payload.workspace_id, @@ -90,10 +90,10 @@ async def create_child( async def get_child( child_id: UUID, service: ChildServiceDep, - _current_user: CurrentUserDep, + scope: WorkspaceScopeDep, ) -> ChildResponse: try: - child = await service.get(child_id) + child = await service.get(child_id, workspace_id=scope.workspace_id) return _child_to_response(child) except NotFoundError as exc: raise HTTPException(status.HTTP_404_NOT_FOUND, str(exc)) from exc @@ -101,9 +101,8 @@ async def get_child( @child_router.get("/workspace/{workspace_id}", response_model=list[ChildResponse]) async def list_children( - workspace_id: UUID, + workspace_id: WorkspaceIdDep, service: ChildServiceDep, - _current_user: CurrentUserDep, ) -> list[ChildResponse]: children = await service.list_by_workspace(workspace_id) return [_child_to_response(c) for c in children] @@ -118,7 +117,7 @@ async def update_child( child_id: UUID, payload: ChildUpdate, service: ChildServiceDep, - _current_user: CurrentUserDep, + scope: WorkspaceScopeDep, ) -> ChildResponse: try: child = await service.update( @@ -127,6 +126,7 @@ async def update_child( birth_date=payload.birth_date, preschool_start_date=payload.preschool_start_date, preschool_planning_days_per_year=payload.preschool_planning_days_per_year, + workspace_id=scope.workspace_id, ) return _child_to_response(child) except ValidationError as exc: @@ -139,10 +139,10 @@ async def update_child( async def delete_child( child_id: UUID, service: ChildServiceDep, - _current_user: CurrentUserDep, + scope: WorkspaceScopeDep, ) -> None: try: - await service.delete(child_id) + await service.delete(child_id, workspace_id=scope.workspace_id) except NotFoundError as exc: raise HTTPException(status.HTTP_404_NOT_FOUND, str(exc)) from exc @@ -152,7 +152,7 @@ async def update_parent_settings( personal_data_uuid: UUID, payload: PersonalDataUpdate, service: PersonalDataServiceDep, - _current_user: CurrentUserDep, + scope: WorkspaceScopeDep, ) -> PersonalDataResponse: try: updated = await service.update( @@ -160,6 +160,7 @@ async def update_parent_settings( salary_type=payload.salary_type, employment_status=payload.employment_status, min_vacation_days_per_year=payload.min_vacation_days_per_year, + workspace_id=scope.workspace_id, ) return PersonalDataResponse.from_entity(updated) except ValidationError as exc: diff --git a/backend/app/routers/vacation_days.py b/backend/app/routers/vacation_days.py index 295ca18..b03abe3 100644 --- a/backend/app/routers/vacation_days.py +++ b/backend/app/routers/vacation_days.py @@ -8,7 +8,7 @@ from uuid import UUID from fastapi import APIRouter, HTTPException, status -from app.dependencies import VacationDaysServiceDep +from app.dependencies import VacationDaysServiceDep, WorkspaceIdDep, WorkspaceScopeDep from app.models.vacation_days import VacationDaysCreate, VacationDaysResponse from app.utils.errors import NotFoundError, ValidationError @@ -17,8 +17,10 @@ router = APIRouter(prefix="/api/vacation-days", tags=["vacation-days"]) @router.post("", response_model=VacationDaysResponse, status_code=status.HTTP_201_CREATED) async def upsert_vacation_days( - payload: VacationDaysCreate, service: VacationDaysServiceDep + payload: VacationDaysCreate, service: VacationDaysServiceDep, scope: WorkspaceScopeDep ) -> VacationDaysResponse: + scope.check_workspace(payload.workspace_id) + await scope.check_member(payload.personal_data_uuid) try: entity = await service.upsert( personal_data_uuid=payload.personal_data_uuid, @@ -34,10 +36,10 @@ async def upsert_vacation_days( @router.get("/{vacation_days_id}", response_model=VacationDaysResponse) async def get_vacation_days( - vacation_days_id: UUID, service: VacationDaysServiceDep + vacation_days_id: UUID, service: VacationDaysServiceDep, scope: WorkspaceScopeDep ) -> VacationDaysResponse: try: - entity = await service.get(vacation_days_id) + entity = await service.get(vacation_days_id, workspace_id=scope.workspace_id) except NotFoundError as exc: raise HTTPException(status.HTTP_404_NOT_FOUND, str(exc)) from exc return VacationDaysResponse.from_entity(entity) @@ -45,7 +47,7 @@ async def get_vacation_days( @router.get("/workspace/{workspace_id}", response_model=list[VacationDaysResponse]) async def list_vacation_days( - workspace_id: UUID, service: VacationDaysServiceDep + workspace_id: WorkspaceIdDep, service: VacationDaysServiceDep ) -> list[VacationDaysResponse]: entities = await service.list_for_workspace(workspace_id) return [VacationDaysResponse.from_entity(e) for e in entities] diff --git a/backend/app/routers/workspace.py b/backend/app/routers/workspace.py index 6717f46..d2360ab 100644 --- a/backend/app/routers/workspace.py +++ b/backend/app/routers/workspace.py @@ -4,7 +4,12 @@ from uuid import UUID from fastapi import APIRouter, HTTPException, status -from app.dependencies import CurrentUserDep, WorkspaceServiceDep +from app.dependencies import ( + CurrentUserDep, + CurrentWorkspaceIdDep, + WorkspaceIdDep, + WorkspaceServiceDep, +) from app.entities.personal_data import PersonalData from app.models.workspace import ( InvitationResponse, @@ -62,9 +67,8 @@ async def create_workspace( @router.get("/{workspace_id}", response_model=WorkspaceResponse) async def get_workspace( - workspace_id: UUID, + workspace_id: WorkspaceIdDep, service: WorkspaceServiceDep, - _current_user: CurrentUserDep, ) -> WorkspaceResponse: try: ws = await service.get_workspace(workspace_id) @@ -75,10 +79,9 @@ async def get_workspace( @router.put("/{workspace_id}", response_model=WorkspaceResponse) async def update_workspace( - workspace_id: UUID, + workspace_id: WorkspaceIdDep, payload: WorkspaceUpdate, service: WorkspaceServiceDep, - _current_user: CurrentUserDep, ) -> WorkspaceResponse: try: ws = await service.update_workspace(workspace_id, payload.name, payload.planning_mode) @@ -94,9 +97,8 @@ async def update_workspace( @router.get("/{workspace_id}/members", response_model=list[PersonalDataResponse]) async def list_members( - workspace_id: UUID, + workspace_id: WorkspaceIdDep, service: WorkspaceServiceDep, - _current_user: CurrentUserDep, ) -> list[PersonalDataResponse]: members = await service.list_members(workspace_id) return [_to_response(m) for m in members] @@ -108,10 +110,9 @@ async def list_members( status_code=status.HTTP_201_CREATED, ) async def add_placeholder_parent( - workspace_id: UUID, + workspace_id: WorkspaceIdDep, payload: PersonalDataCreate, service: WorkspaceServiceDep, - _current_user: CurrentUserDep, ) -> PersonalDataResponse: try: pd = await service.add_placeholder_parent(workspace_id, payload.display_name) @@ -125,11 +126,10 @@ async def add_placeholder_parent( response_model=PersonalDataResponse, ) async def rename_parent( - workspace_id: UUID, + workspace_id: WorkspaceIdDep, personal_data_uuid: UUID, payload: PersonalDataCreate, service: WorkspaceServiceDep, - _current_user: CurrentUserDep, ) -> PersonalDataResponse: try: pd = await service.rename_member(workspace_id, personal_data_uuid, payload.display_name) @@ -143,7 +143,7 @@ async def rename_parent( status_code=status.HTTP_204_NO_CONTENT, ) async def remove_parent( - workspace_id: UUID, + workspace_id: WorkspaceIdDep, personal_data_uuid: UUID, service: WorkspaceServiceDep, current_user: CurrentUserDep, @@ -162,7 +162,7 @@ async def remove_parent( status_code=status.HTTP_201_CREATED, ) async def create_invitation( - workspace_id: UUID, + workspace_id: WorkspaceIdDep, service: WorkspaceServiceDep, current_user: CurrentUserDep, ) -> InviteResponse: @@ -194,10 +194,10 @@ async def redeem_invitation( async def revoke_invitation( invitation_id: UUID, service: WorkspaceServiceDep, - _current_user: CurrentUserDep, + workspace_id: CurrentWorkspaceIdDep, ) -> None: try: - await service.revoke_invitation(invitation_id) + await service.revoke_invitation(invitation_id, workspace_id) except (NotFoundError, ValidationError) as exc: code = ( status.HTTP_404_NOT_FOUND diff --git a/backend/app/scrapers/fk_scraper.py b/backend/app/scrapers/fk_scraper.py index e2d9ea7..83c4aa3 100644 --- a/backend/app/scrapers/fk_scraper.py +++ b/backend/app/scrapers/fk_scraper.py @@ -4,10 +4,17 @@ Legal/ToS review completed (2026-06). Legal has cleared automated read-only access for users who have given explicit per-session consent. Operational guardrails must be maintained (see §2.3): - Sandboxed browser context, torn down after each session. - - No cookies, session tokens, or QR images ever persisted to disk or DB. - - No personal data (names, personnummer, day counts) in logs. + - No cookies, session tokens, or QR images ever persisted to disk or DB by + default. Debug artifacts (HTML + screenshot of the logged-in page) are + OFF unless both scraper_debug_enabled and scraper_debug_dir are set — + they contain personal data and are for local debugging only. + - No personal data (names, personnummer, day counts, raw page text) in + logs above DEBUG; DEBUG lines log shapes/counts, never scraped values. - Per-session user consent required before starting a session. - Only the extracted day-count numbers are persisted (see persist_callback). + - QR frames and results are retained in RAM only for + scraper_result_retention_seconds after a session ends, then purged; at + most one live session per user. Enable via APP_SCRAPER_ENABLED=true in the environment or backend/.env. @@ -94,6 +101,10 @@ class _SessionState: workspace_id: UUID personal_data_uuid: UUID child_id: UUID + # The User.id that started this session. `poll_session`/`cancel_session` + # treat a mismatched owner exactly like an unknown session_id — never a + # 403, so a foreign session_id can't be distinguished from a made-up one. + owner_user_id: UUID status: SessionStatus = "launching" qr_data: bytes | None = None # raw PNG bytes; never persisted to disk/DB result: ParentalLeaveFetch | None = None @@ -146,19 +157,35 @@ class FKScraperService(FetchDataService): workspace_id: UUID, personal_data_uuid: UUID, child_id: UUID, + owner_user_id: UUID, persist_callback: Callable[[ParentalLeaveFetch], Awaitable[None]] | None = None, ) -> str: """Launch a new headless browser session. Returns the session_id. persist_callback, if provided, is called with the validated ParentalLeaveFetch once extraction succeeds. Failure in the callback sets the session to 'error'. + `owner_user_id` binds the session to the caller that started it — see + `_SessionState.owner_user_id`. + + At most one live (non-terminal) session per `owner_user_id` — starting + a new one cancels any session already in flight for the same caller + (see F5: unbounded growth of `_sessions` per owner). """ + for existing_id, existing_state in list(_sessions.items()): + if existing_state.owner_user_id == owner_user_id and existing_state.status not in ( + "done", + "error", + "cancelled", + ): + await self.cancel_session(existing_id, owner_user_id) + session_id = str(uuid.uuid4()) state = _SessionState( session_id=session_id, workspace_id=workspace_id, personal_data_uuid=personal_data_uuid, child_id=child_id, + owner_user_id=owner_user_id, status="launching", persist_callback=persist_callback, ) @@ -172,10 +199,15 @@ class FKScraperService(FetchDataService): logger.info("scraper session started [id=%s]", session_id) return session_id - async def poll_session(self, session_id: str) -> dict: - """Return current status, QR bytes (base64), or result.""" + async def poll_session(self, session_id: str, owner_user_id: UUID) -> dict: + """Return current status, QR bytes (base64), or result. + + A `session_id` owned by a different caller is treated exactly like + an unknown one — never a 403, so a foreign session_id can't be + distinguished from a made-up one. + """ state = _sessions.get(session_id) - if state is None: + if state is None or state.owner_user_id != owner_user_id: return {"status": "error", "message": "Unknown session_id"} response: dict = {"status": state.status} @@ -187,12 +219,17 @@ class FKScraperService(FetchDataService): response["message"] = state.message return response - async def cancel_session(self, session_id: str) -> None: - """Cancel a running session and tear down the browser.""" + async def cancel_session(self, session_id: str, owner_user_id: UUID) -> None: + """Cancel a running session and tear down the browser. + + A `session_id` owned by a different caller is a no-op, exactly like + an unknown one — never a 403 (see `poll_session`). + """ state = _sessions.get(session_id) - if state is None: + if state is None or state.owner_user_id != owner_user_id: return _set_status(state, "cancelled") + state.qr_data = None if state._task and not state._task.done(): state._task.cancel() logger.info("scraper session cancelled [id=%s]", session_id) @@ -209,15 +246,50 @@ def _set_status(state: _SessionState, status: SessionStatus, message: str | None state.message = message +def _generic_error_message(exc: Exception) -> str: + """Map an internal exception to a fixed, generic message safe to return + to the poll client (see F4). Never embeds `str(exc)` — a `ValueError` + raised while parsing the FK page may otherwise carry raw page text. + Logs carry the exception type name only; full detail is never emitted + above DEBUG and is not persisted unless debug artifacts are explicitly + enabled (`scraper_debug_enabled`). + """ + if isinstance(exc, ValueError): + return "Scraper error: could not read data from the FK page. Please try again." + if type(exc).__name__ == "TimeoutError": + return "Scraper error: timed out waiting for the FK page. Please try again." + return "Scraper error: an unexpected error occurred. Please try again." + + async def _ttl_watchdog(session_id: str) -> None: - """Auto-cancel sessions that sit in awaiting_scan/authenticating too long.""" + """Auto-cancel sessions that sit in awaiting_scan/authenticating too long. + + Also acts as a secondary safety net (F5): if the session has already + reached a terminal state by the time this fires, purge it immediately + rather than rely solely on `_purge_later`. + """ await asyncio.sleep(settings.scraper_session_ttl_seconds) state = _sessions.get(session_id) - if state and state.status in ("awaiting_scan", "authenticating", "launching"): + if state is None: + return + if state.status in ("awaiting_scan", "authenticating", "launching"): _set_status(state, "error", "Session timed out waiting for BankID scan") + state.qr_data = None if state._task and not state._task.done(): state._task.cancel() logger.info("scraper session timed out [id=%s]", session_id) + elif state.status in ("done", "error", "cancelled"): + _sessions.pop(session_id, None) + + +async def _purge_later(session_id: str) -> None: + """Remove a finished session from `_sessions` after the retention window. + + Bounds how long a scraped result or a stale (already-cleared) QR frame + slot can be read back via `/poll` after the session ended (see F5). + """ + await asyncio.sleep(settings.scraper_result_retention_seconds) + _sessions.pop(session_id, None) async def _check_maintenance(page) -> str | None: @@ -243,9 +315,14 @@ async def _check_maintenance(page) -> str | None: async def _save_debug_artifacts(page, session_id: str, console_messages: list[str]) -> None: - """Save HTML, screenshot, and console log to scraper_debug_dir on error.""" + """Save HTML, screenshot, and console log to scraper_debug_dir on error. + + OFF by default and requires both `scraper_debug_enabled` and + `scraper_debug_dir` to be set — these artifacts contain personal data + (name, balances, payout amounts) from the logged-in FK page. + """ debug_dir = settings.scraper_debug_dir - if not debug_dir: + if not settings.scraper_debug_enabled or not debug_dir: return try: out = Path(debug_dir) @@ -289,8 +366,10 @@ async def _run_session(state: _SessionState) -> None: except asyncio.CancelledError: _set_status(state, "cancelled") except Exception as exc: - _set_status(state, "error", f"Scraper error: {type(exc).__name__}: {exc}") - logger.error("scraper error [id=%s]: %s", state.session_id, exc, exc_info=False) + _set_status(state, "error", _generic_error_message(exc)) + logger.error( + "scraper error [id=%s]: %s", state.session_id, type(exc).__name__, exc_info=False + ) await _save_debug_artifacts(page, state.session_id, console_messages) finally: try: @@ -299,6 +378,10 @@ async def _run_session(state: _SessionState) -> None: except Exception: pass # already torn down + # Session has reached a terminal state (done/error/cancelled) — schedule + # its removal from `_sessions` after the retention window (see F5). + asyncio.create_task(_purge_later(state.session_id)) + async def _do_login(page, state: _SessionState) -> None: """Navigate to FK login, handle cookie consent, click BankID QR, relay QR.""" @@ -372,6 +455,10 @@ async def _do_login(page, state: _SessionState) -> None: logger.debug("[%s] QR element gone; BankID app likely confirming", state.session_id) break + # Leaving awaiting_scan (scanned, cancelled, or timed out) — the last QR + # frame is no longer needed and must not linger in RAM (see F5). + state.qr_data = None + if state.status == "cancelled": return @@ -440,14 +527,14 @@ async def _extract_parental_leave(page, state: _SessionState) -> ParentalLeaveFe logger.debug("[%s] reading days_left (FOR_DIG_DAGAR_KVAR)", state.session_id) days_left_text = await page.locator(_SEL.FOR_DIG_DAGAR_KVAR).first.inner_text(timeout=5_000) days_left = int(days_left_text.strip()) - logger.debug("[%s] days_left=%d", state.session_id, days_left) + logger.debug("[%s] days_left parsed", state.session_id) # -- total days from "av 240 dagar kvar efter ansökan" -- logger.debug("[%s] reading total days (FOR_DIG_DISCRETE)", state.session_id) discrete_text = await page.locator(_SEL.FOR_DIG_DISCRETE).first.inner_text(timeout=5_000) total_match = re.search(r"av\s+(\d+)\s+dagar", discrete_text) if not total_match: - raise ValueError(f"Could not parse total days from: {discrete_text!r}") + raise ValueError("Could not parse total days from FK page text") total_days = int(total_match.group(1)) if total_days == 0: raise ValueError( @@ -455,14 +542,14 @@ async def _extract_parental_leave(page, state: _SessionState) -> ParentalLeaveFe "(placeholder state). The hydration wait in _do_extract should prevent this." ) days_used = total_days - days_left - logger.debug("[%s] total_days=%d days_used=%d", state.session_id, total_days, days_used) + logger.debug("[%s] total/used parsed", state.session_id) # -- sjukpenningnivå and lägstanivå remaining (desktop niva-statistik) -- # all_inner_texts() does not accept a timeout argument — wait for the first element first. logger.debug("[%s] reading tier breakdown (NIVA_STATISTIK_DESKTOP)", state.session_id) await page.locator(_SEL.NIVA_STATISTIK_DESKTOP).first.wait_for(timeout=5_000) niva_texts = await page.locator(_SEL.NIVA_STATISTIK_DESKTOP).all_inner_texts() - logger.debug("[%s] niva_texts raw=%r", state.session_id, niva_texts) + logger.debug("[%s] niva items=%d", state.session_id, len(niva_texts)) # Each item is like "(119 dagar)" — filter to desktop section (first two) niva_numbers = [] for t in niva_texts: @@ -473,31 +560,20 @@ async def _extract_parental_leave(page, state: _SessionState) -> ParentalLeaveFe break if len(niva_numbers) < 2: - raise ValueError(f"Expected 2 tier values, got {niva_numbers} from {niva_texts!r}") + raise ValueError(f"Expected 2 tier values, got {len(niva_numbers)}") sjukpenning_left, lagsta_niva_left = niva_numbers[0], niva_numbers[1] - logger.debug( - "[%s] sjukpenning_left=%d lagsta_niva_left=%d", - state.session_id, - sjukpenning_left, - lagsta_niva_left, - ) + logger.debug("[%s] tier breakdown parsed", state.session_id) # Cross-check: sjukpenning_left + lagsta_niva_left should equal days_left if sjukpenning_left + lagsta_niva_left != days_left: - logger.warning( - "Tier sum mismatch: sjukpenning=%d + lagstaniva=%d != days_left=%d [id=%s]", - sjukpenning_left, - lagsta_niva_left, - days_left, - state.session_id, - ) + logger.warning("Tier sum mismatch [id=%s]", state.session_id) # -- dubbeldagar remaining -- logger.debug("[%s] reading dubbeldagar (DUBBELDAG_KVAR)", state.session_id) dubbeldag_text = await page.locator(_SEL.DUBBELDAG_KVAR).first.inner_text(timeout=5_000) dubbeldagar_left = int(dubbeldag_text.strip()) - logger.debug("[%s] dubbeldagar_left=%d", state.session_id, dubbeldagar_left) + logger.debug("[%s] dubbeldagar parsed", state.session_id) # -- Derive tier totals (standard FK allocation per parent) -- # Standard: 195 sjukpenningnivå + 45 lägstanivå = 240 per parent. diff --git a/backend/app/services/analytics_service.py b/backend/app/services/analytics_service.py index 74eaa96..e8e6b3b 100644 --- a/backend/app/services/analytics_service.py +++ b/backend/app/services/analytics_service.py @@ -92,13 +92,17 @@ class AnalyticsService: return resolve_age4_savable_cap(planning_mode, len(members)) async def get_days_left_at_age_4( - self, personal_data_uuid: UUID, child_id: UUID + self, personal_data_uuid: UUID, child_id: UUID, workspace_id: UUID | None = None ) -> Age4Projection: - """Compute the headline projection for a single parent+child pair.""" + """Compute the headline projection for a single parent+child pair. + + `workspace_id`, when given, must match the child's own — a foreign + workspace's child 404s exactly like a nonexistent one. + """ rules = settings.rules child = await self._children.get_by_id(child_id) - if child is None: + if child is None or (workspace_id is not None and child.workspace_id != workspace_id): raise NotFoundError(f"Child {child_id} not found") # Compute the 4th birthday with leap-year-safe arithmetic. diff --git a/backend/app/services/auth_service.py b/backend/app/services/auth_service.py index 53914ba..15305a9 100644 --- a/backend/app/services/auth_service.py +++ b/backend/app/services/auth_service.py @@ -11,6 +11,7 @@ TODO (post-MVP): password reset + email verification flows are not implemented. from __future__ import annotations +import asyncio from datetime import datetime, timedelta, timezone from uuid import UUID, uuid4 @@ -23,12 +24,19 @@ from app.entities.personal_data import PersonalData from app.entities.user import User from app.entities.workspace import Workspace from app.repositories.personal_data_repository import PersonalDataRepository +from app.repositories.unit_of_work import NoOpUnitOfWork, UnitOfWork from app.repositories.user_repository import UserRepository from app.repositories.workspace_repository import WorkspaceRepository from app.utils.errors import AuthError, ConflictError, UnauthorizedError _ph = PasswordHasher() +# Precomputed at import time so a login against an unknown email still pays the +# cost of an argon2 verify (against a hash nobody's password can match) — this +# keeps the response-time profile of "unknown email" indistinguishable from +# "known email, wrong password" and closes the user-enumeration timing oracle. +_DUMMY_HASH = _ph.hash(uuid4().hex) + class AuthService: def __init__( @@ -36,10 +44,16 @@ class AuthService: user_repo: UserRepository, personal_data_repo: PersonalDataRepository, workspace_repo: WorkspaceRepository, + uow: UnitOfWork | None = None, ) -> None: self._users = user_repo self._personal_data = personal_data_repo self._workspaces = workspace_repo + # Defaults to a no-op so pre-existing direct-construction call sites + # (scripts/seed_demo.py, test fixtures) keep today's per-write-commit + # behavior; `app.dependencies.get_auth_service` always wires a real + # `SQLiteUnitOfWork`. + self._uow = uow or NoOpUnitOfWork() async def register( self, email: str, password: str, display_name: str @@ -47,45 +61,66 @@ class AuthService: """Create User + personal workspace + PersonalData. Raises ConflictError if email taken.""" existing = await self._users.get_by_email(email) if existing is not None: - raise ConflictError(f"Email already registered: {email}") + raise ConflictError("Email already registered") - # Create a personal workspace for the registering user. - workspace = Workspace( - id=uuid4(), - name=f"{display_name}s planering", - created_at=datetime.now(), - ) - workspace = await self._workspaces.create(workspace) + # Hash before opening the unit of work: argon2 is CPU work + # (offloaded via to_thread), not a DB write — no reason to hold the + # write lock for it. + hashed_password = await asyncio.to_thread(_ph.hash, password) - pd = PersonalData( - uuid=uuid4(), - workspace_id=workspace.id, - name=display_name, - created_at=datetime.now(), - ) - pd = await self._personal_data.create(pd) + # Workspace + PersonalData + User must land together: a crash between + # any two of these would otherwise leave an orphaned workspace, a + # personal_data row with no login, or (with the FK ON DELETE CASCADE) + # a user pointing at nothing. + async with self._uow(): + # Create a personal workspace for the registering user. + workspace = Workspace( + id=uuid4(), + name=f"{display_name}s planering", + created_at=datetime.now(), + ) + workspace = await self._workspaces.create(workspace) - user = User( - id=uuid4(), - personal_data_uuid=pd.uuid, - email=email, - hashed_password=_ph.hash(password), - created_at=datetime.now(), - ) - user = await self._users.create(user) + pd = PersonalData( + uuid=uuid4(), + workspace_id=workspace.id, + name=display_name, + created_at=datetime.now(), + ) + pd = await self._personal_data.create(pd) + + user = User( + id=uuid4(), + personal_data_uuid=pd.uuid, + email=email, + hashed_password=hashed_password, + created_at=datetime.now(), + ) + user = await self._users.create(user) return user, pd async def login(self, email: str, password: str) -> User: - """Verify credentials. Raises AuthError on wrong email or password.""" + """Verify credentials. Raises AuthError on wrong email/password/inactive account. + + Always uses a generic message so unknown-email and wrong-password are + indistinguishable, and always runs an argon2 verify (real or dummy) so + the two cases take comparable time. + """ user = await self._users.get_by_email(email) if user is None: + try: + await asyncio.to_thread(_ph.verify, _DUMMY_HASH, password) + except VerifyMismatchError: + pass raise AuthError("Invalid email or password") try: - _ph.verify(user.hashed_password, password) + await asyncio.to_thread(_ph.verify, user.hashed_password, password) except VerifyMismatchError as exc: raise AuthError("Invalid email or password") from exc + if not user.is_active: + raise UnauthorizedError("Account is inactive") if _ph.check_needs_rehash(user.hashed_password): - user.hashed_password = _ph.hash(password) + user.hashed_password = await asyncio.to_thread(_ph.hash, password) await self._users.update(user.id, user) return user @@ -117,6 +152,8 @@ class AuthService: user = await self._users.get_by_id(user_id) if user is None: raise UnauthorizedError("User not found") + if not user.is_active: + raise UnauthorizedError("Account is inactive") return user async def refresh_access_token(self, refresh_token: str) -> str: diff --git a/backend/app/services/child_service.py b/backend/app/services/child_service.py index 8ceebd9..3ce03d9 100644 --- a/backend/app/services/child_service.py +++ b/backend/app/services/child_service.py @@ -37,10 +37,12 @@ class ChildService: ) return await self._repo.create(entity) - async def get(self, child_id: UUID) -> Child: - """Get a child by ID.""" + async def get(self, child_id: UUID, workspace_id: UUID | None = None) -> Child: + """Get a child by ID. `workspace_id`, when given, must match the + child's own — a foreign workspace's child 404s exactly like a + nonexistent one.""" entity = await self._repo.get_by_id(child_id) - if entity is None: + if entity is None or (workspace_id is not None and entity.workspace_id != workspace_id): raise NotFoundError(f"Child {child_id} not found") return entity @@ -55,9 +57,10 @@ class ChildService: birth_date=None, preschool_start_date=None, preschool_planning_days_per_year: int | None = None, + workspace_id: UUID | None = None, ) -> Child: """Update a child (partial update).""" - entity = await self.get(child_id) + entity = await self.get(child_id, workspace_id) if name is not None: entity.name = name @@ -78,9 +81,9 @@ class ChildService: return await self._repo.update(child_id, entity) - async def delete(self, child_id: UUID) -> None: + async def delete(self, child_id: UUID, workspace_id: UUID | None = None) -> None: """Delete a child.""" - await self.get(child_id) # raises NotFoundError if missing + await self.get(child_id, workspace_id) # raises NotFoundError if missing/foreign await self._repo.delete(child_id) diff --git a/backend/app/services/day_override_service.py b/backend/app/services/day_override_service.py index 0c75bbf..39421e0 100644 --- a/backend/app/services/day_override_service.py +++ b/backend/app/services/day_override_service.py @@ -49,16 +49,17 @@ class DayOverrideService: ) return await self._repo.create(entity) - async def get(self, day_override_id: UUID) -> DayOverride: + async def get(self, day_override_id: UUID, workspace_id: UUID | None = None) -> DayOverride: entity = await self._repo.get_by_id(day_override_id) - if entity is None: + if entity is None or (workspace_id is not None and entity.workspace_id != workspace_id): raise NotFoundError(f"DayOverride {day_override_id} not found") return entity async def list_for_workspace(self, workspace_id: UUID) -> list[DayOverride]: return await self._repo.list_by_workspace(workspace_id) - async def delete(self, day_override_id: UUID) -> None: + async def delete(self, day_override_id: UUID, workspace_id: UUID | None = None) -> None: + await self.get(day_override_id, workspace_id) # raises NotFoundError if missing/foreign await self._repo.delete(day_override_id) async def get_effective_map(self, workspace_id: UUID) -> dict[date, bool]: diff --git a/backend/app/services/leave_wish_service.py b/backend/app/services/leave_wish_service.py index 919f02b..ebfbebd 100644 --- a/backend/app/services/leave_wish_service.py +++ b/backend/app/services/leave_wish_service.py @@ -89,9 +89,15 @@ class LeaveWishService: ) return await self._repo.create(entity) - async def get(self, leave_wish_id: UUID) -> LeaveWish: + async def get(self, leave_wish_id: UUID, workspace_id: UUID | None = None) -> LeaveWish: + """`workspace_id`, when given, must match the entity's own — callers + that already know the caller's tenant (routers) pass it so a foreign + workspace's wish 404s exactly like a nonexistent one; internal + callers that don't need that check (e.g. `update`'s own re-reads + with no tenant context) leave it `None`. + """ entity = await self._repo.get_by_id(leave_wish_id) - if entity is None: + if entity is None or (workspace_id is not None and entity.workspace_id != workspace_id): raise NotFoundError(f"LeaveWish {leave_wish_id} not found") return entity @@ -101,6 +107,7 @@ class LeaveWishService: async def update( self, leave_wish_id: UUID, + workspace_id: UUID | None = None, child_id: UUID | None = None, name: str | None = None, name_message_key: str | None = None, @@ -138,7 +145,7 @@ class LeaveWishService: `_clear_target_days` resets a day-granular wish back to being sized by `target_weeks` alone. """ - entity = await self.get(leave_wish_id) + entity = await self.get(leave_wish_id, workspace_id) original_name = entity.name if _clear_child_id: @@ -202,7 +209,7 @@ class LeaveWishService: ) return await self._repo.update(leave_wish_id, entity) - async def delete(self, leave_wish_id: UUID) -> None: + async def delete(self, leave_wish_id: UUID, workspace_id: UUID | None = None) -> None: """Delete a wish and every `planned_leave_day` row it produced. Under the calendar-direct-manipulation UX a period *is* its wish: the @@ -224,7 +231,7 @@ class LeaveWishService: deletes every day in the band regardless of `is_pinned` with no special-casing. """ - await self.get(leave_wish_id) # raises NotFoundError if missing + await self.get(leave_wish_id, workspace_id) # raises NotFoundError if missing/foreign await self._planned_leave_repo.delete_by_wish(leave_wish_id) await self._repo.delete(leave_wish_id) diff --git a/backend/app/services/personal_data_service.py b/backend/app/services/personal_data_service.py index 7a07da7..a10110a 100644 --- a/backend/app/services/personal_data_service.py +++ b/backend/app/services/personal_data_service.py @@ -21,9 +21,9 @@ class PersonalDataService: def __init__(self, repo: PersonalDataRepository) -> None: self._repo = repo - async def get(self, personal_data_uuid: UUID) -> PersonalData: + async def get(self, personal_data_uuid: UUID, workspace_id: UUID | None = None) -> PersonalData: entity = await self._repo.get_by_id(personal_data_uuid) - if entity is None: + if entity is None or (workspace_id is not None and entity.workspace_id != workspace_id): raise NotFoundError(f"PersonalData {personal_data_uuid} not found") return entity @@ -33,9 +33,10 @@ class PersonalDataService: salary_type: str | None = None, employment_status: str | None = None, min_vacation_days_per_year: int | None = None, + workspace_id: UUID | None = None, ) -> PersonalData: """Partial update. Raises ValidationError/NotFoundError.""" - entity = await self.get(personal_data_uuid) + entity = await self.get(personal_data_uuid, workspace_id) if salary_type is not None: entity.salary_type = salary_type diff --git a/backend/app/services/planning_service.py b/backend/app/services/planning_service.py index 943e5a7..52d6a81 100644 --- a/backend/app/services/planning_service.py +++ b/backend/app/services/planning_service.py @@ -10,6 +10,7 @@ from app.config import settings from app.entities.planned_leave import PlannedLeaveDay from app.repositories.parental_leave_repository import ParentalLeaveRepository from app.repositories.planned_leave_repository import PlannedLeaveRepository +from app.repositories.unit_of_work import NoOpUnitOfWork, UnitOfWork from app.repositories.vacation_days_repository import VacationDaysRepository from app.services.allocation_service import PlanningEngineService, PlanProposal from app.services.analytics_service import AnalyticsService @@ -140,6 +141,7 @@ class PlanningService: parental_leave_repo: ParentalLeaveRepository, vacation_repo: VacationDaysRepository, planning_engine_service: PlanningEngineService | None = None, + uow: UnitOfWork | None = None, ) -> None: self._repo = repo self._analytics = analytics_service @@ -150,6 +152,11 @@ class PlanningService: # ever use the per-day CRUD/`apply_proposed_days` paths don't need to # wire up a `PlanningEngineService` they'll never use. self._engine = planning_engine_service + # Defaults to a no-op so pre-existing direct-construction call sites + # (scripts/seed_demo.py, test fixtures) keep today's per-write-commit + # behavior; `app.dependencies.get_planning_service` always wires a + # real `SQLiteUnitOfWork`. + self._uow = uow or NoOpUnitOfWork() async def add_planned_day( self, @@ -200,11 +207,13 @@ class PlanningService: ) return await self._repo.create(entity) - async def remove_planned_day(self, planned_leave_day_id: UUID) -> None: + async def remove_planned_day( + self, planned_leave_day_id: UUID, workspace_id: UUID | None = None + ) -> None: """Remove a planned leave day.""" - entity = await self._repo.get_by_id(planned_leave_day_id) - if entity is None: - raise NotFoundError(f"PlannedLeaveDay {planned_leave_day_id} not found") + await self.get( + planned_leave_day_id, workspace_id + ) # raises NotFoundError if missing/foreign await self._repo.delete(planned_leave_day_id) async def list_by_workspace(self, workspace_id: UUID) -> list[PlannedLeaveDay]: @@ -226,10 +235,14 @@ class PlanningService: to_date=end_date, ) - async def get(self, planned_leave_day_id: UUID) -> PlannedLeaveDay: - """Get a planned day by ID.""" + async def get( + self, planned_leave_day_id: UUID, workspace_id: UUID | None = None + ) -> PlannedLeaveDay: + """Get a planned day by ID. `workspace_id`, when given, must match the + row's own — a foreign workspace's row 404s exactly like a nonexistent + one.""" entity = await self._repo.get_by_id(planned_leave_day_id) - if entity is None: + if entity is None or (workspace_id is not None and entity.workspace_id != workspace_id): raise NotFoundError(f"PlannedLeaveDay {planned_leave_day_id} not found") return entity @@ -244,11 +257,10 @@ class PlanningService: is_dubbeldag: bool | None = None, wish_id: UUID | None = None, is_pinned: bool | None = None, + workspace_id: UUID | None = None, ) -> PlannedLeaveDay: """Update a planned day (partial update).""" - entity = await self._repo.get_by_id(planned_leave_day_id) - if entity is None: - raise NotFoundError(f"PlannedLeaveDay {planned_leave_day_id} not found") + entity = await self.get(planned_leave_day_id, workspace_id) if day_type is not None: entity.day_type = day_type @@ -314,10 +326,16 @@ class PlanningService: Validates the entire batch up front (day types, per-day dubbeldagar window, batch-wide dubbeldagar quota, child eligibility window, remaining sjukpenning/lagsta balance, credited for anything in - `delete_keys`) and only writes if everything passes — SQLite gives us - no cross-request transaction here, so this explicit validate-then- - write split is what keeps a rejection from ever leaving partial - writes behind. + `delete_keys`) and only writes if everything passes. Stages 2-4 below + (the classification read through the final write) run inside one + `self._uow()` unit of work — a `BEGIN IMMEDIATE` transaction on the + shared connection, serialized against any other unit of work via a + per-connection lock (see `app.db.connection.unit_of_work`). That lock + + transaction is what actually keeps a rejection, a crash mid-batch, + or a concurrent apply/sync on the same workspace from ever leaving + partial writes behind; the up-front validate-then-write split alone + only ever protected against a *rejected* batch writing anything, not + a crashed or interleaved one. """ delete_keys = delete_keys or set() @@ -379,149 +397,161 @@ class PlanningService: if not unique_days and not delete_keys: return PlanApplyResult(accepted=True, skipped_free_count=skipped_free_count) - # -- stage 2: classify against existing rows (collision detection) -- - by_child: dict[UUID, list[date]] = {} - for d in unique_days: - by_child.setdefault(d.child_id, []).append(d.date) - for child_id, _parent_uuid, day in delete_keys: - by_child.setdefault(child_id, []).append(day) + # Stages 2-4 run inside one unit of work: a concurrent apply/sync for + # this same workspace blocks at `BEGIN IMMEDIATE` (inside `self._uow()`) + # until this call commits or rolls back, then re-reads `existing_by_key` + # against the now-committed state instead of racing this call's reads. + async with self._uow(): + # -- stage 2: classify against existing rows (collision detection) -- + by_child: dict[UUID, list[date]] = {} + for d in unique_days: + by_child.setdefault(d.child_id, []).append(d.date) + for child_id, _parent_uuid, day in delete_keys: + by_child.setdefault(child_id, []).append(day) - existing_by_key: dict[tuple[UUID, UUID, date], PlannedLeaveDay] = {} - for child_id, dates in by_child.items(): - existing_rows = await self._repo.list_by_workspace_child( - workspace_id=workspace_id, - child_id=child_id, - from_date=min(dates), - to_date=max(dates), - ) - for row in existing_rows: - existing_by_key[(row.child_id, row.parent_personal_data_uuid, row.date)] = row - - # Rows named by `delete_keys` that actually exist — a key with no - # matching row (already gone) is silently ignored. - delete_rows: list[PlannedLeaveDay] = [ - existing_by_key[key] for key in delete_keys if key in existing_by_key - ] - - to_create: list[ProposedDayInput] = [] - to_replace: list[tuple[PlannedLeaveDay, ProposedDayInput]] = [] - to_update_metadata: list[tuple[PlannedLeaveDay, ProposedDayInput]] = [] - collisions: list[DayCollision] = [] - skipped_identical_count = 0 - - for d in unique_days: - key = (d.child_id, d.parent_personal_data_uuid, d.date) - existing = existing_by_key.get(key) - if existing is None: - to_create.append(d) - continue - if existing.day_type == d.day_type and existing.is_dubbeldag == d.is_dubbeldag: - skipped_identical_count += 1 - # Identical-vs-collision is (and stays) keyed on - # day_type/is_dubbeldag only. A row whose only difference is - # `wish_id`/`is_pinned` is never a collision requiring - # `replace=True` — it's a plain metadata update, written - # below without touching `skipped_identical_count` or - # `collisions`. - if existing.wish_id != d.wish_id or existing.is_pinned != d.is_pinned: - to_update_metadata.append((existing, d)) - continue - collisions.append( - DayCollision( - date=d.date, - parent_personal_data_uuid=d.parent_personal_data_uuid, - child_id=d.child_id, - existing_day_type=existing.day_type, - proposed_day_type=d.day_type, + existing_by_key: dict[tuple[UUID, UUID, date], PlannedLeaveDay] = {} + for child_id, dates in by_child.items(): + existing_rows = await self._repo.list_by_workspace_child( + workspace_id=workspace_id, + child_id=child_id, + from_date=min(dates), + to_date=max(dates), ) - ) - if replace: - to_replace.append((existing, d)) + for row in existing_rows: + existing_by_key[(row.child_id, row.parent_personal_data_uuid, row.date)] = row - if collisions and not replace: - return PlanApplyResult( - accepted=False, - collisions=collisions, - rejections=[ - Rejection( - message_key="planApply.rejection.collisionsNeedReplace", - data={"count": len(collisions)}, + # Rows named by `delete_keys` that actually exist — a key with no + # matching row (already gone) is silently ignored. + delete_rows: list[PlannedLeaveDay] = [ + existing_by_key[key] for key in delete_keys if key in existing_by_key + ] + + to_create: list[ProposedDayInput] = [] + to_replace: list[tuple[PlannedLeaveDay, ProposedDayInput]] = [] + to_update_metadata: list[tuple[PlannedLeaveDay, ProposedDayInput]] = [] + collisions: list[DayCollision] = [] + skipped_identical_count = 0 + + for d in unique_days: + key = (d.child_id, d.parent_personal_data_uuid, d.date) + existing = existing_by_key.get(key) + if existing is None: + to_create.append(d) + continue + if existing.day_type == d.day_type and existing.is_dubbeldag == d.is_dubbeldag: + skipped_identical_count += 1 + # Identical-vs-collision is (and stays) keyed on + # day_type/is_dubbeldag only. A row whose only difference is + # `wish_id`/`is_pinned` is never a collision requiring + # `replace=True` — it's a plain metadata update, written + # below without touching `skipped_identical_count` or + # `collisions`. + if existing.wish_id != d.wish_id or existing.is_pinned != d.is_pinned: + to_update_metadata.append((existing, d)) + continue + collisions.append( + DayCollision( + date=d.date, + parent_personal_data_uuid=d.parent_personal_data_uuid, + child_id=d.child_id, + existing_day_type=existing.day_type, + proposed_day_type=d.day_type, ) - ], + ) + if replace: + to_replace.append((existing, d)) + + if collisions and not replace: + return PlanApplyResult( + accepted=False, + collisions=collisions, + rejections=[ + Rejection( + message_key="planApply.rejection.collisionsNeedReplace", + data={"count": len(collisions)}, + ) + ], + ) + + pending = to_create + [new for _, new in to_replace] + if not pending and not to_update_metadata and not delete_rows: + # Nothing left to write — either an empty batch or a fully + # idempotent re-apply. + return PlanApplyResult( + accepted=True, + skipped_identical_count=skipped_identical_count, + skipped_free_count=skipped_free_count, + ) + + # -- stage 3: domain validation on the batch that would actually write + # + # `delete_rows` is threaded into both checks below so a row this same + # call is about to delete is credited back to its tier/quota BEFORE + # the new demand is checked against what's left — the bug this + # `delete_keys` parameter exists to fix was validating against a + # pre-write snapshot that hadn't yet had that capacity freed. + domain_errors: list[str] = [] + domain_errors.extend( + await self._validate_dubbeldagar_batch(pending, to_replace, delete_rows) + ) + domain_errors.extend(await self._validate_eligible_range(pending)) + domain_errors.extend( + await self._validate_remaining_balance(pending, to_replace, delete_rows) + ) + domain_errors.extend( + await self._validate_remaining_vacation_balance(pending, to_replace, delete_rows) ) - pending = to_create + [new for _, new in to_replace] - if not pending and not to_update_metadata and not delete_rows: - # Nothing left to write — either an empty batch or a fully - # idempotent re-apply. + if domain_errors: + return PlanApplyResult( + accepted=False, collisions=collisions, rejections=domain_errors + ) + + # -- stage 4: write, batched (H1: one executemany per operation + # instead of one commit per row) -------------------------------- + delete_ids = [old.id for old, _ in to_replace] + [row.id for row in delete_rows] + if delete_ids: + await self._repo.delete_many(delete_ids) + + now = datetime.now() + to_insert: list[PlannedLeaveDay] = [ + PlannedLeaveDay( + id=uuid4(), + workspace_id=workspace_id, + child_id=d.child_id, + parent_personal_data_uuid=d.parent_personal_data_uuid, + date=d.date, + day_type=d.day_type, + is_dubbeldag=d.is_dubbeldag, + wish_id=d.wish_id, + is_pinned=d.is_pinned, + created_at=now, + updated_at=now, + ) + for d in pending + ] + if to_insert: + await self._repo.create_many(to_insert) + created = to_insert + + if to_update_metadata: + metadata_pairs: list[tuple[UUID, UUID | None, bool]] = [] + for existing, d in to_update_metadata: + existing.wish_id = d.wish_id + existing.is_pinned = d.is_pinned + metadata_pairs.append((existing.id, d.wish_id, d.is_pinned)) + await self._repo.update_metadata_many(metadata_pairs) + return PlanApplyResult( accepted=True, + created=created, + created_count=len(to_create), skipped_identical_count=skipped_identical_count, skipped_free_count=skipped_free_count, + replaced_count=len(to_replace), + deleted_count=len(delete_rows), ) - # -- stage 3: domain validation on the batch that would actually write - # - # `delete_rows` is threaded into both checks below so a row this same - # call is about to delete is credited back to its tier/quota BEFORE - # the new demand is checked against what's left — the bug this - # `delete_keys` parameter exists to fix was validating against a - # pre-write snapshot that hadn't yet had that capacity freed. - domain_errors: list[str] = [] - domain_errors.extend( - await self._validate_dubbeldagar_batch(pending, to_replace, delete_rows) - ) - domain_errors.extend(await self._validate_eligible_range(pending)) - domain_errors.extend( - await self._validate_remaining_balance(pending, to_replace, delete_rows) - ) - domain_errors.extend( - await self._validate_remaining_vacation_balance(pending, to_replace, delete_rows) - ) - - if domain_errors: - return PlanApplyResult(accepted=False, collisions=collisions, rejections=domain_errors) - - # -- stage 4: write -------------------------------------------------- - for old, _ in to_replace: - await self._repo.delete(old.id) - - for row in delete_rows: - await self._repo.delete(row.id) - - now = datetime.now() - created: list[PlannedLeaveDay] = [] - for d in pending: - entity = PlannedLeaveDay( - id=uuid4(), - workspace_id=workspace_id, - child_id=d.child_id, - parent_personal_data_uuid=d.parent_personal_data_uuid, - date=d.date, - day_type=d.day_type, - is_dubbeldag=d.is_dubbeldag, - wish_id=d.wish_id, - is_pinned=d.is_pinned, - created_at=now, - updated_at=now, - ) - created.append(await self._repo.create(entity)) - - for existing, d in to_update_metadata: - existing.wish_id = d.wish_id - existing.is_pinned = d.is_pinned - await self._repo.update(existing.id, existing) - - return PlanApplyResult( - accepted=True, - created=created, - created_count=len(to_create), - skipped_identical_count=skipped_identical_count, - skipped_free_count=skipped_free_count, - replaced_count=len(to_replace), - deleted_count=len(delete_rows), - ) - # -- sync (POST /api/plan/sync) --------------------------------------- # # Recomputes and persists a workspace's plan in one step, then reports @@ -561,126 +591,135 @@ class PlanningService: if self._engine is None: raise ValidationError("PlanningService.sync_plan requires a PlanningEngineService") - before = await self._repo.list(workspace_id=workspace_id) - before_by_key: dict[tuple[UUID, UUID, date], PlannedLeaveDay] = { - (d.child_id, d.parent_personal_data_uuid, d.date): d for d in before - } - pinned_keys = {key for key, d in before_by_key.items() if d.is_pinned} + # One unit of work spans the pre-write snapshot through the post-write + # re-read, not just `apply_proposed_days`'s own writes — otherwise a + # concurrent sync could commit between this call's `before` read and + # its (nested, no-op-passthrough — see `unit_of_work`'s docstring) + # write, and this call's `changes` diff would be computed against a + # snapshot that was already stale by the time it read `before`. + async with self._uow(): + before = await self._repo.list(workspace_id=workspace_id) + before_by_key: dict[tuple[UUID, UUID, date], PlannedLeaveDay] = { + (d.child_id, d.parent_personal_data_uuid, d.date): d for d in before + } + pinned_keys = {key for key, d in before_by_key.items() if d.is_pinned} - proposal = await self._engine.propose(workspace_id, today=today, only_pinned_existing=True) - - batch = [ - ProposedDayInput( - date=d.date, - parent_personal_data_uuid=d.parent_personal_data_uuid, - child_id=d.child_id, - day_type=d.day_type, - is_dubbeldag=d.is_dubbeldag, - wish_id=d.wish_id, - is_pinned=False, + proposal = await self._engine.propose( + workspace_id, today=today, only_pinned_existing=True ) - for d in proposal.days - if d.day_type != "free" - and d.child_id is not None - # Defense-in-depth only: the allocator never proposes anything - # for an already-occupied key (pinned rows stay in - # `existing_planned` — see `only_pinned_existing` above), so this - # should never actually filter anything out. - and (d.child_id, d.parent_personal_data_uuid, d.date) not in pinned_keys - ] - batch_keys = {(d.child_id, d.parent_personal_data_uuid, d.date) for d in batch} - # Rows the freshly-computed plan no longer accounts for at all: - # unpinned, wish-attributed, and absent from this run's batch — e.g. - # the wish was deleted, or its window moved elsewhere. Passed as - # `delete_keys` so `apply_proposed_days` validates the deletions - # together with the creates/replaces (see the comment above this - # method) rather than deleting them itself afterwards. - stale_keys = { - key - for key, row in before_by_key.items() - if not row.is_pinned and row.wish_id is not None and key not in batch_keys - } + batch = [ + ProposedDayInput( + date=d.date, + parent_personal_data_uuid=d.parent_personal_data_uuid, + child_id=d.child_id, + day_type=d.day_type, + is_dubbeldag=d.is_dubbeldag, + wish_id=d.wish_id, + is_pinned=False, + ) + for d in proposal.days + if d.day_type != "free" + and d.child_id is not None + # Defense-in-depth only: the allocator never proposes anything + # for an already-occupied key (pinned rows stay in + # `existing_planned` — see `only_pinned_existing` above), so this + # should never actually filter anything out. + and (d.child_id, d.parent_personal_data_uuid, d.date) not in pinned_keys + ] - apply_result = await self.apply_proposed_days( - workspace_id=workspace_id, days=batch, replace=True, delete_keys=stale_keys - ) - - changes: list[PlanChange] = [] - if apply_result.accepted: - after = await self._repo.list(workspace_id=workspace_id) - after_by_key: dict[tuple[UUID, UUID, date], PlannedLeaveDay] = { - (d.child_id, d.parent_personal_data_uuid, d.date): d for d in after + batch_keys = {(d.child_id, d.parent_personal_data_uuid, d.date) for d in batch} + # Rows the freshly-computed plan no longer accounts for at all: + # unpinned, wish-attributed, and absent from this run's batch — e.g. + # the wish was deleted, or its window moved elsewhere. Passed as + # `delete_keys` so `apply_proposed_days` validates the deletions + # together with the creates/replaces (see the comment above this + # method) rather than deleting them itself afterwards. + stale_keys = { + key + for key, row in before_by_key.items() + if not row.is_pinned and row.wish_id is not None and key not in batch_keys } - for key in sorted( - set(before_by_key) | set(after_by_key), - key=lambda k: (k[2], str(k[0]), str(k[1])), - ): - child_id, parent_uuid, d = key - old = before_by_key.get(key) - new = after_by_key.get(key) - if old is None and new is not None: - changes.append( - PlanChange( - kind="added", - date=d, - parent_personal_data_uuid=parent_uuid, - child_id=child_id, - old_day_type=None, - new_day_type=new.day_type, - old_is_dubbeldag=None, - new_is_dubbeldag=new.is_dubbeldag, - old_wish_id=None, - new_wish_id=new.wish_id, - ) - ) - elif old is not None and new is None: - changes.append( - PlanChange( - kind="removed", - date=d, - parent_personal_data_uuid=parent_uuid, - child_id=child_id, - old_day_type=old.day_type, - new_day_type=None, - old_is_dubbeldag=old.is_dubbeldag, - new_is_dubbeldag=None, - old_wish_id=old.wish_id, - new_wish_id=None, - ) - ) - elif old is not None and new is not None: - if old.day_type != new.day_type or old.is_dubbeldag != new.is_dubbeldag: + apply_result = await self.apply_proposed_days( + workspace_id=workspace_id, days=batch, replace=True, delete_keys=stale_keys + ) + + changes: list[PlanChange] = [] + if apply_result.accepted: + after = await self._repo.list(workspace_id=workspace_id) + after_by_key: dict[tuple[UUID, UUID, date], PlannedLeaveDay] = { + (d.child_id, d.parent_personal_data_uuid, d.date): d for d in after + } + + for key in sorted( + set(before_by_key) | set(after_by_key), + key=lambda k: (k[2], str(k[0]), str(k[1])), + ): + child_id, parent_uuid, d = key + old = before_by_key.get(key) + new = after_by_key.get(key) + if old is None and new is not None: changes.append( PlanChange( - kind="retyped", + kind="added", + date=d, + parent_personal_data_uuid=parent_uuid, + child_id=child_id, + old_day_type=None, + new_day_type=new.day_type, + old_is_dubbeldag=None, + new_is_dubbeldag=new.is_dubbeldag, + old_wish_id=None, + new_wish_id=new.wish_id, + ) + ) + elif old is not None and new is None: + changes.append( + PlanChange( + kind="removed", date=d, parent_personal_data_uuid=parent_uuid, child_id=child_id, old_day_type=old.day_type, - new_day_type=new.day_type, + new_day_type=None, old_is_dubbeldag=old.is_dubbeldag, - new_is_dubbeldag=new.is_dubbeldag, + new_is_dubbeldag=None, old_wish_id=old.wish_id, - new_wish_id=new.wish_id, + new_wish_id=None, ) ) - elif old.wish_id != new.wish_id: - changes.append( - PlanChange( - kind="reattributed", - date=d, - parent_personal_data_uuid=parent_uuid, - child_id=child_id, - old_day_type=old.day_type, - new_day_type=new.day_type, - old_is_dubbeldag=old.is_dubbeldag, - new_is_dubbeldag=new.is_dubbeldag, - old_wish_id=old.wish_id, - new_wish_id=new.wish_id, + elif old is not None and new is not None: + if old.day_type != new.day_type or old.is_dubbeldag != new.is_dubbeldag: + changes.append( + PlanChange( + kind="retyped", + date=d, + parent_personal_data_uuid=parent_uuid, + child_id=child_id, + old_day_type=old.day_type, + new_day_type=new.day_type, + old_is_dubbeldag=old.is_dubbeldag, + new_is_dubbeldag=new.is_dubbeldag, + old_wish_id=old.wish_id, + new_wish_id=new.wish_id, + ) + ) + elif old.wish_id != new.wish_id: + changes.append( + PlanChange( + kind="reattributed", + date=d, + parent_personal_data_uuid=parent_uuid, + child_id=child_id, + old_day_type=old.day_type, + new_day_type=new.day_type, + old_is_dubbeldag=old.is_dubbeldag, + new_is_dubbeldag=new.is_dubbeldag, + old_wish_id=old.wish_id, + new_wish_id=new.wish_id, + ) ) - ) return PlanSyncResult(proposal=proposal, apply_result=apply_result, changes=changes) diff --git a/backend/app/services/vacation_days_service.py b/backend/app/services/vacation_days_service.py index 1b05510..3cf7ac1 100644 --- a/backend/app/services/vacation_days_service.py +++ b/backend/app/services/vacation_days_service.py @@ -51,9 +51,9 @@ class VacationDaysService: ) return await self._repo.create(entity) - async def get(self, vacation_days_id: UUID) -> VacationDays: + async def get(self, vacation_days_id: UUID, workspace_id: UUID | None = None) -> VacationDays: entity = await self._repo.get_by_id(vacation_days_id) - if entity is None: + if entity is None or (workspace_id is not None and entity.workspace_id != workspace_id): raise NotFoundError(f"VacationDays {vacation_days_id} not found") return entity diff --git a/backend/app/services/workspace_service.py b/backend/app/services/workspace_service.py index 2d820cf..d1666a8 100644 --- a/backend/app/services/workspace_service.py +++ b/backend/app/services/workspace_service.py @@ -21,6 +21,7 @@ from app.repositories.leave_wish_repository import LeaveWishRepository from app.repositories.parental_leave_repository import ParentalLeaveRepository from app.repositories.personal_data_repository import PersonalDataRepository from app.repositories.planned_leave_repository import PlannedLeaveRepository +from app.repositories.unit_of_work import NoOpUnitOfWork, UnitOfWork from app.repositories.user_repository import UserRepository from app.repositories.vacation_days_repository import VacationDaysRepository from app.repositories.workspace_repository import ( @@ -43,6 +44,7 @@ class WorkspaceService: planned_leave_repo: PlannedLeaveRepository, parental_leave_repo: ParentalLeaveRepository, vacation_days_repo: VacationDaysRepository, + uow: UnitOfWork | None = None, ) -> None: self._ws = ws_repo self._inv = inv_repo @@ -52,6 +54,11 @@ class WorkspaceService: self._planned_leave = planned_leave_repo self._parental_leave = parental_leave_repo self._vacation_days = vacation_days_repo + # Defaults to a no-op so pre-existing direct-construction call sites + # (scripts/seed_demo.py, test fixtures) keep today's per-write-commit + # behavior; `app.dependencies.get_workspace_service` always wires a + # real `SQLiteUnitOfWork`. + self._uow = uow or NoOpUnitOfWork() # ------------------------------------------------------------------ # Workspace CRUD @@ -264,16 +271,30 @@ class WorkspaceService: pd = await self._pd.get_by_id(personal_data_uuid) if pd is None: raise NotFoundError(f"PersonalData {personal_data_uuid} not found") - pd.workspace_id = inv.workspace_id - await self._pd.update(pd.uuid, pd) - inv.status = "accepted" - inv.accepted_at = datetime.now() - return await self._inv.update(inv.id, inv) + if pd.workspace_id == inv.workspace_id: + raise InvitationError("You already belong to this workspace") - async def revoke_invitation(self, invitation_id: UUID) -> None: + existing_members = await self._pd.list_by_workspace(inv.workspace_id) + if len(existing_members) >= 2: + raise InvitationError("This workspace already has its maximum number of members") + + # Both writes must land together: a crash between them would leave + # the redeemer either moved to the new workspace with a still-pending + # (re-usable) invitation, or stuck in their old workspace with an + # invitation marked accepted. + async with self._uow(): + pd.workspace_id = inv.workspace_id + await self._pd.update(pd.uuid, pd) + + inv.status = "accepted" + inv.accepted_at = datetime.now() + inv = await self._inv.update(inv.id, inv) + return inv + + async def revoke_invitation(self, invitation_id: UUID, workspace_id: UUID) -> None: inv = await self._inv.get_by_id(invitation_id) - if inv is None: + if inv is None or inv.workspace_id != workspace_id: raise NotFoundError(f"Invitation {invitation_id} not found") if inv.status != "pending": raise ValidationError(f"Cannot revoke a {inv.status} invitation") diff --git a/backend/migrations/versions/011_add_indexes.py b/backend/migrations/versions/011_add_indexes.py new file mode 100644 index 0000000..67db585 --- /dev/null +++ b/backend/migrations/versions/011_add_indexes.py @@ -0,0 +1,99 @@ +"""Add indexes on hot filter/FK-cascade columns, and user.is_active. + +Backend-perf audit (`scratchpad/audit/backend-perf.md` §4, M1): every +migration to date created zero indexes; `EXPLAIN QUERY PLAN` against the full +schema shows several hot paths doing a full table SCAN once the DB carries +more than a handful of rows per table (multi-workspace deployment, years of +FK-fetch history) — `planned_leave_day.count_by_parent_type` (2x per +parent+child in analytics and plan/apply), `count_dubbeldagar`, +`parental_leave_fetch.get_latest_by_parent`, and every FK-cascade column that +`ON DELETE CASCADE` walks on a GDPR delete. These 14 indexes cover exactly +those columns; see the audit for the full per-index rationale. Every +`CREATE INDEX` below is mirrored verbatim (same columns, same order, no +partial indexes) in `_FULL_SCHEMA` (`app/db/connection.py`) so +`tests/integration/test_schema_parity.py` — which compares index sets between +the Alembic chain and that hand-maintained DDL — stays green. + +Also adds `user.is_active` (default 1): `AuthService.login` and +`get_user_from_token` already check `User.is_active` (account +deactivation), but the column never existed — every account was +unconditionally treated as active. This migration adds it and +`SQLiteUserRepository` now reads/writes it. + +Revision ID: 011 +Revises: 010 +Create Date: 2026-09-18 +""" + +from __future__ import annotations + +from alembic import op + +revision: str = "011" +down_revision: str | None = "010" +branch_labels: str | None = None +depends_on: str | None = None + +_UPGRADE_SQL = """ +ALTER TABLE "user" ADD COLUMN is_active INTEGER NOT NULL DEFAULT 1; +CREATE INDEX idx_pld_parent_child_type_date + ON planned_leave_day (parent_personal_data_uuid, child_id, day_type, date); +CREATE INDEX idx_pld_child_dubbeldag_date + ON planned_leave_day (child_id, is_dubbeldag, date); +CREATE INDEX idx_pld_wish + ON planned_leave_day (wish_id); +CREATE INDEX idx_plf_parent_child_fetched + ON parental_leave_fetch (personal_data_uuid, child_id, fetched_at); +CREATE INDEX idx_plf_workspace + ON parental_leave_fetch (workspace_id); +CREATE INDEX idx_plf_child + ON parental_leave_fetch (child_id); +CREATE INDEX idx_leave_wish_workspace + ON leave_wish (workspace_id); +CREATE INDEX idx_leave_wish_child + ON leave_wish (child_id); +CREATE INDEX idx_leave_wish_parent + ON leave_wish (parent_personal_data_uuid); +CREATE INDEX idx_child_workspace_birth + ON child (workspace_id, birth_date); +CREATE INDEX idx_personal_data_workspace_deleted + ON personal_data (workspace_id, deleted_at); +CREATE INDEX idx_invitation_workspace_status_created + ON workspace_invitation (workspace_id, status, created_at); +CREATE INDEX idx_invitation_invited_by + ON workspace_invitation (invited_by_user_id); +CREATE INDEX idx_vacation_days_workspace + ON vacation_days (workspace_id); +""" + +_DOWNGRADE_SQL = """ +DROP INDEX idx_vacation_days_workspace; +DROP INDEX idx_invitation_invited_by; +DROP INDEX idx_invitation_workspace_status_created; +DROP INDEX idx_personal_data_workspace_deleted; +DROP INDEX idx_child_workspace_birth; +DROP INDEX idx_leave_wish_parent; +DROP INDEX idx_leave_wish_child; +DROP INDEX idx_leave_wish_workspace; +DROP INDEX idx_plf_child; +DROP INDEX idx_plf_workspace; +DROP INDEX idx_plf_parent_child_fetched; +DROP INDEX idx_pld_wish; +DROP INDEX idx_pld_child_dubbeldag_date; +DROP INDEX idx_pld_parent_child_type_date; +ALTER TABLE "user" DROP COLUMN is_active; +""" + + +def upgrade() -> None: + from sqlalchemy import text + + for stmt in [s.strip() for s in _UPGRADE_SQL.split(";") if s.strip()]: + op.execute(text(stmt)) + + +def downgrade() -> None: + from sqlalchemy import text + + for stmt in [s.strip() for s in _DOWNGRADE_SQL.split(";") if s.strip()]: + op.execute(text(stmt)) diff --git a/backend/scripts/demo_plan.py b/backend/scripts/demo_plan.py index c318aac..43d6547 100644 --- a/backend/scripts/demo_plan.py +++ b/backend/scripts/demo_plan.py @@ -32,6 +32,7 @@ from app.services.allocation_service import ( WindowOutcome, ) from app.utils.holidays import get_swedish_holidays +from app.utils.tip_predicates import resolve_age4_savable_cap # --------------------------------------------------------------------------- # 1. Build a realistic AllocationInput, in memory, Swedish defaults. @@ -124,6 +125,7 @@ def _build_input() -> tuple[AllocationInput, dict]: today=TODAY, horizon_end=horizon_end, planning_mode="couple", + age4_savable_cap=resolve_age4_savable_cap("couple", parent_count=2), children=[child], parents=[parent_1, parent_2], pools=pools, diff --git a/backend/tests/conftest.py b/backend/tests/conftest.py index d8c67e7..63318f9 100644 --- a/backend/tests/conftest.py +++ b/backend/tests/conftest.py @@ -13,11 +13,15 @@ from uuid import UUID, uuid4 import aiosqlite import pytest_asyncio +from httpx import ASGITransport, AsyncClient from app.db.connection import ensure_schema +from app.dependencies import get_connection, get_current_user from app.entities.child import Child from app.entities.personal_data import PersonalData +from app.entities.user import User from app.entities.workspace import Workspace +from app.main import app from app.repositories_impl.sqlite.child_repository import SQLiteChildRepository from app.repositories_impl.sqlite.day_override_repository import SQLiteDayOverrideRepository from app.repositories_impl.sqlite.leave_wish_repository import SQLiteLeaveWishRepository @@ -25,6 +29,7 @@ from app.repositories_impl.sqlite.parental_leave_repository import SQLiteParenta from app.repositories_impl.sqlite.personal_data_repository import SQLitePersonalDataRepository from app.repositories_impl.sqlite.planned_leave_repository import SQLitePlannedLeaveRepository from app.repositories_impl.sqlite.tip_dismissal_repository import SQLiteTipDismissalRepository +from app.repositories_impl.sqlite.unit_of_work import SQLiteUnitOfWork from app.repositories_impl.sqlite.user_repository import SQLiteUserRepository from app.repositories_impl.sqlite.vacation_days_repository import ( SQLiteVacationDaysRepository, @@ -183,6 +188,51 @@ async def seeded_ids( return SeededIds(workspace_id=ws.id, personal_data_uuid=pd.uuid, child_id=child.id) +# --------------------------------------------------------------------------- +# Shared authenticated HTTP client — bound to `seeded_ids`'s workspace. +# +# T1 authz work: every router now rejects ids outside the caller's workspace, +# so a router test's `client` must be authenticated as a real member of +# whatever workspace the test operates on. This fixture is that identity for +# any router test built around `seeded_ids` (see `tests/unit/test_fetch_data_ +# router.py`). Router tests that seed their own bespoke workspace per test +# (e.g. `test_plan_router.py`) still define their own local `client`/current- +# user fixture bound to THAT workspace instead — a single global identity +# can't satisfy both shapes. +# --------------------------------------------------------------------------- + + +@pytest_asyncio.fixture +async def current_user(seeded_ids: SeededIds) -> User: + """A real, logged-in user whose PersonalData is `seeded_ids.personal_data_uuid`.""" + return User( + id=uuid4(), + personal_data_uuid=seeded_ids.personal_data_uuid, + email="seeded-user@example.com", + hashed_password="not-a-real-hash", + created_at=datetime.now(), + ) + + +@pytest_asyncio.fixture +async def authed_client( + connection: aiosqlite.Connection, current_user: User +) -> AsyncIterator[AsyncClient]: + """An httpx client against the real app, authenticated as `current_user` + and wired to the test's in-memory `connection` — the same technique + every router test file uses, consolidated here for tests keyed off + `seeded_ids`. + """ + app.dependency_overrides[get_connection] = lambda: connection + app.dependency_overrides[get_current_user] = lambda: current_user + try: + async with AsyncClient(transport=ASGITransport(app=app), base_url="http://test") as ac: + yield ac + finally: + app.dependency_overrides.pop(get_connection, None) + app.dependency_overrides.pop(get_current_user, None) + + # --------------------------------------------------------------------------- # Child & leave # --------------------------------------------------------------------------- @@ -313,6 +363,40 @@ async def planning_service( ) +# --------------------------------------------------------------------------- +# Unit of work — a real `SQLiteUnitOfWork` bound to the test's shared +# `connection`, for tests that specifically exercise atomicity/locking (see +# `tests/unit/test_db_unit_of_work.py`). `planning_service` above deliberately +# keeps its default no-op unit of work so every other test's per-write-commit +# behavior stays exactly as it is; use `planning_service_with_uow` instead +# when a test needs the real thing. +# --------------------------------------------------------------------------- + + +@pytest_asyncio.fixture +async def sqlite_uow(connection: aiosqlite.Connection) -> SQLiteUnitOfWork: + return SQLiteUnitOfWork(connection) + + +@pytest_asyncio.fixture +async def planning_service_with_uow( + planned_leave_repo: SQLitePlannedLeaveRepository, + analytics_service: AnalyticsService, + parental_leave_repo: SQLiteParentalLeaveRepository, + vacation_repo: SQLiteVacationDaysRepository, + planning_engine_service: PlanningEngineService, + sqlite_uow: SQLiteUnitOfWork, +) -> PlanningService: + return PlanningService( + planned_leave_repo, + analytics_service, + parental_leave_repo, + vacation_repo, + planning_engine_service, + uow=sqlite_uow, + ) + + # --------------------------------------------------------------------------- # Tips # --------------------------------------------------------------------------- diff --git a/backend/tests/fixtures/fk_overview_synthetic.html b/backend/tests/fixtures/fk_overview_synthetic.html new file mode 100644 index 0000000..5e41903 --- /dev/null +++ b/backend/tests/fixtures/fk_overview_synthetic.html @@ -0,0 +1,81 @@ + + + + + + Ansök och planera föräldrapenning - Försäkringskassan (synthetic fixture) + + +
+
+

För dig

+
+
+
+
+
+ 100 + av 240 dagar kvar efter ansökan +
+
+
+ +
+

Sjukpenningnivå (60 dagar)

+
+
+
+ +
+

Lägstanivå (40 dagar)

+
+
+
+
+
+
+
+
+
+
+
+
+ +
+
+

Totalt för Testbarn

+
+

Exempel på belopp: 1 234 kronor för 5 dagar.

+
+
+
+ +
+ +
+
+
+
+ 20 + av 60 dubbeldagar kvar efter ansökan +
+
+
+
+
+
+
+ + + + diff --git a/backend/tests/unit/test_authz.py b/backend/tests/unit/test_authz.py new file mode 100644 index 0000000..c93455c --- /dev/null +++ b/backend/tests/unit/test_authz.py @@ -0,0 +1,646 @@ +"""Cross-cutting authorization tests (T1 authz audit). + +Two things every route in the ten scoped routers must satisfy (see +`app/dependencies.py`'s `WorkspaceScope`/`WorkspaceIdDep` and the audit at +`graphify`-adjacent scratch notes for the full route table): + + 1. No `Authorization` header -> 401 (F1: `plan`, `leave_wishes`, + `day_overrides`, `vacation_days` had NO auth at all before this task). + 2. A real id belonging to a DIFFERENT workspace -> 404, never 403 (F3: IDOR + on every workspace-/child-/pd-keyed route) — a 403 would confirm the id + exists, which is itself information disclosure. + +This file exercises the real app + dependency graph (httpx ASGITransport), +following the pattern in `tests/unit/test_plan_router.py`, rather than only +the service layer — the bug class here is specifically about what the +ROUTER does (or fails to do) before calling the service. +""" + +from datetime import date, datetime, timedelta +from uuid import uuid4 + +import aiosqlite +import pytest_asyncio +from httpx import ASGITransport, AsyncClient + +from app.dependencies import get_connection, get_current_user +from app.entities.child import Child +from app.entities.day_override import DayOverride +from app.entities.leave_wish import LeaveWish +from app.entities.personal_data import PersonalData +from app.entities.planned_leave import PlannedLeaveDay +from app.entities.user import User +from app.entities.vacation_days import VacationDays +from app.entities.workspace import Workspace +from app.entities.workspace_invitation import WorkspaceInvitation +from app.main import app +from app.repositories_impl.sqlite.child_repository import SQLiteChildRepository +from app.repositories_impl.sqlite.day_override_repository import SQLiteDayOverrideRepository +from app.repositories_impl.sqlite.leave_wish_repository import SQLiteLeaveWishRepository +from app.repositories_impl.sqlite.personal_data_repository import SQLitePersonalDataRepository +from app.repositories_impl.sqlite.planned_leave_repository import SQLitePlannedLeaveRepository +from app.repositories_impl.sqlite.user_repository import SQLiteUserRepository +from app.repositories_impl.sqlite.vacation_days_repository import SQLiteVacationDaysRepository +from app.repositories_impl.sqlite.workspace_repository import ( + SQLiteWorkspaceInvitationRepository, + SQLiteWorkspaceRepository, +) + +# The caller's fixed identity across this file's authenticated tests: a +# member of "workspace A" (seeded per-test by `two_workspaces`). +_CALLER_PD_UUID = uuid4() + + +@pytest_asyncio.fixture +async def anon_client(connection: aiosqlite.Connection) -> AsyncClient: + """No `get_current_user` override at all — a real, unauthenticated + request (no Authorization header) must be rejected by the real JWT + dependency, not by a test double standing in for it. + """ + app.dependency_overrides[get_connection] = lambda: connection + async with AsyncClient(transport=ASGITransport(app=app), base_url="http://test") as ac: + yield ac + app.dependency_overrides.pop(get_connection, None) + + +@pytest_asyncio.fixture +async def client(connection: aiosqlite.Connection) -> AsyncClient: + """Authenticated as `_CALLER_PD_UUID` — a member of whatever workspace + the test seeds for them (see `two_workspaces`). + """ + app.dependency_overrides[get_connection] = lambda: connection + app.dependency_overrides[get_current_user] = lambda: User( + id=uuid4(), + personal_data_uuid=_CALLER_PD_UUID, + email="caller@example.com", + hashed_password="not-a-real-hash", + created_at=datetime.now(), + ) + async with AsyncClient(transport=ASGITransport(app=app), base_url="http://test") as ac: + yield ac + app.dependency_overrides.pop(get_connection, None) + app.dependency_overrides.pop(get_current_user, None) + + +class _TwoWorkspaces: + def __init__( + self, + ws_a: Workspace, + ws_b: Workspace, + child_a: Child, + child_b: Child, + pd_b: PersonalData, + ) -> None: + self.ws_a = ws_a + self.ws_b = ws_b + self.child_a = child_a + self.child_b = child_b + self.pd_b = pd_b + + +@pytest_asyncio.fixture +async def two_workspaces(connection: aiosqlite.Connection) -> _TwoWorkspaces: + """Seed workspace A (the caller's own) and workspace B (a foreign + tenant), each with its own child and member. + """ + now = datetime.now() + ws_repo = SQLiteWorkspaceRepository(connection) + pd_repo = SQLitePersonalDataRepository(connection) + child_repo = SQLiteChildRepository(connection) + + ws_a = await ws_repo.create(Workspace(id=uuid4(), name="Workspace A", created_at=now)) + await pd_repo.create( + PersonalData(uuid=_CALLER_PD_UUID, workspace_id=ws_a.id, name="Caller", created_at=now) + ) + child_a = await child_repo.create( + Child(id=uuid4(), workspace_id=ws_a.id, name="Child A", birth_date=date(2025, 1, 1)) + ) + + ws_b = await ws_repo.create(Workspace(id=uuid4(), name="Workspace B", created_at=now)) + pd_b = await pd_repo.create( + PersonalData(uuid=uuid4(), workspace_id=ws_b.id, name="Victim", created_at=now) + ) + child_b = await child_repo.create( + Child(id=uuid4(), workspace_id=ws_b.id, name="Child B", birth_date=date(2025, 1, 1)) + ) + + return _TwoWorkspaces(ws_a, ws_b, child_a, child_b, pd_b) + + +# --------------------------------------------------------------------------- +# F1 — routes that had NO authentication at all before this task. +# --------------------------------------------------------------------------- + + +class TestNoTokenIsUnauthenticated: + async def test_plan_propose_requires_auth(self, anon_client: AsyncClient) -> None: + resp = await anon_client.post("/api/plan/propose", json={"workspace_id": str(uuid4())}) + assert resp.status_code == 401 + + async def test_plan_apply_requires_auth(self, anon_client: AsyncClient) -> None: + resp = await anon_client.post( + "/api/plan/apply", json={"workspace_id": str(uuid4()), "days": []} + ) + assert resp.status_code == 401 + + async def test_plan_sync_requires_auth(self, anon_client: AsyncClient) -> None: + resp = await anon_client.post("/api/plan/sync", json={"workspace_id": str(uuid4())}) + assert resp.status_code == 401 + + async def test_leave_wishes_create_requires_auth(self, anon_client: AsyncClient) -> None: + resp = await anon_client.post( + "/api/leave-wishes", + json={ + "workspace_id": str(uuid4()), + "name": "X", + "window_start_month": 1, + "window_start_day": 1, + "window_end_month": 1, + "window_end_day": 7, + "target_weeks": 1, + "first_year": 2030, + }, + ) + assert resp.status_code == 401 + + async def test_leave_wishes_list_requires_auth(self, anon_client: AsyncClient) -> None: + resp = await anon_client.get(f"/api/leave-wishes/{uuid4()}") + assert resp.status_code == 401 + + async def test_day_overrides_create_requires_auth(self, anon_client: AsyncClient) -> None: + resp = await anon_client.post( + "/api/day-overrides", + json={"workspace_id": str(uuid4()), "date": "2030-01-01", "is_working_day": False}, + ) + assert resp.status_code == 401 + + async def test_day_overrides_list_requires_auth(self, anon_client: AsyncClient) -> None: + resp = await anon_client.get(f"/api/day-overrides/workspace/{uuid4()}") + assert resp.status_code == 401 + + async def test_vacation_days_create_requires_auth(self, anon_client: AsyncClient) -> None: + resp = await anon_client.post( + "/api/vacation-days", + json={ + "personal_data_uuid": str(uuid4()), + "workspace_id": str(uuid4()), + "year": 2030, + "total_days": 25, + }, + ) + assert resp.status_code == 401 + + async def test_vacation_days_list_requires_auth(self, anon_client: AsyncClient) -> None: + resp = await anon_client.get(f"/api/vacation-days/workspace/{uuid4()}") + assert resp.status_code == 401 + + +# --------------------------------------------------------------------------- +# F3 — a real id from a DIFFERENT workspace must 404, never 403. +# --------------------------------------------------------------------------- + + +class TestForeignWorkspaceIdIs404: + async def test_workspace_get(self, client: AsyncClient, two_workspaces: _TwoWorkspaces) -> None: + resp = await client.get(f"/api/workspace/{two_workspaces.ws_b.id}") + assert resp.status_code == 404 + + async def test_workspace_members( + self, client: AsyncClient, two_workspaces: _TwoWorkspaces + ) -> None: + resp = await client.get(f"/api/workspace/{two_workspaces.ws_b.id}/members") + assert resp.status_code == 404 + + async def test_tips_get(self, client: AsyncClient, two_workspaces: _TwoWorkspaces) -> None: + resp = await client.get(f"/api/tips/{two_workspaces.ws_b.id}") + assert resp.status_code == 404 + + async def test_children_by_workspace( + self, client: AsyncClient, two_workspaces: _TwoWorkspaces + ) -> None: + resp = await client.get(f"/api/children/workspace/{two_workspaces.ws_b.id}") + assert resp.status_code == 404 + + async def test_children_get_foreign_child( + self, client: AsyncClient, two_workspaces: _TwoWorkspaces + ) -> None: + resp = await client.get(f"/api/children/{two_workspaces.child_b.id}") + assert resp.status_code == 404 + + async def test_settings_update_foreign_child( + self, client: AsyncClient, two_workspaces: _TwoWorkspaces + ) -> None: + resp = await client.put( + f"/api/settings/child/{two_workspaces.child_b.id}", json={"name": "Hijacked"} + ) + assert resp.status_code == 404 + + async def test_settings_delete_foreign_child( + self, client: AsyncClient, two_workspaces: _TwoWorkspaces + ) -> None: + resp = await client.delete(f"/api/settings/{two_workspaces.child_b.id}") + assert resp.status_code == 404 + + async def test_settings_update_foreign_parent( + self, client: AsyncClient, two_workspaces: _TwoWorkspaces + ) -> None: + resp = await client.put( + f"/api/settings/parent/{two_workspaces.pd_b.uuid}", + json={"employment_status": "none"}, + ) + assert resp.status_code == 404 + + async def test_calendar_days_foreign_workspace( + self, client: AsyncClient, two_workspaces: _TwoWorkspaces + ) -> None: + resp = await client.get( + "/api/calendar/days", + params={"workspace_id": str(two_workspaces.ws_b.id), "start_year": 2030}, + ) + assert resp.status_code == 404 + + async def test_analytics_overview_foreign_workspace( + self, client: AsyncClient, two_workspaces: _TwoWorkspaces + ) -> None: + resp = await client.get( + f"/api/analytics/{two_workspaces.ws_b.id}/overview", + params={"child_id": str(two_workspaces.child_b.id)}, + ) + assert resp.status_code == 404 + + async def test_analytics_age4_foreign_parent( + self, client: AsyncClient, two_workspaces: _TwoWorkspaces + ) -> None: + resp = await client.get( + f"/api/analytics/parent/{two_workspaces.pd_b.uuid}" + f"/child/{two_workspaces.child_b.id}/age4" + ) + assert resp.status_code == 404 + + async def test_plan_propose_foreign_workspace( + self, client: AsyncClient, two_workspaces: _TwoWorkspaces + ) -> None: + resp = await client.post( + "/api/plan/propose", json={"workspace_id": str(two_workspaces.ws_b.id)} + ) + assert resp.status_code == 404 + + async def test_plan_apply_foreign_child( + self, client: AsyncClient, two_workspaces: _TwoWorkspaces + ) -> None: + resp = await client.post( + "/api/plan/apply", + json={ + "workspace_id": str(two_workspaces.ws_a.id), + "days": [ + { + "date": "2030-06-01", + "parent_personal_data_uuid": str(_CALLER_PD_UUID), + "child_id": str(two_workspaces.child_b.id), + "day_type": "vacation", + } + ], + }, + ) + assert resp.status_code == 404 + + async def test_plan_sync_foreign_workspace( + self, client: AsyncClient, two_workspaces: _TwoWorkspaces + ) -> None: + resp = await client.post( + "/api/plan/sync", json={"workspace_id": str(two_workspaces.ws_b.id)} + ) + assert resp.status_code == 404 + + async def test_leave_wishes_create_foreign_workspace( + self, client: AsyncClient, two_workspaces: _TwoWorkspaces + ) -> None: + resp = await client.post( + "/api/leave-wishes", + json={ + "workspace_id": str(two_workspaces.ws_b.id), + "name": "X", + "window_start_month": 1, + "window_start_day": 1, + "window_end_month": 1, + "window_end_day": 7, + "target_weeks": 1, + "first_year": 2030, + }, + ) + assert resp.status_code == 404 + + async def test_leave_wishes_create_foreign_child( + self, client: AsyncClient, two_workspaces: _TwoWorkspaces + ) -> None: + resp = await client.post( + "/api/leave-wishes", + json={ + "workspace_id": str(two_workspaces.ws_a.id), + "child_id": str(two_workspaces.child_b.id), + "name": "X", + "window_start_month": 1, + "window_start_day": 1, + "window_end_month": 1, + "window_end_day": 7, + "target_weeks": 1, + "first_year": 2030, + }, + ) + assert resp.status_code == 404 + + async def test_leave_wishes_list_foreign_workspace( + self, client: AsyncClient, two_workspaces: _TwoWorkspaces + ) -> None: + resp = await client.get(f"/api/leave-wishes/{two_workspaces.ws_b.id}") + assert resp.status_code == 404 + + async def test_leave_wishes_update_foreign_wish( + self, client: AsyncClient, connection: aiosqlite.Connection, two_workspaces: _TwoWorkspaces + ) -> None: + wish_repo = SQLiteLeaveWishRepository(connection) + wish = await wish_repo.create( + LeaveWish( + id=uuid4(), + workspace_id=two_workspaces.ws_b.id, + child_id=None, + name="Foreign", + window_start_month=1, + window_start_day=1, + window_end_month=1, + window_end_day=7, + target_weeks=1, + recurrence="once", + first_year=2030, + last_year=None, + priority=1, + parent_mode="either", + parent_personal_data_uuid=None, + ) + ) + resp = await client.put(f"/api/leave-wishes/{wish.id}", json={"name": "Hijacked"}) + assert resp.status_code == 404 + + async def test_leave_wishes_delete_foreign_wish( + self, client: AsyncClient, connection: aiosqlite.Connection, two_workspaces: _TwoWorkspaces + ) -> None: + wish_repo = SQLiteLeaveWishRepository(connection) + wish = await wish_repo.create( + LeaveWish( + id=uuid4(), + workspace_id=two_workspaces.ws_b.id, + child_id=None, + name="Foreign", + window_start_month=1, + window_start_day=1, + window_end_month=1, + window_end_day=7, + target_weeks=1, + recurrence="once", + first_year=2030, + last_year=None, + priority=1, + parent_mode="either", + parent_personal_data_uuid=None, + ) + ) + resp = await client.delete(f"/api/leave-wishes/{wish.id}") + assert resp.status_code == 404 + + async def test_day_overrides_create_foreign_workspace( + self, client: AsyncClient, two_workspaces: _TwoWorkspaces + ) -> None: + resp = await client.post( + "/api/day-overrides", + json={ + "workspace_id": str(two_workspaces.ws_b.id), + "date": "2030-01-01", + "is_working_day": False, + }, + ) + assert resp.status_code == 404 + + async def test_day_overrides_get_foreign( + self, client: AsyncClient, connection: aiosqlite.Connection, two_workspaces: _TwoWorkspaces + ) -> None: + repo = SQLiteDayOverrideRepository(connection) + row = await repo.create( + DayOverride( + id=uuid4(), + workspace_id=two_workspaces.ws_b.id, + date=date(2030, 1, 1), + is_working_day=False, + ) + ) + resp = await client.get(f"/api/day-overrides/{row.id}") + assert resp.status_code == 404 + + async def test_day_overrides_list_foreign_workspace( + self, client: AsyncClient, two_workspaces: _TwoWorkspaces + ) -> None: + resp = await client.get(f"/api/day-overrides/workspace/{two_workspaces.ws_b.id}") + assert resp.status_code == 404 + + async def test_day_overrides_delete_foreign( + self, client: AsyncClient, connection: aiosqlite.Connection, two_workspaces: _TwoWorkspaces + ) -> None: + repo = SQLiteDayOverrideRepository(connection) + row = await repo.create( + DayOverride( + id=uuid4(), + workspace_id=two_workspaces.ws_b.id, + date=date(2030, 1, 2), + is_working_day=False, + ) + ) + resp = await client.delete(f"/api/day-overrides/{row.id}") + assert resp.status_code == 404 + + async def test_vacation_days_create_foreign_workspace( + self, client: AsyncClient, two_workspaces: _TwoWorkspaces + ) -> None: + resp = await client.post( + "/api/vacation-days", + json={ + "personal_data_uuid": str(_CALLER_PD_UUID), + "workspace_id": str(two_workspaces.ws_b.id), + "year": 2030, + "total_days": 25, + }, + ) + assert resp.status_code == 404 + + async def test_vacation_days_create_foreign_member( + self, client: AsyncClient, two_workspaces: _TwoWorkspaces + ) -> None: + resp = await client.post( + "/api/vacation-days", + json={ + "personal_data_uuid": str(two_workspaces.pd_b.uuid), + "workspace_id": str(two_workspaces.ws_a.id), + "year": 2030, + "total_days": 25, + }, + ) + assert resp.status_code == 404 + + async def test_vacation_days_get_foreign( + self, client: AsyncClient, connection: aiosqlite.Connection, two_workspaces: _TwoWorkspaces + ) -> None: + repo = SQLiteVacationDaysRepository(connection) + row = await repo.create( + VacationDays( + id=uuid4(), + personal_data_uuid=two_workspaces.pd_b.uuid, + workspace_id=two_workspaces.ws_b.id, + year=2030, + total_days=25, + ) + ) + resp = await client.get(f"/api/vacation-days/{row.id}") + assert resp.status_code == 404 + + async def test_vacation_days_list_foreign_workspace( + self, client: AsyncClient, two_workspaces: _TwoWorkspaces + ) -> None: + resp = await client.get(f"/api/vacation-days/workspace/{two_workspaces.ws_b.id}") + assert resp.status_code == 404 + + async def test_planned_days_create_foreign_child( + self, client: AsyncClient, two_workspaces: _TwoWorkspaces + ) -> None: + resp = await client.post( + "/api/planned-days", + json={ + "workspace_id": str(two_workspaces.ws_a.id), + "child_id": str(two_workspaces.child_b.id), + "parent_personal_data_uuid": str(_CALLER_PD_UUID), + "date": "2030-06-01", + "day_type": "vacation", + }, + ) + assert resp.status_code == 404 + + async def test_planned_days_list_by_child_foreign( + self, client: AsyncClient, two_workspaces: _TwoWorkspaces + ) -> None: + resp = await client.get( + "/api/planned-days", params={"child_id": str(two_workspaces.child_b.id)} + ) + assert resp.status_code == 404 + + async def test_planned_days_get_foreign( + self, client: AsyncClient, connection: aiosqlite.Connection, two_workspaces: _TwoWorkspaces + ) -> None: + repo = SQLitePlannedLeaveRepository(connection) + row = await repo.create( + PlannedLeaveDay( + id=uuid4(), + workspace_id=two_workspaces.ws_b.id, + child_id=two_workspaces.child_b.id, + parent_personal_data_uuid=two_workspaces.pd_b.uuid, + date=date(2030, 6, 1), + day_type="vacation", + ) + ) + resp = await client.get(f"/api/planned-days/{row.id}") + assert resp.status_code == 404 + + async def test_planned_days_by_workspace_foreign( + self, client: AsyncClient, two_workspaces: _TwoWorkspaces + ) -> None: + resp = await client.get(f"/api/planned-days/workspace/{two_workspaces.ws_b.id}") + assert resp.status_code == 404 + + async def test_planned_days_update_foreign( + self, client: AsyncClient, connection: aiosqlite.Connection, two_workspaces: _TwoWorkspaces + ) -> None: + repo = SQLitePlannedLeaveRepository(connection) + row = await repo.create( + PlannedLeaveDay( + id=uuid4(), + workspace_id=two_workspaces.ws_b.id, + child_id=two_workspaces.child_b.id, + parent_personal_data_uuid=two_workspaces.pd_b.uuid, + date=date(2030, 6, 2), + day_type="vacation", + ) + ) + resp = await client.put(f"/api/planned-days/{row.id}", json={"day_type": "sjukpenning"}) + assert resp.status_code == 404 + + async def test_planned_days_delete_foreign( + self, client: AsyncClient, connection: aiosqlite.Connection, two_workspaces: _TwoWorkspaces + ) -> None: + repo = SQLitePlannedLeaveRepository(connection) + row = await repo.create( + PlannedLeaveDay( + id=uuid4(), + workspace_id=two_workspaces.ws_b.id, + child_id=two_workspaces.child_b.id, + parent_personal_data_uuid=two_workspaces.pd_b.uuid, + date=date(2030, 6, 3), + day_type="vacation", + ) + ) + resp = await client.delete(f"/api/planned-days/{row.id}") + assert resp.status_code == 404 + + async def test_fetch_data_manual_foreign_workspace( + self, client: AsyncClient, two_workspaces: _TwoWorkspaces + ) -> None: + resp = await client.post( + "/api/parental-leave/manual", + json={ + "personal_data_uuid": str(_CALLER_PD_UUID), + "workspace_id": str(two_workspaces.ws_b.id), + "child_id": str(two_workspaces.child_a.id), + "total_days": 480, + "days_used": 0, + "reserved_days_left": 90, + "transferable_days_left": 150, + "sjukpenning_used": 0, + "lagsta_niva_used": 0, + }, + ) + assert resp.status_code == 404 + + async def test_fetch_data_get_latest_foreign_pd( + self, client: AsyncClient, two_workspaces: _TwoWorkspaces + ) -> None: + resp = await client.get( + f"/api/parental-leave/{two_workspaces.pd_b.uuid}/{two_workspaces.child_a.id}" + ) + assert resp.status_code == 404 + + async def test_invitations_revoke_foreign_workspace( + self, client: AsyncClient, connection: aiosqlite.Connection, two_workspaces: _TwoWorkspaces + ) -> None: + """The invitation belongs to workspace B; the caller (workspace A) + must not be able to revoke it, even by guessing/enumerating its id. + """ + # `invited_by_user_id` FKs to a real `user` row. + user_repo = SQLiteUserRepository(connection) + inviter = await user_repo.create( + User( + id=uuid4(), + personal_data_uuid=two_workspaces.pd_b.uuid, + email="inviter-b@example.com", + hashed_password="not-a-real-hash", + created_at=datetime.now(), + ) + ) + inv_repo = SQLiteWorkspaceInvitationRepository(connection) + inv = await inv_repo.create( + WorkspaceInvitation( + id=uuid4(), + workspace_id=two_workspaces.ws_b.id, + token_hash="deadbeef", + invited_by_user_id=inviter.id, + status="pending", + expires_at=datetime.now() + timedelta(hours=1), + created_at=datetime.now(), + ) + ) + resp = await client.delete(f"/api/invitations/{inv.id}") + assert resp.status_code == 404 diff --git a/backend/tests/unit/test_calendar_router.py b/backend/tests/unit/test_calendar_router.py index 0f767a1..a6778cd 100644 --- a/backend/tests/unit/test_calendar_router.py +++ b/backend/tests/unit/test_calendar_router.py @@ -23,6 +23,23 @@ from app.repositories_impl.sqlite.day_override_repository import SQLiteDayOverri from app.repositories_impl.sqlite.personal_data_repository import SQLitePersonalDataRepository from app.repositories_impl.sqlite.workspace_repository import SQLiteWorkspaceRepository +# Fixed identity for the authenticated caller in every test in this file — +# each test must also create a `PersonalData` row with this uuid in the +# workspace it seeds, so `get_current_personal_data` resolves to that +# workspace (see CLAUDE.md T1 authz work / audit §4 blast radius). +_CURRENT_USER_PD_UUID = uuid4() + + +async def _seed_caller(connection: aiosqlite.Connection, workspace_id) -> None: + await SQLitePersonalDataRepository(connection).create( + PersonalData( + uuid=_CURRENT_USER_PD_UUID, + workspace_id=workspace_id, + name="Alex", + created_at=datetime.now(), + ) + ) + @pytest_asyncio.fixture async def client(connection: aiosqlite.Connection) -> AsyncClient: @@ -33,7 +50,7 @@ async def client(connection: aiosqlite.Connection) -> AsyncClient: app.dependency_overrides[get_connection] = lambda: connection app.dependency_overrides[get_current_user] = lambda: User( id=uuid4(), - personal_data_uuid=uuid4(), + personal_data_uuid=_CURRENT_USER_PD_UUID, email="test@example.com", hashed_password="not-a-real-hash", created_at=datetime.now(), @@ -52,6 +69,7 @@ class TestGetCalendarDays: ws = await workspace_repo.create( Workspace(id=uuid4(), name="Familj", created_at=datetime.now()) ) + await _seed_caller(connection, ws.id) response = await client.get( "/api/calendar/days", @@ -76,6 +94,7 @@ class TestGetCalendarDays: ws = await workspace_repo.create( Workspace(id=uuid4(), name="Familj", created_at=datetime.now()) ) + await _seed_caller(connection, ws.id) response = await client.get( "/api/calendar/days", @@ -91,6 +110,7 @@ class TestGetCalendarDays: ws = await workspace_repo.create( Workspace(id=uuid4(), name="Familj", created_at=datetime.now()) ) + await _seed_caller(connection, ws.id) response = await client.get( "/api/calendar/days", @@ -112,6 +132,7 @@ class TestGetCalendarDays: ws = await workspace_repo.create( Workspace(id=uuid4(), name="Familj", created_at=datetime.now()) ) + await _seed_caller(connection, ws.id) # Discover max_year from a normal response rather than hardcoding it. bounds_response = await client.get( @@ -144,7 +165,12 @@ class TestGetCalendarDays: Workspace(id=uuid4(), name="Familj", created_at=datetime.now()) ) await personal_data_repo.create( - PersonalData(uuid=uuid4(), workspace_id=ws.id, name="Alex", created_at=datetime.now()) + PersonalData( + uuid=_CURRENT_USER_PD_UUID, + workspace_id=ws.id, + name="Alex", + created_at=datetime.now(), + ) ) override_date = date(2026, 4, 30) # Valborgsmässoafton: not a default day off (§6) await day_override_repo.create( diff --git a/backend/tests/unit/test_config.py b/backend/tests/unit/test_config.py new file mode 100644 index 0000000..eb57015 --- /dev/null +++ b/backend/tests/unit/test_config.py @@ -0,0 +1,41 @@ +"""Tests for Settings' production hardening validator. + +Never mutates the module-level `settings` singleton — constructs fresh +`Settings()` instances instead, passing kwargs directly (bypassing env/file +loading concerns) so these tests are isolated from the process environment. +""" + +import pytest + +from app.config import Settings + + +class TestProductionJwtSecretValidation: + def test_development_allows_default_secret(self) -> None: + settings = Settings(app_env="development") + assert settings.jwt_secret == "dev-secret-change-in-production-min-32-chars" + + def test_test_env_allows_default_secret(self) -> None: + settings = Settings(app_env="test") + assert settings.app_env == "test" + + def test_production_rejects_default_secret(self) -> None: + with pytest.raises(ValueError, match="APP_JWT_SECRET"): + Settings(app_env="production") + + def test_production_rejects_short_secret(self) -> None: + with pytest.raises(ValueError, match="APP_JWT_SECRET"): + Settings(app_env="production", jwt_secret="short-secret") + + def test_production_accepts_strong_secret(self) -> None: + strong_secret = "a" * 32 + settings = Settings(app_env="production", jwt_secret=strong_secret) + assert settings.jwt_secret == strong_secret + + def test_default_app_env_is_development(self) -> None: + settings = Settings() + assert settings.app_env == "development" + + def test_default_log_level_is_info(self) -> None: + settings = Settings() + assert settings.log_level == "INFO" diff --git a/backend/tests/unit/test_db_unit_of_work.py b/backend/tests/unit/test_db_unit_of_work.py new file mode 100644 index 0000000..e4efbf7 --- /dev/null +++ b/backend/tests/unit/test_db_unit_of_work.py @@ -0,0 +1,243 @@ +"""Tests for the unit-of-work mechanism (`app.db.connection`) and the +batched `PlannedLeaveRepository` methods it exists to support. + +See `scratchpad/audit/backend-perf.md` §3: the app runs on one shared +connection, so a multi-statement write needs (a) mutual exclusion for the +whole unit of work and (b) the per-row `commit()` calls suppressed while one +is in progress. These tests exercise that mechanism directly against an +in-memory connection, plus the `PlanningService.apply_proposed_days` +atomicity it is meant to guarantee end-to-end. +""" + +import asyncio +from datetime import date, datetime +from uuid import uuid4 + +import aiosqlite +import pytest + +from app.db.connection import commit, ensure_schema, unit_of_work +from app.entities.leave_wish import LeaveWish +from app.entities.planned_leave import PlannedLeaveDay +from app.repositories_impl.sqlite.leave_wish_repository import SQLiteLeaveWishRepository +from app.repositories_impl.sqlite.planned_leave_repository import SQLitePlannedLeaveRepository +from app.services.planning_service import PlanningService, ProposedDayInput +from tests.conftest import SeededIds + + +@pytest.fixture +async def uow_connection() -> aiosqlite.Connection: + conn = await aiosqlite.connect(":memory:") + conn.row_factory = aiosqlite.Row + await conn.execute("PRAGMA foreign_keys = ON;") + await ensure_schema(conn) + try: + yield conn + finally: + await conn.close() + + +async def _workspace_count(conn: aiosqlite.Connection) -> int: + cursor = await conn.execute("SELECT COUNT(*) FROM workspace") + row = await cursor.fetchone() + return int(row[0]) + + +async def _insert_workspace(conn: aiosqlite.Connection, workspace_id: str) -> None: + await conn.execute( + "INSERT INTO workspace (id, name, planning_mode, created_at) VALUES (?, ?, ?, ?)", + (workspace_id, "Test", "solo", datetime.now().isoformat()), + ) + + +class TestUnitOfWork: + async def test_commits_on_success(self, uow_connection: aiosqlite.Connection) -> None: + async with unit_of_work(uow_connection): + await _insert_workspace(uow_connection, "ws-commit") + assert await _workspace_count(uow_connection) == 1 + + async def test_rolls_back_on_exception(self, uow_connection: aiosqlite.Connection) -> None: + with pytest.raises(ValueError): + async with unit_of_work(uow_connection): + await _insert_workspace(uow_connection, "ws-rollback") + raise ValueError("simulated failure") + assert await _workspace_count(uow_connection) == 0 + + async def test_commit_helper_is_noop_inside_uow( + self, uow_connection: aiosqlite.Connection + ) -> None: + """`commit()` must not durably commit inside a unit of work — otherwise + a later rollback in the same block could only undo writes made after + that no-op `commit()` call, not before it (see backend-perf audit §3, + "Is BEGIN/COMMIT ... safe on the shared connection? No, not alone."). + """ + with pytest.raises(RuntimeError): + async with unit_of_work(uow_connection): + await _insert_workspace(uow_connection, "ws-noop-commit") + await commit(uow_connection) # must be a no-op here + raise RuntimeError("still rolls back everything, including the row above") + assert await _workspace_count(uow_connection) == 0 + + async def test_concurrent_calls_serialize(self, uow_connection: aiosqlite.Connection) -> None: + """Two concurrent `unit_of_work()` blocks on the same connection never + interleave — the second blocks at entry until the first fully exits. + """ + events: list[str] = [] + + async def worker(name: str, hold: float) -> None: + async with unit_of_work(uow_connection): + events.append(f"{name}-start") + await asyncio.sleep(hold) + events.append(f"{name}-end") + + await asyncio.gather(worker("a", 0.05), worker("b", 0)) + + assert events in ( + ["a-start", "a-end", "b-start", "b-end"], + ["b-start", "b-end", "a-start", "a-end"], + ) + + +class TestPlannedLeaveRepositoryBatchMethods: + async def test_create_many_then_delete_many( + self, + planned_leave_repo: SQLitePlannedLeaveRepository, + seeded_ids: SeededIds, + ) -> None: + now = datetime.now() + entities = [ + PlannedLeaveDay( + id=uuid4(), + workspace_id=seeded_ids.workspace_id, + child_id=seeded_ids.child_id, + parent_personal_data_uuid=seeded_ids.personal_data_uuid, + date=date(2026, 3, day), + day_type="sjukpenning", + created_at=now, + updated_at=now, + ) + for day in (1, 2, 3) + ] + + await planned_leave_repo.create_many(entities) + rows = await planned_leave_repo.list(workspace_id=seeded_ids.workspace_id) + assert len(rows) == 3 + + deleted = await planned_leave_repo.delete_many([entities[0].id, entities[1].id]) + assert deleted == 2 + remaining = await planned_leave_repo.list(workspace_id=seeded_ids.workspace_id) + assert [r.id for r in remaining] == [entities[2].id] + + async def test_create_many_empty_list_is_noop( + self, + planned_leave_repo: SQLitePlannedLeaveRepository, + seeded_ids: SeededIds, + ) -> None: + await planned_leave_repo.create_many([]) + assert await planned_leave_repo.list(workspace_id=seeded_ids.workspace_id) == [] + + async def test_delete_many_is_idempotent( + self, planned_leave_repo: SQLitePlannedLeaveRepository + ) -> None: + """Unlike `delete()`, a missing id is silently ignored rather than raising.""" + deleted = await planned_leave_repo.delete_many([uuid4(), uuid4()]) + assert deleted == 0 + + async def test_update_metadata_many( + self, + planned_leave_repo: SQLitePlannedLeaveRepository, + leave_wish_repo: SQLiteLeaveWishRepository, + seeded_ids: SeededIds, + ) -> None: + now = datetime.now() + entity = PlannedLeaveDay( + id=uuid4(), + workspace_id=seeded_ids.workspace_id, + child_id=seeded_ids.child_id, + parent_personal_data_uuid=seeded_ids.personal_data_uuid, + date=date(2026, 4, 1), + day_type="vacation", + created_at=now, + updated_at=now, + ) + await planned_leave_repo.create(entity) + + wish = await leave_wish_repo.create( + LeaveWish( + id=uuid4(), + workspace_id=seeded_ids.workspace_id, + child_id=seeded_ids.child_id, + name="Test wish", + window_start_month=4, + window_start_day=1, + window_end_month=4, + window_end_day=30, + target_weeks=1, + recurrence="once", + first_year=2026, + last_year=None, + priority=1, + parent_mode="either", + parent_personal_data_uuid=None, + window_mode="fixed", + created_at=now, + updated_at=now, + ) + ) + + await planned_leave_repo.update_metadata_many([(entity.id, wish.id, True)]) + + refreshed = await planned_leave_repo.get_by_id(entity.id) + assert refreshed is not None + assert refreshed.wish_id == wish.id + assert refreshed.is_pinned is True + # day_type/is_dubbeldag are untouched by the metadata-only update. + assert refreshed.day_type == "vacation" + + +class TestApplyProposedDaysAtomicity: + async def test_mid_batch_failure_leaves_nothing_persisted( + self, + planning_service_with_uow: PlanningService, + planned_leave_repo: SQLitePlannedLeaveRepository, + seeded_ids: SeededIds, + monkeypatch: pytest.MonkeyPatch, + ) -> None: + """A batch that replaces an existing row (delete + create, both inside + stage 4) must not leave the delete applied if the create half fails — + the whole `async with self._uow():` block must roll back together. + Uses `planning_service_with_uow` (a real `SQLiteUnitOfWork`), not the + default `planning_service` fixture, which wires a no-op unit of work + and would (correctly, given a no-op) leave the delete committed. + """ + existing = await planning_service_with_uow.add_planned_day( + workspace_id=seeded_ids.workspace_id, + child_id=seeded_ids.child_id, + parent_personal_data_uuid=seeded_ids.personal_data_uuid, + date=date(2026, 5, 1), + day_type="sjukpenning", + ) + + async def boom(_items: list[PlannedLeaveDay]) -> None: + raise RuntimeError("simulated mid-batch failure") + + monkeypatch.setattr(planned_leave_repo, "create_many", boom) + + days = [ + ProposedDayInput( + date=date(2026, 5, 1), + parent_personal_data_uuid=seeded_ids.personal_data_uuid, + child_id=seeded_ids.child_id, + day_type="lagsta", # differs from the existing row -> collision -> to_replace + ) + ] + + with pytest.raises(RuntimeError): + await planning_service_with_uow.apply_proposed_days( + workspace_id=seeded_ids.workspace_id, days=days, replace=True + ) + + remaining = await planned_leave_repo.list(workspace_id=seeded_ids.workspace_id) + assert len(remaining) == 1 + assert remaining[0].id == existing.id + assert remaining[0].day_type == "sjukpenning" diff --git a/backend/tests/unit/test_fetch_data_router.py b/backend/tests/unit/test_fetch_data_router.py index f09bb92..37608be 100644 --- a/backend/tests/unit/test_fetch_data_router.py +++ b/backend/tests/unit/test_fetch_data_router.py @@ -28,40 +28,24 @@ non-headless browser as a fire-and-forget background task, which is unsafe to do from a router test). """ -from datetime import datetime from typing import Any -from uuid import UUID, uuid4 +from uuid import UUID -import aiosqlite import pytest import pytest_asyncio -from httpx import ASGITransport, AsyncClient +from httpx import AsyncClient -from app.dependencies import get_connection, get_current_user -from app.entities.user import User -from app.main import app from app.routers import fetch_data as fetch_data_router from tests.conftest import SeededIds @pytest_asyncio.fixture -async def client(connection: aiosqlite.Connection) -> AsyncClient: - """An httpx client against the real app: `get_connection` and - `get_current_user` are overridden to use the test's in-memory connection - and a fixed fake user. +async def client(authed_client: AsyncClient) -> AsyncClient: + """The shared `seeded_ids`-bound authenticated client (see + `tests/conftest.py`) — required now that the routes reject ids outside + the caller's workspace. See CLAUDE.md T1 authz work / audit §4. """ - app.dependency_overrides[get_connection] = lambda: connection - app.dependency_overrides[get_current_user] = lambda: User( - id=uuid4(), - personal_data_uuid=uuid4(), - email="test@example.com", - hashed_password="not-a-real-hash", - created_at=datetime.now(), - ) - async with AsyncClient(transport=ASGITransport(app=app), base_url="http://test") as ac: - yield ac - app.dependency_overrides.pop(get_connection, None) - app.dependency_overrides.pop(get_current_user, None) + return authed_client def _manual_payload(seeded_ids: SeededIds, **overrides: object) -> dict: @@ -233,11 +217,17 @@ class TestScraperEndpointsEnabled: captured: dict[str, Any] = {} async def fake_start_session( - *, workspace_id: UUID, personal_data_uuid: UUID, child_id: UUID, persist_callback: Any + *, + workspace_id: UUID, + personal_data_uuid: UUID, + child_id: UUID, + persist_callback: Any, + owner_user_id: UUID, ) -> str: captured["workspace_id"] = workspace_id captured["personal_data_uuid"] = personal_data_uuid captured["child_id"] = child_id + captured["owner_user_id"] = owner_user_id return "fake-session-id" monkeypatch.setattr(fetch_data_router._scraper_service, "start_session", fake_start_session) @@ -263,7 +253,7 @@ class TestScraperEndpointsEnabled: ) -> None: monkeypatch.setattr(fetch_data_router.settings, "scraper_enabled", True) - async def fake_poll_session(session_id: str) -> dict: + async def fake_poll_session(session_id: str, owner_user_id: UUID) -> dict: assert session_id == "some-session-id" return {"status": "awaiting_scan"} @@ -283,7 +273,7 @@ class TestScraperEndpointsEnabled: cancelled: dict[str, Any] = {} - async def fake_cancel_session(session_id: str) -> None: + async def fake_cancel_session(session_id: str, owner_user_id: UUID) -> None: cancelled["session_id"] = session_id monkeypatch.setattr( diff --git a/backend/tests/unit/test_fk_scraper.py b/backend/tests/unit/test_fk_scraper.py index 112bee7..741e2b0 100644 --- a/backend/tests/unit/test_fk_scraper.py +++ b/backend/tests/unit/test_fk_scraper.py @@ -35,15 +35,16 @@ from app.scrapers.fk_scraper import ( # Helpers for the HTML-fixture tests # --------------------------------------------------------------------------- -CAPTURES_DIR = Path(__file__).parents[3] / "captures" +FIXTURES_DIR = Path(__file__).parents[1] / "fixtures" def _load_fixture(name: str) -> str: - """Read a page capture from captures/, skipping if not present.""" - path = CAPTURES_DIR / name - if not path.exists(): - pytest.skip(f"Fixture {name} not found — run the captures step first") - return path.read_text(encoding="utf-8") + """Read a synthetic HTML fixture from tests/fixtures/. + + Fixtures here are hand-built (never real FK page captures — see F1 of + the scraper/config/ops security audit) and always present in the repo. + """ + return (FIXTURES_DIR / name).read_text(encoding="utf-8") # --------------------------------------------------------------------------- @@ -144,84 +145,90 @@ from app.scrapers.fk_scraper import _extract_parental_leave # noqa: E402 class TestExtractFromFixture: - """Parse the captured overview HTML and assert correct field values.""" + """Parse the synthetic overview HTML fixture and assert correct field values.""" async def test_days_left_and_total_parsed(self) -> None: - html = _load_fixture("05_foraldrapenning_overview.html") + html = _load_fixture("fk_overview_synthetic.html") state = _SessionState( session_id="test-1", workspace_id=uuid.uuid4(), personal_data_uuid=uuid.uuid4(), child_id=uuid.uuid4(), + owner_user_id=uuid.uuid4(), ) page = _make_mock_page(html) result = await _extract_parental_leave(page, state) - # From the capture: 164 av 240 dagar - assert result.days_left == 164 + # From the synthetic fixture: 100 av 240 dagar + assert result.days_left == 100 assert result.total_days == 240 - assert result.days_used == 76 + assert result.days_used == 140 async def test_tier_values_parsed(self) -> None: - html = _load_fixture("05_foraldrapenning_overview.html") + html = _load_fixture("fk_overview_synthetic.html") state = _SessionState( session_id="test-2", workspace_id=uuid.uuid4(), personal_data_uuid=uuid.uuid4(), child_id=uuid.uuid4(), + owner_user_id=uuid.uuid4(), ) page = _make_mock_page(html) result = await _extract_parental_leave(page, state) - # From the capture: sjukpenning 119, lägstanivå 45 - assert result.tier_data.sjukpenning_left == 119 - assert result.tier_data.lagsta_niva_left == 45 + # From the synthetic fixture: sjukpenning 60, lägstanivå 40 + assert result.tier_data.sjukpenning_left == 60 + assert result.tier_data.lagsta_niva_left == 40 async def test_dubbeldagar_parsed(self) -> None: - html = _load_fixture("05_foraldrapenning_overview.html") + html = _load_fixture("fk_overview_synthetic.html") state = _SessionState( session_id="test-3", workspace_id=uuid.uuid4(), personal_data_uuid=uuid.uuid4(), child_id=uuid.uuid4(), + owner_user_id=uuid.uuid4(), ) page = _make_mock_page(html) result = await _extract_parental_leave(page, state) - # From the capture: 39 av 60 dubbeldagar - assert result.dubbeldagar_left == 39 + # From the synthetic fixture: 20 av 60 dubbeldagar + assert result.dubbeldagar_left == 20 async def test_source_is_scraped(self) -> None: - html = _load_fixture("05_foraldrapenning_overview.html") + html = _load_fixture("fk_overview_synthetic.html") state = _SessionState( session_id="test-4", workspace_id=uuid.uuid4(), personal_data_uuid=uuid.uuid4(), child_id=uuid.uuid4(), + owner_user_id=uuid.uuid4(), ) page = _make_mock_page(html) result = await _extract_parental_leave(page, state) assert result.source == "scraped" async def test_reserved_days_derived(self) -> None: - html = _load_fixture("05_foraldrapenning_overview.html") + html = _load_fixture("fk_overview_synthetic.html") state = _SessionState( session_id="test-5", workspace_id=uuid.uuid4(), personal_data_uuid=uuid.uuid4(), child_id=uuid.uuid4(), + owner_user_id=uuid.uuid4(), ) page = _make_mock_page(html) result = await _extract_parental_leave(page, state) - # 76 days used < 150 transferable → all reserved days remain + # 140 days used < 150 transferable → all reserved days remain assert result.reserved_days_total == 90 assert result.reserved_days_left == 90 - assert result.transferable_days_left == 240 - 90 - 76 # 74 + assert result.transferable_days_left == 240 - 90 - 140 # 10 async def test_tier_totals_sum_to_total_days(self) -> None: - html = _load_fixture("05_foraldrapenning_overview.html") + html = _load_fixture("fk_overview_synthetic.html") state = _SessionState( session_id="test-6", workspace_id=uuid.uuid4(), personal_data_uuid=uuid.uuid4(), child_id=uuid.uuid4(), + owner_user_id=uuid.uuid4(), ) page = _make_mock_page(html) result = await _extract_parental_leave(page, state) @@ -231,12 +238,13 @@ class TestExtractFromFixture: ) async def test_tier_used_nonnegative(self) -> None: - html = _load_fixture("05_foraldrapenning_overview.html") + html = _load_fixture("fk_overview_synthetic.html") state = _SessionState( session_id="test-7", workspace_id=uuid.uuid4(), personal_data_uuid=uuid.uuid4(), child_id=uuid.uuid4(), + owner_user_id=uuid.uuid4(), ) page = _make_mock_page(html) result = await _extract_parental_leave(page, state) @@ -296,6 +304,7 @@ class TestValidation: workspace_id=uuid.uuid4(), personal_data_uuid=uuid.uuid4(), child_id=uuid.uuid4(), + owner_user_id=uuid.uuid4(), ) async def test_negative_days_left_raises(self) -> None: @@ -390,43 +399,51 @@ class TestSessionStateMachine: async def test_start_session_creates_entry(self) -> None: service = FKScraperService() + owner_user_id = uuid.uuid4() with patch("app.scrapers.fk_scraper._run_session", new_callable=AsyncMock) as mock_run: mock_run.return_value = None sid = await service.start_session( workspace_id=uuid.uuid4(), personal_data_uuid=uuid.uuid4(), child_id=uuid.uuid4(), + owner_user_id=owner_user_id, ) assert sid in _sessions assert _sessions[sid].status == "launching" + assert _sessions[sid].owner_user_id == owner_user_id async def test_poll_unknown_session_returns_error(self) -> None: service = FKScraperService() - result = await service.poll_session("nonexistent") + result = await service.poll_session("nonexistent", uuid.uuid4()) assert result["status"] == "error" assert "Unknown" in result["message"] async def test_poll_awaiting_scan_returns_qr_data(self) -> None: service = FKScraperService() sid = str(uuid.uuid4()) + owner_user_id = uuid.uuid4() state = _SessionState( session_id=sid, workspace_id=uuid.uuid4(), personal_data_uuid=uuid.uuid4(), child_id=uuid.uuid4(), + owner_user_id=owner_user_id, status="awaiting_scan", qr_data=b"PNG_BYTES", ) _sessions[sid] = state - result = await service.poll_session(sid) + result = await service.poll_session(sid, owner_user_id) assert result["status"] == "awaiting_scan" assert "qr_data" in result import base64 assert base64.b64decode(result["qr_data"]) == b"PNG_BYTES" - async def test_cancel_sets_status(self) -> None: + async def test_poll_with_wrong_owner_is_treated_as_unknown_session(self) -> None: + """F4 (T1 authz audit): a session_id known to a caller who did not + start it must behave EXACTLY like a made-up session_id — never leak + that the session exists (no QR data, no distinct error).""" service = FKScraperService() sid = str(uuid.uuid4()) state = _SessionState( @@ -434,16 +451,72 @@ class TestSessionStateMachine: workspace_id=uuid.uuid4(), personal_data_uuid=uuid.uuid4(), child_id=uuid.uuid4(), + owner_user_id=uuid.uuid4(), + status="awaiting_scan", + qr_data=b"PNG_BYTES", + ) + _sessions[sid] = state + + result = await service.poll_session(sid, uuid.uuid4()) # different owner + assert result == {"status": "error", "message": "Unknown session_id"} + + async def test_cancel_sets_status(self) -> None: + service = FKScraperService() + sid = str(uuid.uuid4()) + owner_user_id = uuid.uuid4() + state = _SessionState( + session_id=sid, + workspace_id=uuid.uuid4(), + personal_data_uuid=uuid.uuid4(), + child_id=uuid.uuid4(), + owner_user_id=owner_user_id, status="awaiting_scan", ) _sessions[sid] = state - await service.cancel_session(sid) + await service.cancel_session(sid, owner_user_id) assert _sessions[sid].status == "cancelled" + async def test_cancel_clears_qr_data(self) -> None: + """F5: cancelling a session must not leave the last QR frame in RAM.""" + service = FKScraperService() + sid = str(uuid.uuid4()) + owner_user_id = uuid.uuid4() + state = _SessionState( + session_id=sid, + workspace_id=uuid.uuid4(), + personal_data_uuid=uuid.uuid4(), + child_id=uuid.uuid4(), + owner_user_id=owner_user_id, + status="awaiting_scan", + qr_data=b"PNG_BYTES", + ) + _sessions[sid] = state + + await service.cancel_session(sid, owner_user_id) + assert _sessions[sid].qr_data is None + + async def test_cancel_with_wrong_owner_is_a_no_op(self) -> None: + """F4: cancelling with the wrong owner must not affect the session — + exactly like cancelling an unknown session_id.""" + service = FKScraperService() + sid = str(uuid.uuid4()) + state = _SessionState( + session_id=sid, + workspace_id=uuid.uuid4(), + personal_data_uuid=uuid.uuid4(), + child_id=uuid.uuid4(), + owner_user_id=uuid.uuid4(), + status="awaiting_scan", + ) + _sessions[sid] = state + + await service.cancel_session(sid, uuid.uuid4()) # different owner + assert _sessions[sid].status == "awaiting_scan" + async def test_cancel_nonexistent_session_is_safe(self) -> None: service = FKScraperService() - await service.cancel_session("does-not-exist") # should not raise + await service.cancel_session("does-not-exist", uuid.uuid4()) # should not raise async def test_ttl_watchdog_cancels_stale_session(self) -> None: from app.scrapers.fk_scraper import _ttl_watchdog @@ -456,6 +529,7 @@ class TestSessionStateMachine: workspace_id=uuid.uuid4(), personal_data_uuid=uuid.uuid4(), child_id=uuid.uuid4(), + owner_user_id=uuid.uuid4(), status="awaiting_scan", ) _sessions[sid] = state @@ -465,6 +539,106 @@ class TestSessionStateMachine: assert _sessions[sid].status == "error" assert "timed out" in (_sessions[sid].message or "") + async def test_ttl_watchdog_purges_already_terminal_session(self) -> None: + """F5: if the session already finished by the time the watchdog + fires, it's a safety net that purges it immediately rather than + leaving it for `_purge_later`.""" + from app.scrapers.fk_scraper import _ttl_watchdog + + with patch("app.scrapers.fk_scraper.settings") as mock_settings: + mock_settings.scraper_session_ttl_seconds = 0 # immediate + sid = str(uuid.uuid4()) + state = _SessionState( + session_id=sid, + workspace_id=uuid.uuid4(), + personal_data_uuid=uuid.uuid4(), + child_id=uuid.uuid4(), + owner_user_id=uuid.uuid4(), + status="done", + ) + _sessions[sid] = state + + await _ttl_watchdog(sid) + + assert sid not in _sessions + + async def test_purge_later_removes_session_after_retention_window(self) -> None: + """F5: a finished session is removed from `_sessions` after + `scraper_result_retention_seconds`.""" + from app.scrapers.fk_scraper import _purge_later + + with patch("app.scrapers.fk_scraper.settings") as mock_settings: + mock_settings.scraper_result_retention_seconds = 0 # immediate + sid = str(uuid.uuid4()) + state = _SessionState( + session_id=sid, + workspace_id=uuid.uuid4(), + personal_data_uuid=uuid.uuid4(), + child_id=uuid.uuid4(), + owner_user_id=uuid.uuid4(), + status="done", + result=None, + ) + _sessions[sid] = state + + await _purge_later(sid) + + assert sid not in _sessions + + async def test_purge_later_is_a_no_op_if_already_purged(self) -> None: + from app.scrapers.fk_scraper import _purge_later + + with patch("app.scrapers.fk_scraper.settings") as mock_settings: + mock_settings.scraper_result_retention_seconds = 0 + await _purge_later("does-not-exist") # should not raise + + async def test_start_session_cancels_existing_session_for_same_owner(self) -> None: + """F5: at most one live session per owner — starting a new one + cancels any non-terminal session already in flight for that owner.""" + service = FKScraperService() + owner_user_id = uuid.uuid4() + with patch("app.scrapers.fk_scraper._run_session", new_callable=AsyncMock): + first_sid = await service.start_session( + workspace_id=uuid.uuid4(), + personal_data_uuid=uuid.uuid4(), + child_id=uuid.uuid4(), + owner_user_id=owner_user_id, + ) + _sessions[first_sid].status = "awaiting_scan" + + second_sid = await service.start_session( + workspace_id=uuid.uuid4(), + personal_data_uuid=uuid.uuid4(), + child_id=uuid.uuid4(), + owner_user_id=owner_user_id, + ) + + assert _sessions[first_sid].status == "cancelled" + assert _sessions[second_sid].status == "launching" + assert first_sid != second_sid + + async def test_start_session_does_not_cancel_other_owners_session(self) -> None: + service = FKScraperService() + owner_a = uuid.uuid4() + owner_b = uuid.uuid4() + with patch("app.scrapers.fk_scraper._run_session", new_callable=AsyncMock): + sid_a = await service.start_session( + workspace_id=uuid.uuid4(), + personal_data_uuid=uuid.uuid4(), + child_id=uuid.uuid4(), + owner_user_id=owner_a, + ) + _sessions[sid_a].status = "awaiting_scan" + + await service.start_session( + workspace_id=uuid.uuid4(), + personal_data_uuid=uuid.uuid4(), + child_id=uuid.uuid4(), + owner_user_id=owner_b, + ) + + assert _sessions[sid_a].status == "awaiting_scan" + async def test_set_status_updates_message(self) -> None: sid = str(uuid.uuid4()) state = _SessionState( @@ -472,6 +646,7 @@ class TestSessionStateMachine: workspace_id=uuid.uuid4(), personal_data_uuid=uuid.uuid4(), child_id=uuid.uuid4(), + owner_user_id=uuid.uuid4(), ) _set_status(state, "error", "Something went wrong") assert state.status == "error" @@ -517,6 +692,7 @@ class TestSessionStateMachine: workspace_id=uuid.uuid4(), personal_data_uuid=uuid.uuid4(), child_id=uuid.uuid4(), + owner_user_id=uuid.uuid4(), persist_callback=mock_persist, ) _sessions[sid] = state @@ -575,6 +751,7 @@ class TestSessionStateMachine: workspace_id=uuid.uuid4(), personal_data_uuid=uuid.uuid4(), child_id=uuid.uuid4(), + owner_user_id=uuid.uuid4(), persist_callback=failing_persist, ) _sessions[sid] = state @@ -627,6 +804,7 @@ class TestSessionStateMachine: workspace_id=uuid.uuid4(), personal_data_uuid=uuid.uuid4(), child_id=uuid.uuid4(), + owner_user_id=uuid.uuid4(), ) _sessions[sid] = state @@ -654,6 +832,58 @@ class TestSessionStateMachine: assert call_args[1]["timeout"] == 30_000 +# --------------------------------------------------------------------------- +# F3: debug artifacts are opt-in (contain personal data when written) +# --------------------------------------------------------------------------- + + +class TestSaveDebugArtifacts: + def _make_page(self) -> MagicMock: + async def _fake_screenshot(path: str, full_page: bool = True) -> None: + # Real Playwright writes the PNG to `path` itself; mirror that so + # the "both files got written" assertion is meaningful. + Path(path).write_bytes(b"fake-png-bytes") + + page = MagicMock() + page.content = AsyncMock(return_value="fake") + page.screenshot = AsyncMock(side_effect=_fake_screenshot) + return page + + async def test_noop_when_debug_disabled_even_with_dir_set(self, tmp_path) -> None: + from app.scrapers.fk_scraper import _save_debug_artifacts + + with patch("app.scrapers.fk_scraper.settings") as mock_settings: + mock_settings.scraper_debug_dir = str(tmp_path) + mock_settings.scraper_debug_enabled = False + + await _save_debug_artifacts(self._make_page(), "sid-1", ["[log] hi"]) + + assert list(tmp_path.iterdir()) == [] + + async def test_noop_when_dir_unset_even_if_enabled(self, tmp_path) -> None: + from app.scrapers.fk_scraper import _save_debug_artifacts + + with patch("app.scrapers.fk_scraper.settings") as mock_settings: + mock_settings.scraper_debug_dir = None + mock_settings.scraper_debug_enabled = True + + await _save_debug_artifacts(self._make_page(), "sid-2", ["[log] hi"]) + + assert list(tmp_path.iterdir()) == [] + + async def test_writes_artifacts_when_both_enabled_and_dir_set(self, tmp_path) -> None: + from app.scrapers.fk_scraper import _save_debug_artifacts + + with patch("app.scrapers.fk_scraper.settings") as mock_settings: + mock_settings.scraper_debug_dir = str(tmp_path) + mock_settings.scraper_debug_enabled = True + + await _save_debug_artifacts(self._make_page(), "sid-3", ["[log] hi"]) + + written = {p.name for p in tmp_path.iterdir()} + assert written == {"sid-3.html", "sid-3.png", "sid-3.console.txt"} + + # --------------------------------------------------------------------------- # Persistence integration: real SQLite repo round-trip # --------------------------------------------------------------------------- @@ -702,6 +932,7 @@ class TestPersistenceIntegration: workspace_id=seeded_ids.workspace_id, personal_data_uuid=seeded_ids.personal_data_uuid, child_id=seeded_ids.child_id, + owner_user_id=uuid.uuid4(), persist_callback=persist_callback, ) _sessions[sid] = state diff --git a/backend/tests/unit/test_leave_wish_router.py b/backend/tests/unit/test_leave_wish_router.py index ab75e6a..67854db 100644 --- a/backend/tests/unit/test_leave_wish_router.py +++ b/backend/tests/unit/test_leave_wish_router.py @@ -14,11 +14,19 @@ import pytest_asyncio from httpx import ASGITransport, AsyncClient from app.dependencies import get_connection, get_current_user +from app.entities.personal_data import PersonalData from app.entities.user import User from app.entities.workspace import Workspace from app.main import app +from app.repositories_impl.sqlite.personal_data_repository import SQLitePersonalDataRepository from app.repositories_impl.sqlite.workspace_repository import SQLiteWorkspaceRepository +# Fixed identity for the authenticated caller in every test in this file — +# each test must also create a `PersonalData` row with this uuid in the +# workspace it seeds, so `get_current_personal_data` resolves to that +# workspace (see CLAUDE.md T1 authz work / audit §4 blast radius). +_CURRENT_USER_PD_UUID = uuid4() + @pytest_asyncio.fixture async def client(connection: aiosqlite.Connection) -> AsyncClient: @@ -29,7 +37,7 @@ async def client(connection: aiosqlite.Connection) -> AsyncClient: app.dependency_overrides[get_connection] = lambda: connection app.dependency_overrides[get_current_user] = lambda: User( id=uuid4(), - personal_data_uuid=uuid4(), + personal_data_uuid=_CURRENT_USER_PD_UUID, email="test@example.com", hashed_password="not-a-real-hash", created_at=datetime.now(), @@ -53,6 +61,11 @@ class TestLeaveWishTargetDaysRoundTrip: now = datetime.now() workspace_repo = SQLiteWorkspaceRepository(connection) ws = await workspace_repo.create(Workspace(id=uuid4(), name="Familj", created_at=now)) + await SQLitePersonalDataRepository(connection).create( + PersonalData( + uuid=_CURRENT_USER_PD_UUID, workspace_id=ws.id, name="Alex", created_at=now + ) + ) target_days_value = 15 response = await client.post( @@ -91,6 +104,11 @@ class TestLeaveWishTargetDaysRoundTrip: now = datetime.now() workspace_repo = SQLiteWorkspaceRepository(connection) ws = await workspace_repo.create(Workspace(id=uuid4(), name="Familj", created_at=now)) + await SQLitePersonalDataRepository(connection).create( + PersonalData( + uuid=_CURRENT_USER_PD_UUID, workspace_id=ws.id, name="Alex", created_at=now + ) + ) response = await client.post( "/api/leave-wishes", @@ -121,6 +139,11 @@ class TestLeaveWishTargetDaysRoundTrip: now = datetime.now() workspace_repo = SQLiteWorkspaceRepository(connection) ws = await workspace_repo.create(Workspace(id=uuid4(), name="Familj", created_at=now)) + await SQLitePersonalDataRepository(connection).create( + PersonalData( + uuid=_CURRENT_USER_PD_UUID, workspace_id=ws.id, name="Alex", created_at=now + ) + ) # Create a wish without target_days create_response = await client.post( @@ -168,6 +191,11 @@ class TestLeaveWishTargetDaysRoundTrip: now = datetime.now() workspace_repo = SQLiteWorkspaceRepository(connection) ws = await workspace_repo.create(Workspace(id=uuid4(), name="Familj", created_at=now)) + await SQLitePersonalDataRepository(connection).create( + PersonalData( + uuid=_CURRENT_USER_PD_UUID, workspace_id=ws.id, name="Alex", created_at=now + ) + ) # Create a wish with target_days target_days_value = 25 @@ -212,6 +240,11 @@ class TestLeaveWishTargetDaysRoundTrip: now = datetime.now() workspace_repo = SQLiteWorkspaceRepository(connection) ws = await workspace_repo.create(Workspace(id=uuid4(), name="Familj", created_at=now)) + await SQLitePersonalDataRepository(connection).create( + PersonalData( + uuid=_CURRENT_USER_PD_UUID, workspace_id=ws.id, name="Alex", created_at=now + ) + ) # Create a wish with target_days target_days_value = 30 diff --git a/backend/tests/unit/test_plan_router.py b/backend/tests/unit/test_plan_router.py index d90b580..8a2be65 100644 --- a/backend/tests/unit/test_plan_router.py +++ b/backend/tests/unit/test_plan_router.py @@ -24,10 +24,11 @@ import pytest_asyncio from dateutil.relativedelta import relativedelta from httpx import ASGITransport, AsyncClient -from app.dependencies import get_connection +from app.dependencies import get_connection, get_current_user from app.entities.child import Child from app.entities.parental_leave import ParentalLeaveFetch, TierData from app.entities.personal_data import PersonalData +from app.entities.user import User from app.entities.vacation_days import VacationDays from app.entities.workspace import Workspace from app.main import app @@ -38,6 +39,14 @@ from app.repositories_impl.sqlite.vacation_days_repository import SQLiteVacation from app.repositories_impl.sqlite.workspace_repository import SQLiteWorkspaceRepository from app.services.allocation_service import PlanProposal, WindowOutcome +# Fixed identity for the authenticated caller in every test in this file. +# Each test creates its own workspace + "parent" PersonalData row — that row +# is created with THIS uuid (instead of a random one) so the caller's +# `get_current_personal_data` -> `workspace_id` resolves to the workspace the +# test just seeded. See CLAUDE.md T1 authz work / audit §4 blast radius. +_CURRENT_USER_PD_UUID = uuid4() +_CURRENT_USER_ID = uuid4() + def _make_fetch( personal_data_uuid, @@ -82,9 +91,17 @@ async def client(connection: aiosqlite.Connection) -> AsyncClient: on-disk database via the (never-run) lifespan. """ app.dependency_overrides[get_connection] = lambda: connection + app.dependency_overrides[get_current_user] = lambda: User( + id=_CURRENT_USER_ID, + personal_data_uuid=_CURRENT_USER_PD_UUID, + email="alex@example.com", + hashed_password="x", + created_at=datetime.now(), + ) async with AsyncClient(transport=ASGITransport(app=app), base_url="http://test") as ac: yield ac app.dependency_overrides.pop(get_connection, None) + app.dependency_overrides.pop(get_current_user, None) class TestProposeSurplusAdviceOverTheWire: @@ -107,7 +124,7 @@ class TestProposeSurplusAdviceOverTheWire: ws = await workspace_repo.create(Workspace(id=uuid4(), name="Familj", created_at=now)) parent = await personal_data_repo.create( PersonalData( - uuid=uuid4(), + uuid=_CURRENT_USER_PD_UUID, workspace_id=ws.id, name="Alex", salary_type="monthly", @@ -178,7 +195,7 @@ class TestProposeAge4ProjectionsOverTheWire: ws = await workspace_repo.create(Workspace(id=uuid4(), name="Familj", created_at=now)) parent = await personal_data_repo.create( PersonalData( - uuid=uuid4(), + uuid=_CURRENT_USER_PD_UUID, workspace_id=ws.id, name="Alex", salary_type="monthly", @@ -254,7 +271,7 @@ class TestProposeMessageKeysOverTheWire: ws = await workspace_repo.create(Workspace(id=uuid4(), name="Familj", created_at=now)) parent = await personal_data_repo.create( PersonalData( - uuid=uuid4(), + uuid=_CURRENT_USER_PD_UUID, workspace_id=ws.id, name="Alex", salary_type="monthly", @@ -369,7 +386,7 @@ class TestProposeWishNameMessageKeyOverTheWire: ws = await workspace_repo.create(Workspace(id=uuid4(), name="Familj", created_at=now)) parent = await personal_data_repo.create( PersonalData( - uuid=uuid4(), + uuid=_CURRENT_USER_PD_UUID, workspace_id=ws.id, name="Alex", salary_type="monthly", @@ -451,7 +468,7 @@ class TestProposeWishNameMessageKeyOverTheWire: ws = await workspace_repo.create(Workspace(id=uuid4(), name="Familj", created_at=now)) parent = await personal_data_repo.create( PersonalData( - uuid=uuid4(), + uuid=_CURRENT_USER_PD_UUID, workspace_id=ws.id, name="Alex", salary_type="monthly", @@ -528,7 +545,7 @@ class TestApplyRejectionsOverTheWireMessageKeys: ws = await workspace_repo.create(Workspace(id=uuid4(), name="Familj", created_at=now)) parent = await personal_data_repo.create( PersonalData( - uuid=uuid4(), + uuid=_CURRENT_USER_PD_UUID, workspace_id=ws.id, name="Alex", salary_type="monthly", @@ -598,7 +615,13 @@ class TestLeaveWishWindowModeOverTheWire: ) -> None: now = datetime.now() workspace_repo = SQLiteWorkspaceRepository(connection) + personal_data_repo = SQLitePersonalDataRepository(connection) ws = await workspace_repo.create(Workspace(id=uuid4(), name="Familj", created_at=now)) + await personal_data_repo.create( + PersonalData( + uuid=_CURRENT_USER_PD_UUID, workspace_id=ws.id, name="Alex", created_at=now + ) + ) response = await client.post( "/api/leave-wishes", @@ -632,7 +655,13 @@ class TestLeaveWishWindowModeOverTheWire: ) -> None: now = datetime.now() workspace_repo = SQLiteWorkspaceRepository(connection) + personal_data_repo = SQLitePersonalDataRepository(connection) ws = await workspace_repo.create(Workspace(id=uuid4(), name="Familj", created_at=now)) + await personal_data_repo.create( + PersonalData( + uuid=_CURRENT_USER_PD_UUID, workspace_id=ws.id, name="Alex", created_at=now + ) + ) response = await client.post( "/api/leave-wishes", @@ -668,7 +697,11 @@ class TestLeaveWishWindowModeOverTheWire: ws = await workspace_repo.create(Workspace(id=uuid4(), name="Familj", created_at=now)) parent = await personal_data_repo.create( PersonalData( - uuid=uuid4(), workspace_id=ws.id, name="Alex", salary_type="monthly", created_at=now + uuid=_CURRENT_USER_PD_UUID, + workspace_id=ws.id, + name="Alex", + salary_type="monthly", + created_at=now, ) ) child = await child_repo.create( @@ -740,7 +773,11 @@ class TestLeaveWishWindowModeOverTheWire: ws = await workspace_repo.create(Workspace(id=uuid4(), name="Familj", created_at=now)) parent = await personal_data_repo.create( PersonalData( - uuid=uuid4(), workspace_id=ws.id, name="Alex", salary_type="monthly", created_at=now + uuid=_CURRENT_USER_PD_UUID, + workspace_id=ws.id, + name="Alex", + salary_type="monthly", + created_at=now, ) ) child = await child_repo.create( @@ -924,7 +961,7 @@ class TestPlanProposalFieldParityWithDomain: ws = await workspace_repo.create(Workspace(id=uuid4(), name="Familj", created_at=now)) parent = await personal_data_repo.create( PersonalData( - uuid=uuid4(), + uuid=_CURRENT_USER_PD_UUID, workspace_id=ws.id, name="Alex", salary_type="monthly", diff --git a/backend/tests/unit/test_services/test_auth_service.py b/backend/tests/unit/test_services/test_auth_service.py index a67fa1f..dfd2bfa 100644 --- a/backend/tests/unit/test_services/test_auth_service.py +++ b/backend/tests/unit/test_services/test_auth_service.py @@ -2,6 +2,7 @@ import pytest +from app.repositories_impl.sqlite.user_repository import SQLiteUserRepository from app.services.auth_service import AuthService from app.utils.errors import AuthError, ConflictError, UnauthorizedError @@ -34,6 +35,35 @@ class TestAuthServiceRegisterLogin: with pytest.raises(AuthError): await auth_service.login("nobody@example.com", "anything") + async def test_register_conflict_message_is_generic(self, auth_service: AuthService) -> None: + await auth_service.register("henry@example.com", "password123", "Henry") + with pytest.raises(ConflictError) as exc_info: + await auth_service.register("henry@example.com", "otherpass", "Henry2") + assert "henry@example.com" not in str(exc_info.value) + assert str(exc_info.value) == "Email already registered" + + async def test_login_wrong_password_and_unknown_email_share_message( + self, auth_service: AuthService + ) -> None: + await auth_service.register("ivy@example.com", "correctpass", "Ivy") + with pytest.raises(AuthError) as wrong_password: + await auth_service.login("ivy@example.com", "wrongpass") + with pytest.raises(AuthError) as unknown_email: + await auth_service.login("nobody2@example.com", "anything") + assert str(wrong_password.value) == str(unknown_email.value) + + async def test_login_inactive_account_raises_unauthorized( + self, + auth_service: AuthService, + user_repo: SQLiteUserRepository, + ) -> None: + user, _ = await auth_service.register("jack@example.com", "password123", "Jack") + user.is_active = False + await user_repo.update(user.id, user) + + with pytest.raises(UnauthorizedError): + await auth_service.login("jack@example.com", "password123") + class TestAuthServiceTokens: async def test_access_token_round_trip(self, auth_service: AuthService) -> None: @@ -60,3 +90,17 @@ class TestAuthServiceTokens: async def test_invalid_token_raises_unauthorized(self, auth_service: AuthService) -> None: with pytest.raises(UnauthorizedError): await auth_service.get_user_from_token("not.a.valid.jwt") + + async def test_token_for_inactive_user_raises_unauthorized( + self, + auth_service: AuthService, + user_repo: SQLiteUserRepository, + ) -> None: + user, _ = await auth_service.register("heidi@example.com", "pass3456", "Heidi") + token = auth_service.create_access_token(user.id) + + user.is_active = False + await user_repo.update(user.id, user) + + with pytest.raises(UnauthorizedError): + await auth_service.get_user_from_token(token) diff --git a/backend/tests/unit/test_tips_router.py b/backend/tests/unit/test_tips_router.py index 11f3cc6..c78f408 100644 --- a/backend/tests/unit/test_tips_router.py +++ b/backend/tests/unit/test_tips_router.py @@ -57,12 +57,19 @@ def _make_fetch(personal_data_uuid, workspace_id, child_id, used: int = 20) -> P ) +# Fixed identity for the authenticated caller in every test in this file — +# `workspace_with_expiring_days` creates its parent ("Alice") with this uuid +# so `get_current_personal_data` resolves to the seeded workspace (see +# CLAUDE.md T1 authz work / audit §4 blast radius). +_CURRENT_USER_PD_UUID = uuid4() + + @pytest_asyncio.fixture async def client(connection: aiosqlite.Connection) -> AsyncClient: app.dependency_overrides[get_connection] = lambda: connection app.dependency_overrides[get_current_user] = lambda: User( id=uuid4(), - personal_data_uuid=uuid4(), + personal_data_uuid=_CURRENT_USER_PD_UUID, email="test@example.com", hashed_password="not-a-real-hash", created_at=datetime.now(), @@ -83,7 +90,7 @@ async def workspace_with_expiring_days(connection: aiosqlite.Connection): ws = await workspace_repo.create(Workspace(id=uuid4(), name="Familj", created_at=now)) parent = await personal_data_repo.create( - PersonalData(uuid=uuid4(), workspace_id=ws.id, name="Alice", created_at=now) + PersonalData(uuid=_CURRENT_USER_PD_UUID, workspace_id=ws.id, name="Alice", created_at=now) ) child = await child_repo.create( Child( diff --git a/backend/tests/unit/test_workspace_service.py b/backend/tests/unit/test_workspace_service.py index 1561e67..6a0e43a 100644 --- a/backend/tests/unit/test_workspace_service.py +++ b/backend/tests/unit/test_workspace_service.py @@ -5,8 +5,9 @@ Covers workspace creation/update, with a focus on the `planning_mode` field renaming/removing a placeholder partner (`rename_member` / `remove_member`). """ +import hashlib from datetime import date, datetime -from uuid import uuid4 +from uuid import UUID, uuid4 import pytest @@ -21,10 +22,33 @@ from app.repositories_impl.sqlite.planned_leave_repository import SQLitePlannedL from app.repositories_impl.sqlite.user_repository import SQLiteUserRepository from app.repositories_impl.sqlite.vacation_days_repository import SQLiteVacationDaysRepository from app.services.workspace_service import WorkspaceService -from app.utils.errors import NotFoundError, ValidationError +from app.utils.errors import InvitationError, NotFoundError, ValidationError from tests.conftest import SeededIds +def _hash(token: str) -> str: + return hashlib.sha256(token.encode()).hexdigest() + + +async def _make_inviter(user_repo: SQLiteUserRepository, personal_data_uuid: UUID) -> UUID: + """`workspace_invitation.invited_by_user_id` FKs to a real `user` row, + which itself FKs to a real `personal_data` row — a bare `uuid4()` fails + both constraints, so tests that create an invitation attribute it to a + user backed by an already-seeded `PersonalData` row (`user.personal_ + data_uuid` is UNIQUE, so this only works once per `personal_data_uuid` + per test).""" + user = await user_repo.create( + User( + id=uuid4(), + personal_data_uuid=personal_data_uuid, + email=f"inviter-{uuid4()}@example.com", + hashed_password="not-a-real-hash", + created_at=datetime.now(), + ) + ) + return user.id + + class TestWorkspacePlanningMode: """Test planning_mode defaults and validation.""" @@ -328,3 +352,80 @@ class TestRemoveMember: await workspace_service.remove_member( seeded_ids.workspace_id, placeholder.uuid, seeded_ids.personal_data_uuid ) + + +class TestInvitationScoping: + """F2 (T1 authz audit): self-invitation used to grant full membership of + any workspace whose id was known, and `revoke_invitation` was unscoped. + """ + + async def test_revoke_invitation_from_a_different_workspace_raises_not_found( + self, + workspace_service: WorkspaceService, + seeded_ids: SeededIds, + user_repo: SQLiteUserRepository, + ) -> None: + """An invitation belonging to workspace B must not be revocable by + naming workspace A — closes F6/part of F2 (`revoke_invitation` + gained a `workspace_id` argument).""" + other_ws = await workspace_service.create_workspace("Other WS") + token = await workspace_service.invite_partner( + other_ws.id, await _make_inviter(user_repo, seeded_ids.personal_data_uuid) + ) + inv = await workspace_service._inv.get_by_token_hash(_hash(token)) + assert inv is not None + + # `seeded_ids.workspace_id` is NOT where the invitation lives. + with pytest.raises(NotFoundError): + await workspace_service.revoke_invitation(inv.id, seeded_ids.workspace_id) + + async def test_revoke_invitation_from_its_own_workspace_succeeds( + self, + workspace_service: WorkspaceService, + seeded_ids: SeededIds, + user_repo: SQLiteUserRepository, + ) -> None: + token = await workspace_service.invite_partner( + seeded_ids.workspace_id, await _make_inviter(user_repo, seeded_ids.personal_data_uuid) + ) + inv = await workspace_service._inv.get_by_token_hash(_hash(token)) + assert inv is not None + await workspace_service.revoke_invitation(inv.id, seeded_ids.workspace_id) + + refreshed = await workspace_service._inv.get_by_id(inv.id) + assert refreshed is not None + assert refreshed.status == "revoked" + + async def test_redeem_refuses_when_already_a_member_of_the_target_workspace( + self, + workspace_service: WorkspaceService, + seeded_ids: SeededIds, + user_repo: SQLiteUserRepository, + ) -> None: + """The exploit from F2: a caller who is ALREADY a member of a + workspace could invite themselves and "redeem" their own invite, + confirming nothing new but exercising a codepath that must still be + closed for a caller re-redeeming while already a member.""" + token = await workspace_service.invite_partner( + seeded_ids.workspace_id, await _make_inviter(user_repo, seeded_ids.personal_data_uuid) + ) + with pytest.raises(InvitationError, match="already belong"): + await workspace_service.redeem_invitation(token, seeded_ids.personal_data_uuid) + + async def test_redeem_refuses_when_workspace_already_has_two_members( + self, + workspace_service: WorkspaceService, + seeded_ids: SeededIds, + user_repo: SQLiteUserRepository, + ) -> None: + # Fill the target workspace to its 2-member cap first. + await workspace_service.add_placeholder_parent(seeded_ids.workspace_id, "Partner") + + other_ws = await workspace_service.create_workspace("Other WS") + other_pd = await workspace_service.add_placeholder_parent(other_ws.id, "Redeemer") + + token = await workspace_service.invite_partner( + seeded_ids.workspace_id, await _make_inviter(user_repo, seeded_ids.personal_data_uuid) + ) + with pytest.raises(InvitationError, match="maximum number of members"): + await workspace_service.redeem_invitation(token, other_pd.uuid) diff --git a/frontend/src/components/layout/Layout.test.tsx b/frontend/src/components/layout/Layout.test.tsx index 7c4316c..62c51a5 100644 --- a/frontend/src/components/layout/Layout.test.tsx +++ b/frontend/src/components/layout/Layout.test.tsx @@ -128,4 +128,20 @@ describe('Layout', () => { expect(logout).toHaveBeenCalled() expect(mockNavigate).toHaveBeenCalledWith('/auth', { replace: true }) }) + + it('still navigates to /auth when logout rejects (S3 — best-effort server call)', async () => { + const logout = vi.fn().mockRejectedValue(new Error('network down')) + mockUseAuth.mockReturnValue({ + accessToken: 'token', + isAuthenticated: true, + isLoading: false, + login: vi.fn(), + register: vi.fn(), + logout, + }) + renderLayout() + await userEvent.click(screen.getByTestId('logout-button')) + expect(logout).toHaveBeenCalled() + expect(mockNavigate).toHaveBeenCalledWith('/auth', { replace: true }) + }) }) diff --git a/frontend/src/components/layout/Layout.tsx b/frontend/src/components/layout/Layout.tsx index 2ac357b..f6ce319 100644 --- a/frontend/src/components/layout/Layout.tsx +++ b/frontend/src/components/layout/Layout.tsx @@ -19,8 +19,16 @@ export function Layout({ children, header }: LayoutProps): ReactNode { const selectedChild = childrenList.find((c) => c.id === selectedChildId) const handleLogout = useCallback(async () => { - await logout() - navigate('/auth', { replace: true }) + try { + await logout() + } catch { + // Best-effort server call — AuthContext already clears local + // token/state in its own `finally` even when this rejects. Swallow it + // here so a network failure during logout doesn't surface as an + // unhandled rejection; still navigate away below. + } finally { + navigate('/auth', { replace: true }) + } }, [logout, navigate]) const headerProps: HeaderProps = { diff --git a/frontend/src/context/AuthContext.test.tsx b/frontend/src/context/AuthContext.test.tsx index a422ef9..b10cbfc 100644 --- a/frontend/src/context/AuthContext.test.tsx +++ b/frontend/src/context/AuthContext.test.tsx @@ -122,6 +122,47 @@ describe('AuthContext', () => { expect(result.current.accessToken).toBeNull() }) + it('clears local state even when the server logout call rejects (S3 — offline logout)', async () => { + vi.mocked(api.refreshToken).mockResolvedValue(tokenResponse) + vi.mocked(api.logout).mockRejectedValue(new Error('network down')) + + const { result } = renderHook(() => useAuth(), { wrapper }) + await waitFor(() => expect(result.current.isAuthenticated).toBe(true)) + + // The rejection still propagates to the caller (Layout decides how to + // handle a failed best-effort server call) — but local state must + // already be cleared by the time it does. + await act(async () => { + await expect(result.current.logout()).rejects.toThrow('network down') + }) + + expect(api.setAccessToken).toHaveBeenCalledWith(null) + expect(result.current.isAuthenticated).toBe(false) + expect(result.current.accessToken).toBeNull() + }) + + it('logs the user out when apiFetch reports a failed refresh-and-retry (S1 interceptor wiring)', async () => { + vi.mocked(api.refreshToken).mockResolvedValue(tokenResponse) + + const { result } = renderHook(() => useAuth(), { wrapper }) + await waitFor(() => expect(result.current.isAuthenticated).toBe(true)) + + // Simulate services/api.ts's apiFetch invoking the handler AuthContext + // registered via api.setUnauthorizedHandler after a mid-session 401's + // refresh-and-retry gives up. + const registeredHandler = vi.mocked(api.setUnauthorizedHandler).mock.calls.find( + (call) => call[0] !== null, + )?.[0] + expect(registeredHandler).toBeInstanceOf(Function) + + act(() => { + registeredHandler?.() + }) + + expect(result.current.isAuthenticated).toBe(false) + expect(result.current.accessToken).toBeNull() + }) + it('throws when useAuth is used outside an AuthProvider', () => { expect(() => renderHook(() => useAuth())).toThrow( 'useAuth must be used inside ', diff --git a/frontend/src/context/AuthContext.tsx b/frontend/src/context/AuthContext.tsx index d42107f..8d8621f 100644 --- a/frontend/src/context/AuthContext.tsx +++ b/frontend/src/context/AuthContext.tsx @@ -42,6 +42,18 @@ export function AuthProvider({ children }: { children: React.ReactNode }) { }) }, []) + // apiFetch's own refresh-and-retry (see services/api.ts) handles a 401 hit + // mid-session by refreshing once and retrying the original request. If + // that refresh itself fails, this is the hook that actually logs the user + // out — without it, isAuthenticated would stay true forever while every + // subsequent request 401s. + useEffect(() => { + api.setUnauthorizedHandler(() => { + setState({ accessToken: null, isAuthenticated: false, isLoading: false }) + }) + return () => api.setUnauthorizedHandler(null) + }, []) + const handleTokenResponse = useCallback((res: TokenResponse) => { api.setAccessToken(res.access_token) setState({ accessToken: res.access_token, isAuthenticated: true, isLoading: false }) @@ -64,9 +76,15 @@ export function AuthProvider({ children }: { children: React.ReactNode }) { ) const logout = useCallback(async () => { - await api.logout() - api.setAccessToken(null) - setState({ accessToken: null, isAuthenticated: false, isLoading: false }) + // Clear local state even if the server call fails (offline logout must + // still work) — but still let the failure propagate to the caller so it + // can decide how to handle a best-effort server call that didn't land. + try { + await api.logout() + } finally { + api.setAccessToken(null) + setState({ accessToken: null, isAuthenticated: false, isLoading: false }) + } }, []) return ( diff --git a/frontend/src/context/WorkspaceContext.test.tsx b/frontend/src/context/WorkspaceContext.test.tsx index 06f3417..6bd0f93 100644 --- a/frontend/src/context/WorkspaceContext.test.tsx +++ b/frontend/src/context/WorkspaceContext.test.tsx @@ -217,7 +217,12 @@ describe('WorkspaceContext', () => { expect(result.current.selectedChildId).toBe('c2') }) - it('setWorkspaceId stores the id and updates state', async () => { + it('setWorkspaceId stores the id, updates state, and fetches that workspace directly', async () => { + // P2 in the frontend audit: this used to rely on a reactive effect that + // only fired while `workspace` was still null, so switching workspaces + // after the initial load silently never fetched the new one. Fetching + // explicitly here fixes that without reintroducing the cold-load + // duplicate GET /workspace the same effect used to cause. vi.mocked(useAuth).mockReturnValue(authenticated) vi.mocked(api.getUserContext).mockResolvedValue(userContext) vi.mocked(api.getWorkspace).mockResolvedValue(workspace) @@ -225,12 +230,19 @@ describe('WorkspaceContext', () => { const { result } = renderHook(() => useWorkspace(), { wrapper }) await waitFor(() => expect(result.current.workspace).not.toBeNull()) + const otherWorkspace: Workspace = { ...workspace, id: 'ws2', name: 'Other workspace' } + vi.mocked(api.getWorkspace).mockResolvedValue(otherWorkspace) + vi.mocked(api.getWorkspace).mockClear() + act(() => { result.current.setWorkspaceId('ws2') }) expect(result.current.workspaceId).toBe('ws2') expect(localStorage.getItem('workspaceId')).toBe('ws2') + + await waitFor(() => expect(api.getWorkspace).toHaveBeenCalledWith('ws2')) + await waitFor(() => expect(result.current.workspace).toEqual(otherWorkspace)) }) it('refetch re-fetches the current workspace', async () => { @@ -322,6 +334,38 @@ describe('WorkspaceContext', () => { expect(result.current.workspace).toEqual(otherWorkspace) }) + it('discards a refreshContext response that resolves after logout (S4)', async () => { + vi.mocked(useAuth).mockReturnValue(authenticated) + let resolveContext: (value: api.UserContext) => void = () => {} + const pending = new Promise((resolve) => { + resolveContext = resolve + }) + vi.mocked(api.getUserContext).mockReturnValue(pending) + + const { result, rerender } = renderHook(() => useWorkspace(), { wrapper }) + + // Logout happens while the initial refreshContext() call is still + // in flight (e.g. a slow /auth/context response). + vi.mocked(useAuth).mockReturnValue(unauthenticated) + rerender() + + await waitFor(() => expect(result.current.workspace).toBeNull()) + expect(result.current.workspaceId).toBeNull() + + // The stale response arrives after logout — it must be discarded + // rather than repopulating this account's context. + await act(async () => { + resolveContext(userContext) + await pending + }) + + expect(result.current.workspaceId).toBeNull() + expect(result.current.parentName).toBeNull() + expect(result.current.personalDataUuid).toBeNull() + expect(result.current.children).toEqual([]) + expect(localStorage.getItem('workspaceId')).toBeNull() + }) + it('throws when useWorkspace is used outside a WorkspaceProvider', () => { vi.mocked(useAuth).mockReturnValue(unauthenticated) diff --git a/frontend/src/context/WorkspaceContext.tsx b/frontend/src/context/WorkspaceContext.tsx index 23eca74..950041d 100644 --- a/frontend/src/context/WorkspaceContext.tsx +++ b/frontend/src/context/WorkspaceContext.tsx @@ -43,25 +43,38 @@ export function WorkspaceProvider({ children: childrenProp }: { children: React. // absence) of cached data — see the effect for why that distinction is // what makes a login swap actually refetch. const wasAuthenticatedRef = useRef(false) + // Bumped by clearWorkspaceState() (logout). A refreshContext/fetchWorkspace + // call captures the generation it started with; if that generation is + // stale by the time the response arrives (logout happened mid-flight), + // the response is discarded instead of repopulating state for the + // account that just logged out — see S4 in the frontend audit. + const generationRef = useRef(0) const fetchWorkspace = useCallback(async (id: string) => { + const generation = generationRef.current setIsLoading(true) setError(null) try { const ws = await api.getWorkspace(id) + if (generation !== generationRef.current) return setWorkspace(ws) } catch (err: unknown) { + if (generation !== generationRef.current) return setError(err instanceof Error ? err.message : 'Failed to load workspace') setWorkspace(null) } finally { - setIsLoading(false) + if (generation === generationRef.current) { + setIsLoading(false) + } } }, []) const refreshContext = useCallback(async () => { + const generation = generationRef.current setIsLoading(true) try { const context = await api.getUserContext() + if (generation !== generationRef.current) return localStorage.setItem('workspaceId', context.workspace_id) setWorkspaceId(context.workspace_id) setPersonalDataUuid(context.personal_data_uuid) @@ -73,15 +86,20 @@ export function WorkspaceProvider({ children: childrenProp }: { children: React. } try { const ws = await api.getWorkspace(context.workspace_id) + if (generation !== generationRef.current) return setWorkspace(ws) } catch (err: unknown) { + if (generation !== generationRef.current) return setError(err instanceof Error ? err.message : 'Failed to load workspace') setWorkspace(null) } } catch (err: unknown) { + if (generation !== generationRef.current) return setError(err instanceof Error ? err.message : 'Failed to load user context') } finally { - setIsLoading(false) + if (generation === generationRef.current) { + setIsLoading(false) + } } }, []) @@ -92,6 +110,7 @@ export function WorkspaceProvider({ children: childrenProp }: { children: React. // one account's data could otherwise flash under another account's // session. const clearWorkspaceState = useCallback(() => { + generationRef.current += 1 localStorage.removeItem('workspaceId') setWorkspace(null) setWorkspaceId(null) @@ -123,16 +142,22 @@ export function WorkspaceProvider({ children: childrenProp }: { children: React. } }, [isAuthenticated, refreshContext, clearWorkspaceState]) - useEffect(() => { - if (isAuthenticated && workspaceId && !workspace) { - void fetchWorkspace(workspaceId) - } - }, [isAuthenticated, workspaceId, workspace, fetchWorkspace]) - - const handleSetWorkspaceId = useCallback((id: string) => { - localStorage.setItem('workspaceId', id) - setWorkspaceId(id) - }, []) + // NOTE (P2 in the frontend audit): a cold login already fetches the + // workspace once inside refreshContext() above. This used to be a + // reactive `useEffect` on `[isAuthenticated, workspaceId, workspace]` that + // fired a *second* GET /workspace as soon as `workspaceId` state landed + // but before `workspace` itself did — a duplicate cold-load request. + // Fetching explicitly here, only when the id is set imperatively (e.g. a + // future workspace switcher), removes that duplicate without losing the + // ability to load a workspace picked outside the login flow. + const handleSetWorkspaceId = useCallback( + (id: string) => { + localStorage.setItem('workspaceId', id) + setWorkspaceId(id) + void fetchWorkspace(id) + }, + [fetchWorkspace], + ) const handleSelectChild = useCallback((id: string) => { setSelectedChildId(id) diff --git a/frontend/src/hooks/useAnalytics.test.ts b/frontend/src/hooks/useAnalytics.test.ts index 645c8cf..0871bd4 100644 --- a/frontend/src/hooks/useAnalytics.test.ts +++ b/frontend/src/hooks/useAnalytics.test.ts @@ -3,6 +3,7 @@ import { renderHook, act } from '@testing-library/react' import { useAnalytics } from './useAnalytics' import * as api from '@/services/api' import { makeWorkspaceOverview } from '../../tests/fixtures' +import type { WorkspaceOverview } from '@/types' vi.mock('@/services/api') @@ -123,4 +124,59 @@ describe('useAnalytics', () => { expect(result.current.data).toBeNull() }) + + it('discards a stale response for a previous childId (P1 stale-response guard)', async () => { + let resolveFirst: (value: WorkspaceOverview) => void = () => {} + let resolveSecond: (value: WorkspaceOverview) => void = () => {} + const firstPromise = new Promise((resolve) => { + resolveFirst = resolve + }) + const secondPromise = new Promise((resolve) => { + resolveSecond = resolve + }) + + vi.mocked(api.getWorkspaceOverview) + .mockReturnValueOnce(firstPromise) + .mockReturnValueOnce(secondPromise) + + const { result, rerender } = renderHook( + ({ childId }: { childId: string }) => useAnalytics('ws1', childId), + { initialProps: { childId: 'child1' } }, + ) + + // Switching childId while the first (slower) request is still in + // flight fires a second request for the new child. + rerender({ childId: 'child2' }) + + const child1Data = makeWorkspaceOverview({ + workspaceId: 'ws1', + childId: 'child1', + parentProjections: [], + combinedDaysUsed: 1, + combinedDaysLeft: 1, + combinedExpiringAtB4: 0, + }) + const child2Data = makeWorkspaceOverview({ + workspaceId: 'ws1', + childId: 'child2', + parentProjections: [], + combinedDaysUsed: 5, + combinedDaysLeft: 5, + combinedExpiringAtB4: 0, + }) + + // The newer (child2) request resolves first... + await act(async () => { + resolveSecond(child2Data) + await secondPromise + }) + expect(result.current.data).toEqual(child2Data) + + // ...then the stale child1 response arrives late and must not overwrite it. + await act(async () => { + resolveFirst(child1Data) + await firstPromise + }) + expect(result.current.data).toEqual(child2Data) + }) }) diff --git a/frontend/src/hooks/useAnalytics.ts b/frontend/src/hooks/useAnalytics.ts index 4ab3f75..1c8e2af 100644 --- a/frontend/src/hooks/useAnalytics.ts +++ b/frontend/src/hooks/useAnalytics.ts @@ -1,5 +1,6 @@ import { useState, useEffect, useCallback } from 'react' import * as api from '@/services/api' +import { useLatestRequest } from './useLatestRequest' import type { WorkspaceOverview } from '@/types' interface UseAnalyticsResult { @@ -13,8 +14,10 @@ export function useAnalytics(workspaceId: string | null, childId?: string): UseA const [data, setData] = useState(null) const [isLoading, setIsLoading] = useState(false) const [error, setError] = useState(null) + const { start, isCurrent } = useLatestRequest() const fetch = useCallback(async () => { + const requestId = start() if (!workspaceId || !childId) { // The identity this data belongs to is gone (e.g. a logout cleared // the workspace/child selection) — clear rather than leave the @@ -27,14 +30,18 @@ export function useAnalytics(workspaceId: string | null, childId?: string): UseA setError(null) try { const response = await api.getWorkspaceOverview(workspaceId, childId) + if (!isCurrent(requestId)) return setData(response) } catch (err) { + if (!isCurrent(requestId)) return setError(err instanceof Error ? err.message : 'Failed to fetch analytics') setData(null) } finally { - setIsLoading(false) + if (isCurrent(requestId)) { + setIsLoading(false) + } } - }, [workspaceId, childId]) + }, [workspaceId, childId, start, isCurrent]) useEffect(() => { fetch() diff --git a/frontend/src/hooks/useCalendarDays.ts b/frontend/src/hooks/useCalendarDays.ts index 390e1bc..5027800 100644 --- a/frontend/src/hooks/useCalendarDays.ts +++ b/frontend/src/hooks/useCalendarDays.ts @@ -15,6 +15,7 @@ import { useCallback, useEffect, useState } from 'react' import * as api from '@/services/api' +import { useLatestRequest } from './useLatestRequest' import type { CalendarWindow } from '@/types' interface UseCalendarDaysResult { @@ -32,20 +33,24 @@ export function useCalendarDays( const [data, setData] = useState(null) const [isLoading, setIsLoading] = useState(false) const [error, setError] = useState(null) + const { start, isCurrent } = useLatestRequest() const fetchWindow = useCallback(async () => { + const requestId = start() if (!workspaceId) return setIsLoading(true) setError(null) try { const result = await api.getCalendarDays(workspaceId, startYear, years) + if (!isCurrent(requestId)) return setData(result) } catch (err: unknown) { + if (!isCurrent(requestId)) return setError(err instanceof Error ? err.message : 'Failed to load calendar days') } finally { - setIsLoading(false) + if (isCurrent(requestId)) setIsLoading(false) } - }, [workspaceId, startYear, years]) + }, [workspaceId, startYear, years, start, isCurrent]) useEffect(() => { void fetchWindow() diff --git a/frontend/src/hooks/useChildSettings.ts b/frontend/src/hooks/useChildSettings.ts index 322b8a6..e02b8ce 100644 --- a/frontend/src/hooks/useChildSettings.ts +++ b/frontend/src/hooks/useChildSettings.ts @@ -6,6 +6,7 @@ import { useCallback, useEffect, useState } from 'react' import * as api from '@/services/api' +import { useLatestRequest } from './useLatestRequest' import type { Child } from '@/types' export interface ChildSettingsUpdatePayload { @@ -24,8 +25,10 @@ export function useChildSettings(childId: string | null): UseChildSettingsResult const [data, setData] = useState(null) const [isLoading, setIsLoading] = useState(false) const [error, setError] = useState(null) + const { start, isCurrent } = useLatestRequest() const fetch = useCallback(async () => { + const requestId = start() if (!childId) { setData(null) return @@ -34,13 +37,15 @@ export function useChildSettings(childId: string | null): UseChildSettingsResult setError(null) try { const result = await api.getChild(childId) + if (!isCurrent(requestId)) return setData(result) } catch (err: unknown) { + if (!isCurrent(requestId)) return setError(err instanceof Error ? err.message : 'Failed to load child') } finally { - setIsLoading(false) + if (isCurrent(requestId)) setIsLoading(false) } - }, [childId]) + }, [childId, start, isCurrent]) useEffect(() => { void fetch() diff --git a/frontend/src/hooks/useDayOverrides.ts b/frontend/src/hooks/useDayOverrides.ts index 862158d..feb7ed2 100644 --- a/frontend/src/hooks/useDayOverrides.ts +++ b/frontend/src/hooks/useDayOverrides.ts @@ -1,5 +1,6 @@ import { useCallback, useEffect, useState } from 'react' import * as api from '@/services/api' +import { useLatestRequest } from './useLatestRequest' import type { DayOverride, DayOverrideCreateRequest } from '@/types' interface UseDayOverridesResult { @@ -14,20 +15,24 @@ export function useDayOverrides(workspaceId: string): UseDayOverridesResult { const [data, setData] = useState([]) const [isLoading, setIsLoading] = useState(false) const [error, setError] = useState(null) + const { start, isCurrent } = useLatestRequest() const fetch = useCallback(async () => { + const requestId = start() if (!workspaceId) return setIsLoading(true) setError(null) try { const result = await api.listDayOverridesByWorkspace(workspaceId) + if (!isCurrent(requestId)) return setData(result) } catch (err: unknown) { + if (!isCurrent(requestId)) return setError(err instanceof Error ? err.message : 'Failed to load day overrides') } finally { - setIsLoading(false) + if (isCurrent(requestId)) setIsLoading(false) } - }, [workspaceId]) + }, [workspaceId, start, isCurrent]) useEffect(() => { void fetch() diff --git a/frontend/src/hooks/useLatestRequest.ts b/frontend/src/hooks/useLatestRequest.ts new file mode 100644 index 0000000..7c7e7d7 --- /dev/null +++ b/frontend/src/hooks/useLatestRequest.ts @@ -0,0 +1,41 @@ +import { useCallback, useRef } from 'react' + +interface UseLatestRequestResult { + /** Call at the start of a fetch to obtain this call's request id. */ + start: () => number + /** True if `id` is still the most recently started request — i.e. no + * newer call to `start()` has happened since. Guard every `setState` in + * an async fetch with this so a stale response (e.g. from a fetch kicked + * off for a since-replaced childId/workspaceId) can't overwrite state a + * newer, faster request already set. */ + isCurrent: (id: number) => boolean +} + +/** + * Shared stale-response guard for the data-fetching hooks in this + * directory. Each hook has no cache/dedup layer (no TanStack Query), so + * switching identity (e.g. the selected child) while a fetch is in flight + * can otherwise let an older, slower response land after — and overwrite — + * a newer one. See P1 in the frontend audit. + * + * Usage: + * const { start, isCurrent } = useLatestRequest() + * const fetch = useCallback(async () => { + * const requestId = start() + * const result = await api.get(...) + * if (!isCurrent(requestId)) return + * setData(result) + * }, [...]) + */ +export function useLatestRequest(): UseLatestRequestResult { + const idRef = useRef(0) + + const start = useCallback((): number => { + idRef.current += 1 + return idRef.current + }, []) + + const isCurrent = useCallback((id: number): boolean => id === idRef.current, []) + + return { start, isCurrent } +} diff --git a/frontend/src/hooks/useLeaveWishes.ts b/frontend/src/hooks/useLeaveWishes.ts index 074b921..3524e4b 100644 --- a/frontend/src/hooks/useLeaveWishes.ts +++ b/frontend/src/hooks/useLeaveWishes.ts @@ -1,5 +1,6 @@ import { useCallback, useEffect, useState } from 'react' import * as api from '@/services/api' +import { useLatestRequest } from './useLatestRequest' import type { LeaveWish, LeaveWishCreateRequest, LeaveWishUpdateRequest } from '@/types' interface UseLeaveWishesResult { @@ -34,9 +35,11 @@ export function useLeaveWishes(workspaceId: string | null): UseLeaveWishesResult const [isLoading, setIsLoading] = useState(false) const [isRefreshing, setIsRefreshing] = useState(false) const [error, setError] = useState(null) + const { start, isCurrent } = useLatestRequest() const fetch = useCallback( async (options?: { background?: boolean }) => { + const requestId = start() if (!workspaceId) return const background = options?.background ?? false const setBusy = background ? setIsRefreshing : setIsLoading @@ -44,14 +47,16 @@ export function useLeaveWishes(workspaceId: string | null): UseLeaveWishesResult setError(null) try { const result = await api.getLeaveWishes(workspaceId) + if (!isCurrent(requestId)) return setData(result) } catch (err: unknown) { + if (!isCurrent(requestId)) return setError(err instanceof Error ? err.message : 'Failed to load leave wishes') } finally { - setBusy(false) + if (isCurrent(requestId)) setBusy(false) } }, - [workspaceId], + [workspaceId, start, isCurrent], ) useEffect(() => { diff --git a/frontend/src/hooks/useOptimizationTips.ts b/frontend/src/hooks/useOptimizationTips.ts index 6e48758..2ce8267 100644 --- a/frontend/src/hooks/useOptimizationTips.ts +++ b/frontend/src/hooks/useOptimizationTips.ts @@ -1,5 +1,6 @@ import { useState, useEffect, useCallback } from 'react' import * as api from '@/services/api' +import { useLatestRequest } from './useLatestRequest' import type { Tip } from '@/types' interface UseOptimizationTipsResult { @@ -15,21 +16,25 @@ export function useOptimizationTips(workspaceId: string | null): UseOptimization const [isLoading, setIsLoading] = useState(false) const [error, setError] = useState(null) const [dismissed, setDismissed] = useState>(new Set()) + const { start, isCurrent } = useLatestRequest() const fetch = useCallback(async () => { + const requestId = start() if (!workspaceId) return setIsLoading(true) setError(null) try { const response = await api.getTips(workspaceId) + if (!isCurrent(requestId)) return setData(response) } catch (err) { + if (!isCurrent(requestId)) return setError(err instanceof Error ? err.message : 'Failed to fetch tips') setData(null) } finally { - setIsLoading(false) + if (isCurrent(requestId)) setIsLoading(false) } - }, [workspaceId]) + }, [workspaceId, start, isCurrent]) const dismiss = useCallback( async (tipId: string) => { diff --git a/frontend/src/hooks/useParentalLeave.ts b/frontend/src/hooks/useParentalLeave.ts index 79591e3..8d64115 100644 --- a/frontend/src/hooks/useParentalLeave.ts +++ b/frontend/src/hooks/useParentalLeave.ts @@ -1,5 +1,6 @@ import { useState, useEffect, useCallback } from 'react' import * as api from '@/services/api' +import { useLatestRequest } from './useLatestRequest' import type { ParentalLeaveFetch } from '@/types' interface UseParentalLeaveResult { @@ -16,8 +17,10 @@ export function useParentalLeave( const [data, setData] = useState(null) const [isLoading, setIsLoading] = useState(false) const [error, setError] = useState(null) + const { start, isCurrent } = useLatestRequest() const fetch = useCallback(async () => { + const requestId = start() if (!personalDataUuid || !childId) { // The identity this data belongs to is gone (e.g. a logout cleared // the parent/child selection) — clear rather than leave the previous @@ -30,14 +33,16 @@ export function useParentalLeave( setError(null) try { const response = await api.getParentalLeave(personalDataUuid, childId) + if (!isCurrent(requestId)) return setData(response) } catch (err) { + if (!isCurrent(requestId)) return setError(err instanceof Error ? err.message : 'Failed to fetch parental leave data') setData(null) } finally { - setIsLoading(false) + if (isCurrent(requestId)) setIsLoading(false) } - }, [personalDataUuid, childId]) + }, [personalDataUuid, childId, start, isCurrent]) useEffect(() => { fetch() diff --git a/frontend/src/hooks/usePlannedDays.test.ts b/frontend/src/hooks/usePlannedDays.test.ts index c3902ec..df2b606 100644 --- a/frontend/src/hooks/usePlannedDays.test.ts +++ b/frontend/src/hooks/usePlannedDays.test.ts @@ -339,6 +339,65 @@ describe('usePlannedDays', () => { expect(updateError?.message).toBe('Update failed') }) + it('discards a stale response for a previous childId (P1 stale-response guard)', async () => { + let resolveFirst: (value: ReturnType[]) => void = () => {} + let resolveSecond: (value: ReturnType[]) => void = () => {} + const firstPromise = new Promise[]>((resolve) => { + resolveFirst = resolve + }) + const secondPromise = new Promise[]>((resolve) => { + resolveSecond = resolve + }) + + vi.mocked(api.getPlannedDays).mockReturnValueOnce(firstPromise).mockReturnValueOnce(secondPromise) + + const { result, rerender } = renderHook( + ({ childId }: { childId: string }) => usePlannedDays(childId), + { initialProps: { childId: 'child1' } }, + ) + + // Switching childId while the first (slower) request is still in + // flight fires a second request for the new child. + rerender({ childId: 'child2' }) + + const child1Data = [ + makePlannedLeaveDay({ + id: 'p1', + workspaceId: 'ws1', + childId: 'child1', + parentPersonalDataUuid: 'parent1', + date: '2026-06-01', + dayType: 'vacation', + isDubbeldag: false, + }), + ] + const child2Data = [ + makePlannedLeaveDay({ + id: 'p2', + workspaceId: 'ws1', + childId: 'child2', + parentPersonalDataUuid: 'parent1', + date: '2026-07-01', + dayType: 'vacation', + isDubbeldag: false, + }), + ] + + // The newer (child2) request resolves first... + await act(async () => { + resolveSecond(child2Data) + await secondPromise + }) + expect(result.current.data).toEqual(child2Data) + + // ...then the stale child1 response arrives late and must not overwrite it. + await act(async () => { + resolveFirst(child1Data) + await firstPromise + }) + expect(result.current.data).toEqual(child2Data) + }) + it('handles remove error gracefully', async () => { vi.mocked(api.getPlannedDays).mockResolvedValue([]) vi.mocked(api.deletePlannedDay).mockRejectedValue(new Error('Delete failed')) diff --git a/frontend/src/hooks/usePlannedDays.ts b/frontend/src/hooks/usePlannedDays.ts index e4fa1f1..1bcd0ff 100644 --- a/frontend/src/hooks/usePlannedDays.ts +++ b/frontend/src/hooks/usePlannedDays.ts @@ -1,5 +1,6 @@ import { useState, useEffect, useCallback } from 'react' import * as api from '@/services/api' +import { useLatestRequest } from './useLatestRequest' import type { PlannedLeaveDay } from '@/types' interface UsePlannedDaysResult { @@ -20,8 +21,10 @@ export function usePlannedDays(childId?: string): UsePlannedDaysResult { const [data, setData] = useState(null) const [isLoading, setIsLoading] = useState(false) const [error, setError] = useState(null) + const { start, isCurrent } = useLatestRequest() const fetch = useCallback(async () => { + const requestId = start() if (!childId) { // The child this data belongs to is gone (e.g. a logout cleared the // selection) — clear rather than leave the previous account's @@ -34,16 +37,18 @@ export function usePlannedDays(childId?: string): UsePlannedDaysResult { setError(null) try { const response = await api.getPlannedDays(childId) - setData(response) + if (isCurrent(requestId)) setData(response) return response } catch (err) { - setError(err instanceof Error ? err.message : 'Failed to fetch planned days') - setData(null) + if (isCurrent(requestId)) { + setError(err instanceof Error ? err.message : 'Failed to fetch planned days') + setData(null) + } return null } finally { - setIsLoading(false) + if (isCurrent(requestId)) setIsLoading(false) } - }, [childId]) + }, [childId, start, isCurrent]) const add = useCallback( async (payload: api.CreatePlannedDayPayload) => { diff --git a/frontend/src/hooks/useScraperFetch.test.ts b/frontend/src/hooks/useScraperFetch.test.ts index 9170869..ba30dbd 100644 --- a/frontend/src/hooks/useScraperFetch.test.ts +++ b/frontend/src/hooks/useScraperFetch.test.ts @@ -1,6 +1,7 @@ import { describe, it, expect, beforeEach, vi, afterEach } from 'vitest' import { renderHook, act } from '@testing-library/react' import { useScraperFetch } from './useScraperFetch' +import type { PollResponse } from './useScraperFetch' import * as api from '@/services/api' import * as errors from '@/services/errors' import { makeScraperSessionStartResponse, makePollResponse } from '../../tests/fixtures' @@ -335,6 +336,81 @@ describe('useScraperFetch', () => { expect(result.current.sessionId).toBeNull() }) + it('skips a poll tick while the previous one is still in flight (P4 overlap guard)', async () => { + vi.mocked(api.startScraperSession).mockResolvedValue( + makeScraperSessionStartResponse({ + session_id: 'sess-123', + workspace_id: 'ws-1', + personal_data_uuid: 'pd-1', + child_id: 'c-1', + }) + ) + + let resolvePoll: (value: PollResponse) => void = () => {} + const pending = new Promise((resolve) => { + resolvePoll = resolve + }) + vi.mocked(api.pollScraperSession).mockReturnValueOnce(pending) + + const { result } = renderHook(() => useScraperFetch()) + + await act(async () => { + await result.current.start() + }) + + // First tick starts the still-pending poll. + await act(async () => { + vi.advanceTimersByTime(1000) + }) + expect(api.pollScraperSession).toHaveBeenCalledTimes(1) + + // A second tick fires while the first poll is still in flight — it must + // be skipped rather than stacking a second concurrent request. + await act(async () => { + vi.advanceTimersByTime(1000) + }) + expect(api.pollScraperSession).toHaveBeenCalledTimes(1) + + // Resolve the first poll; the next tick is then free to run normally. + vi.mocked(api.pollScraperSession).mockResolvedValue(makePollResponse({ status: 'awaiting_scan' })) + await act(async () => { + resolvePoll(makePollResponse({ status: 'authenticating' })) + await pending + }) + + await act(async () => { + vi.advanceTimersByTime(1000) + }) + expect(api.pollScraperSession).toHaveBeenCalledTimes(2) + }) + + it('clears the polling interval on unmount (P4)', async () => { + vi.mocked(api.startScraperSession).mockResolvedValue( + makeScraperSessionStartResponse({ + session_id: 'sess-123', + workspace_id: 'ws-1', + personal_data_uuid: 'pd-1', + child_id: 'c-1', + }) + ) + vi.mocked(api.pollScraperSession).mockResolvedValue(makePollResponse({ status: 'awaiting_scan' })) + + const { result, unmount } = renderHook(() => useScraperFetch()) + + await act(async () => { + await result.current.start() + }) + + unmount() + vi.mocked(api.pollScraperSession).mockClear() + + await act(async () => { + vi.advanceTimersByTime(5000) + }) + + expect(api.pollScraperSession).not.toHaveBeenCalled() + }) + it('handles poll error', async () => { vi.mocked(api.startScraperSession).mockResolvedValue( makeScraperSessionStartResponse({ diff --git a/frontend/src/hooks/useScraperFetch.ts b/frontend/src/hooks/useScraperFetch.ts index f2f9723..ea0c871 100644 --- a/frontend/src/hooks/useScraperFetch.ts +++ b/frontend/src/hooks/useScraperFetch.ts @@ -1,4 +1,4 @@ -import { useState, useRef, useCallback } from 'react' +import { useState, useRef, useCallback, useEffect } from 'react' import * as api from '@/services/api' import { isUnauthorized } from '@/services/errors' @@ -41,6 +41,10 @@ export function useScraperFetch(): UseScraperFetchResult { const [unauthorizedError, setUnauthorizedError] = useState(false) const intervalRef = useRef | null>(null) + // Guards against overlapping ticks: if a poll response hasn't come back + // yet when the next 1s tick fires (a slow/backed-up server), skip that + // tick rather than stacking a second in-flight request on top of it. + const pollInFlightRef = useRef(false) const stopPolling = useCallback(() => { if (intervalRef.current) { @@ -55,6 +59,8 @@ export function useScraperFetch(): UseScraperFetchResult { const startPolling = useCallback((sid: string) => { stopPolling() intervalRef.current = setInterval(async () => { + if (pollInFlightRef.current) return + pollInFlightRef.current = true try { const data = await api.pollScraperSession(sid) setPollData(data as PollResponse) @@ -64,10 +70,19 @@ export function useScraperFetch(): UseScraperFetchResult { } catch (err) { setError(err instanceof Error ? err.message : 'Poll error') stopPolling() + } finally { + pollInFlightRef.current = false } }, POLL_INTERVAL_MS) }, [stopPolling]) + // The interval otherwise outlives the component: a poll response arriving + // after unmount would still call setState, and the timer itself would + // keep firing (and keep holding the closure alive) forever. + useEffect(() => { + return () => stopPolling() + }, [stopPolling]) + const start = useCallback( async (childId?: string) => { if (sessionId && pollData && !isTerminalStatus(pollData.status)) { diff --git a/frontend/src/hooks/useVacationDays.ts b/frontend/src/hooks/useVacationDays.ts index 9f4ddcc..7aa1f4f 100644 --- a/frontend/src/hooks/useVacationDays.ts +++ b/frontend/src/hooks/useVacationDays.ts @@ -10,6 +10,7 @@ import { useCallback, useEffect, useState } from 'react' import * as api from '@/services/api' +import { useLatestRequest } from './useLatestRequest' import type { VacationDays, VacationDaysCreateRequest } from '@/types' interface UseVacationDaysResult { @@ -23,20 +24,24 @@ export function useVacationDays(workspaceId: string): UseVacationDaysResult { const [data, setData] = useState([]) const [isLoading, setIsLoading] = useState(false) const [error, setError] = useState(null) + const { start, isCurrent } = useLatestRequest() const fetch = useCallback(async () => { + const requestId = start() if (!workspaceId) return setIsLoading(true) setError(null) try { const result = await api.getVacationDaysByWorkspace(workspaceId) + if (!isCurrent(requestId)) return setData(result) } catch (err: unknown) { + if (!isCurrent(requestId)) return setError(err instanceof Error ? err.message : 'Failed to load vacation days') } finally { - setIsLoading(false) + if (isCurrent(requestId)) setIsLoading(false) } - }, [workspaceId]) + }, [workspaceId, start, isCurrent]) useEffect(() => { void fetch() diff --git a/frontend/src/hooks/useWorkspaceMembers.ts b/frontend/src/hooks/useWorkspaceMembers.ts index f079cb9..e6f7b79 100644 --- a/frontend/src/hooks/useWorkspaceMembers.ts +++ b/frontend/src/hooks/useWorkspaceMembers.ts @@ -1,5 +1,6 @@ import { useCallback, useEffect, useState } from 'react' import * as api from '@/services/api' +import { useLatestRequest } from './useLatestRequest' import type { PersonalData } from '@/types' interface UseWorkspaceMembersResult { @@ -14,20 +15,24 @@ export function useWorkspaceMembers(workspaceId: string | null): UseWorkspaceMem const [data, setData] = useState([]) const [isLoading, setIsLoading] = useState(false) const [error, setError] = useState(null) + const { start, isCurrent } = useLatestRequest() const fetch = useCallback(async () => { + const requestId = start() if (!workspaceId) return setIsLoading(true) setError(null) try { const result = await api.listWorkspaceMembers(workspaceId) + if (!isCurrent(requestId)) return setData(result) } catch (err: unknown) { + if (!isCurrent(requestId)) return setError(err instanceof Error ? err.message : 'Failed to load workspace members') } finally { - setIsLoading(false) + if (isCurrent(requestId)) setIsLoading(false) } - }, [workspaceId]) + }, [workspaceId, start, isCurrent]) useEffect(() => { void fetch() diff --git a/frontend/src/services/api.test.ts b/frontend/src/services/api.test.ts index 552ed1d..3f2234e 100644 --- a/frontend/src/services/api.test.ts +++ b/frontend/src/services/api.test.ts @@ -1774,6 +1774,114 @@ describe('endpoint functions (representative coverage)', () => { }) }) +describe('apiFetch — 401 refresh-and-retry (S1)', () => { + let fetchMock: ReturnType + + beforeEach(() => { + fetchMock = vi.fn() + globalThis.fetch = fetchMock as typeof fetch + }) + + afterEach(() => { + vi.clearAllMocks() + api.setAccessToken(null) + api.setUnauthorizedHandler(null) + }) + + function unauthorizedResponse(detail = 'Unauthorized') { + return { ok: false, status: 401, statusText: 'Unauthorized', json: async () => ({ detail }) } + } + + function tokenResponse(access_token: string) { + return { + ok: true, + status: 200, + json: async () => ({ access_token, token_type: 'bearer', expires_in: 3600 }), + } + } + + it('refreshes the token once and retries the original request after a 401', async () => { + api.setAccessToken('old-token') + fetchMock + .mockResolvedValueOnce(unauthorizedResponse()) + .mockResolvedValueOnce(tokenResponse('new-token')) + .mockResolvedValueOnce({ ok: true, status: 200, json: async () => [] }) + + const result = await api.getTips('ws1') + + expect(result).toEqual([]) + expect(fetchMock).toHaveBeenCalledTimes(3) + expect(fetchMock.mock.calls[1]![0]).toBe('/api/auth/refresh') + const [retryUrl, retryOptions] = fetchMock.mock.calls[2] as [string, RequestInit] + expect(retryUrl).toBe('/api/tips/ws1') + expect((retryOptions.headers as Record)['Authorization']).toBe('Bearer new-token') + }) + + it('dedupes concurrent 401s from parallel requests into a single refresh call', async () => { + api.setAccessToken('old-token') + fetchMock.mockImplementation(async (url: string, options?: RequestInit) => { + if (url === '/api/auth/refresh') { + return tokenResponse('new-token') + } + const headers = options?.headers as Record | undefined + if (headers?.['Authorization'] === 'Bearer new-token') { + return { ok: true, status: 200, json: async () => [] } + } + return unauthorizedResponse() + }) + + const [r1, r2] = await Promise.all([api.getTips('ws1'), api.getTips('ws1')]) + + expect(r1).toEqual([]) + expect(r2).toEqual([]) + const refreshCalls = fetchMock.mock.calls.filter(([url]) => url === '/api/auth/refresh') + expect(refreshCalls).toHaveLength(1) + }) + + it('clears the token, notifies the unauthorized handler, and surfaces the original 401 when refresh fails', async () => { + api.setAccessToken('old-token') + const onUnauthorized = vi.fn() + api.setUnauthorizedHandler(onUnauthorized) + fetchMock + .mockResolvedValueOnce(unauthorizedResponse('session expired')) + .mockResolvedValueOnce(unauthorizedResponse('refresh token expired')) + + await expect(api.getTips('ws1')).rejects.toMatchObject({ + status: 401, + error: { message: 'session expired' }, + }) + + expect(fetchMock).toHaveBeenCalledTimes(2) + expect(onUnauthorized).toHaveBeenCalledTimes(1) + }) + + it('does not loop when the retried request also 401s', async () => { + api.setAccessToken('old-token') + fetchMock + .mockResolvedValueOnce(unauthorizedResponse()) + .mockResolvedValueOnce(tokenResponse('new-token')) + .mockResolvedValueOnce(unauthorizedResponse('still unauthorized')) + + await expect(api.getTips('ws1')).rejects.toMatchObject({ + status: 401, + error: { message: 'still unauthorized' }, + }) + + // Original + refresh + one retry — no second refresh attempt. + expect(fetchMock).toHaveBeenCalledTimes(3) + }) + + it('does not attempt a refresh-and-retry for the auth endpoints themselves', async () => { + fetchMock.mockResolvedValue(unauthorizedResponse()) + + await expect(api.login({ email: 'a@b.com', password: 'x' })).rejects.toMatchObject({ status: 401 }) + await expect(api.refreshToken()).rejects.toMatchObject({ status: 401 }) + await expect(api.logout()).rejects.toMatchObject({ status: 401 }) + + expect(fetchMock).toHaveBeenCalledTimes(3) + }) +}) + describe('apiFetch — structured {message_key, data} errors', () => { let fetchMock: ReturnType diff --git a/frontend/src/services/api.ts b/frontend/src/services/api.ts index 36ece1a..d0bf8dc 100644 --- a/frontend/src/services/api.ts +++ b/frontend/src/services/api.ts @@ -520,9 +520,30 @@ export function setAccessToken(token: string | null): void { _accessToken = token } +// Endpoints that must never trigger a refresh-and-retry on 401 — retrying +// them would either be nonsensical (the refresh call retrying itself) or +// mask the real "not logged in" outcome (login/logout). +const AUTH_NO_RETRY_PATHS = new Set(['/auth/login', '/auth/refresh', '/auth/logout']) + +// Dedupe concurrent 401s (e.g. Dashboard's ~7 parallel requests all +// expiring at once) into a single in-flight refresh call, shared across +// every apiFetch caller. +let refreshPromise: Promise | null = null + +// AuthContext registers this so a refresh-and-retry failure can log the +// user out immediately, instead of leaving `isAuthenticated: true` while +// every subsequent request 401s. Kept out of this module so api.ts stays +// framework-agnostic. +let onUnauthorized: (() => void) | null = null + +export function setUnauthorizedHandler(handler: (() => void) | null): void { + onUnauthorized = handler +} + async function apiFetch( path: string, options: RequestInit = {}, + isRetry = false, ): Promise { const headers: Record = { 'Content-Type': 'application/json', @@ -532,6 +553,31 @@ async function apiFetch( headers['Authorization'] = `Bearer ${_accessToken}` } const response = await fetch(`${BASE}${path}`, { ...options, headers }) + + if (response.status === 401 && !isRetry && !AUTH_NO_RETRY_PATHS.has(path)) { + let refreshed: TokenResponse | null = null + try { + refreshPromise ??= refreshToken().finally(() => { + refreshPromise = null + }) + refreshed = await refreshPromise + } catch { + // Refresh itself failed (expired/invalid refresh cookie, or a + // network error) — clear the token and let the caller's original + // 401 surface below so AuthContext can log the user out. + setAccessToken(null) + onUnauthorized?.() + } + if (refreshed) { + setAccessToken(refreshed.access_token) + // Retry exactly once, outside the try/catch above — `isRetry: true` + // means a 401 on this attempt is final and must propagate as-is + // rather than being caught here and re-reported as the original + // (now stale) 401. + return await apiFetch(path, options, true) + } + } + if (!response.ok) { let error: { code: string; message: string; details?: Record } = { code: String(response.status),