feat(verifier): OIDC gate + auth routes + reviewer_id on jobs.db (re-target)#71
Merged
Merged
Conversation
Wires `core/auth.py` (from the previous commit) into the verifier UI.
Server (`verifier/serve.py`):
- Tri-modal middleware install: OIDC session cookie when WXYC_OIDC_CLIENT_ID is set, BasicAuth when only VERIFIER_PASSWORD is set, no gate otherwise. OIDC wins when both are configured so the BasicAuth env can stay set during the cutover without competing.
- `_SessionCookieMiddleware` reads `flowsheet_session`, stashes the decoded `ReviewerSession` on `request.state.reviewer`. 302 to /auth/login for HTML nav, 401 JSON for `Accept: application/json` so the SPA can redirect via location.href without being trapped by a silent fetch redirect.
- `_PUBLIC_PATHS` = {/auth/login, /auth/callback, /auth/logout, /api/version}. /api/version is load-bearing — it's the Railway healthcheck and dropping it from this set would roll back every deploy.
- Four new routes: GET /auth/login (302 to authorize URL + three signed one-shot cookies for state/verifier/return_to), GET /auth/callback (verify state + exchange code + seal session, transport failures → 503, claim failures → 400), POST /auth/logout (clear cookie locally only; Better Auth session at api.wxyc.org is unaffected), GET /auth/me (return current ReviewerSession or 401).
- `/api/save` now takes `Depends(get_reviewer)`, validates the client PageResult, then **overwrites** `verified_by` with the authenticated reviewer's identity before persistence. A client cannot spoof another reviewer into the saved file. `reviewer_id` is denormalized into `jobs.db` so per-reviewer queries skip JSON parsing.
- Cookie `Secure` flag derives from `FLOWSHEET_PUBLIC_URL.startswith("https")` so local-dev HTTP works without per-env conditionals.
Schema + jobs:
- `core/schema.py` adds `VerifiedBy` and `PageResult.verified_by` (Optional, default None). Every existing verified.json parses unchanged; new saves write the block; old files are upgraded on next save.
- `core/jobs.py` adds `reviewer_id TEXT` as a late column (`ALTER TABLE ADD COLUMN` migrates existing prod DBs without row rewrite) plus a `reviewer_id` arg on `mark_verified`. `Job.reviewer_id` is `str | None`; the existing BasicAuth and no-auth deployments persist NULL without changes elsewhere.
UI (`verifier/app.js`, `index.html`, `styles.css`):
- New `authFetch` wrapper around the API fetches with `Accept: application/json`. On 401 it redirects to /auth/login with the current path as `return_to`.
- New `renderReviewerName()` fetches /auth/me on load and shows the reviewer's `real_name || dj_name || username || email` in the header chip. Silent if /auth/me 404s (no-auth deployment).
- All API calls (/api/save, /api/bundles, /api/lookup) routed through `authFetch`. /api/version stays a plain fetch (public).
Tests:
- New `tests/unit/conftest.py` with `_reset_serve_module_state` helper — clears `serve._initialized_jobs_dbs` and `auth._metadata`/`_jwks` after `importlib.reload`, which the parametrized middleware-precedence and Secure-flag tests need.
- New `tests/unit/test_verifier_auth_routes.py` covers all four /auth routes, /api/save reviewer threading + verified_by overwrite + malformed-payload defense, /api/version public, middleware precedence (OIDC > BasicAuth > none), Secure flag gating, transport-failure → 503, claim-failure → 400, and the full login → callback return_to round-trip.
- `tests/unit/test_jobs.py` covers `reviewer_id` round-trip, NULL default, overwrite on re-save, and the ALTER TABLE migration including the new column.
- `tests/unit/test_schema.py` covers the `verified_by` default, the legacy verified.json (no `verified_by` key) backward-compat load, and full encode/decode round-trip.
- `tests/unit/test_auth.py` tamper tests now replace 4 chars instead of 1 — base64url's 27-char signature has 2 padding bits in the last char, giving 1-in-16 odds of a random 1-char tamper landing on an alias that decodes to the same HMAC bytes. The 4-char replacement is deterministic.
CLAUDE.md and .env.example updated for the new module and the five env vars (WXYC_AUTH_ISSUER, WXYC_OIDC_CLIENT_ID, WXYC_OIDC_CLIENT_SECRET, WXYC_SESSION_SECRET, FLOWSHEET_PUBLIC_URL).
Plan: plans/oidc-auth.md (PR #3 scope).
Code review (max effort, 4 angles in parallel) on PR #70 surfaced 6 substantive issues. This commit fixes the ones with concrete failure scenarios. Data safety: `/api/save` previously did `validated.verified_by = None` (and `reviewer_id=None` into jobs.db) whenever no reviewer was authenticated, which destroys a previously-recorded OIDC reviewer on every BasicAuth / no-auth re-save. Per the org data-safety rule ('never overwrite successfully collected data'), the no-reviewer path now reads the prior `verified_by` off disk via a new `_existing_verified_by(path)` helper and writes that back; jobs.reviewer_id mirrors the preserved value. The anti-spoof rule still holds — clients cannot set `verified_by` in either mode; in non-OIDC mode the server's source of truth is the prior file, not the client. Correctness: `auth_login` and `auth_callback` previously caught only `httpx.HTTPError` from the OIDC client, letting `RuntimeError` from `core.auth`'s env-var accessors (e.g., `WXYC_AUTH_ISSUER` unset after a partial config rollout) bubble up as a 500 with the env-var name in the traceback. Both routes now also catch `RuntimeError` and translate to 503 'auth not configured', mirroring the protection `core.auth.decode_session` already has for the request-gating path. `_validate_return_to` no longer uses a plain `startswith("/verifier")` — it requires a path-separator-or-end boundary (`"/verifier" exact OR "/verifier/" prefix OR "/verifier?" prefix`) so a future sibling route at `/verifierx-admin` or `/verifierdata` can't smuggle through the allowlist. UX: `loadBundleFromUrlParam` was using plain `fetch` against `/data/<stem>.bundle.json` and `/data/<stem>.verified.json`. With OIDC enabled, the `/data/` static mount is behind the session middleware — and the middleware's 302→/auth/login response would be silently followed by the browser, then `JSON.parse` would fail on the login page HTML, surfacing as 'Failed to load bundle: Unexpected token <'. Now uses `authFetch` so an expired session triggers a clean redirect. `index.html` bumps the cache-bust query strings to `v=6` (the existing comment documents the bump-on-change convention; PR #70 added new JS + CSS without bumping). Data quality: `JobStore.mark_verified` was calling `_now()` twice (once for `verified_at`, once for `updated_at`), producing two microsecond-differentiated timestamps for one operation. Other methods (`register()`) bind `now = _now()` once. Switched to match. Tests: new tests pin (a) `_validate_return_to`'s word-boundary check via parametrize over `/verifierx-admin`, `/verifierdata`, `//evil/...` , `https://evil/`; (b) the RuntimeError→503 mapping on both auth routes; (c) the no-auth-mode preservation of an existing `verified_by` on disk; (d) the no-auth-mode anti-spoof rule (client-supplied verified_by still discarded when there's no prior file). Deferred (lower-impact items the review surfaced): unconditional `/auth/me` registration in non-OIDC mode (returns 401 instead of 404; SPA already handles both the same way, so no user-visible bug); `authFetch` error flashing 'Save failed' briefly before navigation kicks in (UX polish); chip flex layout that wraps on long status messages (CSS polish); split test_verifier_auth_routes.py into per-concern files (size cleanup); cookie Path / SameSite assertion tightening (test depth).
Iter 2 of /code-review max surfaced two additional RuntimeError gaps in auth_callback / auth_login that iter 1 missed. auth_callback's `return_to` cookie decode block (the third verify_one_shot call) only caught BadSignature, not RuntimeError — so a config-blank window between the first two verify_one_shot calls (which iter 1 protected) and this third one would still 500. Added the matching RuntimeError → 503 catch so all three one-shot decodes have consistent protection. auth_login's try/except in iter 1 wrapped only `build_authorize_url`, but the three subsequent `sign_one_shot` calls reach `_session_secret()` via `_one_shot_signer()` — a deploy that updates WXYC_OIDC_CLIENT_ID and forgets WXYC_SESSION_SECRET would succeed in building the URL and then 500 sealing the cookies. Widened the try/except to cover the URL build AND the cookie sealing so any RuntimeError from any env-var accessor surfaces as 503. The existing 503 test only patched build_authorize_url directly; added a second test that lets build_authorize_url succeed and then blanks WXYC_SESSION_SECRET to actually exercise the sign_one_shot RuntimeError path — pinning the regression. Deferred: image fetches via <img src="/data/..."> can't go through authFetch and silently follow the middleware 302 to /auth/login when the session is expired, displaying a broken-image instead of redirecting. Real UX issue but the fix (detect broken image + redirect, or middleware content-type sniffing) is larger and adjacent to the existing UX-polish items (authFetch error flash, chip flex wrap).
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
1 participant
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.
Re-targets PR #70's content to
main. The original PR #70 merged into its stack base (feat/oidc-core-auth), but that branch never flowed forward to main — only PR #69 reached main, so the verifier-side wiring of the OIDC client is currently stranded.This PR cherry-picks the same three commits onto a fresh branch off the current main:
feat(verifier): OIDC reviewer-identity gate + verified_by provenancefix(verifier): address code-review findings (iter 1)fix(verifier): address code-review findings (iter 2)No content changes vs the original PR #70 head — same diff (~1722 lines net against main).
Closes #68.
Summary (unchanged from original PR #70)
verifier/serve.py: OIDC session cookie whenWXYC_OIDC_CLIENT_IDis set, BasicAuth when onlyVERIFIER_PASSWORDis set, no gate otherwise. OIDC wins when both are configured.flowsheet_session, stashes the decodedReviewerSessiononrequest.state.reviewer. 302 to /auth/login for HTML; 401 JSON forAccept: application/json./auth/login,/auth/callback,/auth/logout,/auth/me. Transport failures → 503; claim failures → 400; env-var unset (config rollout) → 503./api/saveserver-as-authority: overwritesverified_bywith the authenticated reviewer's identity. In BasicAuth/no-auth mode preserves any prior on-diskverified_by(data-safety rule: never overwrite successfully collected data).reviewer_id TEXTcolumn onjobs.db(late-column ALTER TABLE migration).VerifiedByschema model + optionalPageResult.verified_by. Old verified.json files parse unchanged.authFetchwrapper on/api/save,/api/bundles,/api/lookup, and the/data/*bundle/verified fetches./auth/mefetched on load to render the reviewer's name in the header chip.Test plan
pytest— 572 passed, 8 skippedruff check,ruff format --check,mypy core cli.pycleanCross-repo coordination
Both sides gate on env vars; rollout is reversible. Backend-Service
trustedCliententry is a separate PR in that repo and doesn't block this one (env-var gate handles the unconfigured case).