From f68caab6e827b9b17f103be35a53349bbfff2689 Mon Sep 17 00:00:00 2001 From: Philip Molares Date: Sat, 23 Jan 2021 21:24:11 +0100 Subject: [PATCH] auth: Integrate suggestions by @davidmehren Add number type alias TimestampMillis Remove solved ToDos Change AuthToken and AuthTokenDto to use Date Rename authService unit tests Signed-off-by: Philip Molares --- src/api/private/tokens/tokens.controller.ts | 5 +- src/api/public/media/media.controller.ts | 2 - src/auth/auth-token.dto.ts | 16 ++- src/auth/auth-token.entity.ts | 20 ++- src/auth/auth.service.spec.ts | 148 ++++++++++++-------- src/auth/auth.service.ts | 44 ++++-- src/utils/timestamp.ts | 7 + 7 files changed, 149 insertions(+), 93 deletions(-) create mode 100644 src/utils/timestamp.ts diff --git a/src/api/private/tokens/tokens.controller.ts b/src/api/private/tokens/tokens.controller.ts index 89d47cbde..780e11be2 100644 --- a/src/api/private/tokens/tokens.controller.ts +++ b/src/api/private/tokens/tokens.controller.ts @@ -17,6 +17,7 @@ import { ConsoleLoggerService } from '../../../logger/console-logger.service'; import { AuthTokenDto } from '../../../auth/auth-token.dto'; import { AuthTokenWithSecretDto } from '../../../auth/auth-token-with-secret.dto'; import { AuthService } from '../../../auth/auth.service'; +import { TimestampMillis } from '../../../utils/timestamp'; @Controller('tokens') export class TokensController { @@ -38,10 +39,10 @@ export class TokensController { @Post() async postTokenRequest( @Body('label') label: string, - @Body('until') until: number, + @Body('validUntil') validUntil: TimestampMillis, ): Promise { // ToDo: Get real userName - return this.authService.createTokenForUser('hardcoded', label, until); + return this.authService.createTokenForUser('hardcoded', label, validUntil); } @Delete('/:keyId') diff --git a/src/api/public/media/media.controller.ts b/src/api/public/media/media.controller.ts index 3cc8b2471..f8fd3bb45 100644 --- a/src/api/public/media/media.controller.ts +++ b/src/api/public/media/media.controller.ts @@ -48,7 +48,6 @@ export class MediaController { @UploadedFile() file: MulterFile, @Headers('HedgeDoc-Note') noteId: string, ) { - //TODO: Get user from request const username = req.user.userName; this.logger.debug( `Recieved filename '${file.originalname}' for note '${noteId}' from user '${username}'`, @@ -74,7 +73,6 @@ export class MediaController { @UseGuards(TokenAuthGuard) @Delete(':filename') async deleteMedia(@Request() req, @Param('filename') filename: string) { - //TODO: Get user from request const username = req.user.userName; try { await this.mediaService.deleteFile(filename, username); diff --git a/src/auth/auth-token.dto.ts b/src/auth/auth-token.dto.ts index 1c3592db3..46af7d6be 100644 --- a/src/auth/auth-token.dto.ts +++ b/src/auth/auth-token.dto.ts @@ -4,17 +4,19 @@ * SPDX-License-Identifier: AGPL-3.0-only */ -import { IsNumber, IsString } from 'class-validator'; +import { IsDate, IsOptional, IsString } from 'class-validator'; export class AuthTokenDto { @IsString() label: string; @IsString() keyId: string; - @IsNumber() - created: number; - @IsNumber() - validUntil: number | null; - @IsNumber() - lastUsed: number | null; + @IsDate() + createdAt: Date; + @IsDate() + @IsOptional() + validUntil: Date; + @IsDate() + @IsOptional() + lastUsed: Date; } diff --git a/src/auth/auth-token.entity.ts b/src/auth/auth-token.entity.ts index c0538f52a..9d216b40c 100644 --- a/src/auth/auth-token.entity.ts +++ b/src/auth/auth-token.entity.ts @@ -36,29 +36,35 @@ export class AuthToken { @Column({ nullable: true, }) - validUntil: number; + validUntil: Date; @Column({ nullable: true, }) - lastUsed: number; + lastUsed: Date; public static create( user: User, identifier: string, keyId: string, accessToken: string, - validUntil?: number, - ): Pick { + validUntil?: Date, + ): Pick< + AuthToken, + | 'user' + | 'identifier' + | 'keyId' + | 'accessTokenHash' + | 'createdAt' + | 'validUntil' + > { const newToken = new AuthToken(); newToken.user = user; newToken.identifier = identifier; newToken.keyId = keyId; newToken.accessTokenHash = accessToken; newToken.createdAt = new Date(); - if (validUntil !== undefined) { - newToken.validUntil = validUntil; - } + newToken.validUntil = validUntil; return newToken; } } diff --git a/src/auth/auth.service.spec.ts b/src/auth/auth.service.spec.ts index 13c5a9007..e5375e15d 100644 --- a/src/auth/auth.service.spec.ts +++ b/src/auth/auth.service.spec.ts @@ -64,7 +64,9 @@ describe('AuthService', () => { if (entity.lastUsed === undefined) { expect(entity.lastUsed).toBeUndefined(); } else { - expect(entity.lastUsed).toBeLessThanOrEqual(new Date().getTime()); + expect(entity.lastUsed.getTime()).toBeLessThanOrEqual( + new Date().getTime(), + ); } return entity; }, @@ -95,78 +97,100 @@ describe('AuthService', () => { expect(service).toBeDefined(); }); - it('checkPassword', async () => { - const testPassword = 'thisIsATestPassword'; - const hash = await service.hashPassword(testPassword); - service - .checkPassword(testPassword, hash) - .then((result) => expect(result).toBeTruthy()); - }); - - it('getTokensByUsername', async () => { - const tokens = await service.getTokensByUsername(user.userName); - expect(tokens).toHaveLength(1); - expect(tokens).toEqual([authToken]); - }); - - it('getAuthToken', async () => { - const token = 'testToken'; - authToken.accessTokenHash = await service.hashPassword(token); - const authTokenFromCall = await service.getAuthTokenAndValidate( - authToken.keyId, - token, - ); - expect(authTokenFromCall).toEqual({ - ...authToken, - user: user, + describe('checkPassword', () => { + it('works', async () => { + const testPassword = 'thisIsATestPassword'; + const hash = await service.hashPassword(testPassword); + service + .checkPassword(testPassword, hash) + .then((result) => expect(result).toBeTruthy()); }); }); - it('setLastUsedToken', async () => { - await service.setLastUsedToken(authToken.keyId); - }); - - it('validateToken', async () => { - const token = 'testToken'; - authToken.accessTokenHash = await service.hashPassword(token); - const userByToken = await service.validateToken( - `${authToken.keyId}.${token}`, - ); - expect(userByToken).toEqual({ - ...user, - authTokens: [authToken], + describe('getTokensByUsername', () => { + it('works', async () => { + const tokens = await service.getTokensByUsername(user.userName); + expect(tokens).toHaveLength(1); + expect(tokens).toEqual([authToken]); }); }); - it('removeToken', async () => { - await service.removeToken(user.userName, authToken.keyId); + describe('getAuthToken', () => { + it('works', async () => { + const token = 'testToken'; + authToken.accessTokenHash = await service.hashPassword(token); + const authTokenFromCall = await service.getAuthTokenAndValidate( + authToken.keyId, + token, + ); + expect(authTokenFromCall).toEqual({ + ...authToken, + user: user, + }); + }); }); - it('createTokenForUser', async () => { - const identifier = 'identifier2'; - const token = await service.createTokenForUser( - user.userName, - identifier, - 0, - ); - expect(token.label).toEqual(identifier); - expect(token.validUntil).toBeUndefined(); - expect(token.lastUsed).toBeUndefined(); - expect(token.secret.startsWith(token.keyId)).toBeTruthy(); + describe('setLastUsedToken', () => { + it('works', async () => { + await service.setLastUsedToken(authToken.keyId); + }); }); - it('BufferToBase64Url', () => { - expect( - service.BufferToBase64Url(Buffer.from('testsentence is a test sentence')), - ).toEqual('dGVzdHNlbnRlbmNlIGlzIGEgdGVzdCBzZW50ZW5jZQ'); + describe('validateToken', () => { + it('works', async () => { + const token = 'testToken'; + authToken.accessTokenHash = await service.hashPassword(token); + const userByToken = await service.validateToken( + `${authToken.keyId}.${token}`, + ); + expect(userByToken).toEqual({ + ...user, + authTokens: [authToken], + }); + }); }); - it('toAuthTokenDto', async () => { - const tokenDto = await service.toAuthTokenDto(authToken); - expect(tokenDto.keyId).toEqual(authToken.keyId); - expect(tokenDto.lastUsed).toBeNull(); - expect(tokenDto.label).toEqual(authToken.identifier); - expect(tokenDto.validUntil).toBeNull(); - expect(tokenDto.created).toEqual(authToken.createdAt.getTime()); + describe('removeToken', () => { + it('works', async () => { + await service.removeToken(user.userName, authToken.keyId); + }); + }); + + describe('createTokenForUser', () => { + it('works', async () => { + const identifier = 'identifier2'; + const token = await service.createTokenForUser( + user.userName, + identifier, + 0, + ); + expect(token.label).toEqual(identifier); + expect(token.validUntil).toBeNull(); + expect(token.lastUsed).toBeNull(); + expect(token.secret.startsWith(token.keyId)).toBeTruthy(); + }); + }); + + describe('BufferToBase64Url', () => { + it('works', () => { + expect( + service.BufferToBase64Url( + Buffer.from('testsentence is a test sentence'), + ), + ).toEqual('dGVzdHNlbnRlbmNlIGlzIGEgdGVzdCBzZW50ZW5jZQ'); + }); + }); + + describe('toAuthTokenDto', () => { + it('works', async () => { + const tokenDto = await service.toAuthTokenDto(authToken); + expect(tokenDto.keyId).toEqual(authToken.keyId); + expect(tokenDto.lastUsed).toBeNull(); + expect(tokenDto.label).toEqual(authToken.identifier); + expect(tokenDto.validUntil).toBeNull(); + expect(tokenDto.createdAt.getTime()).toEqual( + authToken.createdAt.getTime(), + ); + }); }); }); diff --git a/src/auth/auth.service.ts b/src/auth/auth.service.ts index 636d7a776..10a427ced 100644 --- a/src/auth/auth.service.ts +++ b/src/auth/auth.service.ts @@ -16,6 +16,7 @@ import { randomBytes } from 'crypto'; import { InjectRepository } from '@nestjs/typeorm'; import { Repository } from 'typeorm'; import { ConsoleLoggerService } from '../logger/console-logger.service'; +import { TimestampMillis } from '../utils/timestamp'; @Injectable() export class AuthService { @@ -29,13 +30,13 @@ export class AuthService { } async validateToken(token: string): Promise { - const parts = token.split('.'); - const accessToken = await this.getAuthTokenAndValidate(parts[0], parts[1]); + const [keyId, secret] = token.split('.'); + const accessToken = await this.getAuthTokenAndValidate(keyId, secret); const user = await this.usersService.getUserByUsername( accessToken.user.userName, ); if (user) { - await this.setLastUsedToken(parts[0]); + await this.setLastUsedToken(keyId); return user; } return null; @@ -43,6 +44,7 @@ export class AuthService { 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 hash(cleartext, 12); } @@ -71,7 +73,7 @@ export class AuthService { async createTokenForUser( userName: string, identifier: string, - until: number, + validUntil: TimestampMillis, ): Promise { const user = await this.usersService.getUserByUsername(userName); const secret = await this.randomString(64); @@ -79,10 +81,16 @@ export class AuthService { const accessTokenString = await this.hashPassword(secret.toString()); const accessToken = this.BufferToBase64Url(Buffer.from(accessTokenString)); let token; - if (until === 0) { + if (validUntil === 0) { token = AuthToken.create(user, identifier, keyId, accessToken); } else { - token = AuthToken.create(user, identifier, keyId, accessToken, until); + token = AuthToken.create( + user, + identifier, + keyId, + accessToken, + new Date(validUntil), + ); } const createdToken = await this.authTokenRepository.save(token); return this.toAuthTokenWithSecretDto(createdToken, `${keyId}.${secret}`); @@ -92,7 +100,7 @@ export class AuthService { const accessToken = await this.authTokenRepository.findOne({ where: { keyId: keyId }, }); - accessToken.lastUsed = new Date().getTime(); + accessToken.lastUsed = new Date(); await this.authTokenRepository.save(accessToken); } @@ -113,7 +121,7 @@ export class AuthService { } if ( accessToken.validUntil && - accessToken.validUntil < new Date().getTime() + accessToken.validUntil.getTime() < new Date().getTime() ) { // tokens validUntil Date lies in the past throw new TokenNotValidError( @@ -141,18 +149,28 @@ export class AuthService { await this.authTokenRepository.remove(token); } - toAuthTokenDto(authToken: AuthToken | null | undefined): AuthTokenDto | null { + toAuthTokenDto(authToken: AuthToken): AuthTokenDto | null { if (!authToken) { this.logger.warn(`Recieved ${authToken} argument!`, 'toAuthTokenDto'); return null; } - return { + const tokenDto: AuthTokenDto = { + lastUsed: null, + validUntil: null, label: authToken.identifier, keyId: authToken.keyId, - created: authToken.createdAt.getTime(), - validUntil: authToken.validUntil, - lastUsed: authToken.lastUsed, + createdAt: authToken.createdAt, }; + + if (authToken.validUntil) { + tokenDto.validUntil = new Date(authToken.validUntil); + } + + if (authToken.lastUsed) { + tokenDto.lastUsed = new Date(authToken.lastUsed); + } + + return tokenDto; } toAuthTokenWithSecretDto( diff --git a/src/utils/timestamp.ts b/src/utils/timestamp.ts new file mode 100644 index 000000000..4e427aab4 --- /dev/null +++ b/src/utils/timestamp.ts @@ -0,0 +1,7 @@ +/* + * SPDX-FileCopyrightText: 2021 The HedgeDoc developers (see AUTHORS file) + * + * SPDX-License-Identifier: AGPL-3.0-only + */ + +export type TimestampMillis = number;