-
Notifications
You must be signed in to change notification settings - Fork 9
Submissions performance #173
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
Changes from all commits
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 |
|---|---|---|
|
|
@@ -179,14 +179,15 @@ export class ReviewSummationController { | |
| `Getting review summations with filters - ${JSON.stringify(queryDto)}`, | ||
| ); | ||
| const authUser: JwtUser = req['user'] as JwtUser; | ||
| const wantsTabSeparated = this.requestWantsTabSeparated(req); | ||
| const results = await this.service.searchSummation( | ||
| authUser, | ||
| queryDto, | ||
| paginationDto, | ||
| sortDto, | ||
| ); | ||
|
|
||
| if (!this.requestWantsTabSeparated(req)) { | ||
| if (!wantsTabSeparated) { | ||
| return results; | ||
| } | ||
|
|
||
|
|
@@ -199,7 +200,13 @@ export class ReviewSummationController { | |
| }); | ||
| } | ||
|
|
||
| const payload = this.buildReviewSummationTsv(results); | ||
| const completeResults = await this.loadAllReviewSummationsForExport( | ||
| authUser, | ||
| queryDto, | ||
| sortDto, | ||
| results, | ||
| ); | ||
| const payload = this.buildReviewSummationTsv(completeResults); | ||
| const safeChallengeSlug = this.buildFilenameSlug(challengeId); | ||
| const filename = `${ReviewSummationController.TSV_FILENAME_PREFIX}-${safeChallengeSlug}.tsv`; | ||
| res.setHeader( | ||
|
|
@@ -321,6 +328,28 @@ export class ReviewSummationController { | |
| }; | ||
| } | ||
|
|
||
| private async loadAllReviewSummationsForExport( | ||
| authUser: JwtUser, | ||
| queryDto: ReviewSummationQueryDto, | ||
| sortDto: SortDto | undefined, | ||
| initialResults: PaginatedResponse<ReviewSummationResponseDto>, | ||
| ): Promise<PaginatedResponse<ReviewSummationResponseDto>> { | ||
| const totalCount = | ||
| initialResults.meta?.totalCount ?? initialResults.data.length; | ||
| const currentCount = initialResults.data.length; | ||
|
|
||
| if (!Number.isFinite(totalCount) || totalCount <= currentCount) { | ||
|
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. [ |
||
| return initialResults; | ||
| } | ||
|
|
||
| return this.service.searchSummation( | ||
| authUser, | ||
| queryDto, | ||
| { page: 1, perPage: totalCount }, | ||
| sortDto, | ||
| ); | ||
| } | ||
|
|
||
| private requestWantsTabSeparated(req: Request): boolean { | ||
| const acceptHeader = Array.isArray(req.headers.accept) | ||
| ? req.headers.accept.join(',') | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,7 +8,7 @@ export const AuthConfig = { | |
| // JSON array string of allowed issuers | ||
| validIssuers: | ||
| process.env.VALID_ISSUERS || | ||
| '["https://testsachin.topcoder-dev.com/","https://test-sachin-rs256.auth0.com/","https://api.topcoder.com","https://api.topcoder-dev.com","https://topcoder-dev.auth0.com/", "https://auth.topcoder-dev.com/"]', | ||
| '["https://testsachin.topcoder-dev.com/","https://test-sachin-rs256.auth0.com/","https://api.topcoder.com","https://api.topcoder-dev.com","https://topcoder-dev.auth0.com/","https://auth.topcoder-dev.com/","https://topcoder.auth0.com/","https://auth.topcoder.com/"]', | ||
|
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. [❗❗ |
||
|
|
||
| // Legacy JWT configuration (kept for backward compatibility) | ||
| jwt: { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,21 +7,30 @@ import { | |
| SetMetadata, | ||
| } from '@nestjs/common'; | ||
| import { Reflector } from '@nestjs/core'; | ||
| import { Request } from 'express'; | ||
| import { JwtService } from '../modules/global/jwt.service'; | ||
| import { SCOPES_KEY } from '../decorators/scopes.decorator'; | ||
| import { LoggerService } from '../modules/global/logger.service'; | ||
| import { UserRole } from '../enums/userRole.enum'; | ||
|
|
||
| export const ROLES_KEY = 'roles'; | ||
| export const Roles = (...roles: UserRole[]) => SetMetadata(ROLES_KEY, roles); | ||
|
|
||
| @Injectable() | ||
| export class TokenRolesGuard implements CanActivate { | ||
| private readonly logger = LoggerService.forRoot(TokenRolesGuard.name); | ||
|
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. [ |
||
|
|
||
| constructor( | ||
| private reflector: Reflector, | ||
| private jwtService: JwtService, | ||
| ) {} | ||
|
|
||
| canActivate(context: ExecutionContext): boolean { | ||
| const handler = context.getHandler?.(); | ||
| const controllerClass = context.getClass?.(); | ||
| const controllerName = controllerClass?.name || 'UnknownController'; | ||
| const handlerName = handler?.name || 'UnknownHandler'; | ||
|
|
||
| // Get required roles and scopes from decorators | ||
| const requiredRoles = | ||
| this.reflector.get<UserRole[]>(ROLES_KEY, context.getHandler()) || []; | ||
|
|
@@ -31,18 +40,49 @@ export class TokenRolesGuard implements CanActivate { | |
|
|
||
| // If no roles or scopes are required, allow access | ||
| if (requiredRoles.length === 0 && requiredScopes.length === 0) { | ||
| this.logger.log({ | ||
| message: 'No roles or scopes required, allowing request', | ||
| controllerName, | ||
| handlerName, | ||
| }); | ||
| return true; | ||
| } | ||
|
|
||
| const request = context.switchToHttp().getRequest<Request>(); | ||
| const requestMeta = this.buildRequestLogMeta( | ||
| request, | ||
| controllerName, | ||
| handlerName, | ||
| ); | ||
|
|
||
| this.logger.log({ | ||
| message: 'Evaluating token roles guard', | ||
| ...requestMeta, | ||
| requiredRoles, | ||
| requiredScopes, | ||
| }); | ||
|
|
||
| try { | ||
| const user = request['user']; | ||
|
|
||
| if (!user && (requiredRoles.length || requiredScopes.length)) { | ||
| this.logger.warn({ | ||
| message: 'Rejecting request due to missing user payload', | ||
| ...requestMeta, | ||
| hasUser: false, | ||
| }); | ||
| throw new UnauthorizedException('Missing or invalid token!'); | ||
| } | ||
|
|
||
| this.logger.log({ | ||
| message: 'User payload extracted from request', | ||
| ...requestMeta, | ||
| userId: user?.userId, | ||
| isMachine: Boolean(user?.isMachine), | ||
| roles: user?.roles, | ||
| scopes: user?.scopes, | ||
| }); | ||
|
|
||
| const normalizedRequiredRoles = requiredRoles.map((role) => | ||
| String(role).trim().toLowerCase(), | ||
| ); | ||
|
|
@@ -51,10 +91,22 @@ export class TokenRolesGuard implements CanActivate { | |
| if (normalizedRequiredRoles.length > 0) { | ||
| const normalizedUserRoles = this.normalizeUserRoles(user.roles); | ||
|
|
||
| this.logger.log({ | ||
| message: 'Checking role-based permissions', | ||
| ...requestMeta, | ||
| normalizedRequiredRoles, | ||
| normalizedUserRoles, | ||
| }); | ||
|
|
||
| const hasRole = normalizedRequiredRoles.some((role) => | ||
| normalizedUserRoles.includes(role), | ||
| ); | ||
| if (hasRole) { | ||
| this.logger.log({ | ||
| message: 'Access granted via role-based authorization', | ||
| ...requestMeta, | ||
| normalizedRequiredRoles, | ||
| }); | ||
| return true; | ||
| } | ||
|
|
||
|
|
@@ -66,6 +118,13 @@ export class TokenRolesGuard implements CanActivate { | |
| user, | ||
| ) | ||
| ) { | ||
| this.logger.log({ | ||
| message: | ||
| 'Access granted via submission list challenge fallback rule', | ||
| ...requestMeta, | ||
| challengeId: request?.query?.challengeId, | ||
| userId: user?.userId, | ||
| }); | ||
| return true; | ||
| } | ||
| } | ||
|
|
@@ -75,7 +134,20 @@ export class TokenRolesGuard implements CanActivate { | |
| const hasScope = requiredScopes.some((scope) => | ||
| user.scopes ? user.scopes.includes(scope) : false, | ||
| ); | ||
|
|
||
| this.logger.log({ | ||
| message: 'Checking scope-based permissions', | ||
| ...requestMeta, | ||
| requiredScopes, | ||
| tokenScopes: user.scopes, | ||
| hasScope, | ||
| }); | ||
|
|
||
| if (hasScope) { | ||
| this.logger.log({ | ||
| message: 'Access granted via scope-based authorization', | ||
| ...requestMeta, | ||
| }); | ||
| return true; | ||
| } | ||
| } | ||
|
|
@@ -89,10 +161,22 @@ export class TokenRolesGuard implements CanActivate { | |
| requiredRoles.length > 0 && | ||
| requiredScopes.length === 0 | ||
| ) { | ||
| this.logger.warn({ | ||
| message: | ||
| 'M2M token detected without role permissions for role-only endpoint', | ||
| ...requestMeta, | ||
| tokenScopes: user.scopes, | ||
| }); | ||
| throw new ForbiddenException('M2M token not allowed for this endpoint'); | ||
| } | ||
|
|
||
| // Access denied - neither roles nor scopes match | ||
| this.logger.warn({ | ||
| message: 'Access denied due to insufficient permissions', | ||
| ...requestMeta, | ||
| requiredRoles, | ||
| requiredScopes, | ||
| }); | ||
| throw new ForbiddenException('Insufficient permissions'); | ||
| } catch (error) { | ||
| if ( | ||
|
|
@@ -101,6 +185,15 @@ export class TokenRolesGuard implements CanActivate { | |
| ) { | ||
| throw error; | ||
| } | ||
| this.logger.error( | ||
| { | ||
| message: 'Unexpected error validating token roles', | ||
| ...requestMeta, | ||
| error: | ||
| error instanceof Error ? error.message : String(error ?? 'error'), | ||
| }, | ||
| error instanceof Error ? error.stack : undefined, | ||
| ); | ||
| throw new UnauthorizedException('Invalid token'); | ||
| } | ||
| } | ||
|
|
@@ -192,4 +285,33 @@ export class TokenRolesGuard implements CanActivate { | |
|
|
||
| return Array.from(normalizedRoles); | ||
| } | ||
|
|
||
| private buildRequestLogMeta( | ||
| request: any, | ||
|
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. [ |
||
| controllerName: string, | ||
| handlerName: string, | ||
| ) { | ||
| const headers = (request?.headers || {}) as Record< | ||
| string, | ||
| string | string[] | undefined | ||
| >; | ||
| const correlationIdCandidate = | ||
| headers['x-request-id'] || | ||
| headers['x-correlation-id'] || | ||
| headers['x-trace-id']; | ||
| const method = | ||
| typeof request?.method === 'string' | ||
| ? request.method.toUpperCase() | ||
| : undefined; | ||
|
|
||
| return { | ||
| controllerName, | ||
| handlerName, | ||
| method, | ||
| path: request?.originalUrl || request?.url || request?.path, | ||
| correlationId: Array.isArray(correlationIdCandidate) | ||
| ? correlationIdCandidate[0] | ||
| : correlationIdCandidate, | ||
| }; | ||
| } | ||
| } | ||
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.
[⚠️
performance]The method
loadAllReviewSummationsForExportis called without checking ifcompleteResultsis needed. Consider checking iftotalCountis greater thancurrentCountbefore calling this method to avoid unnecessary database queries, which could improve performance.