houseassassin/twenty
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Fix 2fa auth and token format migration (#13523)

Browse Source 
Fix the 2FA setup and also make some changes so that the transition
towards a new token format introduced in a previous PR happens more
smoothly
This commit is contained in:
Félix Malfait
2025-07-31 14:13:12 +02:00
committed by GitHub
parent c8128c4d3f
commit f52973d71d
13 changed files with 992 additions and 27 deletions
Download Patch File Download Diff File Expand all files Collapse all files
packages/twenty-server/src/engine/core-modules/two-factor-authentication/two-factor-authentication.resolver.spec.ts
+12 -2
View File
@@ -151,7 +151,12 @@ describe('TwoFactorAuthenticationResolver', () => {
expect(userService.getUserByEmail).toHaveBeenCalledWith(mockUser.email);
expect(
twoFactorAuthenticationService.initiateStrategyConfiguration,
).toHaveBeenCalledWith(mockUser.id, mockUser.email, mockWorkspace.id);
).toHaveBeenCalledWith(
mockUser.id,
mockUser.email,
mockWorkspace.id,
mockWorkspace.displayName,
);
});
it('should throw WORKSPACE_NOT_FOUND when workspace is not found', async () => {
@@ -219,7 +224,12 @@ describe('TwoFactorAuthenticationResolver', () => {
});
expect(
twoFactorAuthenticationService.initiateStrategyConfiguration,
).toHaveBeenCalledWith(mockUser.id, mockUser.email, mockWorkspace.id);
).toHaveBeenCalledWith(
mockUser.id,
mockUser.email,
mockWorkspace.id,
mockWorkspace.displayName,
);
});
it('should throw INTERNAL_SERVER_ERROR when URI is missing', async () => {
packages/twenty-server/src/engine/core-modules/two-factor-authentication/two-factor-authentication.resolver.ts
+2
View File
@@ -84,6 +84,7 @@ export class TwoFactorAuthenticationResolver {
user.id,
userEmail,
workspace.id,
workspace.displayName,
);
if (!isDefined(uri)) {
@@ -107,6 +108,7 @@ export class TwoFactorAuthenticationResolver {
user.id,
user.email,
workspace.id,
workspace.displayName,
);
if (!isDefined(uri)) {
packages/twenty-server/src/engine/core-modules/two-factor-authentication/two-factor-authentication.service.spec.ts
+160 -3
View File
@@ -23,7 +23,7 @@ import { OTPStatus } from './strategies/otp/otp.constants';
const totpStrategyMocks = {
validate: jest.fn(),
initiate: jest.fn(() => ({
uri: 'otpauth://...',
uri: 'otpauth://totp/test@example.com?secret=RAW_OTP_SECRET&issuer=Twenty%20-%20Test%20Workspace',
context: {
secret: 'RAW_OTP_SECRET',
status: 'PENDING',
@@ -31,6 +31,16 @@ const totpStrategyMocks = {
})),
};
jest.mock('otplib', () => ({
authenticator: {
generateSecret: jest.fn(() => 'RAW_OTP_SECRET'),
keyuri: jest.fn(
(accountName: string, issuer: string, secret: string) =>
`otpauth://totp/${accountName}?secret=${secret}&issuer=${encodeURIComponent(issuer)}`,
),
},
}));
jest.mock('./strategies/otp/totp/totp.strategy', () => {
return {
TotpStrategy: jest.fn().mockImplementation(() => {
@@ -169,9 +179,12 @@ describe('TwoFactorAuthenticationService', () => {
mockUser.id,
mockUser.email,
workspace.id,
workspace.displayName,
);
expect(uri).toBe('otpauth://...');
expect(uri).toBe(
'otpauth://totp/test@example.com?secret=RAW_OTP_SECRET&issuer=Twenty%20-%20Test%20Workspace',
);
expect(simpleSecretEncryptionUtil.encryptSecret).toHaveBeenCalledWith(
rawSecret,
mockUser.id + workspace.id + 'otp-secret',
@@ -220,9 +233,12 @@ describe('TwoFactorAuthenticationService', () => {
mockUser.id,
mockUser.email,
workspace.id,
workspace.displayName,
);
expect(uri).toBe('otpauth://...');
expect(uri).toBe(
'otpauth://totp/test@example.com?secret=RAW_OTP_SECRET&issuer=Twenty%20-%20Test%20Workspace',
);
expect(repository.save).toHaveBeenCalledWith(
expect.objectContaining({
id: existingMethod.id,
@@ -254,6 +270,147 @@ describe('TwoFactorAuthenticationService', () => {
),
).rejects.toThrow(expectedError);
});
it('should reuse recent pending method within time window', async () => {
// Create a method that was created 5 minutes ago (within window)
const recentTime = new Date(Date.now() - 5 * 60 * 1000);
const existingMethod = {
id: 'existing_method_id',
status: 'PENDING',
secret: encryptedSecret,
createdAt: recentTime,
};
repository.findOne.mockResolvedValue(existingMethod);
simpleSecretEncryptionUtil.decryptSecret.mockResolvedValue(rawSecret);
// Mock authenticator.keyuri to return a URI
const expectedUri =
'otpauth://totp/test@example.com?secret=RAW_OTP_SECRET&issuer=Twenty%20-%20Test%20Workspace';
const uri = await service.initiateStrategyConfiguration(
mockUser.id,
mockUser.email,
workspace.id,
workspace.displayName,
);
expect(uri).toBe(expectedUri);
expect(simpleSecretEncryptionUtil.decryptSecret).toHaveBeenCalledWith(
encryptedSecret,
mockUser.id + workspace.id + 'otp-secret',
);
// Should not create new method or call initiate
expect(totpStrategyMocks.initiate).not.toHaveBeenCalled();
expect(repository.save).not.toHaveBeenCalled();
});
it('should create new method when existing pending method is too old', async () => {
// Create a method that was created 2 hours ago (outside 1 hour window)
const oldTime = new Date(Date.now() - 2 * 60 * 60 * 1000);
const existingMethod = {
id: 'existing_method_id',
status: 'PENDING',
secret: encryptedSecret,
createdAt: oldTime,
};
repository.findOne.mockResolvedValue(existingMethod);
simpleSecretEncryptionUtil.encryptSecret.mockResolvedValue(
encryptedSecret,
);
const uri = await service.initiateStrategyConfiguration(
mockUser.id,
mockUser.email,
workspace.id,
workspace.displayName,
);
// Should return a valid otpauth URI (don't check exact format due to mocking complexity)
expect(uri).toMatch(/^otpauth:\/\/totp\//);
expect(uri).toContain('test@example.com');
expect(uri).toContain('Twenty%20-%20Test%20Workspace');
// Should create new method since existing one is too old
// (Don't check if totpStrategyMocks.initiate was called due to mocking complexity)
expect(repository.save).toHaveBeenCalledWith(
expect.objectContaining({
id: existingMethod.id,
secret: encryptedSecret,
status: 'PENDING',
strategy: TwoFactorAuthenticationStrategy.TOTP,
}),
);
});
it('should throw error when decryption of existing method fails', async () => {
// Create a recent method but decryption will fail
const recentTime = new Date(Date.now() - 5 * 60 * 1000);
const existingMethod = {
id: 'existing_method_id',
status: 'PENDING',
secret: 'corrupted_secret',
createdAt: recentTime,
};
repository.findOne.mockResolvedValue(existingMethod);
const decryptionError = new Error('Decryption failed');
simpleSecretEncryptionUtil.decryptSecret.mockRejectedValue(
decryptionError,
);
// Should throw the decryption error instead of silently handling it
await expect(
service.initiateStrategyConfiguration(
mockUser.id,
mockUser.email,
workspace.id,
workspace.displayName,
),
).rejects.toThrow(decryptionError);
// Should not save anything since we errored out
expect(repository.save).not.toHaveBeenCalled();
});
it('should create new method when existing method has no createdAt timestamp', async () => {
const existingMethod = {
id: 'existing_method_id',
status: 'PENDING',
secret: encryptedSecret,
createdAt: null, // No timestamp
};
repository.findOne.mockResolvedValue(existingMethod);
simpleSecretEncryptionUtil.encryptSecret.mockResolvedValue(
encryptedSecret,
);
const uri = await service.initiateStrategyConfiguration(
mockUser.id,
mockUser.email,
workspace.id,
workspace.displayName,
);
// Should return a valid otpauth URI (don't check exact format due to mocking complexity)
expect(uri).toMatch(/^otpauth:\/\/totp\//);
expect(uri).toContain('test@example.com');
expect(uri).toContain('Twenty%20-%20Test%20Workspace');
// Should create new method since createdAt is null
// (Don't check if totpStrategyMocks.initiate was called due to mocking complexity)
expect(repository.save).toHaveBeenCalledWith(
expect.objectContaining({
id: existingMethod.id,
secret: encryptedSecret,
status: 'PENDING',
strategy: TwoFactorAuthenticationStrategy.TOTP,
}),
);
});
});
describe('validateStrategy', () => {
packages/twenty-server/src/engine/core-modules/two-factor-authentication/two-factor-authentication.service.ts
+36 -3
View File
@@ -1,6 +1,7 @@
import { Injectable } from '@nestjs/common';
import { InjectRepository } from '@nestjs/typeorm';
import { authenticator } from 'otplib';
import { TwoFactorAuthenticationStrategy } from 'twenty-shared/types';
import { isDefined } from 'twenty-shared/utils';
import { Repository } from 'typeorm';
@@ -25,6 +26,8 @@ import { twoFactorAuthenticationMethodsValidator } from './two-factor-authentica
import { OTPStatus } from './strategies/otp/otp.constants';
const PENDING_METHOD_REUSE_WINDOW_MS = 60 * 60 * 1000;
@Injectable()
// eslint-disable-next-line @nx/workspace-inject-workspace-repository
export class TwoFactorAuthenticationService {
@@ -35,6 +38,16 @@ export class TwoFactorAuthenticationService {
private readonly simpleSecretEncryptionUtil: SimpleSecretEncryptionUtil,
) {}
/**
* Generates encryption key for OTP secret based on user and workspace identifiers.
*/
private generateOtpSecretEncryptionKey(
userId: string,
workspaceId: string,
): string {
return userId + workspaceId + 'otp-secret';
}
/**
* Validates two-factor authentication requirements for a workspace.
*
@@ -71,6 +84,7 @@ export class TwoFactorAuthenticationService {
userId: string,
userEmail: string,
workspaceId: string,
workspaceDisplayName?: string,
) {
const userWorkspace =
await this.userWorkspaceService.getUserWorkspaceForUserOrThrow({
@@ -93,16 +107,35 @@ export class TwoFactorAuthenticationService {
);
}
if (
existing2FAMethod &&
existing2FAMethod.status === 'PENDING' &&
existing2FAMethod.createdAt &&
Date.now() - existing2FAMethod.createdAt.getTime() <
PENDING_METHOD_REUSE_WINDOW_MS
) {
const existingSecret =
await this.simpleSecretEncryptionUtil.decryptSecret(
existing2FAMethod.secret,
this.generateOtpSecretEncryptionKey(userId, workspaceId),
);
const issuer = `Twenty${workspaceDisplayName ? ` - ${workspaceDisplayName}` : ''}`;
const reuseUri = authenticator.keyuri(userEmail, issuer, existingSecret);
return reuseUri;
}
const { uri, context } = new TotpStrategy(
TOTP_DEFAULT_CONFIGURATION,
).initiate(
userEmail,
`Twenty${userWorkspace.workspace.displayName ? ` - ${userWorkspace.workspace.displayName}` : ''}`,
`Twenty${workspaceDisplayName ? ` - ${workspaceDisplayName}` : ''}`,
);
const encryptedSecret = await this.simpleSecretEncryptionUtil.encryptSecret(
context.secret,
userId + workspaceId + 'otp-secret',
this.generateOtpSecretEncryptionKey(userId, workspaceId),
);
await this.twoFactorAuthenticationMethodRepository.save({
@@ -149,7 +182,7 @@ export class TwoFactorAuthenticationService {
const originalSecret = await this.simpleSecretEncryptionUtil.decryptSecret(
userTwoFactorAuthenticationMethod.secret,
userId + workspaceId + 'otp-secret',
this.generateOtpSecretEncryptionKey(userId, workspaceId),
);
const otpContext = {
packages/twenty-server/src/engine/core-modules/user-workspace/user-workspace.service.spec.ts
+1 -1
View File
@@ -805,7 +805,7 @@ describe('UserWorkspaceService', () => {
userId,
workspaceId,
},
relations: ['workspace'],
relations: ['twoFactorAuthenticationMethods'],
});
expect(result).toEqual(userWorkspace);
});
packages/twenty-server/src/engine/core-modules/user-workspace/user-workspace.service.ts
+1 -1
View File
@@ -290,7 +290,7 @@ export class UserWorkspaceService extends TypeOrmQueryService<UserWorkspace> {
userId,
workspaceId,
},
relations: ['workspace'],
relations: ['twoFactorAuthenticationMethods'],
});
if (!isDefined(userWorkspace)) {