From d04f340b5b61b3e64936e7a4fd3a02afe8390c0e Mon Sep 17 00:00:00 2001 From: Alex Date: Sat, 1 Apr 2023 11:43:45 -0500 Subject: [PATCH] fix(server): user update (#2143) * fix(server): user update * update dto * generate api * improve validation * add e2e tests for updating user --------- Co-authored-by: Michel Heusschen <59014050+michelheusschen@users.noreply.github.com> --- mobile/openapi/doc/UpdateUserDto.md | Bin 719 -> 668 bytes mobile/openapi/lib/model/update_user_dto.dart | Bin 8259 -> 7464 bytes mobile/openapi/test/update_user_dto_test.dart | Bin 1288 -> 1171 bytes .../immich/src/controllers/user.controller.ts | 2 +- server/apps/immich/test/user.e2e-spec.ts | 69 +++++++++++++++++- server/immich-openapi-specs.json | 22 +++--- .../domain/src/user/dto/update-user.dto.ts | 26 ++----- server/libs/domain/src/user/user.core.ts | 10 ++- server/libs/domain/src/user/user.service.ts | 1 + web/src/api/open-api/api.ts | 18 ++--- 10 files changed, 101 insertions(+), 47 deletions(-) diff --git a/mobile/openapi/doc/UpdateUserDto.md b/mobile/openapi/doc/UpdateUserDto.md index 043f2e6ab8a845f3c31fd64000494fd1dc796fa9..e40bf5eb77f2f447a57a3791f85df29d9297cf32 100644 GIT binary patch delta 21 dcmX@lI)`<_jftODPgZ0Un>>>-Zn75BQUGcZ2*v;a delta 54 zcmbQkdY*N{jmb@nQmm=DiJ3W*;~AAFF*Z-0%NQl3rBzUrpO%@E>Y1CEo*Ix?k^z)e Jocx_}DF7S26Wss+ diff --git a/mobile/openapi/lib/model/update_user_dto.dart b/mobile/openapi/lib/model/update_user_dto.dart index edc5298104ccfaa65ddb1d46d000b38b00e89d07..2d59db436e26b2671f9f756fa6a49ced19fcfd68 100644 GIT binary patch delta 132 zcmV-~0DJ$#K&U#fdjXS?0ZNm50UMLx0V)E0DYN?rVga+418D@aECOo*vt0>}0h476 zZIkH?iIdX|7L#!eO_T8rQIl2<#gju06O$Sba+A&vHIoYvO$B5S1%1_J8N%hQ4Oiv9+EXmN}Qc$SX zwB`a*!6ikRdFcw7DVr16vKTj)F(%%DDx(0p#>uC-iY6cB7N2a(-8uO# zw~w)cni|j$AjmJtNG-y!NWs=t0mBSUoHsfDgxJyyXMd+<&+5R}IVy3K4tDa@0j d#Ed3qiGG~iE7mEbfZ@i;`J#%L8hONJ*Z`J7r@H_E diff --git a/mobile/openapi/test/update_user_dto_test.dart b/mobile/openapi/test/update_user_dto_test.dart index 5055f5fa9be72823c5c0b18226367f46d6951a96..8cd9da9944016e77a73c72c459e7dcdda3e0fb8e 100644 GIT binary patch delta 28 kcmeC+n#{T3660oVrcTDm6Ij?MUt$cJyoLGd V6)^?%nANzr6clPTt+}{rxd7oQ8~XqN diff --git a/server/apps/immich/src/controllers/user.controller.ts b/server/apps/immich/src/controllers/user.controller.ts index d426dff78a..9c180d2cb3 100644 --- a/server/apps/immich/src/controllers/user.controller.ts +++ b/server/apps/immich/src/controllers/user.controller.ts @@ -32,7 +32,7 @@ import { UserCountDto } from '@app/domain'; @ApiTags('User') @Controller('user') -@UsePipes(new ValidationPipe({ transform: true })) +@UsePipes(new ValidationPipe({ transform: true, whitelist: true })) export class UserController { constructor(private service: UserService) {} diff --git a/server/apps/immich/test/user.e2e-spec.ts b/server/apps/immich/test/user.e2e-spec.ts index bc1bc11a1d..e208c21b52 100644 --- a/server/apps/immich/test/user.e2e-spec.ts +++ b/server/apps/immich/test/user.e2e-spec.ts @@ -2,7 +2,7 @@ import { Test, TestingModule } from '@nestjs/testing'; import { INestApplication } from '@nestjs/common'; import request from 'supertest'; import { clearDb, authCustom } from './test-utils'; -import { CreateUserDto, UserService, AuthUserDto } from '@app/domain'; +import { CreateUserDto, UserService, AuthUserDto, UserResponseDto } from '@app/domain'; import { DataSource } from 'typeorm'; import { AuthService } from '@app/domain'; import { AppModule } from '../src/app.module'; @@ -39,10 +39,11 @@ describe('User', () => { }); }); - describe('with auth', () => { + describe('with admin auth', () => { let userService: UserService; let authService: AuthService; let authUser: AuthUserDto; + let userOne: UserResponseDto; beforeAll(async () => { const builder = Test.createTestingModule({ imports: [AppModule] }); @@ -69,7 +70,8 @@ describe('User', () => { password: '1234', }); authUser = { ...adminSignupResponseDto, isAdmin: true }; // TODO: find out why adminSignUp doesn't have isAdmin (maybe can just return UserResponseDto) - await Promise.allSettled([ + + [userOne] = await Promise.all([ _createUser(userService, { firstName: 'one', lastName: 'test', @@ -121,6 +123,67 @@ describe('User', () => { ); expect(body).toEqual(expect.not.arrayContaining([expect.objectContaining({ email: authUserEmail })])); }); + + it('disallows admin user from creating a second admin account', async () => { + const { status } = await request(app.getHttpServer()) + .put('/user') + .send({ + ...userOne, + isAdmin: true, + }); + expect(status).toEqual(400); + }); + + it('ignores updates to createdAt, updatedAt and deletedAt', async () => { + const { status, body } = await request(app.getHttpServer()) + .put('/user') + .send({ + ...userOne, + createdAt: '2023-01-01T00:00:00.000Z', + updatedAt: '2023-01-01T00:00:00.000Z', + deletedAt: '2023-01-01T00:00:00.000Z', + }); + expect(status).toEqual(200); + expect(body).toStrictEqual({ + ...userOne, + createdAt: new Date(userOne.createdAt).toISOString(), + updatedAt: expect.anything(), + }); + }); + + it('ignores updates to profileImagePath', async () => { + const { status, body } = await request(app.getHttpServer()) + .put('/user') + .send({ + ...userOne, + profileImagePath: 'invalid.jpg', + }); + expect(status).toEqual(200); + expect(body).toStrictEqual({ + ...userOne, + createdAt: new Date(userOne.createdAt).toISOString(), + updatedAt: expect.anything(), + }); + }); + + it('allows to update first and last name', async () => { + const { status, body } = await request(app.getHttpServer()) + .put('/user') + .send({ + ...userOne, + firstName: 'newFirstName', + lastName: 'newLastName', + }); + + expect(status).toEqual(200); + expect(body).toMatchObject({ + ...userOne, + createdAt: new Date(userOne.createdAt).toISOString(), + updatedAt: expect.anything(), + firstName: 'newFirstName', + lastName: 'newLastName', + }); + }); }); }); }); diff --git a/server/immich-openapi-specs.json b/server/immich-openapi-specs.json index 1a88ff1f8b..6920d0e9ef 100644 --- a/server/immich-openapi-specs.json +++ b/server/immich-openapi-specs.json @@ -4812,29 +4812,31 @@ "UpdateUserDto": { "type": "object", "properties": { - "id": { - "type": "string" - }, "email": { - "type": "string" + "type": "string", + "example": "testuser@email.com" }, "password": { - "type": "string" + "type": "string", + "example": "password" }, "firstName": { - "type": "string" + "type": "string", + "example": "John" }, "lastName": { - "type": "string" + "type": "string", + "example": "Doe" + }, + "id": { + "type": "string", + "format": "uuid" }, "isAdmin": { "type": "boolean" }, "shouldChangePassword": { "type": "boolean" - }, - "profileImagePath": { - "type": "string" } }, "required": [ diff --git a/server/libs/domain/src/user/dto/update-user.dto.ts b/server/libs/domain/src/user/dto/update-user.dto.ts index 73bcdf199b..f404e4d16f 100644 --- a/server/libs/domain/src/user/dto/update-user.dto.ts +++ b/server/libs/domain/src/user/dto/update-user.dto.ts @@ -1,28 +1,18 @@ -import { IsEmail, IsNotEmpty, IsOptional } from 'class-validator'; +import { IsBoolean, IsNotEmpty, IsOptional, IsUUID } from 'class-validator'; +import { CreateUserDto } from './create-user.dto'; +import { ApiProperty, PartialType } from '@nestjs/swagger'; -export class UpdateUserDto { +export class UpdateUserDto extends PartialType(CreateUserDto) { @IsNotEmpty() + @IsUUID('4') + @ApiProperty({ format: 'uuid' }) id!: string; - @IsEmail() - @IsOptional() - email?: string; - - @IsOptional() - password?: string; - - @IsOptional() - firstName?: string; - - @IsOptional() - lastName?: string; - @IsOptional() + @IsBoolean() isAdmin?: boolean; @IsOptional() + @IsBoolean() shouldChangePassword?: boolean; - - @IsOptional() - profileImagePath?: string; } diff --git a/server/libs/domain/src/user/user.core.ts b/server/libs/domain/src/user/user.core.ts index 0e7dd1dc59..2e75231568 100644 --- a/server/libs/domain/src/user/user.core.ts +++ b/server/libs/domain/src/user/user.core.ts @@ -21,12 +21,16 @@ export class UserCore { constructor(private userRepository: IUserRepository, private cryptoRepository: ICryptoRepository) {} async updateUser(authUser: AuthUserDto, id: string, dto: Partial): Promise { - if (!(authUser.isAdmin || authUser.id === id)) { + if (!authUser.isAdmin && authUser.id !== id) { throw new ForbiddenException('You are not allowed to update this user'); } - if (dto.isAdmin && authUser.isAdmin && authUser.id !== id) { - throw new BadRequestException('Admin user exists'); + if (!authUser.isAdmin) { + // Users can never update the isAdmin property. + delete dto.isAdmin; + } else if (dto.isAdmin && authUser.id !== id) { + // Admin cannot create another admin. + throw new BadRequestException('The server already has an admin'); } if (dto.email) { diff --git a/server/libs/domain/src/user/user.service.ts b/server/libs/domain/src/user/user.service.ts index 4bce62ed86..305d5e283a 100644 --- a/server/libs/domain/src/user/user.service.ts +++ b/server/libs/domain/src/user/user.service.ts @@ -90,6 +90,7 @@ export class UserService { if (!user) { throw new NotFoundException('User not found'); } + const updatedUser = await this.userCore.updateUser(authUser, dto.id, dto); return mapUser(updatedUser); } diff --git a/web/src/api/open-api/api.ts b/web/src/api/open-api/api.ts index afed1691ce..4bf1cd687f 100644 --- a/web/src/api/open-api/api.ts +++ b/web/src/api/open-api/api.ts @@ -2292,12 +2292,6 @@ export interface UpdateTagDto { * @interface UpdateUserDto */ export interface UpdateUserDto { - /** - * - * @type {string} - * @memberof UpdateUserDto - */ - 'id': string; /** * * @type {string} @@ -2322,6 +2316,12 @@ export interface UpdateUserDto { * @memberof UpdateUserDto */ 'lastName'?: string; + /** + * + * @type {string} + * @memberof UpdateUserDto + */ + 'id': string; /** * * @type {boolean} @@ -2334,12 +2334,6 @@ export interface UpdateUserDto { * @memberof UpdateUserDto */ 'shouldChangePassword'?: boolean; - /** - * - * @type {string} - * @memberof UpdateUserDto - */ - 'profileImagePath'?: string; } /** *