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(/=+$/, ''); +}