From 87a5f77abe63e422a5a9a66dc5f1132edab9820a Mon Sep 17 00:00:00 2001 From: Philip Molares Date: Sun, 8 Aug 2021 21:47:13 +0200 Subject: [PATCH] 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 --- src/auth/auth.service.spec.ts | 40 +++--------------------- src/auth/auth.service.ts | 43 +++++++++---------------- src/utils/password.spec.ts | 59 +++++++++++++++++++++++++++++++++++ src/utils/password.ts | 30 ++++++++++++++++++ 4 files changed, 109 insertions(+), 63 deletions(-) create mode 100644 src/utils/password.spec.ts create mode 100644 src/utils/password.ts diff --git a/src/auth/auth.service.spec.ts b/src/auth/auth.service.spec.ts index f7aaba855..937a97feb 100644 --- a/src/auth/auth.service.spec.ts +++ b/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(); diff --git a/src/auth/auth.service.ts b/src/auth/auth.service.ts index 609592beb..5bcc01540 100644 --- a/src/auth/auth.service.ts +++ b/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 { - // 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 { - 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 { - 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 keyId = this.bufferToBase64Url(randomBytes(8)); - const accessToken = await this.hashPassword(secret); + const secret = bufferToBase64Url(randomBytes(54)); + const keyId = bufferToBase64Url(randomBytes(8)); + 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 { - const user = await this.usersService.getUserByUsername(userName, true); + const user = await this.usersService.getUserByUsername(userName, [ + UserRelationEnum.AUTHTOKENS, + ]); if (user.authTokens === undefined) { return []; } diff --git a/src/utils/password.spec.ts b/src/utils/password.spec.ts new file mode 100644 index 000000000..199c7d16b --- /dev/null +++ b/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'); + }); +}); diff --git a/src/utils/password.ts b/src/utils/password.ts new file mode 100644 index 000000000..a419bf01f --- /dev/null +++ b/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 { + // 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 { + 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(/=+$/, ''); +}