Slice 4/5: feat(calm-hub): OIDC-driven auth config and VS Code plugin login - #3065
Merged
Merged
Conversation
jpgough-ms
requested review from
a team,
Thels,
grahampacker-ms,
markscott-ms,
rocketstack-matt and
willosborne
as code owners
September 8, 2026 09:57
github-actions
Bot
added
calm-hub
This was referenced Sep 8, 2026
jpgough-ms
changed the title
jpgough-ms
commented
Sep 8, 2026
| return user.access_token; | ||
| // Entra ID: access_token audience is MS Graph, not our API. | ||
| // Send the id_token which has our client_id as audience. | ||
| return user.id_token || user.access_token; |
Member
Author
There was a problem hiding this comment.
Carried forward from #3001's review (still open there): the ID token is sent as the API bearer credential for every IdP, not just Entra ID. An ID token is an authentication receipt for the client, not an authorization credential for a resource server — this is the anti-pattern the OAuth 2.0 Security BCP specifically calls out. Worth scoping this to Entra specifically (or switching to access_token once the server-side audience validation supports it) rather than applying the workaround globally.
Member
Author
There was a problem hiding this comment.
Correct, and confirmed — not scoped to Entra. Tracked as #3077 rather than fixed here: switching the bearer credential to the access token touches every configured IdP's token issuance/scope config, not just this slice's code, so it's out of scope for this PR. Verified nothing new in this slice compounds it — PluginAuthResource's callback page has the same id_token preference, noted in the issue too.
jpgough-ms
commented
Sep 8, 2026
jpgough-ms
marked this pull request as draft
September 8, 2026 10:26
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
rocketstack-matt
force-pushed
the
slice/3001-s4-oidc-plugin-auth
branch
from
September 8, 2026 16:48
632c5ab to
e122688
Compare
jpgough-ms
force-pushed
the
slice/3001-s4-oidc-plugin-auth
branch
from
September 9, 2026 08:19
e122688 to
f4fd12a
Compare
rocketstack-matt
force-pushed
the
slice/3001-s4-oidc-plugin-auth
branch
from
September 9, 2026 08:57
f4fd12a to
5429d0b
Compare
jpgough-ms
force-pushed
the
slice/3001-s4-oidc-plugin-auth
branch
2 times, most recently
from
September 9, 2026 11:21
2c1e50d to
3bc8cd3
Compare
jpgough-ms
force-pushed
the
slice/3001-s4-oidc-plugin-auth
branch
from
September 9, 2026 12:13
3bc8cd3 to
19f2e41
Compare
Extracted from #3001. Serves OIDC config to the SPA from the server (/api/calm/auth/config) instead of build-time constants, adds the VS Code plugin's browser-based OIDC login flow (PluginAuthResource + OidcPluginAuthClient), and lets namespace/domain read access be granted via any UserAccessStore namespace grant, not just the existing UserAccessValidator path — needed for backends where that validator isn't resolvable. Original-PR: #3001
jpgough-ms
force-pushed
the
slice/3001-s4-oidc-plugin-auth
branch
from
September 9, 2026 12:43
19f2e41 to
aa24360
Compare
- PluginAuthResource: close XSS/state-hijack/open-redirect chain on the public plugin-login/plugin-callback endpoints. Validate redirect_path against a strict allowlist (blocks the http://localhost:<port>@evil.com/ authority-injection variant too), bind state to a HttpOnly SameSite=Lax session cookie, make session lookup/consumption atomic and non-destructive on failure, add a TTL + size cap, and replace the concatenated HTML/JS callback page with a JSON data island read by a constant script (no interpolation), a per-response CSP nonce, and Cache-Control/Referrer-Policy headers. Send and verify the OIDC nonce. - CalmHubPermissionChecker: revert the unconditional hasAnyNamespaceAccess() widening on canReadByDomain -- any namespace grant was unlocking every domain, regardless of relation, across every auth profile. Restores the original ProxyAuthIntegration/unit-test assertions. - UserAccessValidator: drop @IfBuildProfile(secure, proxy-auth) entirely. Bean selection for that annotation is fixed at Maven build time and does not respond to calm-hub's documented runtime '-Dquarkus.profile' deployment model (CI builds one artifact, no -Dquarkus.profile flag) -- under that model the bean never activated for ANY profile, silently leaving SearchResource/DomainResource/SearchTools unfiltered. Every caller already gates on calm.auth.enabled first, so unconditional registration is safe. Removes NamespaceResource's narrower bespoke fallback (now redundant) and routes SearchTools through the shared ReadableScope helper instead of duplicating the lookup inline. Tracked more broadly as #3078 (other @IfBuildProfile usages may share the gap). - OidcPluginAuthClient: remove exchangeCode()/TokenResponse (dead code, unused since the exchange happens browser-side by design) and OidcRoleResolver (dead code, never wired up). Cache the discovery document and reuse one HttpClient instead of rebuilding per request on every public, unauthenticated endpoint hit. Rename calm.github.http.* properties to calm.oidc.http.* (this class has nothing to do with the GitHub backend). Fix a CDI ordering bug from the rewrite: build the HttpClient in @PostConstruct, not a field initializer that ran before @ConfigProperty injection completed. - application.properties: make /api/calm/auth/config public under every auth profile, not just oidc -- the SPA now fetches it before it knows how to authenticate, so it 401'd (and blanked the page, see next point) under the existing secure/proxy-auth profiles. - application-oidc.properties: remove misplaced calm.github.* entries (belong to the github profile), add a server-side HTTPS-only guard (quarkus.http.insecure-requests=disabled, dev override), remove the dead OidcRoleResolver config entries. - index.tsx: catch bootstrap() rejection and render an error state instead of leaving a permanently blank page when auth-config fetch fails. - Remove orphaned LogoutButton component/test (superseded by UserMenu). - Fix a test mock in authService.test.tsx asserting a 'github' field that doesn't exist on the AuthConfig type. - Fix TestNamespaceResourceShould/TestSearchResourceShould fixtures that depended on UserAccessValidator's old build-time gap for unfiltered results; they now mock UserAccessValidator explicitly. Reviewed via /code-review high (two passes) and an independent security review. Two open PR review threads addressed: pendingSessions eviction (fixed, replied+resolved) and the ID-token-as-bearer-credential finding (tracked as #3077, narrow-scoped out of this slice, replied, left open). Filed #3078 for the broader @IfBuildProfile pattern. mvn clean verify (unit+integration, 540 tests) and calm-hub-ui vitest (1443 tests) both green; JaCoCo coverage gate met.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
- Validate the OIDC "nonce" request parameter against an allowlist regex before it reaches the authorize-redirect URL, closing a CodeQL java/unvalidated-url-redirection alert. The redirect's destination host was never attacker-controlled (it comes from the discovery document fetched against the configured, admin-set OIDC authority, and the value itself is URL-encoded before being appended as a query parameter), but bounding it to an opaque-token charset removes the question structurally and gives static analysis a validated value to see instead of a raw request parameter reaching a redirect Location. - Rename the Java-side identifiers carrying this value away from the bare word "nonce" (PendingSession/CallbackPageData field, method parameters, local variables) to "replayGuard" / "scriptToken" for the unrelated CSP nonce. Left untouched only where the literal word is an external contract this code can't rename: the inbound "?nonce=" query parameter name, the outbound "&nonce=" parameter sent to the IdP and echoed to the plugin, the CSP "nonce" directive/ attribute name, and the ID token's own "nonce" claim. - Harden getPendingSessions(): now returns an immutable Map.copyOf snapshot instead of the live map. Its only caller is the test suite, but handing out a mutable internal reference from a resource class is worth closing off regardless of who currently calls it. Added putPendingSessionForTest() as the one remaining test-only seam for planting PendingSession fixtures.
Same gap as TestNamespaceResourceShould/TestSearchResourceShould: @testsecurity(authorizationEnabled = false) bypasses declarative permission checks but not DomainResource's own ReadableScope lookup. Now that UserAccessValidator is unconditionally registered (this slice's fix for the @IfBuildProfile/runtime-profile gap, #3078), this test class was genuinely invoking it for real, hitting the live UserAccessStore for an identity-less test principal — passing when Mongo Dev Services happened to already be warm from earlier test classes in the same run, intermittently timing out (SocketTimeout) when it wasn't. Reproduced the CI failure locally by running the class in isolation. Adds the same @InjectMock UserAccessValidator + Optional.empty() stub already applied to the other two classes.
jpgough-ms
marked this pull request as ready for review
September 9, 2026 15:47
rocketstack-matt
previously approved these changes
Sep 9, 2026
- application-oidc.properties: correct the comment on public.paths — it
claimed the base application.properties entry for /api/calm/auth/config
"would be sufficient" on its own. It would not: Quarkus config sources
don't merge same-named properties across files, a profile-specific value
wholly replaces the base value for that key. This file's list is the
complete, authoritative definition under "oidc".
- application-oidc.properties: fix a startup blocker introduced by the
prior review round. quarkus.http.insecure-requests=disabled was set with
no paired quarkus.http.ssl.certificate.* config (unlike
application-secure.properties) — per Quarkus's own docs, that refuses to
even start outside %dev. Set to "enabled" instead: this profile has no
TLS-terminating mode of its own, deployments sit behind a
TLS-terminating proxy/ingress by design, so the process only ever sees
plain HTTP internally in every environment including production.
- PluginAuthResource: stop deriving the session cookie's Secure attribute
from calm.hub.base-url's scheme (.secure(hubBaseUrl.startsWith("https://"))).
Behind a TLS-terminating proxy the browser-facing connection can be HTTPS
while this process's own config string still reads http://, silently
omitting Secure. Replaced with a dedicated
calm.hub.plugin-auth.cookie-secure config property, true by default,
relaxed to false only under %dev.
- Add OidcPublicPathsIntegration (+ IntegrationTestOidcProfile), a
@QuarkusTest under the real "oidc" config profile proving
application-oidc.properties' public.paths value is actually the one in
effect — not just readable in isolation. Verified the test actually
discriminates: temporarily reverted the public.paths fix locally and
confirmed 3 of 5 assertions fail, then restored the fix.
- Extend the existing session-cookie unit test to assert Secure, and add a
test for the %dev relaxation.
Addresses PR review comments from @rocketstack-matt on #3065.
rocketstack-matt
approved these changes
Sep 9, 2026
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
4 participants
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.
Description
main— merge order matters, review it after Slice 3/5: feat(calm-hub): add a GitHub API response cache #3064/api/calm/auth/config) instead of build-time constantsPluginAuthResource+OidcPluginAuthClient)UserAccessStorenamespace grant, not only the existingUserAccessValidatorpath — needed for backends where that validator isn't resolvableauthService.tsxsends the OIDCid_tokenas the API bearer credential for all IdPs — an Entra-ID-specific workaround applied globallyPluginAuthResourceis a public, unauthenticated endpoint holding pending login sessions in an in-memoryConcurrentHashMap— no visible expiry/eviction; worth reviewing for session handling and unbounded growthType of Change
Affected Components
calm-hub/)calm-hub-ui/)Testing
Checklist
Split from #3001. Depends on #3064.