From 941a70df97dc63fd656c1101efda210b28de37ca Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Wed, 16 Sep 2026 13:55:09 +0200 Subject: [PATCH 01/13] feat(profile-sync): add MFA credential enrollment Co-authored-by: Cursor --- README.md | 1 + eslint-suppressions.json | 5 - packages/profile-sync-controller/CHANGELOG.md | 1 + packages/profile-sync-controller/package.json | 1 + ...nticationController-method-action-types.ts | 39 ++ .../AuthenticationController.test.ts | 355 ++++++++++++++++++ .../AuthenticationController.ts | 233 +++++++++++- .../__fixtures__/mockServices.ts | 79 +++- .../src/controllers/authentication/index.ts | 3 + .../mocks/mockResponses.test.ts | 23 ++ .../authentication/mocks/mockResponses.ts | 40 ++ .../profile-sync-controller/tsconfig.json | 3 + yarn.lock | 1 + 13 files changed, 775 insertions(+), 9 deletions(-) diff --git a/README.md b/README.md index 864b5a1e51e..8151640a297 100644 --- a/README.md +++ b/README.md @@ -619,6 +619,7 @@ linkStyle default opacity:0.5 profile_metrics_controller --> utils; profile_sync_controller --> address_book_controller; profile_sync_controller --> base_controller; + profile_sync_controller --> controller_utils; profile_sync_controller --> keyring_controller; profile_sync_controller --> messenger; profile_sync_controller --> seedless_onboarding_controller; diff --git a/eslint-suppressions.json b/eslint-suppressions.json index 23d85a763b8..5a72df11541 100644 --- a/eslint-suppressions.json +++ b/eslint-suppressions.json @@ -1412,11 +1412,6 @@ "count": 1 } }, - "packages/profile-sync-controller/src/controllers/authentication/__fixtures__/mockServices.ts": { - "@typescript-eslint/explicit-function-return-type": { - "count": 3 - } - }, "packages/profile-sync-controller/src/controllers/authentication/mocks/mockResponses.ts": { "@typescript-eslint/naming-convention": { "count": 1 diff --git a/packages/profile-sync-controller/CHANGELOG.md b/packages/profile-sync-controller/CHANGELOG.md index 73eae18ecbd..09fd20dccf6 100644 --- a/packages/profile-sync-controller/CHANGELOG.md +++ b/packages/profile-sync-controller/CHANGELOG.md @@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added +- Add credential enrollment and refresh methods, in-memory credential state, change events, automatic refresh configuration, and tracing to `AuthenticationController` ([#10266](https://github.com/MetaMask/core/pull/10266)) - Add passkey and email OTP enrollment, verification, credential-list, and elevated-token exchange SDK methods ([#10265](https://github.com/MetaMask/core/pull/10265)) - Add validated MFA domain types and structured `MfaError` classes with a serialization-safe `mfaCode` ([#10264](https://github.com/MetaMask/core/pull/10264)) - Add `rampsOrders` to `USER_STORAGE_FEATURE_NAMES` ([#10227](https://github.com/MetaMask/core/pull/10227)) diff --git a/packages/profile-sync-controller/package.json b/packages/profile-sync-controller/package.json index a3bd0cdc198..e3db1966dae 100644 --- a/packages/profile-sync-controller/package.json +++ b/packages/profile-sync-controller/package.json @@ -75,6 +75,7 @@ "dependencies": { "@metamask/address-book-controller": "^8.0.0", "@metamask/base-controller": "^10.0.0", + "@metamask/controller-utils": "^13.0.0", "@metamask/key-tree": "^10.1.1", "@metamask/keyring-controller": "^28.0.0", "@metamask/messenger": "^3.0.0", 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 ed7195a52b8..3b5481a59b5 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 @@ -20,6 +20,42 @@ export type AuthenticationControllerRequestProfilePairingAction = { handler: AuthenticationController['requestProfilePairing']; }; +/** + * Refreshes credentials enrolled on the canonical profile. + * + * @returns The current supported credentials. + */ +export type AuthenticationControllerRefreshEnrolledCredentialsAction = { + type: `AuthenticationController:refreshEnrolledCredentials`; + handler: AuthenticationController['refreshEnrolledCredentials']; +}; + +/** + * Begins enrollment of a passkey or email OTP credential. + * + * @param request - Credential, optional email address, and trace reason. + * @returns A challenge for the client-owned ceremony. + */ +export type AuthenticationControllerBeginCredentialEnrollmentAction = { + type: `AuthenticationController:beginCredentialEnrollment`; + handler: AuthenticationController['beginCredentialEnrollment']; +}; + +/** + * Completes credential enrollment and refreshes the credential cache. + * + * A cache-refresh failure does not undo successful enrollment. Email + * enrollment invalidates the primary SRP session so its next token includes + * the newly verified email claim. + * + * @param request - Flow identifier and platform or email proof. + * @returns The refreshed credentials, or the existing cache if refresh fails. + */ +export type AuthenticationControllerCompleteCredentialEnrollmentAction = { + type: `AuthenticationController:completeCredentialEnrollment`; + handler: AuthenticationController['completeCredentialEnrollment']; +}; + export type AuthenticationControllerPerformSignOutAction = { type: `AuthenticationController:performSignOut`; handler: AuthenticationController['performSignOut']; @@ -139,6 +175,9 @@ export type AuthenticationControllerIsSignedInAction = { export type AuthenticationControllerMethodActions = | AuthenticationControllerPerformSignInAction | AuthenticationControllerRequestProfilePairingAction + | AuthenticationControllerRefreshEnrolledCredentialsAction + | AuthenticationControllerBeginCredentialEnrollmentAction + | AuthenticationControllerCompleteCredentialEnrollmentAction | AuthenticationControllerPerformSignOutAction | AuthenticationControllerClearStateAction | AuthenticationControllerGetBearerTokenAction 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 95d88c372b0..b017c45c8db 100644 --- a/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.test.ts +++ b/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.test.ts @@ -1,4 +1,5 @@ import { deriveStateFromMetadata } from '@metamask/base-controller'; +import type { TraceCallback } from '@metamask/controller-utils'; import { KeyringTypes } from '@metamask/keyring-controller'; import { Messenger, MOCK_ANY_NAMESPACE } from '@metamask/messenger'; import type { @@ -6,18 +7,26 @@ import type { MessengerEvents, MockAnyNamespace, } from '@metamask/messenger'; +import { cleanAll as cleanAllNock } from 'nock'; import { arrangeAuthAPIs } from '../../sdk/__fixtures__/auth.js'; import type { LoginResponse } from '../../sdk/index.js'; import { EmailRequiredError, Platform } from '../../sdk/index.js'; import { MOCK_ACCESS_JWT, + MOCK_MFA_CREDENTIALS_RESPONSE, + MOCK_MFA_ENROLL_EMAIL_RESPONSE, MOCK_USER_PROFILE_LINEAGE_RESPONSE, } from '../../sdk/mocks/auth.js'; import { getMessageSigningPublicKey, signMessageWithMessageSigningKey, } from '../../shared/utils/message-signing.js'; +import { + mockEndpointMfaCredentials, + mockEndpointMfaEnroll, + mockEndpointMfaEnrollComplete, +} from './__fixtures__/mockServices.js'; import { AuthenticationController, defaultState, @@ -115,6 +124,7 @@ const mockSignedInState = ({ return { isSignedIn: true, + enrolledCredentials: [], needsProfilePairing, needsSocialPairing, srpSessionData, @@ -2306,6 +2316,315 @@ describe('AuthenticationController', () => { }); }); +describe('MFA credential enrollment', () => { + function createController(options?: { + state?: AuthenticationControllerState; + isMfaEnabled?: () => boolean; + trace?: TraceCallback; + }): { + controller: AuthenticationController; + baseMessenger: RootMessenger; + } { + const { messenger, baseMessenger } = createMockAuthenticationMessenger(); + return { + controller: new AuthenticationController({ + messenger, + metametrics: createMockAuthMetaMetrics(), + state: options?.state ?? mockSignedInState(), + config: { + isMfaEnabled: options?.isMfaEnabled ?? ((): boolean => false), + }, + trace: options?.trace, + }), + baseMessenger, + }; + } + + it('starts with an empty memory-only credential cache', () => { + const { controller } = createController({ + state: { ...defaultState }, + }); + expect(controller.state.enrolledCredentials).toStrictEqual([]); + expect(controller.state.stepUpSessionExpiresAt).toBeUndefined(); + }); + + it('refreshes credentials and publishes changes only when data changes', async () => { + mockEndpointMfaCredentials(); + const { controller, baseMessenger } = createController(); + const listener = jest.fn(); + baseMessenger.subscribe( + 'AuthenticationController:credentialsChanged', + listener, + ); + + expect(await controller.refreshEnrolledCredentials()).toHaveLength(2); + expect(controller.state.enrolledCredentials).toHaveLength(2); + expect(listener).toHaveBeenCalledTimes(1); + + await controller.refreshEnrolledCredentials(); + expect(listener).toHaveBeenCalledTimes(1); + }); + + it('begins passkey enrollment with validated tracing tags', async () => { + mockEndpointMfaEnroll(); + const trace = jest.fn( + (_request: unknown, fn?: () => unknown): Promise => + Promise.resolve(fn?.()), + ) as unknown as TraceCallback; + const { controller } = createController({ trace }); + + expect( + await controller.beginCredentialEnrollment({ + type: 'passkey', + reason: { operation: 'settings.addPasskey' }, + }), + ).toMatchObject({ + type: 'passkey', + flowId: 'enroll-passkey-flow-id', + publicKey: expect.objectContaining({ challenge: expect.any(String) }), + }); + expect(trace).toHaveBeenCalledWith( + expect.objectContaining({ + name: 'MFA Enroll Begin', + tags: { + operation: 'settings.addPasskey', + credentialType: 'passkey', + }, + data: { outcome: 'success' }, + }), + expect.any(Function), + ); + }); + + it('begins email enrollment and rejects invalid boundary input', async () => { + mockEndpointMfaEnroll({ + status: 200, + body: MOCK_MFA_ENROLL_EMAIL_RESPONSE, + }); + const { controller } = createController(); + + expect( + await controller.beginCredentialEnrollment({ + type: 'email_otp', + email: 'user@example.com', + reason: { operation: 'settings.addEmail' }, + }), + ).toStrictEqual({ + type: 'email_otp', + flowId: 'enroll-email-flow-id', + expiresAt: Date.parse('2099-09-07T14:30:00Z'), + emailSent: true, + }); + await expect( + controller.beginCredentialEnrollment({ + type: 'email_otp', + reason: { operation: 'invalid operation' }, + }), + ).rejects.toMatchObject({ mfaCode: 'invalid_request' }); + }); + + it('completes passkey enrollment and refreshes the cache', async () => { + mockEndpointMfaEnrollComplete(); + mockEndpointMfaCredentials(); + const { controller } = createController(); + + const credentials = await controller.completeCredentialEnrollment({ + type: 'passkey', + flowId: 'flow-id', + proof: { + type: 'passkey', + attestation: { + id: 'credential-id', + rawId: 'credential-id', + type: 'public-key', + response: { + attestationObject: 'attestation', + clientDataJSON: 'client-data', + }, + }, + }, + }); + + expect(credentials).toHaveLength( + MOCK_MFA_CREDENTIALS_RESPONSE.credentials.length, + ); + expect(controller.state.enrolledCredentials).toStrictEqual(credentials); + }); + + it('invalidates the primary SRP session after email enrollment', async () => { + mockEndpointMfaEnrollComplete(); + mockEndpointMfaCredentials(); + const { controller } = createController(); + + await controller.completeCredentialEnrollment({ + type: 'email_otp', + flowId: 'flow-id', + proof: { type: 'email_otp', code: '123456' }, + }); + + expect( + controller.state.srpSessionData?.[MOCK_ENTROPY_SOURCE_IDS[0]].profile + .canonicalProfileId, + ).toBe(''); + }); + + it('keeps the existing cache when post-enrollment refresh fails', async () => { + mockEndpointMfaEnrollComplete(); + mockEndpointMfaCredentials({ + status: 502, + body: { code: 'kratos_unavailable', message: 'Unavailable' }, + }); + const existing = [ + { + type: 'passkey', + status: 'active', + displayName: 'Existing passkey', + }, + ] as const; + const { controller } = createController({ + state: { + ...mockSignedInState(), + enrolledCredentials: [...existing], + }, + }); + + expect( + await controller.completeCredentialEnrollment({ + type: 'email_otp', + flowId: 'flow-id', + proof: { type: 'email_otp', code: '123456' }, + }), + ).toStrictEqual(existing); + }); + + it.each([ + [ + 'sign-out', + (controller: AuthenticationController): void => + controller.performSignOut(), + ], + [ + 'wallet reset', + (controller: AuthenticationController): void => controller.clearState(), + ], + [ + 'lock', + ( + _controller: AuthenticationController, + baseMessenger: RootMessenger, + ): void => baseMessenger.publish('KeyringController:lock'), + ], + ])( + 'clears cached credentials and publishes the change on %s', + (_name, act) => { + const { controller, baseMessenger } = createController({ + state: { + ...mockSignedInState(), + enrolledCredentials: [ + { + type: 'email_otp', + status: 'active', + email: 'user@example.com', + verified: true, + }, + ], + }, + }); + const listener = jest.fn(); + baseMessenger.subscribe( + 'AuthenticationController:credentialsChanged', + listener, + ); + + act(controller, baseMessenger); + + expect(controller.state.enrolledCredentials).toStrictEqual([]); + expect(listener).toHaveBeenCalledWith({ credentials: [] }); + }, + ); + + it('does not publish a credential change when the cache is already empty', () => { + const { controller, baseMessenger } = createController(); + const listener = jest.fn(); + baseMessenger.subscribe( + 'AuthenticationController:credentialsChanged', + listener, + ); + + controller.performSignOut(); + + expect(listener).not.toHaveBeenCalled(); + }); + + it('rejects MFA calls while the wallet is locked', async () => { + const { controller, baseMessenger } = createController(); + baseMessenger.publish('KeyringController:lock'); + + await expect(controller.refreshEnrolledCredentials()).rejects.toThrow( + 'wallet is locked', + ); + await expect( + controller.beginCredentialEnrollment({ + type: 'passkey', + reason: { operation: 'settings.addPasskey' }, + }), + ).rejects.toThrow('wallet is locked'); + }); + + it('automatically refreshes after sign-in only when enabled', async () => { + const enabledEndpoints = arrangeAuthAPIs(); + const enabled = createController({ + state: { ...defaultState }, + isMfaEnabled: () => true, + }).controller; + await enabled.performSignIn(); + expect(enabledEndpoints.mockMfaCredentialsUrl.isDone()).toBe(true); + + cleanAllNock(); + const disabledEndpoints = arrangeAuthAPIs(); + const disabled = createController({ + state: { ...defaultState }, + isMfaEnabled: () => false, + }).controller; + await disabled.performSignIn(); + expect(disabledEndpoints.mockMfaCredentialsUrl.isDone()).toBe(false); + }); + + it('refreshes after unlock when signed in and enabled', async () => { + mockEndpointMfaCredentials(); + const { controller, baseMessenger } = createController({ + isMfaEnabled: () => true, + }); + const changed = new Promise((resolve) => { + baseMessenger.subscribe( + 'AuthenticationController:credentialsChanged', + () => resolve(), + ); + }); + + baseMessenger.publish('KeyringController:unlock'); + await changed; + + expect(controller.state.enrolledCredentials).toHaveLength(2); + }); + + it('does not fail sign-in when automatic refresh fails', async () => { + arrangeAuthAPIs({ + mockMfaCredentialsUrl: { + status: 502, + body: { code: 'kratos_unavailable', message: 'Unavailable' }, + }, + }); + const { controller } = createController({ + state: { ...defaultState }, + isMfaEnabled: () => true, + }); + + expect(await controller.performSignIn()).toHaveLength(2); + expect(controller.state.isSignedIn).toBe(true); + }); +}); + describe('metadata', () => { it('includes expected state in debug snapshots', () => { const controller = new AuthenticationController({ @@ -2331,6 +2650,39 @@ describe('metadata', () => { }); describe('includeInStateLogs', () => { + it('redacts enrolled email addresses', () => { + const controller = new AuthenticationController({ + messenger: createMockAuthenticationMessenger().messenger, + metametrics: createMockAuthMetaMetrics(), + state: { + ...mockSignedInState(), + enrolledCredentials: [ + { + type: 'email_otp', + status: 'active', + email: 'jane@example.com', + verified: true, + }, + ], + }, + }); + + expect( + deriveStateFromMetadata( + controller.state, + controller.metadata, + 'includeInStateLogs', + ).enrolledCredentials, + ).toStrictEqual([ + { + type: 'email_otp', + status: 'active', + email: 'j••@example.com', + verified: true, + }, + ]); + }); + it('includes expected state in state logs, with access token stripped out', () => { const controller = new AuthenticationController({ messenger: createMockAuthenticationMessenger().messenger, @@ -2347,6 +2699,7 @@ describe('metadata', () => { expect(derivedState).toMatchInlineSnapshot(` { + "enrolledCredentials": [], "isSignedIn": true, "needsProfilePairing": false, "needsSocialPairing": false, @@ -2394,6 +2747,7 @@ describe('metadata', () => { ), ).toMatchInlineSnapshot(` { + "enrolledCredentials": [], "isSignedIn": false, "needsProfilePairing": true, "needsSocialPairing": true, @@ -2465,6 +2819,7 @@ describe('metadata', () => { ), ).toMatchInlineSnapshot(` { + "enrolledCredentials": [], "isSignedIn": true, "needsProfilePairing": false, "needsSocialPairing": false, diff --git a/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.ts b/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.ts index 17b9b4f3ed4..3ab7bf2da87 100644 --- a/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.ts +++ b/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.ts @@ -5,6 +5,7 @@ import type { StateMetadata, } from '@metamask/base-controller'; import { selectHdKeyringEntropySourceIds } from '@metamask/keyring-controller'; +import type { TraceCallback } from '@metamask/controller-utils'; import type { KeyringControllerGetStateAction, KeyringControllerLockEvent, @@ -18,6 +19,11 @@ import type { } from '@metamask/seedless-onboarding-controller'; import type { Json } from '@metamask/utils'; +import { + BeginEnrollmentRequestStruct, + CompleteEnrollmentRequestStruct, + assertValidMfaRequest, +} from '../../sdk/authentication-jwt-bearer/mfa/schemas.js'; import type { LoginIdentifierType, LoginResponse, @@ -28,6 +34,10 @@ import type { UserProfileLineage, OidcTokenAudience, OidcTokenClaims, + BeginEnrollmentRequest, + CompleteEnrollmentRequest, + EnrolledCredential, + EnrollmentChallenge, } from '../../sdk/index.js'; import { assertMessageStartsWithMetamask, @@ -35,6 +45,7 @@ import { Env, JwtBearerAuth, PairConflictError, + getMfaErrorCode, } from '../../sdk/index.js'; import type { MetaMetricsAuth } from '../../shared/types/services.js'; import { getPrimaryHdKeyringEntropySourceId } from '../../shared/utils/entropy-source.js'; @@ -47,10 +58,23 @@ import { AuthenticationControllerMethodActions } from './AuthenticationControlle const controllerName = 'AuthenticationController'; +const defaultTrace = (( + _request: Parameters[0], + fn?: () => ReturnValue, +): Promise => + Promise.resolve(fn?.()) as Promise) as TraceCallback; + // State export type AuthenticationControllerState = { isSignedIn: boolean; srpSessionData?: Record; + /** + * Credentials fetched for the current profile. The controller always seeds + * this to an empty array; it remains optional so partial-state selectors stay + * assignable to the controller state type. + */ + enrolledCredentials?: EnrolledCredential[]; + stepUpSessionExpiresAt?: number; /** * Client gate for profile pairing. Defaults to `true` (fresh install / * upgrade), set to `false` after a successful `performSignIn` pair, set @@ -79,6 +103,7 @@ export type AuthenticationControllerState = { }; export const defaultState: AuthenticationControllerState = { isSignedIn: false, + enrolledCredentials: [], needsProfilePairing: true, needsSocialPairing: true, }; @@ -129,6 +154,34 @@ const metadata: StateMetadata = { includeInDebugSnapshot: false, usedInUi: true, }, + enrolledCredentials: { + includeInStateLogs: (credentials) => { + if (!credentials) { + return null; + } + return credentials.map((credential) => { + if (credential.type !== 'email_otp') { + return credential; + } + const separatorIndex = credential.email.indexOf('@'); + const domain = + separatorIndex >= 0 ? credential.email.slice(separatorIndex) : ''; + return { + ...credential, + email: `${credential.email.slice(0, 1)}••${domain}`, + }; + }); + }, + persist: false, + includeInDebugSnapshot: false, + usedInUi: true, + }, + stepUpSessionExpiresAt: { + includeInStateLogs: false, + persist: false, + includeInDebugSnapshot: false, + usedInUi: true, + }, }; type ControllerConfig = { @@ -139,8 +192,12 @@ type ControllerConfig = { * `() => false`. */ isSocialPairingEnabled: () => boolean; + isMfaEnabled: () => boolean; + stepUpSessionTtlMs: number; }; +export const STEP_UP_SESSION_TTL_MS = 60_000; + const MESSENGER_EXPOSED_METHODS = [ 'performSignIn', 'performSignOut', @@ -153,6 +210,9 @@ const MESSENGER_EXPOSED_METHODS = [ 'isSignedIn', 'requestProfilePairing', 'clearState', + 'refreshEnrolledCredentials', + 'beginCredentialEnrollment', + 'completeCredentialEnrollment', ] as const; export type Actions = @@ -181,9 +241,21 @@ export type AuthenticationControllerProfileSignInEvent = { payload: [ProfileSignInInfo]; }; +export type AuthenticationControllerCredentialsChangedEvent = { + type: `${typeof controllerName}:credentialsChanged`; + payload: [{ credentials: EnrolledCredential[] }]; +}; + +export type AuthenticationControllerStepUpSessionEvent = { + type: `${typeof controllerName}:stepUpSession`; + payload: [{ active: boolean; expiresAt: number | null }]; +}; + export type Events = | AuthenticationControllerStateChangeEvent - | AuthenticationControllerProfileSignInEvent; + | AuthenticationControllerProfileSignInEvent + | AuthenticationControllerCredentialsChangedEvent + | AuthenticationControllerStepUpSessionEvent; // Allowed Actions type AllowedActions = @@ -214,9 +286,13 @@ export class AuthenticationController extends BaseController< readonly #auth: SRPInterface; + readonly #trace: TraceCallback; + readonly #config: ControllerConfig = { env: Env.PRD, isSocialPairingEnabled: () => false, + isMfaEnabled: () => false, + stepUpSessionTtlMs: STEP_UP_SESSION_TTL_MS, }; #isUnlocked = false; @@ -234,10 +310,14 @@ export class AuthenticationController extends BaseController< this.messenger.subscribe('KeyringController:unlock', () => { this.#isUnlocked = true; + if (this.state.isSignedIn && this.#config.isMfaEnabled()) { + this.refreshEnrolledCredentials().catch(() => undefined); + } }); this.messenger.subscribe('KeyringController:lock', () => { this.#isUnlocked = false; + this.#clearEnrolledCredentials(); }); }, }; @@ -247,6 +327,7 @@ export class AuthenticationController extends BaseController< state, config, metametrics, + trace, }: { messenger: AuthenticationControllerMessenger; state?: AuthenticationControllerState; @@ -256,6 +337,7 @@ export class AuthenticationController extends BaseController< * do not want to tie this strictly to extension */ metametrics: MetaMetricsAuth; + trace?: TraceCallback; }) { super({ messenger, @@ -273,9 +355,11 @@ export class AuthenticationController extends BaseController< ...config, isSocialPairingEnabled: config?.isSocialPairingEnabled ?? this.#config.isSocialPairingEnabled, + isMfaEnabled: config?.isMfaEnabled ?? this.#config.isMfaEnabled, }; this.#metametrics = metametrics; + this.#trace = trace ?? defaultTrace; this.#auth = new JwtBearerAuth( { @@ -500,6 +584,10 @@ export class AuthenticationController extends BaseController< // noop } + if (this.#config.isMfaEnabled()) { + await this.refreshEnrolledCredentials().catch(() => undefined); + } + return accessTokens; } @@ -727,7 +815,149 @@ export class AuthenticationController extends BaseController< ); } + async #traceMfaRequest( + name: string, + operation: string, + credentialType: string, + fn: () => Promise, + ): Promise { + const data: Record = { outcome: 'pending' }; + return await this.#trace( + { + name, + tags: { operation, credentialType }, + data, + }, + async () => { + try { + const result = await fn(); + data.outcome = 'success'; + return result; + } catch (error) { + data.outcome = 'error'; + const mfaCode = getMfaErrorCode(error); + if (mfaCode) { + data.mfaCode = mfaCode; + } + throw error; + } + }, + ); + } + + /** + * Refreshes credentials enrolled on the canonical profile. + * + * @returns The current supported credentials. + */ + public async refreshEnrolledCredentials(): Promise { + this.#assertIsUnlocked('refreshEnrolledCredentials'); + const primaryEntropySourceId = this.#getPrimaryEntropySourceId(); + const credentials = await this.#traceMfaRequest( + 'MFA Credentials Refresh', + 'credentials.refresh', + 'all', + async () => await this.#auth.getMfaCredentials(primaryEntropySourceId), + ); + + if ( + JSON.stringify(credentials) !== + JSON.stringify(this.state.enrolledCredentials ?? []) + ) { + this.update((state) => { + state.enrolledCredentials = credentials; + }); + this.messenger.publish('AuthenticationController:credentialsChanged', { + credentials, + }); + } + return credentials; + } + + /** + * Begins enrollment of a passkey or email OTP credential. + * + * @param request - Credential, optional email address, and trace reason. + * @returns A challenge for the client-owned ceremony. + */ + public async beginCredentialEnrollment( + request: BeginEnrollmentRequest, + ): Promise { + this.#assertIsUnlocked('beginCredentialEnrollment'); + assertValidMfaRequest(request, BeginEnrollmentRequestStruct); + const primaryEntropySourceId = this.#getPrimaryEntropySourceId(); + return await this.#traceMfaRequest( + 'MFA Enroll Begin', + request.reason.operation, + request.type, + async () => + await this.#auth.beginMfaEnrollment( + request.type, + request.email, + primaryEntropySourceId, + ), + ); + } + + /** + * Completes credential enrollment and refreshes the credential cache. + * + * A cache-refresh failure does not undo successful enrollment. Email + * enrollment invalidates the primary SRP session so its next token includes + * the newly verified email claim. + * + * @param request - Flow identifier and platform or email proof. + * @returns The refreshed credentials, or the existing cache if refresh fails. + */ + public async completeCredentialEnrollment( + request: CompleteEnrollmentRequest, + ): Promise { + this.#assertIsUnlocked('completeCredentialEnrollment'); + assertValidMfaRequest(request, CompleteEnrollmentRequestStruct); + const primaryEntropySourceId = this.#getPrimaryEntropySourceId(); + await this.#traceMfaRequest( + 'MFA Enroll Complete', + 'credentials.enroll.complete', + request.type, + async () => + await this.#auth.completeMfaEnrollment( + request.type, + request.flowId, + request.proof, + primaryEntropySourceId, + ), + ); + + if (request.type === 'email_otp') { + this.#invalidateSrpSession(primaryEntropySourceId); + } + + try { + return await this.refreshEnrolledCredentials(); + } catch { + return this.state.enrolledCredentials ?? []; + } + } + + /** + * Drops the cached credential list and notifies listeners. Callers that wipe + * profile state must go through this so subscribers never keep credentials + * belonging to a profile that is no longer active. + */ + #clearEnrolledCredentials(): void { + if ((this.state.enrolledCredentials?.length ?? 0) === 0) { + return; + } + this.update((state) => { + state.enrolledCredentials = []; + }); + this.messenger.publish('AuthenticationController:credentialsChanged', { + credentials: [], + }); + } + public performSignOut(): void { + this.#clearEnrolledCredentials(); this.update((state) => { state.isSignedIn = false; state.srpSessionData = undefined; @@ -740,6 +970,7 @@ export class AuthenticationController extends BaseController< */ public clearState(): void { this.#profilePairingRequestEpoch += 1; + this.#clearEnrolledCredentials(); this.update(() => ({ ...defaultState })); } diff --git a/packages/profile-sync-controller/src/controllers/authentication/__fixtures__/mockServices.ts b/packages/profile-sync-controller/src/controllers/authentication/__fixtures__/mockServices.ts index 9b47a8abe76..3a41c560f76 100644 --- a/packages/profile-sync-controller/src/controllers/authentication/__fixtures__/mockServices.ts +++ b/packages/profile-sync-controller/src/controllers/authentication/__fixtures__/mockServices.ts @@ -1,5 +1,17 @@ import nock from 'nock'; +import { + MOCK_MFA_CREDENTIALS_RESPONSE, + MOCK_MFA_CREDENTIALS_URL, + MOCK_MFA_ENROLL_COMPLETE_RESPONSE, + MOCK_MFA_ENROLL_COMPLETE_URL, + MOCK_MFA_ENROLL_PASSKEY_RESPONSE, + MOCK_MFA_ENROLL_URL, + MOCK_MFA_VERIFY_COMPLETE_RESPONSE, + MOCK_MFA_VERIFY_COMPLETE_URL, + MOCK_MFA_VERIFY_PASSKEY_RESPONSE, + MOCK_MFA_VERIFY_URL, +} from '../../../sdk/mocks/auth.js'; import { getMockAuthAccessTokenResponse, getMockAuthLoginResponse, @@ -11,7 +23,7 @@ type MockReply = { body?: nock.Body; }; -export const mockEndpointGetNonce = (mockReply?: MockReply) => { +export const mockEndpointGetNonce = (mockReply?: MockReply): nock.Scope => { const mockResponse = getMockAuthNonceResponse(); const reply = mockReply ?? { status: 200, body: mockResponse.response }; const mockNonceEndpoint = nock(mockResponse.url) @@ -23,7 +35,7 @@ export const mockEndpointGetNonce = (mockReply?: MockReply) => { return mockNonceEndpoint; }; -export const mockEndpointLogin = (mockReply?: MockReply) => { +export const mockEndpointLogin = (mockReply?: MockReply): nock.Scope => { const mockResponse = getMockAuthLoginResponse(); const reply = mockReply ?? { status: 200, body: mockResponse.response }; const mockLoginEndpoint = nock(mockResponse.url) @@ -34,7 +46,7 @@ export const mockEndpointLogin = (mockReply?: MockReply) => { return mockLoginEndpoint; }; -export const mockEndpointAccessToken = (mockReply?: MockReply) => { +export const mockEndpointAccessToken = (mockReply?: MockReply): nock.Scope => { const mockResponse = getMockAuthAccessTokenResponse(); const reply = mockReply ?? { status: 200, body: mockResponse.response }; const mockOidcTokensEndpoint = nock(mockResponse.url) @@ -44,3 +56,64 @@ export const mockEndpointAccessToken = (mockReply?: MockReply) => { return mockOidcTokensEndpoint; }; + +export const mockEndpointMfaEnroll = (mockReply?: MockReply): nock.Scope => { + const reply = mockReply ?? { + status: 200, + body: MOCK_MFA_ENROLL_PASSKEY_RESPONSE, + }; + return nock(MOCK_MFA_ENROLL_URL) + .persist() + .post('') + .reply(reply.status, reply.body); +}; + +export const mockEndpointMfaEnrollComplete = ( + mockReply?: MockReply, +): nock.Scope => { + const reply = mockReply ?? { + status: 200, + body: MOCK_MFA_ENROLL_COMPLETE_RESPONSE, + }; + return nock(MOCK_MFA_ENROLL_COMPLETE_URL) + .persist() + .post('') + .reply(reply.status, reply.body); +}; + +export const mockEndpointMfaVerify = (mockReply?: MockReply): nock.Scope => { + const reply = mockReply ?? { + status: 200, + body: MOCK_MFA_VERIFY_PASSKEY_RESPONSE, + }; + return nock(MOCK_MFA_VERIFY_URL) + .persist() + .post('') + .reply(reply.status, reply.body); +}; + +export const mockEndpointMfaVerifyComplete = ( + mockReply?: MockReply, +): nock.Scope => { + const reply = mockReply ?? { + status: 200, + body: MOCK_MFA_VERIFY_COMPLETE_RESPONSE, + }; + return nock(MOCK_MFA_VERIFY_COMPLETE_URL) + .persist() + .post('') + .reply(reply.status, reply.body); +}; + +export const mockEndpointMfaCredentials = ( + mockReply?: MockReply, +): nock.Scope => { + const reply = mockReply ?? { + status: 200, + body: MOCK_MFA_CREDENTIALS_RESPONSE, + }; + return nock(MOCK_MFA_CREDENTIALS_URL) + .persist() + .get('') + .reply(reply.status, reply.body); +}; diff --git a/packages/profile-sync-controller/src/controllers/authentication/index.ts b/packages/profile-sync-controller/src/controllers/authentication/index.ts index 114f26b6816..8887e1c3d69 100644 --- a/packages/profile-sync-controller/src/controllers/authentication/index.ts +++ b/packages/profile-sync-controller/src/controllers/authentication/index.ts @@ -16,4 +16,7 @@ export type { AuthenticationControllerRequestProfilePairingAction, AuthenticationControllerGetPartnerIdentityTokenAction, AuthenticationControllerClearStateAction, + AuthenticationControllerRefreshEnrolledCredentialsAction, + AuthenticationControllerBeginCredentialEnrollmentAction, + AuthenticationControllerCompleteCredentialEnrollmentAction, } from './AuthenticationController-method-action-types.js'; diff --git a/packages/profile-sync-controller/src/controllers/authentication/mocks/mockResponses.test.ts b/packages/profile-sync-controller/src/controllers/authentication/mocks/mockResponses.test.ts index a847afbce3c..0f6a4b1318c 100644 --- a/packages/profile-sync-controller/src/controllers/authentication/mocks/mockResponses.test.ts +++ b/packages/profile-sync-controller/src/controllers/authentication/mocks/mockResponses.test.ts @@ -6,6 +6,11 @@ import { MOCK_CUSTOMER_SERVICE_TOKEN_RESPONSE, MOCK_PARTNER_IDENTITY_TOKEN_RESPONSE, MOCK_OATH_TOKEN_RESPONSE, + getMockMfaCredentialsResponse, + getMockMfaEnrollCompleteResponse, + getMockMfaEnrollResponse, + getMockMfaVerifyCompleteResponse, + getMockMfaVerifyResponse, } from './mockResponses.js'; describe('getE2EIdentifierFromJwt()', () => { @@ -80,6 +85,24 @@ describe('getMockAuthAccessTokenResponse()', () => { }); }); +describe('MFA mock responses', () => { + it('provides endpoint-compatible response descriptors', () => { + expect([ + getMockMfaEnrollResponse(), + getMockMfaEnrollCompleteResponse(), + getMockMfaVerifyResponse(), + getMockMfaVerifyCompleteResponse(), + getMockMfaCredentialsResponse(), + ]).toStrictEqual([ + expect.objectContaining({ requestMethod: 'POST' }), + expect.objectContaining({ requestMethod: 'POST' }), + expect.objectContaining({ requestMethod: 'POST' }), + expect.objectContaining({ requestMethod: 'POST' }), + expect.objectContaining({ requestMethod: 'GET' }), + ]); + }); +}); + describe('getMockCustomerServiceTokenResponse()', () => { it('returns a POST mock for the customer service token endpoint', () => { const mock = getMockCustomerServiceTokenResponse(); diff --git a/packages/profile-sync-controller/src/controllers/authentication/mocks/mockResponses.ts b/packages/profile-sync-controller/src/controllers/authentication/mocks/mockResponses.ts index ded0a3f352e..eba446db71e 100644 --- a/packages/profile-sync-controller/src/controllers/authentication/mocks/mockResponses.ts +++ b/packages/profile-sync-controller/src/controllers/authentication/mocks/mockResponses.ts @@ -14,6 +14,16 @@ import { MOCK_PAIR_SOCIAL_IDENTIFIER_URL, MOCK_CUSTOMER_SERVICE_TOKEN_URL, MOCK_PARTNER_IDENTITY_TOKEN_URL, + MOCK_MFA_CREDENTIALS_RESPONSE as SDK_MOCK_MFA_CREDENTIALS_RESPONSE, + MOCK_MFA_CREDENTIALS_URL, + MOCK_MFA_ENROLL_COMPLETE_RESPONSE as SDK_MOCK_MFA_ENROLL_COMPLETE_RESPONSE, + MOCK_MFA_ENROLL_COMPLETE_URL, + MOCK_MFA_ENROLL_PASSKEY_RESPONSE as SDK_MOCK_MFA_ENROLL_PASSKEY_RESPONSE, + MOCK_MFA_ENROLL_URL, + MOCK_MFA_VERIFY_COMPLETE_RESPONSE as SDK_MOCK_MFA_VERIFY_COMPLETE_RESPONSE, + MOCK_MFA_VERIFY_COMPLETE_URL, + MOCK_MFA_VERIFY_PASSKEY_RESPONSE as SDK_MOCK_MFA_VERIFY_PASSKEY_RESPONSE, + MOCK_MFA_VERIFY_URL, } from '../../../sdk/mocks/auth.js'; type MockResponse = { @@ -185,3 +195,33 @@ export const getMockPartnerIdentityTokenResponse = (): MockResponse => { response: MOCK_PARTNER_IDENTITY_TOKEN_RESPONSE, } satisfies MockResponse; }; + +export const getMockMfaEnrollResponse = (): MockResponse => ({ + url: MOCK_MFA_ENROLL_URL, + requestMethod: 'POST', + response: SDK_MOCK_MFA_ENROLL_PASSKEY_RESPONSE, +}); + +export const getMockMfaEnrollCompleteResponse = (): MockResponse => ({ + url: MOCK_MFA_ENROLL_COMPLETE_URL, + requestMethod: 'POST', + response: SDK_MOCK_MFA_ENROLL_COMPLETE_RESPONSE, +}); + +export const getMockMfaVerifyResponse = (): MockResponse => ({ + url: MOCK_MFA_VERIFY_URL, + requestMethod: 'POST', + response: SDK_MOCK_MFA_VERIFY_PASSKEY_RESPONSE, +}); + +export const getMockMfaVerifyCompleteResponse = (): MockResponse => ({ + url: MOCK_MFA_VERIFY_COMPLETE_URL, + requestMethod: 'POST', + response: SDK_MOCK_MFA_VERIFY_COMPLETE_RESPONSE, +}); + +export const getMockMfaCredentialsResponse = (): MockResponse => ({ + url: MOCK_MFA_CREDENTIALS_URL, + requestMethod: 'GET', + response: SDK_MOCK_MFA_CREDENTIALS_RESPONSE, +}); diff --git a/packages/profile-sync-controller/tsconfig.json b/packages/profile-sync-controller/tsconfig.json index 096b2d227ea..6542f7e1bf9 100644 --- a/packages/profile-sync-controller/tsconfig.json +++ b/packages/profile-sync-controller/tsconfig.json @@ -4,6 +4,9 @@ { "path": "../base-controller" }, + { + "path": "../controller-utils" + }, { "path": "../keyring-controller" }, diff --git a/yarn.lock b/yarn.lock index e4a60f4fe3d..83c44d09f9f 100644 --- a/yarn.lock +++ b/yarn.lock @@ -8646,6 +8646,7 @@ __metadata: "@metamask/address-book-controller": "npm:^8.0.0" "@metamask/auto-changelog": "npm:^6.1.0" "@metamask/base-controller": "npm:^10.0.0" + "@metamask/controller-utils": "npm:^13.0.0" "@metamask/eth-hd-keyring": "npm:^15.0.0" "@metamask/key-tree": "npm:^10.1.1" "@metamask/keyring-api": "npm:^24.0.0" From c416b85478027ccc3a2246c823eec75671a0d25a Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Wed, 16 Sep 2026 16:24:56 +0200 Subject: [PATCH 02/13] fix(profile-sync): keep MFA credential refresh on the current token Refresh enrolled credentials before invalidating the SRP session after email enrollment, and drop in-flight refresh results if the wallet locks. --- .../AuthenticationController.test.ts | 47 ++++++++++++++++++- .../AuthenticationController.ts | 19 +++++--- .../tsconfig.build.json | 3 ++ .../tsconfig.lint.json | 3 ++ 4 files changed, 64 insertions(+), 8 deletions(-) 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 b017c45c8db..b50ddb189dc 100644 --- a/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.test.ts +++ b/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.test.ts @@ -2453,7 +2453,7 @@ describe('MFA credential enrollment', () => { it('invalidates the primary SRP session after email enrollment', async () => { mockEndpointMfaEnrollComplete(); - mockEndpointMfaCredentials(); + const credentialsScope = mockEndpointMfaCredentials(); const { controller } = createController(); await controller.completeCredentialEnrollment({ @@ -2462,12 +2462,57 @@ describe('MFA credential enrollment', () => { proof: { type: 'email_otp', code: '123456' }, }); + expect(credentialsScope.isDone()).toBe(true); expect( controller.state.srpSessionData?.[MOCK_ENTROPY_SOURCE_IDS[0]].profile .canonicalProfileId, ).toBe(''); }); + it('does not restore credentials if the wallet locks during refresh', async () => { + let release!: (value: Awaited>) => void; + let requestStartedResolve: (() => void) | undefined; + const requestStarted = new Promise((resolve) => { + requestStartedResolve = resolve; + }); + const fetchSpy = jest.spyOn(globalThis, 'fetch').mockImplementationOnce( + async (): ReturnType => + await new Promise((resolve) => { + release = resolve; + requestStartedResolve?.(); + }), + ); + const { controller, baseMessenger } = createController({ + state: { + ...mockSignedInState(), + enrolledCredentials: [], + }, + }); + const listener = jest.fn(); + baseMessenger.subscribe( + 'AuthenticationController:credentialsChanged', + listener, + ); + + try { + const refresh = controller.refreshEnrolledCredentials(); + await requestStarted; + baseMessenger.publish('KeyringController:lock'); + release( + new globalThis.Response( + JSON.stringify(MOCK_MFA_CREDENTIALS_RESPONSE), + { status: 200 }, + ), + ); + + await expect(refresh).rejects.toThrow('wallet is locked'); + expect(controller.state.enrolledCredentials).toStrictEqual([]); + expect(listener).not.toHaveBeenCalled(); + } finally { + fetchSpy.mockRestore(); + } + }); + it('keeps the existing cache when post-enrollment refresh fails', async () => { mockEndpointMfaEnrollComplete(); mockEndpointMfaCredentials({ diff --git a/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.ts b/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.ts index 3ab7bf2da87..82b61823026 100644 --- a/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.ts +++ b/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.ts @@ -859,6 +859,7 @@ export class AuthenticationController extends BaseController< 'all', async () => await this.#auth.getMfaCredentials(primaryEntropySourceId), ); + this.#assertIsUnlocked('refreshEnrolledCredentials'); if ( JSON.stringify(credentials) !== @@ -903,8 +904,9 @@ export class AuthenticationController extends BaseController< * Completes credential enrollment and refreshes the credential cache. * * A cache-refresh failure does not undo successful enrollment. Email - * enrollment invalidates the primary SRP session so its next token includes - * the newly verified email claim. + * enrollment invalidates the primary SRP session *after* refresh so the + * credentials call can reuse the still-valid access token; the next token + * fetch then includes the newly verified email claim. * * @param request - Flow identifier and platform or email proof. * @returns The refreshed credentials, or the existing cache if refresh fails. @@ -928,13 +930,16 @@ export class AuthenticationController extends BaseController< ), ); - if (request.type === 'email_otp') { - this.#invalidateSrpSession(primaryEntropySourceId); - } - try { - return await this.refreshEnrolledCredentials(); + const credentials = await this.refreshEnrolledCredentials(); + if (request.type === 'email_otp') { + this.#invalidateSrpSession(primaryEntropySourceId); + } + return credentials; } catch { + if (request.type === 'email_otp') { + this.#invalidateSrpSession(primaryEntropySourceId); + } return this.state.enrolledCredentials ?? []; } } diff --git a/packages/profile-sync-controller/tsconfig.build.json b/packages/profile-sync-controller/tsconfig.build.json index 24f6ea448d3..220af8e2b65 100644 --- a/packages/profile-sync-controller/tsconfig.build.json +++ b/packages/profile-sync-controller/tsconfig.build.json @@ -23,6 +23,9 @@ }, { "path": "../utils/tsconfig.build.json" + }, + { + "path": "../controller-utils/tsconfig.build.json" } ], "include": ["../../types", "./src"], diff --git a/packages/profile-sync-controller/tsconfig.lint.json b/packages/profile-sync-controller/tsconfig.lint.json index 24ccc04a5cf..0a9b3c044c2 100644 --- a/packages/profile-sync-controller/tsconfig.lint.json +++ b/packages/profile-sync-controller/tsconfig.lint.json @@ -22,6 +22,9 @@ }, { "path": "../utils/tsconfig.lint.json" + }, + { + "path": "../controller-utils/tsconfig.lint.json" } ] } From f34db23c017b38669a42fa4d11a905371fd0e1c3 Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Wed, 16 Sep 2026 16:45:35 +0200 Subject: [PATCH 03/13] fix(profile-sync): sync generated action types and formatting Co-authored-by: Cursor --- .../AuthenticationController-method-action-types.ts | 5 +++-- .../authentication/AuthenticationController.test.ts | 7 +++---- 2 files changed, 6 insertions(+), 6 deletions(-) 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 3b5481a59b5..7daaf901471 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 @@ -45,8 +45,9 @@ export type AuthenticationControllerBeginCredentialEnrollmentAction = { * Completes credential enrollment and refreshes the credential cache. * * A cache-refresh failure does not undo successful enrollment. Email - * enrollment invalidates the primary SRP session so its next token includes - * the newly verified email claim. + * enrollment invalidates the primary SRP session *after* refresh so the + * credentials call can reuse the still-valid access token; the next token + * fetch then includes the newly verified email claim. * * @param request - Flow identifier and platform or email proof. * @returns The refreshed credentials, or the existing cache if refresh fails. 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 b50ddb189dc..30d7f89c934 100644 --- a/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.test.ts +++ b/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.test.ts @@ -2499,10 +2499,9 @@ describe('MFA credential enrollment', () => { await requestStarted; baseMessenger.publish('KeyringController:lock'); release( - new globalThis.Response( - JSON.stringify(MOCK_MFA_CREDENTIALS_RESPONSE), - { status: 200 }, - ), + new globalThis.Response(JSON.stringify(MOCK_MFA_CREDENTIALS_RESPONSE), { + status: 200, + }), ); await expect(refresh).rejects.toThrow('wallet is locked'); From b8cc7461cba3c21496159ce431696e4c242415ba Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Wed, 16 Sep 2026 21:48:21 +0200 Subject: [PATCH 04/13] fix(profile-sync): simplify AuthenticationController enrollment layer - Drop the credentialsChanged event; stateChange already carries the list - Move step-up state, config and event scaffolding to the step-up layer - Derive the completion credential type from the proof; trace reason.operation - Invalidate the SRP session once via finally after email enrollment - Make the post-sign-in credential warm-up non-blocking; drop unlock refresh - Inline the default trace callback Co-authored-by: Cursor --- packages/profile-sync-controller/CHANGELOG.md | 2 +- ...nticationController-method-action-types.ts | 2 +- .../AuthenticationController.test.ts | 159 ++++++++++-------- .../AuthenticationController.ts | 97 +++++------ 4 files changed, 126 insertions(+), 134 deletions(-) diff --git a/packages/profile-sync-controller/CHANGELOG.md b/packages/profile-sync-controller/CHANGELOG.md index 09fd20dccf6..01ecea791f0 100644 --- a/packages/profile-sync-controller/CHANGELOG.md +++ b/packages/profile-sync-controller/CHANGELOG.md @@ -9,7 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added -- Add credential enrollment and refresh methods, in-memory credential state, change events, automatic refresh configuration, and tracing to `AuthenticationController` ([#10266](https://github.com/MetaMask/core/pull/10266)) +- Add `refreshEnrolledCredentials`, `beginCredentialEnrollment` and `completeCredentialEnrollment` to `AuthenticationController`, backed by a memory-only `enrolledCredentials` state, an `isMfaEnabled` config gate for post-sign-in refresh, and an optional `trace` callback ([#10266](https://github.com/MetaMask/core/pull/10266)) - Add passkey and email OTP enrollment, verification, credential-list, and elevated-token exchange SDK methods ([#10265](https://github.com/MetaMask/core/pull/10265)) - Add validated MFA domain types and structured `MfaError` classes with a serialization-safe `mfaCode` ([#10264](https://github.com/MetaMask/core/pull/10264)) - Add `rampsOrders` to `USER_STORAGE_FEATURE_NAMES` ([#10227](https://github.com/MetaMask/core/pull/10227)) 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 7daaf901471..874b07b9723 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 @@ -49,7 +49,7 @@ export type AuthenticationControllerBeginCredentialEnrollmentAction = { * credentials call can reuse the still-valid access token; the next token * fetch then includes the newly verified email claim. * - * @param request - Flow identifier and platform or email proof. + * @param request - Flow identifier, platform or email proof, and trace reason. * @returns The refreshed credentials, or the existing cache if refresh fails. */ export type AuthenticationControllerCompleteCredentialEnrollmentAction = { 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 30d7f89c934..2f6815adcd6 100644 --- a/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.test.ts +++ b/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.test.ts @@ -2340,22 +2340,37 @@ describe('MFA credential enrollment', () => { }; } + /** + * Resolves once `enrolledCredentials` is written with a non-empty list. + * + * @param baseMessenger - Root messenger to observe. + * @returns A promise settled by the next credential-bearing state change. + */ + function waitForCredentials(baseMessenger: RootMessenger): Promise { + return new Promise((resolve) => { + baseMessenger.subscribe( + 'AuthenticationController:stateChange', + (state: AuthenticationControllerState) => { + if ((state.enrolledCredentials?.length ?? 0) > 0) { + resolve(); + } + }, + ); + }); + } + it('starts with an empty memory-only credential cache', () => { const { controller } = createController({ state: { ...defaultState }, }); expect(controller.state.enrolledCredentials).toStrictEqual([]); - expect(controller.state.stepUpSessionExpiresAt).toBeUndefined(); }); - it('refreshes credentials and publishes changes only when data changes', async () => { + it('refreshes credentials and writes state only when data changes', async () => { mockEndpointMfaCredentials(); const { controller, baseMessenger } = createController(); const listener = jest.fn(); - baseMessenger.subscribe( - 'AuthenticationController:credentialsChanged', - listener, - ); + baseMessenger.subscribe('AuthenticationController:stateChange', listener); expect(await controller.refreshEnrolledCredentials()).toHaveLength(2); expect(controller.state.enrolledCredentials).toHaveLength(2); @@ -2413,7 +2428,6 @@ describe('MFA credential enrollment', () => { type: 'email_otp', flowId: 'enroll-email-flow-id', expiresAt: Date.parse('2099-09-07T14:30:00Z'), - emailSent: true, }); await expect( controller.beginCredentialEnrollment({ @@ -2421,15 +2435,25 @@ describe('MFA credential enrollment', () => { reason: { operation: 'invalid operation' }, }), ).rejects.toMatchObject({ mfaCode: 'invalid_request' }); + await expect( + controller.beginCredentialEnrollment({ + type: 'passkey', + email: 'user@example.com', + reason: { operation: 'settings.addPasskey' }, + }), + ).rejects.toMatchObject({ mfaCode: 'invalid_request' }); }); - it('completes passkey enrollment and refreshes the cache', async () => { + it('completes passkey enrollment, refreshes the cache and traces the caller operation', async () => { mockEndpointMfaEnrollComplete(); mockEndpointMfaCredentials(); - const { controller } = createController(); + const trace = jest.fn( + (_request: unknown, fn?: () => unknown): Promise => + Promise.resolve(fn?.()), + ) as unknown as TraceCallback; + const { controller } = createController({ trace }); const credentials = await controller.completeCredentialEnrollment({ - type: 'passkey', flowId: 'flow-id', proof: { type: 'passkey', @@ -2443,12 +2467,28 @@ describe('MFA credential enrollment', () => { }, }, }, + reason: { operation: 'settings.addPasskey' }, }); expect(credentials).toHaveLength( MOCK_MFA_CREDENTIALS_RESPONSE.credentials.length, ); expect(controller.state.enrolledCredentials).toStrictEqual(credentials); + expect(trace).toHaveBeenCalledWith( + expect.objectContaining({ + name: 'MFA Enroll Complete', + tags: { + operation: 'settings.addPasskey', + credentialType: 'passkey', + }, + }), + expect.any(Function), + ); + // The primary SRP session is untouched: passkeys add no token claims. + expect( + controller.state.srpSessionData?.[MOCK_ENTROPY_SOURCE_IDS[0]].profile + .canonicalProfileId, + ).not.toBe(''); }); it('invalidates the primary SRP session after email enrollment', async () => { @@ -2457,9 +2497,9 @@ describe('MFA credential enrollment', () => { const { controller } = createController(); await controller.completeCredentialEnrollment({ - type: 'email_otp', flowId: 'flow-id', proof: { type: 'email_otp', code: '123456' }, + reason: { operation: 'settings.addEmail' }, }); expect(credentialsScope.isDone()).toBe(true); @@ -2488,12 +2528,6 @@ describe('MFA credential enrollment', () => { enrolledCredentials: [], }, }); - const listener = jest.fn(); - baseMessenger.subscribe( - 'AuthenticationController:credentialsChanged', - listener, - ); - try { const refresh = controller.refreshEnrolledCredentials(); await requestStarted; @@ -2506,7 +2540,6 @@ describe('MFA credential enrollment', () => { await expect(refresh).rejects.toThrow('wallet is locked'); expect(controller.state.enrolledCredentials).toStrictEqual([]); - expect(listener).not.toHaveBeenCalled(); } finally { fetchSpy.mockRestore(); } @@ -2534,11 +2567,16 @@ describe('MFA credential enrollment', () => { expect( await controller.completeCredentialEnrollment({ - type: 'email_otp', flowId: 'flow-id', proof: { type: 'email_otp', code: '123456' }, + reason: { operation: 'settings.addEmail' }, }), ).toStrictEqual(existing); + // The SRP session is still invalidated so the next token carries the claim. + expect( + controller.state.srpSessionData?.[MOCK_ENTROPY_SOURCE_IDS[0]].profile + .canonicalProfileId, + ).toBe(''); }); it.each([ @@ -2558,44 +2596,32 @@ describe('MFA credential enrollment', () => { baseMessenger: RootMessenger, ): void => baseMessenger.publish('KeyringController:lock'), ], - ])( - 'clears cached credentials and publishes the change on %s', - (_name, act) => { - const { controller, baseMessenger } = createController({ - state: { - ...mockSignedInState(), - enrolledCredentials: [ - { - type: 'email_otp', - status: 'active', - email: 'user@example.com', - verified: true, - }, - ], - }, - }); - const listener = jest.fn(); - baseMessenger.subscribe( - 'AuthenticationController:credentialsChanged', - listener, - ); + ])('clears cached credentials on %s', (_name, act) => { + const { controller, baseMessenger } = createController({ + state: { + ...mockSignedInState(), + enrolledCredentials: [ + { + type: 'email_otp', + status: 'active', + email: 'user@example.com', + verified: true, + }, + ], + }, + }); - act(controller, baseMessenger); + act(controller, baseMessenger); - expect(controller.state.enrolledCredentials).toStrictEqual([]); - expect(listener).toHaveBeenCalledWith({ credentials: [] }); - }, - ); + expect(controller.state.enrolledCredentials).toStrictEqual([]); + }); - it('does not publish a credential change when the cache is already empty', () => { - const { controller, baseMessenger } = createController(); + it('does not write state on lock when the cache is already empty', () => { + const { baseMessenger } = createController(); const listener = jest.fn(); - baseMessenger.subscribe( - 'AuthenticationController:credentialsChanged', - listener, - ); + baseMessenger.subscribe('AuthenticationController:stateChange', listener); - controller.performSignOut(); + baseMessenger.publish('KeyringController:lock'); expect(listener).not.toHaveBeenCalled(); }); @@ -2615,14 +2641,19 @@ describe('MFA credential enrollment', () => { ).rejects.toThrow('wallet is locked'); }); - it('automatically refreshes after sign-in only when enabled', async () => { + it('warms the credential cache after sign-in without blocking it, only when enabled', async () => { const enabledEndpoints = arrangeAuthAPIs(); const enabled = createController({ state: { ...defaultState }, isMfaEnabled: () => true, - }).controller; - await enabled.performSignIn(); + }); + const warmed = waitForCredentials(enabled.baseMessenger); + await enabled.controller.performSignIn(); + // Sign-in resolved before the credentials call had to complete. + expect(enabled.controller.state.isSignedIn).toBe(true); + await warmed; expect(enabledEndpoints.mockMfaCredentialsUrl.isDone()).toBe(true); + expect(enabled.controller.state.enrolledCredentials).toHaveLength(2); cleanAllNock(); const disabledEndpoints = arrangeAuthAPIs(); @@ -2634,24 +2665,6 @@ describe('MFA credential enrollment', () => { expect(disabledEndpoints.mockMfaCredentialsUrl.isDone()).toBe(false); }); - it('refreshes after unlock when signed in and enabled', async () => { - mockEndpointMfaCredentials(); - const { controller, baseMessenger } = createController({ - isMfaEnabled: () => true, - }); - const changed = new Promise((resolve) => { - baseMessenger.subscribe( - 'AuthenticationController:credentialsChanged', - () => resolve(), - ); - }); - - baseMessenger.publish('KeyringController:unlock'); - await changed; - - expect(controller.state.enrolledCredentials).toHaveLength(2); - }); - it('does not fail sign-in when automatic refresh fails', async () => { arrangeAuthAPIs({ mockMfaCredentialsUrl: { diff --git a/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.ts b/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.ts index 82b61823026..0b22eedecab 100644 --- a/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.ts +++ b/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.ts @@ -58,12 +58,6 @@ import { AuthenticationControllerMethodActions } from './AuthenticationControlle const controllerName = 'AuthenticationController'; -const defaultTrace = (( - _request: Parameters[0], - fn?: () => ReturnValue, -): Promise => - Promise.resolve(fn?.()) as Promise) as TraceCallback; - // State export type AuthenticationControllerState = { isSignedIn: boolean; @@ -74,7 +68,6 @@ export type AuthenticationControllerState = { * assignable to the controller state type. */ enrolledCredentials?: EnrolledCredential[]; - stepUpSessionExpiresAt?: number; /** * Client gate for profile pairing. Defaults to `true` (fresh install / * upgrade), set to `false` after a successful `performSignIn` pair, set @@ -176,12 +169,6 @@ const metadata: StateMetadata = { includeInDebugSnapshot: false, usedInUi: true, }, - stepUpSessionExpiresAt: { - includeInStateLogs: false, - persist: false, - includeInDebugSnapshot: false, - usedInUi: true, - }, }; type ControllerConfig = { @@ -192,12 +179,14 @@ type ControllerConfig = { * `() => false`. */ isSocialPairingEnabled: () => boolean; + /** + * When `true`, `performSignIn` refreshes `enrolledCredentials` after the + * session is established. MFA methods themselves are never gated; the + * client decides when to expose them. Defaults to `() => false`. + */ isMfaEnabled: () => boolean; - stepUpSessionTtlMs: number; }; -export const STEP_UP_SESSION_TTL_MS = 60_000; - const MESSENGER_EXPOSED_METHODS = [ 'performSignIn', 'performSignOut', @@ -241,21 +230,9 @@ export type AuthenticationControllerProfileSignInEvent = { payload: [ProfileSignInInfo]; }; -export type AuthenticationControllerCredentialsChangedEvent = { - type: `${typeof controllerName}:credentialsChanged`; - payload: [{ credentials: EnrolledCredential[] }]; -}; - -export type AuthenticationControllerStepUpSessionEvent = { - type: `${typeof controllerName}:stepUpSession`; - payload: [{ active: boolean; expiresAt: number | null }]; -}; - export type Events = | AuthenticationControllerStateChangeEvent - | AuthenticationControllerProfileSignInEvent - | AuthenticationControllerCredentialsChangedEvent - | AuthenticationControllerStepUpSessionEvent; + | AuthenticationControllerProfileSignInEvent; // Allowed Actions type AllowedActions = @@ -292,7 +269,6 @@ export class AuthenticationController extends BaseController< env: Env.PRD, isSocialPairingEnabled: () => false, isMfaEnabled: () => false, - stepUpSessionTtlMs: STEP_UP_SESSION_TTL_MS, }; #isUnlocked = false; @@ -310,9 +286,6 @@ export class AuthenticationController extends BaseController< this.messenger.subscribe('KeyringController:unlock', () => { this.#isUnlocked = true; - if (this.state.isSignedIn && this.#config.isMfaEnabled()) { - this.refreshEnrolledCredentials().catch(() => undefined); - } }); this.messenger.subscribe('KeyringController:lock', () => { @@ -359,7 +332,7 @@ export class AuthenticationController extends BaseController< }; this.#metametrics = metametrics; - this.#trace = trace ?? defaultTrace; + this.#trace = trace ?? (((_request, fn) => fn?.()) as TraceCallback); this.#auth = new JwtBearerAuth( { @@ -584,8 +557,10 @@ export class AuthenticationController extends BaseController< // noop } + // Best-effort warm-up of the credential cache. Not awaited: sign-in must + // not wait on the MFA service, and MFA flows refresh explicitly anyway. if (this.#config.isMfaEnabled()) { - await this.refreshEnrolledCredentials().catch(() => undefined); + this.refreshEnrolledCredentials().catch(() => undefined); } return accessTokens; @@ -815,7 +790,17 @@ export class AuthenticationController extends BaseController< ); } - async #traceMfaRequest( + /** + * Runs one MFA network step inside a trace span tagged with the caller's + * operation and the credential type, recording the outcome and MFA code. + * + * @param name - Span name. + * @param operation - Caller-supplied `reason.operation`. + * @param credentialType - Credential type the step concerns. + * @param fn - The network step. + * @returns The step's result. + */ + async #runMfaRequest( name: string, operation: string, credentialType: string, @@ -853,7 +838,7 @@ export class AuthenticationController extends BaseController< public async refreshEnrolledCredentials(): Promise { this.#assertIsUnlocked('refreshEnrolledCredentials'); const primaryEntropySourceId = this.#getPrimaryEntropySourceId(); - const credentials = await this.#traceMfaRequest( + const credentials = await this.#runMfaRequest( 'MFA Credentials Refresh', 'credentials.refresh', 'all', @@ -861,6 +846,8 @@ export class AuthenticationController extends BaseController< ); this.#assertIsUnlocked('refreshEnrolledCredentials'); + // Skip the write when nothing changed so subscribers are not woken up by + // a fresh-but-identical array. if ( JSON.stringify(credentials) !== JSON.stringify(this.state.enrolledCredentials ?? []) @@ -868,9 +855,6 @@ export class AuthenticationController extends BaseController< this.update((state) => { state.enrolledCredentials = credentials; }); - this.messenger.publish('AuthenticationController:credentialsChanged', { - credentials, - }); } return credentials; } @@ -887,7 +871,7 @@ export class AuthenticationController extends BaseController< this.#assertIsUnlocked('beginCredentialEnrollment'); assertValidMfaRequest(request, BeginEnrollmentRequestStruct); const primaryEntropySourceId = this.#getPrimaryEntropySourceId(); - return await this.#traceMfaRequest( + return await this.#runMfaRequest( 'MFA Enroll Begin', request.reason.operation, request.type, @@ -908,7 +892,7 @@ export class AuthenticationController extends BaseController< * credentials call can reuse the still-valid access token; the next token * fetch then includes the newly verified email claim. * - * @param request - Flow identifier and platform or email proof. + * @param request - Flow identifier, platform or email proof, and trace reason. * @returns The refreshed credentials, or the existing cache if refresh fails. */ public async completeCredentialEnrollment( @@ -916,14 +900,15 @@ export class AuthenticationController extends BaseController< ): Promise { this.#assertIsUnlocked('completeCredentialEnrollment'); assertValidMfaRequest(request, CompleteEnrollmentRequestStruct); + const { type } = request.proof; const primaryEntropySourceId = this.#getPrimaryEntropySourceId(); - await this.#traceMfaRequest( + await this.#runMfaRequest( 'MFA Enroll Complete', - 'credentials.enroll.complete', - request.type, + request.reason.operation, + type, async () => await this.#auth.completeMfaEnrollment( - request.type, + type, request.flowId, request.proof, primaryEntropySourceId, @@ -931,23 +916,20 @@ export class AuthenticationController extends BaseController< ); try { - const credentials = await this.refreshEnrolledCredentials(); - if (request.type === 'email_otp') { - this.#invalidateSrpSession(primaryEntropySourceId); - } - return credentials; + return await this.refreshEnrolledCredentials(); } catch { - if (request.type === 'email_otp') { + return this.state.enrolledCredentials ?? []; + } finally { + if (type === 'email_otp') { this.#invalidateSrpSession(primaryEntropySourceId); } - return this.state.enrolledCredentials ?? []; } } /** - * Drops the cached credential list and notifies listeners. Callers that wipe - * profile state must go through this so subscribers never keep credentials - * belonging to a profile that is no longer active. + * Drops the cached credential list. Callers that wipe profile state must go + * through this so subscribers never keep credentials belonging to a profile + * that is no longer active. */ #clearEnrolledCredentials(): void { if ((this.state.enrolledCredentials?.length ?? 0) === 0) { @@ -956,9 +938,6 @@ export class AuthenticationController extends BaseController< this.update((state) => { state.enrolledCredentials = []; }); - this.messenger.publish('AuthenticationController:credentialsChanged', { - credentials: [], - }); } public performSignOut(): void { From e49661eccc539ba6b8206e59156f39e412f56c4f Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Wed, 16 Sep 2026 22:39:55 +0200 Subject: [PATCH 05/13] fix: cursor feedback --- .../AuthenticationController.test.ts | 50 ++++++++++++++++++- .../AuthenticationController.ts | 27 +++++++++- 2 files changed, 74 insertions(+), 3 deletions(-) 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 2f6815adcd6..d29ab6561b8 100644 --- a/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.test.ts +++ b/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.test.ts @@ -2538,13 +2538,61 @@ describe('MFA credential enrollment', () => { }), ); - await expect(refresh).rejects.toThrow('wallet is locked'); + await expect(refresh).rejects.toThrow('the authenticated session ended'); expect(controller.state.enrolledCredentials).toStrictEqual([]); } finally { fetchSpy.mockRestore(); } }); + it.each([ + [ + 'sign-out', + (controller: AuthenticationController): void => + controller.performSignOut(), + ], + [ + 'wallet reset', + (controller: AuthenticationController): void => controller.clearState(), + ], + ])( + 'does not restore the previous profile credentials if %s happens during refresh', + async (_name, endSession) => { + let release!: (value: Awaited>) => void; + let requestStartedResolve: (() => void) | undefined; + const requestStarted = new Promise((resolve) => { + requestStartedResolve = resolve; + }); + const fetchSpy = jest.spyOn(globalThis, 'fetch').mockImplementationOnce( + async (): ReturnType => + await new Promise((resolve) => { + release = resolve; + requestStartedResolve?.(); + }), + ); + const { controller } = createController(); + try { + // Mirrors the non-awaited post-sign-in warm-up. + const refresh = controller.refreshEnrolledCredentials(); + await requestStarted; + endSession(controller); + release( + new globalThis.Response( + JSON.stringify(MOCK_MFA_CREDENTIALS_RESPONSE), + { status: 200 }, + ), + ); + + await expect(refresh).rejects.toThrow( + 'the authenticated session ended', + ); + expect(controller.state.enrolledCredentials).toStrictEqual([]); + } finally { + fetchSpy.mockRestore(); + } + }, + ); + it('keeps the existing cache when post-enrollment refresh fails', async () => { mockEndpointMfaEnrollComplete(); mockEndpointMfaCredentials({ diff --git a/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.ts b/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.ts index 0b22eedecab..7e58966554e 100644 --- a/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.ts +++ b/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.ts @@ -273,6 +273,12 @@ export class AuthenticationController extends BaseController< #isUnlocked = false; + /** + * Bumped whenever the authenticated session ends, so an in-flight MFA + * ceremony started under the previous session cannot apply its result. + */ + #authSessionEpoch = 0; + /** * Bumped by `requestProfilePairing` and `clearState` so an in-flight * `performSignIn` can't clear `needsProfilePairing` afterwards. @@ -289,6 +295,7 @@ export class AuthenticationController extends BaseController< }); this.messenger.subscribe('KeyringController:lock', () => { + this.#authSessionEpoch += 1; this.#isUnlocked = false; this.#clearEnrolledCredentials(); }); @@ -400,6 +407,14 @@ export class AuthenticationController extends BaseController< } } + #assertAuthSessionEpoch(epoch: number, methodName: string): void { + if (!this.#isUnlocked || this.#authSessionEpoch !== epoch) { + throw new Error( + `${methodName} - unable to proceed, the authenticated session ended`, + ); + } + } + /** * Reads the HD keyring entropy source IDs from KeyringController. * @@ -837,6 +852,7 @@ export class AuthenticationController extends BaseController< */ public async refreshEnrolledCredentials(): Promise { this.#assertIsUnlocked('refreshEnrolledCredentials'); + const sessionEpoch = this.#authSessionEpoch; const primaryEntropySourceId = this.#getPrimaryEntropySourceId(); const credentials = await this.#runMfaRequest( 'MFA Credentials Refresh', @@ -844,7 +860,7 @@ export class AuthenticationController extends BaseController< 'all', async () => await this.#auth.getMfaCredentials(primaryEntropySourceId), ); - this.#assertIsUnlocked('refreshEnrolledCredentials'); + this.#assertAuthSessionEpoch(sessionEpoch, 'refreshEnrolledCredentials'); // Skip the write when nothing changed so subscribers are not woken up by // a fresh-but-identical array. @@ -869,9 +885,10 @@ export class AuthenticationController extends BaseController< request: BeginEnrollmentRequest, ): Promise { this.#assertIsUnlocked('beginCredentialEnrollment'); + const sessionEpoch = this.#authSessionEpoch; assertValidMfaRequest(request, BeginEnrollmentRequestStruct); const primaryEntropySourceId = this.#getPrimaryEntropySourceId(); - return await this.#runMfaRequest( + const challenge = await this.#runMfaRequest( 'MFA Enroll Begin', request.reason.operation, request.type, @@ -882,6 +899,8 @@ export class AuthenticationController extends BaseController< primaryEntropySourceId, ), ); + this.#assertAuthSessionEpoch(sessionEpoch, 'beginCredentialEnrollment'); + return challenge; } /** @@ -899,6 +918,7 @@ export class AuthenticationController extends BaseController< request: CompleteEnrollmentRequest, ): Promise { this.#assertIsUnlocked('completeCredentialEnrollment'); + const sessionEpoch = this.#authSessionEpoch; assertValidMfaRequest(request, CompleteEnrollmentRequestStruct); const { type } = request.proof; const primaryEntropySourceId = this.#getPrimaryEntropySourceId(); @@ -914,6 +934,7 @@ export class AuthenticationController extends BaseController< primaryEntropySourceId, ), ); + this.#assertAuthSessionEpoch(sessionEpoch, 'completeCredentialEnrollment'); try { return await this.refreshEnrolledCredentials(); @@ -941,6 +962,7 @@ export class AuthenticationController extends BaseController< } public performSignOut(): void { + this.#authSessionEpoch += 1; this.#clearEnrolledCredentials(); this.update((state) => { state.isSignedIn = false; @@ -954,6 +976,7 @@ export class AuthenticationController extends BaseController< */ public clearState(): void { this.#profilePairingRequestEpoch += 1; + this.#authSessionEpoch += 1; this.#clearEnrolledCredentials(); this.update(() => ({ ...defaultState })); } From 352aef038fd0206a60a0017cf2b35608df495cd2 Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Wed, 16 Sep 2026 22:54:14 +0200 Subject: [PATCH 06/13] fix(profile-sync): invalidate SRP session even if the session ends during email enrollment --- .../AuthenticationController.test.ts | 45 +++++++++++++++++++ .../AuthenticationController.ts | 15 ++++++- 2 files changed, 58 insertions(+), 2 deletions(-) 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 d29ab6561b8..52dd8cea0ca 100644 --- a/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.test.ts +++ b/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.test.ts @@ -2593,6 +2593,51 @@ describe('MFA credential enrollment', () => { }, ); + it('still invalidates the SRP session when the wallet locks while email enrollment completes', async () => { + let release!: (value: Awaited>) => void; + let requestStartedResolve: (() => void) | undefined; + const requestStarted = new Promise((resolve) => { + requestStartedResolve = resolve; + }); + const fetchSpy = jest.spyOn(globalThis, 'fetch').mockImplementationOnce( + async (): ReturnType => + await new Promise((resolve) => { + release = resolve; + requestStartedResolve?.(); + }), + ); + const { controller, baseMessenger } = createController(); + try { + const completion = controller.completeCredentialEnrollment({ + flowId: 'flow-id', + proof: { type: 'email_otp', code: '123456' }, + reason: { operation: 'settings.addEmail' }, + }); + await requestStarted; + baseMessenger.publish('KeyringController:lock'); + release( + new globalThis.Response(JSON.stringify({ status: 'enrolled' }), { + status: 200, + }), + ); + + await expect(completion).rejects.toThrow( + 'the authenticated session ended', + ); + // No credentials refresh was attempted after the session ended. + expect(fetchSpy).toHaveBeenCalledTimes(1); + 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. + expect( + controller.state.srpSessionData?.[MOCK_ENTROPY_SOURCE_IDS[0]].profile + .canonicalProfileId, + ).toBe(''); + } finally { + fetchSpy.mockRestore(); + } + }); + it('keeps the existing cache when post-enrollment refresh fails', async () => { mockEndpointMfaEnrollComplete(); mockEndpointMfaCredentials({ diff --git a/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.ts b/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.ts index 7e58966554e..d90487a0ac0 100644 --- a/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.ts +++ b/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.ts @@ -909,7 +909,10 @@ export class AuthenticationController extends BaseController< * 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 - * fetch then includes the newly verified email claim. + * fetch then includes the newly verified email claim. That invalidation + * happens even if the session ends mid-request: the enrollment succeeded + * on the server, so a token cached across a lock must not be reused + * without the new claim. * * @param request - Flow identifier, platform or email proof, and trace reason. * @returns The refreshed credentials, or the existing cache if refresh fails. @@ -934,7 +937,15 @@ export class AuthenticationController extends BaseController< primaryEntropySourceId, ), ); - this.#assertAuthSessionEpoch(sessionEpoch, 'completeCredentialEnrollment'); + + try { + this.#assertAuthSessionEpoch(sessionEpoch, 'completeCredentialEnrollment'); + } catch (error) { + if (type === 'email_otp') { + this.#invalidateSrpSession(primaryEntropySourceId); + } + throw error; + } try { return await this.refreshEnrolledCredentials(); From 55242f053b27455c3f942bf177c1845ce5119e22 Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Wed, 16 Sep 2026 22:57:08 +0200 Subject: [PATCH 07/13] fix: lint --- .../AuthenticationController-method-action-types.ts | 5 ++++- .../controllers/authentication/AuthenticationController.ts | 5 ++++- 2 files changed, 8 insertions(+), 2 deletions(-) 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 874b07b9723..824c2a5ed7a 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 @@ -47,7 +47,10 @@ export type AuthenticationControllerBeginCredentialEnrollmentAction = { * 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 - * fetch then includes the newly verified email claim. + * fetch then includes the newly verified email claim. That invalidation + * happens even if the session ends mid-request: the enrollment succeeded + * on the server, so a token cached across a lock must not be reused + * without the new claim. * * @param request - Flow identifier, platform or email proof, and trace reason. * @returns The refreshed credentials, or the existing cache if refresh fails. diff --git a/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.ts b/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.ts index d90487a0ac0..6187e0e0fc6 100644 --- a/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.ts +++ b/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.ts @@ -939,7 +939,10 @@ export class AuthenticationController extends BaseController< ); try { - this.#assertAuthSessionEpoch(sessionEpoch, 'completeCredentialEnrollment'); + this.#assertAuthSessionEpoch( + sessionEpoch, + 'completeCredentialEnrollment', + ); } catch (error) { if (type === 'email_otp') { this.#invalidateSrpSession(primaryEntropySourceId); From 612027a54b069bed4a859b509020290cc505c4a1 Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Thu, 17 Sep 2026 09:45:06 +0200 Subject: [PATCH 08/13] fix: prevent stale overwrites --- .../AuthenticationController.test.ts | 87 +++++++++++++++++++ .../AuthenticationController.ts | 16 ++++ 2 files changed, 103 insertions(+) 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 52dd8cea0ca..de3c4fa5561 100644 --- a/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.test.ts +++ b/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.test.ts @@ -2773,6 +2773,93 @@ describe('MFA credential enrollment', () => { expect(await controller.performSignIn()).toHaveLength(2); expect(controller.state.isSignedIn).toBe(true); }); + + it('discards a stale warm-up refresh that lands after a newer, faster refresh from enrollment', async () => { + const deferred = (): { + promise: Promise; + resolve: (value: Value) => void; + } => { + let resolveDeferred!: (value: Value) => void; + const promise = new Promise((resolve) => { + resolveDeferred = resolve; + }); + return { promise, resolve: resolveDeferred }; + }; + + const warmUp = deferred>>(); + const enrollComplete = deferred>>(); + const enrollmentCredentials = deferred>>(); + + const warmUpStarted = deferred(); + const enrollCompleteStarted = deferred(); + const enrollmentCredentialsStarted = deferred(); + + const fetchSpy = jest.spyOn(globalThis, 'fetch'); + fetchSpy + .mockImplementationOnce(async (): ReturnType => { + warmUpStarted.resolve(); + return warmUp.promise; + }) + .mockImplementationOnce(async (): ReturnType => { + enrollCompleteStarted.resolve(); + return enrollComplete.promise; + }) + .mockImplementationOnce(async (): ReturnType => { + enrollmentCredentialsStarted.resolve(); + return enrollmentCredentials.promise; + }); + + const { controller } = createController(); + + try { + // 1. Slow warm-up refresh starts first but doesn't resolve yet. + const warmUpRefresh = controller.refreshEnrolledCredentials(); + await warmUpStarted.promise; + + // 2. Enrollment completes while the warm-up is in flight. + const enrollmentCompletion = controller.completeCredentialEnrollment({ + flowId: 'flow-id', + proof: { type: 'email_otp', code: '123456' }, + reason: { operation: 'settings.addEmail' }, + }); + + // 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, + }), + ); + + await enrollmentCredentialsStarted.promise; + enrollmentCredentials.resolve( + new globalThis.Response(JSON.stringify(MOCK_MFA_CREDENTIALS_RESPONSE), { + status: 200, + }), + ); + await enrollmentCompletion; + + const freshCredentials = controller.state.enrolledCredentials; + expect(freshCredentials).toHaveLength( + MOCK_MFA_CREDENTIALS_RESPONSE.credentials.length, + ); + + // 4. The stale warm-up response lands late, with different data. + warmUp.resolve( + new globalThis.Response(JSON.stringify({ credentials: [] }), { + status: 200, + }), + ); + await warmUpRefresh; + + // 5. Must NOT be overwritten by the stale warm-up. + expect(controller.state.enrolledCredentials).toStrictEqual( + freshCredentials, + ); + } finally { + fetchSpy.mockRestore(); + } + }); }); describe('metadata', () => { diff --git a/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.ts b/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.ts index 6187e0e0fc6..05bf9ab2759 100644 --- a/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.ts +++ b/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.ts @@ -279,6 +279,14 @@ export class AuthenticationController extends BaseController< */ #authSessionEpoch = 0; + /** + * Sequence number of the most recently started credentials refresh. Only + * that refresh may write the cache, so a slower, earlier request cannot + * overwrite a newer list (e.g. the post-sign-in warm-up landing after an + * enrollment's refresh). + */ + #credentialsRefreshSeq = 0; + /** * Bumped by `requestProfilePairing` and `clearState` so an in-flight * `performSignIn` can't clear `needsProfilePairing` afterwards. @@ -853,6 +861,8 @@ export class AuthenticationController extends BaseController< public async refreshEnrolledCredentials(): Promise { this.#assertIsUnlocked('refreshEnrolledCredentials'); const sessionEpoch = this.#authSessionEpoch; + this.#credentialsRefreshSeq += 1; + const refreshSeq = this.#credentialsRefreshSeq; const primaryEntropySourceId = this.#getPrimaryEntropySourceId(); const credentials = await this.#runMfaRequest( 'MFA Credentials Refresh', @@ -862,6 +872,12 @@ export class AuthenticationController extends BaseController< ); this.#assertAuthSessionEpoch(sessionEpoch, 'refreshEnrolledCredentials'); + // A newer refresh started while this one was in flight; its result is at + // least as fresh, so defer to it rather than overwrite with older data. + if (refreshSeq !== this.#credentialsRefreshSeq) { + return this.state.enrolledCredentials ?? []; + } + // Skip the write when nothing changed so subscribers are not woken up by // a fresh-but-identical array. if ( From 0d43d437376dcadeb187b78ea6c7465744650b7c Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Thu, 17 Sep 2026 16:36:00 +0200 Subject: [PATCH 09/13] fix: beginMfaEnrollment call signature --- .../authentication/AuthenticationController.ts | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.ts b/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.ts index 05bf9ab2759..50a01538f4c 100644 --- a/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.ts +++ b/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.ts @@ -909,11 +909,10 @@ export class AuthenticationController extends BaseController< request.reason.operation, request.type, async () => - await this.#auth.beginMfaEnrollment( - request.type, - request.email, - primaryEntropySourceId, - ), + await this.#auth.beginMfaEnrollment(request.type, { + email: request.email, + entropySourceId: primaryEntropySourceId, + }), ); this.#assertAuthSessionEpoch(sessionEpoch, 'beginCredentialEnrollment'); return challenge; From 3554ca2c32d27315a0aece89ddc9b9c68fe054f1 Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Fri, 18 Sep 2026 14:50:04 +0200 Subject: [PATCH 10/13] fix: remove lock clear dance and wrap token is sensitive --- .../AuthenticationController.test.ts | 21 ++----------------- .../AuthenticationController.ts | 2 -- .../authentication-jwt-bearer/mfa/schemas.ts | 14 +++++++------ 3 files changed, 10 insertions(+), 27 deletions(-) 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 de3c4fa5561..220b880a797 100644 --- a/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.test.ts +++ b/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.test.ts @@ -2682,15 +2682,8 @@ describe('MFA credential enrollment', () => { 'wallet reset', (controller: AuthenticationController): void => controller.clearState(), ], - [ - 'lock', - ( - _controller: AuthenticationController, - baseMessenger: RootMessenger, - ): void => baseMessenger.publish('KeyringController:lock'), - ], ])('clears cached credentials on %s', (_name, act) => { - const { controller, baseMessenger } = createController({ + const { controller } = createController({ state: { ...mockSignedInState(), enrolledCredentials: [ @@ -2704,21 +2697,11 @@ describe('MFA credential enrollment', () => { }, }); - act(controller, baseMessenger); + act(controller); expect(controller.state.enrolledCredentials).toStrictEqual([]); }); - it('does not write state on lock when the cache is already empty', () => { - const { baseMessenger } = createController(); - const listener = jest.fn(); - baseMessenger.subscribe('AuthenticationController:stateChange', listener); - - baseMessenger.publish('KeyringController:lock'); - - expect(listener).not.toHaveBeenCalled(); - }); - it('rejects MFA calls while the wallet is locked', async () => { const { controller, baseMessenger } = createController(); baseMessenger.publish('KeyringController:lock'); diff --git a/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.ts b/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.ts index 50a01538f4c..f2ebcd3da9f 100644 --- a/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.ts +++ b/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.ts @@ -305,7 +305,6 @@ export class AuthenticationController extends BaseController< this.messenger.subscribe('KeyringController:lock', () => { this.#authSessionEpoch += 1; this.#isUnlocked = false; - this.#clearEnrolledCredentials(); }); }, }; @@ -946,7 +945,6 @@ export class AuthenticationController extends BaseController< type, async () => await this.#auth.completeMfaEnrollment( - type, request.flowId, request.proof, primaryEntropySourceId, 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 3add3dec249..03dcc652922 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 @@ -246,12 +246,14 @@ export const GetElevatedTokenRequestStruct = object({ maxSessionAgeMs: optional(min(integer(), 0)), }); -export const ElevatedTokenClaimsStruct = type({ - sub: sensitive(string()), - aal: literal(2), - exp: integer(), - amr: union([MfaCredentialTypeStruct, array(MfaCredentialTypeStruct)]), -}); +export const ElevatedTokenClaimsStruct = sensitive( + type({ + sub: string(), + aal: literal(2), + exp: integer(), + amr: union([MfaCredentialTypeStruct, array(MfaCredentialTypeStruct)]), + }), +); function formatStructError(error: StructError): string { return error From 37faeef78ce25b2c1c91fff6182f3c67ceb8728e Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Fri, 18 Sep 2026 15:37:34 +0200 Subject: [PATCH 11/13] fix: lint --- .../src/controllers/authentication/AuthenticationController.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.ts b/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.ts index f2ebcd3da9f..b32c3a58638 100644 --- a/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.ts +++ b/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.ts @@ -4,8 +4,8 @@ import type { ControllerStateChangeEvent, StateMetadata, } from '@metamask/base-controller'; -import { selectHdKeyringEntropySourceIds } from '@metamask/keyring-controller'; import type { TraceCallback } from '@metamask/controller-utils'; +import { selectHdKeyringEntropySourceIds } from '@metamask/keyring-controller'; import type { KeyringControllerGetStateAction, KeyringControllerLockEvent, From 622451043504fcca0821f1a9bafecccdbd33be2b Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Fri, 18 Sep 2026 16:37:10 +0200 Subject: [PATCH 12/13] fix: remove isMfaEnabled flag --- packages/profile-sync-controller/CHANGELOG.md | 2 +- .../AuthenticationController.test.ts | 91 +++---------------- .../AuthenticationController.ts | 16 +--- 3 files changed, 15 insertions(+), 94 deletions(-) diff --git a/packages/profile-sync-controller/CHANGELOG.md b/packages/profile-sync-controller/CHANGELOG.md index 01ecea791f0..8a097e0d66c 100644 --- a/packages/profile-sync-controller/CHANGELOG.md +++ b/packages/profile-sync-controller/CHANGELOG.md @@ -9,7 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added -- Add `refreshEnrolledCredentials`, `beginCredentialEnrollment` and `completeCredentialEnrollment` to `AuthenticationController`, backed by a memory-only `enrolledCredentials` state, an `isMfaEnabled` config gate for post-sign-in refresh, and an optional `trace` callback ([#10266](https://github.com/MetaMask/core/pull/10266)) +- Add `refreshEnrolledCredentials`, `beginCredentialEnrollment` and `completeCredentialEnrollment` to `AuthenticationController`, backed by a memory-only `enrolledCredentials` state and an optional `trace` callback ([#10266](https://github.com/MetaMask/core/pull/10266)) - Add passkey and email OTP enrollment, verification, credential-list, and elevated-token exchange SDK methods ([#10265](https://github.com/MetaMask/core/pull/10265)) - Add validated MFA domain types and structured `MfaError` classes with a serialization-safe `mfaCode` ([#10264](https://github.com/MetaMask/core/pull/10264)) - Add `rampsOrders` to `USER_STORAGE_FEATURE_NAMES` ([#10227](https://github.com/MetaMask/core/pull/10227)) 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 220b880a797..5f9d6de636f 100644 --- a/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.test.ts +++ b/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.test.ts @@ -7,7 +7,6 @@ import type { MessengerEvents, MockAnyNamespace, } from '@metamask/messenger'; -import { cleanAll as cleanAllNock } from 'nock'; import { arrangeAuthAPIs } from '../../sdk/__fixtures__/auth.js'; import type { LoginResponse } from '../../sdk/index.js'; @@ -2319,7 +2318,6 @@ describe('AuthenticationController', () => { describe('MFA credential enrollment', () => { function createController(options?: { state?: AuthenticationControllerState; - isMfaEnabled?: () => boolean; trace?: TraceCallback; }): { controller: AuthenticationController; @@ -2331,34 +2329,12 @@ describe('MFA credential enrollment', () => { messenger, metametrics: createMockAuthMetaMetrics(), state: options?.state ?? mockSignedInState(), - config: { - isMfaEnabled: options?.isMfaEnabled ?? ((): boolean => false), - }, trace: options?.trace, }), baseMessenger, }; } - /** - * Resolves once `enrolledCredentials` is written with a non-empty list. - * - * @param baseMessenger - Root messenger to observe. - * @returns A promise settled by the next credential-bearing state change. - */ - function waitForCredentials(baseMessenger: RootMessenger): Promise { - return new Promise((resolve) => { - baseMessenger.subscribe( - 'AuthenticationController:stateChange', - (state: AuthenticationControllerState) => { - if ((state.enrolledCredentials?.length ?? 0) > 0) { - resolve(); - } - }, - ); - }); - } - it('starts with an empty memory-only credential cache', () => { const { controller } = createController({ state: { ...defaultState }, @@ -2572,7 +2548,6 @@ describe('MFA credential enrollment', () => { ); const { controller } = createController(); try { - // Mirrors the non-awaited post-sign-in warm-up. const refresh = controller.refreshEnrolledCredentials(); await requestStarted; endSession(controller); @@ -2717,47 +2692,7 @@ describe('MFA credential enrollment', () => { ).rejects.toThrow('wallet is locked'); }); - it('warms the credential cache after sign-in without blocking it, only when enabled', async () => { - const enabledEndpoints = arrangeAuthAPIs(); - const enabled = createController({ - state: { ...defaultState }, - isMfaEnabled: () => true, - }); - const warmed = waitForCredentials(enabled.baseMessenger); - await enabled.controller.performSignIn(); - // Sign-in resolved before the credentials call had to complete. - expect(enabled.controller.state.isSignedIn).toBe(true); - await warmed; - expect(enabledEndpoints.mockMfaCredentialsUrl.isDone()).toBe(true); - expect(enabled.controller.state.enrolledCredentials).toHaveLength(2); - - cleanAllNock(); - const disabledEndpoints = arrangeAuthAPIs(); - const disabled = createController({ - state: { ...defaultState }, - isMfaEnabled: () => false, - }).controller; - await disabled.performSignIn(); - expect(disabledEndpoints.mockMfaCredentialsUrl.isDone()).toBe(false); - }); - - it('does not fail sign-in when automatic refresh fails', async () => { - arrangeAuthAPIs({ - mockMfaCredentialsUrl: { - status: 502, - body: { code: 'kratos_unavailable', message: 'Unavailable' }, - }, - }); - const { controller } = createController({ - state: { ...defaultState }, - isMfaEnabled: () => true, - }); - - expect(await controller.performSignIn()).toHaveLength(2); - expect(controller.state.isSignedIn).toBe(true); - }); - - it('discards a stale warm-up refresh that lands after a newer, faster refresh from enrollment', async () => { + it('discards a stale refresh that lands after a newer, faster refresh from enrollment', async () => { const deferred = (): { promise: Promise; resolve: (value: Value) => void; @@ -2769,19 +2704,19 @@ describe('MFA credential enrollment', () => { return { promise, resolve: resolveDeferred }; }; - const warmUp = deferred>>(); + const slow = deferred>>(); const enrollComplete = deferred>>(); const enrollmentCredentials = deferred>>(); - const warmUpStarted = deferred(); + const slowStarted = deferred(); const enrollCompleteStarted = deferred(); const enrollmentCredentialsStarted = deferred(); const fetchSpy = jest.spyOn(globalThis, 'fetch'); fetchSpy .mockImplementationOnce(async (): ReturnType => { - warmUpStarted.resolve(); - return warmUp.promise; + slowStarted.resolve(); + return slow.promise; }) .mockImplementationOnce(async (): ReturnType => { enrollCompleteStarted.resolve(); @@ -2795,11 +2730,11 @@ describe('MFA credential enrollment', () => { const { controller } = createController(); try { - // 1. Slow warm-up refresh starts first but doesn't resolve yet. - const warmUpRefresh = controller.refreshEnrolledCredentials(); - await warmUpStarted.promise; + // 1. Slow refresh starts first but doesn't resolve yet. + const slowRefresh = controller.refreshEnrolledCredentials(); + await slowStarted.promise; - // 2. Enrollment completes while the warm-up is in flight. + // 2. Enrollment completes while the slow refresh is in flight. const enrollmentCompletion = controller.completeCredentialEnrollment({ flowId: 'flow-id', proof: { type: 'email_otp', code: '123456' }, @@ -2827,15 +2762,15 @@ describe('MFA credential enrollment', () => { MOCK_MFA_CREDENTIALS_RESPONSE.credentials.length, ); - // 4. The stale warm-up response lands late, with different data. - warmUp.resolve( + // 4. The stale response lands late, with different data. + slow.resolve( new globalThis.Response(JSON.stringify({ credentials: [] }), { status: 200, }), ); - await warmUpRefresh; + await slowRefresh; - // 5. Must NOT be overwritten by the stale warm-up. + // 5. Must NOT be overwritten by the stale response. expect(controller.state.enrolledCredentials).toStrictEqual( freshCredentials, ); diff --git a/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.ts b/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.ts index b32c3a58638..5a466f2f24b 100644 --- a/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.ts +++ b/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.ts @@ -179,12 +179,6 @@ type ControllerConfig = { * `() => false`. */ isSocialPairingEnabled: () => boolean; - /** - * When `true`, `performSignIn` refreshes `enrolledCredentials` after the - * session is established. MFA methods themselves are never gated; the - * client decides when to expose them. Defaults to `() => false`. - */ - isMfaEnabled: () => boolean; }; const MESSENGER_EXPOSED_METHODS = [ @@ -268,7 +262,6 @@ export class AuthenticationController extends BaseController< readonly #config: ControllerConfig = { env: Env.PRD, isSocialPairingEnabled: () => false, - isMfaEnabled: () => false, }; #isUnlocked = false; @@ -282,7 +275,7 @@ export class AuthenticationController extends BaseController< /** * Sequence number of the most recently started credentials refresh. Only * that refresh may write the cache, so a slower, earlier request cannot - * overwrite a newer list (e.g. the post-sign-in warm-up landing after an + * overwrite a newer list (e.g. an explicit refresh landing after an * enrollment's refresh). */ #credentialsRefreshSeq = 0; @@ -342,7 +335,6 @@ export class AuthenticationController extends BaseController< ...config, isSocialPairingEnabled: config?.isSocialPairingEnabled ?? this.#config.isSocialPairingEnabled, - isMfaEnabled: config?.isMfaEnabled ?? this.#config.isMfaEnabled, }; this.#metametrics = metametrics; @@ -579,12 +571,6 @@ export class AuthenticationController extends BaseController< // noop } - // Best-effort warm-up of the credential cache. Not awaited: sign-in must - // not wait on the MFA service, and MFA flows refresh explicitly anyway. - if (this.#config.isMfaEnabled()) { - this.refreshEnrolledCredentials().catch(() => undefined); - } - return accessTokens; } From d70d671c45fdc45bc04254aff3d740702f8adce9 Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Mon, 21 Sep 2026 16:04:15 +0200 Subject: [PATCH 13/13] fix: address PR comments --- .../AuthenticationController.test.ts | 21 ++++++---- .../AuthenticationController.ts | 41 ++++++++----------- 2 files changed, 30 insertions(+), 32 deletions(-) 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 5f9d6de636f..fa6737243d8 100644 --- a/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.test.ts +++ b/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.test.ts @@ -2805,7 +2805,7 @@ describe('metadata', () => { }); describe('includeInStateLogs', () => { - it('redacts enrolled email addresses', () => { + it('keeps only non-PII credential fields', () => { const controller = new AuthenticationController({ messenger: createMockAuthenticationMessenger().messenger, metametrics: createMockAuthMetaMetrics(), @@ -2813,10 +2813,17 @@ describe('metadata', () => { ...mockSignedInState(), enrolledCredentials: [ { - type: 'email_otp', + type: 'passkey', status: 'active', + enrolledAt: 1_000, + displayName: 'My iPhone', + }, + { + type: 'email_otp', + status: 'pending', + enrolledAt: 2_000, email: 'jane@example.com', - verified: true, + verified: false, }, ], }, @@ -2829,12 +2836,8 @@ describe('metadata', () => { 'includeInStateLogs', ).enrolledCredentials, ).toStrictEqual([ - { - type: 'email_otp', - status: 'active', - email: 'j••@example.com', - verified: true, - }, + { type: 'passkey', status: 'active', enrolledAt: 1_000 }, + { type: 'email_otp', status: 'pending', enrolledAt: 2_000 }, ]); }); diff --git a/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.ts b/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.ts index 5a466f2f24b..90dd33b4551 100644 --- a/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.ts +++ b/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.ts @@ -148,23 +148,13 @@ const metadata: StateMetadata = { usedInUi: true, }, enrolledCredentials: { - includeInStateLogs: (credentials) => { - if (!credentials) { - return null; - } - return credentials.map((credential) => { - if (credential.type !== 'email_otp') { - return credential; - } - const separatorIndex = credential.email.indexOf('@'); - const domain = - separatorIndex >= 0 ? credential.email.slice(separatorIndex) : ''; - return { - ...credential, - email: `${credential.email.slice(0, 1)}••${domain}`, - }; - }); - }, + // Allow-list so new credential types cannot leak identifiers by default. + includeInStateLogs: (credentials) => + credentials?.map(({ type, status, enrolledAt }) => ({ + type, + status, + ...(enrolledAt === undefined ? {} : { enrolledAt }), + })) ?? null, persist: false, includeInDebugSnapshot: false, usedInUi: true, @@ -800,7 +790,8 @@ export class AuthenticationController extends BaseController< /** * Runs one MFA network step inside a trace span tagged with the caller's - * operation and the credential type, recording the outcome and MFA code. + * operation and the credential type, recording the outcome and MFA error + * code. * * @param name - Span name. * @param operation - Caller-supplied `reason.operation`. @@ -830,7 +821,7 @@ export class AuthenticationController extends BaseController< data.outcome = 'error'; const mfaCode = getMfaErrorCode(error); if (mfaCode) { - data.mfaCode = mfaCode; + data.mfaErrorCode = mfaCode; } throw error; } @@ -860,14 +851,14 @@ export class AuthenticationController extends BaseController< // A newer refresh started while this one was in flight; its result is at // least as fresh, so defer to it rather than overwrite with older data. if (refreshSeq !== this.#credentialsRefreshSeq) { - return this.state.enrolledCredentials ?? []; + return this.#getEnrolledCredentials(); } // Skip the write when nothing changed so subscribers are not woken up by // a fresh-but-identical array. if ( JSON.stringify(credentials) !== - JSON.stringify(this.state.enrolledCredentials ?? []) + JSON.stringify(this.#getEnrolledCredentials()) ) { this.update((state) => { state.enrolledCredentials = credentials; @@ -952,7 +943,7 @@ export class AuthenticationController extends BaseController< try { return await this.refreshEnrolledCredentials(); } catch { - return this.state.enrolledCredentials ?? []; + return this.#getEnrolledCredentials(); } finally { if (type === 'email_otp') { this.#invalidateSrpSession(primaryEntropySourceId); @@ -966,7 +957,7 @@ export class AuthenticationController extends BaseController< * that is no longer active. */ #clearEnrolledCredentials(): void { - if ((this.state.enrolledCredentials?.length ?? 0) === 0) { + if (this.#getEnrolledCredentials().length === 0) { return; } this.update((state) => { @@ -974,6 +965,10 @@ export class AuthenticationController extends BaseController< }); } + #getEnrolledCredentials(): EnrolledCredential[] { + return this.state.enrolledCredentials ?? []; + } + public performSignOut(): void { this.#authSessionEpoch += 1; this.#clearEnrolledCredentials();