diff --git a/packages/@n8n/db/src/entities/auth-user.ts b/packages/@n8n/db/src/entities/auth-user.ts deleted file mode 100644 index 55cd7a9d34..0000000000 --- a/packages/@n8n/db/src/entities/auth-user.ts +++ /dev/null @@ -1,12 +0,0 @@ -import { Column, Entity } from '@n8n/typeorm'; - -import { User } from './user'; - -@Entity({ name: 'user' }) -export class AuthUser extends User { - @Column({ type: String, nullable: true }) - mfaSecret?: string | null; - - @Column({ type: 'simple-array', default: '' }) - mfaRecoveryCodes: string[]; -} diff --git a/packages/@n8n/db/src/entities/index.ts b/packages/@n8n/db/src/entities/index.ts index b5d75bc355..cb2b0f5342 100644 --- a/packages/@n8n/db/src/entities/index.ts +++ b/packages/@n8n/db/src/entities/index.ts @@ -3,7 +3,6 @@ import { AnnotationTagMapping } from './annotation-tag-mapping.ee'; import { ApiKey } from './api-key'; import { AuthIdentity } from './auth-identity'; import { AuthProviderSyncHistory } from './auth-provider-sync-history'; -import { AuthUser } from './auth-user'; import { CredentialsEntity } from './credentials-entity'; import { EventDestinations } from './event-destinations'; import { ExecutionAnnotation } from './execution-annotation.ee'; @@ -58,7 +57,6 @@ export { WorkflowTagMapping, FolderTagMapping, AuthProviderSyncHistory, - AuthUser, WorkflowHistory, ExecutionData, ExecutionMetadata, @@ -96,7 +94,6 @@ export const entities = { WorkflowTagMapping, FolderTagMapping, AuthProviderSyncHistory, - AuthUser, WorkflowHistory, ExecutionData, ExecutionMetadata, diff --git a/packages/@n8n/db/src/entities/user.ts b/packages/@n8n/db/src/entities/user.ts index 8d026a8fff..d6736bd45f 100644 --- a/packages/@n8n/db/src/entities/user.ts +++ b/packages/@n8n/db/src/entities/user.ts @@ -96,6 +96,12 @@ export class User extends WithTimestamps implements IUser, AuthPrincipal { @Column({ type: Boolean, default: false }) mfaEnabled: boolean; + @Column({ type: String, nullable: true }) + mfaSecret?: string | null; + + @Column({ type: 'simple-array', default: '' }) + mfaRecoveryCodes: string[]; + /** * Whether the user is pending setup completion. */ @@ -108,7 +114,7 @@ export class User extends WithTimestamps implements IUser, AuthPrincipal { } toJSON() { - const { password, ...rest } = this; + const { password, mfaSecret, mfaRecoveryCodes, ...rest } = this; return rest; } diff --git a/packages/@n8n/db/src/repositories/auth-user.repository.ts b/packages/@n8n/db/src/repositories/auth-user.repository.ts deleted file mode 100644 index 99cbe2d783..0000000000 --- a/packages/@n8n/db/src/repositories/auth-user.repository.ts +++ /dev/null @@ -1,11 +0,0 @@ -import { Service } from '@n8n/di'; -import { DataSource, Repository } from '@n8n/typeorm'; - -import { AuthUser } from '../entities'; - -@Service() -export class AuthUserRepository extends Repository { - constructor(dataSource: DataSource) { - super(AuthUser, dataSource.manager); - } -} diff --git a/packages/@n8n/db/src/repositories/index.ts b/packages/@n8n/db/src/repositories/index.ts index acec89570f..775df2e103 100644 --- a/packages/@n8n/db/src/repositories/index.ts +++ b/packages/@n8n/db/src/repositories/index.ts @@ -3,7 +3,6 @@ export { AnnotationTagRepository } from './annotation-tag.repository.ee'; export { ApiKeyRepository } from './api-key.repository'; export { AuthIdentityRepository } from './auth-identity.repository'; export { AuthProviderSyncHistoryRepository } from './auth-provider-sync-history.repository'; -export { AuthUserRepository } from './auth-user.repository'; export { CredentialsRepository } from './credentials.repository'; export { ExecutionAnnotationRepository } from './execution-annotation.repository'; export { ExecutionDataRepository } from './execution-data.repository'; diff --git a/packages/cli/src/auth/__tests__/auth.service.test.ts b/packages/cli/src/auth/__tests__/auth.service.test.ts index e6903a344b..3952b76883 100644 --- a/packages/cli/src/auth/__tests__/auth.service.test.ts +++ b/packages/cli/src/auth/__tests__/auth.service.test.ts @@ -58,13 +58,35 @@ describe('AuthService', () => { it('should generate unique hashes', () => { expect(authService.createJWTHash(user)).toEqual('mJAYx4Wb7k'); expect( - authService.createJWTHash(mock({ email: user.email, password: 'newPasswordHash' })), + authService.createJWTHash( + mock({ email: user.email, password: 'newPasswordHash', mfaEnabled: false }), + ), ).toEqual('FVALtU7AE0'); expect( authService.createJWTHash( - mock({ email: 'test1@example.com', password: user.password }), + mock({ email: 'test1@example.com', password: user.password, mfaEnabled: false }), ), ).toEqual('y8ha6X01jd'); + expect( + authService.createJWTHash( + mock({ + email: user.email, + password: user.password, + mfaEnabled: true, + mfaSecret: 'secret', + }), + ), + ).toEqual('WUXEVFet9W'); + expect( + authService.createJWTHash( + mock({ + email: user.email, + password: 'newPasswordHash', + mfaEnabled: true, + mfaSecret: 'secret', + }), + ), + ).toEqual('toYQYKufH6'); }); }); @@ -245,6 +267,10 @@ describe('AuthService', () => { 'user email does not match the one on the token', { ...userData, email: 'someone@example.com' }, ], + [ + 'user mfa secret does not match the one on the token', + { ...userData, mfaEnabled: true, mfaSecret: '123' }, + ], ])('should throw if %s', async (_, data) => { userRepository.findOne.mockResolvedValueOnce(data && mock(data)); await expect(authService.resolveJwt(validToken, req, res)).rejects.toThrow('Unauthorized'); diff --git a/packages/cli/src/auth/auth.service.ts b/packages/cli/src/auth/auth.service.ts index 1860ac6d6b..0181b62d1e 100644 --- a/packages/cli/src/auth/auth.service.ts +++ b/packages/cli/src/auth/auth.service.ts @@ -229,8 +229,12 @@ export class AuthService { return user; } - createJWTHash({ email, password }: User) { - return this.hash(email + ':' + password).substring(0, 10); + createJWTHash({ email, password, mfaEnabled, mfaSecret }: User) { + const payload = [email, password]; + if (mfaEnabled && mfaSecret) { + payload.push(mfaSecret.substring(0, 3)); + } + return this.hash(payload.join(':')).substring(0, 10); } private hash(input: string) { diff --git a/packages/cli/src/commands/mfa/disable.ts b/packages/cli/src/commands/mfa/disable.ts index 89dc929bd0..1a95ec3553 100644 --- a/packages/cli/src/commands/mfa/disable.ts +++ b/packages/cli/src/commands/mfa/disable.ts @@ -1,4 +1,4 @@ -import { AuthUserRepository } from '@n8n/db'; +import { UserRepository } from '@n8n/db'; import { Container } from '@n8n/di'; import { Flags } from '@oclif/core'; @@ -28,7 +28,7 @@ export class DisableMFACommand extends BaseCommand { return; } - const repository = Container.get(AuthUserRepository); + const repository = Container.get(UserRepository); const user = await repository.findOneBy({ email: flags.email }); if (!user) { diff --git a/packages/cli/src/controllers/__tests__/me.controller.test.ts b/packages/cli/src/controllers/__tests__/me.controller.test.ts index 15e9fddcd8..8c4c4e96f9 100644 --- a/packages/cli/src/controllers/__tests__/me.controller.test.ts +++ b/packages/cli/src/controllers/__tests__/me.controller.test.ts @@ -1,7 +1,6 @@ import { UserUpdateRequestDto } from '@n8n/api-types'; import type { User } from '@n8n/db'; import type { PublicUser } from '@n8n/db'; -import { AuthUserRepository } from '@n8n/db'; import { InvalidAuthTokenRepository } from '@n8n/db'; import { UserRepository } from '@n8n/db'; import { Container } from '@n8n/di'; @@ -30,7 +29,6 @@ describe('MeController', () => { const userService = mockInstance(UserService); const userRepository = mockInstance(UserRepository); const mockMfaService = mockInstance(MfaService); - mockInstance(AuthUserRepository); mockInstance(InvalidAuthTokenRepository); mockInstance(License).isWithinUsersLimit.mockReturnValue(true); const controller = Container.get(MeController); @@ -171,6 +169,7 @@ describe('MeController', () => { authIdentities: [], role: 'global:owner', mfaEnabled: true, + mfaSecret: 'secret', }); const req = mock({ user, browserId }); const res = mock(); @@ -316,7 +315,7 @@ describe('MeController', () => { it('should succeed when mfa code is correct', async () => { const req = mock({ - user: mock({ password: passwordHash, mfaEnabled: true }), + user: mock({ password: passwordHash, mfaEnabled: true, mfaSecret: 'secret' }), browserId, }); const res = mock(); diff --git a/packages/cli/src/controllers/e2e.controller.ts b/packages/cli/src/controllers/e2e.controller.ts index 82f009d7f3..72d0e3098c 100644 --- a/packages/cli/src/controllers/e2e.controller.ts +++ b/packages/cli/src/controllers/e2e.controller.ts @@ -1,7 +1,7 @@ import type { PushMessage } from '@n8n/api-types'; import type { BooleanLicenseFeature, NumericLicenseFeature } from '@n8n/constants'; import { LICENSE_FEATURES, LICENSE_QUOTAS, UNLIMITED_LICENSE_QUOTA } from '@n8n/constants'; -import { AuthUserRepository, SettingsRepository, UserRepository } from '@n8n/db'; +import { SettingsRepository, UserRepository } from '@n8n/db'; import { Patch, Post, RestController } from '@n8n/decorators'; import { Container } from '@n8n/di'; import { Request } from 'express'; @@ -149,7 +149,6 @@ export class E2EController { private readonly passwordUtility: PasswordUtility, private readonly eventBus: MessageEventBus, private readonly userRepository: UserRepository, - private readonly authUserRepository: AuthUserRepository, ) { license.isLicensed = (feature: BooleanLicenseFeature) => this.enabledFeatures[feature] ?? false; @@ -280,7 +279,7 @@ export class E2EController { const { encryptedRecoveryCodes, encryptedSecret } = this.mfaService.encryptSecretAndRecoveryCodes(owner.mfaSecret, owner.mfaRecoveryCodes); - await this.authUserRepository.update(newOwner.user.id, { + await this.userRepository.update(newOwner.user.id, { mfaSecret: encryptedSecret, mfaRecoveryCodes: encryptedRecoveryCodes, }); diff --git a/packages/cli/src/controllers/mfa.controller.ts b/packages/cli/src/controllers/mfa.controller.ts index 39e2ce50e2..b143655b43 100644 --- a/packages/cli/src/controllers/mfa.controller.ts +++ b/packages/cli/src/controllers/mfa.controller.ts @@ -1,5 +1,8 @@ +import { UserRepository } from '@n8n/db'; import { Get, Post, RestController } from '@n8n/decorators'; +import { Response } from 'express'; +import { AuthService } from '@/auth/auth.service'; import { BadRequestError } from '@/errors/response-errors/bad-request.error'; import { ExternalHooks } from '@/external-hooks'; import { MfaService } from '@/mfa/mfa.service'; @@ -10,6 +13,8 @@ export class MFAController { constructor( private mfaService: MfaService, private externalHooks: ExternalHooks, + private authService: AuthService, + private userRepository: UserRepository, ) {} @Post('/can-enable') @@ -59,7 +64,7 @@ export class MFAController { } @Post('/enable', { rateLimit: true }) - async activateMFA(req: MFA.Activate) { + async activateMFA(req: MFA.Activate, res: Response) { const { mfaCode = null } = req.body; const { id, mfaEnabled } = req.user; @@ -81,11 +86,13 @@ export class MFAController { if (!verified) throw new BadRequestError('MFA code expired. Close the modal and enable MFA again', 997); - await this.mfaService.enableMfa(id); + const updatedUser = await this.mfaService.enableMfa(id); + + this.authService.issueCookie(res, updatedUser, req.browserId); } @Post('/disable', { rateLimit: true }) - async disableMFA(req: MFA.Disable) { + async disableMFA(req: MFA.Disable, res: Response) { const { id: userId } = req.user; const { mfaCode, mfaRecoveryCode } = req.body; @@ -105,6 +112,10 @@ export class MFAController { } else if (mfaRecoveryCodeDefined) { await this.mfaService.disableMfaWithRecoveryCode(userId, mfaRecoveryCode); } + + const updatedUser = await this.userRepository.findOneByOrFail({ id: userId }); + + this.authService.issueCookie(res, updatedUser, req.browserId); } @Post('/verify', { rateLimit: true }) diff --git a/packages/cli/src/mfa/mfa.service.ts b/packages/cli/src/mfa/mfa.service.ts index 82d02a3055..9b77ab01af 100644 --- a/packages/cli/src/mfa/mfa.service.ts +++ b/packages/cli/src/mfa/mfa.service.ts @@ -1,4 +1,4 @@ -import { AuthUserRepository } from '@n8n/db'; +import { UserRepository } from '@n8n/db'; import { Service } from '@n8n/di'; import { Cipher } from 'n8n-core'; import { v4 as uuid } from 'uuid'; @@ -11,7 +11,7 @@ import { TOTPService } from './totp.service'; @Service() export class MfaService { constructor( - private authUserRepository: AuthUserRepository, + private userRepository: UserRepository, public totp: TOTPService, private cipher: Cipher, ) {} @@ -26,10 +26,10 @@ export class MfaService { recoveryCodes, ); - const user = await this.authUserRepository.findOneByOrFail({ id: userId }); + const user = await this.userRepository.findOneByOrFail({ id: userId }); user.mfaSecret = encryptedSecret; user.mfaRecoveryCodes = encryptedRecoveryCodes; - await this.authUserRepository.save(user); + await this.userRepository.save(user); } encryptSecretAndRecoveryCodes(rawSecret: string, rawRecoveryCodes: string[]) { @@ -49,7 +49,7 @@ export class MfaService { } async getSecretAndRecoveryCodes(userId: string) { - const { mfaSecret, mfaRecoveryCodes } = await this.authUserRepository.findOneByOrFail({ + const { mfaSecret, mfaRecoveryCodes } = await this.userRepository.findOneByOrFail({ id: userId, }); return this.decryptSecretAndRecoveryCodes(mfaSecret ?? '', mfaRecoveryCodes ?? []); @@ -60,7 +60,7 @@ export class MfaService { mfaCode: string | undefined, mfaRecoveryCode: string | undefined, ) { - const user = await this.authUserRepository.findOneByOrFail({ id: userId }); + const user = await this.userRepository.findOneByOrFail({ id: userId }); if (mfaCode) { const decryptedSecret = this.cipher.decrypt(user.mfaSecret!); return this.totp.verifySecret({ secret: decryptedSecret, mfaCode }); @@ -73,7 +73,7 @@ export class MfaService { // remove used recovery code validCodes.splice(index, 1); user.mfaRecoveryCodes = validCodes.map((code) => this.cipher.encrypt(code)); - await this.authUserRepository.save(user); + await this.userRepository.save(user); return true; } @@ -81,9 +81,9 @@ export class MfaService { } async enableMfa(userId: string) { - const user = await this.authUserRepository.findOneByOrFail({ id: userId }); + const user = await this.userRepository.findOneByOrFail({ id: userId }); user.mfaEnabled = true; - return await this.authUserRepository.save(user); + return await this.userRepository.save(user); } async disableMfaWithMfaCode(userId: string, mfaCode: string) { @@ -107,7 +107,7 @@ export class MfaService { } private async disableMfaForUser(userId: string) { - await this.authUserRepository.update(userId, { + await this.userRepository.update(userId, { mfaEnabled: false, mfaSecret: null, mfaRecoveryCodes: [], diff --git a/packages/cli/src/services/__tests__/hooks.service.test.ts b/packages/cli/src/services/__tests__/hooks.service.test.ts index 461fbadd99..454d091841 100644 --- a/packages/cli/src/services/__tests__/hooks.service.test.ts +++ b/packages/cli/src/services/__tests__/hooks.service.test.ts @@ -1,5 +1,4 @@ -import type { AuthUser, SettingsRepository } from '@n8n/db'; -import type { AuthUserRepository } from '@n8n/db'; +import type { SettingsRepository, User } from '@n8n/db'; import type { CredentialsRepository } from '@n8n/db'; import type { WorkflowRepository } from '@n8n/db'; import type { UserRepository } from '@n8n/db'; @@ -16,14 +15,13 @@ import type { UserService } from '@/services/user.service'; jest.mock('@rudderstack/rudder-sdk-node'); describe('HooksService', () => { - const mockedUser = mock(); + const mockedUser = mock(); const userService = mock(); const authService = mock(); const userRepository = mock(); const settingsRepository = mock(); const workflowRepository = mock(); const credentialsRepository = mock(); - const authUserRepository = mock(); const hooksService = new HooksService( userService, authService, @@ -31,7 +29,6 @@ describe('HooksService', () => { settingsRepository, workflowRepository, credentialsRepository, - authUserRepository, ); beforeEach(() => { @@ -60,7 +57,7 @@ describe('HooksService', () => { expect(authService.issueCookie).toHaveBeenCalledWith(res, mockedUser); }); - it('hooksService.findOneUser should call authUserRepository.findOne', async () => { + it('hooksService.findOneUser should call userRepository.findOne', async () => { // ARRANGE const filter = { where: { id: '1' } }; @@ -68,7 +65,7 @@ describe('HooksService', () => { await hooksService.findOneUser(filter); // ASSERT - expect(authUserRepository.findOne).toHaveBeenCalledWith(filter); + expect(userRepository.findOne).toHaveBeenCalledWith(filter); }); it('hooksService.saveUser should call userRepository.save', async () => { diff --git a/packages/cli/src/services/__tests__/user.service.test.ts b/packages/cli/src/services/__tests__/user.service.test.ts index b439762a44..4858f028cf 100644 --- a/packages/cli/src/services/__tests__/user.service.test.ts +++ b/packages/cli/src/services/__tests__/user.service.test.ts @@ -38,7 +38,7 @@ describe('UserService', () => { }); type MaybeSensitiveProperties = Partial< - Pick + Pick >; // to prevent typechecking from blocking assertions @@ -47,6 +47,8 @@ describe('UserService', () => { expect(publicUser.password).toBeUndefined(); expect(publicUser.updatedAt).toBeUndefined(); expect(publicUser.authIdentities).toBeUndefined(); + expect(publicUser.mfaSecret).toBeUndefined(); + expect(publicUser.mfaRecoveryCodes).toBeUndefined(); }); it('should add scopes if requested', async () => { diff --git a/packages/cli/src/services/hooks.service.ts b/packages/cli/src/services/hooks.service.ts index 301099b59b..fee945f3c1 100644 --- a/packages/cli/src/services/hooks.service.ts +++ b/packages/cli/src/services/hooks.service.ts @@ -1,6 +1,5 @@ -import type { Settings, CredentialsEntity, User, WorkflowEntity, AuthUser } from '@n8n/db'; +import type { Settings, CredentialsEntity, User, WorkflowEntity } from '@n8n/db'; import { - AuthUserRepository, CredentialsRepository, WorkflowRepository, SettingsRepository, @@ -31,7 +30,6 @@ export class HooksService { private readonly settingsRepository: SettingsRepository, private readonly workflowRepository: WorkflowRepository, private readonly credentialsRepository: CredentialsRepository, - private readonly authUserRepository: AuthUserRepository, ) {} /** @@ -45,7 +43,7 @@ export class HooksService { * Set the n8n-auth cookie in the response to auto-login * the user after instance is provisioned */ - issueCookie(res: Response, user: AuthUser) { + issueCookie(res: Response, user: User) { return this.authService.issueCookie(res, user); } @@ -54,8 +52,8 @@ export class HooksService { * 1. To know whether the instance owner is already setup * 2. To know when to update the user's profile also in cloud */ - async findOneUser(filter: FindOneOptions) { - return await this.authUserRepository.findOne(filter); + async findOneUser(filter: FindOneOptions) { + return await this.userRepository.findOne(filter); } /** diff --git a/packages/cli/src/services/user.service.ts b/packages/cli/src/services/user.service.ts index c4d9abda76..3a5cec5ddf 100644 --- a/packages/cli/src/services/user.service.ts +++ b/packages/cli/src/services/user.service.ts @@ -63,7 +63,7 @@ export class UserService { withScopes?: boolean; }, ) { - const { password, updatedAt, authIdentities, ...rest } = user; + const { password, updatedAt, authIdentities, mfaRecoveryCodes, mfaSecret, ...rest } = user; const ldapIdentity = authIdentities?.find((i) => i.providerType === 'ldap'); diff --git a/packages/cli/test/integration/mfa/mfa.api.test.ts b/packages/cli/test/integration/mfa/mfa.api.test.ts index 5ad46ddc1b..812549623c 100644 --- a/packages/cli/test/integration/mfa/mfa.api.test.ts +++ b/packages/cli/test/integration/mfa/mfa.api.test.ts @@ -1,5 +1,4 @@ -import type { User } from '@n8n/db'; -import { AuthUserRepository } from '@n8n/db'; +import { UserRepository, type User } from '@n8n/db'; import { Container } from '@n8n/di'; import { randomString } from 'n8n-workflow'; @@ -130,7 +129,7 @@ describe('Enable MFA setup', () => { await testServer.authAgentFor(owner).post('/mfa/verify').send({ mfaCode }).expect(200); await testServer.authAgentFor(owner).post('/mfa/enable').send({ mfaCode }).expect(200); - const user = await Container.get(AuthUserRepository).findOneOrFail({ + const user = await Container.get(UserRepository).findOneOrFail({ where: {}, }); @@ -153,7 +152,7 @@ describe('Enable MFA setup', () => { await testServer.authAgentFor(owner).post('/mfa/enable').send({ mfaCode }).expect(400); - const user = await Container.get(AuthUserRepository).findOneOrFail({ + const user = await Container.get(UserRepository).findOneOrFail({ where: {}, }); @@ -175,7 +174,7 @@ describe('Disable MFA setup', () => { }) .expect(200); - const dbUser = await Container.get(AuthUserRepository).findOneOrFail({ + const dbUser = await Container.get(UserRepository).findOneOrFail({ where: { id: user.id }, }); @@ -396,7 +395,7 @@ describe('Login', () => { const data = response.body.data; expect(data.mfaEnabled).toBe(true); - const dbUser = await Container.get(AuthUserRepository).findOneOrFail({ + const dbUser = await Container.get(UserRepository).findOneOrFail({ where: { id: user.id }, }); diff --git a/packages/cli/test/integration/shared/db/users.ts b/packages/cli/test/integration/shared/db/users.ts index c77ce2e7fb..a0514047d2 100644 --- a/packages/cli/test/integration/shared/db/users.ts +++ b/packages/cli/test/integration/shared/db/users.ts @@ -1,7 +1,6 @@ import { AuthIdentity } from '@n8n/db'; import { type User } from '@n8n/db'; import { AuthIdentityRepository } from '@n8n/db'; -import { AuthUserRepository } from '@n8n/db'; import { UserRepository } from '@n8n/db'; import { Container } from '@n8n/di'; import type { ApiKeyScope, GlobalRole } from '@n8n/permissions'; @@ -73,11 +72,14 @@ export async function createUserWithMfaEnabled( email, }); - await Container.get(AuthUserRepository).update(user.id, { + await Container.get(UserRepository).update(user.id, { mfaSecret: encryptedSecret, mfaRecoveryCodes: encryptedRecoveryCodes, }); + user.mfaSecret = encryptedSecret; + user.mfaRecoveryCodes = encryptedRecoveryCodes; + return { user, rawPassword: password,