diff --git a/packages/auth/src/Bootstrap/Container.ts b/packages/auth/src/Bootstrap/Container.ts index a89008224..254325b48 100644 --- a/packages/auth/src/Bootstrap/Container.ts +++ b/packages/auth/src/Bootstrap/Container.ts @@ -1171,6 +1171,24 @@ export class ContainerConfigLoader { container.get(TYPES.Auth_CaptchaServer), ), ) + container + .bind(TYPES.Auth_ClearLoginAttempts) + .toConstantValue( + new ClearLoginAttempts( + container.get(TYPES.Auth_UserRepository), + container.get(TYPES.Auth_LockRepository), + container.get(TYPES.Auth_Logger), + ), + ) + container + .bind(TYPES.Auth_IncreaseLoginAttempts) + .toConstantValue( + new IncreaseLoginAttempts( + container.get(TYPES.Auth_UserRepository), + container.get(TYPES.Auth_LockRepository), + container.get(TYPES.Auth_MAX_LOGIN_ATTEMPTS), + ), + ) container .bind(TYPES.Auth_SignIn) .toConstantValue( @@ -1186,6 +1204,8 @@ export class ContainerConfigLoader { container.get(TYPES.Auth_MAX_LOGIN_ATTEMPTS), container.get(TYPES.Auth_LockRepository), container.get(TYPES.Auth_VerifyHumanInteraction), + container.get(TYPES.Auth_IncreaseLoginAttempts), + container.get(TYPES.Auth_ClearLoginAttempts), ), ) container @@ -1204,24 +1224,6 @@ export class ContainerConfigLoader { container.get(TYPES.Auth_Logger), ), ) - container - .bind(TYPES.Auth_ClearLoginAttempts) - .toConstantValue( - new ClearLoginAttempts( - container.get(TYPES.Auth_UserRepository), - container.get(TYPES.Auth_LockRepository), - container.get(TYPES.Auth_Logger), - ), - ) - container - .bind(TYPES.Auth_IncreaseLoginAttempts) - .toConstantValue( - new IncreaseLoginAttempts( - container.get(TYPES.Auth_UserRepository), - container.get(TYPES.Auth_LockRepository), - container.get(TYPES.Auth_MAX_LOGIN_ATTEMPTS), - ), - ) container .bind(TYPES.Auth_GetUserKeyParamsRecovery) .toConstantValue( @@ -1871,7 +1873,6 @@ export class ContainerConfigLoader { container.get(TYPES.Auth_SignIn), container.get(TYPES.Auth_GetUserKeyParams), container.get(TYPES.Auth_ClearLoginAttempts), - container.get(TYPES.Auth_IncreaseLoginAttempts), container.get(TYPES.Auth_Logger), container.get(TYPES.Auth_AuthController), container.get(TYPES.Auth_Register), diff --git a/packages/auth/src/Domain/UseCase/SignIn.spec.ts b/packages/auth/src/Domain/UseCase/SignIn.spec.ts index b7519dac4..43b8e23b4 100644 --- a/packages/auth/src/Domain/UseCase/SignIn.spec.ts +++ b/packages/auth/src/Domain/UseCase/SignIn.spec.ts @@ -17,6 +17,8 @@ import { Session } from '../Session/Session' import { LockRepositoryInterface } from '../User/LockRepositoryInterface' import { VerifyHumanInteraction } from './VerifyHumanInteraction/VerifyHumanInteraction' import { Result } from '@standardnotes/domain-core' +import { IncreaseLoginAttempts } from './IncreaseLoginAttempts' +import { ClearLoginAttempts } from './ClearLoginAttempts' describe('SignIn', () => { let user: User @@ -33,7 +35,8 @@ describe('SignIn', () => { let maxNonCaptchaAttempts: number let lockRepository: LockRepositoryInterface let verifyHumanInteractionUseCase: VerifyHumanInteraction - + let increaseLoginAttempts: IncreaseLoginAttempts + let clearLoginAttempts: ClearLoginAttempts const createUseCase = () => new SignIn( userRepository, @@ -47,6 +50,8 @@ describe('SignIn', () => { maxNonCaptchaAttempts, lockRepository, verifyHumanInteractionUseCase, + increaseLoginAttempts, + clearLoginAttempts, ) beforeEach(() => { @@ -92,8 +97,22 @@ describe('SignIn', () => { lockRepository.getLockCounter = jest.fn().mockReturnValue(0) maxNonCaptchaAttempts = 6 + + increaseLoginAttempts = {} as jest.Mocked + increaseLoginAttempts.execute = jest.fn().mockReturnValue(Result.ok({ isNonCaptchaLimitReached: false })) + + clearLoginAttempts = {} as jest.Mocked + clearLoginAttempts.execute = jest.fn() + + verifyHumanInteractionUseCase = {} as jest.Mocked + verifyHumanInteractionUseCase.execute = jest.fn().mockReturnValue(Result.ok()) }) + const requireHumanVerification = () => { + lockRepository.getLockCounter = jest.fn().mockReturnValueOnce(maxNonCaptchaAttempts).mockReturnValueOnce(0) + verifyHumanInteractionUseCase.execute = jest.fn() + } + it('should fail sign in a legacy user without code verifier', async () => { pkceRepository.removeCodeChallenge = jest.fn().mockReturnValue(false) @@ -113,6 +132,7 @@ describe('SignIn', () => { success: false, errorCode: 410, errorMessage: 'Please update your client application.', + isNonCaptchaLimitReached: false, }) }) @@ -132,6 +152,7 @@ describe('SignIn', () => { success: false, errorCode: 410, errorMessage: 'Please update your client application.', + isNonCaptchaLimitReached: false, }) }) @@ -157,6 +178,7 @@ describe('SignIn', () => { success: false, errorCode: 410, errorMessage: 'Please update your client application.', + isNonCaptchaLimitReached: false, }) }) @@ -173,6 +195,7 @@ describe('SignIn', () => { ).toEqual({ success: false, errorMessage: 'Username cannot be empty', + isNonCaptchaLimitReached: false, }) expect(domainEventFactory.createEmailRequestedEvent).not.toHaveBeenCalled() @@ -192,6 +215,7 @@ describe('SignIn', () => { ).toEqual({ success: false, errorMessage: 'Invalid api version: invalid', + isNonCaptchaLimitReached: false, }) expect(domainEventFactory.createEmailRequestedEvent).not.toHaveBeenCalled() @@ -218,6 +242,7 @@ describe('SignIn', () => { expect(domainEventFactory.createEmailRequestedEvent).toHaveBeenCalled() expect(domainEventPublisher.publish).toHaveBeenCalled() + expect(clearLoginAttempts.execute).toHaveBeenCalledWith({ email: 'test@test.te' }) }) it('should sign in a user even if publishing a sign in event fails', async () => { @@ -256,6 +281,7 @@ describe('SignIn', () => { ).toEqual({ success: false, errorMessage: 'Invalid email or password', + isNonCaptchaLimitReached: false, }) }) @@ -274,6 +300,7 @@ describe('SignIn', () => { ).toEqual({ success: false, errorMessage: 'Invalid email or password', + isNonCaptchaLimitReached: false, }) }) @@ -292,6 +319,7 @@ describe('SignIn', () => { ).toEqual({ success: false, errorMessage: 'Invalid email or password', + isNonCaptchaLimitReached: false, }) }) @@ -363,6 +391,131 @@ describe('SignIn', () => { ).toEqual({ success: false, errorMessage: 'Human verification step failed.', + isNonCaptchaLimitReached: true, }) }) + + it('should return isNonCaptchaLimitReached when incrementing login attempts reaches the limit', async () => { + increaseLoginAttempts.execute = jest.fn().mockReturnValue(Result.ok({ isNonCaptchaLimitReached: true })) + + expect( + await createUseCase().execute({ + email: 'test@test.te', + password: 'asdasd123123', + userAgent: 'Google Chrome', + apiVersion: '20190520', + ephemeralSession: false, + codeVerifier: 'test', + }), + ).toEqual({ + success: false, + errorMessage: 'Invalid email or password', + isNonCaptchaLimitReached: true, + }) + }) + + it('should increment login attempts on invalid password', async () => { + await createUseCase().execute({ + email: 'test@test.te', + password: 'asdasd123123', + userAgent: 'Google Chrome', + apiVersion: '20190520', + ephemeralSession: false, + codeVerifier: 'test', + }) + + expect(increaseLoginAttempts.execute).toHaveBeenCalledWith({ + email: 'test@test.te', + skipUsernameValidation: true, + }) + }) + + it('should not increment login attempts when human verification fails', async () => { + requireHumanVerification() + verifyHumanInteractionUseCase.execute = jest + .fn() + .mockReturnValueOnce(Result.fail('Human verification step failed.')) + + await createUseCase().execute({ + email: 'test@test.te', + password: 'qweqwe123123', + userAgent: 'Google Chrome', + apiVersion: '20190520', + ephemeralSession: false, + codeVerifier: 'test', + hvmToken: 'bad-token', + }) + + expect(increaseLoginAttempts.execute).not.toHaveBeenCalled() + }) + + it('should not increment login attempts when human verification token is missing', async () => { + requireHumanVerification() + verifyHumanInteractionUseCase.execute = jest.fn().mockReturnValueOnce(Result.fail('No HVM token available.')) + + await createUseCase().execute({ + email: 'test@test.te', + password: 'qweqwe123123', + userAgent: 'Google Chrome', + apiVersion: '20190520', + ephemeralSession: false, + codeVerifier: 'test', + }) + + expect(increaseLoginAttempts.execute).not.toHaveBeenCalled() + }) + + it('should not set isNonCaptchaLimitReached when increasing login attempts fails', async () => { + increaseLoginAttempts.execute = jest.fn().mockReturnValue(Result.fail('invalid email')) + + expect( + await createUseCase().execute({ + email: 'test@test.te', + password: 'asdasd123123', + userAgent: 'Google Chrome', + apiVersion: '20190520', + ephemeralSession: false, + codeVerifier: 'test', + }), + ).toEqual({ + success: false, + errorMessage: 'Invalid email or password', + }) + }) + + it('should require human verification in captcha mode and not increment on failure', async () => { + lockRepository.getLockCounter = jest.fn().mockReturnValueOnce(0).mockReturnValueOnce(1) + verifyHumanInteractionUseCase.execute = jest + .fn() + .mockReturnValueOnce(Result.fail('Human verification step failed.')) + + await createUseCase().execute({ + email: 'test@test.te', + password: 'qweqwe123123', + userAgent: 'Google Chrome', + apiVersion: '20190520', + ephemeralSession: false, + codeVerifier: 'test', + hvmToken: 'bad-token', + }) + + expect(increaseLoginAttempts.execute).not.toHaveBeenCalled() + }) + + it('should not increment login attempts on human verification failure for unknown user', async () => { + userRepository.findOneByUsernameOrEmail = jest.fn().mockReturnValue(null) + lockRepository.getLockCounter = jest.fn().mockReturnValueOnce(maxNonCaptchaAttempts).mockReturnValueOnce(0) + verifyHumanInteractionUseCase.execute = jest.fn().mockReturnValueOnce(Result.fail('No HVM token available.')) + + await createUseCase().execute({ + email: 'test@test.te', + password: 'asdasd123123', + userAgent: 'Google Chrome', + apiVersion: '20190520', + ephemeralSession: false, + codeVerifier: 'test', + }) + + expect(increaseLoginAttempts.execute).not.toHaveBeenCalled() + }) }) diff --git a/packages/auth/src/Domain/UseCase/SignIn.ts b/packages/auth/src/Domain/UseCase/SignIn.ts index 1d7ab1fa5..31a8e7ac8 100644 --- a/packages/auth/src/Domain/UseCase/SignIn.ts +++ b/packages/auth/src/Domain/UseCase/SignIn.ts @@ -18,6 +18,8 @@ import { ApiVersion } from '../Api/ApiVersion' import { HttpStatusCode } from '@standardnotes/responses' import { VerifyHumanInteraction } from './VerifyHumanInteraction/VerifyHumanInteraction' import { LockRepositoryInterface } from '../User/LockRepositoryInterface' +import { IncreaseLoginAttempts } from './IncreaseLoginAttempts' +import { ClearLoginAttempts } from './ClearLoginAttempts' export class SignIn implements UseCaseInterface { constructor( @@ -32,43 +34,36 @@ export class SignIn implements UseCaseInterface { private maxNonCaptchaAttempts: number, private lockRepository: LockRepositoryInterface, private verifyHumanInteractionUseCase: VerifyHumanInteraction, + private increaseLoginAttempts: IncreaseLoginAttempts, + private clearLoginAttempts: ClearLoginAttempts, ) {} async execute(dto: SignInDTO): Promise { if (!dto.codeVerifier) { - return { - success: false, - errorMessage: 'Please update your client application.', - errorCode: HttpStatusCode.Gone, - } + return this.failAfterIncrementingLoginAttempts( + dto.email, + 'Please update your client application.', + HttpStatusCode.Gone, + ) } const validCodeVerifier = await this.validateCodeVerifier(dto.codeVerifier) if (!validCodeVerifier) { this.logger.debug('Code verifier does not match') - return { - success: false, - errorMessage: 'Invalid email or password', - } + return this.failAfterIncrementingLoginAttempts(dto.email, 'Invalid email or password') } const apiVersionOrError = ApiVersion.create(dto.apiVersion) if (apiVersionOrError.isFailed()) { - return { - success: false, - errorMessage: apiVersionOrError.getError(), - } + return this.failAfterIncrementingLoginAttempts(dto.email, apiVersionOrError.getError()) } const apiVersion = apiVersionOrError.getValue() /** Skip validation which was newly added in 2025, to allow existing users to continue to sign in */ const usernameOrError = Username.create(dto.email, { skipValidation: true }) if (usernameOrError.isFailed()) { - return { - success: false, - errorMessage: usernameOrError.getError(), - } + return this.failAfterIncrementingLoginAttempts(dto.email, usernameOrError.getError()) } const username = usernameOrError.getValue() @@ -83,26 +78,21 @@ export class SignIn implements UseCaseInterface { return { success: false, errorMessage: humanVerificationBeforeCheckingUsernameAndPasswordResult.getError(), + isNonCaptchaLimitReached: true, } } if (!user) { this.logger.debug(`User with email ${dto.email} was not found`) - return { - success: false, - errorMessage: 'Invalid email or password', - } + return this.failAfterIncrementingLoginAttempts(dto.email, 'Invalid email or password') } const passwordMatches = await bcrypt.compare(dto.password, user.encryptedPassword) if (!passwordMatches) { this.logger.debug('Password does not match') - return { - success: false, - errorMessage: 'Invalid email or password', - } + return this.failAfterIncrementingLoginAttempts(dto.email, 'Invalid email or password') } const authResponseFactory = this.authResponseFactoryResolver.resolveAuthResponseFactoryVersion(apiVersion) @@ -119,12 +109,34 @@ export class SignIn implements UseCaseInterface { application: dto.application, }) + await this.clearLoginAttempts.execute({ email: dto.email }) + return { success: true, result, } } + private async failAfterIncrementingLoginAttempts( + email: string, + errorMessage: string, + errorCode?: HttpStatusCode, + ): Promise { + const increaseResultOrError = await this.increaseLoginAttempts.execute({ + email, + skipUsernameValidation: true, + }) + + return { + success: false, + errorMessage, + errorCode, + isNonCaptchaLimitReached: increaseResultOrError.isFailed() + ? undefined + : increaseResultOrError.getValue().isNonCaptchaLimitReached, + } + } + private async validateCodeVerifier(codeVerifier: string): Promise { const codeChallenge = this.crypter.base64URLEncode(this.crypter.sha256Hash(codeVerifier)) diff --git a/packages/auth/src/Domain/UseCase/SignInResponse.ts b/packages/auth/src/Domain/UseCase/SignInResponse.ts index 43576493f..8cd5434d5 100644 --- a/packages/auth/src/Domain/UseCase/SignInResponse.ts +++ b/packages/auth/src/Domain/UseCase/SignInResponse.ts @@ -7,6 +7,7 @@ export type SignInResponse = success: false errorMessage: string errorCode?: HttpStatusCode + isNonCaptchaLimitReached?: boolean } | { success: true diff --git a/packages/auth/src/Domain/UseCase/SignInWithRecoveryCodes/SignInWithRecoveryCodes.spec.ts b/packages/auth/src/Domain/UseCase/SignInWithRecoveryCodes/SignInWithRecoveryCodes.spec.ts index dd3f3b702..7fc7e64bf 100644 --- a/packages/auth/src/Domain/UseCase/SignInWithRecoveryCodes/SignInWithRecoveryCodes.spec.ts +++ b/packages/auth/src/Domain/UseCase/SignInWithRecoveryCodes/SignInWithRecoveryCodes.spec.ts @@ -1,6 +1,5 @@ import { Result } from '@standardnotes/domain-core' -import { AuthResponse20200115 } from '../../Auth/AuthResponse20200115' import { AuthResponseFactory20200115 } from '../../Auth/AuthResponseFactory20200115' import { AuthenticatorRepositoryInterface } from '../../Authenticator/AuthenticatorRepositoryInterface' import { CrypterInterface } from '../../Encryption/CrypterInterface' @@ -58,7 +57,7 @@ describe('SignInWithRecoveryCodes', () => { } as jest.Mocked) authResponseFactory = {} as jest.Mocked - authResponseFactory.createResponse = jest.fn().mockReturnValue({} as jest.Mocked) + authResponseFactory.createResponse = jest.fn().mockReturnValue({ response: { foo: 'bar' }, session: {} }) pkceRepository = {} as jest.Mocked pkceRepository.removeCodeChallenge = jest.fn().mockReturnValue(true) @@ -76,7 +75,7 @@ describe('SignInWithRecoveryCodes', () => { generateRecoveryCodes.execute = jest.fn().mockReturnValue(Result.ok('1234 5678')) increaseLoginAttempts = {} as jest.Mocked - increaseLoginAttempts.execute = jest.fn() + increaseLoginAttempts.execute = jest.fn().mockReturnValue(Result.ok({ isNonCaptchaLimitReached: false })) clearLoginAttempts = {} as jest.Mocked clearLoginAttempts.execute = jest.fn() @@ -91,8 +90,16 @@ describe('SignInWithRecoveryCodes', () => { lockRepository.getLockCounter = jest.fn().mockReturnValue(0) maxNonCaptchaAttempts = 6 + + verifyHumanInteractionUseCase = {} as jest.Mocked + verifyHumanInteractionUseCase.execute = jest.fn().mockReturnValue(Result.ok()) }) + const requireHumanVerification = () => { + lockRepository.getLockCounter = jest.fn().mockReturnValueOnce(maxNonCaptchaAttempts).mockReturnValueOnce(0) + verifyHumanInteractionUseCase.execute = jest.fn() + } + it('should return error if password is not provided', async () => { const result = await createUseCase().execute({ apiVersion: ApiVersion.VERSIONS.v20200115, @@ -103,8 +110,11 @@ describe('SignInWithRecoveryCodes', () => { recoveryCodes: '1234 5678', }) - expect(result.isFailed()).toBe(true) - expect(result.getError()).toBe('Empty password') + expect(result).toEqual({ + success: false, + errorMessage: 'Empty password', + isNonCaptchaLimitReached: false, + }) }) it('should return error if username is not provided', async () => { @@ -117,8 +127,11 @@ describe('SignInWithRecoveryCodes', () => { recoveryCodes: '1234 5678', }) - expect(result.isFailed()).toBe(true) - expect(result.getError()).toBe('Could not sign in with recovery codes: Username cannot be empty') + expect(result).toEqual({ + success: false, + errorMessage: 'Could not sign in with recovery codes: Username cannot be empty', + isNonCaptchaLimitReached: false, + }) }) it('should return error if code verifier is not provided', async () => { @@ -131,8 +144,11 @@ describe('SignInWithRecoveryCodes', () => { recoveryCodes: '1234 5678', }) - expect(result.isFailed()).toBe(true) - expect(result.getError()).toBe('Invalid code verifier') + expect(result).toEqual({ + success: false, + errorMessage: 'Invalid code verifier', + isNonCaptchaLimitReached: false, + }) }) it('should return error if recovery codes are not provided', async () => { @@ -145,8 +161,11 @@ describe('SignInWithRecoveryCodes', () => { recoveryCodes: '', }) - expect(result.isFailed()).toBe(true) - expect(result.getError()).toBe('Empty recovery codes') + expect(result).toEqual({ + success: false, + errorMessage: 'Empty recovery codes', + isNonCaptchaLimitReached: false, + }) }) it('should return error if code verifier is invalid', async () => { @@ -161,8 +180,11 @@ describe('SignInWithRecoveryCodes', () => { recoveryCodes: '1234 5678', }) - expect(result.isFailed()).toBe(true) - expect(result.getError()).toBe('Invalid code verifier') + expect(result).toEqual({ + success: false, + errorMessage: 'Invalid code verifier', + isNonCaptchaLimitReached: false, + }) }) it('should return error if user is not found', async () => { @@ -177,8 +199,11 @@ describe('SignInWithRecoveryCodes', () => { recoveryCodes: '1234 5678', }) - expect(result.isFailed()).toBe(true) - expect(result.getError()).toBe('Could not find user') + expect(result).toEqual({ + success: false, + errorMessage: 'Could not find user', + isNonCaptchaLimitReached: false, + }) }) it('should return error if recovery codes are invalid', async () => { @@ -191,8 +216,11 @@ describe('SignInWithRecoveryCodes', () => { recoveryCodes: '1234 5678', }) - expect(result.isFailed()).toBe(true) - expect(result.getError()).toBe('Invalid recovery codes') + expect(result).toEqual({ + success: false, + errorMessage: 'Invalid recovery codes', + isNonCaptchaLimitReached: false, + }) }) it('should return error if api version is invalid', async () => { @@ -205,7 +233,15 @@ describe('SignInWithRecoveryCodes', () => { recoveryCodes: '1234 5678', }) - expect(result.isFailed()).toBe(true) + expect(result).toEqual({ + success: false, + errorMessage: 'Invalid api version: invalid', + isNonCaptchaLimitReached: false, + }) + expect(increaseLoginAttempts.execute).toHaveBeenCalledWith({ + email: 'test@test.te', + skipUsernameValidation: true, + }) }) it('should return error if api version does not support recovery sign in', async () => { @@ -218,7 +254,15 @@ describe('SignInWithRecoveryCodes', () => { recoveryCodes: '1234 5678', }) - expect(result.isFailed()).toBe(true) + expect(result).toEqual({ + success: false, + errorMessage: 'Unsupported api version', + isNonCaptchaLimitReached: false, + }) + expect(increaseLoginAttempts.execute).toHaveBeenCalledWith({ + email: 'test@test.te', + skipUsernameValidation: true, + }) }) it('should return error if password does not match', async () => { @@ -231,8 +275,11 @@ describe('SignInWithRecoveryCodes', () => { recoveryCodes: '1234 5678', }) - expect(result.isFailed()).toBe(true) - expect(result.getError()).toBe('Invalid password') + expect(result).toEqual({ + success: false, + errorMessage: 'Invalid password', + isNonCaptchaLimitReached: false, + }) }) it('should return error if recovery codes are not generated for user', async () => { @@ -247,8 +294,11 @@ describe('SignInWithRecoveryCodes', () => { recoveryCodes: '1234 5678', }) - expect(result.isFailed()).toBe(true) - expect(result.getError()).toBe('User does not have recovery codes generated') + expect(result).toEqual({ + success: false, + errorMessage: 'User does not have recovery codes generated', + isNonCaptchaLimitReached: false, + }) }) it('should return error if generating new recovery codes fails', async () => { @@ -263,8 +313,12 @@ describe('SignInWithRecoveryCodes', () => { recoveryCodes: 'foo', }) - expect(result.isFailed()).toBe(true) - expect(result.getError()).toBe('Could not sign in with recovery codes: Oops') + expect(result).toEqual({ + success: false, + errorMessage: 'Could not sign in with recovery codes: Oops', + isNonCaptchaLimitReached: false, + }) + expect(authResponseFactory.createResponse).not.toHaveBeenCalled() }) it('should return error if user has an invalid uuid', async () => { @@ -282,8 +336,11 @@ describe('SignInWithRecoveryCodes', () => { recoveryCodes: 'foo', }) - expect(result.isFailed()).toBe(true) - expect(result.getError()).toBe('Invalid user uuid') + expect(result).toEqual({ + success: false, + errorMessage: 'Invalid user uuid', + isNonCaptchaLimitReached: false, + }) }) it('should return error if user requires human verification but no hvmtoken provided', async () => { @@ -302,8 +359,100 @@ describe('SignInWithRecoveryCodes', () => { recoveryCodes: 'foo', }) - expect(result.isFailed()).toBe(true) - expect(result.getError()).toBe('Human verification step failed.') + expect(result).toEqual({ + success: false, + errorMessage: 'Human verification step failed.', + isNonCaptchaLimitReached: true, + }) + }) + + it('should return isNonCaptchaLimitReached when incrementing login attempts reaches the limit', async () => { + increaseLoginAttempts.execute = jest.fn().mockReturnValue(Result.ok({ isNonCaptchaLimitReached: true })) + + const result = await createUseCase().execute({ + apiVersion: ApiVersion.VERSIONS.v20200115, + userAgent: 'user-agent', + username: 'test@test.te', + password: 'asdasd123123', + codeVerifier: 'code-verifier', + recoveryCodes: '1234 5678', + }) + + expect(result).toEqual({ + success: false, + errorMessage: 'Invalid password', + isNonCaptchaLimitReached: true, + }) + }) + + it('should increment login attempts once on invalid password', async () => { + await createUseCase().execute({ + apiVersion: ApiVersion.VERSIONS.v20200115, + userAgent: 'user-agent', + username: 'test@test.te', + password: 'asdasd123123', + codeVerifier: 'code-verifier', + recoveryCodes: '1234 5678', + }) + + expect(increaseLoginAttempts.execute).toHaveBeenCalledTimes(1) + expect(increaseLoginAttempts.execute).toHaveBeenCalledWith({ + email: 'test@test.te', + skipUsernameValidation: true, + }) + }) + + it('should not increment login attempts when human verification fails', async () => { + requireHumanVerification() + verifyHumanInteractionUseCase.execute = jest + .fn() + .mockReturnValueOnce(Result.fail('Human verification step failed.')) + + await createUseCase().execute({ + apiVersion: ApiVersion.VERSIONS.v20200115, + userAgent: 'user-agent', + username: 'test@test.te', + password: 'qweqwe123123', + codeVerifier: 'code-verifier', + recoveryCodes: 'foo', + hvmToken: 'bad-token', + }) + + expect(increaseLoginAttempts.execute).not.toHaveBeenCalled() + }) + + it('should not increment login attempts when human verification token is missing', async () => { + requireHumanVerification() + verifyHumanInteractionUseCase.execute = jest.fn().mockReturnValueOnce(Result.fail('No HVM token available.')) + + await createUseCase().execute({ + apiVersion: ApiVersion.VERSIONS.v20200115, + userAgent: 'user-agent', + username: 'test@test.te', + password: 'qweqwe123123', + codeVerifier: 'code-verifier', + recoveryCodes: 'foo', + }) + + expect(increaseLoginAttempts.execute).not.toHaveBeenCalled() + }) + + it('should not set isNonCaptchaLimitReached when increasing login attempts fails', async () => { + increaseLoginAttempts.execute = jest.fn().mockReturnValue(Result.fail('invalid email')) + + const result = await createUseCase().execute({ + apiVersion: ApiVersion.VERSIONS.v20200115, + userAgent: 'user-agent', + username: 'test@test.te', + password: 'asdasd123123', + codeVerifier: 'code-verifier', + recoveryCodes: '1234 5678', + }) + + expect(result).toEqual({ + success: false, + errorMessage: 'Invalid password', + }) }) it('should return auth response with human verification required and passing', async () => { @@ -326,7 +475,7 @@ describe('SignInWithRecoveryCodes', () => { userUuid: '00000000-0000-0000-0000-000000000000', }) expect(authenticatorRepository.removeByUserUuid).toHaveBeenCalled() - expect(result.isFailed()).toBe(false) + expect(result.success).toBe(true) }) it('should return auth response', async () => { @@ -345,6 +494,6 @@ describe('SignInWithRecoveryCodes', () => { userUuid: '00000000-0000-0000-0000-000000000000', }) expect(authenticatorRepository.removeByUserUuid).toHaveBeenCalled() - expect(result.isFailed()).toBe(false) + expect(result.success).toBe(true) }) }) diff --git a/packages/auth/src/Domain/UseCase/SignInWithRecoveryCodes/SignInWithRecoveryCodes.ts b/packages/auth/src/Domain/UseCase/SignInWithRecoveryCodes/SignInWithRecoveryCodes.ts index bf7e829fe..5e62c2931 100644 --- a/packages/auth/src/Domain/UseCase/SignInWithRecoveryCodes/SignInWithRecoveryCodes.ts +++ b/packages/auth/src/Domain/UseCase/SignInWithRecoveryCodes/SignInWithRecoveryCodes.ts @@ -1,13 +1,13 @@ import * as bcrypt from 'bcryptjs' -import { Result, SettingName, UseCaseInterface, Username, Uuid, Validator } from '@standardnotes/domain-core' +import { Result, SettingName, Username, Uuid, Validator } from '@standardnotes/domain-core' -import { AuthResponse20200115 } from '../../Auth/AuthResponse20200115' import { CrypterInterface } from '../../Encryption/CrypterInterface' import { PKCERepositoryInterface } from '../../User/PKCERepositoryInterface' import { UserRepositoryInterface } from '../../User/UserRepositoryInterface' import { GenerateRecoveryCodes } from '../GenerateRecoveryCodes/GenerateRecoveryCodes' import { SignInWithRecoveryCodesDTO } from './SignInWithRecoveryCodesDTO' +import { SignInWithRecoveryCodesResponse } from './SignInWithRecoveryCodesResponse' import { AuthResponseFactory20200115 } from '../../Auth/AuthResponseFactory20200115' import { IncreaseLoginAttempts } from '../IncreaseLoginAttempts' import { ClearLoginAttempts } from '../ClearLoginAttempts' @@ -17,8 +17,9 @@ import { ApiVersion } from '../../Api/ApiVersion' import { GetSetting } from '../GetSetting/GetSetting' import { LockRepositoryInterface } from '../../User/LockRepositoryInterface' import { VerifyHumanInteraction } from '../VerifyHumanInteraction/VerifyHumanInteraction' +import { UseCaseInterface } from '../UseCaseInterface' -export class SignInWithRecoveryCodes implements UseCaseInterface { +export class SignInWithRecoveryCodes implements UseCaseInterface { constructor( private userRepository: UserRepositoryInterface, private authResponseFactory: AuthResponseFactory20200115, @@ -35,20 +36,23 @@ export class SignInWithRecoveryCodes implements UseCaseInterface> { + async execute(dto: SignInWithRecoveryCodesDTO): Promise { const apiVersionOrError = ApiVersion.create(dto.apiVersion) if (apiVersionOrError.isFailed()) { - return Result.fail(apiVersionOrError.getError()) + return this.failAfterIncrementingLoginAttempts(dto.username, apiVersionOrError.getError()) } const apiVersion = apiVersionOrError.getValue() if (!apiVersion.isSupportedForRecoverySignIn()) { - return Result.fail('Unsupported api version') + return this.failAfterIncrementingLoginAttempts(dto.username, 'Unsupported api version') } const usernameOrError = Username.create(dto.username) if (usernameOrError.isFailed()) { - return Result.fail(`Could not sign in with recovery codes: ${usernameOrError.getError()}`) + return this.failAfterIncrementingLoginAttempts( + dto.username, + `Could not sign in with recovery codes: ${usernameOrError.getError()}`, + ) } const username = usernameOrError.getValue() @@ -60,49 +64,41 @@ export class SignInWithRecoveryCodes implements UseCaseInterface { + const increaseResultOrError = await this.increaseLoginAttempts.execute({ + email, + skipUsernameValidation: true, + }) + + return { + success: false, + errorMessage, + isNonCaptchaLimitReached: increaseResultOrError.isFailed() + ? undefined + : increaseResultOrError.getValue().isNonCaptchaLimitReached, + } } private async validateCodeVerifier(codeVerifier: string): Promise { diff --git a/packages/auth/src/Domain/UseCase/SignInWithRecoveryCodes/SignInWithRecoveryCodesResponse.ts b/packages/auth/src/Domain/UseCase/SignInWithRecoveryCodes/SignInWithRecoveryCodesResponse.ts new file mode 100644 index 000000000..d573330c1 --- /dev/null +++ b/packages/auth/src/Domain/UseCase/SignInWithRecoveryCodes/SignInWithRecoveryCodesResponse.ts @@ -0,0 +1,12 @@ +import { AuthResponseCreationResult } from '../../Auth/AuthResponseCreationResult' + +export type SignInWithRecoveryCodesResponse = + | { + success: false + errorMessage: string + isNonCaptchaLimitReached?: boolean + } + | { + success: true + result: AuthResponseCreationResult + } diff --git a/packages/auth/src/Infra/InversifyExpressUtils/AnnotatedAuthController.ts b/packages/auth/src/Infra/InversifyExpressUtils/AnnotatedAuthController.ts index fa6eaadc6..c474851af 100644 --- a/packages/auth/src/Infra/InversifyExpressUtils/AnnotatedAuthController.ts +++ b/packages/auth/src/Infra/InversifyExpressUtils/AnnotatedAuthController.ts @@ -10,7 +10,6 @@ import TYPES from '../../Bootstrap/Types' import { SignIn } from '../../Domain/UseCase/SignIn' import { ClearLoginAttempts } from '../../Domain/UseCase/ClearLoginAttempts' import { VerifyMFA } from '../../Domain/UseCase/VerifyMFA' -import { IncreaseLoginAttempts } from '../../Domain/UseCase/IncreaseLoginAttempts' import { Logger } from 'winston' import { GetUserKeyParams } from '../../Domain/UseCase/GetUserKeyParams/GetUserKeyParams' import { AuthController } from '../../Controller/AuthController' @@ -32,7 +31,6 @@ export class AnnotatedAuthController extends BaseAuthController { @inject(TYPES.Auth_SignIn) override signInUseCase: SignIn, @inject(TYPES.Auth_GetUserKeyParams) override getUserKeyParams: GetUserKeyParams, @inject(TYPES.Auth_ClearLoginAttempts) override clearLoginAttempts: ClearLoginAttempts, - @inject(TYPES.Auth_IncreaseLoginAttempts) override increaseLoginAttempts: IncreaseLoginAttempts, @inject(TYPES.Auth_Logger) override logger: Logger, @inject(TYPES.Auth_AuthController) override authController: AuthController, @inject(TYPES.Auth_Register) override registerUser: Register, @@ -50,7 +48,6 @@ export class AnnotatedAuthController extends BaseAuthController { signInUseCase, getUserKeyParams, clearLoginAttempts, - increaseLoginAttempts, logger, authController, registerUser, diff --git a/packages/auth/src/Infra/InversifyExpressUtils/Base/BaseAuthController.ts b/packages/auth/src/Infra/InversifyExpressUtils/Base/BaseAuthController.ts index 76727e9bf..5cb1c7e85 100644 --- a/packages/auth/src/Infra/InversifyExpressUtils/Base/BaseAuthController.ts +++ b/packages/auth/src/Infra/InversifyExpressUtils/Base/BaseAuthController.ts @@ -4,7 +4,6 @@ import { Logger } from 'winston' import { ClearLoginAttempts } from '../../../Domain/UseCase/ClearLoginAttempts' import { GetUserKeyParams } from '../../../Domain/UseCase/GetUserKeyParams/GetUserKeyParams' -import { IncreaseLoginAttempts } from '../../../Domain/UseCase/IncreaseLoginAttempts' import { SignIn } from '../../../Domain/UseCase/SignIn' import { VerifyMFA } from '../../../Domain/UseCase/VerifyMFA' import { AuthController } from '../../../Controller/AuthController' @@ -29,7 +28,6 @@ export class BaseAuthController extends BaseHttpController { protected signInUseCase: SignIn, protected getUserKeyParams: GetUserKeyParams, protected clearLoginAttempts: ClearLoginAttempts, - protected increaseLoginAttempts: IncreaseLoginAttempts, protected logger: Logger, protected authController: AuthController, protected registerUser: Register, @@ -147,16 +145,8 @@ export class BaseAuthController extends BaseHttpController { }) if (!signInResult.success) { - const resultOrError = await this.increaseLoginAttempts.execute({ email: request.body.email, skipUsernameValidation: true }) - if (resultOrError.isFailed()) { - this.logger.error(`Failed to increase login attempts: ${resultOrError.getError()}`, { - application: request.headers['x-application-version'] as string, - }) - } else { - const result = resultOrError.getValue() - if (result.isNonCaptchaLimitReached) { - response.setHeader('x-captcha-required', this.captchaUIUrl) - } + if (signInResult.isNonCaptchaLimitReached) { + response.setHeader('x-captcha-required', this.captchaUIUrl) } return this.json( @@ -169,8 +159,6 @@ export class BaseAuthController extends BaseHttpController { ) } - await this.clearLoginAttempts.execute({ email: request.body.email }) - if (signInResult.result.response !== undefined) { const session = signInResult.result.session as Session const user = signInResult.result.response.user @@ -208,7 +196,7 @@ export class BaseAuthController extends BaseHttpController { } async recoveryLogin(request: Request, response: Response): Promise { - const result = await this.signInWithRecoveryCodes.execute({ + const signInResponse = await this.signInWithRecoveryCodes.execute({ apiVersion: request.body.api_version, userAgent: request.headers['user-agent'], codeVerifier: request.body.code_verifier, @@ -220,24 +208,11 @@ export class BaseAuthController extends BaseHttpController { application: request.headers['x-application-version'] as string, }) - if (result.isFailed()) { - this.logger.debug(`Failed to sign in with recovery codes: ${result.getError()}`) + if (!signInResponse.success) { + this.logger.debug(`Failed to sign in with recovery codes: ${signInResponse.errorMessage}`) - const increasLoginAttemtpsResultOrError = await this.increaseLoginAttempts.execute({ - email: request.body.username, - }) - if (increasLoginAttemtpsResultOrError.isFailed()) { - this.logger.error( - `Failed to increase login attempts on recovery login: ${increasLoginAttemtpsResultOrError.getError()}`, - { - application: request.headers['x-application-version'] as string, - }, - ) - } else { - const increasLoginAttemtpsResult = increasLoginAttemtpsResultOrError.getValue() - if (increasLoginAttemtpsResult.isNonCaptchaLimitReached) { - response.setHeader('x-captcha-required', this.captchaUIUrl) - } + if (signInResponse.isNonCaptchaLimitReached) { + response.setHeader('x-captcha-required', this.captchaUIUrl) } return this.json( @@ -250,14 +225,23 @@ export class BaseAuthController extends BaseHttpController { ) } - await this.clearLoginAttempts.execute({ email: request.body.username }) + const authResponse = signInResponse.result.response - const signInWithRecoveryCodesResult = result.getValue() + if (authResponse === undefined) { + return this.json( + { + error: { + message: 'Invalid login credentials.', + }, + }, + HttpStatusCode.Unauthorized, + ) + } return this.json({ - session: signInWithRecoveryCodesResult.sessionBody, - key_params: signInWithRecoveryCodesResult.keyParams, - user: signInWithRecoveryCodesResult.user, + session: authResponse.sessionBody, + key_params: authResponse.keyParams, + user: authResponse.user, }) }