Infrastructure/logto
0
Fork 0
mirror of https://github.com/logto-io/logto.git synced 2025-01-06 20:40:08 -05:00
Code Issues Activity

fix(core): block the use of unavailable MFA (#4756)

Browse source
This commit is contained in:
wangsijie 2023-10-27 15:14:40 +08:00 committed by GitHub
parent 97d1dfaa90
commit 160fe94df7
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
4 changed files with 282 additions and 142 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

2
packages/core/src/routes/interaction/index.ts
View file

@ -337,7 +337,7 @@ export default function interactionRoutes<T extends AnonymousRouter>(
const accountVerifiedInteraction = await verifyIdentifier(ctx, tenant, interactionStorage);
const mfaVerifiedInteraction = isSignInInteractionResult(accountVerifiedInteraction)
? await verifyMfa(tenant, accountVerifiedInteraction)
? await verifyMfa(ctx, tenant, accountVerifiedInteraction)
: accountVerifiedInteraction;
const profileVerifiedInteraction = await verifyProfile(tenant, mfaVerifiedInteraction);

225
packages/core/src/routes/interaction/verifications/mfa-verification.backup-code.test.ts Normal file
View file

@ -0,0 +1,225 @@
import crypto from 'node:crypto';
import { PasswordPolicyChecker } from '@logto/core-kit';
import { InteractionEvent, MfaFactor, MfaPolicy } from '@logto/schemas';
import { createMockUtils } from '@logto/shared/esm';
import type Provider from 'oidc-provider';
import { mockBackupCodeBind, mockTotpBind } from '#src/__mocks__/mfa-verification.js';
import { mockSignInExperience } from '#src/__mocks__/sign-in-experience.js';
import {
mockUser,
mockUserBackupCodeMfaVerification,
mockUserWithMfaVerifications,
} from '#src/__mocks__/user.js';
import RequestError from '#src/errors/RequestError/index.js';
import { createMockProvider } from '#src/test-utils/oidc-provider.js';
import { MockTenant } from '#src/test-utils/tenant.js';
import { createContextWithRouteParameters } from '#src/utils/test-utils.js';
import type {
AccountVerifiedInteractionResult,
IdentifierVerifiedInteractionResult,
} from '../types/index.js';
const { jest } = import.meta;
const { mockEsmWithActual } = createMockUtils(jest);
const findUserById = jest.fn();
const updateUserById = jest.fn();
const tenantContext = new MockTenant(undefined, {
users: {
findUserById,
updateUserById,
},
});
const mockBackupCodes = ['foo'];
await mockEsmWithActual('../utils/backup-code-validation.js', () => ({
generateBackupCodes: jest.fn().mockReturnValue(mockBackupCodes),
}));
const { storeInteractionResult } = await mockEsmWithActual('../utils/interaction.js', () => ({
storeInteractionResult: jest.fn(),
}));
const { validateBindMfaBackupCode } = await import('./mfa-verification.js');
const baseCtx = {
...createContextWithRouteParameters(),
// eslint-disable-next-line @typescript-eslint/consistent-type-assertions
interactionDetails: {} as Awaited<ReturnType<Provider['interactionDetails']>>,
signInExperience: {
...mockSignInExperience,
mfa: {
factors: [MfaFactor.TOTP],
policy: MfaPolicy.UserControlled,
},
},
passwordPolicyChecker: new PasswordPolicyChecker(
mockSignInExperience.passwordPolicy,
crypto.subtle
),
};
const mfaRequiredCtx = {
...baseCtx,
signInExperience: {
...mockSignInExperience,
mfa: {
factors: [MfaFactor.TOTP, MfaFactor.WebAuthn],
policy: MfaPolicy.Mandatory,
},
},
};
const backupCodeEnabledCtx = {
...baseCtx,
signInExperience: {
...mockSignInExperience,
mfa: {
factors: [MfaFactor.TOTP, MfaFactor.WebAuthn, MfaFactor.BackupCode],
policy: MfaPolicy.Mandatory,
},
},
};
const interaction: IdentifierVerifiedInteractionResult = {
event: InteractionEvent.Register,
identifiers: [{ key: 'accountId', value: 'foo' }],
};
const signInInteraction: AccountVerifiedInteractionResult = {
event: InteractionEvent.SignIn,
identifiers: [{ key: 'accountId', value: 'foo' }],
accountId: 'foo',
};
const provider = createMockProvider();
describe('validateBindMfaBackupCode', () => {
it('should pass if backup code is not enabled', async () => {
await expect(
validateBindMfaBackupCode(
tenantContext,
mfaRequiredCtx,
{
...signInInteraction,
bindMfas: [mockTotpBind],
},
provider
)
).resolves.not.toThrow();
});
it('should pass if backup code is set in bindMfas', async () => {
await expect(
validateBindMfaBackupCode(
tenantContext,
backupCodeEnabledCtx,
{
...signInInteraction,
bindMfas: [mockTotpBind, mockBackupCodeBind],
},
provider
)
).resolves.not.toThrow();
});
it('should pass if there is no other MFA for register', async () => {
await expect(
validateBindMfaBackupCode(tenantContext, backupCodeEnabledCtx, interaction, provider)
).resolves.not.toThrow();
});
it('should pass if there is no other MFA for sign in', async () => {
findUserById.mockResolvedValueOnce(mockUser);
await expect(
validateBindMfaBackupCode(
tenantContext,
backupCodeEnabledCtx,
{
...signInInteraction,
},
provider
)
).resolves.not.toThrow();
});
it('should pass if backup code is set in user mfaVerifications', async () => {
findUserById.mockResolvedValueOnce({
...mockUser,
mfaVerifications: [mockUserBackupCodeMfaVerification],
});
await expect(
validateBindMfaBackupCode(
tenantContext,
backupCodeEnabledCtx,
{
...signInInteraction,
bindMfas: [mockTotpBind],
},
provider
)
).resolves.not.toThrow();
});
it('should reject if backup code is set in user mfaVerifications but used', async () => {
findUserById.mockResolvedValueOnce({
...mockUser,
mfaVerifications: [
{
...mockUserBackupCodeMfaVerification,
codes: [{ code: 'code', usedAt: new Date().toISOString() }],
},
],
});
await expect(
validateBindMfaBackupCode(
tenantContext,
backupCodeEnabledCtx,
{
...signInInteraction,
bindMfas: [mockTotpBind],
},
provider
)
).rejects.toThrowError(
new RequestError(
{ code: 'session.mfa.backup_code_required', status: 422 },
{ codes: mockBackupCodes }
)
);
});
it('should reject if backup code is not set', async () => {
findUserById.mockResolvedValueOnce(mockUserWithMfaVerifications);
await expect(
validateBindMfaBackupCode(
tenantContext,
backupCodeEnabledCtx,
{
...signInInteraction,
bindMfas: [],
},
provider
)
).rejects.toThrowError(
new RequestError(
{ code: 'session.mfa.backup_code_required', status: 422 },
{ codes: mockBackupCodes }
)
);
expect(storeInteractionResult).toHaveBeenCalledWith(
{
pendingMfa: { type: MfaFactor.BackupCode, codes: mockBackupCodes },
},
expect.anything(),
expect.anything(),
expect.anything()
);
});
});

181
packages/core/src/routes/interaction/verifications/mfa-verification.test.ts
View file

@ -5,11 +5,10 @@ import { InteractionEvent, MfaFactor, MfaPolicy } from '@logto/schemas';
import { createMockUtils } from '@logto/shared/esm';
import type Provider from 'oidc-provider';
import { mockBackupCodeBind, mockTotpBind } from '#src/__mocks__/mfa-verification.js';
import { mockSignInExperience } from '#src/__mocks__/sign-in-experience.js';
import {
mockUser,
mockUserBackupCodeMfaVerification,
mockUserWebAuthnMfaVerification,
mockUserWithMfaVerifications,
} from '#src/__mocks__/user.js';
import RequestError from '#src/errors/RequestError/index.js';
@ -40,12 +39,9 @@ await mockEsmWithActual('../utils/backup-code-validation.js', () => ({
generateBackupCodes: jest.fn().mockReturnValue(mockBackupCodes),
}));
const { storeInteractionResult } = await mockEsmWithActual('../utils/interaction.js', () => ({
storeInteractionResult: jest.fn(),
}));
const { validateMandatoryBindMfa, verifyBindMfa, verifyMfa, validateBindMfaBackupCode } =
await import('./mfa-verification.js');
const { validateMandatoryBindMfa, verifyBindMfa, verifyMfa } = await import(
'./mfa-verification.js'
);
const baseCtx = {
...createContextWithRouteParameters(),
@ -75,6 +71,17 @@ const mfaRequiredCtx = {
},
};
const mfaRequiredTotpOnlyCtx = {
...baseCtx,
signInExperience: {
...mockSignInExperience,
mfa: {
factors: [MfaFactor.TOTP],
policy: MfaPolicy.Mandatory,
},
},
};
const backupCodeEnabledCtx = {
...baseCtx,
signInExperience: {
@ -211,6 +218,24 @@ describe('validateMandatoryBindMfa', () => {
validateMandatoryBindMfa(tenantContext, baseCtx, signInInteraction)
).resolves.not.toThrow();
});
it('user mfaVerifications existing (unavailable factor), bindMfa missing and required should throw', async () => {
findUserById.mockResolvedValueOnce({
...mockUser,
mfaVerifications: [mockUserWebAuthnMfaVerification],
});
await expect(
validateMandatoryBindMfa(tenantContext, mfaRequiredTotpOnlyCtx, signInInteraction)
).rejects.toMatchError(
new RequestError(
{
code: 'user.missing_mfa',
status: 422,
},
{ availableFactors: [MfaFactor.TOTP] }
)
);
});
});
});
@ -267,13 +292,21 @@ describe('verifyBindMfa', () => {
describe('verifyMfa', () => {
it('should pass if user mfaVerifications is empty', async () => {
findUserById.mockResolvedValueOnce(mockUser);
await expect(verifyMfa(tenantContext, signInInteraction)).resolves.not.toThrow();
await expect(verifyMfa(baseCtx, tenantContext, signInInteraction)).resolves.not.toThrow();
});
it('should pass if user mfaVerifications is not empty but no available factor', async () => {
findUserById.mockResolvedValueOnce({
...mockUser,
mfaVerifications: [mockUserWebAuthnMfaVerification],
});
await expect(verifyMfa(baseCtx, tenantContext, signInInteraction)).resolves.not.toThrow();
});
it('should pass if verifiedMfa exists', async () => {
findUserById.mockResolvedValueOnce(mockUserWithMfaVerifications);
await expect(
verifyMfa(tenantContext, {
verifyMfa(baseCtx, tenantContext, {
...signInInteraction,
verifiedMfa: {
type: MfaFactor.TOTP,
@ -286,136 +319,10 @@ describe('verifyMfa', () => {
it('should reject if verifiedMfa can not be found', async () => {
findUserById.mockResolvedValueOnce(mockUserWithMfaVerifications);
await expect(
verifyMfa(tenantContext, {
verifyMfa(baseCtx, tenantContext, {
...signInInteraction,
verifiedMfa: undefined,
})
).rejects.toThrowError();
});
});
describe('validateBindMfaBackupCode', () => {
it('should pass if backup code is not enabled', async () => {
await expect(
validateBindMfaBackupCode(
tenantContext,
mfaRequiredCtx,
{
...signInInteraction,
bindMfas: [mockTotpBind],
},
provider
)
).resolves.not.toThrow();
});
it('should pass if backup code is set in bindMfas', async () => {
await expect(
validateBindMfaBackupCode(
tenantContext,
backupCodeEnabledCtx,
{
...signInInteraction,
bindMfas: [mockTotpBind, mockBackupCodeBind],
},
provider
)
).resolves.not.toThrow();
});
it('should pass if there is no other MFA for register', async () => {
await expect(
validateBindMfaBackupCode(tenantContext, backupCodeEnabledCtx, interaction, provider)
).resolves.not.toThrow();
});
it('should pass if there is no other MFA for sign in', async () => {
findUserById.mockResolvedValueOnce(mockUser);
await expect(
validateBindMfaBackupCode(
tenantContext,
backupCodeEnabledCtx,
{
...signInInteraction,
},
provider
)
).resolves.not.toThrow();
});
it('should pass if backup code is set in user mfaVerifications', async () => {
findUserById.mockResolvedValueOnce({
...mockUser,
mfaVerifications: [mockUserBackupCodeMfaVerification],
});
await expect(
validateBindMfaBackupCode(
tenantContext,
backupCodeEnabledCtx,
{
...signInInteraction,
bindMfas: [mockTotpBind],
},
provider
)
).resolves.not.toThrow();
});
it('should reject if backup code is set in user mfaVerifications but used', async () => {
findUserById.mockResolvedValueOnce({
...mockUser,
mfaVerifications: [
{
...mockUserBackupCodeMfaVerification,
codes: [{ code: 'code', usedAt: new Date().toISOString() }],
},
],
});
await expect(
validateBindMfaBackupCode(
tenantContext,
backupCodeEnabledCtx,
{
...signInInteraction,
bindMfas: [mockTotpBind],
},
provider
)
).rejects.toThrowError(
new RequestError(
{ code: 'session.mfa.backup_code_required', status: 422 },
{ codes: mockBackupCodes }
)
);
});
it('should reject if backup code is not set', async () => {
findUserById.mockResolvedValueOnce(mockUserWithMfaVerifications);
await expect(
validateBindMfaBackupCode(
tenantContext,
backupCodeEnabledCtx,
{
...signInInteraction,
bindMfas: [],
},
provider
)
).rejects.toThrowError(
new RequestError(
{ code: 'session.mfa.backup_code_required', status: 422 },
{ codes: mockBackupCodes }
)
);
expect(storeInteractionResult).toHaveBeenCalledWith(
{
pendingMfa: { type: MfaFactor.BackupCode, codes: mockBackupCodes },
},
expect.anything(),
expect.anything(),
expect.anything()
);
});
});

16
packages/core/src/routes/interaction/verifications/mfa-verification.ts
View file

@ -49,14 +49,22 @@ export const verifyBindMfa = async (
};
export const verifyMfa = async (
ctx: WithInteractionSieContext,
tenant: TenantContext,
interaction: AccountVerifiedInteractionResult
): Promise<AccountVerifiedInteractionResult> => {
const {
signInExperience: {
mfa: { factors },
},
} = ctx;
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));
if (mfaVerifications.length > 0) {
if (availableUserVerifications.length > 0) {
assertThat(
verifiedMfa,
new RequestError(
@ -65,7 +73,7 @@ export const verifyMfa = async (
status: 403,
},
{
availableFactors: deduplicate(mfaVerifications.map(({ type }) => type)),
availableFactors: deduplicate(availableUserVerifications.map(({ type }) => type)),
}
)
);
@ -113,8 +121,8 @@ const validateMandatoryBindMfaForSignIn = async (
const { accountId } = interaction;
const { mfaVerifications, customData } = await tenant.queries.users.findUserById(accountId);
// If the user has linked MFA before
const hasFactorInUser = factors.some((factor) =>
// If the user has linked currently available MFA before
const hasFactorInUser = availableFactors.some((factor) =>
mfaVerifications.some(({ type }) => type === factor)
);