diff --git a/packages/db-migrations/databases/fxa/patches/patch-190-191.sql b/packages/db-migrations/databases/fxa/patches/patch-190-191.sql new file mode 100644 index 00000000000..6634f9c3ef2 --- /dev/null +++ b/packages/db-migrations/databases/fxa/patches/patch-190-191.sql @@ -0,0 +1,68 @@ +SET NAMES utf8mb4 COLLATE utf8mb4_bin; + +CALL assertPatchLevel('190'); + +CREATE PROCEDURE `createVerifiedSessionToken_1` ( + IN `tokenId` BINARY(32), + IN `tokenData` BINARY(32), + IN `uid` BINARY(16), + IN `createdAt` BIGINT UNSIGNED, + IN `uaBrowser` VARCHAR(255), + IN `uaBrowserVersion` VARCHAR(255), + IN `uaOS` VARCHAR(255), + IN `uaOSVersion` VARCHAR(255), + IN `uaDeviceType` VARCHAR(255), + IN `uaFormFactor` VARCHAR(255), + IN `mustVerify` BOOLEAN, + IN `providerId` TINYINT, + IN `verificationMethod` INT, + IN `verifiedAt` BIGINT UNSIGNED +) +BEGIN + DECLARE EXIT HANDLER FOR SQLEXCEPTION + BEGIN + ROLLBACK; + RESIGNAL; + END; + + START TRANSACTION; + + INSERT INTO sessionTokens( + tokenId, + tokenData, + uid, + createdAt, + uaBrowser, + uaBrowserVersion, + uaOS, + uaOSVersion, + uaDeviceType, + uaFormFactor, + lastAccessTime, + mustVerify, + providerId, + verificationMethod, + verifiedAt + ) + VALUES( + tokenId, + tokenData, + uid, + createdAt, + uaBrowser, + uaBrowserVersion, + uaOS, + uaOSVersion, + uaDeviceType, + uaFormFactor, + createdAt, + mustVerify, + providerId, + verificationMethod, + verifiedAt + ); + + COMMIT; +END; + +UPDATE dbMetadata SET value = '191' WHERE name = 'schema-patch-level'; diff --git a/packages/db-migrations/databases/fxa/patches/patch-191-190.sql b/packages/db-migrations/databases/fxa/patches/patch-191-190.sql new file mode 100644 index 00000000000..858b526cc6f --- /dev/null +++ b/packages/db-migrations/databases/fxa/patches/patch-191-190.sql @@ -0,0 +1,5 @@ +-- SET NAMES utf8mb4 COLLATE utf8mb4_bin; + +-- DROP PROCEDURE `createVerifiedSessionToken_1`; + +-- UPDATE dbMetadata SET value = '190' WHERE name = 'schema-patch-level'; diff --git a/packages/db-migrations/databases/fxa/target-patch.json b/packages/db-migrations/databases/fxa/target-patch.json index 31fd730cf3d..a2e94da66a8 100644 --- a/packages/db-migrations/databases/fxa/target-patch.json +++ b/packages/db-migrations/databases/fxa/target-patch.json @@ -1,3 +1,3 @@ { - "level": 190 + "level": 191 } diff --git a/packages/fxa-auth-server/lib/db.spec.ts b/packages/fxa-auth-server/lib/db.spec.ts index c2b13ad948a..59020fa86e2 100644 --- a/packages/fxa-auth-server/lib/db.spec.ts +++ b/packages/fxa-auth-server/lib/db.spec.ts @@ -19,6 +19,7 @@ jest.mock('fxa-shared/db/models/auth', () => ({ }, SessionToken: { create: jest.fn().mockResolvedValue(null), + createVerified: jest.fn().mockResolvedValue(null), delete: jest.fn().mockResolvedValue(null), findByUid: jest.fn().mockResolvedValue([]), }, @@ -385,6 +386,35 @@ describe('redis enabled, token-pruning enabled:', () => { ); }); + it('should call RawSessionToken.createVerified (not create) and set verificationMethod/verifiedAt in db.createPasskeyVerifiedSessionToken', async () => { + const metrics = { + increment: jest.fn(), + }; + db.metrics = metrics; + + const result = await db.createPasskeyVerifiedSessionToken({ + uid: 'wibble', + }); + expect(redis.pruneSessionTokens).toHaveBeenCalledTimes(1); + expect(redis.pruneSessionTokens).toHaveBeenCalledWith( + 'wibble', + expect.anything() + ); + // Uses createVerified (the passkey proc), not the plain create + expect(models.SessionToken.createVerified.mock.calls.length).toBe(1); + // Proc receives verificationMethod=5 (passkey) and a numeric verifiedAt + const procArg = models.SessionToken.createVerified.mock.calls[0][0]; + expect(procArg.verificationMethod).toBe(5); + expect(typeof procArg.verifiedAt).toBe('number'); + // Returned token reflects the same values + expect(result.verificationMethod).toBe(5); + expect(typeof result.verifiedAt).toBe('number'); + // Metrics tagged with method: passkey + expect(metrics.increment).toHaveBeenCalledWith('db.sessionToken.created', { + method: 'passkey', + }); + }); + describe('mock db.pruneSessionTokens:', () => { beforeEach(() => { db.pruneSessionTokens = jest.fn(() => Promise.resolve()); diff --git a/packages/fxa-auth-server/lib/db.ts b/packages/fxa-auth-server/lib/db.ts index 31a95717e27..2466feabb6f 100644 --- a/packages/fxa-auth-server/lib/db.ts +++ b/packages/fxa-auth-server/lib/db.ts @@ -33,6 +33,7 @@ import { Container } from 'typedi'; import random, { base32 } from './crypto/random'; import { AppError as error } from '@fxa/accounts/errors'; import { + verificationMethodToNumber, verificationMethodToString, VerificationMethod, } from 'fxa-shared/db/models/auth/session-token'; @@ -180,6 +181,34 @@ export const createDB = ( return sessionToken; } + async createPasskeyVerifiedSessionToken(authToken: any) { + const { uid } = authToken; + + log.trace('DB.createPasskeyVerifiedSessionToken', { uid }); + + const verifiedAt = Date.now(); + const sessionToken = await SessionToken.create({ + ...authToken, + mustVerify: false, + tokenVerificationId: null, + verificationMethod: verificationMethodToNumber('passkey'), + verifiedAt, + }); + + const { id } = sessionToken; + + // Ensure there are no clashes with zombie tokens left behind in Redis + try { + await this.deleteSessionTokenFromRedis(uid, id); + } catch (unusedErr) { + // Ignore errors deleting the token. + } + await RawSessionToken.createVerified(sessionToken); + + this.metrics?.increment('db.sessionToken.created', { method: 'passkey' }); + return sessionToken; + } + async createKeyFetchToken(authToken: any) { log.trace('DB.createKeyFetchToken', { uid: authToken && authToken.uid }); const keyFetchToken = await KeyFetchToken.create(authToken); diff --git a/packages/fxa-auth-server/lib/routes/passkeys.spec.ts b/packages/fxa-auth-server/lib/routes/passkeys.spec.ts index d12822a10ff..3d6951d8411 100644 --- a/packages/fxa-auth-server/lib/routes/passkeys.spec.ts +++ b/packages/fxa-auth-server/lib/routes/passkeys.spec.ts @@ -110,11 +110,9 @@ describe('passkeys routes', () => { emailVerified: true, verifierSetAt: 1234567890, }), - createSessionToken: jest + createPasskeyVerifiedSessionToken: jest .fn() .mockResolvedValue({ id: 'new-session-token-id' }), - verifyTokensWithMethod: jest.fn().mockResolvedValue(undefined), - deleteSessionToken: jest.fn().mockResolvedValue(undefined), securityEvent: jest.fn().mockResolvedValue(undefined), }; @@ -833,35 +831,22 @@ describe('passkeys routes', () => { ); }); - it('creates a pre-verified session token with correct options', async () => { + it('creates a verified session token with correct options', async () => { await handler.createPasskeySessionToken(mockAccount, mockRequest as any); - expect(db.createSessionToken).toHaveBeenCalledWith( - expect.objectContaining({ - uid: UID, - email: TEST_EMAIL, - emailCode: 'emailcode123', - emailVerified: true, - verifierSetAt: 1234567890, - mustVerify: false, - tokenVerificationId: null, - uaBrowser: 'Firefox', - uaBrowserVersion: '124.0', - uaOS: 'macOS', - uaOSVersion: '14.0', - }) - ); - }); - - // TODO(FXA-13444): once the atomic stored procedure lands, this test - // should be updated to assert the single new DB call instead. - it('stamps the token with the passkey verification method', async () => { - await handler.createPasskeySessionToken(mockAccount, mockRequest as any); - - expect(db.verifyTokensWithMethod).toHaveBeenCalledWith( - 'new-session-token-id', - 'passkey' - ); + expect(db.createPasskeyVerifiedSessionToken).toHaveBeenCalledWith({ + uid: UID, + email: TEST_EMAIL, + emailCode: 'emailcode123', + emailVerified: true, + verifierSetAt: 1234567890, + uaBrowser: 'Firefox', + uaBrowserVersion: '124.0', + uaOS: 'macOS', + uaOSVersion: '14.0', + uaDeviceType: null, + uaFormFactor: null, + }); }); it('returns the created session token and emits success metric', async () => { @@ -876,31 +861,13 @@ describe('passkeys routes', () => { ); }); - it('propagates errors from createSessionToken', async () => { + it('propagates errors from createPasskeyVerifiedSessionToken', async () => { const dbError = new Error('DB unavailable'); - db.createSessionToken.mockRejectedValue(dbError); + db.createPasskeyVerifiedSessionToken.mockRejectedValue(dbError); await expect( handler.createPasskeySessionToken(mockAccount, mockRequest as any) ).rejects.toThrow('DB unavailable'); }); - - // TODO(FXA-13444): remove this test once the atomic stored procedure lands. - it('deletes the token, emits failure metric, and rethrows if verifyTokensWithMethod fails', async () => { - const dbError = new Error('DB unavailable'); - db.verifyTokensWithMethod.mockRejectedValue(dbError); - - await expect( - handler.createPasskeySessionToken(mockAccount, mockRequest as any) - ).rejects.toThrow('DB unavailable'); - - expect(db.deleteSessionToken).toHaveBeenCalledWith({ - id: 'new-session-token-id', - uid: UID, - }); - expect(statsd.increment).toHaveBeenCalledWith( - 'passkeys.createSessionToken.failure' - ); - }); }); }); diff --git a/packages/fxa-auth-server/lib/routes/passkeys.ts b/packages/fxa-auth-server/lib/routes/passkeys.ts index 87d0beddf56..ef1915f35c3 100644 --- a/packages/fxa-auth-server/lib/routes/passkeys.ts +++ b/packages/fxa-auth-server/lib/routes/passkeys.ts @@ -42,15 +42,13 @@ interface DB { emailVerified: boolean; verifierSetAt: number; }>; - /** Creates a new session token and persists it. */ - createSessionToken(options: { + /** Creates a passkey session token, pre-verified as AAL2 at creation time. */ + createPasskeyVerifiedSessionToken(options: { uid: string; email: string; emailCode: string; emailVerified: boolean; verifierSetAt: number; - mustVerify: boolean; - tokenVerificationId: string | null; uaBrowser?: string; uaBrowserVersion?: string; uaOS?: string; @@ -58,13 +56,6 @@ interface DB { uaDeviceType?: string; uaFormFactor?: string; }): Promise<{ id: string }>; - /** Sets the verification method on a session token. */ - verifyTokensWithMethod( - tokenId: string, - method: string | number - ): Promise; - /** Deletes a session token. Used for cleanup on partial failure. */ - deleteSessionToken(token: { id: string; uid: string }): Promise; /** Records a security event in the audit log. */ securityEvent: (arg: any) => Promise; } @@ -373,12 +364,10 @@ export class PasskeyHandler { * Creates a passkey-verified session token for the authenticated account. * * No `tokenVerificationId` is set — the passkey assertion is itself AAL2, so - * no follow-up email challenge is needed. After creation, - * `verifyTokensWithMethod` stamps `verificationMethod = 5` (passkey) on the - * row; the token's AMR becomes `{pwd, webauthn}` → AAL2. - * - * TODO(FXA-13444): this is a temporary two-step implementation. The create - * and stamp will be replaced by a single atomic stored procedure. + * no follow-up email challenge is needed. The token is written in a single + * MySQL INSERT with `verificationMethod = 5` (passkey) and `verifiedAt` + * pre-stamped, so no AAL1 window exists. The token's AMR becomes + * `{pwd, webauthn}` → AAL2. */ async createPasskeySessionToken( account: { @@ -390,14 +379,12 @@ export class PasskeyHandler { }, request: AuthRequest ) { - const sessionToken = await this.db.createSessionToken({ + const sessionToken = await this.db.createPasskeyVerifiedSessionToken({ uid: account.uid, email: account.email, emailCode: account.emailCode, emailVerified: account.emailVerified, verifierSetAt: account.verifierSetAt, - mustVerify: false, - tokenVerificationId: null, uaBrowser: request.app.ua.browser, uaBrowserVersion: request.app.ua.browserVersion, uaOS: request.app.ua.os, @@ -406,29 +393,6 @@ export class PasskeyHandler { uaFormFactor: request.app.ua.formFactor, }); - try { - await this.db.verifyTokensWithMethod(sessionToken.id, 'passkey'); - } catch (err) { - // If stamping the verification method fails, delete the token rather than - // leaving an orphan session at AAL1. If cleanup also fails, log and report - // it but always re-throw the original error. - // TODO(FXA-13444): remove this entire catch block once the atomic procedure lands. - try { - await this.db.deleteSessionToken({ - id: sessionToken.id, - uid: account.uid, - }); - } catch (cleanupErr) { - this.log.error('passkeys.createPasskeySessionToken.deleteOrphan', { - err: cleanupErr, - tokenId: sessionToken.id, - }); - reportSentryError(cleanupErr, request); - } - this.statsd.increment('passkeys.createSessionToken.failure'); - throw err; - } - this.statsd.increment('passkeys.createSessionToken.success'); return sessionToken; } diff --git a/packages/fxa-shared/db/models/auth/base-auth.ts b/packages/fxa-shared/db/models/auth/base-auth.ts index 42b62e790ac..7bbf919c8b1 100644 --- a/packages/fxa-shared/db/models/auth/base-auth.ts +++ b/packages/fxa-shared/db/models/auth/base-auth.ts @@ -24,6 +24,7 @@ export enum Proc { CreateRecoveryKey = 'createRecoveryKey_4', CreateSecurityEvent = 'createSecurityEvent_5', CreateSessionToken = 'createSessionToken_10', + CreateVerifiedSessionToken = 'createVerifiedSessionToken_1', CreateSigninCode = 'createSigninCode_2', CreateTotpToken = 'createTotpToken_1', CreateUnblockCode = 'createUnblockCode_1', diff --git a/packages/fxa-shared/db/models/auth/session-token.ts b/packages/fxa-shared/db/models/auth/session-token.ts index 7bd0e28d362..a6344c7857f 100644 --- a/packages/fxa-shared/db/models/auth/session-token.ts +++ b/packages/fxa-shared/db/models/auth/session-token.ts @@ -172,6 +172,58 @@ export class SessionToken extends BaseToken { ); } + static async createVerified({ + id, + data, + uid, + createdAt, + uaBrowser, + uaBrowserVersion, + uaOS, + uaOSVersion, + uaDeviceType, + uaFormFactor, + mustVerify, + providerId, + verificationMethod, + verifiedAt, + }: Pick< + SessionToken, + | 'uid' + | 'createdAt' + | 'uaBrowser' + | 'uaBrowserVersion' + | 'uaOS' + | 'uaOSVersion' + | 'uaDeviceType' + | 'uaFormFactor' + | 'mustVerify' + | 'providerId' + > & { + id: string; + data: string; + verificationMethod: VerificationMethod | number; + verifiedAt: number; + }) { + return this.callProcedure( + Proc.CreateVerifiedSessionToken, + uuidTransformer.to(id), + uuidTransformer.to(data), + uuidTransformer.to(uid), + createdAt, + uaBrowser ?? null, + uaBrowserVersion ?? null, + uaOS ?? null, + uaOSVersion ?? null, + uaDeviceType ?? null, + uaFormFactor ?? null, + !!mustVerify, + providerId ?? null, + verificationMethodToNumber(verificationMethod), + verifiedAt + ); + } + static async update({ id, uaBrowser,