feat(verifier): OIDC gate + auth routes + reviewer_id on jobs.db#70
Merged
jakebromberg merged 3 commits intoJun 10, 2026
Merged
Conversation
Member
Author
jakebromberg
added a commit
that referenced
this pull request
Jun 10, 2026
Code review (max effort, 9 angles + verify + sweep) surfaced 15 substantive findings. This commit addresses the ones with concrete failure scenarios; the cosmetic/altitude items are deferred.
Correctness: `claims["sub"]` raised uncaught KeyError because `claims_options` only declared `iss` and `aud` essential and `JWTClaims.validate()` doesn't enforce `sub` by default — added `sub: {"essential": True}` so a missing sub raises JoseError as documented. `body["id_token"]` raised raw KeyError and `token_response.json()` raised raw JSONDecodeError; both bypassed the documented httpx.HTTPError/JoseError failure-mode contract — wrap with explicit ValueError messages so the route layer's 400-mapping is unambiguous, and add `Accept: application/json` on the token POST so the server can't return form-urlencoded by default. `metadata[<endpoint>]` accesses raised uncaught KeyError on a non-conformant discovery doc — pre-check with `.get()` and raise ValueError with the missing field name. JWKS cache had no invalidation on signature failure — an auth-server key rotation broke logins until the verifier process restarted; now BadSignatureError invalidates the JWKS cache once and retries decode against the refreshed JWKS, mirroring the rotation-graceful behavior the module docstring promised. `_load_metadata`/`_load_jwks` race: concurrent first-callers each passed `if cache is None` and each fetched — added `asyncio.Lock`s with double-checked locking. Authorize-URL assembly used `f"{endpoint}?{urlencode}"` which produces a malformed URL with two `?`s if the endpoint already has a query string — switched to `authlib.common.urls.add_params_to_uri`, which parses and merges correctly. JWKS trust pool included every key in the JWKS regardless of `use`/`alg` — added `_signing_keys_only` filter so an encryption key or unsupported-alg key in the same JWKS can't join the signature-verification trust pool.
Data quality: `claims.get("email", "")` silently produced ReviewerSession(email="") for users without an email claim — now `email: str | None` and absent claims surface as None. `claims.get("preferred_username") or claims.get("username") or None` falsy-collapsed empty strings, masking IdP misconfig — replaced with `_first_present(...)` which distinguishes absent claim (None) from explicitly-empty claim (preserved as "").
Efficiency: TimestampSigner constructed per `encode_session`/`decode_session` call rerouted HMAC key derivation on every gated request — cached via `lru_cache` keyed on (secret, salt) so the request-gating middleware doesn't pay the HMAC-init cost per request. `JsonWebToken(["RS256","ES256"])` hoisted to module scope (`_JWT`). `JsonWebKey.generate_key("RSA", 2048, ...)` test fixtures changed from function-scoped to module-scoped (was ~1-3s of pure RSA keygen per pytest run).
Reuse: hand-rolled S256 PKCE replaced with `authlib.oauth2.rfc7636.create_s256_code_challenge` (authlib is already a dep, the previous implementation was the kind of crypto code we shouldn't own).
Test gaps closed: SESSION_TTL expiry branch of `decode_session` (held the wall clock 12h+60s forward); token endpoint 4xx (`httpx.HTTPStatusError`) — previously only ConnectError was covered; 2xx response missing `id_token` field → ValueError; 2xx response with non-JSON body → ValueError; id_token missing `sub` claim → JoseError; JWKS rotation: BadSignatureError triggers refetch + retry, pinned by a test that flips the stub JWKS between calls.
Deferred: OIDC nonce (code+PKCE flow with a locally-consumed id_token gets equivalent replay protection from the verifier-binding; added a module-docstring section explaining why); ReviewerSession dataclass vs Pydantic divergence (style; PR #70 already wraps with VerifiedBy Pydantic at the storage boundary); env-accessor consolidation (5 near-identical helpers; cosmetic).
jakebromberg
added a commit
that referenced
this pull request
Jun 10, 2026
Iter 3 of /code-review max found no remaining correctness bugs but surfaced two small follow-ups: the `decode_session` docstring didn't mention the `RuntimeError` branch (inviting a future reader to "clean up" the catch that protects against config-reload bugs), and the iter-2 cache-invalidation test added a redundant `_reset_metadata_cache()` call that contradicted the autouse fixture pattern. Also: the 1-char tamper logic in `test_decode_session_returns_none_on_tamper` and `test_verify_one_shot_raises_on_tamper` was the source of an intermittent flake on this branch — base64url's 27-char signature segment has 2 padding bits in the last char, so 4-of-64 random last-char replacements decode to the same HMAC bytes and the tampered value verifies. The fix to swap 4 chars (touching multiple HMAC bytes deterministically) lived on PR #70's branch and didn't carry back to PR #69; applied it here too so this branch's CI is stable in isolation. 3 consecutive full-suite runs clean (532 passed each).
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).
jakebromberg
force-pushed
the
feat/oidc-verifier-integration
branch
from
15ac7d6 to
92f24e5
Compare
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).
jakebromberg
added a commit
that referenced
this pull request
Jun 10, 2026
Code review (max effort, 9 angles + verify + sweep) surfaced 15 substantive findings. This commit addresses the ones with concrete failure scenarios; the cosmetic/altitude items are deferred.
Correctness: `claims["sub"]` raised uncaught KeyError because `claims_options` only declared `iss` and `aud` essential and `JWTClaims.validate()` doesn't enforce `sub` by default — added `sub: {"essential": True}` so a missing sub raises JoseError as documented. `body["id_token"]` raised raw KeyError and `token_response.json()` raised raw JSONDecodeError; both bypassed the documented httpx.HTTPError/JoseError failure-mode contract — wrap with explicit ValueError messages so the route layer's 400-mapping is unambiguous, and add `Accept: application/json` on the token POST so the server can't return form-urlencoded by default. `metadata[<endpoint>]` accesses raised uncaught KeyError on a non-conformant discovery doc — pre-check with `.get()` and raise ValueError with the missing field name. JWKS cache had no invalidation on signature failure — an auth-server key rotation broke logins until the verifier process restarted; now BadSignatureError invalidates the JWKS cache once and retries decode against the refreshed JWKS, mirroring the rotation-graceful behavior the module docstring promised. `_load_metadata`/`_load_jwks` race: concurrent first-callers each passed `if cache is None` and each fetched — added `asyncio.Lock`s with double-checked locking. Authorize-URL assembly used `f"{endpoint}?{urlencode}"` which produces a malformed URL with two `?`s if the endpoint already has a query string — switched to `authlib.common.urls.add_params_to_uri`, which parses and merges correctly. JWKS trust pool included every key in the JWKS regardless of `use`/`alg` — added `_signing_keys_only` filter so an encryption key or unsupported-alg key in the same JWKS can't join the signature-verification trust pool.
Data quality: `claims.get("email", "")` silently produced ReviewerSession(email="") for users without an email claim — now `email: str | None` and absent claims surface as None. `claims.get("preferred_username") or claims.get("username") or None` falsy-collapsed empty strings, masking IdP misconfig — replaced with `_first_present(...)` which distinguishes absent claim (None) from explicitly-empty claim (preserved as "").
Efficiency: TimestampSigner constructed per `encode_session`/`decode_session` call rerouted HMAC key derivation on every gated request — cached via `lru_cache` keyed on (secret, salt) so the request-gating middleware doesn't pay the HMAC-init cost per request. `JsonWebToken(["RS256","ES256"])` hoisted to module scope (`_JWT`). `JsonWebKey.generate_key("RSA", 2048, ...)` test fixtures changed from function-scoped to module-scoped (was ~1-3s of pure RSA keygen per pytest run).
Reuse: hand-rolled S256 PKCE replaced with `authlib.oauth2.rfc7636.create_s256_code_challenge` (authlib is already a dep, the previous implementation was the kind of crypto code we shouldn't own).
Test gaps closed: SESSION_TTL expiry branch of `decode_session` (held the wall clock 12h+60s forward); token endpoint 4xx (`httpx.HTTPStatusError`) — previously only ConnectError was covered; 2xx response missing `id_token` field → ValueError; 2xx response with non-JSON body → ValueError; id_token missing `sub` claim → JoseError; JWKS rotation: BadSignatureError triggers refetch + retry, pinned by a test that flips the stub JWKS between calls.
Deferred: OIDC nonce (code+PKCE flow with a locally-consumed id_token gets equivalent replay protection from the verifier-binding; added a module-docstring section explaining why); ReviewerSession dataclass vs Pydantic divergence (style; PR #70 already wraps with VerifiedBy Pydantic at the storage boundary); env-accessor consolidation (5 near-identical helpers; cosmetic).
jakebromberg
added a commit
that referenced
this pull request
Jun 10, 2026
Iter 3 of /code-review max found no remaining correctness bugs but surfaced two small follow-ups: the `decode_session` docstring didn't mention the `RuntimeError` branch (inviting a future reader to "clean up" the catch that protects against config-reload bugs), and the iter-2 cache-invalidation test added a redundant `_reset_metadata_cache()` call that contradicted the autouse fixture pattern. Also: the 1-char tamper logic in `test_decode_session_returns_none_on_tamper` and `test_verify_one_shot_raises_on_tamper` was the source of an intermittent flake on this branch — base64url's 27-char signature segment has 2 padding bits in the last char, so 4-of-64 random last-char replacements decode to the same HMAC bytes and the tampered value verifies. The fix to swap 4 chars (touching multiple HMAC bytes deterministically) lived on PR #70's branch and didn't carry back to PR #69; applied it here too so this branch's CI is stable in isolation. 3 consecutive full-suite runs clean (532 passed each).
jakebromberg
added a commit
that referenced
this pull request
Jun 10, 2026
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).
3 tasks
jakebromberg
added a commit
that referenced
this pull request
Jun 10, 2026
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).
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.
Closes #68.
Wires the OIDC client from #69 into
verifier/serve.pyand the SPA. This PR targetsfeat/oidc-core-auth(stacked on #69) so the review diff shows only this PR's incremental change. Merge #69 first, then this rebases onto main.Summary
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/jsonso the SPA can redirect via location.href instead of getting trapped by a silent fetch redirect./auth/login(PKCE + three signed one-shot cookies),/auth/callback(state verify + token exchange + session seal),/auth/logout(clear cookie locally only),/auth/me(current reviewer JSON or 401). Transport failures → 503; claim failures → 400./api/saveserver-as-authority: takesDepends(get_reviewer), validates the PageResult, then overwritesverified_bywith the authenticated reviewer's identity. A client cannot spoof another reviewer into the saved file.reviewer_id TEXTcolumn onjobs.db(late-column ALTER TABLE migration). Denormalized OIDC user_id for per-reviewer queries without parsing every JSON.VerifiedByschema model + optionalPageResult.verified_by. Every pre-OIDCverified.jsonparses unchanged; new saves write the block; old files upgrade on next save.authFetchwrapper on/api/save,/api/bundles,/api/lookup— redirects to/auth/loginon 401./auth/mefetched on load to render the reviewer's name in the header chip. Silent if /auth/me 404s (no-auth deployment).tests/unit/conftest.pywith_reset_serve_module_statehelper. Newtest_verifier_auth_routes.pycovers all routes, middleware, server-authority overwrite, transport/claim failures, tri-modal precedence, and Secure-flag gating.Test plan
pytest— 554 passed, 8 skipped (32 net new tests across this + feat(auth): OIDC client + signed session-cookie codec #69)ruff check,ruff format --check,mypy core cli.pycleanlocalhost:8082/authafter the Backend-ServicetrustedClientlands separately: /auth/login → consent → /auth/callback → land at /verifier/, name renders in header, save writesverified_byblock +jobs.reviewer_id.Cross-repo coordination
Both sides gate on env vars, so the rollout is reversible:
FLOWSHEET_OIDC_CLIENT_IDset on Backend-Service.WXYC_OIDC_CLIENT_IDunset.The Backend-Service refactor (
buildTrustedClients()helper + flowsheet client entry) is a separate PR in that repo; it does not block this one from merging behind the env-var gate.