Skip to content

Commit 1775393

Browse files
committed
fix(setup/mfa): auto-skip MFA setup when no factors are offerable
1 parent 9952809 commit 1775393

4 files changed

Lines changed: 177 additions & 4 deletions

File tree

app/modules/auth/providers/fake/fake-provider.ts

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -88,6 +88,13 @@ interface Seed {
8888
* character classes — the legacy behaviour the forms assumed before the policy was wired in.
8989
*/
9090
passwordComplexity?: PasswordComplexity;
91+
92+
/**
93+
* Override the provider's static capability flags. Partial — only provided keys are overridden;
94+
* unspecified keys keep their class defaults. Use in tests to simulate an instance that offers
95+
* no MFA enrollment methods (e.g. all MFA caps false → resolveMfaSetup auto-skip path).
96+
*/
97+
capabilities?: Partial<ProviderCapabilities>;
9198
}
9299

93100
export class FakeAuthProvider implements AuthProvider {
@@ -159,6 +166,10 @@ export class FakeAuthProvider implements AuthProvider {
159166
this.deviceAuthSeeds = seed.deviceAuths ?? []; // P6
160167
this.samlRequestSeeds = seed.samlRequests ?? []; // P6
161168
this.ldapUserSeeds = seed.ldapUsers ?? []; // P6
169+
// Apply any capability overrides from the seed (partial merge over class defaults).
170+
if (seed.capabilities) {
171+
Object.assign(this.capabilities, seed.capabilities);
172+
}
162173
this.passwordComplexity = seed.passwordComplexity ?? {
163174
minLength: 8,
164175
requiresUppercase: false,

app/resources/mfa/mfa.service.ts

Lines changed: 43 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -102,8 +102,7 @@ export async function resolveMfaPicker(
102102
export type ChooseMfaMethodError = 'SESSION_EXPIRED' | 'INVALID_INPUT';
103103

104104
export type ChooseMfaMethodResult =
105-
| { ok: true; target: string }
106-
| { ok: false; error: ChooseMfaMethodError };
105+
{ ok: true; target: string } | { ok: false; error: ChooseMfaMethodError };
107106

108107
/**
109108
* Parse + route a 2nd-factor-method choice for an already-read sessions list.
@@ -257,6 +256,16 @@ export type MfaSetupResult = MfaSetupRedirect | MfaSetupData;
257256
* the raw policy arrays. (Keys, not route objects: the route labels are non-serializable
258257
* JSX.)
259258
*
259+
* AUTO-SKIP when no MFA methods are offerable: if the combined capability+policy gate
260+
* yields an empty set, there is nothing for the user to enroll in. Rendering an empty
261+
* chooser (and hiding the Skip button under `force`) would leave the user stuck. Instead,
262+
* auto-skip by stamping the skip timestamp (setMfaInitSkipped — same as the manual skip
263+
* path) and redirecting to the next step. Stamping is required: without it, nextStep
264+
* would see mfaInitSkippedAt=null and re-route back to /setup/mfa, creating a redirect
265+
* loop. This mirrors recordMfaSetupSkip exactly, including the FORCE+empty case — when
266+
* no factor is available, forced MFA is impossible to satisfy, so auto-skipping is the
267+
* only correct (non-stuck) outcome.
268+
*
260269
* The CSRF token + Set-Cookie header are the route's concern (only emitted on the setup
261270
* branch), so they are NOT part of this result.
262271
*/
@@ -276,6 +285,37 @@ export async function resolveMfaSetup(
276285
const settings = await provider.getLoginSettings(await resolveOrg(provider, organization));
277286
const offerableKeys = offerableSetupRoutes(capabilities, settings);
278287

288+
// AUTO-SKIP: no offerable MFA methods → stamp the skip and redirect to next step.
289+
// Mirrors recordMfaSetupSkip's stamping + next-step logic so the user lands on exactly
290+
// the same screen as a manual skip. Stamping suppresses the /setup/mfa nudge on the
291+
// next nextStep call (otherwise mfaInitSkippedAt=null would re-route back here).
292+
if (offerableKeys.length === 0) {
293+
logAuthEvent('mfa_skip', 'success', { userId: user.id, reason: 'no_offerable_methods' });
294+
await provider.setMfaInitSkipped(user.id);
295+
296+
// Re-fetch so mfaInitSkippedAt is fresh (mirrors recordMfaSetupSkip's re-fetch).
297+
const refreshedUser = await provider.findUser(loginName, organization);
298+
299+
const session = await provider.getSession(entry.id, entry.token);
300+
if (!session) return { kind: 'redirect', target: '/login' };
301+
302+
const [methods, refreshedSettings] = await Promise.all([
303+
provider.listAuthMethods(user.id),
304+
provider.getLoginSettings(await resolveOrg(provider, organization)),
305+
]);
306+
307+
const target = nextStepFromSession({
308+
session,
309+
methods,
310+
settings: refreshedSettings,
311+
loginName: session.user?.loginName ?? loginName,
312+
mfaInitSkippedAt: refreshedUser?.mfaInitSkippedAt,
313+
organization,
314+
});
315+
316+
return { kind: 'redirect', target };
317+
}
318+
279319
return { kind: 'setup', offerableKeys };
280320
}
281321

@@ -290,8 +330,7 @@ const skipFormSchema = z.object({
290330
export type RecordMfaSetupSkipError = 'INVALID_INPUT' | 'SESSION_EXPIRED';
291331

292332
export type RecordMfaSetupSkipResult =
293-
| { ok: true; target: string }
294-
| { ok: false; error: RecordMfaSetupSkipError };
333+
{ ok: true; target: string } | { ok: false; error: RecordMfaSetupSkipError };
295334

296335
/**
297336
* Parse + perform an MFA-setup skip for an already-read sessions list.

cypress/component/resources/mfa/mfa.service.cy.ts

Lines changed: 116 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,8 @@
22
//
33
// cy.task node-spec port of the SESSION-BOUND mfa service tests:
44
// - choose-mfa-method.service.test.ts (chooseMfaMethod findUser-failure audit)
5+
// - resolveMfaSetup auto-skip: when no MFA methods are offerable, the loader must
6+
// redirect instead of rendering an empty chooser.
57
//
68
// These read an already-read SessionEntry[] and emit REAL audit via logAuthEvent (→ console.log,
79
// captured by the harness). The browser bundle stubs observability to a no-op, so they must run
@@ -53,3 +55,117 @@ describe('chooseMfaMethod — findUser failure audit (security: routing continue
5355
});
5456
});
5557
});
58+
59+
// ── resolveMfaSetup: auto-skip when no MFA methods are offerable ────────────────────────────────
60+
//
61+
// Root cause (fixed): when offerableSetupRoutes returns [], resolveMfaSetup used to return
62+
// { kind: 'setup', offerableKeys: [] } — rendering an empty chooser. Under force=true, the
63+
// Skip button is hidden, leaving the user stuck.
64+
//
65+
// Fix: an empty offerable set → stamp the skip (setMfaInitSkipped, same as the manual skip path)
66+
// and return { kind: 'redirect', target } so the loader auto-advances. Stamping is required to
67+
// prevent a re-prompt loop (nextStep re-routes to /setup/mfa when mfaInitSkippedAt is null).
68+
//
69+
// The fresh provider seed uses capabilities: { passkey: false, totpOtp: false, emailOtp: false,
70+
// u2f: false, smsOtp: false } so offerableSetupRoutes returns [] regardless of policy. The
71+
// live session is seeded via `liveSessions` so getSession(entry.id, entry.token) resolves
72+
// (required by the auto-skip nextStepFromSession call inside resolveMfaSetup).
73+
74+
describe('resolveMfaSetup — auto-skip when no MFA methods are offerable', () => {
75+
// A fresh provider with all MFA capabilities disabled and one password-only user.
76+
const NO_MFA_CAPS = {
77+
passkey: false,
78+
u2f: false,
79+
totpOtp: false,
80+
emailOtp: false,
81+
smsOtp: false,
82+
externalIdp: false,
83+
ldap: false,
84+
saml: false,
85+
oidc: false,
86+
registration: false,
87+
};
88+
89+
const baseScenario = {
90+
fn: 'resolveMfaSetup' as const,
91+
provider: 'fresh' as const,
92+
seed: {
93+
users: [{ id: 'u-nomfa', loginName: 'nomfa@test.example' }],
94+
authMethods: { 'u-nomfa': ['password'] as string[] },
95+
capabilities: NO_MFA_CAPS,
96+
},
97+
liveSessions: [
98+
{
99+
id: 'sess-nomfa',
100+
token: 'tok-nomfa',
101+
user: { id: 'u-nomfa', loginName: 'nomfa@test.example' },
102+
},
103+
],
104+
request: {
105+
url: 'http://localhost/id/setup/mfa',
106+
sessions: [{ id: 'sess-nomfa', token: 'tok-nomfa', loginName: 'nomfa@test.example' }],
107+
},
108+
mfaInput: { loginName: 'nomfa@test.example' },
109+
};
110+
111+
it('returns a redirect (not setup) when no MFA methods are offerable', () => {
112+
callService(baseScenario).then((v) => {
113+
const o = v.outcome as { kind: string; offerableKeys?: unknown[] };
114+
expect(o.kind, 'auto-skip: kind must be redirect, not setup').to.equal('redirect');
115+
});
116+
});
117+
118+
it('redirect target does not loop back to /setup/mfa', () => {
119+
callService(baseScenario).then((v) => {
120+
const o = v.outcome as { kind: string; target?: string };
121+
expect(o.target ?? '', 'target must not loop back to /setup/mfa').to.not.include(
122+
'/setup/mfa'
123+
);
124+
});
125+
});
126+
127+
it('emits an mfa_skip success audit line on auto-skip', () => {
128+
callService(baseScenario).then((v) => {
129+
const skipAudit = v.audit.find((e) => e.event === 'mfa_skip' && e.outcome === 'success');
130+
expect(skipAudit, 'mfa_skip success audit must be emitted').to.not.equal(undefined);
131+
});
132+
});
133+
});
134+
135+
// ── resolveMfaSetup: normal path (non-empty offerable set) still returns offerableKeys ──────────
136+
//
137+
// Regression guard: ensure the fix does not break the normal chooser path where MFA methods ARE
138+
// available. Uses the singleton provider which has totpOtp + emailOtp + passkey capabilities.
139+
140+
describe('resolveMfaSetup — normal path returns offerableKeys when MFA methods exist', () => {
141+
const scenario = {
142+
fn: 'resolveMfaSetup' as const,
143+
provider: 'singleton' as const,
144+
liveSessions: [
145+
{
146+
id: 's-alice-setup',
147+
token: 't-alice-setup',
148+
user: { id: 'u1', loginName: 'alice@acme.test' },
149+
},
150+
],
151+
request: {
152+
url: 'http://localhost/id/setup/mfa',
153+
sessions: [{ id: 's-alice-setup', token: 't-alice-setup', loginName: 'alice@acme.test' }],
154+
},
155+
mfaInput: { loginName: 'alice@acme.test' },
156+
};
157+
158+
it('returns kind: setup (not redirect) when offerable MFA methods exist', () => {
159+
callService(scenario).then((v) => {
160+
const o = v.outcome as { kind: string; offerableKeys?: string[] };
161+
expect(o.kind, 'chooser renders when methods available').to.equal('setup');
162+
});
163+
});
164+
165+
it('offerableKeys is non-empty when MFA capabilities are enabled', () => {
166+
callService(scenario).then((v) => {
167+
const o = v.outcome as { kind: string; offerableKeys?: string[] };
168+
expect((o.offerableKeys ?? []).length, 'offerable keys non-empty').to.be.greaterThan(0);
169+
});
170+
});
171+
});

cypress/support/node/scenario.ts

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,13 @@ export interface ScenarioSeed {
3434
/** Instance Default Organization getDefaultOrg returns (org-first fallback). Default
3535
* 'org-default-fake' in the fake; pass `null` for the "no default org" last-resort branch. */
3636
defaultOrgId?: string | null;
37+
38+
/**
39+
* Partial capability override applied over the FakeAuthProvider class defaults.
40+
* Use to simulate an instance that offers no MFA enrollment methods — e.g. all MFA
41+
* capabilities false triggers the resolveMfaSetup auto-skip path.
42+
*/
43+
capabilities?: Partial<Record<string, boolean>>;
3744
deviceAuths?: Array<{ userCode: string; id: string; appName?: string; scope: string[] }>;
3845
samlRequests?: Array<{ id: string; clientId: string; binding: 'redirect' | 'post' }>;
3946
}

0 commit comments

Comments
 (0)