Add email validation in the API when creating new users (#350)

* Refactor user.service - add user-repository

* Add email validation for creating users

server/apps/immich/src/api-v1/auth/dto/sign-up.dto.spec.ts

@@ -0,0 +1,27 @@
+import { plainToInstance } from 'class-transformer';
+import { validate } from 'class-validator';
+import { SignUpDto } from './sign-up.dto';
+
+describe('sign up DTO', () => {
+  it('validates the email', async () => {
+    const params: Partial<SignUpDto> = {
+      email: undefined,
+      password: 'password',
+      firstName: 'first name',
+      lastName: 'last name',
+    };
+    let dto: SignUpDto = plainToInstance(SignUpDto, params);
+    let errors = await validate(dto);
+    expect(errors).toHaveLength(1);
+
+    params.email = 'invalid email';
+    dto = plainToInstance(SignUpDto, params);
+    errors = await validate(dto);
+    expect(errors).toHaveLength(1);
+
+    params.email = 'valid@email.com';
+    dto = plainToInstance(SignUpDto, params);
+    errors = await validate(dto);
+    expect(errors).toHaveLength(0);
+  });
+});

server/apps/immich/src/api-v1/auth/dto/sign-up.dto.ts

@@ -1,8 +1,8 @@
 import { ApiProperty } from '@nestjs/swagger';
-import { IsNotEmpty } from 'class-validator';
+import { IsNotEmpty, IsEmail } from 'class-validator';
 
 export class SignUpDto {
-  @IsNotEmpty()
+  @IsEmail()
   @ApiProperty({ example: 'testuser@email.com' })
   email!: string;
 

server/apps/immich/src/api-v1/user/dto/create-user.dto.spec.ts

@@ -0,0 +1,27 @@
+import { plainToInstance } from 'class-transformer';
+import { validate } from 'class-validator';
+import { CreateUserDto } from './create-user.dto';
+
+describe('create user DTO', () => {
+  it('validates the email', async() => {
+    const params: Partial<CreateUserDto> = {
+      email: undefined,
+      password: 'password',
+      firstName: 'first name',
+      lastName: 'last name',
+    }
+    let dto: CreateUserDto = plainToInstance(CreateUserDto, params);
+    let errors = await validate(dto);
+    expect(errors).toHaveLength(1);
+
+    params.email = 'invalid email';
+    dto = plainToInstance(CreateUserDto, params);
+    errors = await validate(dto);
+    expect(errors).toHaveLength(1);
+
+    params.email = 'valid@email.com';
+    dto = plainToInstance(CreateUserDto, params);
+    errors = await validate(dto);
+    expect(errors).toHaveLength(0);
+  });
+});

server/apps/immich/src/api-v1/user/dto/create-user.dto.ts

@@ -1,8 +1,8 @@
 import { ApiProperty } from '@nestjs/swagger';
-import { IsNotEmpty, IsOptional } from 'class-validator';
+import { IsNotEmpty, IsEmail } from 'class-validator';
 
 export class CreateUserDto {
-  @IsNotEmpty()
+  @IsEmail()
   @ApiProperty({ example: 'testuser@email.com' })
   email!: string;
 

server/apps/immich/src/api-v1/user/user-repository.ts

@@ -0,0 +1,95 @@
+import { UserEntity } from '@app/database/entities/user.entity';
+import { BadRequestException } from '@nestjs/common';
+import { InjectRepository } from '@nestjs/typeorm';
+import { Not, Repository } from 'typeorm';
+import { CreateUserDto } from './dto/create-user.dto';
+import * as bcrypt from 'bcrypt';
+import { UpdateUserDto } from './dto/update-user.dto'
+
+export interface IUserRepository {
+  get(userId: string): Promise<UserEntity | null>;
+  getByEmail(email: string): Promise<UserEntity | null>;
+  getList(filter?: { excludeId?: string }): Promise<UserEntity[]>;
+  create(createUserDto: CreateUserDto): Promise<UserEntity>;
+  update(user: UserEntity, updateUserDto: UpdateUserDto): Promise<UserEntity>;
+  createProfileImage(user: UserEntity, fileInfo: Express.Multer.File): Promise<UserEntity>;
+}
+
+export const USER_REPOSITORY = 'USER_REPOSITORY';
+
+export class UserRepository implements IUserRepository {
+  constructor(
+    @InjectRepository(UserEntity)
+    private userRepository: Repository<UserEntity>,
+  ) {}
+
+  private async hashPassword(password: string, salt: string): Promise<string> {
+    return bcrypt.hash(password, salt);
+  }
+
+  async get(userId: string): Promise<UserEntity | null> {
+    return this.userRepository.findOne({ where: { id: userId } });
+  }
+
+  async getByEmail(email: string): Promise<UserEntity | null> {
+    return this.userRepository.findOne({ where: { email } });
+  }
+
+  // TODO add DTO for filtering
+  async getList({ excludeId }: { excludeId?: string } = {}): Promise<UserEntity[]> {
+    if (!excludeId) {
+      return this.userRepository.find(); // TODO: this should also be ordered the same as below
+    }
+
+    return this.userRepository.find({
+      where: { id: Not(excludeId) },
+      order: {
+        createdAt: 'DESC',
+      },
+    });
+  }
+
+  async create(createUserDto: CreateUserDto): Promise<UserEntity> {
+    const newUser = new UserEntity();
+    newUser.email = createUserDto.email;
+    newUser.salt = await bcrypt.genSalt();
+    newUser.password = await this.hashPassword(createUserDto.password, newUser.salt);
+    newUser.firstName = createUserDto.firstName;
+    newUser.lastName = createUserDto.lastName;
+    newUser.isAdmin = false;
+
+    return this.userRepository.save(newUser);
+  }
+
+  async update(user: UserEntity, updateUserDto: UpdateUserDto): Promise<UserEntity> {
+    user.lastName = updateUserDto.lastName || user.lastName;
+    user.firstName = updateUserDto.firstName || user.firstName;
+    user.profileImagePath = updateUserDto.profileImagePath || user.profileImagePath;
+    user.shouldChangePassword =
+      updateUserDto.shouldChangePassword != undefined ? updateUserDto.shouldChangePassword : user.shouldChangePassword;
+
+    // If payload includes password - Create new password for user
+    if (updateUserDto.password) {
+      user.salt = await bcrypt.genSalt();
+      user.password = await this.hashPassword(updateUserDto.password, user.salt);
+    }
+
+    // TODO: can this happen? If so we can move it to the service, otherwise remove it (also from DTO)
+    if (updateUserDto.isAdmin) {
+      const adminUser = await this.userRepository.findOne({ where: { isAdmin: true } });
+
+      if (adminUser) {
+        throw new BadRequestException('Admin user exists');
+      }
+
+      user.isAdmin = true;
+    }
+
+    return this.userRepository.save(user);
+  }
+
+  async createProfileImage(user: UserEntity, fileInfo: Express.Multer.File): Promise<UserEntity> {
+    user.profileImagePath = fileInfo.path;
+    return this.userRepository.save(user);
+  }
+}

server/apps/immich/src/api-v1/user/user.module.ts

@@ -7,10 +7,18 @@ import { ImmichJwtModule } from '../../modules/immich-jwt/immich-jwt.module';
 import { ImmichJwtService } from '../../modules/immich-jwt/immich-jwt.service';
 import { JwtModule } from '@nestjs/jwt';
 import { jwtConfig } from '../../config/jwt.config';
+import { UserRepository, USER_REPOSITORY } from './user-repository';
 
 @Module({
   imports: [TypeOrmModule.forFeature([UserEntity]), ImmichJwtModule, JwtModule.register(jwtConfig)],
   controllers: [UserController],
-  providers: [UserService, ImmichJwtService],
+  providers: [
+    UserService,
+    ImmichJwtService,
+    {
+      provide: USER_REPOSITORY,
+      useClass: UserRepository
+    }
+  ],
 })
 export class UserModule {}

server/apps/immich/src/api-v1/user/user.service.ts

@@ -1,18 +1,15 @@
 import {
   BadRequestException,
+  Inject,
   Injectable,
   InternalServerErrorException,
   Logger,
   NotFoundException,
   StreamableFile,
 } from '@nestjs/common';
-import { InjectRepository } from '@nestjs/typeorm';
-import { Not, Repository } from 'typeorm';
 import { AuthUserDto } from '../../decorators/auth-user.decorator';
 import { CreateUserDto } from './dto/create-user.dto';
 import { UpdateUserDto } from './dto/update-user.dto';
-import { UserEntity } from '@app/database/entities/user.entity';
-import * as bcrypt from 'bcrypt';
 import { createReadStream } from 'fs';
 import { Response as Res } from 'express';
 import { mapUser, UserResponseDto } from './response-dto/user-response.dto';
@@ -21,32 +18,28 @@ import {
   CreateProfileImageResponseDto,
   mapCreateProfileImageResponse,
 } from './response-dto/create-profile-image-response.dto';
+import { IUserRepository, USER_REPOSITORY } from './user-repository';
 
 @Injectable()
 export class UserService {
   constructor(
-    @InjectRepository(UserEntity)
+    @Inject(USER_REPOSITORY)
-    private userRepository: Repository<UserEntity>,
+    private userRepository: IUserRepository,
   ) {}
 
   async getAllUsers(authUser: AuthUserDto, isAll: boolean): Promise<UserResponseDto[]> {
     if (isAll) {
-      const allUsers = await this.userRepository.find();
+      const allUsers = await this.userRepository.getList();
       return allUsers.map(mapUser);
     }
 
-    const allUserExceptRequestedUser = await this.userRepository.find({
+    const allUserExceptRequestedUser = await this.userRepository.getList({ excludeId: authUser.id });
-      where: { id: Not(authUser.id) },
-      order: {
-        createdAt: 'DESC',
-      },
-    });
 
     return allUserExceptRequestedUser.map(mapUser);
   }
 
   async getUserInfo(authUser: AuthUserDto): Promise<UserResponseDto> {
-    const user = await this.userRepository.findOne({ where: { id: authUser.id } });
+    const user = await this.userRepository.get(authUser.id);
     if (!user) {
       throw new BadRequestException('User not found');
     }
@@ -54,28 +47,20 @@ export class UserService {
   }
 
   async getUserCount(): Promise<UserCountResponseDto> {
-    const users = await this.userRepository.find();
+    const users = await this.userRepository.getList();
 
     return mapUserCountResponse(users.length);
   }
 
   async createUser(createUserDto: CreateUserDto): Promise<UserResponseDto> {
-    const user = await this.userRepository.findOne({ where: { email: createUserDto.email } });
+    const user = await this.userRepository.getByEmail(createUserDto.email);
 
     if (user) {
       throw new BadRequestException('User exists');
     }
 
-    const newUser = new UserEntity();
-    newUser.email = createUserDto.email;
-    newUser.salt = await bcrypt.genSalt();
-    newUser.password = await this.hashPassword(createUserDto.password, newUser.salt);
-    newUser.firstName = createUserDto.firstName;
-    newUser.lastName = createUserDto.lastName;
-    newUser.isAdmin = false;
-
     try {
-      const savedUser = await this.userRepository.save(newUser);
+      const savedUser = await this.userRepository.create(createUserDto);
 
       return mapUser(savedUser);
     } catch (e) {
@@ -84,40 +69,13 @@ export class UserService {
     }
   }
 
-  private async hashPassword(password: string, salt: string): Promise<string> {
-    return bcrypt.hash(password, salt);
-  }
-
   async updateUser(updateUserDto: UpdateUserDto): Promise<UserResponseDto> {
-    const user = await this.userRepository.findOne({ where: { id: updateUserDto.id } });
+    const user = await this.userRepository.get(updateUserDto.id);
     if (!user) {
       throw new NotFoundException('User not found');
     }
-
-    user.lastName = updateUserDto.lastName || user.lastName;
-    user.firstName = updateUserDto.firstName || user.firstName;
-    user.profileImagePath = updateUserDto.profileImagePath || user.profileImagePath;
-    user.shouldChangePassword =
-      updateUserDto.shouldChangePassword != undefined ? updateUserDto.shouldChangePassword : user.shouldChangePassword;
-
-    // If payload includes password - Create new password for user
-    if (updateUserDto.password) {
-      user.salt = await bcrypt.genSalt();
-      user.password = await this.hashPassword(updateUserDto.password, user.salt);
-    }
-
-    if (updateUserDto.isAdmin) {
-      const adminUser = await this.userRepository.findOne({ where: { isAdmin: true } });
-
-      if (adminUser) {
-        throw new BadRequestException('Admin user exists');
-      }
-
-      user.isAdmin = true;
-    }
-
     try {
-      const updatedUser = await this.userRepository.save(user);
+      const updatedUser = await this.userRepository.update(user, updateUserDto);
 
       return mapUser(updatedUser);
     } catch (e) {
@@ -130,10 +88,13 @@ export class UserService {
     authUser: AuthUserDto,
     fileInfo: Express.Multer.File,
   ): Promise<CreateProfileImageResponseDto> {
+    const user = await this.userRepository.get(authUser.id);
+    if (!user) {
+      throw new NotFoundException('User not found');
+    }
+
     try {
-      await this.userRepository.update(authUser.id, {
+      await this.userRepository.createProfileImage(user, fileInfo)
-        profileImagePath: fileInfo.path,
-      });
 
       return mapCreateProfileImageResponse(authUser.id, fileInfo.path);
     } catch (e) {
@@ -144,7 +105,7 @@ export class UserService {
 
   async getUserProfileImage(userId: string, res: Res) {
     try {
-      const user = await this.userRepository.findOne({ where: { id: userId } });
+      const user = await this.userRepository.get(userId);
       if (!user) {
         throw new NotFoundException('User not found');
       }