diff --git a/src/api/private/tokens/tokens.controller.ts b/src/api/private/tokens/tokens.controller.ts index df36d4e6d..ffb4403ef 100644 --- a/src/api/private/tokens/tokens.controller.ts +++ b/src/api/private/tokens/tokens.controller.ts @@ -53,7 +53,7 @@ export class TokensController { @Body() createDto: AuthTokenCreateDto, @RequestUser() user: User, ): Promise { - return await this.authService.createTokenForUser( + return await this.authService.addToken( user, createDto.label, createDto.validUntil, diff --git a/src/auth/auth-token.entity.ts b/src/auth/auth-token.entity.ts index cabae5547..bb3207be2 100644 --- a/src/auth/auth-token.entity.ts +++ b/src/auth/auth-token.entity.ts @@ -48,16 +48,16 @@ export class AuthToken { keyId: string, user: User, label: string, - accessToken: string, + tokenString: string, validUntil: Date, ): Omit { - const newToken = new AuthToken(); - newToken.keyId = keyId; - newToken.user = Promise.resolve(user); - newToken.label = label; - newToken.accessTokenHash = accessToken; - newToken.validUntil = validUntil; - newToken.lastUsedAt = null; - return newToken; + const token = new AuthToken(); + token.keyId = keyId; + token.user = Promise.resolve(user); + token.label = label; + token.accessTokenHash = tokenString; + token.validUntil = validUntil; + token.lastUsedAt = null; + return token; } } diff --git a/src/auth/auth.service.spec.ts b/src/auth/auth.service.spec.ts index 2500ea4ff..170077009 100644 --- a/src/auth/auth.service.spec.ts +++ b/src/auth/auth.service.spec.ts @@ -17,7 +17,6 @@ import { LoggerModule } from '../logger/logger.module'; 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'; @@ -96,10 +95,7 @@ describe('AuthService', () => { user: Promise.resolve(user), accessTokenHash: accessTokenHash, }); - const authTokenFromCall = await service.getAuthTokenAndValidate( - authToken.keyId, - token, - ); + const authTokenFromCall = await service.getAuthToken(authToken.keyId); expect(authTokenFromCall).toEqual({ ...authToken, user: Promise.resolve(user), @@ -109,34 +105,41 @@ describe('AuthService', () => { describe('fails:', () => { it('AuthToken could not be found', async () => { jest.spyOn(authTokenRepo, 'findOne').mockResolvedValueOnce(null); - await expect( - service.getAuthTokenAndValidate(authToken.keyId, token), - ).rejects.toThrow(NotInDBError); - }); - it('AuthToken has wrong hash', async () => { - jest.spyOn(authTokenRepo, 'findOne').mockResolvedValueOnce({ - ...authToken, - user: Promise.resolve(user), - accessTokenHash: 'the wrong hash', - }); - await expect( - service.getAuthTokenAndValidate(authToken.keyId, token), - ).rejects.toThrow(TokenNotValidError); - }); - it('AuthToken has wrong validUntil Date', async () => { - const accessTokenHash = await hashPassword(token); - jest.spyOn(authTokenRepo, 'findOne').mockResolvedValueOnce({ - ...authToken, - user: Promise.resolve(user), - accessTokenHash: accessTokenHash, - validUntil: new Date(1549312452000), - }); - await expect( - service.getAuthTokenAndValidate(authToken.keyId, token), - ).rejects.toThrow(TokenNotValidError); + await expect(service.getAuthToken(authToken.keyId)).rejects.toThrow( + NotInDBError, + ); }); }); }); + describe('checkToken', () => { + it('works', () => { + const [accessToken, secret] = service.createToken( + user, + 'TestToken', + undefined, + ); + + expect(() => + service.checkToken(secret, accessToken as AuthToken), + ).not.toThrow(); + }); + it('AuthToken has wrong hash', () => { + const [accessToken] = service.createToken(user, 'TestToken', undefined); + expect(() => + service.checkToken('secret', accessToken as AuthToken), + ).toThrow(TokenNotValidError); + }); + it('AuthToken has wrong validUntil Date', () => { + const [accessToken, secret] = service.createToken( + user, + 'Test', + 1549312452000, + ); + expect(() => + service.checkToken(secret, accessToken as AuthToken), + ).toThrow(TokenNotValidError); + }); + }); describe('setLastUsedToken', () => { it('works', async () => { @@ -233,7 +236,7 @@ describe('AuthService', () => { }); }); - describe('createTokenForUser', () => { + describe('addToken', () => { describe('works', () => { const identifier = 'testIdentifier'; it('with validUntil 0', async () => { @@ -246,7 +249,7 @@ describe('AuthService', () => { return authTokenSaved; }, ); - const token = await service.createTokenForUser(user, identifier, 0); + const token = await service.addToken(user, identifier, 0); expect(token.label).toEqual(identifier); expect( token.validUntil.getTime() - @@ -266,11 +269,7 @@ describe('AuthService', () => { }, ); const validUntil = new Date().getTime() + 30000; - const token = await service.createTokenForUser( - user, - identifier, - validUntil, - ); + const token = await service.addToken(user, identifier, validUntil); expect(token.label).toEqual(identifier); expect(token.validUntil.getTime()).toEqual(validUntil); expect(token.lastUsedAt).toBeNull(); diff --git a/src/auth/auth.service.ts b/src/auth/auth.service.ts index eb67b7a97..dbc525adc 100644 --- a/src/auth/auth.service.ts +++ b/src/auth/auth.service.ts @@ -33,39 +33,29 @@ export class AuthService { this.logger.setContext(AuthService.name); } - async validateToken(token: string): Promise { - const [keyId, secret] = token.split('.'); + async validateToken(tokenString: string): Promise { + const [keyId, secret] = tokenString.split('.'); if (!secret) { throw new TokenNotValidError('Invalid AuthToken format'); } if (secret.length != 86) { // We always expect 86 characters, as the secret is generated with 64 bytes // and then converted to a base64url string - throw new TokenNotValidError(`AuthToken '${token}' has incorrect length`); + throw new TokenNotValidError( + `AuthToken '${tokenString}' has incorrect length`, + ); } - const accessToken = await this.getAuthTokenAndValidate(keyId, secret); + const token = await this.getAuthToken(keyId); + this.checkToken(secret, token); await this.setLastUsedToken(keyId); - return await this.usersService.getUserByUsername( - ( - await accessToken.user - ).username, - ); + return await token.user; } - async createTokenForUser( + createToken( user: User, identifier: string, validUntil: TimestampMillis | undefined, - ): Promise { - user.authTokens = this.getTokensByUser(user); - - if ((await user.authTokens).length >= 200) { - // This is a very high ceiling unlikely to hinder legitimate usage, - // but should prevent possible attack vectors - throw new TooManyTokensError( - `User '${user.username}' has already 200 tokens and can't have anymore`, - ); - } + ): [Omit, string] { const secret = bufferToBase64Url(randomBytes(64)); const keyId = bufferToBase64Url(randomBytes(8)); // More about the choice of SHA-512 in the dev docs @@ -94,39 +84,60 @@ export class AuthService { new Date(validUntil), ); } + return [token, secret]; + } + + async addToken( + user: User, + identifier: string, + validUntil: TimestampMillis | undefined, + ): Promise { + user.authTokens = this.getTokensByUser(user); + + if ((await user.authTokens).length >= 200) { + // This is a very high ceiling unlikely to hinder legitimate usage, + // but should prevent possible attack vectors + throw new TooManyTokensError( + `User '${user.username}' has already 200 tokens and can't have anymore`, + ); + } + const [token, secret] = this.createToken(user, identifier, validUntil); const createdToken = (await this.authTokenRepository.save( token, )) as AuthToken; - return this.toAuthTokenWithSecretDto(createdToken, `${keyId}.${secret}`); + return this.toAuthTokenWithSecretDto( + createdToken, + `${createdToken.keyId}.${secret}`, + ); } async setLastUsedToken(keyId: string): Promise { - const accessToken = await this.authTokenRepository.findOne({ + const token = await this.authTokenRepository.findOne({ where: { keyId: keyId }, }); - if (accessToken === null) { + if (token === null) { throw new NotInDBError(`AuthToken for key '${keyId}' not found`); } - accessToken.lastUsedAt = new Date(); - await this.authTokenRepository.save(accessToken); + token.lastUsedAt = new Date(); + await this.authTokenRepository.save(token); } - async getAuthTokenAndValidate( - keyId: string, - token: string, - ): Promise { - const accessToken = await this.authTokenRepository.findOne({ + async getAuthToken(keyId: string): Promise { + const token = await this.authTokenRepository.findOne({ where: { keyId: keyId }, relations: ['user'], }); - if (accessToken === null) { - throw new NotInDBError(`AuthToken '${token}' not found`); + if (token === null) { + throw new NotInDBError(`AuthToken '${keyId}' not found`); } - // Hash the user-provided token + return token; + } + + checkToken(secret: string, token: AuthToken): void { const userHash = Buffer.from( - crypto.createHash('sha512').update(token).digest('hex'), + crypto.createHash('sha512').update(secret).digest('hex'), ); - const dbHash = Buffer.from(accessToken.accessTokenHash); + const dbHash = Buffer.from(token.accessTokenHash); if ( // Normally, both hashes have the same length, as they are both SHA512 // This is only defense-in-depth, as timingSafeEqual throws if the buffers are not of the same length @@ -134,18 +145,18 @@ export class AuthService { !crypto.timingSafeEqual(userHash, dbHash) ) { // hashes are not the same - throw new TokenNotValidError(`AuthToken '${token}' is not valid.`); - } - if ( - accessToken.validUntil && - accessToken.validUntil.getTime() < new Date().getTime() - ) { - // tokens validUntil Date lies in the past throw new TokenNotValidError( - `AuthToken '${token}' is not valid since ${accessToken.validUntil.toISOString()}.`, + `Secret does not match Token ${token.label}.`, + ); + } + if (token.validUntil && token.validUntil.getTime() < new Date().getTime()) { + // tokens validUntil Date lies in the past + throw new TokenNotValidError( + `AuthToken '${ + token.label + }' is not valid since ${token.validUntil.toISOString()}.`, ); } - return accessToken; } async getTokensByUser(user: User): Promise { diff --git a/test/test-setup.ts b/test/test-setup.ts index 1a2a49fb2..0b94378be 100644 --- a/test/test-setup.ts +++ b/test/test-setup.ts @@ -343,7 +343,7 @@ export class TestSetupBuilder { // create auth tokens this.testSetup.authTokens = await Promise.all( this.testSetup.users.map(async (user) => { - return await this.testSetup.authService.createTokenForUser( + return await this.testSetup.authService.addToken( user, 'test', new Date().getTime() + 60 * 60 * 1000,