From cc612296d0fbfa7e95556fda45eb9706845e4f58 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Karol=20S=C3=B3jko?= Date: Thu, 18 May 2023 12:30:05 +0200 Subject: [PATCH] fix(auth): changing user credentials to work both on http proxy and direct code call --- .../ChangeCredentials.spec.ts | 52 +++++++++++++++---- .../ChangeCredentials/ChangeCredentials.ts | 30 +++++++---- .../ChangeCredentials/ChangeCredentialsDTO.ts | 4 +- .../InversifyExpressUsersController.spec.ts | 7 +-- .../InversifyExpressUsersController.ts | 16 +++++- 5 files changed, 78 insertions(+), 31 deletions(-) diff --git a/packages/auth/src/Domain/UseCase/ChangeCredentials/ChangeCredentials.spec.ts b/packages/auth/src/Domain/UseCase/ChangeCredentials/ChangeCredentials.spec.ts index f38b1c10d..be59bb722 100644 --- a/packages/auth/src/Domain/UseCase/ChangeCredentials/ChangeCredentials.spec.ts +++ b/packages/auth/src/Domain/UseCase/ChangeCredentials/ChangeCredentials.spec.ts @@ -11,6 +11,7 @@ import { User } from '../../User/User' import { UserRepositoryInterface } from '../../User/UserRepositoryInterface' import { ChangeCredentials } from './ChangeCredentials' +import { Username } from '@standardnotes/domain-core' describe('ChangeCredentials', () => { let userRepository: UserRepositoryInterface @@ -25,9 +26,6 @@ describe('ChangeCredentials', () => { new ChangeCredentials(userRepository, authResponseFactoryResolver, domainEventPublisher, domainEventFactory, timer) beforeEach(() => { - userRepository = {} as jest.Mocked - userRepository.save = jest.fn() - authResponseFactory = {} as jest.Mocked authResponseFactory.createResponse = jest.fn().mockReturnValue({ foo: 'bar' }) @@ -39,6 +37,10 @@ describe('ChangeCredentials', () => { user.uuid = '1-2-3' user.email = 'test@test.te' + userRepository = {} as jest.Mocked + userRepository.save = jest.fn() + userRepository.findOneByUsernameOrEmail = jest.fn().mockReturnValue(user) + domainEventPublisher = {} as jest.Mocked domainEventPublisher.publish = jest.fn() @@ -52,7 +54,7 @@ describe('ChangeCredentials', () => { it('should change password', async () => { expect( await createUseCase().execute({ - user, + username: Username.create('test@test.te').getValue(), apiVersion: '20190520', currentPassword: 'qweqwe123123', newPassword: 'test234', @@ -82,11 +84,11 @@ describe('ChangeCredentials', () => { }) it('should change email', async () => { - userRepository.findOneByUsernameOrEmail = jest.fn().mockReturnValue(null) + userRepository.findOneByUsernameOrEmail = jest.fn().mockReturnValueOnce(user).mockReturnValueOnce(null) expect( await createUseCase().execute({ - user, + username: Username.create('test@test.te').getValue(), apiVersion: '20190520', currentPassword: 'qweqwe123123', newPassword: 'test234', @@ -117,11 +119,14 @@ describe('ChangeCredentials', () => { }) it('should not change email if already taken', async () => { - userRepository.findOneByUsernameOrEmail = jest.fn().mockReturnValue({} as jest.Mocked) + userRepository.findOneByUsernameOrEmail = jest + .fn() + .mockReturnValueOnce(user) + .mockReturnValueOnce({} as jest.Mocked) expect( await createUseCase().execute({ - user, + username: Username.create('test@test.te').getValue(), apiVersion: '20190520', currentPassword: 'qweqwe123123', newPassword: 'test234', @@ -144,7 +149,7 @@ describe('ChangeCredentials', () => { it('should not change email if the new email is invalid', async () => { expect( await createUseCase().execute({ - user, + username: Username.create('test@test.te').getValue(), apiVersion: '20190520', currentPassword: 'qweqwe123123', newPassword: 'test234', @@ -164,10 +169,35 @@ describe('ChangeCredentials', () => { expect(domainEventPublisher.publish).not.toHaveBeenCalled() }) + it('should not change email if the user is not found', async () => { + userRepository.findOneByUsernameOrEmail = jest.fn().mockReturnValue(null) + + expect( + await createUseCase().execute({ + username: Username.create('test@test.te').getValue(), + apiVersion: '20190520', + currentPassword: 'qweqwe123123', + newPassword: 'test234', + newEmail: '', + pwNonce: 'asdzxc', + updatedWithUserAgent: 'Google Chrome', + kpCreated: '123', + kpOrigination: 'password-change', + }), + ).toEqual({ + success: false, + errorMessage: 'User not found.', + }) + + expect(userRepository.save).not.toHaveBeenCalled() + expect(domainEventFactory.createUserEmailChangedEvent).not.toHaveBeenCalled() + expect(domainEventPublisher.publish).not.toHaveBeenCalled() + }) + it('should not change password if current password is incorrect', async () => { expect( await createUseCase().execute({ - user, + username: Username.create('test@test.te').getValue(), apiVersion: '20190520', currentPassword: 'test123', newPassword: 'test234', @@ -185,7 +215,7 @@ describe('ChangeCredentials', () => { it('should update protocol version while changing password', async () => { expect( await createUseCase().execute({ - user, + username: Username.create('test@test.te').getValue(), apiVersion: '20190520', currentPassword: 'qweqwe123123', newPassword: 'test234', diff --git a/packages/auth/src/Domain/UseCase/ChangeCredentials/ChangeCredentials.ts b/packages/auth/src/Domain/UseCase/ChangeCredentials/ChangeCredentials.ts index dbab71d9b..f10adf5a9 100644 --- a/packages/auth/src/Domain/UseCase/ChangeCredentials/ChangeCredentials.ts +++ b/packages/auth/src/Domain/UseCase/ChangeCredentials/ChangeCredentials.ts @@ -25,14 +25,22 @@ export class ChangeCredentials implements UseCaseInterface { ) {} async execute(dto: ChangeCredentialsDTO): Promise { - if (!(await bcrypt.compare(dto.currentPassword, dto.user.encryptedPassword))) { + const user = await this.userRepository.findOneByUsernameOrEmail(dto.username) + if (!user) { + return { + success: false, + errorMessage: 'User not found.', + } + } + + if (!(await bcrypt.compare(dto.currentPassword, user.encryptedPassword))) { return { success: false, errorMessage: 'The current password you entered is incorrect. Please try again.', } } - dto.user.encryptedPassword = await bcrypt.hash(dto.newPassword, User.PASSWORD_HASH_COST) + user.encryptedPassword = await bcrypt.hash(dto.newPassword, User.PASSWORD_HASH_COST) let userEmailChangedEvent: UserEmailChangedEvent | undefined = undefined if (dto.newEmail !== undefined) { @@ -54,27 +62,27 @@ export class ChangeCredentials implements UseCaseInterface { } userEmailChangedEvent = this.domainEventFactory.createUserEmailChangedEvent( - dto.user.uuid, - dto.user.email, + user.uuid, + user.email, newUsername.value, ) - dto.user.email = newUsername.value + user.email = newUsername.value } - dto.user.pwNonce = dto.pwNonce + user.pwNonce = dto.pwNonce if (dto.protocolVersion) { - dto.user.version = dto.protocolVersion + user.version = dto.protocolVersion } if (dto.kpCreated) { - dto.user.kpCreated = dto.kpCreated + user.kpCreated = dto.kpCreated } if (dto.kpOrigination) { - dto.user.kpOrigination = dto.kpOrigination + user.kpOrigination = dto.kpOrigination } - dto.user.updatedAt = this.timer.getUTCDate() + user.updatedAt = this.timer.getUTCDate() - const updatedUser = await this.userRepository.save(dto.user) + const updatedUser = await this.userRepository.save(user) if (userEmailChangedEvent !== undefined) { await this.domainEventPublisher.publish(userEmailChangedEvent) diff --git a/packages/auth/src/Domain/UseCase/ChangeCredentials/ChangeCredentialsDTO.ts b/packages/auth/src/Domain/UseCase/ChangeCredentials/ChangeCredentialsDTO.ts index 31a82363e..fc3191fc3 100644 --- a/packages/auth/src/Domain/UseCase/ChangeCredentials/ChangeCredentialsDTO.ts +++ b/packages/auth/src/Domain/UseCase/ChangeCredentials/ChangeCredentialsDTO.ts @@ -1,7 +1,7 @@ -import { User } from '../../User/User' +import { Username } from '@standardnotes/domain-core' export type ChangeCredentialsDTO = { - user: User + username: Username apiVersion: string currentPassword: string newPassword: string diff --git a/packages/auth/src/Infra/InversifyExpressUtils/InversifyExpressUsersController.spec.ts b/packages/auth/src/Infra/InversifyExpressUtils/InversifyExpressUsersController.spec.ts index 0c9d4e9d8..7cb985758 100644 --- a/packages/auth/src/Infra/InversifyExpressUtils/InversifyExpressUsersController.spec.ts +++ b/packages/auth/src/Infra/InversifyExpressUtils/InversifyExpressUsersController.spec.ts @@ -4,7 +4,7 @@ import * as express from 'express' import { InversifyExpressUsersController } from './InversifyExpressUsersController' import { results } from 'inversify-express-utils' -import { ControllerContainerInterface } from '@standardnotes/domain-core' +import { ControllerContainerInterface, Username } from '@standardnotes/domain-core' import { DeleteAccount } from '../../Domain/UseCase/DeleteAccount/DeleteAccount' import { ChangeCredentials } from '../../Domain/UseCase/ChangeCredentials/ChangeCredentials' import { ClearLoginAttempts } from '../../Domain/UseCase/ClearLoginAttempts' @@ -321,10 +321,7 @@ describe('InversifyExpressUsersController', () => { kpOrigination: 'change-password', pwNonce: 'asdzxc', protocolVersion: '004', - user: { - uuid: '123', - email: 'test@test.te', - }, + username: Username.create('test@test.te').getValue(), }) expect(clearLoginAttempts.execute).toHaveBeenCalled() diff --git a/packages/auth/src/Infra/InversifyExpressUtils/InversifyExpressUsersController.ts b/packages/auth/src/Infra/InversifyExpressUtils/InversifyExpressUsersController.ts index d1546491f..18da98acb 100644 --- a/packages/auth/src/Infra/InversifyExpressUtils/InversifyExpressUsersController.ts +++ b/packages/auth/src/Infra/InversifyExpressUtils/InversifyExpressUsersController.ts @@ -19,7 +19,7 @@ import { GetUserSubscription } from '../../Domain/UseCase/GetUserSubscription/Ge import { ClearLoginAttempts } from '../../Domain/UseCase/ClearLoginAttempts' import { IncreaseLoginAttempts } from '../../Domain/UseCase/IncreaseLoginAttempts' import { ChangeCredentials } from '../../Domain/UseCase/ChangeCredentials/ChangeCredentials' -import { ControllerContainerInterface } from '@standardnotes/domain-core' +import { ControllerContainerInterface, Username } from '@standardnotes/domain-core' @controller('/users') export class InversifyExpressUsersController extends BaseHttpController { @@ -203,9 +203,21 @@ export class InversifyExpressUsersController extends BaseHttpController { 400, ) } + const usernameOrError = Username.create(response.locals.user.email) + if (usernameOrError.isFailed()) { + return this.json( + { + error: { + message: 'Invalid username.', + }, + }, + 400, + ) + } + const username = usernameOrError.getValue() const changeCredentialsResult = await this.changeCredentialsUseCase.execute({ - user: response.locals.user, + username, apiVersion: request.body.api, currentPassword: request.body.current_password, newPassword: request.body.new_password,