From 60a6d677d7cb27b567ddf97b6d273f60db202d09 Mon Sep 17 00:00:00 2001 From: Amin Tarbalouti <12465359+atarbalouti@users.noreply.github.com> Date: Tue, 29 Oct 2024 08:59:13 +0100 Subject: [PATCH] feat(core): initial implementation of user set mfa secrets / codes (#6654) * feat(core): initial implementation of user set mfa secrets / codes * refactor(core): move totp secret validation to util * chore(core): fix openapi document * chore(core): fix unit tests --------- Co-authored-by: wangsijie --- .../admin-user/mfa-verification.openapi.json | 36 ++++++++- .../admin-user/mfa-verifications.test.ts | 75 +++++++++++++++++++ .../routes/admin-user/mfa-verifications.ts | 43 +++++++++-- .../utils/backup-code-validation.ts | 11 ++- .../interaction/utils/totp-validation.test.ts | 14 +++- .../interaction/utils/totp-validation.ts | 7 ++ .../phrases/src/locales/en/errors/user.ts | 2 + 7 files changed, 174 insertions(+), 14 deletions(-) diff --git a/packages/core/src/routes/admin-user/mfa-verification.openapi.json b/packages/core/src/routes/admin-user/mfa-verification.openapi.json index 22a161f39..dcbb608e3 100644 --- a/packages/core/src/routes/admin-user/mfa-verification.openapi.json +++ b/packages/core/src/routes/admin-user/mfa-verification.openapi.json @@ -15,11 +15,39 @@ "content": { "application/json": { "schema": { - "properties": { - "type": { - "description": "The type of MFA verification to create." + "oneOf": [ + { + "type": "object", + "properties": { + "type": { + "type": "string", + "description": "The type of MFA verification to create." + }, + "secret": { + "type": "string", + "description": "The secret for the MFA verification, if not provided, a new secret will be generated." + } + }, + "required": ["type"] + }, + { + "type": "object", + "properties": { + "type": { + "type": "string", + "description": "The type of MFA verification to create." + }, + "codes": { + "type": "array", + "items": { + "type": "string" + }, + "description": "The backup codes for the MFA verification, if not provided, a new group of backup codes will be generated." + } + }, + "required": ["type"] } - } + ] } } } diff --git a/packages/core/src/routes/admin-user/mfa-verifications.test.ts b/packages/core/src/routes/admin-user/mfa-verifications.test.ts index 26c26b59c..e8dd8b0d2 100644 --- a/packages/core/src/routes/admin-user/mfa-verifications.test.ts +++ b/packages/core/src/routes/admin-user/mfa-verifications.test.ts @@ -59,6 +59,19 @@ const usersLibraries = { ), } satisfies Partial; +const codes = [ + 'd94c2f29ae', + '74fa801bb7', + '2cbcc9323c', + '87299f89aa', + '0d95df8598', + '78eedbf35d', + '0fa4c1fd19', + '7384b69eb5', + '7bf2481db7', + 'f00febc9ae', +]; + const adminUserRoutes = await pickDefault(import('./mfa-verifications.js')); describe('adminUserRoutes', () => { @@ -103,6 +116,29 @@ describe('adminUserRoutes', () => { }); expect(response.status).toEqual(422); }); + + it('should fail for malformed secret', async () => { + findUserById.mockResolvedValueOnce(mockUser); + const response = await userRequest.post(`/users/${mockUser.id}/mfa-verifications`).send({ + type: MfaFactor.TOTP, + secret: 'invalid_secret', + }); + expect(response.status).toEqual(422); + }); + + it('should return supplied secret', async () => { + findUserById.mockResolvedValueOnce(mockUser); + const response = await userRequest.post(`/users/${mockUser.id}/mfa-verifications`).send({ + type: MfaFactor.TOTP, + secret: 'JBSWY3DPEHPK3PXP', + }); + expect(response.body).toMatchObject({ + type: MfaFactor.TOTP, + secret: 'JBSWY3DPEHPK3PXP', + // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment + secretQrCode: expect.stringContaining('data:image'), + }); + }); }); }); @@ -142,6 +178,45 @@ describe('adminUserRoutes', () => { }); expect(response.status).toEqual(422); }); + + it('should fail for wrong length', async () => { + findUserById.mockResolvedValueOnce({ + ...mockUser, + mfaVerifications: [mockUserTotpMfaVerification], + }); + const response = await userRequest.post(`/users/${mockUser.id}/mfa-verifications`).send({ + type: MfaFactor.BackupCode, + codes: ['wrong-code'], + }); + expect(response.status).toEqual(422); + }); + + it('should fail for wrong characters', async () => { + findUserById.mockResolvedValueOnce({ + ...mockUser, + mfaVerifications: [mockUserTotpMfaVerification], + }); + const response = await userRequest.post(`/users/${mockUser.id}/mfa-verifications`).send({ + type: MfaFactor.BackupCode, + codes: [...codes, '0fa4c1xd19'], + }); + expect(response.status).toEqual(422); + }); + + it('should return the supplied codes', async () => { + findUserById.mockResolvedValueOnce({ + ...mockUser, + mfaVerifications: [mockUserTotpMfaVerification], + }); + const response = await userRequest.post(`/users/${mockUser.id}/mfa-verifications`).send({ + type: MfaFactor.BackupCode, + codes, + }); + expect(response.body).toMatchObject({ + type: MfaFactor.BackupCode, + codes, + }); + }); }); it('DELETE /users/:userId/mfa-verifications/:verificationId', async () => { diff --git a/packages/core/src/routes/admin-user/mfa-verifications.ts b/packages/core/src/routes/admin-user/mfa-verifications.ts index bbcd5e0af..56a39e2e5 100644 --- a/packages/core/src/routes/admin-user/mfa-verifications.ts +++ b/packages/core/src/routes/admin-user/mfa-verifications.ts @@ -9,8 +9,11 @@ import koaGuard from '#src/middleware/koa-guard.js'; import assertThat from '#src/utils/assert-that.js'; import { transpileUserMfaVerifications } from '#src/utils/user.js'; -import { generateBackupCodes } from '../interaction/utils/backup-code-validation.js'; -import { generateTotpSecret } from '../interaction/utils/totp-validation.js'; +import { + generateBackupCodes, + validateBackupCodes, +} from '../interaction/utils/backup-code-validation.js'; +import { generateTotpSecret, validateTotpSecret } from '../interaction/utils/totp-validation.js'; import type { ManagementApiRouter, RouterInitArgs } from '../types.js'; export default function adminUserMfaVerificationsRoutes( @@ -47,9 +50,16 @@ export default function adminUserMfaVerificationsRoutes { - const codes = Array.from({ length: backupCodeCount }, () => customAlphabet(alphabet, 10)()); - return codes; + return Array.from({ length: backupCodeCount }, () => customAlphabet(alphabet, 10)()); +}; + +/** + * Validates a group of backup codes. + * @param codes + */ +export const validateBackupCodes = (codes: string[]) => { + return codes.length === backupCodeCount && codes.every((code) => /^[\da-f]{10}$/i.test(code)); }; diff --git a/packages/core/src/routes/interaction/utils/totp-validation.test.ts b/packages/core/src/routes/interaction/utils/totp-validation.test.ts index ebb4994d2..24a10a914 100644 --- a/packages/core/src/routes/interaction/utils/totp-validation.test.ts +++ b/packages/core/src/routes/interaction/utils/totp-validation.test.ts @@ -1,6 +1,8 @@ const { jest } = import.meta; -const { generateTotpSecret, validateTotpToken } = await import('./totp-validation.js'); +const { generateTotpSecret, validateTotpToken, validateTotpSecret } = await import( + './totp-validation.js' +); describe('generateTotpSecret', () => { it('should generate a secret', () => { @@ -8,6 +10,16 @@ describe('generateTotpSecret', () => { }); }); +describe('validateTotpSecret', () => { + it('should return true on valid secret', () => { + expect(validateTotpSecret(generateTotpSecret())).toBe(true); + }); + + it('should return false on invalid secret', () => { + expect(validateTotpSecret('invalid')).toBe(false); + }); +}); + describe('validateTotpToken', () => { beforeAll(() => { jest.useFakeTimers(); diff --git a/packages/core/src/routes/interaction/utils/totp-validation.ts b/packages/core/src/routes/interaction/utils/totp-validation.ts index 7b404eca3..90b033f05 100644 --- a/packages/core/src/routes/interaction/utils/totp-validation.ts +++ b/packages/core/src/routes/interaction/utils/totp-validation.ts @@ -12,6 +12,13 @@ authenticator.options = { window: 1 }; export const generateTotpSecret = () => authenticator.generateSecret(); +export const validateTotpSecret = (secret: string) => { + const base32Regex = + /^(?:[2-7A-Z]{8})*(?:[2-7A-Z]{2}={6}|[2-7A-Z]{4}={4}|[2-7A-Z]{5}={3}|[2-7A-Z]{7}=)?$/; + + return secret.length >= 16 && secret.length <= 32 && base32Regex.test(secret); +}; + export const validateTotpToken = (secret: string, token: string) => { return authenticator.check(token, secret); }; diff --git a/packages/phrases/src/locales/en/errors/user.ts b/packages/phrases/src/locales/en/errors/user.ts index bd95b5f10..eb1843366 100644 --- a/packages/phrases/src/locales/en/errors/user.ts +++ b/packages/phrases/src/locales/en/errors/user.ts @@ -35,6 +35,8 @@ const user = { password_algorithm_required: 'Password algorithm is required.', password_and_digest: 'You cannot set both plain text password and password digest.', personal_access_token_name_exists: 'Personal access token name already exists.', + totp_secret_invalid: 'Invalid TOTP secret supplied.', + wrong_backup_code_format: 'Backup code format is invalid.', }; export default Object.freeze(user);