fix(dashboard): match OIDC introspection identity on username, not sub (#1571) - #1605
Merged
Conversation
) Caught live via dashboard/Keycloak logs on the dev box: auth.xore.rocks's introspection responses for the apiary-dashboard client never carry a "sub" claim (confirmed both in the raw access-token JWT itself and in the introspection endpoint's own response -- RFC 7662 lists "sub" as OPTIONAL, and this realm/client combination only puts it on ID tokens). identityFromRequest()'s introspect() call compared the response's "sub" against the session's subject, so result.Subject was always "" and every session failed its very first 30-second re-check unconditionally -- not an intermittent race at all, just gated on request timing: any session that made an API call >=30s after login (or its last check) hit this every single time, then silently "healed" on the next full navigation because Keycloak's own SSO cookie re-authenticated invisibly, looking exactly like #1571's "reload fixes it with no re-login" report. Live evidence: hp-dashboard/hp-dashboard-b logs showed hundreds of "introspection reported inactive" rejections for the same subject, every one at age/lastValidated ~30-60s (the first post-login re-check), with no preceding refresh attempt -- ruling out a stale/expired token. Manually introspecting a live, seconds-old access token pulled from the shared Valkey session store confirmed active:true but no "sub" field, while "username" was present and correct. "username" is also RFC 7662 OPTIONAL but is what this deployment reliably returns, so introspect() now matches on that instead. Existing tests updated to mirror the real (sub-less) response shape; new tests lock in that a sub-less-but-active response is accepted and that a genuine username mismatch is still rejected and the session still deleted. go build ./..., go vet ./..., and go test -race ./... (minus the pre-existing, unrelated flaky race in TestSyntheticSensorsReachDashboardSnapshot, reproduced identically on main without this change) all pass. Fixes #1571 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The public-repository safety check flags any reference to this deployment's real domain in the public repo. Reword the comment to describe the same fact without naming it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…start Scenario 1b (real Keycloak restart) started failing after #1599's username-vs-sub introspection fix, expecting a 303 re-login redirect but getting 200. Investigated with direct evidence: querying Keycloak's own introspection endpoint right after `docker restart` (a graceful SIGTERM stop, Postgres backing store left running) shows the session genuinely survives -- a clean, authoritative "active":true for the pre-restart access token, reproduced across multiple runs. This deployment's Keycloak 26.7.1 reloads SSO session state from its DB on startup; the old "restart wipes all session state" assumption baked into this test's comments was wrong for this restart shape. Before #1599, this scenario's 303 only "passed" because the sub-matching bug rejected every 30s re-check unconditionally (#1571) regardless of what Keycloak actually said -- not because the restart itself invalidated anything. Updated scenario 1b to assert the session survives (200), matching Keycloak's genuinely-authoritative answer and production's own Postgres-backed topology. Added scenario 1c using Keycloak's admin logout-user REST API for a real, deterministic session revocation (confirmed live to flip introspection to "active":false immediately, independent of restart/persistence internals) so the "dashboard cleanly 303s when Keycloak says the session is gone" coverage 1b used to (incorrectly) provide isn't lost. Verified: go build/vet/test all green, and the real-Keycloak chaos script passes deterministically across repeated runs. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ction match #1599 changed identityFromRequest()'s 30s re-check to match the introspection response's "username" instead of "sub" (this deployment's Keycloak never populates "sub" on access tokens). The e2e suite's fake OIDC issuer only ever echoed back "sub", so every fixture session got rejected on its first re-check once the browser matrix ran long enough to cross 30s -- exactly the same bug class, now hitting the test fixture instead of production. Echo "username" instead, sourced from the same FIXTURE_USERNAME constant the fixture session is seeded with, so the two can't drift apart again. Also flip playwright.config.ts to fullyParallel + a fixed CI worker count -- fullyParallel: false meant the whole 333-test single spec file only ever ran on one worker regardless of --workers, which is what let the 30s-aging bug go unnoticed in the first place (fast parallel runs never kept a session alive that long). Verified this doesn't introduce cross-test interference: 333/333 pass repeatably with 4 workers. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…sername-not-sub-v2
7 tasks
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
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.
Supersedes #1599, rebased onto main after #1604 (CI matrix parallelization) merged, so this PR's own CI runs the new parallel Go/Scripts-and-Compose jobs.
The fix (from #1599)
Root cause of #1571 (dashboard fetch()s intermittently 401 despite a valid session):
identityFromRequest()'s 30-second introspection re-check compared the response'ssubclaim against the session's subject. This deployment's Keycloak never populatessubon access tokens (confirmed live), soresult.Subjectwas always""and every session failed its very first 30s re-check unconditionally — deterministic, not intermittent, which is what made it look random from the browser (Keycloak's own SSO cookie silently re-authenticated on the next full page load, masking it).Fix: match on
usernameinstead (also RFC 7662 OPTIONAL, but reliably populated by this deployment).Also included
scripts/test-dashboard-oidc-chaos.sh) had a scenario asserting a graceful Keycloak restart forces re-login — that assumption was itself wrong (real Postgres-backed sessions survive a graceful restart; verified live against real Keycloak). Corrected to assert the actual behavior, plus a new scenario for genuine admin-triggered session revocation, preserving real security coverage.sub, so it hit the exact same bug class once the browser-matrix test run got past 30s — updated to echousernamefrom the same fixture constant the seeded session uses, so they can't drift apart again.playwright.config.tsflipped tofullyParallel: true+ a fixed CI worker count — the previousfullyParallel: falsemeant the whole 333-test suite only ever ran on one worker, which is exactly what let the 30s-aging bug hide in fast local runs. Verified 333/333 pass repeatably with 4 workers.Verified:
go build,go vet,go test ./...all clean. Full Playwright suite: 333/333 passing.Fixes #1571