-
Notifications
You must be signed in to change notification settings - Fork 3.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(api): ApiKey auth guard performance #4972
Changes from 13 commits
fccce09
2de1938
47444a3
bb79d6f
aa91a0f
277963e
36c79c9
798fea5
d533922
4464446
68d2abb
3460070
4409858
49b7290
96ec1a9
e1d276c
8bce71e
499fd1f
81ef5ac
48a6805
304b8a0
4df3d58
01792a0
e8c2fc8
2437f34
fc67c8d
7593079
1189fbf
a521eb7
552c383
a381571
38b67cd
fef2d12
f71ac75
6ba5265
d9d1b18
973cb58
139e860
8762ba2
c50321b
7686063
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -540,6 +540,7 @@ | |
"Ratelimit", | ||
"stdev", | ||
"Stdev", | ||
"headerapikey", | ||
], | ||
"flagWords": [], | ||
"patterns": [ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,18 +13,6 @@ export class RolesGuard implements CanActivate { | |
return true; | ||
} | ||
|
||
const request = context.switchToHttp().getRequest(); | ||
if (!request.headers.authorization) return false; | ||
|
||
const token = request.headers.authorization.split(' ')[1]; | ||
if (!token) return false; | ||
|
||
const authorizationHeader = request.headers.authorization; | ||
if (!authorizationHeader?.includes('ApiKey')) { | ||
const user = jwt.decode(token) as IJwtPayload; | ||
if (!user) return false; | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is handled in the Authentication flow, and shouldn't be part of Authorization. |
||
/* | ||
* TODO: The roles check implementation is currently not enabled | ||
* As we are not using roles in the system at this point | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,14 +13,7 @@ export class RootEnvironmentGuard implements CanActivate { | |
|
||
async canActivate(context: ExecutionContext) { | ||
const request = context.switchToHttp().getRequest(); | ||
if (!request.headers.authorization) return false; | ||
|
||
const token = request.headers.authorization.split(' ')[1]; | ||
if (!token) return false; | ||
|
||
const user = jwt.decode(token) as IJwtPayload; | ||
if (!user) return false; | ||
if (!user.environmentId) return false; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As above, this is handled in the Authentication flow, and shouldn't be part of Authorization. |
||
const user = request.user; | ||
|
||
const environment = await this.authService.isRootEnvironment(user); | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
import { HeaderAPIKeyStrategy } from 'passport-headerapikey'; | ||
import { PassportStrategy } from '@nestjs/passport'; | ||
import { Injectable, Logger } from '@nestjs/common'; | ||
import { AuthService } from '@novu/application-generic'; | ||
import { IJwtPayload } from '@novu/shared'; | ||
|
||
@Injectable() | ||
export class ApiKeyStrategy extends PassportStrategy(HeaderAPIKeyStrategy) { | ||
constructor(private readonly authService: AuthService) { | ||
super( | ||
{ header: 'Authorization', prefix: 'ApiKey ' }, | ||
true, | ||
(apikey: string, verified: (err: Error | null, user?: IJwtPayload | false) => void) => { | ||
this.authService.validateApiKey(apikey).then((user) => { | ||
rifont marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if (!user) { | ||
return verified(null, false); | ||
} | ||
|
||
return verified(null, user); | ||
}); | ||
} | ||
); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,11 +2,11 @@ import { ExtractJwt, Strategy } from 'passport-jwt'; | |
import { PassportStrategy } from '@nestjs/passport'; | ||
import { Injectable, UnauthorizedException } from '@nestjs/common'; | ||
import { IJwtPayload } from '@novu/shared'; | ||
import { AuthService, Instrument, PinoLogger } from '@novu/application-generic'; | ||
import { AuthService, Instrument } from '@novu/application-generic'; | ||
|
||
@Injectable() | ||
export class JwtStrategy extends PassportStrategy(Strategy) { | ||
constructor(private readonly authService: AuthService, private logger: PinoLogger) { | ||
constructor(private readonly authService: AuthService) { | ||
super({ | ||
jwtFromRequest: ExtractJwt.fromAuthHeaderAsBearerToken(), | ||
secretOrKey: process.env.JWT_SECRET, | ||
|
@@ -20,12 +20,6 @@ export class JwtStrategy extends PassportStrategy(Strategy) { | |
throw new UnauthorizedException(); | ||
} | ||
|
||
this.logger.assign({ | ||
userId: user._id, | ||
environmentId: payload.environmentId, | ||
organizationId: payload.organizationId, | ||
}); | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is now handled in a single place in the Auth Guard, for all Auth strategies. |
||
return payload; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,7 +12,6 @@ import { EvaluateApiRateLimit, EvaluateApiRateLimitCommand } from '../usecases/e | |
import { Reflector } from '@nestjs/core'; | ||
import { FeatureFlagCommand, GetIsApiRateLimitingEnabled } from '@novu/application-generic'; | ||
import { ApiRateLimitCategoryEnum, ApiRateLimitCostEnum, IJwtPayload } from '@novu/shared'; | ||
import * as jwt from 'jsonwebtoken'; | ||
import { ThrottlerCost, ThrottlerCategory } from './throttler.decorator'; | ||
|
||
export enum RateLimitHeaderKeysEnum { | ||
|
@@ -175,8 +174,7 @@ export class ApiRateLimitInterceptor extends ThrottlerGuard implements NestInter | |
|
||
private getReqUser(context: ExecutionContext): IJwtPayload { | ||
const req = context.switchToHttp().getRequest(); | ||
const token = req.headers[RateLimitHeaderKeysEnum.AUTHORIZATION.toLowerCase()].split(' ')[1]; | ||
|
||
return jwt.decode(token); | ||
return req.user; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,7 +15,6 @@ import { CacheService } from '@novu/application-generic'; | |
import { Observable, of, throwError } from 'rxjs'; | ||
import { catchError, map } from 'rxjs/operators'; | ||
import { createHash } from 'crypto'; | ||
import * as jwt from 'jsonwebtoken'; | ||
import { IJwtPayload } from '@novu/shared'; | ||
|
||
const LOG_CONTEXT = 'IdempotencyInterceptor'; | ||
|
@@ -93,26 +92,22 @@ export class IdempotencyInterceptor implements NestInterceptor { | |
return request.headers[HEADER_KEYS.IDEMPOTENCY_KEY.toLocaleLowerCase()]; | ||
} | ||
|
||
private getReqUser(context: ExecutionContext): IJwtPayload | null { | ||
private getReqUser(context: ExecutionContext): IJwtPayload { | ||
const req = context.switchToHttp().getRequest(); | ||
if (req?.user?.organizationId) { | ||
return req.user; | ||
} | ||
if (req.headers?.authorization?.length) { | ||
const token = req.headers.authorization.split(' ')[1]; | ||
if (token) { | ||
return jwt.decode(token); | ||
} | ||
} | ||
|
||
return null; | ||
return req.user; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As above, |
||
} | ||
|
||
private getCacheKey(context: ExecutionContext): string { | ||
const { organizationId } = this.getReqUser(context) || {}; | ||
const user = this.getReqUser(context); | ||
if (user === undefined) { | ||
const message = 'Cannot build cache key without user'; | ||
rifont marked this conversation as resolved.
Show resolved
Hide resolved
|
||
Logger.error(message, LOG_CONTEXT); | ||
throw new InternalServerErrorException(message); | ||
} | ||
const env = process.env.NODE_ENV; | ||
|
||
return `${env}-${organizationId}-${this.getIdempotencyKey(context)}`; | ||
return `${env}-${user.organizationId}-${this.getIdempotencyKey(context)}`; | ||
} | ||
|
||
async setCache( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,8 @@ | ||
import { createParamDecorator, UnauthorizedException } from '@nestjs/common'; | ||
import jwt from 'jsonwebtoken'; | ||
import { | ||
InternalServerErrorException, | ||
Logger, | ||
createParamDecorator, | ||
} from '@nestjs/common'; | ||
|
||
// eslint-disable-next-line @typescript-eslint/naming-convention | ||
export const UserSession = createParamDecorator((data, ctx) => { | ||
|
@@ -11,28 +14,12 @@ export const UserSession = createParamDecorator((data, ctx) => { | |
} | ||
|
||
if (req.user) { | ||
/** | ||
* This helps with sentry and other tools that need to know who the user is based on `id` property. | ||
*/ | ||
req.user.id = req.user._id; | ||
req.user.username = (req.user.firstName || '').trim(); | ||
req.user.domain = req.user.email?.split('@')[1]; | ||
|
||
return req.user; | ||
} | ||
|
||
if (req.headers) { | ||
if (req.headers.authorization) { | ||
const tokenParts = req.headers.authorization.split(' '); | ||
if (tokenParts[0] !== 'Bearer') | ||
throw new UnauthorizedException('bad_token'); | ||
if (!tokenParts[1]) throw new UnauthorizedException('bad_token'); | ||
|
||
const user = jwt.decode(tokenParts[1]); | ||
|
||
return user; | ||
} | ||
} | ||
|
||
return null; | ||
Logger.error( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As above, this is handled in the Authentication flow, and shouldn't be part of Authorization. If we don't have a |
||
'Attempted to access user session without a user in the request. You probably forgot to add the AuthGuard', | ||
'UserSession' | ||
); | ||
throw new InternalServerErrorException(); | ||
}); |
rifont marked this conversation as resolved.
Show resolved
Hide resolved
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,54 +1,75 @@ | ||
import { | ||
ExecutionContext, | ||
forwardRef, | ||
Inject, | ||
Injectable, | ||
Logger, | ||
UnauthorizedException, | ||
} from '@nestjs/common'; | ||
import { AuthGuard } from '@nestjs/passport'; | ||
import { AuthGuard, IAuthModuleOptions } from '@nestjs/passport'; | ||
import { Reflector } from '@nestjs/core'; | ||
import { AuthService } from './auth.service'; | ||
import { Instrument } from '../../instrumentation'; | ||
import { PinoLogger } from 'nestjs-pino'; | ||
rifont marked this conversation as resolved.
Show resolved
Hide resolved
|
||
import { IJwtPayload } from '@novu/shared'; | ||
|
||
@Injectable() | ||
export class JwtAuthGuard extends AuthGuard('jwt') { | ||
export class JwtAuthGuard extends AuthGuard(['jwt', 'headerapikey']) { | ||
private readonly reflector: Reflector; | ||
|
||
constructor( | ||
@Inject(forwardRef(() => AuthService)) private authService: AuthService | ||
) { | ||
constructor(private logger: PinoLogger) { | ||
super(); | ||
this.reflector = new Reflector(); | ||
} | ||
|
||
@Instrument() | ||
async canActivate(context: ExecutionContext) { | ||
getAuthenticateOptions(context: ExecutionContext): IAuthModuleOptions<any> { | ||
const request = context.switchToHttp().getRequest(); | ||
const authorizationHeader = request.headers.authorization; | ||
const authScheme = authorizationHeader.split(' ')[0]; | ||
request.authScheme = authScheme; | ||
|
||
if (authorizationHeader) { | ||
const authScheme = authorizationHeader.split(' ')[0]; | ||
request.authScheme = authScheme; | ||
} | ||
switch (authScheme) { | ||
case 'Bearer': | ||
return { | ||
session: false, | ||
defaultStrategy: 'jwt', | ||
rifont marked this conversation as resolved.
Show resolved
Hide resolved
|
||
}; | ||
case 'ApiKey': | ||
const apiEnabled = this.reflector.get<boolean>( | ||
'external_api_accessible', | ||
context.getHandler() | ||
); | ||
if (!apiEnabled) | ||
throw new UnauthorizedException('API endpoint not available'); | ||
|
||
if (authorizationHeader && authorizationHeader.includes('ApiKey')) { | ||
const apiEnabled = this.reflector.get<boolean>( | ||
'external_api_accessible', | ||
context.getHandler() | ||
); | ||
if (!apiEnabled) | ||
throw new UnauthorizedException('API endpoint not available'); | ||
return { | ||
session: false, | ||
defaultStrategy: 'headerapikey', | ||
}; | ||
default: | ||
throw new UnauthorizedException('Invalid auth scheme'); | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Handling invocation different authentication schemes passport strategies with an authScheme switch statement |
||
|
||
const key = authorizationHeader.split(' ')[1]; | ||
handleRequest<TUser = IJwtPayload>( | ||
rifont marked this conversation as resolved.
Show resolved
Hide resolved
|
||
err: any, | ||
user: any, | ||
info: any, | ||
context: ExecutionContext, | ||
status?: any | ||
): TUser { | ||
const request = context.switchToHttp().getRequest(); | ||
|
||
// eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
return this.authService.apiKeyAuthenticate(key).then<any>((result) => { | ||
request.headers.authorization = `Bearer ${result}`; | ||
this.logger.assign({ | ||
userId: user._id, | ||
environmentId: user.environmentId, | ||
organizationId: user.organizationId, | ||
authScheme: request.authScheme, | ||
}); | ||
|
||
return true; | ||
}); | ||
} | ||
/** | ||
* This helps with sentry and other tools that need to know who the user is based on `id` property. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🚀 |
||
*/ | ||
user.id = user._id; | ||
user.username = (user.firstName || '').trim(); | ||
user.domain = user.email?.split('@')[1]; | ||
|
||
return super.canActivate(context); | ||
return user; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moving the passport strategies out to where they belong.