From dae29ed3590e558e26d248dc136276856f3704c0 Mon Sep 17 00:00:00 2001 From: Maximilian Kreuzkam Date: Fri, 17 May 2024 11:04:33 +0200 Subject: [PATCH] EW-649: Refactoring of authentication. --- .../authentication/authentication-config.ts | 22 +++++++++++++++++++ .../authentication/authentication.module.ts | 22 +------------------ .../controllers/login.controller.ts | 3 +++ .../decorator/auth.decorator.ts | 21 +++++++++--------- .../errors/brute-force.error.ts | 2 +- .../jwt-extractor.spec.ts | 0 .../{strategy => helper}/jwt-extractor.ts | 0 .../jwt-validation.adapter.spec.ts | 0 .../jwt-validation.adapter.ts | 1 - .../authentication/interface/jwt-payload.ts | 8 +++---- .../modules/authentication/interface/user.ts | 14 ++++++++++++ .../services/authentication.service.spec.ts | 2 +- .../services/authentication.service.ts | 8 +++++-- .../strategy/jwt.strategy.spec.ts | 2 +- .../authentication/strategy/jwt.strategy.ts | 4 ++-- .../authentication/strategy/ldap.strategy.ts | 3 --- .../strategy/ws-jwt.strategy.spec.ts | 2 +- .../strategy/ws-jwt.strategy.ts | 4 ++-- 18 files changed, 69 insertions(+), 49 deletions(-) rename apps/server/src/modules/authentication/{strategy => helper}/jwt-extractor.spec.ts (100%) rename apps/server/src/modules/authentication/{strategy => helper}/jwt-extractor.ts (100%) rename apps/server/src/modules/authentication/{strategy => helper}/jwt-validation.adapter.spec.ts (100%) rename apps/server/src/modules/authentication/{strategy => helper}/jwt-validation.adapter.ts (96%) diff --git a/apps/server/src/modules/authentication/authentication-config.ts b/apps/server/src/modules/authentication/authentication-config.ts index 9ed4f7250e5..39efb5ac0b3 100644 --- a/apps/server/src/modules/authentication/authentication-config.ts +++ b/apps/server/src/modules/authentication/authentication-config.ts @@ -1,5 +1,27 @@ import { AccountConfig } from '@modules/account'; import { XApiKeyConfig } from './config'; +import { jwtConstants } from './constants'; + +// values copied from Algorithm definition. Type does not exist at runtime and can't be checked anymore otherwise +const algorithms = [ + 'HS256', + 'HS384', + 'HS512', + 'RS256', + 'RS384', + 'RS512', + 'ES256', + 'ES384', + 'ES512', + 'PS256', + 'PS384', + 'PS512', + 'none', +]; + +if (!algorithms.includes(jwtConstants.jwtOptions.algorithm)) { + throw new Error(`${jwtConstants.jwtOptions.algorithm} is not a valid JWT signing algorithm`); +} export interface AuthenticationConfig extends AccountConfig, XApiKeyConfig { LOGIN_BLOCK_TIME: number; diff --git a/apps/server/src/modules/authentication/authentication.module.ts b/apps/server/src/modules/authentication/authentication.module.ts index 2719f337abe..fea67ce4ae6 100644 --- a/apps/server/src/modules/authentication/authentication.module.ts +++ b/apps/server/src/modules/authentication/authentication.module.ts @@ -13,7 +13,7 @@ import { Algorithm, SignOptions } from 'jsonwebtoken'; import { jwtConstants } from './constants'; import { AuthenticationService } from './services/authentication.service'; import { LdapService } from './services/ldap.service'; -import { JwtValidationAdapter } from './strategy/jwt-validation.adapter'; +import { JwtValidationAdapter } from './helper/jwt-validation.adapter'; import { JwtStrategy } from './strategy/jwt.strategy'; import { LdapStrategy } from './strategy/ldap.strategy'; import { LocalStrategy } from './strategy/local.strategy'; @@ -21,26 +21,6 @@ import { Oauth2Strategy } from './strategy/oauth2.strategy'; import { XApiKeyStrategy } from './strategy/x-api-key.strategy'; import { WsJwtStrategy } from './strategy/ws-jwt.strategy'; -// values copied from Algorithm definition. Type does not exist at runtime and can't be checked anymore otherwise -const algorithms = [ - 'HS256', - 'HS384', - 'HS512', - 'RS256', - 'RS384', - 'RS512', - 'ES256', - 'ES384', - 'ES512', - 'PS256', - 'PS384', - 'PS512', - 'none', -]; - -if (!algorithms.includes(jwtConstants.jwtOptions.algorithm)) { - throw new Error(`${jwtConstants.jwtOptions.algorithm} is not a valid JWT signing algorithm`); -} const signAlgorithm = jwtConstants.jwtOptions.algorithm as Algorithm; const signOptions: SignOptions = { diff --git a/apps/server/src/modules/authentication/controllers/login.controller.ts b/apps/server/src/modules/authentication/controllers/login.controller.ts index 68af396233e..1b9e2da9419 100644 --- a/apps/server/src/modules/authentication/controllers/login.controller.ts +++ b/apps/server/src/modules/authentication/controllers/login.controller.ts @@ -27,6 +27,7 @@ export class LoginController { @ApiResponse({ status: 200, type: LoginResponse, description: 'Login was successful.' }) @ApiResponse({ status: 400, type: ValidationError, description: 'Request data has invalid format.' }) @ApiResponse({ status: 403, type: ForbiddenOperationError, description: 'Invalid user credentials.' }) + // Body is not used, but validated and used in the strategy implementation // eslint-disable-next-line @typescript-eslint/no-unused-vars async loginLdap(@CurrentUser() user: ICurrentUser, @Body() _: LdapAuthorizationBodyParams): Promise { const loginDto: LoginDto = await this.loginUc.getLoginData(user); @@ -43,6 +44,7 @@ export class LoginController { @ApiResponse({ status: 200, type: LoginResponse, description: 'Login was successful.' }) @ApiResponse({ status: 400, type: ValidationError, description: 'Request data has invalid format.' }) @ApiResponse({ status: 403, type: ForbiddenOperationError, description: 'Invalid user credentials.' }) + // Body is not used, but validated and used in the strategy implementation // eslint-disable-next-line @typescript-eslint/no-unused-vars async loginLocal(@CurrentUser() user: ICurrentUser, @Body() _: LocalAuthorizationBodyParams): Promise { const loginDto: LoginDto = await this.loginUc.getLoginData(user); @@ -61,6 +63,7 @@ export class LoginController { @ApiResponse({ status: 403, type: ForbiddenOperationError, description: 'Invalid user credentials.' }) async loginOauth2( @CurrentUser() user: OauthCurrentUser, + // Body is not used, but validated and used in the strategy implementation // eslint-disable-next-line @typescript-eslint/no-unused-vars @Body() _: Oauth2AuthorizationBodyParams ): Promise { diff --git a/apps/server/src/modules/authentication/decorator/auth.decorator.ts b/apps/server/src/modules/authentication/decorator/auth.decorator.ts index 6d36784af0d..0bcfb55abc4 100644 --- a/apps/server/src/modules/authentication/decorator/auth.decorator.ts +++ b/apps/server/src/modules/authentication/decorator/auth.decorator.ts @@ -10,8 +10,8 @@ import { ApiBearerAuth } from '@nestjs/swagger'; import { Request } from 'express'; import { ExtractJwt } from 'passport-jwt'; import { JwtAuthGuard } from '../guard/jwt-auth.guard'; -import { ICurrentUser } from '../interface/user'; -import { JwtExtractor } from '../strategy/jwt-extractor'; +import { ICurrentUser, isICurrentUser } from '../interface/user'; +import { JwtExtractor } from '../helper/jwt-extractor'; const STRATEGIES = ['jwt'] as const; type Strategies = typeof STRATEGIES; @@ -21,7 +21,6 @@ type Strategies = typeof STRATEGIES; * @param strategies accepted strategies * @returns */ -// eslint-disable-next-line @typescript-eslint/explicit-module-boundary-types export const Authenticate = (...strategies: Strategies) => { if (strategies.includes('jwt')) { const decorators = [ @@ -39,22 +38,24 @@ export const Authenticate = (...strategies: Strategies) => { * Returns the current authenticated user. * @requires Authenticated */ -// eslint-disable-next-line @typescript-eslint/no-explicit-any -export const CurrentUser = createParamDecorator((data: unknown, ctx: ExecutionContext) => { - const { user }: Request = ctx.switchToHttp().getRequest(); - if (!user) +export const CurrentUser = createParamDecorator((_, ctx: ExecutionContext) => { + const expressRequest = ctx.switchToHttp().getRequest(); + const requestUser = expressRequest.user; + + if (!requestUser || !isICurrentUser(requestUser)) { throw new UnauthorizedException( 'CurrentUser missing in request context. This route requires jwt authentication guard enabled.' ); - return user as ICurrentUser; + } + + return requestUser; }); /** * Returns the current JWT. * @requires Authenticated */ -// eslint-disable-next-line @typescript-eslint/no-explicit-any -export const JWT = createParamDecorator((data: unknown, ctx: ExecutionContext) => { +export const JWT = createParamDecorator((_, ctx: ExecutionContext) => { const getJWT = ExtractJwt.fromExtractors([ExtractJwt.fromAuthHeaderAsBearerToken(), JwtExtractor.fromCookie('jwt')]); const req: Request = ctx.switchToHttp().getRequest(); const jwt = getJWT(req) || req.headers.authorization; diff --git a/apps/server/src/modules/authentication/errors/brute-force.error.ts b/apps/server/src/modules/authentication/errors/brute-force.error.ts index bea572d2fdd..f8d9df13b25 100644 --- a/apps/server/src/modules/authentication/errors/brute-force.error.ts +++ b/apps/server/src/modules/authentication/errors/brute-force.error.ts @@ -6,7 +6,7 @@ export class BruteForceError extends BusinessError { constructor(timeToWait: number, message: string) { super( - { type: 'ENTITY_NOT_FOUND', title: 'Entity Not Found', defaultMessage: message }, + { type: 'TOO_MANY_REQUESTS', title: 'Too many requests', defaultMessage: message }, HttpStatus.TOO_MANY_REQUESTS, { timeToWait, diff --git a/apps/server/src/modules/authentication/strategy/jwt-extractor.spec.ts b/apps/server/src/modules/authentication/helper/jwt-extractor.spec.ts similarity index 100% rename from apps/server/src/modules/authentication/strategy/jwt-extractor.spec.ts rename to apps/server/src/modules/authentication/helper/jwt-extractor.spec.ts diff --git a/apps/server/src/modules/authentication/strategy/jwt-extractor.ts b/apps/server/src/modules/authentication/helper/jwt-extractor.ts similarity index 100% rename from apps/server/src/modules/authentication/strategy/jwt-extractor.ts rename to apps/server/src/modules/authentication/helper/jwt-extractor.ts diff --git a/apps/server/src/modules/authentication/strategy/jwt-validation.adapter.spec.ts b/apps/server/src/modules/authentication/helper/jwt-validation.adapter.spec.ts similarity index 100% rename from apps/server/src/modules/authentication/strategy/jwt-validation.adapter.spec.ts rename to apps/server/src/modules/authentication/helper/jwt-validation.adapter.spec.ts diff --git a/apps/server/src/modules/authentication/strategy/jwt-validation.adapter.ts b/apps/server/src/modules/authentication/helper/jwt-validation.adapter.ts similarity index 96% rename from apps/server/src/modules/authentication/strategy/jwt-validation.adapter.ts rename to apps/server/src/modules/authentication/helper/jwt-validation.adapter.ts index dee98747c46..6a67f555c14 100644 --- a/apps/server/src/modules/authentication/strategy/jwt-validation.adapter.ts +++ b/apps/server/src/modules/authentication/helper/jwt-validation.adapter.ts @@ -23,7 +23,6 @@ export class JwtValidationAdapter { * @param jti jwt id (here required to make jwt identifiers identical in redis) */ async isWhitelisted(accountId: string, jti: string): Promise { - // eslint-disable-next-line @typescript-eslint/no-unsafe-call await ensureTokenIsWhitelisted({ accountId, jti, privateDevice: false }); } diff --git a/apps/server/src/modules/authentication/interface/jwt-payload.ts b/apps/server/src/modules/authentication/interface/jwt-payload.ts index ca46acbe761..51747ccf2c0 100644 --- a/apps/server/src/modules/authentication/interface/jwt-payload.ts +++ b/apps/server/src/modules/authentication/interface/jwt-payload.ts @@ -12,8 +12,8 @@ export interface CreateJwtPayload { export interface JwtPayload extends CreateJwtPayload { /** audience */ aud: string; - /** expiration in // TODO - * + /** + * expiration in */ exp: number; iat: number; @@ -21,8 +21,8 @@ export interface JwtPayload extends CreateJwtPayload { iss: string; jti: string; - /** // TODO - * + /** + * subject */ sub: string; } diff --git a/apps/server/src/modules/authentication/interface/user.ts b/apps/server/src/modules/authentication/interface/user.ts index b4d43e138c8..6792e9585ab 100644 --- a/apps/server/src/modules/authentication/interface/user.ts +++ b/apps/server/src/modules/authentication/interface/user.ts @@ -1,3 +1,7 @@ +/* eslint-disable @typescript-eslint/no-unsafe-member-access */ +/* eslint-disable @typescript-eslint/no-unsafe-call */ +/* eslint-disable @typescript-eslint/no-explicit-any */ + import { EntityId } from '@shared/domain/types'; export interface ICurrentUser { @@ -19,3 +23,13 @@ export interface ICurrentUser { /** True if the user is an external user e.g. an oauth user or ldap user */ isExternalUser: boolean; } + +export function isICurrentUser(user: any): user is ICurrentUser { + return ( + typeof user?.userId === 'string' && + Array.isArray(user?.roles) && + (user?.roles as any[]).every((id: any) => typeof id === 'string') && + typeof user?.schoolId === 'string' && + typeof user?.accountId === 'string' + ); +} diff --git a/apps/server/src/modules/authentication/services/authentication.service.spec.ts b/apps/server/src/modules/authentication/services/authentication.service.spec.ts index baff54c7904..fffb83f72fc 100644 --- a/apps/server/src/modules/authentication/services/authentication.service.spec.ts +++ b/apps/server/src/modules/authentication/services/authentication.service.spec.ts @@ -7,7 +7,7 @@ import { JwtService } from '@nestjs/jwt'; import { Test, TestingModule } from '@nestjs/testing'; import jwt from 'jsonwebtoken'; import { BruteForceError } from '../errors/brute-force.error'; -import { JwtValidationAdapter } from '../strategy/jwt-validation.adapter'; +import { JwtValidationAdapter } from '../helper/jwt-validation.adapter'; import { AuthenticationService } from './authentication.service'; jest.mock('jsonwebtoken'); diff --git a/apps/server/src/modules/authentication/services/authentication.service.ts b/apps/server/src/modules/authentication/services/authentication.service.ts index 7b6fadfd9b1..ae92a3d3e6b 100644 --- a/apps/server/src/modules/authentication/services/authentication.service.ts +++ b/apps/server/src/modules/authentication/services/authentication.service.ts @@ -8,7 +8,7 @@ import { randomUUID } from 'crypto'; import jwt, { JwtPayload } from 'jsonwebtoken'; import { BruteForceError, UnauthorizedLoggableException } from '../errors'; import { CreateJwtPayload } from '../interface/jwt-payload'; -import { JwtValidationAdapter } from '../strategy/jwt-validation.adapter'; +import { JwtValidationAdapter } from '../helper/jwt-validation.adapter'; import { LoginDto } from '../uc/dto'; @Injectable() @@ -55,11 +55,15 @@ export class AuthenticationService { async removeJwtFromWhitelist(jwtToken: string): Promise { const decodedJwt: JwtPayload | null = jwt.decode(jwtToken, { json: true }); - if (decodedJwt && decodedJwt.jti && decodedJwt.accountId && typeof decodedJwt.accountId === 'string') { + if (this.isValidJwt(decodedJwt)) { await this.jwtValidationAdapter.removeFromWhitelist(decodedJwt.accountId, decodedJwt.jti); } } + private isValidJwt(decodedJwt: JwtPayload | null): decodedJwt is { accountId: string; jti: string } { + return typeof decodedJwt?.jti === 'string' && typeof decodedJwt?.accountId === 'string'; + } + checkBrutForce(account: Account): void { if (account.lasttriedFailedLogin) { const timeDifference = (new Date().getTime() - account.lasttriedFailedLogin.getTime()) / 1000; diff --git a/apps/server/src/modules/authentication/strategy/jwt.strategy.spec.ts b/apps/server/src/modules/authentication/strategy/jwt.strategy.spec.ts index e9f758db326..7e581164daf 100644 --- a/apps/server/src/modules/authentication/strategy/jwt.strategy.spec.ts +++ b/apps/server/src/modules/authentication/strategy/jwt.strategy.spec.ts @@ -9,7 +9,7 @@ import { setupEntities } from '@shared/testing'; import { jwtConstants } from '../constants'; import { JwtPayload } from '../interface/jwt-payload'; -import { JwtValidationAdapter } from './jwt-validation.adapter'; +import { JwtValidationAdapter } from '../helper/jwt-validation.adapter'; import { JwtStrategy } from './jwt.strategy'; describe('jwt strategy', () => { diff --git a/apps/server/src/modules/authentication/strategy/jwt.strategy.ts b/apps/server/src/modules/authentication/strategy/jwt.strategy.ts index 6133cf28a69..0dbe9e39bf4 100644 --- a/apps/server/src/modules/authentication/strategy/jwt.strategy.ts +++ b/apps/server/src/modules/authentication/strategy/jwt.strategy.ts @@ -5,8 +5,8 @@ import { jwtConstants } from '../constants'; import { ICurrentUser } from '../interface'; import { JwtPayload } from '../interface/jwt-payload'; import { CurrentUserMapper } from '../mapper'; -import { JwtExtractor } from './jwt-extractor'; -import { JwtValidationAdapter } from './jwt-validation.adapter'; +import { JwtExtractor } from '../helper/jwt-extractor'; +import { JwtValidationAdapter } from '../helper/jwt-validation.adapter'; @Injectable() export class JwtStrategy extends PassportStrategy(Strategy) { diff --git a/apps/server/src/modules/authentication/strategy/ldap.strategy.ts b/apps/server/src/modules/authentication/strategy/ldap.strategy.ts index d6f8bd7438c..d732d39d25c 100644 --- a/apps/server/src/modules/authentication/strategy/ldap.strategy.ts +++ b/apps/server/src/modules/authentication/strategy/ldap.strategy.ts @@ -30,7 +30,6 @@ export class LdapStrategy extends PassportStrategy(Strategy, 'ldap') { const { username, password, systemId, schoolId } = this.extractParamsFromRequest(request); const system: SystemEntity = await this.systemRepo.findById(systemId); - const school: LegacySchoolDo = await this.schoolRepo.findById(schoolId); if (!school.systems || !school.systems.includes(systemId)) { @@ -38,13 +37,11 @@ export class LdapStrategy extends PassportStrategy(Strategy, 'ldap') { } const account: Account = await this.loadAccount(username, system.id, school); - const userId: string = this.checkValue(account.userId); this.authenticationService.checkBrutForce(account); const user: User = await this.userRepo.findById(userId); - const ldapDn: string = this.checkValue(user.ldapDn); await this.checkCredentials(account, system, ldapDn, password); diff --git a/apps/server/src/modules/authentication/strategy/ws-jwt.strategy.spec.ts b/apps/server/src/modules/authentication/strategy/ws-jwt.strategy.spec.ts index ee1c0d6eacf..2361f2b48b9 100644 --- a/apps/server/src/modules/authentication/strategy/ws-jwt.strategy.spec.ts +++ b/apps/server/src/modules/authentication/strategy/ws-jwt.strategy.spec.ts @@ -7,7 +7,7 @@ import { WsException } from '@nestjs/websockets'; import { setupEntities } from '@shared/testing'; import { jwtConstants } from '../constants'; import { JwtPayload } from '../interface/jwt-payload'; -import { JwtValidationAdapter } from './jwt-validation.adapter'; +import { JwtValidationAdapter } from '../helper/jwt-validation.adapter'; import { WsJwtStrategy } from './ws-jwt.strategy'; describe('jwt strategy', () => { diff --git a/apps/server/src/modules/authentication/strategy/ws-jwt.strategy.ts b/apps/server/src/modules/authentication/strategy/ws-jwt.strategy.ts index 3cf508ce926..ef9bf54b67c 100644 --- a/apps/server/src/modules/authentication/strategy/ws-jwt.strategy.ts +++ b/apps/server/src/modules/authentication/strategy/ws-jwt.strategy.ts @@ -6,8 +6,8 @@ import { jwtConstants } from '../constants'; import { ICurrentUser } from '../interface'; import { JwtPayload } from '../interface/jwt-payload'; import { CurrentUserMapper } from '../mapper'; -import { JwtExtractor } from './jwt-extractor'; -import { JwtValidationAdapter } from './jwt-validation.adapter'; +import { JwtExtractor } from '../helper/jwt-extractor'; +import { JwtValidationAdapter } from '../helper/jwt-validation.adapter'; @Injectable() export class WsJwtStrategy extends PassportStrategy(Strategy, 'wsjwt') {