chore: move password related functions from AuthService to utils file

As these methods will be used in both the AuthService and the IdentityService, it makes sense to extract them and use them in this manner. Especially if one considers that they are quite standalone functions.

Signed-off-by: Philip Molares <philip.molares@udo.edu>

src/auth/auth.service.spec.ts

@@ -7,16 +7,16 @@ import { ConfigModule } from '@nestjs/config';
 import { PassportModule } from '@nestjs/passport';
 import { Test, TestingModule } from '@nestjs/testing';
 import { getRepositoryToken } from '@nestjs/typeorm';
-import { randomBytes } from 'crypto';
 import { Repository } from 'typeorm';
 
 import appConfigMock from '../config/mock/app.config.mock';
 import { NotInDBError, TokenNotValidError } from '../errors/errors';
+import { Identity } from '../identity/identity.entity';
 import { LoggerModule } from '../logger/logger.module';
-import { Identity } from '../users/identity.entity';
 import { Session } from '../users/session.entity';
 import { User } from '../users/user.entity';
 import { UsersModule } from '../users/users.module';
+import { hashPassword } from '../utils/password';
 import { AuthToken } from './auth-token.entity';
 import { AuthService } from './auth.service';
 
@@ -74,26 +74,6 @@ describe('AuthService', () => {
     expect(service).toBeDefined();
   });
 
-  describe('checkPassword', () => {
-    it('works', async () => {
-      const testPassword = 'thisIsATestPassword';
-      const hash = await service.hashPassword(testPassword);
-      await service
-        .checkPassword(testPassword, hash)
-        .then((result) => expect(result).toBeTruthy());
-    });
-    it('fails, if secret is too short', async () => {
-      const secret = service.bufferToBase64Url(randomBytes(54));
-      const hash = await service.hashPassword(secret);
-      await service
-        .checkPassword(secret, hash)
-        .then((result) => expect(result).toBeTruthy());
-      await service
-        .checkPassword(secret.substr(0, secret.length - 1), hash)
-        .then((result) => expect(result).toBeFalsy());
-    });
-  });
-
   describe('getTokensByUsername', () => {
     it('works', async () => {
       jest
@@ -108,7 +88,7 @@ describe('AuthService', () => {
   describe('getAuthToken', () => {
     const token = 'testToken';
     it('works', async () => {
-      const accessTokenHash = await service.hashPassword(token);
+      const accessTokenHash = await hashPassword(token);
       jest.spyOn(authTokenRepo, 'findOne').mockResolvedValueOnce({
         ...authToken,
         user: user,
@@ -142,7 +122,7 @@ describe('AuthService', () => {
         ).rejects.toThrow(TokenNotValidError);
       });
       it('AuthToken has wrong validUntil Date', async () => {
-        const accessTokenHash = await service.hashPassword(token);
+        const accessTokenHash = await hashPassword(token);
         jest.spyOn(authTokenRepo, 'findOne').mockResolvedValueOnce({
           ...authToken,
           user: user,
@@ -185,7 +165,7 @@ describe('AuthService', () => {
   describe('validateToken', () => {
     it('works', async () => {
       const token = 'testToken';
-      const accessTokenHash = await service.hashPassword(token);
+      const accessTokenHash = await hashPassword(token);
       jest.spyOn(userRepo, 'findOne').mockResolvedValueOnce({
         ...user,
         authTokens: [authToken],
@@ -303,16 +283,6 @@ describe('AuthService', () => {
     });
   });
 
-  describe('bufferToBase64Url', () => {
-    it('works', () => {
-      expect(
-        service.bufferToBase64Url(
-          Buffer.from('testsentence is a test sentence'),
-        ),
-      ).toEqual('dGVzdHNlbnRlbmNlIGlzIGEgdGVzdCBzZW50ZW5jZQ');
-    });
-  });
-
   describe('toAuthTokenDto', () => {
     it('works', () => {
       const authToken = new AuthToken();

src/auth/auth.service.ts

@@ -6,7 +6,6 @@
 import { Injectable } from '@nestjs/common';
 import { Cron, Timeout } from '@nestjs/schedule';
 import { InjectRepository } from '@nestjs/typeorm';
-import { compare, hash } from 'bcrypt';
 import { randomBytes } from 'crypto';
 import { Repository } from 'typeorm';
 
@@ -16,8 +15,13 @@ import {
   TooManyTokensError,
 } from '../errors/errors';
 import { ConsoleLoggerService } from '../logger/console-logger.service';
+import { UserRelationEnum } from '../users/user-relation.enum';
 import { User } from '../users/user.entity';
 import { UsersService } from '../users/users.service';
+import {
+  bufferToBase64Url,
+  hashPassword,
+} from '../utils/password';
 import { TimestampMillis } from '../utils/timestamp';
 import { AuthTokenWithSecretDto } from './auth-token-with-secret.dto';
 import { AuthTokenDto } from './auth-token.dto';
@@ -52,33 +56,14 @@ export class AuthService {
     return await this.usersService.getUserByUsername(accessToken.user.userName);
   }
 
-  async hashPassword(cleartext: string): Promise<string> {
-    // hash the password with bcrypt and 2^12 iterations
-    // this was decided on the basis of https://cheatsheetseries.owasp.org/cheatsheets/Password_Storage_Cheat_Sheet.html#bcrypt
-    return await hash(cleartext, 12);
-  }
-
-  async checkPassword(cleartext: string, password: string): Promise<boolean> {
-    return await compare(cleartext, password);
-  }
-
-  bufferToBase64Url(text: Buffer): string {
-    // This is necessary as the is no base64url encoding in the toString method
-    // but as can be seen on https://tools.ietf.org/html/rfc4648#page-7
-    // base64url is quite easy buildable from base64
-    return text
-      .toString('base64')
-      .replace(/\+/g, '-')
-      .replace(/\//g, '_')
-      .replace(/=+$/, '');
-  }
-
   async createTokenForUser(
     userName: string,
     identifier: string,
     validUntil: TimestampMillis,
   ): Promise<AuthTokenWithSecretDto> {
-    const user = await this.usersService.getUserByUsername(userName, true);
+    const user = await this.usersService.getUserByUsername(userName, [
+      UserRelationEnum.AUTHTOKENS,
+    ]);
     if (user.authTokens.length >= 200) {
       // This is a very high ceiling unlikely to hinder legitimate usage,
       // but should prevent possible attack vectors
@@ -86,9 +71,9 @@ export class AuthService {
         `User '${user.userName}' has already 200 tokens and can't have anymore`,
       );
     }
-    const secret = this.bufferToBase64Url(randomBytes(54));
+    const secret = bufferToBase64Url(randomBytes(54));
-    const keyId = this.bufferToBase64Url(randomBytes(8));
+    const keyId = bufferToBase64Url(randomBytes(8));
-    const accessToken = await this.hashPassword(secret);
+    const accessToken = await hashPassword(secret);
     let token;
     // Tokens can only be valid for a maximum of 2 years
     const maximumTokenValidity =
@@ -138,7 +123,7 @@ export class AuthService {
     if (accessToken === undefined) {
       throw new NotInDBError(`AuthToken '${token}' not found`);
     }
-    if (!(await this.checkPassword(token, accessToken.accessTokenHash))) {
+    if (!(await checkPassword(token, accessToken.accessTokenHash))) {
       // hashes are not the same
       throw new TokenNotValidError(`AuthToken '${token}' is not valid.`);
     }
@@ -155,7 +140,9 @@ export class AuthService {
   }
 
   async getTokensByUsername(userName: string): Promise<AuthToken[]> {
-    const user = await this.usersService.getUserByUsername(userName, true);
+    const user = await this.usersService.getUserByUsername(userName, [
+      UserRelationEnum.AUTHTOKENS,
+    ]);
     if (user.authTokens === undefined) {
       return [];
     }

src/utils/password.spec.ts

@@ -0,0 +1,59 @@
+/*
+ * SPDX-FileCopyrightText: 2021 The HedgeDoc developers (see AUTHORS file)
+ *
+ * SPDX-License-Identifier: AGPL-3.0-only
+ */
+import bcrypt from 'bcrypt';
+import { randomBytes } from 'crypto';
+
+import { bufferToBase64Url, checkPassword, hashPassword } from './password';
+
+const testPassword = 'thisIsATestPassword';
+
+describe('hashPassword', () => {
+  it('output looks like a bcrypt hash with 2^12 rounds of hashing', async () => {
+    /*
+     * a bcrypt hash example with the different parts highlighted:
+     * $2a$10$N9qo8uLOickgx2ZMRZoMyeIjZAgcfl7p92ldGxad68LJZdL17lhWy
+     * \__/\/ \____________________/\_____________________________/
+     * Alg Cost      Salt                        Hash
+     * from https://en.wikipedia.org/wiki/Bcrypt#Description
+     */
+    const regexBcrypt = /^\$2[abxy]\$12\$[A-Za-z0-9/.]{53}$/;
+    const hash = await hashPassword(testPassword);
+    expect(regexBcrypt.test(hash)).toBeTruthy();
+  });
+  it('calls bcrypt.hash with the correct parameters', async () => {
+    const spy = jest.spyOn(bcrypt, 'hash');
+    await hashPassword(testPassword);
+    expect(spy).toHaveBeenCalledWith(testPassword, 12);
+  });
+});
+
+describe('checkPassword', () => {
+  it("is returning true if the inputs are a plaintext password and it's bcrypt-hashed version", async () => {
+    const hashOfTestPassword =
+      '$2a$12$WHKCq4c0rg19zyx5WgX0p.or0rjSKYpIBcHhQQGLrxrr6FfMPylIW';
+    await checkPassword(testPassword, hashOfTestPassword).then((result) =>
+      expect(result).toBeTruthy(),
+    );
+  });
+  it('fails, if secret is too short', async () => {
+    const secret = bufferToBase64Url(randomBytes(54));
+    const hash = await hashPassword(secret);
+    await checkPassword(secret, hash).then((result) =>
+      expect(result).toBeTruthy(),
+    );
+    await checkPassword(secret.substr(0, secret.length - 1), hash).then(
+      (result) => expect(result).toBeFalsy(),
+    );
+  });
+});
+
+describe('bufferToBase64Url', () => {
+  it('transforms a buffer to the correct base64url encoded string', () => {
+    expect(
+      bufferToBase64Url(Buffer.from('testsentence is a test sentence')),
+    ).toEqual('dGVzdHNlbnRlbmNlIGlzIGEgdGVzdCBzZW50ZW5jZQ');
+  });
+});

src/utils/password.ts

@@ -0,0 +1,30 @@
+/*
+ * SPDX-FileCopyrightText: 2021 The HedgeDoc developers (see AUTHORS file)
+ *
+ * SPDX-License-Identifier: AGPL-3.0-only
+ */
+import { compare, hash } from 'bcrypt';
+
+export async function hashPassword(cleartext: string): Promise<string> {
+  // hash the password with bcrypt and 2^12 iterations
+  // this was decided on the basis of https://cheatsheetseries.owasp.org/cheatsheets/Password_Storage_Cheat_Sheet.html#bcrypt
+  return await hash(cleartext, 12);
+}
+
+export async function checkPassword(
+  cleartext: string,
+  password: string,
+): Promise<boolean> {
+  return await compare(cleartext, password);
+}
+
+export function bufferToBase64Url(text: Buffer): string {
+  // This is necessary as the is no base64url encoding in the toString method
+  // but as can be seen on https://tools.ietf.org/html/rfc4648#page-7
+  // base64url is quite easy buildable from base64
+  return text
+    .toString('base64')
+    .replace(/\+/g, '-')
+    .replace(/\//g, '_')
+    .replace(/=+$/, '');
+}