Repository navigation
feat: Login page SSO button - #144
Conversation
e556017 to
ded30f5
Compare
|
@gcgoncalves needs rebase from main |
ded30f5 to
2f46c00
Compare
marekdano
left a comment
There was a problem hiding this comment.
Findings
🟠 Medium — Stale SSO error not cleared from URL
File: src/pages/Login.tsx:27
The ?error=sso_* query param is read and displayed but never stripped from the URL after being consumed.
Failure scenario: A user's SSO attempt fails and they land on /app/login?error=sso_access_denied, see the "Single sign-on failed" banner, then refresh the page or navigate back/forward. The same stale error reappears even though nothing new failed, since no history.replaceState / param-clearing step removes it — unlike the codebase's existing pattern of clearing one-time state (e.g. router.tsx's replaceState usage).
🟡 Low — Error param read post-mount instead of synchronously
File: src/pages/Login.tsx:27
The SSO error is read via a post-mount useEffect (with react-hooks/exhaustive-deps disabled) instead of computed synchronously like the codebase's existing query-param pattern.
Failure scenario: PasswordChangeRequired.tsx reads its email query param synchronously via useMemo(() => ..., []). This effect instead defers the read to after the first paint, causing an extra render — the login form is briefly visible without the SSO-failure banner before it pops in — and requires an eslint-disable that a lazy useState/useMemo initializer would avoid entirely.
🟡 Low — Test leaks mutated window.history
File: src/pages/Login.test.tsx:279
The new SSO test mutates the real window.history via pushState, but the accompanying afterEach only calls vi.unstubAllGlobals(), never restoring the URL.
Failure scenario: If a future test is appended after this one in the same file (or Vitest ever runs with isolate: false), it inherits window.location.search = '?error=sso_access_denied' from this test, silently corrupting any assertion that reads window.location (e.g. another resolveNextParam/query-param test) with no cleanup to blame.
There was a problem hiding this comment.
LGTM! 🚀
Non-blocking LLM-assisted review feedback, in case it's useful:
--->
Local run is green, 30 tests across Login and AuthContext, with prettier and eslint clean. The error param never reaches the DOM, since it only selects a fixed message when it starts with sso_. resolveNextParam also holds up here: validateDestination in src/router/index.tsx:46-65 rejects protocol URLs, protocol-relative URLs and .., and requires an /app prefix, so the value forwarded to /auth/sso/login?next= can only be an app-relative path.
src/auth/AuthContext.tsx:147 duplicates the setSso call at line 144, which already sets the same state before the version guard. The values are identical, so the only cost is an extra render, but one of them should go. It looks like a leftover from the "address comments" commit.
src/pages/Login.tsx:41 sends the browser to /auth/sso/login, which does not exist yet. server/src/plugins/static.ts:69-79 returns a JSON 404 for unmatched /auth/* paths, so the SPA fallback does not catch it, and the user lands on a JSON error page outside the app with only the back button to return. Reaching that needs SSO_ENABLED=true, which server/src/config.ts:185-197 accepts today, after which /auth/session reports ssoEnabled: true and the button renders.
The flag is already settable on main, so this PR makes the gap visible rather than creating it, and holding the merge for #6915 seems heavier than it needs to be. A note in the SSO_ENABLED blocks at .env.example:76-78 and .env.prod.example:61-63 saying the flag is incomplete until #6915 and #6916 land would cover it.
src/pages/Login.test.tsx:274 only covers the default next=/app/, so nothing checks that an explicit ?next= reaches the SSO URL, which is one of the acceptance criteria on #6922. The existing stub supports it: {...window.location} does carry search under jsdom, so a case loading /app/login?next=%2Fapp%2Ftools would be short.
Minor: the description says the page fetches /auth/sso/status on mount. It reads ssoEnabled from /auth/session instead, per #6921 and server/src/routes/auth/session.ts:24-28.
c09f030 to
c7a6a80
Compare
Signed-off-by: Gabriel Costa <gabrielcg@proton.me>
Signed-off-by: Gabriel Costa <gabrielcg@proton.me>
c7a6a80 to
be4726c
Compare
Signed-off-by: Gabriel Costa <gabrielcg@proton.me>
Closes IBM/mcp-context-forge#6922
DEPENDS ON #143MERGEDAdds a
Sign in with Keycloakbutton to the login page, shown only when SSO is enabled, plus handling for a failed SSO round trip on return.Changes
src/pages/Login.tsx: fetches/auth/sso/statuson mount (best-effort, hidden on any failure); renders an outlineSign in with {provider}button below the password form when enabled, doing a full-page redirect (window.location.href) to/auth/sso/login?next=...(standard IdP-redirect login). Also checks?error=sso_*on mount and surfaces it via the existing alert UI. Password login is untouched.src/api/client.ts: added/auth/sso/statustoBFF_OWNED_PATHS(otherwise it'd get misrouted through/api/*to the upstream gateway).src/i18n/locales/{en-US,es-ES,pt-BR}/auth.json: new keys for the button label, the "or" divider, and the SSO-failure message.src/test/mocks/handlers.ts: added a global default MSW handler forGET */auth/sso/status(disabled by default) - needed since MSW here errors on unhandled requests, and every test rendering<Login>now triggers this fetch.src/pages/Login.test.tsx: 4 new cases (hidden by default, hidden on probe failure, shown + navigates when enabled, error renders without navigating further).Why
Part of the Keycloak SSO plan (Phase 3 — SPA UI). Lets users start an SSO login without needing to be authenticated first, reusing Task 3.1's status probe.