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 <philip.molares@udo.edu>

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<AuthTokenWithSecretDto> {
     // ToDo: Get real userName
-    return this.authService.createTokenForUser('hardcoded', label, until);
+    return this.authService.createTokenForUser('hardcoded', label, validUntil);
   }
 
   @Delete('/:keyId')

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);

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()
+  @IsDate()
-  created: number;
+  createdAt: Date;
-  @IsNumber()
+  @IsDate()
-  validUntil: number | null;
+  @IsOptional()
-  @IsNumber()
+  validUntil: Date;
-  lastUsed: number | null;
+  @IsDate()
+  @IsOptional()
+  lastUsed: Date;
 }

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,
+    validUntil?: Date,
-  ): Pick<AuthToken, 'user' | 'accessTokenHash'> {
+  ): 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;
-    }
     return newToken;
   }
 }

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,21 +97,26 @@ describe('AuthService', () => {
     expect(service).toBeDefined();
   });
 
-  it('checkPassword', async () => {
+  describe('checkPassword', () => {
+    it('works', async () => {
       const testPassword = 'thisIsATestPassword';
       const hash = await service.hashPassword(testPassword);
       service
         .checkPassword(testPassword, hash)
         .then((result) => expect(result).toBeTruthy());
     });
+  });
 
-  it('getTokensByUsername', async () => {
+  describe('getTokensByUsername', () => {
+    it('works', async () => {
       const tokens = await service.getTokensByUsername(user.userName);
       expect(tokens).toHaveLength(1);
       expect(tokens).toEqual([authToken]);
     });
+  });
 
-  it('getAuthToken', async () => {
+  describe('getAuthToken', () => {
+    it('works', async () => {
       const token = 'testToken';
       authToken.accessTokenHash = await service.hashPassword(token);
       const authTokenFromCall = await service.getAuthTokenAndValidate(
@@ -121,12 +128,16 @@ describe('AuthService', () => {
         user: user,
       });
     });
-
-  it('setLastUsedToken', async () => {
-    await service.setLastUsedToken(authToken.keyId);
   });
 
-  it('validateToken', async () => {
+  describe('setLastUsedToken', () => {
+    it('works', async () => {
+      await service.setLastUsedToken(authToken.keyId);
+    });
+  });
+
+  describe('validateToken', () => {
+    it('works', async () => {
       const token = 'testToken';
       authToken.accessTokenHash = await service.hashPassword(token);
       const userByToken = await service.validateToken(
@@ -137,12 +148,16 @@ describe('AuthService', () => {
         authTokens: [authToken],
       });
     });
-
-  it('removeToken', async () => {
-    await service.removeToken(user.userName, authToken.keyId);
   });
 
-  it('createTokenForUser', async () => {
+  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,
@@ -150,23 +165,32 @@ describe('AuthService', () => {
         0,
       );
       expect(token.label).toEqual(identifier);
-    expect(token.validUntil).toBeUndefined();
+      expect(token.validUntil).toBeNull();
-    expect(token.lastUsed).toBeUndefined();
+      expect(token.lastUsed).toBeNull();
       expect(token.secret.startsWith(token.keyId)).toBeTruthy();
     });
-
-  it('BufferToBase64Url', () => {
-    expect(
-      service.BufferToBase64Url(Buffer.from('testsentence is a test sentence')),
-    ).toEqual('dGVzdHNlbnRlbmNlIGlzIGEgdGVzdCBzZW50ZW5jZQ');
   });
 
-  it('toAuthTokenDto', async () => {
+  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.created).toEqual(authToken.createdAt.getTime());
+      expect(tokenDto.createdAt.getTime()).toEqual(
+        authToken.createdAt.getTime(),
+      );
+    });
   });
 });

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<User> {
-    const parts = token.split('.');
+    const [keyId, secret] = token.split('.');
-    const accessToken = await this.getAuthTokenAndValidate(parts[0], parts[1]);
+    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<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 hash(cleartext, 12);
   }
 
@@ -71,7 +73,7 @@ export class AuthService {
   async createTokenForUser(
     userName: string,
     identifier: string,
-    until: number,
+    validUntil: TimestampMillis,
   ): Promise<AuthTokenWithSecretDto> {
     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(),
+      createdAt: authToken.createdAt,
-      validUntil: authToken.validUntil,
-      lastUsed: authToken.lastUsed,
     };
+
+    if (authToken.validUntil) {
+      tokenDto.validUntil = new Date(authToken.validUntil);
+    }
+
+    if (authToken.lastUsed) {
+      tokenDto.lastUsed = new Date(authToken.lastUsed);
+    }
+
+    return tokenDto;
   }
 
   toAuthTokenWithSecretDto(

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;