From 1887b5a860adeb769fefb82a23f3444c2cb8bb0f Mon Sep 17 00:00:00 2001 From: Jaime Baez Date: Fri, 15 Jul 2022 21:30:56 +0200 Subject: [PATCH] Add email validation in the API when creating new users (#350) * Refactor user.service - add user-repository * Add email validation for creating users --- .../src/api-v1/auth/dto/sign-up.dto.spec.ts | 27 ++++++ .../immich/src/api-v1/auth/dto/sign-up.dto.ts | 4 +- .../api-v1/user/dto/create-user.dto.spec.ts | 27 ++++++ .../src/api-v1/user/dto/create-user.dto.ts | 4 +- .../immich/src/api-v1/user/user-repository.ts | 95 +++++++++++++++++++ .../immich/src/api-v1/user/user.module.ts | 10 +- .../immich/src/api-v1/user/user.service.ts | 77 ++++----------- 7 files changed, 181 insertions(+), 63 deletions(-) create mode 100644 server/apps/immich/src/api-v1/auth/dto/sign-up.dto.spec.ts create mode 100644 server/apps/immich/src/api-v1/user/dto/create-user.dto.spec.ts create mode 100644 server/apps/immich/src/api-v1/user/user-repository.ts diff --git a/server/apps/immich/src/api-v1/auth/dto/sign-up.dto.spec.ts b/server/apps/immich/src/api-v1/auth/dto/sign-up.dto.spec.ts new file mode 100644 index 0000000000..5f93fa0a19 --- /dev/null +++ b/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 = { + 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); + }); +}); diff --git a/server/apps/immich/src/api-v1/auth/dto/sign-up.dto.ts b/server/apps/immich/src/api-v1/auth/dto/sign-up.dto.ts index bbeef17b96..d8dbd8a8df 100644 --- a/server/apps/immich/src/api-v1/auth/dto/sign-up.dto.ts +++ b/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; diff --git a/server/apps/immich/src/api-v1/user/dto/create-user.dto.spec.ts b/server/apps/immich/src/api-v1/user/dto/create-user.dto.spec.ts new file mode 100644 index 0000000000..c71609b083 --- /dev/null +++ b/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 = { + 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); + }); +}); diff --git a/server/apps/immich/src/api-v1/user/dto/create-user.dto.ts b/server/apps/immich/src/api-v1/user/dto/create-user.dto.ts index 66f311dc20..13facfad34 100644 --- a/server/apps/immich/src/api-v1/user/dto/create-user.dto.ts +++ b/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; diff --git a/server/apps/immich/src/api-v1/user/user-repository.ts b/server/apps/immich/src/api-v1/user/user-repository.ts new file mode 100644 index 0000000000..4c5401bd52 --- /dev/null +++ b/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; + getByEmail(email: string): Promise; + getList(filter?: { excludeId?: string }): Promise; + create(createUserDto: CreateUserDto): Promise; + update(user: UserEntity, updateUserDto: UpdateUserDto): Promise; + createProfileImage(user: UserEntity, fileInfo: Express.Multer.File): Promise; +} + +export const USER_REPOSITORY = 'USER_REPOSITORY'; + +export class UserRepository implements IUserRepository { + constructor( + @InjectRepository(UserEntity) + private userRepository: Repository, + ) {} + + private async hashPassword(password: string, salt: string): Promise { + return bcrypt.hash(password, salt); + } + + async get(userId: string): Promise { + return this.userRepository.findOne({ where: { id: userId } }); + } + + async getByEmail(email: string): Promise { + return this.userRepository.findOne({ where: { email } }); + } + + // TODO add DTO for filtering + async getList({ excludeId }: { excludeId?: string } = {}): Promise { + 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 { + 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 { + 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 { + user.profileImagePath = fileInfo.path; + return this.userRepository.save(user); + } +} \ No newline at end of file diff --git a/server/apps/immich/src/api-v1/user/user.module.ts b/server/apps/immich/src/api-v1/user/user.module.ts index 021db43339..cfa92cd73d 100644 --- a/server/apps/immich/src/api-v1/user/user.module.ts +++ b/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 {} 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 ac5c3f295d..36d995d830 100644 --- a/server/apps/immich/src/api-v1/user/user.service.ts +++ b/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) - private userRepository: Repository, + @Inject(USER_REPOSITORY) + private userRepository: IUserRepository, ) {} async getAllUsers(authUser: AuthUserDto, isAll: boolean): Promise { if (isAll) { - const allUsers = await this.userRepository.find(); + const allUsers = await this.userRepository.getList(); return allUsers.map(mapUser); } - const allUserExceptRequestedUser = await this.userRepository.find({ - where: { id: Not(authUser.id) }, - order: { - createdAt: 'DESC', - }, - }); + const allUserExceptRequestedUser = await this.userRepository.getList({ excludeId: authUser.id }); return allUserExceptRequestedUser.map(mapUser); } async getUserInfo(authUser: AuthUserDto): Promise { - 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 { - const users = await this.userRepository.find(); + const users = await this.userRepository.getList(); return mapUserCountResponse(users.length); } async createUser(createUserDto: CreateUserDto): Promise { - 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 { - return bcrypt.hash(password, salt); - } - async updateUser(updateUserDto: UpdateUserDto): Promise { - 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 { + const user = await this.userRepository.get(authUser.id); + if (!user) { + throw new NotFoundException('User not found'); + } + try { - await this.userRepository.update(authUser.id, { - profileImagePath: fileInfo.path, - }); + await this.userRepository.createProfileImage(user, fileInfo) 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'); }