Skip to content

feat: Login page SSO button - #144

Merged
gcgoncalves merged 3 commits into
mainfrom
6922-sso-login-button
Sep 23, 2026
Merged

gcgoncalves merged 3 commits into
mainfrom
6922-sso-login-button

Conversation

@gcgoncalves

@gcgoncalves gcgoncalves commented Sep 21, 2026 •

Copy link
Copy Markdown
Contributor

Closes IBM/mcp-context-forge#6922

DEPENDS ON #143 MERGED

Adds a Sign in with Keycloak button 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/status on mount (best-effort, hidden on any failure); renders an outline Sign 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/status to BFF_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 for GET */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.

@gcgoncalves
gcgoncalves force-pushed the 6922-sso-login-button branch 2 times, most recently from e556017 to ded30f5 Compare September 22, 2026 09:30
@vishu-bh

Copy link
Copy Markdown
Contributor

@gcgoncalves needs rebase from main

@marekdano marekdano left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@marekdano marekdano left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR looks good to me now!

We should review it again after merging #143

@a-effort a-effort left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Signed-off-by: Gabriel Costa <gabrielcg@proton.me>
Signed-off-by: Gabriel Costa <gabrielcg@proton.me>
Signed-off-by: Gabriel Costa <gabrielcg@proton.me>
@gcgoncalves
gcgoncalves merged commit 5d05bbc into main Sep 23, 2026
5 checks passed
@gcgoncalves
gcgoncalves deleted the 6922-sso-login-button branch September 23, 2026 10:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Login page SSO button + error handling

4 participants