diff --git a/packages/auth/src/Bootstrap/Container.ts b/packages/auth/src/Bootstrap/Container.ts index cd676b230..3c897ef52 100644 --- a/packages/auth/src/Bootstrap/Container.ts +++ b/packages/auth/src/Bootstrap/Container.ts @@ -1347,6 +1347,7 @@ export class ContainerConfigLoader { container.get(TYPES.Auth_MAX_LOGIN_ATTEMPTS), container.get(TYPES.Auth_LockRepository), container.get(TYPES.Auth_VerifyHumanInteraction), + container.get(TYPES.Auth_Logger), ), ) container diff --git a/packages/auth/src/Domain/UseCase/GetUserKeyParams/GetUserKeyParams.spec.ts b/packages/auth/src/Domain/UseCase/GetUserKeyParams/GetUserKeyParams.spec.ts index e4a208c70..5b8f4304f 100644 --- a/packages/auth/src/Domain/UseCase/GetUserKeyParams/GetUserKeyParams.spec.ts +++ b/packages/auth/src/Domain/UseCase/GetUserKeyParams/GetUserKeyParams.spec.ts @@ -21,7 +21,7 @@ describe('GetUserKeyParams', () => { keyParamsFactory.create = jest.fn().mockReturnValue({ foo: 'bar' }) keyParamsFactory.createPseudoParams = jest.fn().mockReturnValue({ bar: 'baz' }) - user = {} as jest.Mocked + user = { uuid: '1-2-3' } as jest.Mocked userRepository = {} as jest.Mocked userRepository.findOneByUsernameOrEmail = jest.fn().mockReturnValue(user) @@ -97,7 +97,7 @@ describe('GetUserKeyParams', () => { }, }) - expect(pkceRepository.storeCodeChallenge).toHaveBeenCalledWith('test') + expect(pkceRepository.storeCodeChallenge).toHaveBeenCalledWith('test', '1-2-3') }) it('should get pseudo key params for a non existing user - when searching by email', async () => { diff --git a/packages/auth/src/Domain/UseCase/GetUserKeyParams/GetUserKeyParams.ts b/packages/auth/src/Domain/UseCase/GetUserKeyParams/GetUserKeyParams.ts index 81f708857..e7e850c10 100644 --- a/packages/auth/src/Domain/UseCase/GetUserKeyParams/GetUserKeyParams.ts +++ b/packages/auth/src/Domain/UseCase/GetUserKeyParams/GetUserKeyParams.ts @@ -63,7 +63,7 @@ export class GetUserKeyParams implements UseCaseInterface { private async createKeyParams(dto: GetUserKeyParamsDTO, user: User, authenticated: boolean): Promise { if (this.isCodeChallengedVersion(dto)) { - await this.pkceRepository.storeCodeChallenge(dto.codeChallenge) + await this.pkceRepository.storeCodeChallenge(dto.codeChallenge, user.uuid) } return this.keyParamsFactory.create(user, authenticated) diff --git a/packages/auth/src/Domain/UseCase/GetUserKeyParamsRecovery/GetUserKeyParams.spec.ts b/packages/auth/src/Domain/UseCase/GetUserKeyParamsRecovery/GetUserKeyParams.spec.ts index ba887f5dc..251254030 100644 --- a/packages/auth/src/Domain/UseCase/GetUserKeyParamsRecovery/GetUserKeyParams.spec.ts +++ b/packages/auth/src/Domain/UseCase/GetUserKeyParamsRecovery/GetUserKeyParams.spec.ts @@ -22,7 +22,7 @@ describe('GetUserKeyParamsRecovery', () => { keyParamsFactory.create = jest.fn().mockReturnValue({ foo: 'bar' }) keyParamsFactory.createPseudoParams = jest.fn().mockReturnValue({ bar: 'baz' }) - user = {} as jest.Mocked + user = { uuid: 'user-uuid' } as jest.Mocked userRepository = {} as jest.Mocked userRepository.findOneByUsernameOrEmail = jest.fn().mockReturnValue(user) @@ -121,6 +121,7 @@ describe('GetUserKeyParamsRecovery', () => { }) expect(keyParamsFactory.create).toHaveBeenCalled() + expect(pkceRepository.storeCodeChallenge).toHaveBeenCalledWith('codeChallenge', 'user-uuid') expect(result.isFailed()).toBe(false) }) diff --git a/packages/auth/src/Domain/UseCase/GetUserKeyParamsRecovery/GetUserKeyParamsRecovery.ts b/packages/auth/src/Domain/UseCase/GetUserKeyParamsRecovery/GetUserKeyParamsRecovery.ts index 8df307292..84aa14af7 100644 --- a/packages/auth/src/Domain/UseCase/GetUserKeyParamsRecovery/GetUserKeyParamsRecovery.ts +++ b/packages/auth/src/Domain/UseCase/GetUserKeyParamsRecovery/GetUserKeyParamsRecovery.ts @@ -70,7 +70,7 @@ export class GetUserKeyParamsRecovery implements UseCaseInterface } private async createKeyParams(codeChallenge: string, user: User): Promise { - await this.pkceRepository.storeCodeChallenge(codeChallenge) + await this.pkceRepository.storeCodeChallenge(codeChallenge, user.uuid) return this.keyParamsFactory.create(user, false) } diff --git a/packages/auth/src/Domain/UseCase/SignIn.spec.ts b/packages/auth/src/Domain/UseCase/SignIn.spec.ts index 43b8e23b4..6cfb276b1 100644 --- a/packages/auth/src/Domain/UseCase/SignIn.spec.ts +++ b/packages/auth/src/Domain/UseCase/SignIn.spec.ts @@ -242,9 +242,31 @@ describe('SignIn', () => { expect(domainEventFactory.createEmailRequestedEvent).toHaveBeenCalled() expect(domainEventPublisher.publish).toHaveBeenCalled() + expect(pkceRepository.removeCodeChallenge).toHaveBeenCalledWith('base64-url-encoded', '1-2-3') expect(clearLoginAttempts.execute).toHaveBeenCalledWith({ email: 'test@test.te' }) }) + it('should not sign in when pkce challenge was registered for a different user', async () => { + pkceRepository.removeCodeChallenge = jest.fn().mockReturnValue(false) + + expect( + await createUseCase().execute({ + email: 'test@test.te', + password: 'qweqwe123123', + userAgent: 'Google Chrome', + apiVersion: '20190520', + ephemeralSession: false, + codeVerifier: 'test', + }), + ).toEqual({ + success: false, + errorMessage: 'Invalid email or password', + isNonCaptchaLimitReached: false, + }) + + expect(pkceRepository.removeCodeChallenge).toHaveBeenCalledWith('base64-url-encoded', '1-2-3') + }) + it('should sign in a user even if publishing a sign in event fails', async () => { domainEventPublisher.publish = jest.fn().mockImplementation(() => { throw new Error('Oops') diff --git a/packages/auth/src/Domain/UseCase/SignIn.ts b/packages/auth/src/Domain/UseCase/SignIn.ts index 31a8e7ac8..d9cd6a040 100644 --- a/packages/auth/src/Domain/UseCase/SignIn.ts +++ b/packages/auth/src/Domain/UseCase/SignIn.ts @@ -47,13 +47,6 @@ export class SignIn implements UseCaseInterface { ) } - const validCodeVerifier = await this.validateCodeVerifier(dto.codeVerifier) - if (!validCodeVerifier) { - this.logger.debug('Code verifier does not match') - - return this.failAfterIncrementingLoginAttempts(dto.email, 'Invalid email or password') - } - const apiVersionOrError = ApiVersion.create(dto.apiVersion) if (apiVersionOrError.isFailed()) { return this.failAfterIncrementingLoginAttempts(dto.email, apiVersionOrError.getError()) @@ -88,6 +81,13 @@ export class SignIn implements UseCaseInterface { return this.failAfterIncrementingLoginAttempts(dto.email, 'Invalid email or password') } + const validCodeVerifier = await this.validateCodeVerifier(dto.codeVerifier, user.uuid) + if (!validCodeVerifier) { + this.logger.debug('Code verifier does not match') + + 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') @@ -137,10 +137,13 @@ export class SignIn implements UseCaseInterface { } } - private async validateCodeVerifier(codeVerifier: string): Promise { + private async validateCodeVerifier(codeVerifier: string, userUuid: string): Promise { const codeChallenge = this.crypter.base64URLEncode(this.crypter.sha256Hash(codeVerifier)) - const matchingCodeChallengeWasPresentAndRemoved = await this.pkceRepository.removeCodeChallenge(codeChallenge) + const matchingCodeChallengeWasPresentAndRemoved = await this.pkceRepository.removeCodeChallenge( + codeChallenge, + userUuid, + ) return matchingCodeChallengeWasPresentAndRemoved } diff --git a/packages/auth/src/Domain/UseCase/SignInWithRecoveryCodes/SignInWithRecoveryCodes.spec.ts b/packages/auth/src/Domain/UseCase/SignInWithRecoveryCodes/SignInWithRecoveryCodes.spec.ts index 7fc7e64bf..854426371 100644 --- a/packages/auth/src/Domain/UseCase/SignInWithRecoveryCodes/SignInWithRecoveryCodes.spec.ts +++ b/packages/auth/src/Domain/UseCase/SignInWithRecoveryCodes/SignInWithRecoveryCodes.spec.ts @@ -16,6 +16,7 @@ import { GetSetting } from '../GetSetting/GetSetting' import { ApiVersion } from '../../Api/ApiVersion' import { LockRepositoryInterface } from '../../User/LockRepositoryInterface' import { VerifyHumanInteraction } from '../VerifyHumanInteraction/VerifyHumanInteraction' +import { Logger } from 'winston' describe('SignInWithRecoveryCodes', () => { let userRepository: UserRepositoryInterface @@ -31,6 +32,7 @@ describe('SignInWithRecoveryCodes', () => { let maxNonCaptchaAttempts: number let lockRepository: LockRepositoryInterface let verifyHumanInteractionUseCase: VerifyHumanInteraction + let logger: Logger const createUseCase = () => new SignInWithRecoveryCodes( @@ -47,6 +49,7 @@ describe('SignInWithRecoveryCodes', () => { maxNonCaptchaAttempts, lockRepository, verifyHumanInteractionUseCase, + logger, ) beforeEach(() => { @@ -93,6 +96,9 @@ describe('SignInWithRecoveryCodes', () => { verifyHumanInteractionUseCase = {} as jest.Mocked verifyHumanInteractionUseCase.execute = jest.fn().mockReturnValue(Result.ok()) + + logger = {} as jest.Mocked + logger.debug = jest.fn() }) const requireHumanVerification = () => { @@ -201,7 +207,7 @@ describe('SignInWithRecoveryCodes', () => { expect(result).toEqual({ success: false, - errorMessage: 'Could not find user', + errorMessage: 'Invalid code verifier', isNonCaptchaLimitReached: false, }) }) diff --git a/packages/auth/src/Domain/UseCase/SignInWithRecoveryCodes/SignInWithRecoveryCodes.ts b/packages/auth/src/Domain/UseCase/SignInWithRecoveryCodes/SignInWithRecoveryCodes.ts index 5e62c2931..a17bcadb4 100644 --- a/packages/auth/src/Domain/UseCase/SignInWithRecoveryCodes/SignInWithRecoveryCodes.ts +++ b/packages/auth/src/Domain/UseCase/SignInWithRecoveryCodes/SignInWithRecoveryCodes.ts @@ -1,5 +1,6 @@ import * as bcrypt from 'bcryptjs' import { Result, SettingName, Username, Uuid, Validator } from '@standardnotes/domain-core' +import { Logger } from 'winston' import { CrypterInterface } from '../../Encryption/CrypterInterface' import { PKCERepositoryInterface } from '../../User/PKCERepositoryInterface' @@ -34,6 +35,7 @@ export class SignInWithRecoveryCodes implements UseCaseInterface { private maxNonCaptchaAttempts: number, private lockRepository: LockRepositoryInterface, private verifyHumanInteractionUseCase: VerifyHumanInteraction, + private logger: Logger, ) {} async execute(dto: SignInWithRecoveryCodesDTO): Promise { @@ -71,8 +73,16 @@ export class SignInWithRecoveryCodes implements UseCaseInterface { } } - const validCodeVerifier = await this.validateCodeVerifier(dto.codeVerifier) + if (!user) { + this.logger.debug(`User with username ${username.value} was not found`) + + return this.failAfterIncrementingLoginAttempts(username.value, 'Invalid code verifier') + } + + const validCodeVerifier = await this.validateCodeVerifier(dto.codeVerifier, user.uuid) if (!validCodeVerifier) { + this.logger.debug('Code verifier does not match') + return this.failAfterIncrementingLoginAttempts(username.value, 'Invalid code verifier') } @@ -86,10 +96,6 @@ export class SignInWithRecoveryCodes implements UseCaseInterface { return this.failAfterIncrementingLoginAttempts(username.value, 'Empty recovery codes') } - if (!user) { - return this.failAfterIncrementingLoginAttempts(username.value, 'Could not find user') - } - const userUuidOrError = Uuid.create(user.uuid) if (userUuidOrError.isFailed()) { return this.failAfterIncrementingLoginAttempts(username.value, 'Invalid user uuid') @@ -169,7 +175,7 @@ export class SignInWithRecoveryCodes implements UseCaseInterface { } } - private async validateCodeVerifier(codeVerifier: string): Promise { + private async validateCodeVerifier(codeVerifier: string, userUuid: string): Promise { const codeEmptinessVerificationResult = Validator.isNotEmpty(codeVerifier) if (codeEmptinessVerificationResult.isFailed()) { return false @@ -177,7 +183,10 @@ export class SignInWithRecoveryCodes implements UseCaseInterface { const codeChallenge = this.crypter.base64URLEncode(this.crypter.sha256Hash(codeVerifier)) - const matchingCodeChallengeWasPresentAndRemoved = await this.pkceRepository.removeCodeChallenge(codeChallenge) + const matchingCodeChallengeWasPresentAndRemoved = await this.pkceRepository.removeCodeChallenge( + codeChallenge, + userUuid, + ) return matchingCodeChallengeWasPresentAndRemoved } diff --git a/packages/auth/src/Domain/User/PKCERepositoryInterface.ts b/packages/auth/src/Domain/User/PKCERepositoryInterface.ts index 3665cdc5e..cf52ca54e 100644 --- a/packages/auth/src/Domain/User/PKCERepositoryInterface.ts +++ b/packages/auth/src/Domain/User/PKCERepositoryInterface.ts @@ -1,4 +1,4 @@ export interface PKCERepositoryInterface { - storeCodeChallenge(codeChallenge: string): Promise - removeCodeChallenge(codeChallenge: string): Promise + storeCodeChallenge(codeChallenge: string, userUuid: string): Promise + removeCodeChallenge(codeChallenge: string, userUuid: string): Promise } diff --git a/packages/auth/src/Infra/Redis/RedisPKCERepository.ts b/packages/auth/src/Infra/Redis/RedisPKCERepository.ts index 5666c434c..a76880e09 100644 --- a/packages/auth/src/Infra/Redis/RedisPKCERepository.ts +++ b/packages/auth/src/Infra/Redis/RedisPKCERepository.ts @@ -14,14 +14,22 @@ export class RedisPKCERepository implements PKCERepositoryInterface { @inject(TYPES.Auth_Logger) private logger: Logger, ) {} - async storeCodeChallenge(codeChallenge: string): Promise { + async storeCodeChallenge(codeChallenge: string, userUuid: string): Promise { this.logger.debug(`Storing code challenge: ${codeChallenge}`) - await this.redisClient.setex(`${this.PREFIX}:${codeChallenge}`, 3600, codeChallenge) + await this.redisClient.setex(`${this.PREFIX}:${codeChallenge}`, 3600, userUuid) } - async removeCodeChallenge(codeChallenge: string): Promise { - const entriesRemoved = await this.redisClient.del(`${this.PREFIX}:${codeChallenge}`) + async removeCodeChallenge(codeChallenge: string, userUuid: string): Promise { + const key = `${this.PREFIX}:${codeChallenge}` + const storedUserUuid = await this.redisClient.get(key) + + // Legacy entries (pre user-uuid binding) stored value = codeChallenge; remove after 3600s TTL window. + if (!storedUserUuid || (storedUserUuid !== userUuid && storedUserUuid !== codeChallenge)) { + return false + } + + const entriesRemoved = await this.redisClient.del(key) this.logger.debug(`Removed ${entriesRemoved} entries for code challenge: ${codeChallenge}`) diff --git a/packages/auth/src/Infra/TypeORM/TypeORMPKCERepository.ts b/packages/auth/src/Infra/TypeORM/TypeORMPKCERepository.ts index 4f2131726..21497a69b 100644 --- a/packages/auth/src/Infra/TypeORM/TypeORMPKCERepository.ts +++ b/packages/auth/src/Infra/TypeORM/TypeORMPKCERepository.ts @@ -13,20 +13,29 @@ export class TypeORMPKCERepository implements PKCERepositoryInterface { private timer: TimerInterface, ) {} - async storeCodeChallenge(codeChallenge: string): Promise { + async storeCodeChallenge(codeChallenge: string, userUuid: string): Promise { this.logger.debug(`Storing code challenge: ${codeChallenge}`) await this.cacheEntryRepository.save( CacheEntry.create({ key: `${this.PREFIX}:${codeChallenge}`, - value: codeChallenge, + value: userUuid, expiresAt: this.timer.getUTCDateNSecondsAhead(3600), }).getValue(), ) } - async removeCodeChallenge(codeChallenge: string): Promise { - await this.cacheEntryRepository.removeByKey(`${this.PREFIX}:${codeChallenge}`) + async removeCodeChallenge(codeChallenge: string, userUuid: string): Promise { + const key = `${this.PREFIX}:${codeChallenge}` + const cacheEntry = await this.cacheEntryRepository.findUnexpiredOneByKey(key) + + // Legacy entries (pre user-uuid binding) stored value = codeChallenge; remove after 3600s TTL window. + const storedValue = cacheEntry?.props.value + if (!storedValue || (storedValue !== userUuid && storedValue !== codeChallenge)) { + return false + } + + await this.cacheEntryRepository.removeByKey(key) return true }