feat(auth): add JWT access tokens with rotating refresh sessions (E1.2)#73
Merged
Conversation
10 tasks
Collaborator
Author
|
Dispositions for the automated review threads:
Also added since first review: atomic refresh rotation (concurrency-safe), an |
Native token authentication on top of the E1.1 identity model. Purely additive: existing routes keep their current behaviour, so nothing breaks before the frontend can sign in (E1.4), and the single enforcement flip stays E1.3's job. - /auth/register, /auth/login, /auth/refresh, /auth/logout, /auth/me. Access tokens are HS256 JWTs (~15 min TTL) signed with AUTH_SECRET_KEY, which staging/production refuse to start without. - Refresh tokens are opaque 256-bit secrets delivered in an httpOnly SameSite=Lax cookie scoped to /auth; only their sha256 is stored. Each refresh rotates the token within a family; presenting an already-used token is treated as theft and revokes the whole family, logging the event. - Passwords are hashed with argon2id. Unknown emails and credential-less accounts (seed user) burn the same dummy verification as a real attempt, and every credential failure returns an identical 401 body, so responses cannot enumerate accounts. Registration enforces a 10-char minimum. - get_current_user now strictly requires a Bearer token (first consumer: /auth/me). Repository routes move to get_current_user_or_default, which validates a presented token strictly (bad token = 401, never a silent fallback) and otherwise keeps the temporary pre-auth attribution that E1.3 will delete. - Migration 0003_auth_credentials adds users.password_hash and the refresh_tokens table; upgrade/downgrade/upgrade verified on SQLite, and the revision id stays under the 32-char alembic_version limit. - New deps: PyJWT, argon2-cffi, pydantic[email]. Tests: 17 new covering rotation, family revocation on reuse, logout idempotency, expired/garbage/missing tokens, indistinguishable credential failures, storage hygiene (argon2id hashes, no raw tokens in the database), and that anonymous access to existing routes still works. Full suite: 109 passed, 1 skipped.
…or (E1.2) Pre-review hardening for the auth PR. - Refresh rotation is now atomic. The old read-then-write let two requests presenting the same token both pass the used_at check and each mint a successor. Rotation now claims the token with a single UPDATE ... WHERE used_at IS NULL: the row lock serializes racers and only the one whose update affects a row proceeds; the loser is treated as a concurrent replay and its family is revoked. - Enforce AUTH_SECRET_KEY strength: outside development/test the signing key must be at least 32 characters, so a weak HS256 secret fails fast at startup instead of silently weakening every token. - Replace the burn-check's empty except with contextlib.suppress plus a comment explaining the discarded result is intentional (anti-enumeration timing). Tests: - Deterministic single-use claim test (runs on any engine) proving a second claim of the same token affects zero rows. - A real-Postgres threaded test: two concurrent refreshes of one token, exactly one succeeds. Gated on PARTHA_TEST_PG_URL and wired into the Backend CI job, which now runs a postgres:16 service so the race is exercised for real (SQLite serializes writes and cannot). Skips locally without the env var. - Secret-strength validation test. Full suite: 111 passed, 2 skipped (Postgres-gated + pre-existing).
…-validator (E1.2) Owed since #72 merged the pinned-lockfile convention after this branch forked. Regenerated from a clean venv per the file header procedure (pip install ./apps/backend into an empty venv, then freeze) so no stale packages from a dev environment leak in. Adds argon2-cffi + argon2-cffi-bindings (password hashing), PyJWT (access tokens), and email-validator + dnspython (pydantic[email]'s validation extra) - exactly the new dependencies pyproject.toml declares for E1.2. Every other pinned version is unchanged; networkx and GitPython remain absent.
c5cd485 to
8446ba8
Compare
The code-quality bot's finding on test_auth_concurrency.py:49 stayed resolved-but-not-outdated through the rebase (the line was untouched), so it was still live. Only token_id is asserted on; raw was never read.
AuthService.register() checked for an existing email, then inserted and committed - two simultaneous registrations for the same normalized email could both pass the check, and the loser's commit would raise a raw IntegrityError, surfacing as a generic 500 instead of the intended 409. Catch IntegrityError on commit, roll back (leaving the session usable), and re-check whether the email now exists before reporting the same ConflictServiceError the normal duplicate path already returns - any other integrity failure is re-raised rather than mislabeled as a duplicate email. No database error text, table, or constraint name is ever exposed; the unique constraint itself is unchanged and remains the real guard. Added a deterministic regression test that reproduces the exact race by intercepting the losing session's own commit() to run a second, independent session's successful registration first - the same database-level interleaving two real concurrent requests would produce - then verifies: the public 409/conflict_error contract, that the session is usable after rollback, that email normalization holds across the race (mixed case and whitespace on both sides), and that exactly one user row exists afterward.
The existing Postgres concurrency test only proved "one refresh wins, one is rejected." It didn't prove or document what happens to the winner's own successor once the loser's replay handling revokes the whole family - which is the actual, intentional, strict fail-closed behavior: any concurrent presentation of the same refresh token is treated as possible theft, so the winner is logged out too rather than silently kept alive. Improving UX here (e.g. preserving the winner) would need a larger protocol decision - recoverable successor state, a grace window, device binding - that is out of scope for E1.2 and is not implemented here. Extended the real-Postgres test to capture the winner's successor, then from a fresh session after both threads finish: confirm the successor is itself rejected, confirm every row in the token family has revoked_at set, and confirm no unused/unrevoked row remains. Thread timeouts and unexpected exceptions (anything other than the intended UnauthorizedError) now fail the test explicitly rather than being folded into "rejected." Added a deterministic companion test (SQLite, any engine) that proves the same service-level rule sequentially, without needing threads or a real row lock - the atomic claim primitive itself is already covered separately by test_claim_token_is_single_use; this proves what happens next once a claim is lost.
parthrohit22
approved these changes
Jul 12, 2026
SHAURYAKSHARMA24
added a commit
that referenced
this pull request
Jul 12, 2026
…RS/security headers and concurrent atomicity (E2.1) Pre-merge correctness pass after rebasing onto dev (post-#73 auth merge): the Redis client built in build_rate_limit_store was never closed, leaking its connection pool across app restarts/reloads. lifespan now closes it via a duck-typed aclose() on shutdown, a no-op for the in-memory store. Also strengthens test coverage per review: a 429 now has an explicit test proving it still carries CORS and security headers (RateLimitMiddleware is innermost of the three), and the gated real-Redis test gains a concurrent-hit case proving the atomic Lua script yields exact, non-overlapping counts under real parallel load rather than only sequential calls.
parthrohit22
pushed a commit
that referenced
this pull request
Jul 12, 2026
…RS/security headers and concurrent atomicity (E2.1) Pre-merge correctness pass after rebasing onto dev (post-#73 auth merge): the Redis client built in build_rate_limit_store was never closed, leaking its connection pool across app restarts/reloads. lifespan now closes it via a duck-typed aclose() on shutdown, a no-op for the in-memory store. Also strengthens test coverage per review: a 429 now has an explicit test proving it still carries CORS and security headers (RateLimitMiddleware is innermost of the three), and the gated real-Redis test gains a concurrent-hit case proving the atomic Lua script yields exact, non-overlapping counts under real parallel load rather than only sequential calls.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements E1.2 — native token authentication on top of E1.1's identity model, per the build-vs-buy decision recorded on #60 (native JWT over Auth0: PARTHA is open-source and self-hosted, so the core login path shouldn't require a vendor tenant, and the local
User+ scoping work was needed either way).What's here
POST /auth/register,/auth/login,/auth/refresh,/auth/logout,GET /auth/me.AUTH_SECRET_KEY. Staging/production refuse to boot without an explicit secret (and now enforce a ≥32-char floor outside dev/test); development/test substitute a fixed, clearly-labelled insecure value.SameSite=Laxcookie scoped to/auth(XSS cannot read it;localhost:5173 → localhost:8000is same-site, so dev works over plain http;Secureflag turns on outside dev/test). Only the sha256 of a token is ever stored. Every/auth/refreshrotates within a token family via an atomicUPDATE ... WHERE used_at IS NULLclaim (the row lock serializes concurrent racers so only one winner mints a successor)./auth/refreshrequests for the same browser session. Noted on E1.4 — Frontend auth flow (login/register, token handling, guarded routes) #64.IntegrityErroris now caught, the session rolled back, and — only after confirming the email really does exist — the same 409/conflict_errorthe normal duplicate path returns is raised. An unrelated integrity failure is never mislabeled as a duplicate-email conflict, and no DB error text/table/constraint name is ever exposed.argon2-cffi). Unknown emails and credential-less accounts (the seed user) burn the same dummy verification as a real attempt, and all credential failures return byte-identical 401 bodies — login cannot be used to enumerate accounts. Registration requires ≥10 chars; login deliberately doesn't reveal the policy.get_current_useris now strict (valid Bearer or 401) — first consumer is/auth/me. Repository routes move toget_current_user_or_default: a presented token is always validated strictly (bad token = 401, never a silent fallback to the seed user); only the absence of a token falls back to the temporary pre-auth attribution. That fallback (and theX-Dev-Userheader that drives it in development/test) is not removed by this PR — it's intentionally retained until E1.3 flips enforcement once the frontend can sign in (E1.4) — this PR deliberately breaks nothing.0003_auth_credentials(21 chars — respects the 32-charalembic_versionlimit that bit feat(auth): add User model and per-user repository ownership (E1.1) #70), correctly based on the already-merged0002_users_and_repo_owner: addsusers.password_hash(nullable — pre-auth rows must never become logins) and therefresh_tokenstable (hash, family, expiry, used/revoked timestamps, all indexed).dev's existing convention:app/models/__init__.pyimportsRefreshTokenalongsideRepositoryRecord/User, andmain.py/alembic/env.pyrely on that package-init side effect viafrom app.models.base import Basealone — no explicit per-name imports, nonoqa: F401anywhere in the backend. (Base.metadata.tablesverified to containrepositories,users,refresh_tokens.)Dependencies:
PyJWT,argon2-cffi,pydantic[email](pulls inemail-validator/dnspython) — unchanged since the last update;apps/backend/requirements.txtwas regenerated once from a clean venv and is not touched again here (nothing new to pin:IntegrityErrorhandling uses SQLAlchemy, already a dependency).networkx/GitPythonremain absent.Type
Verification
alembic upgrade headCMD; CI's Docker Compose job passed on this head.env.exampledocumentsAUTH_SECRET_KEY+ TTLs, including the generation commandCommands/results (local, Windows, Python 3.13; CI is Linux with a
postgres:16service):CI status: see the PR checks for the current head — do not treat this section as a substitute for the live status.
Risk and Rollback
AUTH_SECRET_KEYhandling: never logged, required outside dev/test with a ≥32-char floor, documented generation command. Rotating the secret invalidates all outstanding access tokens (acceptable: 15-min TTL) but not refresh sessions./authrate limiting (E2.1's budget list already includes/auth/login+/auth/register).postgres:16service and Docker Compose job on this head, not a local Docker install — none was available in this environment.alembic downgrade 0002_users_and_repo_owner.Checklist
/auth/*endpoints described above; existing routes unchangedCloses #62