Skip to content

Feat/auth #3

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

Closed
wants to merge 8 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
95 changes: 95 additions & 0 deletions .circleci/config.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
version: 2.1
defaults: &defaults
docker:
- image: cimg/python:3.13.5-browsers

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high
correctness
The Docker image cimg/python:3.13.5-browsers does not exist. Consider using a valid version or checking for typos in the version number.

install_dependency: &install_dependency
name: Installation of build and deployment dependencies.
command: |
pip3 install awscli --upgrade
install_deploysuite: &install_deploysuite
name: Installation of install_deploysuite.
command: |
git clone --branch v1.4.17 https://github.com/topcoder-platform/tc-deploy-scripts ../buildscript

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium
maintainability
Cloning a specific branch from a public repository without pinning to a commit hash can lead to unexpected changes if the branch is updated. Consider using a commit hash to ensure consistency.

cp ./../buildscript/master_deploy.sh .
cp ./../buildscript/buildenv.sh .
cp ./../buildscript/awsconfiguration.sh .
cp ./../buildscript/psvar-processor.sh .

restore_cache_settings_for_build: &restore_cache_settings_for_build
key: docker-node-modules-{{ checksum "pnpm-lock.yaml" }}

save_cache_settings: &save_cache_settings
key: docker-node-modules-{{ checksum "pnpm-lock.yaml" }}
paths:
- node_modules


builddeploy_steps: &builddeploy_steps
- checkout
- setup_remote_docker
- run: *install_dependency
- run: *install_deploysuite
- restore_cache: *restore_cache_settings_for_build
- run:
name: "Build docker image"
command: |
./build.sh
- save_cache: *save_cache_settings
- deploy:
name: Running MasterScript.
command: |
./awsconfiguration.sh $DEPLOY_ENV

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high
security
The script awsconfiguration.sh is executed with a variable $DEPLOY_ENV that is not validated within this context. Ensure that $DEPLOY_ENV is sanitized to prevent potential security issues.

source awsenvconf
./psvar-processor.sh -t appenv -p /config/${APPNAME}/deployvar
source deployvar_env
./master_deploy.sh -d ECS -e $DEPLOY_ENV -t latest -j /config/${APPNAME}/appvar -i ${APPNAME} -p FARGATE

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high
security
The master_deploy.sh script is executed with several environment variables. Ensure these variables are properly validated and sanitized to prevent injection vulnerabilities.

jobs:
# Build & Deploy against development backend
"build-dev":
!!merge <<: *defaults
environment:
DEPLOY_ENV: "DEV"
LOGICAL_ENV: "dev"
APPNAME: "tc-mcp"
steps: *builddeploy_steps

"build-qa":
!!merge <<: *defaults
environment:
DEPLOY_ENV: "QA"
LOGICAL_ENV: "qa"
APPNAME: "tc-mcp"
steps: *builddeploy_steps

"build-prod":
!!merge <<: *defaults
environment:
DEPLOY_ENV: "PROD"
LOGICAL_ENV: "prod"
APPNAME: "tc-mcp"
steps: *builddeploy_steps

workflows:
version: 2
build:
jobs:
# Development builds are executed on "develop" branch only.
- "build-dev":
context: org-global
filters:
branches:
only:
- dev

- "build-qa":
context: org-global
filters:
branches:
only:
- qa

- "build-prod":
context: org-global
filters:
branches:
only: master
2 changes: 1 addition & 1 deletion build.sh
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
#!/bin/bash
set -eo pipefail
docker buildx build --no-cache=true -t ${APPNAME}}:latest .
docker buildx build --no-cache=true -t ${APPNAME}:latest .

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high
correctness
There is a typo in the variable substitution syntax. The closing bracket should be removed. It should be ${APPNAME}:latest instead of ${APPNAME}}:latest.

2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@
"@nestjs/common": "^11.0.1",
"@nestjs/core": "^11.0.1",
"@nestjs/platform-express": "^11.0.1",
"@rekog/mcp-nest": "^1.6.2",
"class-transformer": "^0.5.1",
"class-validator": "^0.14.2",
"dotenv": "^16.5.0",
Expand All @@ -34,6 +33,7 @@
"nanoid": "^5.1.5",
"reflect-metadata": "^0.2.2",
"rxjs": "^7.8.1",
"@tc/mcp-nest": "topcoder-platform/MCP-Nest.git",
"zod": "^3.25.67"
},
"devDependencies": {
Expand Down
13 changes: 7 additions & 6 deletions pnpm-lock.yaml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion src/api/health-check/healthCheck.controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ export class HealthCheckController {

@Public()
@Version([VERSION_NEUTRAL, '1'])
@Get('/healthcheck')
@Get('/health')

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium
maintainability
The endpoint path has been changed from /healthcheck to /health. Ensure that any clients or documentation referencing the old path are updated accordingly to prevent potential disruptions.

healthCheck(): Promise<GetHealthCheckResponseDto> {
const response = new GetHealthCheckResponseDto();

Expand Down
25 changes: 5 additions & 20 deletions src/app.module.ts
Original file line number Diff line number Diff line change
@@ -1,14 +1,11 @@
import { MiddlewareConsumer, Module, NestModule } from '@nestjs/common';
import { McpModule } from '@rekog/mcp-nest';
import { McpModule } from '@tc/mcp-nest';

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high
correctness
The import path change from @rekog/mcp-nest to @tc/mcp-nest should be verified to ensure that the new module path is correct and that all dependencies are properly updated. Incorrect module paths can lead to runtime errors.

import { QueryChallengesTool } from './mcp/tools/challenges/queryChallenges.tool';
import { randomUUID } from 'crypto';
import { GlobalProvidersModule } from './shared/global/globalProviders.module';
import { TopcoderModule } from './shared/topcoder/topcoder.module';
import { HealthCheckController } from './api/health-check/healthCheck.controller';
import { TokenValidatorMiddleware } from './core/auth/middleware/tokenValidator.middleware';
import { CreateRequestStoreMiddleware } from './core/request/createRequestStore.middleware';
import { AuthGuard, RolesGuard } from './core/auth/guards';
import { APP_GUARD } from '@nestjs/core';
import { nanoid } from 'nanoid';

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium
security
The change from randomUUID to nanoid should be reviewed to ensure that nanoid provides the necessary uniqueness and security properties required by the application. If randomUUID was chosen for its cryptographic properties, ensure nanoid is a suitable replacement.


@Module({
imports: [
Expand All @@ -17,30 +14,18 @@ import { APP_GUARD } from '@nestjs/core';
version: '1.0.0',
streamableHttp: {
enableJsonResponse: false,
sessionIdGenerator: () => randomUUID(),
sessionIdGenerator: () => nanoid(),

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high
security
Switching from randomUUID() to nanoid() for session ID generation may have implications on the uniqueness and security of the IDs. Ensure nanoid() provides sufficient entropy and uniqueness for your use case.

statelessMode: false,
},
// guards: [AuthGuard, RolesGuard],
}),
GlobalProvidersModule,
TopcoderModule,
],
controllers: [HealthCheckController],
providers: [
// {
// provide: APP_GUARD,
// useClass: AuthGuard,
// },
// {
// provide: APP_GUARD,
// useClass: RolesGuard,
// },
QueryChallengesTool,
],
providers: [QueryChallengesTool],
})
export class AppModule implements NestModule {
configure(consumer: MiddlewareConsumer) {
// consumer.apply(TokenValidatorMiddleware).forRoutes('*');
// consumer.apply(CreateRequestStoreMiddleware).forRoutes('*');
consumer.apply(TokenValidatorMiddleware).forRoutes('*');

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium
correctness
Re-enabling TokenValidatorMiddleware without CreateRequestStoreMiddleware might lead to issues if the latter is required for proper request handling. Verify if CreateRequestStoreMiddleware is necessary for the application's functionality.

}
}
4 changes: 4 additions & 0 deletions src/config/config.env.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,4 +16,8 @@ export class ConfigEnv {

@IsString()
AUTH0_CLIENT_ID!: string;

@IsString()
@IsOptional()
API_BASE = '/v6/mcp';

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium
maintainability
Consider using a constant or configuration file for the default value of API_BASE to enhance maintainability and avoid hardcoding paths directly in the class.

}
5 changes: 4 additions & 1 deletion src/core/auth/auth.constants.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
export enum Role {
Admin = 'administrator',

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium
maintainability
Consider using a consistent naming convention for enum values. The Admin value uses a different naming style compared to User. This could lead to confusion and maintenance challenges in the future.

User = 'Topcoder User',
}

export enum M2mScope {}
export enum M2mScope {
QueryPublicChallenges = 'query:public:challenges',
}
1 change: 0 additions & 1 deletion src/core/auth/decorators/index.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
export * from './m2m.decorator';
export * from './m2mScope.decorator';
export * from './public.decorator';
export * from './roles.decorator';
export * from './user.decorator';
5 changes: 0 additions & 5 deletions src/core/auth/decorators/roles.decorator.ts

This file was deleted.

71 changes: 14 additions & 57 deletions src/core/auth/guards/auth.guard.ts
Original file line number Diff line number Diff line change
@@ -1,59 +1,16 @@
import {
CanActivate,
ExecutionContext,
Injectable,
UnauthorizedException,
} from '@nestjs/common';
import { Reflector } from '@nestjs/core';
import { IS_PUBLIC_KEY } from '../decorators/public.decorator';
import { IS_M2M_KEY } from '../decorators/m2m.decorator';
import { M2mScope } from '../auth.constants';
import { SCOPES_KEY } from '../decorators/m2mScope.decorator';

@Injectable()
export class AuthGuard implements CanActivate {
constructor(private reflector: Reflector) {}

canActivate(context: ExecutionContext): boolean {
const isPublic = this.reflector.getAllAndOverride<boolean>(IS_PUBLIC_KEY, [
context.getHandler(),
context.getClass(),
]);

if (isPublic) return true;

const req = context.switchToHttp().getRequest();
const isM2M = this.reflector.getAllAndOverride<boolean>(IS_M2M_KEY, [
context.getHandler(),
context.getClass(),
]);

const { m2mUserId } = req;
if (m2mUserId) {
req.user = {
id: m2mUserId,
handle: '',
};
}

// Regular authentication - check that we have user's email and have verified the id token
if (!isM2M) {
return Boolean(req.email && req.idTokenVerified);
}

// M2M authentication - check scopes
if (!req.idTokenVerified || !req.m2mTokenScope)
throw new UnauthorizedException();

const allowedM2mScopes = this.reflector.getAllAndOverride<M2mScope[]>(
SCOPES_KEY,
[context.getHandler(), context.getClass()],
);

const reqScopes = req.m2mTokenScope.split(' ');
if (reqScopes.some((reqScope) => allowedM2mScopes.includes(reqScope))) {
return true;
}
import { Request } from 'express';
import { decodeAuthToken } from './guards.utils';

/**
* Auth guard function to validate the authorization token from the request headers.
*
* @param req - The incoming HTTP request object.
* @returns A promise that resolves to `true` if the authorization token is valid, otherwise `false`.
*/
export const authGuard = async (req: Request) => {
if (!(await decodeAuthToken(req.headers.authorization ?? ''))) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high
correctness
The use of await without a try-catch block could lead to unhandled promise rejections if decodeAuthToken throws an error. Consider wrapping this in a try-catch to handle potential errors gracefully.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium
correctness
The use of req.headers.authorization ?? '' assumes that an empty string is a valid fallback. If decodeAuthToken does not handle empty strings appropriately, this could lead to unexpected behavior. Ensure that decodeAuthToken can handle this case or provide a more suitable default value.

return false;
}
}

return true;
};
35 changes: 35 additions & 0 deletions src/core/auth/guards/guards.utils.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
import * as jwt from 'jsonwebtoken';
import { Logger } from 'src/shared/global';
import { getSigningKey } from '../jwt';

const logger = new Logger('guards.utils()');

/**
* Decodes and verifies a JWT token from the provided authorization header.
*
* @param authHeader - The authorization header containing the token, expected in the format "Bearer <token>".
* @returns A promise that resolves to the decoded JWT payload if the token is valid,
* a string if the payload is a string, or `false` if the token is invalid or the header is improperly formatted.
*
* @throws This function does not throw directly but will return `false` if an error occurs during verification.
*/
export const decodeAuthToken = async (
authHeader: string,
): Promise<boolean | jwt.JwtPayload | string> => {
const [type, idToken] = authHeader?.split(' ') ?? [];

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

low
readability
Consider using a more descriptive variable name than idToken to avoid confusion with ID tokens, as this variable represents a JWT access token.


if (type !== 'Bearer' || !idToken) {
return false;
}

let decoded: jwt.JwtPayload | string;
try {
const signingKey = await getSigningKey(idToken);
decoded = jwt.verify(idToken, signingKey);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium
maintainability
The jwt.verify function can throw errors for various reasons, including token expiration and invalid signature. Consider handling specific error types to provide more informative logging and potentially different handling strategies.

} catch (error) {
logger.error('Error verifying JWT', error);
return false;
}

return decoded;
};
3 changes: 2 additions & 1 deletion src/core/auth/guards/index.ts
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
export * from './auth.guard';
export * from './roles.guard';
export * from './m2m-scope.guard';

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high
correctness
Ensure that the new m2m-scope.guard module is correctly implemented and tested, as it replaces the previous roles.guard. This change could impact the authorization logic if not properly handled.

export * from './role.guard';

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high
security
Verify that the new role.guard module is correctly integrated and does not introduce any regressions or security issues. Ensure it is thoroughly tested.

30 changes: 30 additions & 0 deletions src/core/auth/guards/m2m-scope.guard.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
import { Request } from 'express';
import { decodeAuthToken } from './guards.utils';
import { JwtPayload } from 'jsonwebtoken';
import { M2mScope } from '../auth.constants';

/**
* A utility function to check if the required M2M (Machine-to-Machine) scopes are present
* in the authorization token provided in the request headers.
*
* @param {...M2mScope[]} requiredM2mScopes - The list of required M2M scopes to validate against.
* @returns {Promise<(req: Request) => boolean>} A function that takes an Express `Request` object
* and returns a boolean indicating whether the required scopes are present.
*
* The function decodes the authorization token from the request headers and checks if
* the required scopes are included in the token's scope claim.
*/
export const checkM2MScope =
(...requiredM2mScopes: M2mScope[]) =>
async (req: Request) => {
const decodedAuth = await decodeAuthToken(req.headers.authorization ?? '');

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high
correctness
Consider handling the case where decodeAuthToken might throw an error, such as when the token is invalid or malformed. This will prevent unhandled promise rejections and improve the robustness of the function.


const authorizedScopes = ((decodedAuth as JwtPayload).scope ?? '').split(

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium
correctness
The scope property is assumed to be a string. Ensure that decodedAuth is always a JwtPayload and that scope is a string to avoid runtime errors. Consider adding type checks or assertions.

' ',
);
if (!requiredM2mScopes.some((scope) => authorizedScopes.includes(scope))) {
return false;
}

return true;
};
Loading