feat(core): Invalidate all sessions when MFA is enabled/disabled (#15524)

This commit is contained in:
Ricardo Espinoza
2025-05-21 02:59:22 -04:00
committed by GitHub
parent a1a33deee5
commit 2a35c19ef9
18 changed files with 92 additions and 76 deletions

View File

@@ -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[];
}

View File

@@ -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,

View File

@@ -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;
}

View File

@@ -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<AuthUser> {
constructor(dataSource: DataSource) {
super(AuthUser, dataSource.manager);
}
}

View File

@@ -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';

View File

@@ -58,13 +58,35 @@ describe('AuthService', () => {
it('should generate unique hashes', () => {
expect(authService.createJWTHash(user)).toEqual('mJAYx4Wb7k');
expect(
authService.createJWTHash(mock<User>({ email: user.email, password: 'newPasswordHash' })),
authService.createJWTHash(
mock<User>({ email: user.email, password: 'newPasswordHash', mfaEnabled: false }),
),
).toEqual('FVALtU7AE0');
expect(
authService.createJWTHash(
mock<User>({ email: 'test1@example.com', password: user.password }),
mock<User>({ email: 'test1@example.com', password: user.password, mfaEnabled: false }),
),
).toEqual('y8ha6X01jd');
expect(
authService.createJWTHash(
mock<User>({
email: user.email,
password: user.password,
mfaEnabled: true,
mfaSecret: 'secret',
}),
),
).toEqual('WUXEVFet9W');
expect(
authService.createJWTHash(
mock<User>({
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<User>(data));
await expect(authService.resolveJwt(validToken, req, res)).rejects.toThrow('Unauthorized');

View File

@@ -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) {

View File

@@ -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) {

View File

@@ -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<AuthenticatedRequest>({ user, browserId });
const res = mock<Response>();
@@ -316,7 +315,7 @@ describe('MeController', () => {
it('should succeed when mfa code is correct', async () => {
const req = mock<AuthenticatedRequest>({
user: mock({ password: passwordHash, mfaEnabled: true }),
user: mock({ password: passwordHash, mfaEnabled: true, mfaSecret: 'secret' }),
browserId,
});
const res = mock<Response>();

View File

@@ -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,
});

View File

@@ -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 })

View File

@@ -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: [],

View File

@@ -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<AuthUser>();
const mockedUser = mock<User>();
const userService = mock<UserService>();
const authService = mock<AuthService>();
const userRepository = mock<UserRepository>();
const settingsRepository = mock<SettingsRepository>();
const workflowRepository = mock<WorkflowRepository>();
const credentialsRepository = mock<CredentialsRepository>();
const authUserRepository = mock<AuthUserRepository>();
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 () => {

View File

@@ -38,7 +38,7 @@ describe('UserService', () => {
});
type MaybeSensitiveProperties = Partial<
Pick<User, 'password' | 'updatedAt' | 'authIdentities'>
Pick<User, 'password' | 'updatedAt' | 'authIdentities' | 'mfaSecret' | 'mfaRecoveryCodes'>
>;
// 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 () => {

View File

@@ -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<AuthUser>) {
return await this.authUserRepository.findOne(filter);
async findOneUser(filter: FindOneOptions<User>) {
return await this.userRepository.findOne(filter);
}
/**

View File

@@ -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');

View File

@@ -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 },
});

View File

@@ -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,