diff --git a/src/utils/auth-recovery.js b/src/utils/auth-recovery.js index 5ce20bf71..2602e564d 100644 --- a/src/utils/auth-recovery.js +++ b/src/utils/auth-recovery.js @@ -27,15 +27,15 @@ export const parseAuthChallenge = (challenge) => { export const isStepUpChallenge = (challenge) => parseAuthChallenge(challenge)?.error === STEP_UP_ERROR; -// A stalled silent renewal (e.g. a network black-hole) must not hang recovery for both -// transports, so it is time-boxed. Interactive step-up is deliberately NOT bounded — it -// waits on the user completing MFA, which can take far longer than any network timeout. +// Time-boxed so a stalled recovery can't hang forever. Step-up's longer bound only trips +// if the redirect never navigated (a real redirect unloads the page first). const SILENT_RENEW_TIMEOUT_MS = 30_000; +const STEP_UP_REDIRECT_TIMEOUT_MS = 60_000; -const withTimeout = async (promise, ms) => { +const withTimeout = async (promise, ms, label = 'recovery') => { let timeoutId; const timeout = new Promise((_, reject) => { - timeoutId = setTimeout(() => reject(new Error('auth-recovery: renewal timed out')), ms); + timeoutId = setTimeout(() => reject(new Error(`auth-recovery: ${label} timed out`)), ms); }); try { return await Promise.race([promise, timeout]); @@ -65,9 +65,11 @@ export const recoverAuth = ({ stepUp = false, challenge = null } = {}) => { throw new Error(`auth-recovery: no ${stepUp ? 'stepUp' : 'silentRenew'} primitive registered`); } - const accessToken = stepUp - ? await recover(challenge) - : await withTimeout(recover(challenge), SILENT_RENEW_TIMEOUT_MS); + const accessToken = await withTimeout( + recover(challenge), + stepUp ? STEP_UP_REDIRECT_TIMEOUT_MS : SILENT_RENEW_TIMEOUT_MS, + stepUp ? 'step-up redirect' : 'silent renewal', + ); if (!accessToken) { throw new Error('auth-recovery: renewal returned no token'); } diff --git a/src/utils/auth-recovery.test.js b/src/utils/auth-recovery.test.js index e6af8ca16..8ae5e9e0f 100644 --- a/src/utils/auth-recovery.test.js +++ b/src/utils/auth-recovery.test.js @@ -98,7 +98,7 @@ describe('auth-recovery', () => { const silentRenew = jest.fn(() => new Promise(() => {})); registerAuthRecovery({ silentRenew }); - const rejection = expect(recoverAuth()).rejects.toThrow(/timed out/); + const rejection = expect(recoverAuth()).rejects.toThrow(/silent renewal timed out/); await jest.advanceTimersByTimeAsync(30_000); await rejection; @@ -112,18 +112,22 @@ describe('auth-recovery', () => { } }); - test('does not time out an interactive step-up (it waits on the user)', async () => { + test('times out a step-up whose redirect never navigates, clearing in-flight so a later step-up retries', async () => { jest.useFakeTimers(); try { - const stepUp = jest.fn(() => new Promise((resolve) => { - setTimeout(() => resolve('stepped.token'), 90_000); // past the 30s silent-renewal timeout - })); + // A redirect that resolves but never navigates: pending forever. + const stepUp = jest.fn(() => new Promise(() => {})); registerAuthRecovery({ stepUp }); - const pending = recoverAuth({ stepUp: true }); + const rejection = expect(recoverAuth({ stepUp: true, challenge: {} })).rejects.toThrow(/step-up redirect timed out/); await jest.advanceTimersByTimeAsync(60_000); - await jest.advanceTimersByTimeAsync(30_000); - await expect(pending).resolves.toBe('stepped.token'); + await rejection; + + expect(store.dispatch).not.toHaveBeenCalled(); + + stepUp.mockImplementationOnce(() => Promise.resolve('stepped.token')); + await expect(recoverAuth({ stepUp: true, challenge: {} })).resolves.toBe('stepped.token'); + expect(stepUp).toHaveBeenCalledTimes(2); } finally { jest.useRealTimers(); } diff --git a/src/withSocketConnection/index.test.js b/src/withSocketConnection/index.test.js index 78cab51d8..24867d436 100644 --- a/src/withSocketConnection/index.test.js +++ b/src/withSocketConnection/index.test.js @@ -6,7 +6,11 @@ import { recoverAuth } from '../utils/auth-recovery'; import { clearAuth } from '../ducks/auth'; jest.mock('socket.io-client', () => ({ __esModule: true, default: jest.fn() })); -jest.mock('../utils/auth-recovery', () => ({ recoverAuth: jest.fn() })); +// Real parsers; only recoverAuth is stubbed. +jest.mock('../utils/auth-recovery', () => ({ + ...jest.requireActual('../utils/auth-recovery'), + recoverAuth: jest.fn(), +})); jest.mock('../ducks/auth', () => ({ clearAuth: jest.fn() })); // The socket handler dispatches through the imported store singleton; stub it so the // test controls dispatch and importing the hook doesn't build the real reducer tree. @@ -143,4 +147,66 @@ describe('websocket auth recovery (resp_authorization)', () => { expect(recoverAuth).toHaveBeenCalledTimes(2); expect(clearAuth).not.toHaveBeenCalled(); }); + + test('a 401 carrying the RFC 9470 step-up challenge routes to interactive step-up, not silent-renew', async () => { + // Step-up never settles (redirect); assert the routing decision, not a completion. + recoverAuth.mockReturnValue(new Promise(() => {})); + const challenge = 'Bearer error="insufficient_user_authentication", acr_values="http://schemas.openid.net/pape/policies/2007/06/multi-factor", max_age="31536000"'; + + const { socket, store } = bindSocket(); + await act(async () => { + socket.handlers.resp_authorization({ status: { code: 401, www_authenticate: challenge } }); + await Promise.resolve(); + }); + + expect(recoverAuth).toHaveBeenCalledWith({ + stepUp: true, + challenge: { + error: 'insufficient_user_authentication', + acrValues: 'http://schemas.openid.net/pape/policies/2007/06/multi-factor', + maxAge: '31536000', + }, + }); + expect(clearAuth).not.toHaveBeenCalled(); + expect(store.dispatch).not.toHaveBeenCalledWith(CLEAR_AUTH_ACTION); + }); + + test('a 401 with an ordinary (non-step-up) challenge still takes the silent-renew path', async () => { + recoverAuth.mockResolvedValue('fresh.token'); + + const { socket } = bindSocket(); + await act(async () => { + await socket.handlers.resp_authorization({ + status: { code: 401, www_authenticate: 'Bearer error="invalid_token"' }, + }); + }); + + expect(recoverAuth).toHaveBeenCalledWith(undefined); + expect(clearAuth).not.toHaveBeenCalled(); + }); + + test('escalates to step-up (not sign-out) when a silently-renewed socket then 401s with a step-up challenge', async () => { + const { socket, store } = bindSocket(); + + // Ordinary 401 → silent renew → re-authorize (sets the once-per-cycle guard). + recoverAuth.mockResolvedValueOnce('renewed.token'); + await act(async () => { + await socket.handlers.resp_authorization({ status: { code: 401 } }); + }); + + // The re-authorize 401s with a step-up challenge (guard already set); step-up bypasses it. + recoverAuth.mockReturnValue(new Promise(() => {})); + const challenge = 'Bearer error="insufficient_user_authentication", acr_values="urn:mfa", max_age="3600"'; + await act(async () => { + socket.handlers.resp_authorization({ status: { code: 401, www_authenticate: challenge } }); + await Promise.resolve(); + }); + + expect(recoverAuth).toHaveBeenLastCalledWith({ + stepUp: true, + challenge: { error: 'insufficient_user_authentication', acrValues: 'urn:mfa', maxAge: '3600' }, + }); + expect(clearAuth).not.toHaveBeenCalled(); + expect(store.dispatch).not.toHaveBeenCalledWith(CLEAR_AUTH_ACTION); + }); }); diff --git a/src/withSocketConnection/useRealTimeImplementation/index.js b/src/withSocketConnection/useRealTimeImplementation/index.js index 175649deb..e673064d7 100644 --- a/src/withSocketConnection/useRealTimeImplementation/index.js +++ b/src/withSocketConnection/useRealTimeImplementation/index.js @@ -4,7 +4,7 @@ import { DAS_HOST } from '../../constants'; import { resetSocketStateTracking } from './helpers'; import { SOCKET_HEALTHY_STATUS } from '../../ducks/system-status'; import { clearAuth } from '../../ducks/auth'; -import { recoverAuth } from '../../utils/auth-recovery'; +import { isStepUpChallenge, parseAuthChallenge, recoverAuth } from '../../utils/auth-recovery'; import { events } from './config'; import { calcEventFilterForRequest } from '../../utils/event-filter'; import { calcPatrolFilterForRequest } from '../../utils/patrol-filter'; @@ -73,10 +73,13 @@ const useRealTimeImplementation = () => { socket.on('resp_authorization', async (msg) => { const { status } = msg; if (status.code === 401) { - if (!authRetried) { + // Step-up bypasses the once-per-cycle guard: it redirects rather than looping. + const challenge = status.www_authenticate; + const stepUp = isStepUpChallenge(challenge); + if (stepUp || !authRetried) { authRetried = true; try { - await recoverAuth(); + await recoverAuth(stepUp ? { stepUp: true, challenge: parseAuthChallenge(challenge) } : undefined); console.log('realtime: token renewed; re-authorizing'); return authorize(); } catch (renewalError) {