From 34f4d47bc6b6e0900e4e74057a8919969004fda6 Mon Sep 17 00:00:00 2001 From: wangsijie Date: Tue, 31 Oct 2023 10:40:01 +0800 Subject: [PATCH] feat(core): add last use time to user mfa verifications (#4767) --- .../core/src/routes/interaction/mfa.test.ts | 4 ++ packages/core/src/routes/interaction/mfa.ts | 15 ++++++ .../mfa-payload-verification.test.ts | 6 ++- .../verifications/mfa-payload-verification.ts | 2 +- .../verifications/mfa-verification.test.ts | 42 +++++++++++++++- .../verifications/mfa-verification.ts | 50 +++++++++++++++++-- .../src/foundations/jsonb-types/users.ts | 1 + 7 files changed, 111 insertions(+), 9 deletions(-) diff --git a/packages/core/src/routes/interaction/mfa.test.ts b/packages/core/src/routes/interaction/mfa.test.ts index 8beeecc8d..e36a38939 100644 --- a/packages/core/src/routes/interaction/mfa.test.ts +++ b/packages/core/src/routes/interaction/mfa.test.ts @@ -65,6 +65,8 @@ const baseProviderMock = { client_id: demoAppApplicationId, }; +const updateUserById = jest.fn(); + const tenantContext = new MockTenant( createMockProvider(jest.fn().mockResolvedValue(baseProviderMock)), { @@ -73,6 +75,7 @@ const tenantContext = new MockTenant( }, users: { findUserById: jest.fn().mockResolvedValue(mockUserWithMfaVerifications), + updateUserById, }, } ); @@ -216,6 +219,7 @@ describe('interaction routes (MFA verification)', () => { expect(response.status).toEqual(204); expect(getInteractionStorage).toBeCalled(); expect(verifyMfaPayloadVerification).toBeCalled(); + expect(updateUserById).toBeCalled(); expect(storeInteractionResult).toBeCalledWith( { verifiedMfa: { diff --git a/packages/core/src/routes/interaction/mfa.ts b/packages/core/src/routes/interaction/mfa.ts index 90d79fe83..a929dbb62 100644 --- a/packages/core/src/routes/interaction/mfa.ts +++ b/packages/core/src/routes/interaction/mfa.ts @@ -123,6 +123,21 @@ export default function mfaRoutes( { accountId, rpId: hostname, origin } ); + // Update last used time + const user = await queries.users.findUserById(accountId); + await queries.users.updateUserById(accountId, { + mfaVerifications: user.mfaVerifications.map((mfa) => { + if (mfa.id !== verifiedMfa.id) { + return mfa; + } + + return { + ...mfa, + lastUsedAt: new Date().toISOString(), + }; + }), + }); + await storeInteractionResult({ verifiedMfa }, ctx, provider, true); ctx.status = 204; diff --git a/packages/core/src/routes/interaction/verifications/mfa-payload-verification.test.ts b/packages/core/src/routes/interaction/verifications/mfa-payload-verification.test.ts index 541d986d5..c3a6d5b82 100644 --- a/packages/core/src/routes/interaction/verifications/mfa-payload-verification.test.ts +++ b/packages/core/src/routes/interaction/verifications/mfa-payload-verification.test.ts @@ -283,11 +283,15 @@ describe('verifyMfaPayloadVerification', () => { }); describe('webauthn', () => { + beforeEach(() => { + updateUserById.mockClear(); + }); + it('should return result of VerifyMfaResult and update newCounter', async () => { findUserById.mockResolvedValueOnce({ mfaVerifications: [mockUserWebAuthnMfaVerification], }); - const result = { type: MfaFactor.WebAuthn, id: 'id' }; + const result = { type: MfaFactor.WebAuthn, id: mockUserWebAuthnMfaVerification.id }; verifyWebAuthnAuthentication.mockResolvedValueOnce({ result, newCounter: 1, diff --git a/packages/core/src/routes/interaction/verifications/mfa-payload-verification.ts b/packages/core/src/routes/interaction/verifications/mfa-payload-verification.ts index bba3972a0..c791ccb83 100644 --- a/packages/core/src/routes/interaction/verifications/mfa-payload-verification.ts +++ b/packages/core/src/routes/interaction/verifications/mfa-payload-verification.ts @@ -232,7 +232,7 @@ export async function verifyMfaPayloadVerification( // Update the authenticator's counter in the DB to the newest count in the authentication await tenant.queries.users.updateUserById(accountId, { mfaVerifications: user.mfaVerifications.map((mfa) => { - if (mfa.type !== MfaFactor.WebAuthn) { + if (mfa.type !== MfaFactor.WebAuthn || mfa.id !== result.id) { return mfa; } diff --git a/packages/core/src/routes/interaction/verifications/mfa-verification.test.ts b/packages/core/src/routes/interaction/verifications/mfa-verification.test.ts index 27fcf3261..7c120eff5 100644 --- a/packages/core/src/routes/interaction/verifications/mfa-verification.test.ts +++ b/packages/core/src/routes/interaction/verifications/mfa-verification.test.ts @@ -10,6 +10,8 @@ import { mockUser, mockUserWebAuthnMfaVerification, mockUserWithMfaVerifications, + mockUserTotpMfaVerification, + mockUserBackupCodeMfaVerification, } from '#src/__mocks__/user.js'; import RequestError from '#src/errors/RequestError/index.js'; import { MockTenant } from '#src/test-utils/tenant.js'; @@ -84,13 +86,13 @@ const mfaRequiredTotpOnlyCtx = { }, }; -const backupCodeEnabledCtx = { +const allFactorsEnabledCtx = { ...baseCtx, signInExperience: { ...mockSignInExperience, mfa: { factors: [MfaFactor.TOTP, MfaFactor.WebAuthn, MfaFactor.BackupCode], - policy: MfaPolicy.Mandatory, + policy: MfaPolicy.UserControlled, }, }, }; @@ -348,4 +350,40 @@ describe('verifyMfa', () => { }) ).rejects.toThrowError(); }); + + it('should reject and sort availableFactors', async () => { + findUserById.mockResolvedValueOnce({ + ...mockUser, + mfaVerifications: [ + { + ...mockUserWebAuthnMfaVerification, + lastUsedAt: new Date('2021-01-01').toISOString(), + }, + { + ...mockUserBackupCodeMfaVerification, + lastUsedAt: new Date('2023-01-01').toISOString(), + }, + { + ...mockUserTotpMfaVerification, + lastUsedAt: new Date('2022-01-01').toISOString(), + }, + ], + }); + await expect( + verifyMfa(allFactorsEnabledCtx, tenantContext, { + ...signInInteraction, + verifiedMfa: undefined, + }) + ).rejects.toMatchError( + new RequestError( + { + code: 'session.mfa.require_mfa_verification', + status: 403, + }, + { + availableFactors: [MfaFactor.TOTP, MfaFactor.WebAuthn, MfaFactor.BackupCode], + } + ) + ); + }); }); diff --git a/packages/core/src/routes/interaction/verifications/mfa-verification.ts b/packages/core/src/routes/interaction/verifications/mfa-verification.ts index 4ea9dee2c..392f48578 100644 --- a/packages/core/src/routes/interaction/verifications/mfa-verification.ts +++ b/packages/core/src/routes/interaction/verifications/mfa-verification.ts @@ -1,5 +1,10 @@ -import { InteractionEvent, MfaFactor, MfaPolicy, type JsonObject } from '@logto/schemas'; -import { deduplicate } from '@silverhand/essentials'; +import { + InteractionEvent, + MfaFactor, + MfaPolicy, + type JsonObject, + type MfaVerification, +} from '@logto/schemas'; import { type Context } from 'koa'; import type Provider from 'oidc-provider'; import { z } from 'zod'; @@ -61,8 +66,43 @@ export const verifyMfa = async ( const { accountId, verifiedMfa } = interaction; const { mfaVerifications } = await tenant.queries.users.findUserById(accountId); - // Only allow MFA that is configured in sign-in experience - const availableUserVerifications = mfaVerifications.filter(({ type }) => factors.includes(type)); + const availableUserVerifications = mfaVerifications + .filter((verification) => { + // Only allow MFA that is configured in sign-in experience + if (!factors.includes(verification.type)) { + return false; + } + + if (verification.type !== MfaFactor.BackupCode) { + return true; + } + + // Skip backup code if it is used + return verification.codes.some((code) => !code.usedAt); + }) + .reduce((factors, verification) => { + // Ingnore duplicated verification + if (factors.some(({ type }) => type === verification.type)) { + return factors; + } + + return [...factors, verification]; + }, []) + .slice() + .sort((factorA, factorB) => { + // Sort by last used time, the latest used factor is the first one, backup code is always the last one + if (factorA.type === MfaFactor.BackupCode) { + return 1; + } + + if (factorB.type === MfaFactor.BackupCode) { + return -1; + } + + return ( + new Date(factorB.lastUsedAt ?? 0).getTime() - new Date(factorA.lastUsedAt ?? 0).getTime() + ); + }); if (availableUserVerifications.length > 0) { assertThat( @@ -73,7 +113,7 @@ export const verifyMfa = async ( status: 403, }, { - availableFactors: deduplicate(availableUserVerifications.map(({ type }) => type)), + availableFactors: availableUserVerifications.map(({ type }) => type), } ) ); diff --git a/packages/schemas/src/foundations/jsonb-types/users.ts b/packages/schemas/src/foundations/jsonb-types/users.ts index 33c0e238b..aa6cc6347 100644 --- a/packages/schemas/src/foundations/jsonb-types/users.ts +++ b/packages/schemas/src/foundations/jsonb-types/users.ts @@ -16,6 +16,7 @@ export type Identities = z.infer; export const baseMfaVerification = { id: z.string(), createdAt: z.string(), + lastUsedAt: z.string().optional(), }; export const mfaVerificationTotp = z.object({