From 5887ed649806eb1f1ba8d0d959f8a074fdd9fd1c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E0=A4=95=E0=A4=BE=E0=A4=B0=E0=A4=A4=E0=A5=8B=E0=A4=AB?= =?UTF-8?q?=E0=A5=8D=E0=A4=AB=E0=A5=87=E0=A4=B2=E0=A4=B8=E0=A5=8D=E0=A4=95?= =?UTF-8?q?=E0=A5=8D=E0=A4=B0=E0=A4=BF=E0=A4=AA=E0=A5=8D=E0=A4=9F=E2=84=A2?= Date: Fri, 31 May 2024 09:40:19 +0200 Subject: [PATCH] refactor(core): Extract all Auth-related User columns into a separate entity (#9557) Co-authored-by: Ricardo Espinoza --- packages/cli/src/Interfaces.ts | 1 - packages/cli/src/Mfa/mfa.service.ts | 82 +++---- packages/cli/src/commands/mfa/disable.ts | 7 +- .../cli/src/controllers/auth.controller.ts | 38 +--- .../cli/src/controllers/e2e.controller.ts | 21 +- .../cli/src/controllers/users.controller.ts | 1 - .../cli/src/databases/entities/AuthUser.ts | 19 ++ packages/cli/src/databases/entities/User.ts | 8 +- packages/cli/src/databases/entities/index.ts | 2 + .../repositories/authUser.repository.ts | 10 + .../databases/repositories/user.repository.ts | 1 + packages/cli/src/services/user.service.ts | 4 +- .../cli/test/integration/mfa/mfa.api.test.ts | 215 ++++++------------ .../cli/test/integration/shared/db/users.ts | 20 +- .../databases/entities/user.entity.test.ts | 2 - .../test/unit/services/user.service.test.ts | 4 +- packages/editor-ui/src/Interface.ts | 1 - .../editor-ui/src/__tests__/data/users.ts | 1 - .../src/__tests__/server/factories/user.ts | 3 - .../__tests__/PersonalizationModal.spec.ts | 1 - packages/editor-ui/src/views/MfaView.vue | 6 +- packages/editor-ui/src/views/SigninView.vue | 16 -- .../__tests__/SettingsPersonalView.test.ts | 1 - 23 files changed, 182 insertions(+), 282 deletions(-) create mode 100644 packages/cli/src/databases/entities/AuthUser.ts create mode 100644 packages/cli/src/databases/repositories/authUser.repository.ts diff --git a/packages/cli/src/Interfaces.ts b/packages/cli/src/Interfaces.ts index 32825f2aba..9a6cffd782 100644 --- a/packages/cli/src/Interfaces.ts +++ b/packages/cli/src/Interfaces.ts @@ -624,7 +624,6 @@ export interface PublicUser { passwordResetToken?: string; createdAt: Date; isPending: boolean; - hasRecoveryCodesLeft: boolean; role?: GlobalRole; globalScopes?: Scope[]; signInType: AuthProviderType; diff --git a/packages/cli/src/Mfa/mfa.service.ts b/packages/cli/src/Mfa/mfa.service.ts index 53f1229a4d..91270cf4ab 100644 --- a/packages/cli/src/Mfa/mfa.service.ts +++ b/packages/cli/src/Mfa/mfa.service.ts @@ -1,43 +1,34 @@ import { v4 as uuid } from 'uuid'; import { Service } from 'typedi'; import { Cipher } from 'n8n-core'; -import { UserRepository } from '@db/repositories/user.repository'; +import { AuthUserRepository } from '@db/repositories/authUser.repository'; import { TOTPService } from './totp.service'; @Service() export class MfaService { constructor( - private userRepository: UserRepository, + private authUserRepository: AuthUserRepository, public totp: TOTPService, private cipher: Cipher, ) {} - public generateRecoveryCodes(n = 10) { + generateRecoveryCodes(n = 10) { return Array.from(Array(n)).map(() => uuid()); } - public generateEncryptedRecoveryCodes() { - return this.generateRecoveryCodes().map((code) => this.cipher.encrypt(code)); - } - - public async saveSecretAndRecoveryCodes(userId: string, secret: string, recoveryCodes: string[]) { + async saveSecretAndRecoveryCodes(userId: string, secret: string, recoveryCodes: string[]) { const { encryptedSecret, encryptedRecoveryCodes } = this.encryptSecretAndRecoveryCodes( secret, recoveryCodes, ); - const user = await this.userRepository.findOneBy({ id: userId }); - if (user) { - Object.assign(user, { - mfaSecret: encryptedSecret, - mfaRecoveryCodes: encryptedRecoveryCodes, - }); - - await this.userRepository.save(user); - } + const user = await this.authUserRepository.findOneByOrFail({ id: userId }); + user.mfaSecret = encryptedSecret; + user.mfaRecoveryCodes = encryptedRecoveryCodes; + await this.authUserRepository.save(user); } - public encryptSecretAndRecoveryCodes(rawSecret: string, rawRecoveryCodes: string[]) { + encryptSecretAndRecoveryCodes(rawSecret: string, rawRecoveryCodes: string[]) { const encryptedSecret = this.cipher.encrypt(rawSecret), encryptedRecoveryCodes = rawRecoveryCodes.map((code) => this.cipher.encrypt(code)); return { @@ -53,37 +44,46 @@ export class MfaService { }; } - public async getSecretAndRecoveryCodes(userId: string) { - const { mfaSecret, mfaRecoveryCodes } = await this.userRepository.findOneOrFail({ - where: { id: userId }, - select: ['id', 'mfaSecret', 'mfaRecoveryCodes'], + async getSecretAndRecoveryCodes(userId: string) { + const { mfaSecret, mfaRecoveryCodes } = await this.authUserRepository.findOneByOrFail({ + id: userId, }); return this.decryptSecretAndRecoveryCodes(mfaSecret ?? '', mfaRecoveryCodes ?? []); } - public async enableMfa(userId: string) { - const user = await this.userRepository.findOneBy({ id: userId }); - if (user) { - user.mfaEnabled = true; - - await this.userRepository.save(user); + async validateMfa( + userId: string, + mfaToken: string | undefined, + mfaRecoveryCode: string | undefined, + ) { + const user = await this.authUserRepository.findOneByOrFail({ id: userId }); + if (mfaToken) { + const decryptedSecret = this.cipher.decrypt(user.mfaSecret!); + return this.totp.verifySecret({ secret: decryptedSecret, token: mfaToken }); + } else if (mfaRecoveryCode) { + const validCodes = user.mfaRecoveryCodes.map((code) => this.cipher.decrypt(code)); + const index = validCodes.indexOf(mfaRecoveryCode); + if (index === -1) return false; + // remove used recovery code + validCodes.splice(index, 1); + user.mfaRecoveryCodes = validCodes.map((code) => this.cipher.encrypt(code)); + await this.authUserRepository.save(user); + return true; } + return false; } - public encryptRecoveryCodes(mfaRecoveryCodes: string[]) { - return mfaRecoveryCodes.map((code) => this.cipher.encrypt(code)); + async enableMfa(userId: string) { + const user = await this.authUserRepository.findOneByOrFail({ id: userId }); + user.mfaEnabled = true; + return await this.authUserRepository.save(user); } - public async disableMfa(userId: string) { - const user = await this.userRepository.findOneBy({ id: userId }); - - if (user) { - Object.assign(user, { - mfaEnabled: false, - mfaSecret: null, - mfaRecoveryCodes: [], - }); - await this.userRepository.save(user); - } + async disableMfa(userId: string) { + const user = await this.authUserRepository.findOneByOrFail({ id: userId }); + user.mfaEnabled = false; + user.mfaSecret = null; + user.mfaRecoveryCodes = []; + return await this.authUserRepository.save(user); } } diff --git a/packages/cli/src/commands/mfa/disable.ts b/packages/cli/src/commands/mfa/disable.ts index a839b56e0d..dceb81b8b9 100644 --- a/packages/cli/src/commands/mfa/disable.ts +++ b/packages/cli/src/commands/mfa/disable.ts @@ -1,6 +1,6 @@ import Container from 'typedi'; import { Flags } from '@oclif/core'; -import { UserRepository } from '@db/repositories/user.repository'; +import { AuthUserRepository } from '@db/repositories/authUser.repository'; import { BaseCommand } from '../BaseCommand'; export class DisableMFACommand extends BaseCommand { @@ -27,7 +27,8 @@ export class DisableMFACommand extends BaseCommand { return; } - const user = await Container.get(UserRepository).findOneBy({ email: flags.email }); + const repository = Container.get(AuthUserRepository); + const user = await repository.findOneBy({ email: flags.email }); if (!user) { this.reportUserDoesNotExistError(flags.email); @@ -46,7 +47,7 @@ export class DisableMFACommand extends BaseCommand { Object.assign(user, { mfaSecret: null, mfaRecoveryCodes: [], mfaEnabled: false }); - await Container.get(UserRepository).save(user); + await repository.save(user); this.reportSuccess(flags.email); } diff --git a/packages/cli/src/controllers/auth.controller.ts b/packages/cli/src/controllers/auth.controller.ts index 97de4c8d8e..39285b797e 100644 --- a/packages/cli/src/controllers/auth.controller.ts +++ b/packages/cli/src/controllers/auth.controller.ts @@ -79,16 +79,11 @@ export class AuthController { throw new AuthError('MFA Error', 998); } - const { decryptedRecoveryCodes, decryptedSecret } = - await this.mfaService.getSecretAndRecoveryCodes(user.id); - - user.mfaSecret = decryptedSecret; - user.mfaRecoveryCodes = decryptedRecoveryCodes; - - const isMFATokenValid = - (await this.validateMfaToken(user, mfaToken)) || - (await this.validateMfaRecoveryCode(user, mfaRecoveryCode)); - + const isMFATokenValid = await this.mfaService.validateMfa( + user.id, + mfaToken, + mfaRecoveryCode, + ); if (!isMFATokenValid) { throw new AuthError('Invalid mfa token or recovery code'); } @@ -193,27 +188,4 @@ export class AuthController { this.authService.clearCookie(res); return { loggedOut: true }; } - - private async validateMfaToken(user: User, token?: string) { - if (!!!token) return false; - return this.mfaService.totp.verifySecret({ - secret: user.mfaSecret ?? '', - token, - }); - } - - private async validateMfaRecoveryCode(user: User, mfaRecoveryCode?: string) { - if (!!!mfaRecoveryCode) return false; - const index = user.mfaRecoveryCodes.indexOf(mfaRecoveryCode); - if (index === -1) return false; - - // remove used recovery code - user.mfaRecoveryCodes.splice(index, 1); - - await this.userService.update(user.id, { - mfaRecoveryCodes: this.mfaService.encryptRecoveryCodes(user.mfaRecoveryCodes), - }); - - return true; - } } diff --git a/packages/cli/src/controllers/e2e.controller.ts b/packages/cli/src/controllers/e2e.controller.ts index e8fc7c5ca9..ea9364e854 100644 --- a/packages/cli/src/controllers/e2e.controller.ts +++ b/packages/cli/src/controllers/e2e.controller.ts @@ -16,6 +16,7 @@ import { CacheService } from '@/services/cache/cache.service'; import { PasswordUtility } from '@/services/password.utility'; import Container from 'typedi'; import { Logger } from '@/Logger'; +import { AuthUserRepository } from '@/databases/repositories/authUser.repository'; if (!inE2ETests) { Container.get(Logger).error('E2E endpoints only allowed during E2E tests'); @@ -106,6 +107,7 @@ export class E2EController { private readonly passwordUtility: PasswordUtility, private readonly eventBus: MessageEventBus, private readonly userRepository: UserRepository, + private readonly authUserRepository: AuthUserRepository, ) { license.isFeatureEnabled = (feature: BooleanLicenseFeature) => this.enabledFeatures[feature] ?? false; @@ -185,13 +187,6 @@ export class E2EController { members: UserSetupPayload[], admin: UserSetupPayload, ) { - if (owner?.mfaSecret && owner.mfaRecoveryCodes?.length) { - const { encryptedRecoveryCodes, encryptedSecret } = - this.mfaService.encryptSecretAndRecoveryCodes(owner.mfaSecret, owner.mfaRecoveryCodes); - owner.mfaSecret = encryptedSecret; - owner.mfaRecoveryCodes = encryptedRecoveryCodes; - } - const userCreatePromises = [ this.userRepository.createUserWithProject({ id: uuid(), @@ -221,7 +216,17 @@ export class E2EController { ); } - await Promise.all(userCreatePromises); + const [newOwner] = await Promise.all(userCreatePromises); + + if (owner?.mfaSecret && owner.mfaRecoveryCodes?.length) { + const { encryptedRecoveryCodes, encryptedSecret } = + this.mfaService.encryptSecretAndRecoveryCodes(owner.mfaSecret, owner.mfaRecoveryCodes); + + await this.authUserRepository.update(newOwner.user.id, { + mfaSecret: encryptedSecret, + mfaRecoveryCodes: encryptedRecoveryCodes, + }); + } await this.settingsRepo.update( { key: 'userManagement.isInstanceOwnerSetUp' }, diff --git a/packages/cli/src/controllers/users.controller.ts b/packages/cli/src/controllers/users.controller.ts index b4bfad1f91..ec7df4ff95 100644 --- a/packages/cli/src/controllers/users.controller.ts +++ b/packages/cli/src/controllers/users.controller.ts @@ -77,7 +77,6 @@ export class UsersController { delete user.isOwner; delete user.isPending; delete user.signInType; - delete user.hasRecoveryCodesLeft; } } diff --git a/packages/cli/src/databases/entities/AuthUser.ts b/packages/cli/src/databases/entities/AuthUser.ts new file mode 100644 index 0000000000..f9f8d89f8e --- /dev/null +++ b/packages/cli/src/databases/entities/AuthUser.ts @@ -0,0 +1,19 @@ +import { Column, Entity, PrimaryColumn } from '@n8n/typeorm'; + +@Entity({ name: 'user' }) +export class AuthUser { + @PrimaryColumn({ type: 'uuid', update: false }) + id: string; + + @Column({ type: String, update: false }) + email: string; + + @Column({ type: Boolean, default: false }) + mfaEnabled: boolean; + + @Column({ type: String, nullable: true }) + mfaSecret?: string | null; + + @Column({ type: 'simple-array', default: '' }) + mfaRecoveryCodes: string[]; +} diff --git a/packages/cli/src/databases/entities/User.ts b/packages/cli/src/databases/entities/User.ts index 9aeb62d92c..5755f24d84 100644 --- a/packages/cli/src/databases/entities/User.ts +++ b/packages/cli/src/databases/entities/User.ts @@ -109,12 +109,6 @@ export class User extends WithTimestamps implements IUser { @Column({ type: Boolean, default: false }) mfaEnabled: boolean; - @Column({ type: String, nullable: true, select: false }) - mfaSecret?: string | null; - - @Column({ type: 'simple-array', default: '', select: false }) - mfaRecoveryCodes: string[]; - /** * Whether the user is pending setup completion. */ @@ -152,7 +146,7 @@ export class User extends WithTimestamps implements IUser { } toJSON() { - const { password, apiKey, mfaSecret, mfaRecoveryCodes, ...rest } = this; + const { password, apiKey, ...rest } = this; return rest; } diff --git a/packages/cli/src/databases/entities/index.ts b/packages/cli/src/databases/entities/index.ts index 71be3c07bb..0f30e5bfc8 100644 --- a/packages/cli/src/databases/entities/index.ts +++ b/packages/cli/src/databases/entities/index.ts @@ -1,6 +1,7 @@ /* eslint-disable @typescript-eslint/naming-convention */ import { AuthIdentity } from './AuthIdentity'; import { AuthProviderSyncHistory } from './AuthProviderSyncHistory'; +import { AuthUser } from './AuthUser'; import { CredentialsEntity } from './CredentialsEntity'; import { EventDestinations } from './EventDestinations'; import { ExecutionEntity } from './ExecutionEntity'; @@ -25,6 +26,7 @@ import { ProjectRelation } from './ProjectRelation'; export const entities = { AuthIdentity, AuthProviderSyncHistory, + AuthUser, CredentialsEntity, EventDestinations, ExecutionEntity, diff --git a/packages/cli/src/databases/repositories/authUser.repository.ts b/packages/cli/src/databases/repositories/authUser.repository.ts new file mode 100644 index 0000000000..7db9ab89ad --- /dev/null +++ b/packages/cli/src/databases/repositories/authUser.repository.ts @@ -0,0 +1,10 @@ +import { Service } from 'typedi'; +import { DataSource, Repository } from '@n8n/typeorm'; +import { AuthUser } from '../entities/AuthUser'; + +@Service() +export class AuthUserRepository extends Repository { + constructor(dataSource: DataSource) { + super(AuthUser, dataSource.manager); + } +} diff --git a/packages/cli/src/databases/repositories/user.repository.ts b/packages/cli/src/databases/repositories/user.repository.ts index 4591c20498..a934b0f05f 100644 --- a/packages/cli/src/databases/repositories/user.repository.ts +++ b/packages/cli/src/databases/repositories/user.repository.ts @@ -6,6 +6,7 @@ import type { ListQuery } from '@/requests'; import { type GlobalRole, User } from '../entities/User'; import { Project } from '../entities/Project'; import { ProjectRelation } from '../entities/ProjectRelation'; + @Service() export class UserRepository extends Repository { constructor(dataSource: DataSource) { diff --git a/packages/cli/src/services/user.service.ts b/packages/cli/src/services/user.service.ts index e65e5a07c9..24343c8957 100644 --- a/packages/cli/src/services/user.service.ts +++ b/packages/cli/src/services/user.service.ts @@ -57,15 +57,13 @@ export class UserService { withScopes?: boolean; }, ) { - const { password, updatedAt, apiKey, authIdentities, mfaRecoveryCodes, mfaSecret, ...rest } = - user; + const { password, updatedAt, apiKey, authIdentities, ...rest } = user; const ldapIdentity = authIdentities?.find((i) => i.providerType === 'ldap'); let publicUser: PublicUser = { ...rest, signInType: ldapIdentity ? 'ldap' : 'email', - hasRecoveryCodesLeft: !!user.mfaRecoveryCodes?.length, }; if (options?.withInviteUrl && !options?.inviterId) { diff --git a/packages/cli/test/integration/mfa/mfa.api.test.ts b/packages/cli/test/integration/mfa/mfa.api.test.ts index d66261f60e..de4f80abde 100644 --- a/packages/cli/test/integration/mfa/mfa.api.test.ts +++ b/packages/cli/test/integration/mfa/mfa.api.test.ts @@ -3,7 +3,7 @@ import Container from 'typedi'; import { AuthService } from '@/auth/auth.service'; import config from '@/config'; import type { User } from '@db/entities/User'; -import { UserRepository } from '@db/repositories/user.repository'; +import { AuthUserRepository } from '@db/repositories/authUser.repository'; import { randomPassword } from '@/Ldap/helpers'; import { TOTPService } from '@/Mfa/totp.service'; @@ -35,24 +35,22 @@ afterAll(async () => { describe('Enable MFA setup', () => { describe('Step one', () => { test('GET /qr should fail due to unauthenticated user', async () => { - const response = await testServer.authlessAgent.get('/mfa/qr'); - - expect(response.statusCode).toBe(401); + await testServer.authlessAgent.get('/mfa/qr').expect(401); }); test('GET /qr should reuse secret and recovery codes until setup is complete', async () => { - const firstCall = await testServer.authAgentFor(owner).get('/mfa/qr'); + const firstCall = await testServer.authAgentFor(owner).get('/mfa/qr').expect(200); - const secondCall = await testServer.authAgentFor(owner).get('/mfa/qr'); + const secondCall = await testServer.authAgentFor(owner).get('/mfa/qr').expect(200); expect(firstCall.body.data.secret).toBe(secondCall.body.data.secret); expect(firstCall.body.data.recoveryCodes.join('')).toBe( secondCall.body.data.recoveryCodes.join(''), ); - await testServer.authAgentFor(owner).delete('/mfa/disable'); + await testServer.authAgentFor(owner).delete('/mfa/disable').expect(200); - const thirdCall = await testServer.authAgentFor(owner).get('/mfa/qr'); + const thirdCall = await testServer.authAgentFor(owner).get('/mfa/qr').expect(200); expect(firstCall.body.data.secret).not.toBe(thirdCall.body.data.secret); expect(firstCall.body.data.recoveryCodes.join('')).not.toBe( @@ -61,9 +59,7 @@ describe('Enable MFA setup', () => { }); test('GET /qr should return qr, secret and recovery codes', async () => { - const response = await testServer.authAgentFor(owner).get('/mfa/qr'); - - expect(response.statusCode).toBe(200); + const response = await testServer.authAgentFor(owner).get('/mfa/qr').expect(200); const { data } = response.body; @@ -77,93 +73,57 @@ describe('Enable MFA setup', () => { describe('Step two', () => { test('POST /verify should fail due to unauthenticated user', async () => { - const response = await testServer.authlessAgent.post('/mfa/verify'); - - expect(response.statusCode).toBe(401); + await testServer.authlessAgent.post('/mfa/verify').expect(401); }); test('POST /verify should fail due to invalid MFA token', async () => { - const response = await testServer - .authAgentFor(owner) - .post('/mfa/verify') - .send({ token: '123' }); - - expect(response.statusCode).toBe(400); + await testServer.authAgentFor(owner).post('/mfa/verify').send({ token: '123' }).expect(400); }); test('POST /verify should fail due to missing token parameter', async () => { - await testServer.authAgentFor(owner).get('/mfa/qr'); - - const response = await testServer.authAgentFor(owner).post('/mfa/verify').send({ token: '' }); - - expect(response.statusCode).toBe(400); + await testServer.authAgentFor(owner).get('/mfa/qr').expect(200); + await testServer.authAgentFor(owner).post('/mfa/verify').send({ token: '' }).expect(400); }); test('POST /verify should validate MFA token', async () => { - const response = await testServer.authAgentFor(owner).get('/mfa/qr'); + const response = await testServer.authAgentFor(owner).get('/mfa/qr').expect(200); const { secret } = response.body.data; - const token = new TOTPService().generateTOTP(secret); - const { statusCode } = await testServer - .authAgentFor(owner) - .post('/mfa/verify') - .send({ token }); - - expect(statusCode).toBe(200); + await testServer.authAgentFor(owner).post('/mfa/verify').send({ token }).expect(200); }); }); describe('Step three', () => { test('POST /enable should fail due to unauthenticated user', async () => { - const response = await testServer.authlessAgent.post('/mfa/enable'); - - expect(response.statusCode).toBe(401); + await testServer.authlessAgent.post('/mfa/enable').expect(401); }); test('POST /verify should fail due to missing token parameter', async () => { - const response = await testServer.authAgentFor(owner).post('/mfa/verify').send({ token: '' }); - - expect(response.statusCode).toBe(400); + await testServer.authAgentFor(owner).post('/mfa/verify').send({ token: '' }).expect(400); }); test('POST /enable should fail due to invalid MFA token', async () => { - await testServer.authAgentFor(owner).get('/mfa/qr'); - - const response = await testServer - .authAgentFor(owner) - .post('/mfa/enable') - .send({ token: '123' }); - - expect(response.statusCode).toBe(400); + await testServer.authAgentFor(owner).get('/mfa/qr').expect(200); + await testServer.authAgentFor(owner).post('/mfa/enable').send({ token: '123' }).expect(400); }); test('POST /enable should fail due to empty secret and recovery codes', async () => { - const response = await testServer.authAgentFor(owner).post('/mfa/enable'); - - expect(response.statusCode).toBe(400); + await testServer.authAgentFor(owner).post('/mfa/enable').expect(400); }); test('POST /enable should enable MFA in account', async () => { - const response = await testServer.authAgentFor(owner).get('/mfa/qr'); + const response = await testServer.authAgentFor(owner).get('/mfa/qr').expect(200); const { secret } = response.body.data; - const token = new TOTPService().generateTOTP(secret); - await testServer.authAgentFor(owner).post('/mfa/verify').send({ token }); + await testServer.authAgentFor(owner).post('/mfa/verify').send({ token }).expect(200); + await testServer.authAgentFor(owner).post('/mfa/enable').send({ token }).expect(200); - const { statusCode } = await testServer - .authAgentFor(owner) - .post('/mfa/enable') - .send({ token }); - - expect(statusCode).toBe(200); - - const user = await Container.get(UserRepository).findOneOrFail({ + const user = await Container.get(AuthUserRepository).findOneOrFail({ where: {}, - select: ['mfaEnabled', 'mfaRecoveryCodes', 'mfaSecret'], }); expect(user.mfaEnabled).toBe(true); @@ -177,13 +137,10 @@ describe('Disable MFA setup', () => { test('POST /disable should disable login with MFA', async () => { const { user } = await createUserWithMfaEnabled(); - const response = await testServer.authAgentFor(user).delete('/mfa/disable'); + await testServer.authAgentFor(user).delete('/mfa/disable').expect(200); - expect(response.statusCode).toBe(200); - - const dbUser = await Container.get(UserRepository).findOneOrFail({ + const dbUser = await Container.get(AuthUserRepository).findOneOrFail({ where: { id: user.id }, - select: ['mfaEnabled', 'mfaRecoveryCodes', 'mfaSecret'], }); expect(dbUser.mfaEnabled).toBe(false); @@ -198,42 +155,39 @@ describe('Change password with MFA enabled', () => { const newPassword = randomPassword(); - const response = await testServer + await testServer .authAgentFor(user) .patch('/me/password') - .send({ currentPassword: rawPassword, newPassword }); - - expect(response.statusCode).toBe(400); + .send({ currentPassword: rawPassword, newPassword }) + .expect(400); }); test('POST /change-password should fail due to missing MFA token', async () => { await createUserWithMfaEnabled(); const newPassword = randomValidPassword(); - const resetPasswordToken = uniqueId(); - const response = await testServer.authlessAgent + await testServer.authlessAgent .post('/change-password') - .send({ password: newPassword, token: resetPasswordToken }); - - expect(response.statusCode).toBe(404); + .send({ password: newPassword, token: resetPasswordToken }) + .expect(404); }); test('POST /change-password should fail due to invalid MFA token', async () => { await createUserWithMfaEnabled(); const newPassword = randomValidPassword(); - const resetPasswordToken = uniqueId(); - const response = await testServer.authlessAgent.post('/change-password').send({ - password: newPassword, - token: resetPasswordToken, - mfaToken: randomDigit(), - }); - - expect(response.statusCode).toBe(404); + await testServer.authlessAgent + .post('/change-password') + .send({ + password: newPassword, + token: resetPasswordToken, + mfaToken: randomDigit(), + }) + .expect(404); }); test('POST /change-password should update password', async () => { @@ -247,13 +201,14 @@ describe('Change password with MFA enabled', () => { const mfaToken = new TOTPService().generateTOTP(rawSecret); - const response = await testServer.authlessAgent.post('/change-password').send({ - password: newPassword, - token: resetPasswordToken, - mfaToken, - }); - - expect(response.statusCode).toBe(200); + await testServer.authlessAgent + .post('/change-password') + .send({ + password: newPassword, + token: resetPasswordToken, + mfaToken, + }) + .expect(200); const loginResponse = await testServer .authAgentFor(user) @@ -262,9 +217,9 @@ describe('Change password with MFA enabled', () => { email: user.email, password: newPassword, mfaToken: new TOTPService().generateTOTP(rawSecret), - }); + }) + .expect(200); - expect(loginResponse.statusCode).toBe(200); expect(loginResponse.body).toHaveProperty('data'); }); }); @@ -275,30 +230,14 @@ describe('Login', () => { const user = await createUser({ password }); - const response = await testServer.authlessAgent - .post('/login') - .send({ email: user.email, password }); - - expect(response.statusCode).toBe(200); - }); - - test('GET /login should include hasRecoveryCodesLeft property in response', async () => { - const response = await testServer.authAgentFor(owner).get('/login'); - - const { data } = response.body; - - expect(response.statusCode).toBe(200); - - expect(data.hasRecoveryCodesLeft).toBeDefined(); + await testServer.authlessAgent.post('/login').send({ email: user.email, password }).expect(200); }); test('GET /login should not include mfaSecret and mfaRecoveryCodes property in response', async () => { - const response = await testServer.authAgentFor(owner).get('/login'); + const response = await testServer.authAgentFor(owner).get('/login').expect(200); const { data } = response.body; - expect(response.statusCode).toBe(200); - expect(data.recoveryCodes).not.toBeDefined(); expect(data.mfaSecret).not.toBeDefined(); }); @@ -306,22 +245,20 @@ describe('Login', () => { test('POST /login with email/password should fail when mfa is enabled', async () => { const { user, rawPassword } = await createUserWithMfaEnabled(); - const response = await testServer.authlessAgent + await testServer.authlessAgent .post('/login') - .send({ email: user.email, password: rawPassword }); - - expect(response.statusCode).toBe(401); + .send({ email: user.email, password: rawPassword }) + .expect(401); }); describe('Login with MFA token', () => { test('POST /login should fail due to invalid MFA token', async () => { const { user, rawPassword } = await createUserWithMfaEnabled(); - const response = await testServer.authlessAgent + await testServer.authlessAgent .post('/login') - .send({ email: user.email, password: rawPassword, mfaToken: 'wrongvalue' }); - - expect(response.statusCode).toBe(401); + .send({ email: user.email, password: rawPassword, mfaToken: 'wrongvalue' }) + .expect(401); }); test('POST /login should fail due two MFA step needed', async () => { @@ -329,9 +266,9 @@ describe('Login', () => { const response = await testServer.authlessAgent .post('/login') - .send({ email: user.email, password: rawPassword }); + .send({ email: user.email, password: rawPassword }) + .expect(401); - expect(response.statusCode).toBe(401); expect(response.body.code).toBe(998); }); @@ -342,11 +279,11 @@ describe('Login', () => { const response = await testServer.authlessAgent .post('/login') - .send({ email: user.email, password: rawPassword, mfaToken: token }); + .send({ email: user.email, password: rawPassword, mfaToken: token }) + .expect(200); const data = response.body.data; - expect(response.statusCode).toBe(200); expect(data.mfaEnabled).toBe(true); }); }); @@ -355,11 +292,10 @@ describe('Login', () => { test('POST /login should fail due to invalid MFA recovery code', async () => { const { user, rawPassword } = await createUserWithMfaEnabled(); - const response = await testServer.authlessAgent + await testServer.authlessAgent .post('/login') - .send({ email: user.email, password: rawPassword, mfaRecoveryCode: 'wrongvalue' }); - - expect(response.statusCode).toBe(401); + .send({ email: user.email, password: rawPassword, mfaRecoveryCode: 'wrongvalue' }) + .expect(401); }); test('POST /login should succeed with MFA recovery code', async () => { @@ -367,38 +303,19 @@ describe('Login', () => { const response = await testServer.authlessAgent .post('/login') - .send({ email: user.email, password: rawPassword, mfaRecoveryCode: rawRecoveryCodes[0] }); + .send({ email: user.email, password: rawPassword, mfaRecoveryCode: rawRecoveryCodes[0] }) + .expect(200); const data = response.body.data; - - expect(response.statusCode).toBe(200); expect(data.mfaEnabled).toBe(true); - expect(data.hasRecoveryCodesLeft).toBe(true); - const dbUser = await Container.get(UserRepository).findOneOrFail({ + const dbUser = await Container.get(AuthUserRepository).findOneOrFail({ where: { id: user.id }, - select: ['mfaEnabled', 'mfaRecoveryCodes', 'mfaSecret'], }); // Make sure the recovery code used was removed expect(dbUser.mfaRecoveryCodes.length).toBe(rawRecoveryCodes.length - 1); expect(dbUser.mfaRecoveryCodes.includes(rawRecoveryCodes[0])).toBe(false); }); - - test('POST /login with MFA recovery code should update hasRecoveryCodesLeft property', async () => { - const { user, rawPassword, rawRecoveryCodes } = await createUserWithMfaEnabled({ - numberOfRecoveryCodes: 1, - }); - - const response = await testServer.authlessAgent - .post('/login') - .send({ email: user.email, password: rawPassword, mfaRecoveryCode: rawRecoveryCodes[0] }); - - const data = response.body.data; - - expect(response.statusCode).toBe(200); - expect(data.mfaEnabled).toBe(true); - expect(data.hasRecoveryCodesLeft).toBe(false); - }); }); }); diff --git a/packages/cli/test/integration/shared/db/users.ts b/packages/cli/test/integration/shared/db/users.ts index 81ca3b199e..aa7a3baaa7 100644 --- a/packages/cli/test/integration/shared/db/users.ts +++ b/packages/cli/test/integration/shared/db/users.ts @@ -8,6 +8,7 @@ import { TOTPService } from '@/Mfa/totp.service'; import { MfaService } from '@/Mfa/mfa.service'; import { randomApiKey, randomEmail, randomName, randomValidPassword } from '../random'; +import { AuthUserRepository } from '@/databases/repositories/authUser.repository'; // pre-computed bcrypt hash for the string 'password', using `await hash('password', 10)` const passwordHash = '$2a$10$njedH7S6V5898mj6p0Jr..IGY9Ms.qNwR7RbSzzX9yubJocKfvGGK'; @@ -58,14 +59,19 @@ export async function createUserWithMfaEnabled( recoveryCodes, ); + const user = await createUser({ + mfaEnabled: true, + password, + email, + }); + + await Container.get(AuthUserRepository).update(user.id, { + mfaSecret: encryptedSecret, + mfaRecoveryCodes: encryptedRecoveryCodes, + }); + return { - user: await createUser({ - mfaEnabled: true, - password, - email, - mfaSecret: encryptedSecret, - mfaRecoveryCodes: encryptedRecoveryCodes, - }), + user, rawPassword: password, rawSecret: secret, rawRecoveryCodes: recoveryCodes, diff --git a/packages/cli/test/unit/databases/entities/user.entity.test.ts b/packages/cli/test/unit/databases/entities/user.entity.test.ts index 7fac71c5fa..aaa798b184 100644 --- a/packages/cli/test/unit/databases/entities/user.entity.test.ts +++ b/packages/cli/test/unit/databases/entities/user.entity.test.ts @@ -9,8 +9,6 @@ describe('User Entity', () => { lastName: 'Joe', password: '123456789', apiKey: '123', - mfaSecret: '123', - mfaRecoveryCodes: ['123'], }); expect(JSON.stringify(user)).toEqual( '{"email":"test@example.com","firstName":"Don","lastName":"Joe"}', diff --git a/packages/cli/test/unit/services/user.service.test.ts b/packages/cli/test/unit/services/user.service.test.ts index 5dabdf6646..9f84b1d202 100644 --- a/packages/cli/test/unit/services/user.service.test.ts +++ b/packages/cli/test/unit/services/user.service.test.ts @@ -30,15 +30,13 @@ describe('UserService', () => { }); type MaybeSensitiveProperties = Partial< - Pick + Pick >; // to prevent typechecking from blocking assertions const publicUser: MaybeSensitiveProperties = await userService.toPublic(mockUser); expect(publicUser.password).toBeUndefined(); - expect(publicUser.mfaSecret).toBeUndefined(); - expect(publicUser.mfaRecoveryCodes).toBeUndefined(); expect(publicUser.updatedAt).toBeUndefined(); expect(publicUser.authIdentities).toBeUndefined(); }); diff --git a/packages/editor-ui/src/Interface.ts b/packages/editor-ui/src/Interface.ts index c4b441ee86..cb80d57bbf 100644 --- a/packages/editor-ui/src/Interface.ts +++ b/packages/editor-ui/src/Interface.ts @@ -742,7 +742,6 @@ export interface CurrentUserResponse extends IUserResponse { export interface IUser extends IUserResponse { isDefaultUser: boolean; isPendingUser: boolean; - hasRecoveryCodesLeft: boolean; inviteAcceptUrl?: string; fullName?: string; createdAt?: string; diff --git a/packages/editor-ui/src/__tests__/data/users.ts b/packages/editor-ui/src/__tests__/data/users.ts index 6406b08255..f51921f48a 100644 --- a/packages/editor-ui/src/__tests__/data/users.ts +++ b/packages/editor-ui/src/__tests__/data/users.ts @@ -10,7 +10,6 @@ export const createUser = (overrides?: Partial): IUser => ({ isDefaultUser: false, isPending: false, isPendingUser: false, - hasRecoveryCodesLeft: false, mfaEnabled: false, signInType: SignInType.EMAIL, ...overrides, diff --git a/packages/editor-ui/src/__tests__/server/factories/user.ts b/packages/editor-ui/src/__tests__/server/factories/user.ts index e8fc06cec9..f7d045592d 100644 --- a/packages/editor-ui/src/__tests__/server/factories/user.ts +++ b/packages/editor-ui/src/__tests__/server/factories/user.ts @@ -28,7 +28,4 @@ export const userFactory = Factory.extend({ mfaEnabled() { return false; }, - hasRecoveryCodesLeft() { - return false; - }, }); diff --git a/packages/editor-ui/src/components/__tests__/PersonalizationModal.spec.ts b/packages/editor-ui/src/components/__tests__/PersonalizationModal.spec.ts index 4af40df601..3ab834b20e 100644 --- a/packages/editor-ui/src/components/__tests__/PersonalizationModal.spec.ts +++ b/packages/editor-ui/src/components/__tests__/PersonalizationModal.spec.ts @@ -30,7 +30,6 @@ const pinia = createTestingPinia({ lastName: 'Doe', isDefaultUser: false, isPendingUser: false, - hasRecoveryCodesLeft: true, role: ROLE.Owner, mfaEnabled: false, }, diff --git a/packages/editor-ui/src/views/MfaView.vue b/packages/editor-ui/src/views/MfaView.vue index ccd2e23bd7..53f0819420 100644 --- a/packages/editor-ui/src/views/MfaView.vue +++ b/packages/editor-ui/src/views/MfaView.vue @@ -148,7 +148,11 @@ export default defineComponent({ this.verifyingMfaToken = true; this.hasAnyChanges = true; - this.onSubmit({ token: value, recoveryCode: value }) + const dataToSubmit = isSubmittingMfaToken + ? { token: value, recoveryCode: '' } + : { token: '', recoveryCode: value }; + + this.onSubmit(dataToSubmit) .catch(() => {}) .finally(() => (this.verifyingMfaToken = false)); }, diff --git a/packages/editor-ui/src/views/SigninView.vue b/packages/editor-ui/src/views/SigninView.vue index c7b39e820b..985c46ab64 100644 --- a/packages/editor-ui/src/views/SigninView.vue +++ b/packages/editor-ui/src/views/SigninView.vue @@ -144,7 +144,6 @@ export default defineComponent({ } await this.settingsStore.getSettings(); this.clearAllStickyNotifications(); - this.checkRecoveryCodesLeft(); this.$telemetry.track('User attempted to login', { result: this.showMfaView ? 'mfa_success' : 'success', @@ -198,21 +197,6 @@ export default defineComponent({ this.email = form.email; this.password = form.password; }, - checkRecoveryCodesLeft() { - if (this.usersStore.currentUser) { - const { hasRecoveryCodesLeft, mfaEnabled } = this.usersStore.currentUser; - - if (mfaEnabled && !hasRecoveryCodesLeft) { - this.showToast({ - title: this.$locale.baseText('settings.mfa.toast.noRecoveryCodeLeft.title'), - message: this.$locale.baseText('settings.mfa.toast.noRecoveryCodeLeft.message'), - type: 'info', - duration: 0, - dangerouslyUseHTMLString: true, - }); - } - } - }, }, }); diff --git a/packages/editor-ui/src/views/__tests__/SettingsPersonalView.test.ts b/packages/editor-ui/src/views/__tests__/SettingsPersonalView.test.ts index 7236800010..b839228065 100644 --- a/packages/editor-ui/src/views/__tests__/SettingsPersonalView.test.ts +++ b/packages/editor-ui/src/views/__tests__/SettingsPersonalView.test.ts @@ -24,7 +24,6 @@ const currentUser = { isDefaultUser: false, isPendingUser: false, isPending: false, - hasRecoveryCodesLeft: false, mfaEnabled: false, };