diff --git a/packages/profile-sync-controller/CHANGELOG.md b/packages/profile-sync-controller/CHANGELOG.md index 2b3ca2ca5be..8554a7602a4 100644 --- a/packages/profile-sync-controller/CHANGELOG.md +++ b/packages/profile-sync-controller/CHANGELOG.md @@ -9,9 +9,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- Open the verification session on enrollment and let the server set its lifetime ([#10653](https://github.com/MetaMask/core/pull/10653)) + - `completeCredentialEnrollment` opens a verification session with the assertion `POST /api/v2/mfa/enroll/complete` returns for the new credential, replacing any earlier one, so no separate verification is needed right after enrolling. If the token exchange fails, the enrollment still succeeds and the earlier session is kept + - `SRPJwtBearerAuth.completeMfaEnrollment` and `JwtBearerAuth.completeMfaEnrollment` return that assertion + - The session lasts for the token's `expires_in`, measured on the device clock, instead of at most 15 minutes - Replace JS AES implementation with `@metamask/cryptography` ([#10621](https://github.com/MetaMask/core/pull/10621)) - Bump `immer` from `^9.0.21` to `^11.1.18` ([#10382](https://github.com/MetaMask/core/pull/10382)) +### Removed + +- **BREAKING:** Remove `VERIFICATION_SESSION_TTL_MS`, as the server now sets the verification session lifetime ([#10653](https://github.com/MetaMask/core/pull/10653)) + ### Fixed - Coalesce overlapping `performSignIn` calls so only one sign-in runs at a time ([#10646](https://github.com/MetaMask/core/pull/10646)) diff --git a/packages/profile-sync-controller/README.md b/packages/profile-sync-controller/README.md index b4d6174d0ec..9cd1ab16c66 100644 --- a/packages/profile-sync-controller/README.md +++ b/packages/profile-sync-controller/README.md @@ -58,14 +58,21 @@ email OTP enrollment and verification: The controller never inspects the token's assurance level; the server decides. A setup flow that proved a factor itself can pass `maxSessionAgeMs` (for example, the time since the flow started) so chained - enrollments reuse that proof. Enrollment does not end the session. + enrollments reuse that proof. `completeCredentialEnrollment()` opens a + verification session with the assertion the server returns for the new + credential (replacing any earlier one), so no separate verification is + needed right after enrolling. - `beginCredentialVerification()` and `completeCredentialVerification()` verify an enrolled credential and return a verification token. - `getVerificationToken()` reuses a live verification session when it satisfies the caller's freshness requirement; `clearVerificationSession()` clears it. The - session lasts as long as the verification token (at most - `VERIFICATION_SESSION_TTL_MS`, 15 minutes) and ends on lock, sign-out, reset, or - a rejected base session. + session lasts as long as the server says the token does (`expires_in`, + measured from when it was obtained) and ends on lock, sign-out, reset, or a + rejected base session. It is + a low-level read: features should go through the client MFA kit + (`verifyOrEnroll`), which reuses a matching session without showing any + screen. Read it directly only from code that cannot show UI, and treat `null` + as "let the UI layer ask". Clients must retain the challenge `flowId`, perform the platform ceremony, and send the resulting proof to the matching completion method. OTP codes, diff --git a/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController-method-action-types.ts b/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController-method-action-types.ts index 14c5b290065..9531c2b8287 100644 --- a/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController-method-action-types.ts +++ b/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController-method-action-types.ts @@ -44,6 +44,11 @@ export type AuthenticationControllerBeginCredentialEnrollmentAction = { /** * Completes credential enrollment and refreshes the credential cache. * + * The server returns an assertion for the new credential, which opens a + * verification session like `completeCredentialVerification`, replacing + * any earlier one. If that exchange fails, the earlier session is kept: + * the credential is enrolled either way. + * * A cache-refresh failure does not undo successful enrollment. Email * enrollment invalidates the primary SRP session *after* refresh so the * credentials call can reuse the still-valid access token; the next token @@ -91,6 +96,12 @@ export type AuthenticationControllerCompleteCredentialVerificationAction = { * Returns the active verification token when it meets the requested * freshness. * + * Low-level: features should go through the client MFA kit + * (`verifyOrEnroll`), which reuses a matching session without showing any + * screen and checks which method proved it. Read the token directly only + * from code that cannot show UI, and treat `null` as "let the UI layer + * ask". + * * @param request - Optional maximum session age in milliseconds, measured * from when the token was obtained. Zero always requires a new ceremony. * @returns A live verification token, or null when no reusable session diff --git a/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.test.ts b/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.test.ts index 8632da513a1..072ed2219fb 100644 --- a/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.test.ts +++ b/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.test.ts @@ -16,6 +16,7 @@ import { MOCK_ACCESS_JWT, MOCK_MFA_CREDENTIALS_RESPONSE, MOCK_VERIFICATION_ACCESS_TOKEN_RESPONSE, + MOCK_MFA_ENROLL_COMPLETE_RESPONSE, MOCK_MFA_ENROLL_EMAIL_RESPONSE, MOCK_MFA_VERIFY_COMPLETE_RESPONSE, MOCK_USER_PROFILE_LINEAGE_RESPONSE, @@ -36,7 +37,6 @@ import { AuthenticationController, defaultState, ENROLLMENT_MAX_SESSION_AGE_MS, - VERIFICATION_SESSION_TTL_MS, } from './AuthenticationController.js'; import type { AuthenticationControllerMessenger, @@ -59,6 +59,27 @@ jest.mock('../../shared/utils/message-signing.js', () => ({ const MOCK_HD_SEED = new Uint8Array(64).fill(1); +/** + * Builds an unsigned verification access token. + * + * @param claims - Token claims. + * @param claims.amr - Method the verification used. + * @param claims.exp - Expiry, in seconds since the epoch. + * @returns The JWT. + */ +const buildVerificationJwt = ({ + amr, + exp, +}: { + amr: string; + exp: number; +}): string => + [ + btoa(JSON.stringify({ alg: 'none', typ: 'JWT' })), + btoa(JSON.stringify({ sub: 'profile-id', aal: 2, amr, exp })), + 'signature', + ].join('.'); + const MOCK_ENTROPY_SOURCE_IDS = [ 'MOCK_ENTROPY_SOURCE_ID', 'MOCK_ENTROPY_SOURCE_ID2', @@ -2781,13 +2802,21 @@ describe('MFA credential enrollment', () => { const requestStarted = new Promise((resolve) => { requestStartedResolve = resolve; }); - const fetchSpy = jest.spyOn(globalThis, 'fetch').mockImplementationOnce( - async (): ReturnType => - await new Promise((resolve) => { - release = resolve; - requestStartedResolve?.(); - }), - ); + const fetchSpy = jest + .spyOn(globalThis, 'fetch') + .mockImplementationOnce( + async (): ReturnType => + await new Promise((resolve) => { + release = resolve; + requestStartedResolve?.(); + }), + ) + .mockResolvedValueOnce( + new globalThis.Response( + JSON.stringify(MOCK_VERIFICATION_ACCESS_TOKEN_RESPONSE), + { status: 200 }, + ), + ); const { controller, baseMessenger } = createController(); try { const completion = controller.completeCredentialEnrollment({ @@ -2798,16 +2827,23 @@ describe('MFA credential enrollment', () => { await requestStarted; baseMessenger.publish('KeyringController:lock'); release( - new globalThis.Response(JSON.stringify({ status: 'enrolled' }), { - status: 200, - }), + new globalThis.Response( + JSON.stringify(MOCK_MFA_ENROLL_COMPLETE_RESPONSE), + { + status: 200, + }, + ), ); await expect(completion).rejects.toThrow( 'the authenticated session ended', ); - // No credentials refresh was attempted after the session ended. - expect(fetchSpy).toHaveBeenCalledTimes(1); + // Enrollment and token exchange only: no credentials refresh after the + // session ended. + expect(fetchSpy).toHaveBeenCalledTimes(2); + expect(() => controller.getVerificationToken()).toThrow( + 'wallet is locked', + ); expect(controller.state.enrolledCredentials).toStrictEqual([]); // The enrollment succeeded server-side, so the token cached across the // lock must not be reused without the new email claim. @@ -2892,13 +2928,21 @@ describe('MFA credential enrollment', () => { const requestStarted = new Promise((resolve) => { requestStartedResolve = resolve; }); - const fetchSpy = jest.spyOn(globalThis, 'fetch').mockImplementationOnce( - async (): ReturnType => - await new Promise((resolve) => { - resolveCompletion = resolve; - requestStartedResolve?.(); - }), - ); + const fetchSpy = jest + .spyOn(globalThis, 'fetch') + .mockImplementationOnce( + async (): ReturnType => + await new Promise((resolve) => { + resolveCompletion = resolve; + requestStartedResolve?.(); + }), + ) + .mockResolvedValueOnce( + new globalThis.Response( + JSON.stringify(MOCK_VERIFICATION_ACCESS_TOKEN_RESPONSE), + { status: 200 }, + ), + ); const { controller, baseMessenger } = createController(); try { const completion = controller.completeCredentialEnrollment({ @@ -2909,15 +2953,19 @@ describe('MFA credential enrollment', () => { await requestStarted; baseMessenger.publish('KeyringController:lock'); resolveCompletion?.( - new globalThis.Response(JSON.stringify({ status: 'enrolled' }), { - status: 200, - }), + new globalThis.Response( + JSON.stringify(MOCK_MFA_ENROLL_COMPLETE_RESPONSE), + { + status: 200, + }, + ), ); await expect(completion).rejects.toThrow( 'the authenticated session ended', ); - expect(fetchSpy).toHaveBeenCalledTimes(1); + // Enrollment and token exchange only: no credentials refresh. + expect(fetchSpy).toHaveBeenCalledTimes(2); expect(controller.state.enrolledCredentials).toStrictEqual([]); } finally { fetchSpy.mockRestore(); @@ -2969,6 +3017,12 @@ describe('MFA credential enrollment', () => { enrollCompleteStarted.resolve(); return enrollComplete.promise; }) + .mockResolvedValueOnce( + new globalThis.Response( + JSON.stringify(MOCK_VERIFICATION_ACCESS_TOKEN_RESPONSE), + { status: 200 }, + ), + ) .mockImplementationOnce(async (): ReturnType => { enrollmentCredentialsStarted.resolve(); return enrollmentCredentials.promise; @@ -2991,9 +3045,12 @@ describe('MFA credential enrollment', () => { // 3. Let the enroll-complete POST resolve, then its internal refresh GET. await enrollCompleteStarted.promise; enrollComplete.resolve( - new globalThis.Response(JSON.stringify({ status: 'enrolled' }), { - status: 200, - }), + new globalThis.Response( + JSON.stringify(MOCK_MFA_ENROLL_COMPLETE_RESPONSE), + { + status: 200, + }, + ), ); await enrollmentCredentialsStarted.promise; @@ -3236,48 +3293,22 @@ describe('MFA credential verification', () => { expect(controller.getVerificationToken()).not.toBeNull(); }); - it('hard-clears the session at the session TTL', async () => { - const fetchSpy = jest - .spyOn(globalThis, 'fetch') - .mockResolvedValueOnce( - new globalThis.Response( - JSON.stringify(MOCK_MFA_VERIFY_COMPLETE_RESPONSE), - { status: 200 }, - ), - ) - .mockResolvedValueOnce( - new globalThis.Response( - JSON.stringify(MOCK_VERIFICATION_ACCESS_TOKEN_RESPONSE), - { status: 200 }, - ), - ); - jest.useFakeTimers({ doNotFake: ['nextTick', 'setImmediate'] }); - jest.setSystemTime(new Date('2026-09-16T10:00:00Z')); - try { - const { controller } = createController(); - await completeCredentialVerification(controller); - - expect(controller.getVerificationToken()).not.toBeNull(); - jest.advanceTimersByTime(VERIFICATION_SESSION_TTL_MS); - expect(controller.getVerificationToken()).toBeNull(); - } finally { - jest.useRealTimers(); - fetchSpy.mockRestore(); - } - }); - - it('hard-clears the session when the verification token expires first', async () => { - const now = new Date('2026-09-16T10:00:00Z'); - const header = btoa(JSON.stringify({ alg: 'none', typ: 'JWT' })); - const payload = btoa( - JSON.stringify({ - sub: 'profile-id', - aal: 2, - amr: 'passkey', - exp: Math.floor(now.getTime() / 1000) + 1, - }), - ); - const fetchSpy = jest + /** + * Mocks a verification that exchanges to a token expiring `lifetimeSeconds` + * after `now`. + * + * @param now - Current time. + * @param lifetimeSeconds - Token lifetime. + * @param serverClockOffsetSeconds - How far the server clock, which sets + * `exp`, is ahead of the device clock. + * @returns The fetch spy. + */ + function mockVerificationWithLifetime( + now: Date, + lifetimeSeconds: number, + serverClockOffsetSeconds = 0, + ): jest.SpyInstance { + return jest .spyOn(globalThis, 'fetch') .mockResolvedValueOnce( new globalThis.Response( @@ -3288,51 +3319,101 @@ describe('MFA credential verification', () => { .mockResolvedValueOnce( new globalThis.Response( JSON.stringify({ - access_token: `${header}.${payload}.signature`, - expires_in: 900, + access_token: buildVerificationJwt({ + amr: 'passkey', + exp: + Math.floor(now.getTime() / 1000) + + serverClockOffsetSeconds + + lifetimeSeconds, + }), + expires_in: lifetimeSeconds, }), { status: 200 }, ), ); - jest.useFakeTimers({ doNotFake: ['nextTick', 'setImmediate'] }); - jest.setSystemTime(now); - try { + } + + it.each([ + ['ahead of', -2 * 60 * 60], + ['behind', 2 * 60 * 60], + ])( + 'keeps the session for the token lifetime when the device clock is %s the server', + async (_name, serverClockOffsetSeconds) => { + const now = new Date('2026-09-16T10:00:00Z'); + const fetchSpy = mockVerificationWithLifetime( + now, + 60 * 60, + serverClockOffsetSeconds, + ); + jest.useFakeTimers({ doNotFake: ['nextTick', 'setImmediate'] }); + jest.setSystemTime(now); + try { + const { controller } = createController(); + await completeCredentialVerification(controller); + + jest.advanceTimersByTime(60 * 60 * 1000 - 1); + expect(controller.getVerificationToken()).not.toBeNull(); + jest.advanceTimersByTime(1); + expect(controller.getVerificationToken()).toBeNull(); + } finally { + jest.useRealTimers(); + fetchSpy.mockRestore(); + } + }, + ); + + it.each([0, -1, undefined])( + 'rejects an exchanged token whose lifetime is %s', + async (expiresIn) => { + mockEndpointMfaVerifyComplete(); + mockEndpointAccessToken({ + status: 200, + body: { + access_token: MOCK_VERIFICATION_ACCESS_TOKEN_RESPONSE.access_token, + expires_in: expiresIn, + }, + }); const { controller } = createController(); - await completeCredentialVerification(controller); - jest.advanceTimersByTime(999); - expect(controller.getVerificationToken()).not.toBeNull(); - jest.advanceTimersByTime(1); + await expect( + completeCredentialVerification(controller), + ).rejects.toMatchObject({ mfaCode: 'verification_token_invalid' }); expect(controller.getVerificationToken()).toBeNull(); - } finally { - jest.useRealTimers(); - fetchSpy.mockRestore(); - } - }); + }, + ); + + it.each([1, 60 * 60, 24 * 60 * 60])( + 'keeps the session for exactly the token lifetime the server set (%ss)', + async (lifetimeSeconds) => { + const now = new Date('2026-09-16T10:00:00Z'); + const fetchSpy = mockVerificationWithLifetime(now, lifetimeSeconds); + jest.useFakeTimers({ doNotFake: ['nextTick', 'setImmediate'] }); + jest.setSystemTime(now); + try { + const { controller } = createController(); + await completeCredentialVerification(controller); + + jest.advanceTimersByTime(lifetimeSeconds * 1000 - 1); + expect(controller.getVerificationToken()).not.toBeNull(); + jest.advanceTimersByTime(1); + expect(controller.getVerificationToken()).toBeNull(); + } finally { + jest.useRealTimers(); + fetchSpy.mockRestore(); + } + }, + ); it('drops the session when the clock passes expiration before the timer runs', async () => { const now = new Date('2026-09-16T10:00:00Z'); - const fetchSpy = jest - .spyOn(globalThis, 'fetch') - .mockResolvedValueOnce( - new globalThis.Response( - JSON.stringify(MOCK_MFA_VERIFY_COMPLETE_RESPONSE), - { status: 200 }, - ), - ) - .mockResolvedValueOnce( - new globalThis.Response( - JSON.stringify(MOCK_VERIFICATION_ACCESS_TOKEN_RESPONSE), - { status: 200 }, - ), - ); + const fetchSpy = mockVerificationWithLifetime(now, 60); jest.useFakeTimers({ doNotFake: ['nextTick', 'setImmediate'] }); jest.setSystemTime(now); try { const { controller } = createController(); await completeCredentialVerification(controller); - jest.setSystemTime(now.getTime() + VERIFICATION_SESSION_TTL_MS + 1_000); + jest.setSystemTime(now.getTime() + 61_000); expect(controller.getVerificationToken()).toBeNull(); } finally { jest.useRealTimers(); @@ -3657,24 +3738,158 @@ describe('MFA credential verification', () => { }); }); - it('keeps the verification session after successful enrollment so chained enrollments reuse it', async () => { - mockSuccessfulCompletion(); - mockEndpointMfaEnrollComplete(); - mockEndpointMfaCredentials(); - const { controller } = createController(); - await completeCredentialVerification(controller); - const verificationToken = controller.getVerificationToken()?.accessToken; + describe('session from the enrollment assertion', () => { + const enrolledEmailToken = buildVerificationJwt({ + amr: 'email_otp', + exp: 4102444800, + }); - await controller.completeCredentialEnrollment({ - flowId: 'enrollment-flow', - proof: { type: 'email_otp', code: '123456' }, - reason: { operation: 'settings.addEmail' }, + const completeEmailEnrollment = async ( + controller: AuthenticationController, + ): Promise => + await controller.completeCredentialEnrollment({ + flowId: 'enrollment-flow', + proof: { type: 'email_otp', code: '123456' }, + reason: { operation: 'vba.startKyc' }, + }); + + it('opens a verification session for the enrolled credential and traces the exchange', async () => { + mockEndpointMfaEnrollComplete(); + mockEndpointAccessToken({ + status: 200, + body: { access_token: enrolledEmailToken, expires_in: 3600 }, + }); + mockEndpointMfaCredentials(); + const requests: { name?: string }[] = []; + const trace = jest.fn( + ( + request: { name?: string }, + fn?: (context?: unknown) => unknown, + ): Promise => { + requests.push(request); + return Promise.resolve(fn?.()); + }, + ) as unknown as TraceCallback; + const { controller } = createController({ trace }); + + await completeEmailEnrollment(controller); + + expect(controller.getVerificationToken()).toMatchObject({ + accessToken: enrolledEmailToken, + claims: { amr: ['email_otp'] }, + }); + expect(requests.map(({ name }) => name)).toStrictEqual([ + 'MFA Enroll Complete', + 'MFA Token Exchange', + 'MFA Credentials Refresh', + ]); + expect(await getEnrollmentBearer(controller)).toBe(enrolledEmailToken); }); - expect(controller.getVerificationToken()?.accessToken).toBe( - verificationToken, + it('replaces an earlier verification session', async () => { + mockSuccessfulCompletion(); + const { controller } = createController(); + await completeCredentialVerification(controller); + cleanAllNock(); + mockEndpointMfaEnrollComplete(); + mockEndpointAccessToken({ + status: 200, + body: { access_token: enrolledEmailToken, expires_in: 3600 }, + }); + mockEndpointMfaCredentials(); + + await completeEmailEnrollment(controller); + + expect(controller.getVerificationToken()?.accessToken).toBe( + enrolledEmailToken, + ); + }); + + it.each([ + [ + 'the exchange fails', + { status: 500, body: { message: 'Internal error' } }, + ], + [ + 'the exchanged token has no verification claims', + { + status: 200, + body: { access_token: MOCK_ACCESS_JWT, expires_in: 900 }, + }, + ], + ])( + 'still completes the enrollment and keeps the earlier session when %s', + async (_name, exchangeReply) => { + mockSuccessfulCompletion(); + const { controller } = createController(); + await completeCredentialVerification(controller); + const earlier = controller.getVerificationToken()?.accessToken; + cleanAllNock(); + mockEndpointMfaEnrollComplete(); + mockEndpointAccessToken(exchangeReply); + mockEndpointMfaCredentials(); + + expect(await completeEmailEnrollment(controller)).toHaveLength( + MOCK_MFA_CREDENTIALS_RESPONSE.credentials.length, + ); + expect(controller.getVerificationToken()?.accessToken).toBe(earlier); + expect( + controller.state.srpSessionData?.[MOCK_ENTROPY_SOURCE_IDS[0]].profile + .canonicalProfileId, + ).toBe(''); + }, ); - expect(await getEnrollmentBearer(controller)).toBe(verificationToken); + + it('opens no session and still invalidates the SRP session if the wallet locks during the exchange', async () => { + let releaseExchange!: (value: Awaited>) => void; + let exchangeStartedResolve: (() => void) | undefined; + const exchangeStarted = new Promise((resolve) => { + exchangeStartedResolve = resolve; + }); + const fetchSpy = jest + .spyOn(globalThis, 'fetch') + .mockResolvedValueOnce( + new globalThis.Response( + JSON.stringify(MOCK_MFA_VERIFY_COMPLETE_RESPONSE), + { status: 200 }, + ), + ) + .mockImplementationOnce( + async (): ReturnType => + await new Promise((resolve) => { + releaseExchange = resolve; + exchangeStartedResolve?.(); + }), + ); + const { controller, baseMessenger } = createController(); + try { + const completion = completeEmailEnrollment(controller); + await exchangeStarted; + baseMessenger.publish('KeyringController:lock'); + releaseExchange( + new globalThis.Response( + JSON.stringify({ + access_token: enrolledEmailToken, + expires_in: 3600, + }), + { status: 200 }, + ), + ); + + await expect(completion).rejects.toThrow( + 'the authenticated session ended', + ); + expect(fetchSpy).toHaveBeenCalledTimes(2); + baseMessenger.publish('KeyringController:unlock'); + expect(controller.getVerificationToken()).toBeNull(); + expect( + controller.state.srpSessionData?.[MOCK_ENTROPY_SOURCE_IDS[0]].profile + .canonicalProfileId, + ).toBe(''); + } finally { + fetchSpy.mockRestore(); + } + }); }); }); diff --git a/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.ts b/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.ts index bce495dfd0e..9bdc9b3da27 100644 --- a/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.ts +++ b/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.ts @@ -47,6 +47,8 @@ import type { CompleteVerificationRequest, VerificationToken, GetVerificationTokenRequest, + MfaCredentialType, + MfaVerificationAssertion, VerificationChallenge, } from '../../sdk/index.js'; import { @@ -179,15 +181,6 @@ const metadata: StateMetadata = { }, }; -/** - * Upper bound on a verification session's lifetime. The session also ends at - * the verification token's own `exp` (15 minutes today), whichever comes - * first, so this only matters if the server ever issues longer-lived tokens. - * Callers needing a fresher proof pass `maxSessionAgeMs` to - * `getVerificationToken`. - */ -export const VERIFICATION_SESSION_TTL_MS = 15 * 60_000; - /** * Default maximum age of a verification session that may authorize enrolling a * credential. A session opened for an unrelated operation must not be able to @@ -1036,6 +1029,11 @@ export class AuthenticationController extends BaseController< /** * Completes credential enrollment and refreshes the credential cache. * + * The server returns an assertion for the new credential, which opens a + * verification session like `completeCredentialVerification`, replacing + * any earlier one. If that exchange fails, the earlier session is kept: + * the credential is enrolled either way. + * * A cache-refresh failure does not undo successful enrollment. Email * enrollment invalidates the primary SRP session *after* refresh so the * credentials call can reuse the still-valid access token; the next token @@ -1054,10 +1052,11 @@ export class AuthenticationController extends BaseController< const sessionEpoch = this.#authSessionEpoch; assertValidMfaRequest(request, CompleteEnrollmentRequestStruct); const { type } = request.proof; + const { operation } = request.reason; const primaryEntropySourceId = this.#getPrimaryEntropySourceId(); - await this.#runMfaRequest( + const assertion = await this.#runMfaRequest( 'MFA Enroll Complete', - request.reason.operation, + operation, type, async () => await this.#auth.completeMfaEnrollment( @@ -1067,6 +1066,19 @@ export class AuthenticationController extends BaseController< ), ); + try { + await this.#openVerificationSessionFromAssertion(assertion, { + operation, + type, + sessionEpoch, + methodName: 'completeCredentialEnrollment', + }); + } catch { + // Callers see no session and verify the credential instead. + } + + // Also catches a session that ended during the enrollment request; the + // exchange above then ran for nothing, and its session was never opened. try { this.#assertAuthSessionEpoch( sessionEpoch, @@ -1079,9 +1091,6 @@ export class AuthenticationController extends BaseController< throw error; } - // The verification session is deliberately kept: adding a factor does not - // weaken an earlier proof, and `beginCredentialEnrollment` already limits - // which sessions may add the next one. try { return await this.refreshEnrolledCredentials(); } catch { @@ -1155,16 +1164,47 @@ export class AuthenticationController extends BaseController< sessionEpoch, 'completeCredentialVerification', ); + return await this.#openVerificationSessionFromAssertion(assertion, { + operation, + type, + sessionEpoch, + methodName: 'completeCredentialVerification', + }); + } + + /** + * Exchanges an MFA assertion at Hydra and opens the verification session + * with the resulting token. + * + * @param assertion - Assertion returned by a verify or enroll completion. + * @param context - Trace tags and the session the call started in. + * @param context.operation - Feature operation, for tracing. + * @param context.type - Credential type the assertion proves, for tracing. + * @param context.sessionEpoch - Authenticated session the call started in. + * @param context.methodName - Public method name, for error messages. + * @returns The verification token. + */ + async #openVerificationSessionFromAssertion( + assertion: MfaVerificationAssertion, + { + operation, + type, + sessionEpoch, + methodName, + }: { + operation: string; + type: MfaCredentialType; + sessionEpoch: number; + methodName: string; + }, + ): Promise { const accessToken = await this.#runMfaRequest( 'MFA Token Exchange', operation, type, async () => await this.#auth.exchangeMfaAssertion(assertion.token), ); - this.#assertAuthSessionEpoch( - sessionEpoch, - 'completeCredentialVerification', - ); + this.#assertAuthSessionEpoch(sessionEpoch, methodName); let decodedClaims: unknown; try { @@ -1173,8 +1213,10 @@ export class AuthenticationController extends BaseController< throw new VerificationTokenInvalidError(toErrorMessage(error)); } const claims = parseVerificationTokenClaims(decodedClaims); - if (claims.exp * 1000 <= Date.now()) { - throw new VerificationTokenInvalidError('Verification token is expired'); + if (!Number.isFinite(accessToken.expiresIn) || accessToken.expiresIn <= 0) { + throw new VerificationTokenInvalidError( + 'Verification token has no remaining lifetime', + ); } const token: VerificationToken = { ...accessToken, claims }; @@ -1186,6 +1228,12 @@ export class AuthenticationController extends BaseController< * Returns the active verification token when it meets the requested * freshness. * + * Low-level: features should go through the client MFA kit + * (`verifyOrEnroll`), which reuses a matching session without showing any + * screen and checks which method proved it. Read the token directly only + * from code that cannot show UI, and treat `null` as "let the UI layer + * ask". + * * @param request - Optional maximum session age in milliseconds, measured * from when the token was obtained. Zero always requires a new ceremony. * @returns A live verification token, or null when no reusable session @@ -1216,18 +1264,16 @@ export class AuthenticationController extends BaseController< } /** - * Opens the verification session. Its lifetime is the session TTL clamped to - * the token's own `exp`, so the session never outlives the token. + * Opens the verification session for the lifetime the server gave the token. + * It is measured from `expires_in` on the local clock rather than read from + * `exp`, so a skewed device clock cannot shorten or extend it. * * @param token - The freshly exchanged verification token. */ #openVerificationSession(token: VerificationToken): void { const audience = DEFAULT_AUDIENCE; this.#dropVerificationSession(audience); - const expiresAt = Math.min( - token.obtainedAt + VERIFICATION_SESSION_TTL_MS, - token.claims.exp * 1000, - ); + const expiresAt = token.obtainedAt + token.expiresIn * 1000; const timer = setTimeout( () => this.#dropVerificationSession(audience), Math.max(0, expiresAt - Date.now()), diff --git a/packages/profile-sync-controller/src/sdk/authentication-jwt-bearer/flow-srp.test.ts b/packages/profile-sync-controller/src/sdk/authentication-jwt-bearer/flow-srp.test.ts index d4112d39245..2576e8d4418 100644 --- a/packages/profile-sync-controller/src/sdk/authentication-jwt-bearer/flow-srp.test.ts +++ b/packages/profile-sync-controller/src/sdk/authentication-jwt-bearer/flow-srp.test.ts @@ -437,7 +437,10 @@ describe('SRP MFA methods', () => { it('completes both enrollment proof types', async () => { const auth = createAuth(); - mockMfaEnrollComplete.mockResolvedValue(undefined); + mockMfaEnrollComplete.mockResolvedValue({ + token: 'assertion-jwt', + expiresIn: 900, + }); const attestation = { id: 'id', rawId: 'raw-id', diff --git a/packages/profile-sync-controller/src/sdk/authentication-jwt-bearer/flow-srp.ts b/packages/profile-sync-controller/src/sdk/authentication-jwt-bearer/flow-srp.ts index 37a400f45f6..c072c7fd5d8 100644 --- a/packages/profile-sync-controller/src/sdk/authentication-jwt-bearer/flow-srp.ts +++ b/packages/profile-sync-controller/src/sdk/authentication-jwt-bearer/flow-srp.ts @@ -286,14 +286,15 @@ export class SRPJwtBearerAuth implements IBaseAuth { * @param flowId - Identifier returned by the begin call. * @param proof - Platform attestation or email code. * @param entropySourceId - Entropy source whose profile owns the credential. + * @returns Assertion proving the enrolled credential. */ async completeMfaEnrollment( flowId: string, proof: EnrollmentProof, entropySourceId?: string, - ): Promise { + ): Promise { const accessToken = await this.getAccessToken(entropySourceId); - await mfaEnrollComplete(this.#config.env, accessToken, { + return await mfaEnrollComplete(this.#config.env, accessToken, { credential_type: proof.type, flow_id: flowId, ...(proof.type === 'passkey' diff --git a/packages/profile-sync-controller/src/sdk/authentication-jwt-bearer/mfa/schemas.ts b/packages/profile-sync-controller/src/sdk/authentication-jwt-bearer/mfa/schemas.ts index cebe9bc8570..34b0161290b 100644 --- a/packages/profile-sync-controller/src/sdk/authentication-jwt-bearer/mfa/schemas.ts +++ b/packages/profile-sync-controller/src/sdk/authentication-jwt-bearer/mfa/schemas.ts @@ -135,9 +135,11 @@ export const MfaEnrollResponseStruct = type({ passkey_create_data: optional(sensitive(string())), }); -export const MfaEnrollCompleteResponseStruct = type({ - status: literal('enrolled'), -}); +/** + * Enrollment returns the same assertion as verification, proving the + * credential just enrolled. + */ +export const MfaEnrollCompleteResponseStruct = MfaVerifyCompleteResponseStruct; export const MfaVerifyResponseStruct = type({ flow_id: string(), diff --git a/packages/profile-sync-controller/src/sdk/authentication-jwt-bearer/mfa/services.test.ts b/packages/profile-sync-controller/src/sdk/authentication-jwt-bearer/mfa/services.test.ts index a51e1f2ecd6..adc2232253b 100644 --- a/packages/profile-sync-controller/src/sdk/authentication-jwt-bearer/mfa/services.test.ts +++ b/packages/profile-sync-controller/src/sdk/authentication-jwt-bearer/mfa/services.test.ts @@ -98,12 +98,12 @@ describe('MFA services', () => { ); }); - it('completes passkey and email enrollment', async () => { + it('completes passkey and email enrollment and returns the assertion', async () => { mockFetch .mockResolvedValueOnce(response(MOCK_MFA_ENROLL_COMPLETE_RESPONSE)) .mockResolvedValueOnce(response(MOCK_MFA_ENROLL_COMPLETE_RESPONSE)); - await mfaEnrollComplete(Env.PRD, 'access-token', { + const passkeyCompletion = await mfaEnrollComplete(Env.PRD, 'access-token', { credential_type: 'passkey', flow_id: 'flow-id', passkey_attestation: registration, @@ -114,12 +114,28 @@ describe('MFA services', () => { otp_code: '123456', }); + expect(passkeyCompletion).toStrictEqual({ + token: MOCK_MFA_ENROLL_COMPLETE_RESPONSE.token, + expiresIn: 900, + }); const firstBody = JSON.parse(mockFetch.mock.calls[0][1].body); const secondBody = JSON.parse(mockFetch.mock.calls[1][1].body); expect(firstBody.passkey_attestation).toBe(JSON.stringify(registration)); expect(secondBody.otp_code).toBe('123456'); }); + it('rejects an enrollment response without an assertion', async () => { + mockFetch.mockResolvedValueOnce(response({ expires_in: 900 })); + + await expect( + mfaEnrollComplete(Env.PRD, 'access-token', { + credential_type: 'email_otp', + flow_id: 'flow-id', + otp_code: '123456', + }), + ).rejects.toMatchObject({ mfaCode: 'invalid_response' }); + }); + it('begins and completes passkey verification', async () => { mockFetch .mockResolvedValueOnce(response(MOCK_MFA_VERIFY_PASSKEY_RESPONSE)) diff --git a/packages/profile-sync-controller/src/sdk/authentication-jwt-bearer/mfa/services.ts b/packages/profile-sync-controller/src/sdk/authentication-jwt-bearer/mfa/services.ts index fe9ed1f35c6..6b017ffffb1 100644 --- a/packages/profile-sync-controller/src/sdk/authentication-jwt-bearer/mfa/services.ts +++ b/packages/profile-sync-controller/src/sdk/authentication-jwt-bearer/mfa/services.ts @@ -390,12 +390,14 @@ export async function mfaEnroll( * @param env - Authentication environment. * @param accessToken - Primary profile access token. * @param params - Flow identifier and enrollment proof. + * @returns The assertion JWT proving the enrolled credential, and its lifetime + * in seconds. */ export async function mfaEnrollComplete( env: Env, accessToken: string, params: EnrollmentCompletionParams, -): Promise { +): Promise { let body: MfaEnrollCompleteRequest = { credential_type: params.credential_type, flow_id: params.flow_id, @@ -417,6 +419,7 @@ export async function mfaEnrollComplete( body, }); assertValidMfaResponse(json, MfaEnrollCompleteResponseStruct); + return { token: json.token, expiresIn: json.expires_in }; } /** diff --git a/packages/profile-sync-controller/src/sdk/authentication.test.ts b/packages/profile-sync-controller/src/sdk/authentication.test.ts index 2da8e022ad0..329fd0caca1 100644 --- a/packages/profile-sync-controller/src/sdk/authentication.test.ts +++ b/packages/profile-sync-controller/src/sdk/authentication.test.ts @@ -15,7 +15,11 @@ import { UnsupportedAuthTypeError, ValidationError, } from './errors.js'; -import { MOCK_ACCESS_JWT, MOCK_SRP_LOGIN_RESPONSE } from './mocks/auth.js'; +import { + MOCK_ACCESS_JWT, + MOCK_MFA_ASSERTION_JWT, + MOCK_SRP_LOGIN_RESPONSE, +} from './mocks/auth.js'; import * as Eip6963MetamaskProvider from './utils/eip-6963-metamask-provider.js'; const MOCK_SRP = '0x6265617665726275696c642e6f7267'; @@ -766,7 +770,7 @@ describe('MFA authentication facade', () => { type: 'passkey', attestation: registration, }), - ).toBeUndefined(); + ).toStrictEqual({ token: MOCK_MFA_ASSERTION_JWT, expiresIn: 900 }); expect(await auth.beginMfaVerification('passkey')).toMatchObject({ type: 'passkey', flowId: 'verify-passkey-flow-id', diff --git a/packages/profile-sync-controller/src/sdk/authentication.ts b/packages/profile-sync-controller/src/sdk/authentication.ts index df3423e3c3a..5a6f2b78b69 100644 --- a/packages/profile-sync-controller/src/sdk/authentication.ts +++ b/packages/profile-sync-controller/src/sdk/authentication.ts @@ -129,9 +129,13 @@ export class JwtBearerAuth implements SIWEInterface, SRPInterface { flowId: string, proof: EnrollmentProof, entropySourceId?: string, - ): Promise { + ): Promise { this.#assertSRP(this.#type, this.#sdk); - await this.#sdk.completeMfaEnrollment(flowId, proof, entropySourceId); + return await this.#sdk.completeMfaEnrollment( + flowId, + proof, + entropySourceId, + ); } async beginMfaVerification( diff --git a/packages/profile-sync-controller/src/sdk/mocks/auth.ts b/packages/profile-sync-controller/src/sdk/mocks/auth.ts index b7755533ad5..fc5d580588f 100644 --- a/packages/profile-sync-controller/src/sdk/mocks/auth.ts +++ b/packages/profile-sync-controller/src/sdk/mocks/auth.ts @@ -79,10 +79,6 @@ export const MOCK_MFA_ENROLL_EMAIL_RESPONSE = { expires_at: '2099-09-07T14:30:00Z', }; -export const MOCK_MFA_ENROLL_COMPLETE_RESPONSE = { - status: 'enrolled', -}; - export const MOCK_MFA_VERIFY_PASSKEY_RESPONSE = { flow_id: 'verify-passkey-flow-id', expires_at: '2099-09-07T14:30:00Z', @@ -109,6 +105,9 @@ export const MOCK_MFA_VERIFY_COMPLETE_RESPONSE = { profile_aliases: [], }; +export const MOCK_MFA_ENROLL_COMPLETE_RESPONSE = + MOCK_MFA_VERIFY_COMPLETE_RESPONSE; + export const MOCK_MFA_CREDENTIALS_RESPONSE = { credentials: [ {