diff --git a/packages/twenty-server/src/engine/core-modules/two-factor-authentication/strategies/otp/totp/constants/totp.strategy.constants.ts b/packages/twenty-server/src/engine/core-modules/two-factor-authentication/strategies/otp/totp/constants/totp.strategy.constants.ts index 03db3853df..ee69684bf5 100644 --- a/packages/twenty-server/src/engine/core-modules/two-factor-authentication/strategies/otp/totp/constants/totp.strategy.constants.ts +++ b/packages/twenty-server/src/engine/core-modules/two-factor-authentication/strategies/otp/totp/constants/totp.strategy.constants.ts @@ -21,7 +21,7 @@ export const TOTP_DEFAULT_CONFIGURATION = { algorithm: TOTPHashAlgorithms.SHA1, digits: 6, encodings: TOTPKeyEncodings.HEX, // Keep as hex - this is correct for @otplib/core - window: 3, + window: 1, step: 30, }; diff --git a/packages/twenty-server/src/engine/core-modules/two-factor-authentication/strategies/otp/totp/totp.strategy.spec.ts b/packages/twenty-server/src/engine/core-modules/two-factor-authentication/strategies/otp/totp/totp.strategy.spec.ts index 60ec132fbb..a9b27d6197 100644 --- a/packages/twenty-server/src/engine/core-modules/two-factor-authentication/strategies/otp/totp/totp.strategy.spec.ts +++ b/packages/twenty-server/src/engine/core-modules/two-factor-authentication/strategies/otp/totp/totp.strategy.spec.ts @@ -2,6 +2,7 @@ import { authenticator } from 'otplib'; import { type PlaintextString } from 'src/engine/core-modules/secret-encryption/branded-strings/plaintext-string.type'; import { OTPStatus } from 'src/engine/core-modules/two-factor-authentication/strategies/otp/otp.constants'; +import { TwoFactorAuthenticationException } from 'src/engine/core-modules/two-factor-authentication/two-factor-authentication.exception'; import { TotpStrategy } from './totp.strategy'; @@ -10,23 +11,21 @@ import { type TotpContext, } from './constants/totp.strategy.constants'; -const RESYNCH_WINDOW = 3; +const FIXED_EPOCH_MS = 1_700_000_000_000; +const STEP_DURATION_MS = 30 * 1000; + +const generateTokenAtEpoch = (secret: string, epochMs: number): string => + authenticator.clone({ epoch: epochMs }).generate(secret); describe('TOTPStrategy Configuration', () => { let strategy: TotpStrategy; let secret: string; let context: TotpContext; - let warnSpy: jest.SpyInstance; beforeEach(() => { - warnSpy = jest.spyOn(console, 'warn').mockImplementation(); secret = authenticator.generateSecret(); }); - afterEach(() => { - warnSpy.mockRestore(); - }); - describe('Valid Configurations', () => { it('should create a strategy with default options', () => { expect(() => new TotpStrategy()).not.toThrow(); @@ -43,27 +42,22 @@ describe('TOTPStrategy Configuration', () => { expect(() => new TotpStrategy(validOptions)).not.toThrow(); }); - it('should warn when all custom options are valid but not recommended', () => { - // Since we simplified the implementation, this test no longer applies - // as we don't have custom configuration warnings + it('should accept a large window', () => { expect(() => new TotpStrategy({ window: 10 })).not.toThrow(); - // Remove the warning expectation since our simplified implementation doesn't warn + }); + }); + + describe('Invalid Configurations', () => { + it('should throw for a negative window', () => { + expect(() => new TotpStrategy({ window: -1 })).toThrow( + TwoFactorAuthenticationException, + ); }); - it('should correctly set the window property', () => { - // Since we simplified the implementation to use otplib defaults, - // we can't directly access internal configuration - const strategy = new TotpStrategy({ window: 10 }); - - expect(strategy).toBeDefined(); - }); - - it('should default window to 0 if not provided', () => { - // Since we simplified the implementation to use otplib defaults, - // we can't directly access internal configuration - const strategy = new TotpStrategy(); - - expect(strategy).toBeDefined(); + it('should throw for digits below the minimum', () => { + expect(() => new TotpStrategy({ digits: 4 })).toThrow( + TwoFactorAuthenticationException, + ); }); }); @@ -92,9 +86,7 @@ describe('TOTPStrategy Configuration', () => { describe('validate', () => { beforeEach(() => { - strategy = new TotpStrategy({ - window: RESYNCH_WINDOW, - }); + strategy = new TotpStrategy({ window: 1, epoch: FIXED_EPOCH_MS }); context = { status: OTPStatus.VERIFIED, @@ -103,12 +95,9 @@ describe('TOTPStrategy Configuration', () => { }); it('should return true for a valid token at the current counter', () => { - // Use the initiate method to generate a proper secret - const initResult = strategy.initiate('test@example.com', 'TestApp'); - // Use authenticator.generate to match what authenticator.check expects - const token = authenticator.generate(initResult.context.secret); + const token = generateTokenAtEpoch(secret, FIXED_EPOCH_MS); - const result = strategy.validate(token, initResult.context); + const result = strategy.validate(token, context); expect(result.isValid).toBe(true); }); @@ -120,23 +109,49 @@ describe('TOTPStrategy Configuration', () => { expect(result.isValid).toBe(false); }); - it('should succeed if the token is valid within the window', () => { - // Use the initiate method to generate a proper secret - const initResult = strategy.initiate('test@example.com', 'TestApp'); - // Use authenticator.generate to match what authenticator.check expects - const futureToken = authenticator.generate(initResult.context.secret); + it('should accept the previous token within the window', () => { + const previousToken = generateTokenAtEpoch( + secret, + FIXED_EPOCH_MS - STEP_DURATION_MS, + ); - const result = strategy.validate(futureToken, initResult.context); + const result = strategy.validate(previousToken, context); expect(result.isValid).toBe(true); }); - it('should fail if the token is valid but outside the window', () => { - // For this test, we'll use a completely invalid token since we can't easily - // generate tokens outside the window with the simplified implementation - const invalidToken = '000000'; + it('should accept the next token within the window', () => { + const nextToken = generateTokenAtEpoch( + secret, + FIXED_EPOCH_MS + STEP_DURATION_MS, + ); - const result = strategy.validate(invalidToken, context); + const result = strategy.validate(nextToken, context); + + expect(result.isValid).toBe(true); + }); + + it('should reject a token generated outside the window', () => { + const staleToken = generateTokenAtEpoch( + secret, + FIXED_EPOCH_MS - 2 * STEP_DURATION_MS, + ); + + const result = strategy.validate(staleToken, context); + + expect(result.isValid).toBe(false); + }); + + it('should reject the previous token when no window is configured', () => { + const strategyWithoutWindow = new TotpStrategy({ + epoch: FIXED_EPOCH_MS, + }); + const previousToken = generateTokenAtEpoch( + secret, + FIXED_EPOCH_MS - STEP_DURATION_MS, + ); + + const result = strategyWithoutWindow.validate(previousToken, context); expect(result.isValid).toBe(false); }); @@ -147,8 +162,6 @@ describe('TOTPStrategy Configuration', () => { secret: 'invalid-secret' as PlaintextString, }; - // The authenticator.check method doesn't throw for invalid secrets, - // it just returns false const result = strategy.validate('123456', invalidContext); expect(result.isValid).toBe(false); @@ -160,18 +173,17 @@ describe('TOTPStrategy Configuration', () => { secret: '' as PlaintextString, }; - // The authenticator.check method doesn't throw for empty secrets, - // it just returns false const result = strategy.validate('123456', invalidContext); expect(result.isValid).toBe(false); }); it('should return the original context on validation success', () => { - // Use the initiate method to generate a proper secret const initResult = strategy.initiate('test@example.com', 'TestApp'); - // Use authenticator.generate to match what authenticator.check expects - const token = authenticator.generate(initResult.context.secret); + const token = generateTokenAtEpoch( + initResult.context.secret, + FIXED_EPOCH_MS, + ); const result = strategy.validate(token, initResult.context); @@ -194,24 +206,27 @@ describe('TOTPStrategy Configuration', () => { }); it('should handle empty token gracefully', () => { - const context = { + const errorHandlingContext = { status: OTPStatus.VERIFIED, secret: secret as PlaintextString, }; - const result = strategy.validate('', context); + const result = strategy.validate('', errorHandlingContext); expect(result.isValid).toBe(false); expect(result.context.status).toBe(OTPStatus.VERIFIED); }); it('should handle null token gracefully', () => { - const context = { + const errorHandlingContext = { status: OTPStatus.VERIFIED, secret: secret as PlaintextString, }; - const result = strategy.validate(null as any, context); + const result = strategy.validate( + null as unknown as string, + errorHandlingContext, + ); expect(result.isValid).toBe(false); expect(result.context.status).toBe(OTPStatus.VERIFIED); diff --git a/packages/twenty-server/src/engine/core-modules/two-factor-authentication/strategies/otp/totp/totp.strategy.ts b/packages/twenty-server/src/engine/core-modules/two-factor-authentication/strategies/otp/totp/totp.strategy.ts index f360b71d8b..51b94780e9 100644 --- a/packages/twenty-server/src/engine/core-modules/two-factor-authentication/strategies/otp/totp/totp.strategy.ts +++ b/packages/twenty-server/src/engine/core-modules/two-factor-authentication/strategies/otp/totp/totp.strategy.ts @@ -3,7 +3,6 @@ import { Injectable } from '@nestjs/common'; import { authenticator } from 'otplib'; import { TwoFactorAuthenticationStrategy } from 'twenty-shared/types'; import { isDefined } from 'twenty-shared/utils'; -import { type ZodSafeParseResult } from 'zod'; import { type OTPAuthenticationStrategyInterface } from 'src/engine/core-modules/two-factor-authentication/strategies/otp/interfaces/otp.strategy.interface'; import { type PlaintextString } from 'src/engine/core-modules/secret-encryption/branded-strings/plaintext-string.type'; @@ -20,15 +19,22 @@ import { TOTPStrategyConfig, } from './constants/totp.strategy.constants'; +type TotpAuthenticator = typeof authenticator; +type TotpAuthenticatorOptions = NonNullable< + Parameters[0] +>; + @Injectable() export class TotpStrategy implements OTPAuthenticationStrategyInterface { public readonly name = TwoFactorAuthenticationStrategy.TOTP; + private readonly totpAuthenticator: TotpAuthenticator; + constructor(options?: TOTPStrategyConfig) { - let result: ZodSafeParseResult | undefined; + let validatedOptions: TOTPStrategyConfig | undefined; if (isDefined(options)) { - result = TOTP_STRATEGY_CONFIG_SCHEMA.safeParse(options); + const result = TOTP_STRATEGY_CONFIG_SCHEMA.safeParse(options); if (!result.success) { const errorMessages = Object.entries(result.error.flatten().fieldErrors) @@ -43,9 +49,51 @@ export class TotpStrategy implements OTPAuthenticationStrategyInterface { TwoFactorAuthenticationExceptionCode.INVALID_CONFIGURATION, ); } + + validatedOptions = result.data; } - // otplib will use its defaults: sha1, 6 digits, 30 second step, etc. + this.totpAuthenticator = authenticator.clone( + this.buildAuthenticatorOptions(validatedOptions), + ); + } + + private buildAuthenticatorOptions( + config?: TOTPStrategyConfig, + ): TotpAuthenticatorOptions { + const authenticatorOptions: TotpAuthenticatorOptions = {}; + + if (!isDefined(config)) { + return authenticatorOptions; + } + + if (isDefined(config.algorithm)) { + authenticatorOptions.algorithm = + config.algorithm as unknown as TotpAuthenticatorOptions['algorithm']; + } + + if (isDefined(config.encodings)) { + authenticatorOptions.encoding = + config.encodings as unknown as TotpAuthenticatorOptions['encoding']; + } + + if (isDefined(config.digits)) { + authenticatorOptions.digits = config.digits; + } + + if (isDefined(config.window)) { + authenticatorOptions.window = config.window; + } + + if (isDefined(config.step)) { + authenticatorOptions.step = config.step; + } + + if (isDefined(config.epoch)) { + authenticatorOptions.epoch = config.epoch; + } + + return authenticatorOptions; } public initiate( @@ -55,8 +103,8 @@ export class TotpStrategy implements OTPAuthenticationStrategyInterface { uri: string; context: TotpContext; } { - const secret = authenticator.generateSecret() as PlaintextString; - const uri = authenticator.keyuri(accountName, issuer, secret); + const secret = this.totpAuthenticator.generateSecret() as PlaintextString; + const uri = this.totpAuthenticator.keyuri(accountName, issuer, secret); return { uri, @@ -74,7 +122,7 @@ export class TotpStrategy implements OTPAuthenticationStrategyInterface { isValid: boolean; context: TotpContext; } { - const isValid = authenticator.check(token, context.secret); + const isValid = this.totpAuthenticator.check(token, context.secret); return { isValid, diff --git a/packages/twenty-server/test/integration/graphql/suites/auth/two-factor-authentication/two-factor-authentication-totp.integration-spec.ts b/packages/twenty-server/test/integration/graphql/suites/auth/two-factor-authentication/two-factor-authentication-totp.integration-spec.ts new file mode 100644 index 0000000000..5387e80cf3 --- /dev/null +++ b/packages/twenty-server/test/integration/graphql/suites/auth/two-factor-authentication/two-factor-authentication-totp.integration-spec.ts @@ -0,0 +1,117 @@ +import { authenticator } from 'otplib'; +import { initiateOtpProvisioningForAuthenticatedUser } from 'test/integration/graphql/utils/initiate-otp-provisioning-for-authenticated-user.util'; +import { verifyTwoFactorAuthenticationMethod } from 'test/integration/graphql/utils/verify-two-factor-authentication-method.util'; + +import { USER_WORKSPACE_DATA_SEED_IDS } from 'src/engine/workspace-manager/dev-seeder/core/utils/seed-user-workspaces.util'; + +const TOTP_STEP_DURATION_MS = 30_000; +const MINIMUM_STEP_REMAINING_MS = 5_000; + +const generateTokenAtEpoch = (secret: string, epochMs: number): string => + authenticator.clone({ epoch: epochMs }).generate(secret); + +// A previous-step token generated right before a step boundary would already be +// two steps old once the server validates it, so wait out the boundary first +const waitUntilSafelyInsideTotpStep = async (): Promise => { + const millisecondsRemainingInStep = + TOTP_STEP_DURATION_MS - (Date.now() % TOTP_STEP_DURATION_MS); + + if (millisecondsRemainingInStep < MINIMUM_STEP_REMAINING_MS) { + await new Promise((resolve) => + setTimeout(resolve, millisecondsRemainingInStep + 100), + ); + } +}; + +const deleteJonyTwoFactorAuthenticationMethods = async (): Promise => { + await global.testDataSource.query( + `DELETE FROM core."twoFactorAuthenticationMethod" WHERE "userWorkspaceId" = $1`, + [USER_WORKSPACE_DATA_SEED_IDS.JONY], + ); +}; + +describe('Two-factor authentication TOTP verification (integration)', () => { + let secret: string; + + beforeAll(async () => { + await deleteJonyTwoFactorAuthenticationMethods(); + + const { data, errors } = await initiateOtpProvisioningForAuthenticatedUser({ + accessToken: APPLE_JONY_MEMBER_ACCESS_TOKEN, + expectToFail: false, + }); + + expect(errors).toBeUndefined(); + + const uri = data.initiateOTPProvisioningForAuthenticatedUser.uri; + + expect(uri).toMatch(/^otpauth:\/\/totp\//); + + const secretFromUri = uri.match(/[?&]secret=([^&]+)/)?.[1]; + + if (secretFromUri === undefined || secretFromUri === '') { + throw new Error('Expected the otpauth URI to contain a secret'); + } + + secret = secretFromUri; + }); + + afterAll(async () => { + await deleteJonyTwoFactorAuthenticationMethods(); + }); + + it('should reject a code that is more than one step old', async () => { + const staleToken = generateTokenAtEpoch( + secret, + Date.now() - 2 * TOTP_STEP_DURATION_MS, + ); + + const { data, errors } = await verifyTwoFactorAuthenticationMethod({ + otp: staleToken, + accessToken: APPLE_JONY_MEMBER_ACCESS_TOKEN, + expectToFail: true, + }); + + expect(errors).toBeDefined(); + expect( + data?.verifyTwoFactorAuthenticationMethodForAuthenticatedUser, + ).toBeFalsy(); + }); + + it('should accept the current code', async () => { + await waitUntilSafelyInsideTotpStep(); + + const currentToken = generateTokenAtEpoch(secret, Date.now()); + + const { data, errors } = await verifyTwoFactorAuthenticationMethod({ + otp: currentToken, + accessToken: APPLE_JONY_MEMBER_ACCESS_TOKEN, + expectToFail: false, + }); + + expect(errors).toBeUndefined(); + expect( + data.verifyTwoFactorAuthenticationMethodForAuthenticatedUser.success, + ).toBe(true); + }); + + it('should accept the code from the previous step', async () => { + await waitUntilSafelyInsideTotpStep(); + + const previousStepToken = generateTokenAtEpoch( + secret, + Date.now() - TOTP_STEP_DURATION_MS, + ); + + const { data, errors } = await verifyTwoFactorAuthenticationMethod({ + otp: previousStepToken, + accessToken: APPLE_JONY_MEMBER_ACCESS_TOKEN, + expectToFail: false, + }); + + expect(errors).toBeUndefined(); + expect( + data.verifyTwoFactorAuthenticationMethodForAuthenticatedUser.success, + ).toBe(true); + }); +}); diff --git a/packages/twenty-server/test/integration/graphql/utils/initiate-otp-provisioning-for-authenticated-user.util.ts b/packages/twenty-server/test/integration/graphql/utils/initiate-otp-provisioning-for-authenticated-user.util.ts new file mode 100644 index 0000000000..c40051637e --- /dev/null +++ b/packages/twenty-server/test/integration/graphql/utils/initiate-otp-provisioning-for-authenticated-user.util.ts @@ -0,0 +1,50 @@ +import gql from 'graphql-tag'; +import { makeMetadataAPIRequest } from 'test/integration/metadata/suites/utils/make-metadata-api-request.util'; +import { type CommonResponseBody } from 'test/integration/metadata/types/common-response-body.type'; +import { warnIfErrorButNotExpectedToFail } from 'test/integration/metadata/utils/warn-if-error-but-not-expected-to-fail.util'; +import { warnIfNoErrorButExpectedToFail } from 'test/integration/metadata/utils/warn-if-no-error-but-expected-to-fail.util'; + +import { type InitiateTwoFactorAuthenticationProvisioningDTO } from 'src/engine/core-modules/two-factor-authentication/dto/initiate-two-factor-authentication-provisioning.dto'; + +type InitiateOtpProvisioningForAuthenticatedUserUtilArgs = { + accessToken: string; + expectToFail?: boolean; +}; + +export const initiateOtpProvisioningForAuthenticatedUser = async ({ + accessToken, + expectToFail, +}: InitiateOtpProvisioningForAuthenticatedUserUtilArgs): CommonResponseBody<{ + initiateOTPProvisioningForAuthenticatedUser: InitiateTwoFactorAuthenticationProvisioningDTO; +}> => { + const mutation = gql` + mutation InitiateOTPProvisioningForAuthenticatedUser { + initiateOTPProvisioningForAuthenticatedUser { + uri + } + } + `; + + const response = await makeMetadataAPIRequest( + { + query: mutation, + }, + accessToken, + ); + + if (expectToFail === true) { + warnIfNoErrorButExpectedToFail({ + response, + errorMessage: 'OTP provisioning should have failed but did not', + }); + } + + if (expectToFail === false) { + warnIfErrorButNotExpectedToFail({ + response, + errorMessage: 'OTP provisioning has failed but should not', + }); + } + + return { data: response.body.data, errors: response.body.errors }; +}; diff --git a/packages/twenty-server/test/integration/graphql/utils/verify-two-factor-authentication-method.util.ts b/packages/twenty-server/test/integration/graphql/utils/verify-two-factor-authentication-method.util.ts new file mode 100644 index 0000000000..310431a882 --- /dev/null +++ b/packages/twenty-server/test/integration/graphql/utils/verify-two-factor-authentication-method.util.ts @@ -0,0 +1,55 @@ +import gql from 'graphql-tag'; +import { makeMetadataAPIRequest } from 'test/integration/metadata/suites/utils/make-metadata-api-request.util'; +import { type CommonResponseBody } from 'test/integration/metadata/types/common-response-body.type'; +import { warnIfErrorButNotExpectedToFail } from 'test/integration/metadata/utils/warn-if-error-but-not-expected-to-fail.util'; +import { warnIfNoErrorButExpectedToFail } from 'test/integration/metadata/utils/warn-if-no-error-but-expected-to-fail.util'; + +import { type VerifyTwoFactorAuthenticationMethodDTO } from 'src/engine/core-modules/two-factor-authentication/dto/verify-two-factor-authentication-method.dto'; + +type VerifyTwoFactorAuthenticationMethodUtilArgs = { + otp: string; + accessToken: string; + expectToFail?: boolean; +}; + +export const verifyTwoFactorAuthenticationMethod = async ({ + otp, + accessToken, + expectToFail, +}: VerifyTwoFactorAuthenticationMethodUtilArgs): CommonResponseBody<{ + verifyTwoFactorAuthenticationMethodForAuthenticatedUser: VerifyTwoFactorAuthenticationMethodDTO; +}> => { + const mutation = gql` + mutation VerifyTwoFactorAuthenticationMethodForAuthenticatedUser( + $otp: String! + ) { + verifyTwoFactorAuthenticationMethodForAuthenticatedUser(otp: $otp) { + success + } + } + `; + + const response = await makeMetadataAPIRequest( + { + query: mutation, + variables: { otp }, + }, + accessToken, + ); + + if (expectToFail === true) { + warnIfNoErrorButExpectedToFail({ + response, + errorMessage: 'OTP verification should have failed but did not', + }); + } + + if (expectToFail === false) { + warnIfErrorButNotExpectedToFail({ + response, + errorMessage: 'OTP verification has failed but should not', + }); + } + + return { data: response.body.data, errors: response.body.errors }; +};