fix(auth): chroot anon redirect to /login (PIN page), never KC hosted login (#1089, #1090 cluster A) (#1093)

SovereignConsoleLayout previously called initiateLogin() on the no-cookie
+ no-token path, which redirected the operator to Keycloak's hosted
login UI (auth.<sov>/realms/sovereign/protocol/openid-connect/auth).
That surface is forbidden by the routing matrix — operators must sign
in via the OpenOva 6-digit PIN page (/login). Issue #1089.

The fix:
  - SovereignConsoleLayout now redirects to `/login?next=<encoded-path>`
    via window.location.replace, both on the "no tokens" branch and on
    the "expired tokens + silentRefresh failure" branch.
  - Deep-link preservation: the original window.location.pathname +
    search are encoded into the `next` query param. After PIN verify,
    VerifyPinPage already routes to `next` (existing behaviour).
  - LoginPage URL-driven error banner now renders independently of the
    input state, so ?error=pin-expired / attempts-exceeded /
    flow_changed surface the matching banner copy on first paint.
    Closes the TC-R-033 + TC-R-061 UX regressions.
  - Removed initiateLogin import from SovereignConsoleLayout (last
    call site in the codebase; the function remains in oidc.ts for
    completeness but is no longer wired into any layout).

Tests:
  - Rewrote SovereignConsoleLayout.test.tsx: window.location.replace
    spy asserts redirect target = /login?next=<encoded>; assertion
    that initiateLoginSpy is NEVER called. Coverage for plain path,
    deep-linked path, path+search, expired-tokens fallback, and
    /whoami 5xx safety branch.
  - New LoginPage.test.tsx: ?error=* renders the correct banner copy;
    the deep-link `next` round-trips through PIN issue → /login/verify.

Routing matrix FAIL rows closed (26):
  TC-R-001, TC-R-002, TC-R-011, TC-R-012, TC-R-013, TC-R-014,
  TC-R-016, TC-R-017, TC-R-033, TC-R-049, TC-R-050, TC-R-051,
  TC-R-052, TC-R-053, TC-R-054, TC-R-055, TC-R-056, TC-R-057,
  TC-R-058, TC-R-059, TC-R-060, TC-R-061, TC-R-069, TC-R-070,
  TC-R-074, TC-R-075, TC-R-076, TC-R-091, TC-R-093.

Per docs/INVIOLABLE-PRINCIPLES.md #4: redirect target is built from
runtime window.location, never hardcoded.

Co-authored-by: Hati Yildiz <hati.yildiz@openova.io>
This commit is contained in:
e3mrah 2026-05-08 20:14:41 +04:00 committed by GitHub
parent daf2bbea4c
commit cb8c7892c6
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
4 changed files with 312 additions and 55 deletions

View File

@ -1,28 +1,34 @@
/**
* SovereignConsoleLayout.test.tsx Phase-8b followup
* SovereignConsoleLayout.test.tsx
*
* Regression coverage for the auth-guard order-of-operations bug caught
* live on otech49 + otech52 (2026-05-03):
* Regression coverage for two stacked auth-guard bugs:
*
* The wizard's handover button drops the operator at
* https://console.<sov>.omani.works/auth/handover?token=<jwt>
* and the catalyst-api 302-redirects to /console/dashboard with a
* `catalyst_session` HttpOnly Secure SameSite=Lax cookie attached.
* 1. Phase-8b followup (otech49 + otech52, 2026-05-03)
* The wizard's handover button drops the operator at
* https://console.<sov>.omani.works/auth/handover?token=<jwt>
* and the catalyst-api 302-redirects to /dashboard with a
* `catalyst_session` HttpOnly Secure SameSite=Lax cookie attached.
* PREVIOUS layout went straight from "no sessionStorage tokens" to
* Keycloak's hosted login. The fix probes /api/v1/whoami first.
*
* The PREVIOUS layout went straight from "no sessionStorage tokens"
* to `initiateLogin()` (PKCE redirect to Keycloak), so the operator
* landed on a username/password screen. Bug ticket from the field:
* "fuck, this is asking username password!!!"
* 2. Issue #1089 (chroot anon KC, 2026-05-08)
* On a chroot Sovereign with no cookie + no OIDC tokens, the
* layout USED TO call initiateLogin() which redirected to the
* Keycloak hosted login UI (`auth.<sov>/realms/sovereign/...`).
* The matrix forbids that surface. Operators must land on the
* OpenOva PIN-login page (`/login`) with the original deep-link
* preserved as `?next=<original-path>`. After PIN verify, the
* VerifyPinPage routes the operator back to `next`.
*
* The fix probes GET /api/v1/whoami (with credentials:'include')
* FIRST. If 200, the layout renders the console without ever
* redirecting to Keycloak. If 401, the existing OIDC flow runs.
*
* The two contracts under test:
* 1. /whoami 200 cookie-authenticated, console shell renders, NO
* navigation to auth.<sov>.../auth?... happens.
* 2. /whoami 401 falls back to OIDC initiateLogin (existing
* behaviour preserved sessionStorage tokens path also works).
* Contracts under test:
* A. /whoami 200 cookie-authenticated, console shell renders, NO
* navigation away.
* B. /whoami 401 + no sessionStorage tokens window.location.replace
* to '/login?next=<encoded-pathname>', NEVER initiateLogin().
* C. /whoami 5xx + no tokens falls through safely to the PIN-login
* bounce (NOT to KC).
* D. Deep-link preservation window.location.pathname is encoded
* verbatim into the `next` query param.
*/
import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'
@ -36,9 +42,8 @@ vi.mock('@/shared/lib/detectMode', () => ({
detectMode: () => ({ mode: 'sovereign' as const, sovereignFQDN: 'otech49.omani.works' }),
}))
// Stub the OIDC module — the test only needs to observe that
// initiateLogin is (or isn't) called. We can't let the real
// implementation run because it does a window.location.replace.
// Stub the OIDC module — initiateLogin must NEVER be called from this
// layout post-#1089. We export it as a spy so we can assert that.
const initiateLoginSpy = vi.fn<(fqdn: string) => Promise<void>>()
const initiateLogoutSpy = vi.fn<(fqdn: string) => void>()
const silentRefreshSpy = vi.fn<(fqdn: string) => Promise<unknown>>()
@ -85,6 +90,27 @@ vi.mock('@tanstack/react-router', async (importOriginal) => {
// Now safe to import the SUT.
import { SovereignConsoleLayout } from './SovereignConsoleLayout'
/**
* Stub `window.location` so we can observe redirect targets without
* jsdom navigating away. We replace `replace` with a spy and read the
* arg back; pathname is set per-test.
*/
function stubLocation(pathname: string, search = '') {
const replaceSpy = vi.fn<(url: string) => void>()
Object.defineProperty(window, 'location', {
value: {
...window.location,
pathname,
search,
replace: replaceSpy,
assign: vi.fn(),
href: `https://console.otech49.omani.works${pathname}${search}`,
},
writable: true,
})
return replaceSpy
}
beforeEach(() => {
initiateLoginSpy.mockClear()
initiateLogoutSpy.mockClear()
@ -100,6 +126,7 @@ afterEach(() => {
describe('SovereignConsoleLayout — auth-guard order of operations', () => {
it('renders the console shell when /whoami returns 200 (cookie-authenticated)', async () => {
stubLocation('/dashboard')
// Server-side handover minted a catalyst_session cookie, browser
// arrived here with it attached.
const fetchSpy = vi.spyOn(global, 'fetch').mockImplementation(async (input) => {
@ -133,11 +160,11 @@ describe('SovereignConsoleLayout — auth-guard order of operations', () => {
const init = whoamiCall![1] as RequestInit | undefined
expect(init?.credentials).toBe('include')
})
})
it('falls back to OIDC initiateLogin when /whoami returns 401 and no sessionStorage tokens', async () => {
// No cookie, no OIDC tokens — the only escape hatch is a fresh
// PKCE flow against Keycloak. This is the "returning user whose
// session expired" branch.
describe('SovereignConsoleLayout — #1089 anon redirect to OpenOva /login', () => {
it('on /whoami 401 + no sessionStorage tokens, redirects to /login?next=<path>, NOT to Keycloak', async () => {
const replaceSpy = stubLocation('/dashboard')
vi.spyOn(global, 'fetch').mockImplementation(async (input) => {
const url = typeof input === 'string' ? input : (input as URL).toString()
if (url.endsWith('/v1/whoami')) {
@ -149,24 +176,87 @@ describe('SovereignConsoleLayout — auth-guard order of operations', () => {
render(<SovereignConsoleLayout />)
// The auth-gate loading screen renders while initiateLogin runs.
await screen.findByTestId('sov-auth-loading')
// Wait for the OIDC fallback to fire — the spy is async-resolved
// inside useEffect → Promise chain.
// Wait for the redirect to fire (chained off the /whoami promise).
await waitFor(() => {
expect(initiateLoginSpy).toHaveBeenCalledWith('otech49.omani.works')
expect(replaceSpy).toHaveBeenCalled()
})
// The console shell never rendered.
expect(screen.queryByTestId('sov-console-shell')).toBeNull()
const target = replaceSpy.mock.calls[0]![0]
expect(target).toContain('/login?next=')
expect(target).toContain(encodeURIComponent('/dashboard'))
// The decisive assertion: Keycloak's hosted login was NEVER touched.
expect(initiateLoginSpy).not.toHaveBeenCalled()
})
it('does not redirect to Keycloak on a 5xx /whoami — falls through to the OIDC path safely', async () => {
// 5xx must NOT be confused with "no session". The fallback to OIDC
// is itself idempotent, so falling through is the safe behaviour;
// the relevant assertion is that we don't render the console
// either (no spurious "authenticated" state).
it('preserves a deep-linked path (/jobs/timeline) verbatim in the next param', async () => {
const replaceSpy = stubLocation('/jobs/timeline')
vi.spyOn(global, 'fetch').mockImplementation(async (input) => {
const url = typeof input === 'string' ? input : (input as URL).toString()
if (url.endsWith('/v1/whoami')) {
return new Response(JSON.stringify({ error: 'unauthenticated' }), { status: 401 })
}
return new Response(null, { status: 404 })
})
loadTokensSpy.mockReturnValue(null)
render(<SovereignConsoleLayout />)
await waitFor(() => {
expect(replaceSpy).toHaveBeenCalled()
})
const target = replaceSpy.mock.calls[0]![0]
expect(target).toBe('/login?next=' + encodeURIComponent('/jobs/timeline'))
expect(initiateLoginSpy).not.toHaveBeenCalled()
})
it('preserves a deep-linked path including search (/apps?filter=running) verbatim', async () => {
const replaceSpy = stubLocation('/apps', '?filter=running')
vi.spyOn(global, 'fetch').mockImplementation(async (input) => {
const url = typeof input === 'string' ? input : (input as URL).toString()
if (url.endsWith('/v1/whoami')) {
return new Response(JSON.stringify({ error: 'unauthenticated' }), { status: 401 })
}
return new Response(null, { status: 404 })
})
loadTokensSpy.mockReturnValue(null)
render(<SovereignConsoleLayout />)
await waitFor(() => {
expect(replaceSpy).toHaveBeenCalled()
})
const target = replaceSpy.mock.calls[0]![0]
expect(target).toBe('/login?next=' + encodeURIComponent('/apps?filter=running'))
})
it('on /whoami 401 + expired tokens + silentRefresh failure, also redirects to /login (NOT KC)', async () => {
const replaceSpy = stubLocation('/cloud')
vi.spyOn(global, 'fetch').mockImplementation(async (input) => {
const url = typeof input === 'string' ? input : (input as URL).toString()
if (url.endsWith('/v1/whoami')) {
return new Response(JSON.stringify({ error: 'unauthenticated' }), { status: 401 })
}
return new Response(null, { status: 404 })
})
// Have tokens, but they're expired AND silentRefresh returns null.
loadTokensSpy.mockReturnValue({ idToken: 'expired', accessToken: 'expired', expiresAt: 0 })
silentRefreshSpy.mockResolvedValue(null)
render(<SovereignConsoleLayout />)
await waitFor(() => {
expect(replaceSpy).toHaveBeenCalled()
})
const target = replaceSpy.mock.calls[0]![0]
expect(target).toBe('/login?next=' + encodeURIComponent('/cloud'))
expect(initiateLoginSpy).not.toHaveBeenCalled()
})
it('on /whoami 5xx, falls through to the PIN-login bounce (not KC)', async () => {
const replaceSpy = stubLocation('/dashboard')
vi.spyOn(global, 'fetch').mockImplementation(async (input) => {
const url = typeof input === 'string' ? input : (input as URL).toString()
if (url.endsWith('/v1/whoami')) {
@ -179,8 +269,10 @@ describe('SovereignConsoleLayout — auth-guard order of operations', () => {
render(<SovereignConsoleLayout />)
await waitFor(() => {
expect(initiateLoginSpy).toHaveBeenCalled()
expect(replaceSpy).toHaveBeenCalled()
})
expect(screen.queryByTestId('sov-console-shell')).toBeNull()
const target = replaceSpy.mock.calls[0]![0]
expect(target).toContain('/login?next=')
expect(initiateLoginSpy).not.toHaveBeenCalled()
})
})

View File

@ -8,10 +8,12 @@
* `catalyst_session` cookie minted by the server-side /auth/handover
* handler. If 200, the operator is authenticated via the cookie
* render the console without ever touching Keycloak.
* 2. If 401, fall back to the legacy OIDC flow:
* - read tokens from sessionStorage,
* - if missing/expired, attempt silentRefresh,
* - if that fails, initiateLogin (PKCE redirect to Keycloak).
* 2. If 401, fall back to the OpenOva PIN-login page (`/login`) with
* the original deep-link preserved as `?next=<path+search>`. We do
* NOT redirect to Keycloak's hosted login operators sign in via
* the OpenOva 6-digit PIN flow (issue #688 + #1089). Keycloak
* remains the IdP behind catalyst-api but its hosted UI must never
* surface to the operator.
* 3. After login, if the id_token contains required actions
* (UPDATE_PASSWORD / configure-passkey / CONFIGURE_TOTP), shows the
* RequiredActionsModal before rendering the console.
@ -24,7 +26,7 @@
* `catalyst_session` HttpOnly Secure SameSite=Lax cookie for, and 302s to
* /console/dashboard. The browser arrives with a fresh cookie but no
* sessionStorage tokens the layout MUST recognise that cookie before
* bouncing to Keycloak's hosted login (issue: live regression on
* sending the operator to /login (issue: live regression on
* otech49 + otech52 today, operator landed on a username/password
* screen instead of the dashboard).
*
@ -42,7 +44,8 @@
* runtime state, never inlined.
*
* Related: GitHub issue #607 (initial OIDC gate),
* Phase-8b followup (session-cookie precedence).
* Phase-8b followup (session-cookie precedence),
* issue #1089 (chroot anon must redirect to /login, never KC).
*/
import { useEffect, useState } from 'react'
@ -54,12 +57,40 @@ import {
loadTokens,
isTokenExpired,
silentRefresh,
initiateLogin,
getRequiredActions,
} from '@/shared/lib/oidc'
import type { TokenSet } from '@/shared/lib/oidc'
import { RequiredActionsModal } from '@/components/RequiredActionsModal'
/**
* Build the basepath-aware URL prefix so the redirect target works both
* on chroot Sovereigns (served at domain root) and on the mothership
* (served at /sovereign/* with a strip-prefix middleware that the
* browser URL still reflects).
*/
function uiBase(): string {
if (typeof window === 'undefined') return ''
return window.location.pathname.startsWith('/sovereign') ? '/sovereign' : ''
}
/**
* Redirect to the OpenOva PIN-login page, preserving the original
* deep-link as `?next=<path+search>` so post-verify the operator lands
* back on the page they tried to visit. Hard navigation (location.replace)
* is used so any stale OIDC sessionStorage state is naturally discarded
* by the next page load.
*
* Per #1089: NEVER redirect to Keycloak's hosted login UI here. The
* IdP-hosted flow is for catalyst-api server-side use only; the
* operator-facing surface is the OpenOva PIN page.
*/
function redirectToLogin(): void {
if (typeof window === 'undefined') return
const next = window.location.pathname + window.location.search
const target = `${uiBase()}/login?next=${encodeURIComponent(next)}`
window.location.replace(target)
}
/**
* Shape returned by GET /api/v1/whoami when the catalyst_session cookie
* is present and valid (HMAC-verified server-side by the session
@ -161,14 +192,17 @@ export function SovereignConsoleLayout() {
return
}
// Legacy OIDC fallback — for operators who arrived via direct
// navigation to /console/* without going through the wizard
// handover (e.g. a returning user whose cookie expired). The PKCE
// flow remains the canonical re-auth path.
// No session cookie — try existing OIDC tokens (set by the legacy
// PKCE flow on otech<=46 builds before #688), refreshing silently
// if expired. If there's no usable token AND silentRefresh fails,
// bounce to the PIN-login page with deep-link preservation
// (#1089). The previous behaviour here was to call initiateLogin()
// which redirected to Keycloak's hosted login — that surface is
// forbidden for operator-facing flows.
const existing = loadTokens()
if (!existing) {
setAuthState({ status: 'unauthenticated' })
await initiateLogin(sovereignFQDN)
redirectToLogin()
return
}
@ -176,7 +210,7 @@ export function SovereignConsoleLayout() {
const refreshed = await silentRefresh(sovereignFQDN)
if (!refreshed) {
setAuthState({ status: 'unauthenticated' })
await initiateLogin(sovereignFQDN)
redirectToLogin()
return
}
const actions = getRequiredActions(refreshed.idToken)
@ -189,14 +223,15 @@ export function SovereignConsoleLayout() {
}
void initAuth()
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [sovereignFQDN])
function handleRequiredActionsComplete(tokens: TokenSet) {
setAuthState({ status: 'authenticated', tokens, requiredActions: [] })
}
// Loading — blank page while we check sessionStorage + maybe redirect to KC
// Loading — blank page while we probe /whoami and (if needed)
// redirect to /login. The `unauthenticated` branch keeps the same
// shell while window.location.replace executes.
if (authState.status === 'loading' || authState.status === 'unauthenticated') {
return (
<div

View File

@ -0,0 +1,112 @@
/**
* LoginPage.test.tsx coverage for the URL-driven error banner and
* deep-link `next` propagation (issue #1089 cluster).
*
* The matrix asserts the operator sees a literal banner copy whenever
* they land on /login with a known error code:
* - ?error=pin-expired "code expired"
* - ?error=attempts-exceeded "wrong attempts"
* - ?error=flow_changed "flow has changed"
*
* The banner must be visible REGARDLESS of input state (the form
* starts in 'idle' on a fresh mount). Previously the banner was wired
* into the Input's inline error which only renders when state ===
* 'error', so the URL-driven copy never surfaced (TC-R-033, TC-R-061).
*
* Also asserts that the `next` search param is preserved end-to-end so
* deep-linked entries (#1089) round-trip through PIN verify back to the
* original page.
*/
import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'
import { render, screen, cleanup, fireEvent, waitFor } from '@testing-library/react'
const navigateSpy = vi.fn<(opts: unknown) => void>()
const searchState: { current: Record<string, string | undefined> } = { current: {} }
vi.mock('@tanstack/react-router', async (importOriginal) => {
const actual = (await importOriginal()) as object
return {
...actual,
useNavigate: () => navigateSpy,
useSearch: () => searchState.current,
}
})
vi.mock('@/app/layouts/AuthLayout', () => ({
AuthShell: ({ children }: { children: React.ReactNode }) => <div>{children}</div>,
}))
import { LoginPage } from './LoginPage'
beforeEach(() => {
navigateSpy.mockClear()
searchState.current = {}
})
afterEach(() => {
cleanup()
vi.restoreAllMocks()
})
describe('LoginPage — URL-driven error banner', () => {
it('renders the "code expired" banner on ?error=pin-expired (#1089 / TC-R-061)', () => {
searchState.current = { error: 'pin-expired' }
render(<LoginPage />)
const banner = screen.getByTestId('login-error-banner')
expect(banner.textContent).toMatch(/code expired/i)
})
it('renders the "wrong attempts" banner on ?error=attempts-exceeded (TC-R-060)', () => {
searchState.current = { error: 'attempts-exceeded' }
render(<LoginPage />)
const banner = screen.getByTestId('login-error-banner')
expect(banner.textContent).toMatch(/wrong attempts/i)
})
it('renders the "flow has changed" banner on ?error=flow_changed (TC-R-033)', () => {
searchState.current = { error: 'flow_changed' }
render(<LoginPage />)
const banner = screen.getByTestId('login-error-banner')
expect(banner.textContent).toMatch(/flow has changed/i)
})
it('does not render the banner when ?error is absent', () => {
render(<LoginPage />)
expect(screen.queryByTestId('login-error-banner')).toBeNull()
})
})
describe('LoginPage — deep-link `next` propagation (#1089)', () => {
it('forwards a deep-linked `next` param into /login/verify after PIN issue', async () => {
searchState.current = { next: '/jobs/timeline' }
const fetchSpy = vi.spyOn(global, 'fetch').mockResolvedValue(
new Response(JSON.stringify({ ok: true, requestId: 'req-abc' }), {
status: 200,
headers: { 'Content-Type': 'application/json' },
}),
)
render(<LoginPage />)
const email = screen.getByTestId('login-email') as HTMLInputElement
fireEvent.change(email, { target: { value: 'sarah@omantel.om' } })
// The form's onSubmit handler does the work; clicking the submit
// button issues a 'submit' event on the surrounding <form>.
const form = email.closest('form')!
fireEvent.submit(form)
await waitFor(() => {
expect(navigateSpy).toHaveBeenCalled()
})
const navArg = navigateSpy.mock.calls[0]![0] as { to: string; search: Record<string, string> }
expect(navArg.to).toBe('/login/verify')
expect(navArg.search.email).toBe('sarah@omantel.om')
expect(navArg.search.requestId).toBe('req-abc')
expect(navArg.search.next).toBe('/jobs/timeline')
expect(fetchSpy).toHaveBeenCalled()
})
})

View File

@ -26,6 +26,10 @@ export function LoginPage() {
const [email, setEmail] = useState('')
const [state, setState] = useState<State>('idle')
// URL-driven error banner copy. Each branch contains the literal token
// the routing matrix asserts on (TC-R-033 'flow has changed', TC-R-061
// 'code expired', TC-R-060 'wrong attempts'). Keep these phrases
// verbatim — they are the user-visible contract.
const [errorMsg, setErrorMsg] = useState<string>(
initialError === 'pin-expired'
? 'Your sign-in code expired. Request a new one.'
@ -101,6 +105,20 @@ export function LoginPage() {
</p>
</div>
{/* URL-driven error banner. Surfaces independent of input state
so the operator sees context for why they were bounced here
(e.g. ?error=pin-expired, ?error=flow_changed). The Input's
inline error continues to serve form-submit failures. */}
{errorMsg && state !== 'error' && (
<div
role="alert"
data-testid="login-error-banner"
className="rounded-md border border-[--color-error]/30 bg-[--color-error]/10 px-4 py-3 text-sm text-[--color-error]"
>
{errorMsg}
</div>
)}
<form onSubmit={onSubmit} className="flex flex-col gap-4" noValidate>
<Input
label="Email"