feat(portal,chat-frontend): SSO front-door with static per-site config#395
Closed
vjauhari-work wants to merge 1 commit into
Closed
feat(portal,chat-frontend): SSO front-door with static per-site config#395vjauhari-work wants to merge 1 commit into
vjauhari-work wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (31)
💤 Files with no reviewable changes (2)
✅ Files skipped from review due to trivial changes (5)
🚧 Files skipped from review as they are similar to previous changes (19)
📝 WalkthroughWalkthroughThe PR adds portal-service OIDC login and callback handling, trims directory and user-info payloads to base URLs and IDs, registers a new Keycloak ChangesPortal SSO front-door rollout
Sequence Diagram(s)Portal-service login callback flowsequenceDiagram
participant Browser
participant portal_service as portal-service
participant Keycloak
Browser->>portal_service: GET /login
portal_service-->>Browser: redirect to Keycloak authorize URL
Browser->>Keycloak: authorize request
Keycloak-->>Browser: redirect with code and state
Browser->>portal_service: GET /auth/callback?code&state
portal_service->>Keycloak: ExchangeAndVerify(code, nonce)
portal_service->>portal_service: resolveSite(account)
portal_service-->>Browser: redirect to resolved BaseURL
chat-frontend connect flowsequenceDiagram
participant Browser
participant chat_frontend as chat-frontend
participant auth_service as auth-service
participant NATS
Browser->>chat_frontend: load app
chat_frontend->>auth_service: POST /auth
auth_service-->>chat_frontend: JWT and user payload
chat_frontend->>NATS: connect(NATS_URL, SITE_ID, JWT)
NATS-->>chat_frontend: connected
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@chat-frontend/src/context/NatsContext/NatsContext.jsx`:
- Around line 77-104: The connect flow in connectToNats needs a stale-request
guard before any state writes, not just in nc.closed(). After await
authResp.json() and again immediately after natsConnect, compare myGen to
connectGenRef.current and bail out if they differ so an older connect() cannot
overwrite setCredentials, ncRef.current, setUser, or setConnected after a newer
attempt has already started. Use connectToNats and connectGenRef as the key
points to place the guard.
In `@chat-frontend/src/lib/runtimeConfig.js`:
- Around line 6-13: The runtime config in runtimeConfig.js is still falling back
to localhost values, which should not happen outside development. Update the
AUTH_URL, NATS_URL, and SITE_ID resolution to use dev-only defaults and fail
fast when runtime config is missing in production, so missing or broken
config.js does not silently connect to localhost. Use the existing
runtimeConfig.js exports and the related deploy-time guard behavior to gate the
fallbacks by environment and surface a startup error when production values are
absent.
In `@docker-local/setup.sh`:
- Around line 117-118: The frontend env bootstrap in setup.sh currently uses
VITE_AUTH_URL as the only gate but appends defaults for VITE_AUTH_URL,
VITE_NATS_URL, and VITE_SITE_ID together, which can duplicate and overwrite
existing custom values. Update the logic around the FRONTEND_ENV_FILE checks so
each key is tested independently and only the missing key is appended. Keep the
fix localized to the env initialization block that writes the VITE_* variables.
In `@docs/superpowers/plans/2026-06-24-portal-sso-front-door.md`:
- Around line 820-825: The writeHTMLError example currently concatenates msg
directly into the HTML response, which leaves an XSS sink in the plan. Update
the example so the writeHTMLError helper escapes msg before embedding it in the
page, matching the safer approach already used in portal-service/handler.go via
template.HTMLEscapeString(msg). Keep the helper name and browser-facing error
page behavior the same, but ensure the rendered body cannot interpret raw HTML
from msg.
- Around line 126-145: `newOIDCLogin` only applies `oidc.ClientContext` during
discovery, so the custom HTTP client is not used by the later token exchange and
ID token verification flow in `oidcLogin.ExchangeAndVerify`. Store the provided
`httpClient` on `oidcLogin` and re-bind the context before calling
`o.oauth.Exchange` and `o.verifier.Verify`, or otherwise route both steps
through the same client-aware OIDC context so the custom transport/TLS settings
are preserved end to end.
In `@docs/superpowers/specs/2026-06-24-portal-sso-front-door-design.md`:
- Around line 203-205: Update the testing note in the design doc to match the
current suite: the statement that newOIDCLogin stays uncovered is stale because
portal-service/oidclogin_test.go now includes TestNewOIDCLogin. Adjust the
wording around oidclogin_test.go and newOIDCLogin so the coverage description
reflects the actual tests currently present.
In `@pkg/oidc/oidc.go`:
- Around line 65-68: The shared OIDC client helper is using a
certificate-validation bypass via InsecureSkipVerify, which should not live in
pkg/oidc or be suppressed with G402. Update the client creation logic in the
OIDC helper to use normal TLS verification by default, and move any
TLS_SKIP_VERIFY behavior into non-production-only local wiring or a separate
opt-in path that is clearly isolated from shared discovery/JWKS fetching. Keep
the http.Client and issuer discovery setup intact, but remove the insecure
transport from the reusable helper and avoid the `#nosec` suppression unless it is
a genuine false positive.
In `@portal-service/handler.go`:
- Around line 199-211: Short-circuit PortalHandler.HandleAuthCallback when auth
is unavailable in dev mode so the nil h.auth call is never reached. Add an early
guard before ExchangeAndVerify that detects a nil/unset auth client and returns
a deterministic redirect or not-found response instead of proceeding with the
callback flow. Keep the existing handshake/state checks intact, and make sure
the fallback happens in HandleAuthCallback before any h.auth usage.
In `@portal-service/integration_test.go`:
- Around line 53-56: The integration test fixtures still seed the legacy natsUrl
field even though the assertions in integration_test.go now expect the trimmed
employee schema. Update the hr_employee seed data used by the test so it matches
the new contract by removing natsUrl from the fixture rows, and keep the
existing assertions against byAccount and employee values aligned with the
updated data shape.
In `@portal-service/oidclogin.go`:
- Around line 23-26: The OIDC login flow is dropping the configured HTTP client
after discovery, so the callback path uses the default transport instead of the
intended client. Update oidcLogin and the newOIDCLogin/ExchangeAndVerify flow to
retain and reuse the same httpClient for both the OAuth code exchange and the
verifier/JWKS fetches, and make sure the verifier is created with
VerifierContext so all OIDC requests use the configured client consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9d1d8450-f94e-4ab8-800e-53da2fe1d31c
📒 Files selected for processing (31)
auth-service/deploy/keycloak/realm-export.jsonchat-frontend/deploy/30-render-config.shchat-frontend/deploy/config.js.templatechat-frontend/deploy/docker-compose.ymlchat-frontend/src/context/NatsContext/NatsContext.jsxchat-frontend/src/context/NatsContext/NatsContext.test.jsxchat-frontend/src/context/NatsContext/useJwtRefresh.jschat-frontend/src/context/NatsContext/useJwtRefresh.test.jschat-frontend/src/lib/runtimeConfig.jschat-frontend/src/lib/runtimeConfig.test.jschat-frontend/src/pages/LoginPage/LoginPage.jsxdocker-local/setup.shdocs/client-api.mddocs/superpowers/plans/2026-06-24-portal-sso-front-door.mddocs/superpowers/specs/2026-06-24-portal-sso-front-door-design.mdgo.modpkg/oidc/oidc.gopkg/oidc/oidc_test.goportal-service/cache_test.goportal-service/deploy/docker-compose.ymlportal-service/handler.goportal-service/handler_test.goportal-service/integration_test.goportal-service/main.goportal-service/main_test.goportal-service/mock_oidclogin_test.goportal-service/oidclogin.goportal-service/oidclogin_test.goportal-service/routes.goportal-service/store.goportal-service/store_mongo.go
💤 Files with no reviewable changes (2)
- portal-service/store_mongo.go
- portal-service/cache_test.go
…onfig
portal-service gains a confidential-OIDC SSO front-door: GET /login signs the
user in at Keycloak; GET /auth/callback verifies the code (state CSRF guard +
explicit nonce check), resolves the account's home-site baseUrl from the
in-process directory, and 302-redirects the browser there. /api/userInfo is
trimmed to {account, employeeId, siteId, baseUrl} and is no longer on the
connect path (retained as a directory endpoint).
chat-frontend reverts to static per-site config (AUTH_URL/NATS_URL/SITE_ID),
drops the portal lookup, and silently re-auths after the redirect.
Supporting changes:
- pkg/oidc: shared HTTPClient + DiscoverProvider helpers (also used by
auth-service and upload-service).
- Keycloak: confidential `portal` client in the chatapp realm.
- docs/client-api.md: two-step connect narrative + trimmed /api/userInfo.
Review fixes incorporated:
- oidclogin: carry the configured httpClient through ExchangeAndVerify so the
token exchange and JWKS fetch use its TLS/transport, not just discovery.
- portal handler: short-circuit /auth/callback in dev mode (nil-auth guard);
nosemgrep the false-positive cookie-missing-secure on the handshake cookies.
- NatsContext: guard stale connect() completions before writing state.
- docker-local/setup.sh: append only individually-missing VITE_ keys.
- portal integration test: drop dead natsUrl seed field.
vjauhari-work
force-pushed
the
claude/gallant-darwin-k0sc1h
branch
from
31b3098 to
1619e4c
Compare
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
2 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.
Summary
Implements a confidential OIDC SSO front-door in
portal-serviceand revertschat-frontendto static per-site configuration. Users now sign in once at portal-service, which redirects them to their home-site chat-frontend already authenticated via Keycloak's SSO cookie. The chat-frontend no longer calls/api/userInfoand instead uses staticAUTH_URL,NATS_URL, andSITE_IDenvironment variables.Key Changes
portal-service (Go):
oidclogin.gowithloginAuthenticatorinterface,AuthCodeURL, andExchangeAndVerifymethods for confidential OIDC web-app flow/loginand/auth/callbackendpoints with state/nonce handshake cookie helpers/api/userInforesponse to 4 fields (account,employeeId,siteId,baseUrl); removedauthServiceUrlandnatsUrlresolveSitehelper to map account → employee + site URLs using the in-process directory cacheOIDC_ISSUER_URL,OIDC_CLIENT_ID,OIDC_CLIENT_SECRET,OIDC_REDIRECT_URL,OIDC_SCOPES) with conditional prod validationNATSURLfield fromemployeestruct and MongoDB projectionNewPortalHandlersignature to acceptloginAuthenticatorandcookieSecureparameterschat-frontend (JS):
PORTAL_URLwith staticAUTH_URL,NATS_URL, andSITE_IDinruntimeConfig.jsNatsContext.jsxto use staticAUTH_URLinstead of resolving it from portal/api/userInfocalluser.siteIdnow comes fromSITE_IDconfigKeycloak:
portalclient to the chatapp realm with appropriate redirect URIs and scopesDocumentation:
docs/superpowers/specs/2026-06-24-portal-sso-front-door-design.md)docs/superpowers/plans/2026-06-24-portal-sso-front-door.md)docs/client-api.mdto reflect the new login flowImplementation Details
/), which triggerssigninRedirect()that finds the Keycloak SSO cookie and re-authenticates without user interactionExchangeAndVerifysince go-oidc does not validate nonce automaticallyHttpOnly,Secure,SameSite=Laxwith configurablePORTAL_COOKIE_SECUREflaghttps://claude.ai/code/session_01WkCRxoR82h2iLS3EXGJ5c3
Summary by CodeRabbit