[Fix][Refactor][Test] Login password validation

Added proper password validation on login.
Fixed the cardinal sin of saving a password in plain-text.
Refactored AuthService.findUser to be easier to use.
Session serializer now stores only user ids in the cookie.
Adapted UserService unit tests to the changes.
Resolves #17.

server/package.json

@@ -52,6 +52,7 @@
     "@nestjs/cli": "^8.0.0",
     "@nestjs/schematics": "^8.0.0",
     "@nestjs/testing": "^8.0.0",
+    "@types/bcrypt": "^5.0.0",
     "@types/express": "^4.17.13",
     "@types/jest": "^27.0.1",
     "@types/node": "^16.0.0",

server/src/auth/controllers/auth.controller.ts

@@ -52,7 +52,7 @@ export class AuthController {
   @ApiConflictResponse({ description: 'User already exists.' })
   async getLocalRegister(@Body() user: RegisterModel) {
     try {
-      return { id: (await this.authService.createUser(user, 'local')).id };
+      return { id: (await this.authService.createUser(user)).id };
     } catch (ex) {
       if (ex instanceof AuthError && ex.message === 'User already exists.') {
         throw new ConflictException(ex.message);

server/src/auth/services/auth.service.ts

@@ -3,50 +3,68 @@ import { IUser } from 'src/users/entities/user.entity';
 import { Injectable } from '@nestjs/common';
 import { UsersService } from 'src/users/services/users.service';
 
-export type AccountType = 'local' | 'google' | 'discord' | 'github';
-
 @Injectable()
 export class AuthService {
   constructor(private readonly usersService: UsersService) {}
 
   async validateUser(userDetails: IUser) {
-    let accountType: AccountType;
-    userDetails.username && userDetails.password && (accountType = 'local');
-    userDetails.googleId && (accountType = 'google');
-    userDetails.googleId && (accountType = 'discord');
-    userDetails.googleId && (accountType = 'github');
+    const user = await this.findUser(userDetails);
+    const isLocalUser = userDetails.username && userDetails.password;
 
-    const user = await this.findUser(userDetails, accountType);
     if (!user) {
-      if (accountType === 'local') {
-        throw new AuthError('User does not exist.');
+      if (isLocalUser) {
+        throw new AuthError('Invalid login credentials.');
       }
-      return this.createUser(userDetails, accountType);
+      return await this.createUser(userDetails);
+    }
+
+    if (
+      isLocalUser &&
+      !(await this.usersService.verifyPassword(
+        userDetails.username,
+        userDetails.password,
+      ))
+    ) {
+      throw new AuthError('Invalid login credentials.');
     }
 
     return user;
   }
 
-  async createUser(userDetails: IUser, accountType: AccountType) {
-    if (await this.findUser(userDetails, accountType)) {
+  async createUser(userDetails: IUser) {
+    const { username, email } = userDetails;
+    if (
+      (await this.findUser({ username })) ||
+      (await this.findUser({ email }))
+    ) {
       throw new AuthError('User already exists.');
     }
     return await this.usersService.createUser(userDetails);
   }
 
-  async findUser(userDetails: IUser, accountType: AccountType) {
-    switch (accountType) {
-      case 'local':
-        return (
-          (await this.usersService.findByUsername(userDetails.username)) ??
-          (await this.usersService.findByEmail(userDetails.email))
-        );
-      case 'google':
-        return await this.usersService.findByGoogleId(userDetails.googleId);
-      case 'discord':
-        return await this.usersService.findByDiscordId(userDetails.discordId);
-      case 'github':
-        return await this.usersService.findByGithubId(userDetails.githubId);
+  async findUser(userDetails: IUser) {
+    if (userDetails.id) {
+      return await this.usersService.findById(userDetails.id);
+    }
+
+    if (userDetails.email) {
+      return await this.usersService.findByEmail(userDetails.email);
+    }
+
+    if (userDetails.googleId) {
+      return await this.usersService.findByGoogleId(userDetails.googleId);
+    }
+
+    if (userDetails.discordId) {
+      return await this.usersService.findByDiscordId(userDetails.discordId);
+    }
+
+    if (userDetails.githubId) {
+      return await this.usersService.findByGithubId(userDetails.githubId);
+    }
+
+    if (userDetails.username) {
+      return await this.usersService.findByUsername(userDetails.username);
     }
   }
 }

server/src/auth/strategies/auth.local.strategy.ts

@@ -1,17 +1,25 @@
-import { Injectable } from '@nestjs/common';
+import { Injectable, UnauthorizedException } from '@nestjs/common';
+
+import AuthError from '../errors/auth.error';
+import { AuthService } from '../services/auth.service';
 import { PassportStrategy } from '@nestjs/passport';
 import { Strategy } from 'passport-local';
-import { AuthService } from '../services/auth.service';
 
 @Injectable()
 export class AuthLocalStrategy extends PassportStrategy(Strategy, 'local') {
   constructor(private readonly authService: AuthService) {
     super();
   }
 
-  validate(username: string, password: string) {
+  async validate(username: string, password: string) {
     const userDetails = { username, password };
-    const user = this.authService.validateUser(userDetails);
-    return user;
+    try {
+      const user = await this.authService.validateUser(userDetails);
+      return user;
+    } catch (ex) {
+      if (ex instanceof AuthError) {
+        throw new UnauthorizedException(ex.message);
+      }
+    }
   }
 }

server/src/auth/utils/auth.serializer.util.ts

@@ -1,22 +1,22 @@
 import { AuthService } from '../services/auth.service';
+import { IUser } from 'src/users/entities/user.entity';
 import { Injectable } from '@nestjs/common';
 import { PassportSerializer } from '@nestjs/passport';
-import { User } from 'src/users/entities/user.entity';
 
-type Done = (err: Error, user: User) => void;
+type Done = (err: Error, user: IUser) => void;
 
 @Injectable()
 export class SessionSerializerUtil extends PassportSerializer {
   constructor(private readonly authService: AuthService) {
     super();
   }
 
-  serializeUser(user: User, done: Done) {
-    done(null, user);
+  serializeUser(user: IUser, done: Done) {
+    done(null, { id: user.id });
   }
 
-  async deserializeUser(user: User, done: Done) {
-    const userFromDB = await this.authService.validateUser(user);
+  async deserializeUser(user: IUser, done: Done) {
+    const userFromDB = await this.authService.findUser(user);
     return userFromDB ? done(null, userFromDB) : done(null, null);
   }
 }

server/src/users/entities/user.entity.ts

@@ -41,7 +41,7 @@ export class User implements IUser {
 
 export interface IUser {
   id?: string;
-  username: string;
+  username?: string;
   password?: string;
   email?: string;
   phoneNumber?: string;

server/src/users/services/users.service.spec.ts

@@ -1,3 +1,5 @@
+import * as bcrypt from 'bcrypt';
+
 import { IUser, User } from '../entities/user.entity';
 import { Test, TestingModule } from '@nestjs/testing';
 
@@ -10,15 +12,23 @@ describe('UsersService', () => {
 
   const mockUser: IUser = {
     id: uuid(),
+    password: 'correct_password',
     username: 'gosho',
     email: '[email protected]',
     googleId: '133773578008135',
     discordId: '8008135420889696',
     githubId: '42080085',
   };
 
+  const hashedPassword = bcrypt.hashSync(mockUser.password, 10);
+
   const mockUsersRepository = {
-    findOne: jest.fn().mockImplementation(() => Promise.resolve(mockUser)),
+    findOne: jest.fn().mockImplementation(() =>
+      Promise.resolve({
+        ...mockUser,
+        password: hashedPassword,
+      }),
+    ),
     save: jest
       .fn()
       .mockImplementation((entity: IUser) =>
@@ -41,31 +51,73 @@ describe('UsersService', () => {
     expect(service).toBeDefined();
   });
 
+  it('findById should search for a user by id and return it', async () => {
+    expect(await service.findByUsername(mockUser.username)).toEqual({
+      ...mockUser,
+      password: hashedPassword,
+    });
+  });
+
   it('findByUsername should search for a user by username and return it', async () => {
-    expect(await service.findByUsername(mockUser.username)).toEqual(mockUser);
+    expect(await service.findByUsername(mockUser.username)).toEqual({
+      ...mockUser,
+      password: hashedPassword,
+    });
   });
 
   it('findByEmail should search for a user by email and return it', async () => {
-    expect(await service.findByEmail(mockUser.email)).toEqual(mockUser);
+    expect(await service.findByEmail(mockUser.email)).toEqual({
+      ...mockUser,
+      password: hashedPassword,
+    });
   });
 
   it('findByGoogleId should search for a user by a google id and return it', async () => {
-    expect(await service.findByGoogleId(mockUser.googleId)).toEqual(mockUser);
+    expect(await service.findByGoogleId(mockUser.googleId)).toEqual({
+      ...mockUser,
+      password: hashedPassword,
+    });
   });
 
   it('findByDiscordId should search for a user by a discord id and return it', async () => {
-    expect(await service.findByDiscordId(mockUser.discordId)).toEqual(mockUser);
+    expect(await service.findByDiscordId(mockUser.discordId)).toEqual({
+      ...mockUser,
+      password: hashedPassword,
+    });
   });
 
   it('findByGithubId should search for a user by github id and return it', async () => {
-    expect(await service.findByGithubId(mockUser.githubId)).toEqual(mockUser);
+    expect(await service.findByGithubId(mockUser.githubId)).toEqual({
+      ...mockUser,
+      password: hashedPassword,
+    });
   });
 
-  it('createUser should create new users and return it', async () => {
-    expect(await service.createUser({ username: 'gosho' })).toEqual({
-      id: expect.any(String),
-      username: 'gosho',
+  it('createUser should create a new user, hash the password and return it', async () => {
+    const user = await service.createUser({
+      username: mockUser.username,
+      password: mockUser.password,
     });
+
+    expect(user.id).toEqual(expect.any(String));
+    expect(user.username).toEqual(mockUser.username);
+    expect(user.password).toEqual(expect.any(String));
+    expect(user.password).not.toBe(mockUser.password);
+
     expect(mockUsersRepository.save).toBeCalledTimes(1);
   });
+
+  it('verifyPassword should return false on wrong password', async () => {
+    const wrongPassword = 'wrong_password';
+
+    expect(
+      await service.verifyPassword(mockUser.username, wrongPassword),
+    ).toEqual(false);
+  });
+
+  it('verifyPassword should return true on correct password', async () => {
+    expect(
+      await service.verifyPassword(mockUser.username, mockUser.password),
+    ).toEqual(true);
+  });
 });

server/src/users/services/users.service.ts

@@ -2,13 +2,18 @@ import { Injectable } from '@nestjs/common';
 import { InjectRepository } from '@nestjs/typeorm';
 import { Repository } from 'typeorm';
 import { IUser, User } from '../entities/user.entity';
+import * as bcrypt from 'bcrypt';
 
 @Injectable()
 export class UsersService {
   constructor(
     @InjectRepository(User) private userRepository: Repository<User>,
   ) {}
 
+  async findById(id: string) {
+    return await this.userRepository.findOne({ id });
+  }
+
   async findByUsername(username: string) {
     return await this.userRepository.findOne({ username });
   }
@@ -30,6 +35,13 @@ export class UsersService {
   }
 
   async createUser(userDetails: IUser) {
-    return await this.userRepository.save(userDetails);
+    const salt = await bcrypt.genSalt(10);
+    const hash = await bcrypt.hash(userDetails.password, salt);
+    return await this.userRepository.save({ ...userDetails, password: hash });
+  }
+
+  async verifyPassword(username: string, password: string) {
+    const userFromDatabase = await this.findByUsername(username);
+    return bcrypt.compare(password, userFromDatabase.password);
   }
 }