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

@@ -8,14 +8,13 @@ import 'package:openapi/api.dart';
 ## Properties
 Name | Type | Description | Notes
 ------------ | ------------- | ------------- | -------------
-**id** | **String** |  | 
 **email** | **String** |  | [optional] 
 **password** | **String** |  | [optional] 
 **firstName** | **String** |  | [optional] 
 **lastName** | **String** |  | [optional] 
+**id** | **String** |  | 
 **isAdmin** | **bool** |  | [optional] 
 **shouldChangePassword** | **bool** |  | [optional] 
-**profileImagePath** | **String** |  | [optional] 
 
 [[Back to Model list]](../README.md#documentation-for-models) [[Back to API list]](../README.md#documentation-for-api-endpoints) [[Back to README]](../README.md)
 

mobile/openapi/lib/model/update_user_dto.dart

@@ -13,18 +13,15 @@ part of openapi.api;
 class UpdateUserDto {
   /// Returns a new [UpdateUserDto] instance.
   UpdateUserDto({
-    required this.id,
     this.email,
     this.password,
     this.firstName,
     this.lastName,
+    required this.id,
     this.isAdmin,
     this.shouldChangePassword,
-    this.profileImagePath,
   });
 
-  String id;
-
   ///
   /// Please note: This property should have been non-nullable! Since the specification file
   /// does not include a default value (using the "default:" property), however, the generated
@@ -57,6 +54,8 @@ class UpdateUserDto {
   ///
   String? lastName;
 
+  String id;
+
   ///
   /// Please note: This property should have been non-nullable! Since the specification file
   /// does not include a default value (using the "default:" property), however, the generated
@@ -73,43 +72,32 @@ class UpdateUserDto {
   ///
   bool? shouldChangePassword;
 
-  ///
-  /// Please note: This property should have been non-nullable! Since the specification file
-  /// does not include a default value (using the "default:" property), however, the generated
-  /// source code must fall back to having a nullable type.
-  /// Consider adding a "default:" property in the specification file to hide this note.
-  ///
-  String? profileImagePath;
-
   @override
   bool operator ==(Object other) => identical(this, other) || other is UpdateUserDto &&
-     other.id == id &&
      other.email == email &&
      other.password == password &&
      other.firstName == firstName &&
      other.lastName == lastName &&
+     other.id == id &&
      other.isAdmin == isAdmin &&
-     other.shouldChangePassword == shouldChangePassword &&
-     other.profileImagePath == profileImagePath;
+     other.shouldChangePassword == shouldChangePassword;
 
   @override
   int get hashCode =>
     // ignore: unnecessary_parenthesis
-    (id.hashCode) +
     (email == null ? 0 : email!.hashCode) +
     (password == null ? 0 : password!.hashCode) +
     (firstName == null ? 0 : firstName!.hashCode) +
     (lastName == null ? 0 : lastName!.hashCode) +
+    (id.hashCode) +
     (isAdmin == null ? 0 : isAdmin!.hashCode) +
-    (shouldChangePassword == null ? 0 : shouldChangePassword!.hashCode) +
-    (profileImagePath == null ? 0 : profileImagePath!.hashCode);
+    (shouldChangePassword == null ? 0 : shouldChangePassword!.hashCode);
 
   @override
-  String toString() => 'UpdateUserDto[id=$id, email=$email, password=$password, firstName=$firstName, lastName=$lastName, isAdmin=$isAdmin, shouldChangePassword=$shouldChangePassword, profileImagePath=$profileImagePath]';
+  String toString() => 'UpdateUserDto[email=$email, password=$password, firstName=$firstName, lastName=$lastName, id=$id, isAdmin=$isAdmin, shouldChangePassword=$shouldChangePassword]';
 
   Map<String, dynamic> toJson() {
     final json = <String, dynamic>{};
-      json[r'id'] = this.id;
     if (this.email != null) {
       json[r'email'] = this.email;
     } else {
@@ -130,6 +118,7 @@ class UpdateUserDto {
     } else {
       // json[r'lastName'] = null;
     }
+      json[r'id'] = this.id;
     if (this.isAdmin != null) {
       json[r'isAdmin'] = this.isAdmin;
     } else {
@@ -140,11 +129,6 @@ class UpdateUserDto {
     } else {
       // json[r'shouldChangePassword'] = null;
     }
-    if (this.profileImagePath != null) {
-      json[r'profileImagePath'] = this.profileImagePath;
-    } else {
-      // json[r'profileImagePath'] = null;
-    }
     return json;
   }
 
@@ -167,14 +151,13 @@ class UpdateUserDto {
       }());
 
       return UpdateUserDto(
-        id: mapValueOfType<String>(json, r'id')!,
         email: mapValueOfType<String>(json, r'email'),
         password: mapValueOfType<String>(json, r'password'),
         firstName: mapValueOfType<String>(json, r'firstName'),
         lastName: mapValueOfType<String>(json, r'lastName'),
+        id: mapValueOfType<String>(json, r'id')!,
         isAdmin: mapValueOfType<bool>(json, r'isAdmin'),
         shouldChangePassword: mapValueOfType<bool>(json, r'shouldChangePassword'),
-        profileImagePath: mapValueOfType<String>(json, r'profileImagePath'),
       );
     }
     return null;

mobile/openapi/test/update_user_dto_test.dart

@@ -16,11 +16,6 @@ void main() {
   // final instance = UpdateUserDto();
 
   group('test UpdateUserDto', () {
-    // String id
-    test('to test the property `id`', () async {
-      // TODO
-    });
-
     // String email
     test('to test the property `email`', () async {
       // TODO
@@ -41,6 +36,11 @@ void main() {
       // TODO
     });
 
+    // String id
+    test('to test the property `id`', () async {
+      // TODO
+    });
+
     // bool isAdmin
     test('to test the property `isAdmin`', () async {
       // TODO
@@ -51,11 +51,6 @@ void main() {
       // TODO
     });
 
-    // String profileImagePath
-    test('to test the property `profileImagePath`', () async {
-      // TODO
-    });
-
 
   });
 

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) {}
 

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',
+        });
+      });
     });
   });
 });

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": [

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;
 }

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<UserEntity>): Promise<UserEntity> {
-    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) {

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);
   }

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}
@@ -2324,22 +2318,22 @@ export interface UpdateUserDto {
     'lastName'?: string;
     /**
      * 
-     * @type {boolean}
+     * @type {string}
      * @memberof UpdateUserDto
      */
-    'isAdmin'?: boolean;
+    'id': string;
     /**
      * 
      * @type {boolean}
      * @memberof UpdateUserDto
      */
-    'shouldChangePassword'?: boolean;
+    'isAdmin'?: boolean;
     /**
      * 
-     * @type {string}
+     * @type {boolean}
      * @memberof UpdateUserDto
      */
-    'profileImagePath'?: string;
+    'shouldChangePassword'?: boolean;
 }
 /**
  *