diff --git a/packages/cli/src/controllers/__tests__/mfa.controller.test.ts b/packages/cli/src/controllers/__tests__/mfa.controller.test.ts index 4b69b66b330..494d6a2b9fc 100644 --- a/packages/cli/src/controllers/__tests__/mfa.controller.test.ts +++ b/packages/cli/src/controllers/__tests__/mfa.controller.test.ts @@ -80,10 +80,7 @@ describe('MFAController', () => { mfaService.validateProof.mockResolvedValue(true); webauthnService.getUserCredentials.mockResolvedValue([{ id: credId } as never]); mfaService.isMFAEnforced.mockResolvedValue(true); - mfaService.getSecretAndRecoveryCodes.mockResolvedValue({ - decryptedSecret: '', - decryptedRecoveryCodes: [], - }); + mfaService.hasTotpSecret.mockResolvedValue(false); await expect( controller.deleteWebAuthnCredential(mkReq({ mfaCode: '123456' }), mock()), @@ -95,10 +92,7 @@ describe('MFAController', () => { mfaService.validateProof.mockResolvedValue(true); webauthnService.getUserCredentials.mockResolvedValue([{ id: credId } as never]); mfaService.isMFAEnforced.mockResolvedValue(true); - mfaService.getSecretAndRecoveryCodes.mockResolvedValue({ - decryptedSecret: 'totp-secret', - decryptedRecoveryCodes: ['rc'], - }); + mfaService.hasTotpSecret.mockResolvedValue(true); webauthnService.deleteCredential.mockResolvedValue(true); const res = mock(); @@ -114,10 +108,7 @@ describe('MFAController', () => { mfaService.validateProof.mockResolvedValue(true); webauthnService.getUserCredentials.mockResolvedValue([{ id: credId } as never]); mfaService.isMFAEnforced.mockResolvedValue(false); - mfaService.getSecretAndRecoveryCodes.mockResolvedValue({ - decryptedSecret: '', - decryptedRecoveryCodes: [], - }); + mfaService.hasTotpSecret.mockResolvedValue(false); webauthnService.deleteCredential.mockResolvedValue(true); const res = mock(); @@ -142,10 +133,7 @@ describe('MFAController', () => { { id: 'cred-2' }, ] as never); mfaService.isMFAEnforced.mockResolvedValue(false); - mfaService.getSecretAndRecoveryCodes.mockResolvedValue({ - decryptedSecret: '', - decryptedRecoveryCodes: [], - }); + mfaService.hasTotpSecret.mockResolvedValue(false); webauthnService.deleteCredential.mockResolvedValue(true); const res = mock(); @@ -159,10 +147,7 @@ describe('MFAController', () => { mfaService.validateProof.mockResolvedValue(true); webauthnService.getUserCredentials.mockResolvedValue([{ id: credId } as never]); mfaService.isMFAEnforced.mockResolvedValue(false); - mfaService.getSecretAndRecoveryCodes.mockResolvedValue({ - decryptedSecret: 'totp-secret', - decryptedRecoveryCodes: ['rc'], - }); + mfaService.hasTotpSecret.mockResolvedValue(true); webauthnService.deleteCredential.mockResolvedValue(false); await expect( diff --git a/packages/cli/src/controllers/mfa.controller.ts b/packages/cli/src/controllers/mfa.controller.ts index 56fd8815e1f..000891b9f30 100644 --- a/packages/cli/src/controllers/mfa.controller.ts +++ b/packages/cli/src/controllers/mfa.controller.ts @@ -206,10 +206,6 @@ export class MFAController { if (!verified) throw new BadRequestError('MFA secret could not be verified'); } - // --------------------------------------------------------------- - // WebAuthn endpoints - // --------------------------------------------------------------- - @Get('/webauthn/registration-options', { allowSkipMFA: true, }) @@ -361,16 +357,13 @@ export class MFAController { throw new BadRequestError('Invalid two-factor proof'); } - const [credentialsBefore, isMFAEnforced, { decryptedSecret }] = await Promise.all([ + const [credentialsBefore, isMFAEnforced, hasTotpSecret] = await Promise.all([ this.mfaService.webauthn.getUserCredentials(userId), this.mfaService.isMFAEnforced(), - this.mfaService.getSecretAndRecoveryCodes(userId), + this.mfaService.hasTotpSecret(userId), ]); const remainingWebauthn = credentialsBefore.filter((c) => c.id !== id).length; - const hasTotpSecret = !!decryptedSecret; - // Refuse if removing this credential would leave the user with no - // authentication factor on an instance that enforces MFA. if (isMFAEnforced && remainingWebauthn === 0 && !hasTotpSecret) { throw new BadRequestError( 'You can’t remove your last MFA credential while two-factor authentication is required on this instance', @@ -382,9 +375,8 @@ export class MFAController { throw new BadRequestError('Credential not found'); } - // If this was the last MFA factor, mirror disable-MFA semantics so the - // user's session reflects the new state. The pre-delete read above - // already told us what's left — no need to re-query. + // Mirror disable-MFA semantics when the last factor is gone so the + // session reflects the new state. if (remainingWebauthn === 0 && !hasTotpSecret) { await this.userRepository.update(userId, { mfaEnabled: false, diff --git a/packages/cli/src/mfa/mfa.service.ts b/packages/cli/src/mfa/mfa.service.ts index fadfa3ac5dd..8ef32697076 100644 --- a/packages/cli/src/mfa/mfa.service.ts +++ b/packages/cli/src/mfa/mfa.service.ts @@ -121,6 +121,14 @@ export class MfaService { return await this.decryptSecretAndRecoveryCodes(mfaSecret ?? '', mfaRecoveryCodes ?? []); } + async hasTotpSecret(userId: string): Promise { + const user = await this.userRepository.findOne({ + where: { id: userId }, + select: { mfaSecret: true }, + }); + return !!user?.mfaSecret; + } + async validateMfa( userId: string, mfaCode: string | undefined, @@ -164,18 +172,16 @@ export class MfaService { async getAvailableMfaMethods( userId: string, ): Promise> { + const [user, credentials] = await Promise.all([ + this.userRepository.findOneByOrFail({ id: userId }), + this.webauthn.getUserCredentials(userId), + ]); + const methods = new Set<'totp' | 'passkey' | 'security_key'>(); - - const user = await this.userRepository.findOneByOrFail({ id: userId }); - if (user.mfaSecret) { - methods.add('totp'); - } - - const credentials = await this.webauthn.getUserCredentials(userId); + if (user.mfaSecret) methods.add('totp'); for (const cred of credentials) { methods.add(isPlatformCredential(cred) ? 'passkey' : 'security_key'); } - return Array.from(methods); }