From ece94f6bdcfc7377b5db52be6bcb2700bbdd0e77 Mon Sep 17 00:00:00 2001 From: Alex Date: Sun, 18 Sep 2022 09:27:06 -0500 Subject: [PATCH] fix(server): correct user permission to update user info (#716) --- .../immich/src/api-v1/user/user.controller.ts | 7 +- .../src/api-v1/user/user.service.spec.ts | 137 ++++++++++++++++++ .../immich/src/api-v1/user/user.service.ts | 18 ++- 3 files changed, 157 insertions(+), 5 deletions(-) create mode 100644 server/apps/immich/src/api-v1/user/user.service.spec.ts diff --git a/server/apps/immich/src/api-v1/user/user.controller.ts b/server/apps/immich/src/api-v1/user/user.controller.ts index 6dc42a1276..4ed0b1dd49 100644 --- a/server/apps/immich/src/api-v1/user/user.controller.ts +++ b/server/apps/immich/src/api-v1/user/user.controller.ts @@ -74,8 +74,11 @@ export class UserController { @UseGuards(JwtAuthGuard) @ApiBearerAuth() @Put() - async updateUser(@Body(ValidationPipe) updateUserDto: UpdateUserDto): Promise { - return await this.userService.updateUser(updateUserDto); + async updateUser( + @GetAuthUser() authUser: AuthUserDto, + @Body(ValidationPipe) updateUserDto: UpdateUserDto, + ): Promise { + return await this.userService.updateUser(authUser, updateUserDto); } @UseInterceptors(FileInterceptor('file', profileImageUploadOption)) diff --git a/server/apps/immich/src/api-v1/user/user.service.spec.ts b/server/apps/immich/src/api-v1/user/user.service.spec.ts new file mode 100644 index 0000000000..f60856dbe1 --- /dev/null +++ b/server/apps/immich/src/api-v1/user/user.service.spec.ts @@ -0,0 +1,137 @@ +import { UserEntity } from '@app/database/entities/user.entity'; +import { BadRequestException, NotFoundException } from '@nestjs/common'; +import { AuthUserDto } from '../../decorators/auth-user.decorator'; +import { IUserRepository } from './user-repository'; +import { UserService } from './user.service'; + +describe('UserService', () => { + let sui: UserService; + let userRepositoryMock: jest.Mocked; + + const adminAuthUser: AuthUserDto = Object.freeze({ + id: 'admin_id', + email: 'admin@test.com', + }); + + const immichAuthUser: AuthUserDto = Object.freeze({ + id: 'immich_id', + email: 'immich@test.com', + }); + + const adminUser: UserEntity = Object.freeze({ + id: 'admin_id', + email: 'admin@test.com', + password: 'admin_password', + salt: 'admin_salt', + firstName: 'admin_first_name', + lastName: 'admin_last_name', + isAdmin: true, + shouldChangePassword: false, + profileImagePath: '', + createdAt: '2021-01-01', + }); + + const immichUser: UserEntity = Object.freeze({ + id: 'immich_id', + email: 'immich@test.com', + password: 'immich_password', + salt: 'immich_salt', + firstName: 'immich_first_name', + lastName: 'immich_last_name', + isAdmin: false, + shouldChangePassword: false, + profileImagePath: '', + createdAt: '2021-01-01', + }); + + const updatedImmichUser: UserEntity = Object.freeze({ + id: 'immich_id', + email: 'immich@test.com', + password: 'immich_password', + salt: 'immich_salt', + firstName: 'updated_immich_first_name', + lastName: 'updated_immich_last_name', + isAdmin: false, + shouldChangePassword: true, + profileImagePath: '', + createdAt: '2021-01-01', + }); + + beforeAll(() => { + userRepositoryMock = { + create: jest.fn(), + createProfileImage: jest.fn(), + get: jest.fn(), + getByEmail: jest.fn(), + getList: jest.fn(), + update: jest.fn(), + }; + + sui = new UserService(userRepositoryMock); + }); + + it('should be defined', () => { + expect(sui).toBeDefined(); + }); + + describe('Update user', () => { + it('should update user', () => { + const requestor = immichAuthUser; + const userToUpdate = immichUser; + + userRepositoryMock.get.mockImplementationOnce(() => Promise.resolve(immichUser)); + userRepositoryMock.get.mockImplementationOnce(() => Promise.resolve(userToUpdate)); + userRepositoryMock.update.mockImplementationOnce(() => Promise.resolve(updatedImmichUser)); + + const result = sui.updateUser(requestor, { + id: userToUpdate.id, + shouldChangePassword: true, + }); + expect(result).resolves.toBeDefined(); + }); + + it('user can only update its information', () => { + const requestor = immichAuthUser; + + userRepositoryMock.get.mockImplementationOnce(() => Promise.resolve(immichUser)); + + const result = sui.updateUser(requestor, { + id: 'not_immich_auth_user_id', + password: 'I take over your account now', + }); + expect(result).rejects.toBeInstanceOf(BadRequestException); + }); + + it('admin can update any user information', async () => { + const requestor = adminAuthUser; + const userToUpdate = immichUser; + + userRepositoryMock.get.mockImplementationOnce(() => Promise.resolve(adminUser)); + userRepositoryMock.get.mockImplementationOnce(() => Promise.resolve(userToUpdate)); + userRepositoryMock.update.mockImplementationOnce(() => Promise.resolve(updatedImmichUser)); + + const result = await sui.updateUser(requestor, { + id: userToUpdate.id, + shouldChangePassword: true, + }); + + expect(result).toBeDefined(); + expect(result.id).toEqual(updatedImmichUser.id); + expect(result.shouldChangePassword).toEqual(updatedImmichUser.shouldChangePassword); + }); + + it('update user information should throw error if user not found', () => { + const requestor = adminAuthUser; + const userToUpdate = immichUser; + + userRepositoryMock.get.mockImplementationOnce(() => Promise.resolve(adminUser)); + userRepositoryMock.get.mockImplementationOnce(() => Promise.resolve(null)); + + const result = sui.updateUser(requestor, { + id: userToUpdate.id, + shouldChangePassword: true, + }); + expect(result).rejects.toBeInstanceOf(NotFoundException); + }); + }); +}); diff --git a/server/apps/immich/src/api-v1/user/user.service.ts b/server/apps/immich/src/api-v1/user/user.service.ts index 9a648b9964..d62d56de23 100644 --- a/server/apps/immich/src/api-v1/user/user.service.ts +++ b/server/apps/immich/src/api-v1/user/user.service.ts @@ -78,7 +78,19 @@ export class UserService { } } - async updateUser(updateUserDto: UpdateUserDto): Promise { + async updateUser(authUser: AuthUserDto, updateUserDto: UpdateUserDto): Promise { + const requestor = await this.userRepository.get(authUser.id); + + if (!requestor) { + throw new NotFoundException('Requestor not found'); + } + + if (!requestor.isAdmin) { + if (requestor.id !== updateUserDto.id) { + throw new BadRequestException('Unauthorized'); + } + } + const user = await this.userRepository.get(updateUserDto.id); if (!user) { throw new NotFoundException('User not found'); @@ -88,8 +100,8 @@ export class UserService { return mapUser(updatedUser); } catch (e) { - Logger.error(e, 'Create new user'); - throw new InternalServerErrorException('Failed to register new user'); + Logger.error(e, 'Failed to update user info'); + throw new InternalServerErrorException('Failed to update user info'); } }