Conversation
…rvice and fix a bug in the copilots portal
…ssions issue when challenge-api-v6 calls into the project service when launching a challenge.
What was broken - PM users and the opportunity creator were not receiving emails when a copilot applied to an opportunity. Root cause - Copilot notification email publishing was stubbed out in CopilotNotificationService.publishEmail, so no external.action.email events were sent. - Apply recipient resolution diverged from legacy behavior by using project-member managers and copilot-request creator instead of global Project Manager role users plus the opportunity creator. What was changed - Re-enabled copilot email dispatch by publishing to external.action.email through EventBusService. - Added MemberService.getRoleSubjects(roleName) to resolve Identity role subjects. - Updated apply notification recipients to include all Project Manager role users and opportunity.createdBy, with email deduplication. Any added/updated tests - Added src/api/copilot/copilot-notification.service.spec.ts to verify apply notifications go to PM role users + creator, deduplicate recipients, and do not throw if event publishing fails.
What was broken - Invite email target resolution depended only on Identity API email lookups. - When those lookups failed, existing Topcoder members invited by email were treated as non-members and routed through the registration-template path. Root cause - had no fallback source for unresolved emails and silently returned no user matches. What was changed - Updated to keep Identity API as primary lookup and fallback unresolved emails to Member API email search (). - Mapped Member API records into shape and deduplicated merged results. - Added safe primitive-string normalization helpers used during response parsing. Any added/updated tests - Added with coverage for Identity API success and Member API fallback paths.
…sers What was broken:\nInvite emails for existing Topcoder users could render the register CTA instead of the join/decline flow.\n\nRoot cause (if identifiable):\nprojects-api-v6 always sent a single SendGrid invite template id and relied on template-side conditional logic, so known-vs-unknown invite behavior was not enforced by the API.\n\nWhat was changed:\n- Added explicit invite template resolution in EmailService using dedicated env vars for known and unknown users.\n- Known users now use SENDGRID_PROJECT_INVITATION_KNOWN_USER_TEMPLATE_ID.\n- Unknown emails now use SENDGRID_PROJECT_INVITATION_UNKNOWN_USER_TEMPLATE_ID.\n- Preserved backward compatibility by falling back to SENDGRID_TEMPLATE_PROJECT_MEMBER_INVITED when dedicated vars are unset.\n- Updated .env.example and README environment variable docs.\n\nAny added/updated tests:\n- Added src/shared/services/email.service.spec.ts with coverage for known-user template selection, unknown-user template selection, legacy fallback, and no-template skip behavior.\n- Re-ran existing project-invite unit tests to confirm isSSO routing still works.
What was broken: When dedicated known/unknown invite template IDs were unset and the service fell back to SENDGRID_TEMPLATE_PROJECT_MEMBER_INVITED, it still sent the new payload shape instead of the legacy payload expected by that template. Root cause (if identifiable): Template ID fallback remained in place after payload formatting was split for the new templates, but legacy payload compatibility (including sections[].isSSO) was not preserved. What was changed: - Added legacy-template detection in EmailService when resolved template ID matches SENDGRID_TEMPLATE_PROJECT_MEMBER_INVITED. - Restored legacy payload construction for that path, including work/accounts URLs, subject/title fields, and sections[].isSSO. - Kept dedicated known/unknown template payloads unchanged. Any added/updated tests: - Updated EmailService fallback test to validate legacy payload structure and isSSO propagation when legacy template fallback is used.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| return []; | ||
| } | ||
|
|
||
| // TODO (security + performance): cache the Salesforce access token until its expiry to reduce token surface area and latency. |
There was a problem hiding this comment.
[performance]
Consider caching the Salesforce access token until its expiry to reduce the token surface area and improve performance by reducing authentication requests.
| @@ -226,6 +295,7 @@ export class BillingAccountService { | |||
| const privateKey = this.normalizePrivateKey( | |||
| process.env.SALESFORCE_CLIENT_KEY || '', | |||
| ); | |||
| // TODO: parse and cache the private KeyObject at construction time. | |||
There was a problem hiding this comment.
[performance]
Parsing and caching the private KeyObject at construction time could improve performance by avoiding repeated parsing of the key during each authentication request.
|
|
||
| expect(topic).toBe('external.action.email'); | ||
| expect(normalizedPayload.sendgrid_template_id).toBe('known-template-id'); | ||
| expect(normalizedPayload.recipients).toEqual(['knownuser@topcoder.com']); |
There was a problem hiding this comment.
[correctness]
The email address in expect(normalizedPayload.recipients).toEqual(['knownuser@topcoder.com']); should be in lowercase to match the input email KnownUser@topcoder.com. Consider normalizing email addresses to lowercase to avoid potential mismatches.
| expect(normalizedPayload.sendgrid_template_id).toBe('unknown-template-id'); | ||
| expect(normalizedPayload.data).toEqual({ | ||
| projectName: 'Demo', | ||
| firstName: '', |
There was a problem hiding this comment.
[correctness]
The firstName and lastName fields are set to empty strings when isSSO=false. Ensure this behavior is intentional and that the system can handle empty names appropriately.
| }); | ||
|
|
||
| it('skips publish when invite id is missing', async () => { | ||
| process.env.SENDGRID_PROJECT_INVITATION_KNOWN_USER_TEMPLATE_ID = |
There was a problem hiding this comment.
[design]
The test case it('skips publish when invite id is missing', async () => { ... }); assumes that the absence of an id field in the invite object should prevent publishing. Verify that this behavior aligns with the intended business logic.
| ): Promise<void> { | ||
| const recipient = invite.email?.trim().toLowerCase(); | ||
| // TODO: add basic email format validation before publishing the event. |
There was a problem hiding this comment.
[correctness]
Consider implementing basic email format validation here to ensure the email address is valid before proceeding. This will prevent invalid email addresses from being processed further.
| projectId: string, | ||
| action: 'accepted' | 'refused', | ||
| ): string { | ||
| const workManagerUrl = process.env.WORK_MANAGER_URL?.trim(); |
There was a problem hiding this comment.
[correctness]
The function returns an empty string if WORK_MANAGER_URL is not configured. Consider throwing an error or handling this case more explicitly to avoid potential issues with empty URLs being used elsewhere.
| @Injectable() | ||
| export class FileService { | ||
| private readonly logger = LoggerService.forRoot('FileService'); | ||
| private readonly client: S3Client; | ||
|
|
||
| constructor() { | ||
| // TODO: consider explicitly setting the AWS region via APP_CONFIG or env var to avoid region resolution failures. |
There was a problem hiding this comment.
[correctness]
Consider explicitly setting the AWS region in the S3Client constructor using APP_CONFIG or an environment variable to avoid potential region resolution failures.
| async getPresignedDownloadUrl(bucket: string, key: string): Promise<string> { | ||
| // TODO: validate that bucket and key values are within expected prefixes before issuing S3 commands. |
There was a problem hiding this comment.
[❗❗ security]
Validate that bucket and key values are within expected prefixes before issuing S3 commands to prevent unauthorized access or incorrect operations.
| async getPresignedDownloadUrl(bucket: string, key: string): Promise<string> { | ||
| // TODO: validate that bucket and key values are within expected prefixes before issuing S3 commands. | ||
| // TODO: review whether 3600s expiry is appropriate for the attachment use case; consider a shorter window. |
There was a problem hiding this comment.
[security]
Review whether the default URL expiry of 3600 seconds is appropriate for the attachment use case; consider a shorter window to enhance security.
| async transferFile( | ||
| sourceBucket: string, | ||
| sourceKey: string, | ||
| destBucket: string, | ||
| destKey: string, | ||
| ): Promise<void> { | ||
| // TODO: validate that bucket and key values are within expected prefixes before issuing S3 commands. |
There was a problem hiding this comment.
[❗❗ security]
Validate that sourceBucket, sourceKey, destBucket, and destKey values are within expected prefixes before issuing S3 commands to prevent unauthorized access or incorrect operations.
| async transferFile( | ||
| sourceBucket: string, | ||
| sourceKey: string, | ||
| destBucket: string, | ||
| destKey: string, | ||
| ): Promise<void> { | ||
| // TODO: validate that bucket and key values are within expected prefixes before issuing S3 commands. | ||
| // TODO: consider wrapping S3 errors in a domain-specific exception for consistent error handling across the service. |
There was a problem hiding this comment.
[maintainability]
Consider wrapping S3 errors in a domain-specific exception for consistent error handling across the service, which can improve maintainability and clarity.
| async deleteFile(bucket: string, key: string): Promise<void> { | ||
| // TODO: validate that bucket and key values are within expected prefixes before issuing S3 commands. |
There was a problem hiding this comment.
[❗❗ security]
Validate that bucket and key values are within expected prefixes before issuing S3 commands to prevent unauthorized access or incorrect operations.
| async deleteFile(bucket: string, key: string): Promise<void> { | ||
| // TODO: validate that bucket and key values are within expected prefixes before issuing S3 commands. | ||
| // TODO: consider wrapping S3 errors in a domain-specific exception for consistent error handling across the service. |
There was a problem hiding this comment.
[maintainability]
Consider wrapping S3 errors in a domain-specific exception for consistent error handling across the service, which can improve maintainability and clarity.
| httpServiceMock.get.mockImplementation( | ||
| (url: string, options: { params?: { email?: string } }) => { | ||
| if (url === 'https://identity.test/users') { | ||
| return throwError(() => new Error('identity lookup failed')); |
There was a problem hiding this comment.
[maintainability]
Using throwError with a function that returns an Error object is correct, but consider providing more context in the error message to aid debugging. For example, include the email or other relevant details that caused the failure.
| } | ||
|
|
||
| if ( | ||
| url === 'https://member.test' && |
There was a problem hiding this comment.
[💡 maintainability]
The mock implementation for the memberApiUrl uses hardcoded email addresses. Consider parameterizing these values to make the test more flexible and easier to maintain.
| const identityApiBaseUrl = this.identityApiUrl.replace(/\/$/, ''); | ||
| const responses = await Promise.all( | ||
| emails.map((email) => | ||
| // TODO: URL-encode the email value in the filter param to prevent query string injection. |
There was a problem hiding this comment.
[❗❗ security]
The email value in the filter parameter should be URL-encoded to prevent query string injection. This is a security concern and should be addressed.
| ).catch(() => null), | ||
| ), | ||
| ); | ||
| // TODO: consider batching or throttling parallel email lookups. |
There was a problem hiding this comment.
[performance]
Consider implementing batching or throttling for parallel email lookups to avoid potential performance bottlenecks or rate limiting issues with the API.
| } | ||
|
|
||
| if (url === 'https://identity.test/roles/102') { | ||
| return throwError(() => new Error('role lookup failed')); |
There was a problem hiding this comment.
[maintainability]
Consider handling the error more gracefully instead of throwing a generic error. This could improve the maintainability and debuggability of the code by providing more context-specific error messages.
| 'https://identity.test/roles/101', | ||
| expect.objectContaining({ | ||
| params: expect.objectContaining({ | ||
| selector: 'subjects', |
There was a problem hiding this comment.
[❗❗ correctness]
The selector and perPage parameters are being checked in the expectation but are not set in the mock implementation. Ensure these parameters are correctly set in the actual implementation to avoid test failures due to mismatched expectations.
| import { M2MService } from 'src/shared/modules/global/m2m.service'; | ||
| import { LoggerService } from 'src/shared/modules/global/logger.service'; | ||
| import { MemberDetail } from 'src/shared/utils/member.utils'; | ||
|
|
||
| export interface MemberRoleRecord { | ||
| id?: string | number; |
There was a problem hiding this comment.
[correctness]
Consider making the id field non-optional if it is always expected to be present. This can help prevent potential runtime errors when accessing id without checking for its existence.
| roleName: string; | ||
| } | ||
|
|
||
| type RoleSubjectRecord = { | ||
| subjectID?: string | number; |
There was a problem hiding this comment.
[correctness]
Consider making subjectID and userId non-optional if they are always expected to be present. This can help prevent potential runtime errors when accessing these fields without checking for their existence.
|
|
||
| try { | ||
| const token = await this.m2mService.getM2MToken(); | ||
| const identityApiBaseUrl = this.identityApiUrl.replace(/\/$/, ''); |
There was a problem hiding this comment.
[💡 correctness]
The identityApiBaseUrl is constructed by replacing a trailing slash. Ensure that this operation is safe and does not inadvertently alter URLs that do not have a trailing slash.
| 'Content-Type': 'application/json', | ||
| }, | ||
| params: { | ||
| filter: `roleName=${normalizedRoleName}`, |
There was a problem hiding this comment.
[❗❗ security]
Ensure that the filter parameter is properly encoded to prevent potential injection attacks or malformed queries.
| 'Content-Type': 'application/json', | ||
| }, | ||
| params: { | ||
| selector: 'subjects', |
There was a problem hiding this comment.
[❗❗ security]
Ensure that the selector and perPage parameters are properly validated and encoded to prevent potential injection attacks or malformed queries.
| subject.subjectID || subject.userId || '', | ||
| ).trim(); | ||
|
|
||
| const key = email || subjectId; |
There was a problem hiding this comment.
[security]
The key is constructed using email or subjectId. Ensure that both are properly sanitized to prevent potential security issues or incorrect behavior.
| : []; | ||
|
|
||
| return { | ||
| isMachine: payload?.gty === 'client-credentials' || scopes.length > 0, |
There was a problem hiding this comment.
[correctness]
The condition payload?.gty === 'client-credentials' || scopes.length > 0 may incorrectly classify a token as a machine token if it has scopes but is not actually a machine token. Consider explicitly checking for machine token criteria to avoid potential misclassification.
| payload?.permissions; | ||
| const scopes = | ||
| typeof rawScope === 'string' | ||
| ? rawScope.split(/\s+/u).map((scope) => scope.trim().toLowerCase()) |
There was a problem hiding this comment.
[💡 correctness]
The use of scope.trim().toLowerCase() assumes that all scopes should be case-insensitive and whitespace-trimmed. Ensure this transformation aligns with the expected behavior of scope handling in your application.
| ? rawScope.map((scope) => String(scope).trim().toLowerCase()) | ||
| : []; | ||
|
|
||
| return { |
There was a problem hiding this comment.
[💡 correctness]
Filtering scopes with scope.length > 0 is a good practice to avoid empty scopes, but ensure that this behavior is consistent with the rest of the application logic where scopes are used.
|
|
||
| // TODO: replace 'topcoder_manager' string literal with UserRole enum value. |
There was a problem hiding this comment.
[maintainability]
Using a string literal for 'topcoder_manager' can lead to errors if the role name changes. Consider replacing it with an enum value from UserRole to ensure consistency and maintainability.
| }); | ||
|
|
||
| // TODO: extract to private isAdminManagerOrCopilot() helper to reduce duplication. |
There was a problem hiding this comment.
[maintainability]
The TODO comment suggests extracting a helper function to reduce duplication. This is a good idea for improving maintainability and readability. Consider implementing this change.
| * | ||
| * @param user authenticated JWT user context | ||
| * @returns first matched default {@link ProjectMemberRole}, or `undefined` | ||
| */ | ||
| getDefaultProjectRole(user: JwtUser): ProjectMemberRole | undefined { |
There was a problem hiding this comment.
[maintainability]
The TODO comment indicates duplication with another utility function. Consolidating this logic into a shared utility would improve maintainability and reduce the risk of inconsistencies.
| * | ||
| * @param role role string to normalize | ||
| * @returns normalized lowercase role | ||
| */ | ||
| normalizeRole(role: string): string { |
There was a problem hiding this comment.
[maintainability]
The TODO comment suggests extracting this normalization logic into a shared utility. This would enhance maintainability by centralizing the logic and reducing duplication.
| private normalizeUserId( | ||
| userId: string | number | bigint | undefined, | ||
| ): string { | ||
| // TODO: duplicated with src/shared/utils/member.utils.ts#normalizeUserId; extract shared normalization utility. |
There was a problem hiding this comment.
[maintainability]
The TODO comment indicates duplication with another utility function. Extracting this logic into a shared utility would improve maintainability and reduce redundancy.
| * | ||
| * Uses `parseInt` semantics for string values to preserve legacy behavior. | ||
| */ | ||
| export function parseOptionalLooseInteger(value: unknown): number | undefined { |
There was a problem hiding this comment.
[correctness]
The function parseOptionalLooseInteger uses parseInt which can lead to unexpected results if the string contains non-numeric characters. Consider documenting this behavior or handling such cases explicitly.
| /** | ||
| * Parses optional boolean values from boolean/string input. | ||
| */ | ||
| export function parseOptionalBoolean(value: unknown): boolean | undefined { |
There was a problem hiding this comment.
[correctness]
The function parseOptionalBoolean does not handle numeric inputs that could be interpreted as booleans (e.g., 1 for true, 0 for false). Consider handling these cases if they are relevant to the use case.
| } | ||
|
|
||
| if (input instanceof Prisma.Decimal) { | ||
| return Number(input.toString()); |
There was a problem hiding this comment.
[❗❗ correctness]
Converting Prisma.Decimal to a number using Number(input.toString()) can lead to precision loss for very large or very small decimal values. Consider using input.toNumber() if available, or ensure that the precision requirements are met.
|
|
||
| if (input && typeof input === 'object') { | ||
| if (input instanceof Date) { | ||
| return input; |
There was a problem hiding this comment.
[correctness]
Returning Date objects as-is might not be suitable for all serialization contexts, as it could lead to inconsistencies in how dates are represented. Consider converting Date objects to a standardized string format, such as ISO 8601.
| * | ||
| * @security `AUTH0_CLIENT_SECRET` is injected from environment and must never | ||
| * be logged. | ||
| */ | ||
| function buildBusApiConfig(): Record<string, unknown> { | ||
| return { | ||
| BUSAPI_URL: process.env.BUSAPI_URL, |
There was a problem hiding this comment.
[❗❗ correctness]
The removal of KAFKA_URL, KAFKA_CLIENT_CERT, and KAFKA_CLIENT_CERT_KEY from the configuration could impact the functionality if these are required for the Kafka connection. Ensure these are not needed or are being set elsewhere.
| * Builds `{ resource: 'project', data }` payload. | ||
| * | ||
| * @todo These `buildXxxEventPayload` functions are structurally identical. | ||
| * Replace with `buildEventPayload(resource, data)` + resource constants map. |
There was a problem hiding this comment.
[maintainability]
Consider consolidating the buildXxxEventPayload functions into a single buildEventPayload(resource, data) function with a resource constants map. This will reduce redundancy and improve maintainability.
| * @param project Domain payload wrapped as `resource: 'project'`. | ||
| * @param callback Optional success callback. | ||
| * | ||
| * @todo All `publishXxxEvent` helpers share identical try/catch + retry |
There was a problem hiding this comment.
[maintainability]
The publishXxxEvent functions share identical try/catch and retry logic. Consider consolidating these into a single publishEvent(topic, resource, data, callback?) function to reduce code duplication and improve maintainability.
| * Manager-tier Topcoder roles allowed to hold management project roles. | ||
| * | ||
| * @todo Duplicates `MANAGER_ROLES` from `userRole.enum.ts`. Import the shared | ||
| * constant instead of maintaining a local copy. |
There was a problem hiding this comment.
[maintainability]
The MANAGER_TOPCODER_ROLES constant duplicates the MANAGER_ROLES from userRole.enum.ts. Consider importing the shared constant to avoid maintenance overhead and potential inconsistencies.
| * Mapping between project roles and allowed Topcoder roles. | ||
| * | ||
| * @todo Duplicates matrix data from `permissions.constants.ts`. Consolidate to | ||
| * a single source of truth. |
There was a problem hiding this comment.
[maintainability]
The PROJECT_TO_TOPCODER_ROLES_MATRIX duplicates data from permissions.constants.ts. Consolidate this to a single source of truth to improve maintainability and reduce the risk of inconsistencies.
| /** | ||
| * Normalizes role strings for case-insensitive comparisons. | ||
| * | ||
| * @todo Duplicated in `permission.service.ts`; extract shared utility. |
There was a problem hiding this comment.
[maintainability]
The normalizeRole function is duplicated in permission.service.ts. Consider extracting this utility to a shared module to enhance maintainability.
| /** | ||
| * Normalizes user ids to trimmed strings. | ||
| * | ||
| * @todo Duplicated in `permission.service.ts`; extract shared utility. |
There was a problem hiding this comment.
[maintainability]
The normalizeUserId function is duplicated in permission.service.ts. Extracting this utility to a shared module would improve maintainability.
| * | ||
| * Evaluates `DEFAULT_PROJECT_ROLE` in order; first matching Topcoder role wins. | ||
| * | ||
| * @todo Duplicated in `permission.service.ts`; consolidate shared role |
There was a problem hiding this comment.
[maintainability]
The logic for deriving the default project role is duplicated in permission.service.ts. Consider consolidating this logic to a shared utility to enhance maintainability.
| * | ||
| * Supported fields: `handle`, `email`, `firstName`, `lastName`. | ||
| * | ||
| * @todo Shares near-identical logic with `enrichInvitesWithUserDetails`. |
There was a problem hiding this comment.
[maintainability]
The enrichMembersWithUserDetails function shares near-identical logic with enrichInvitesWithUserDetails. Extract a generic function enrichWithUserDetails<T>(items, getKey, fields, userDetails) to reduce code duplication and improve maintainability.
| * Supported fields: `handle`, `email`, `firstName`, `lastName`. | ||
| * For `email`, falls back to `invite.email` when user detail is absent. | ||
| * | ||
| * @todo Shares near-identical logic with `enrichMembersWithUserDetails`. |
There was a problem hiding this comment.
[maintainability]
The enrichInvitesWithUserDetails function shares near-identical logic with enrichMembersWithUserDetails. Extract a generic function enrichWithUserDetails<T>(items, getKey, fields, userDetails) to reduce code duplication and improve maintainability.
| * | ||
| * @deprecated Use `setProjectPaginationHeaders` directly. | ||
| * @todo Remove this alias after all call sites are migrated. | ||
| */ | ||
| export const setResHeader = setProjectPaginationHeaders; |
There was a problem hiding this comment.
[maintainability]
The alias setResHeader is marked as deprecated, but there is no mechanism in place to track or enforce the migration of call sites to setProjectPaginationHeaders. Consider adding a deprecation warning or logging to ensure developers are aware of the need to migrate.
| function dedupeStrings(values: readonly string[]): string[] { | ||
| return Array.from( | ||
| new Set( | ||
| values.map((value) => String(value).trim()).filter((value) => value), |
There was a problem hiding this comment.
[correctness]
The dedupeStrings function uses String(value).trim() on each value, which could potentially lead to unexpected behavior if the input array contains non-string values. Consider ensuring that the input is always an array of strings or handle non-string values explicitly.
| const resolvedSummaries = summaries | ||
| .map((permission) => | ||
| typeof permission === 'string' | ||
| ? getNamedPermissionDocumentation(permission) |
There was a problem hiding this comment.
[correctness]
In getRequiredPermissionsDocumentation, the function getNamedPermissionDocumentation is called with a permission parameter assumed to be a NamedPermission. Ensure that the summaries array only contains valid NamedPermission strings or handle invalid cases to prevent runtime errors.
| const where = buildProjectWhereClause( | ||
| { | ||
| keyword: '"my ba"', | ||
| } as any, |
There was a problem hiding this comment.
[maintainability]
Using as any can mask type errors and reduce type safety. Consider defining a specific type for the input object to leverage TypeScript's type-checking capabilities.
| { | ||
| userId: '123', | ||
| isMachine: false, | ||
| } as any, |
There was a problem hiding this comment.
[maintainability]
Using as any here reduces the benefits of TypeScript's type system. Consider defining a specific type for the second parameter to improve type safety and maintainability.
| const where = buildProjectWhereClause( | ||
| { | ||
| keyword: '""', | ||
| } as any, |
There was a problem hiding this comment.
[maintainability]
Using as any can hide potential type issues. It would be beneficial to define a specific type for the input object to ensure type safety.
| { | ||
| userId: '123', | ||
| isMachine: false, | ||
| } as any, |
There was a problem hiding this comment.
[maintainability]
The use of as any here bypasses TypeScript's type checking. Consider specifying a type for the second parameter to enhance code maintainability and safety.
| */ | ||
| function buildFullTextSearchQuery(value: string): string | undefined { | ||
| const normalizedKeyword = normalizeKeywordSearchText(value); | ||
| const tokens = normalizedKeyword.match(/[\p{L}\p{N}]+/gu) ?? []; |
There was a problem hiding this comment.
[correctness]
The use of Unicode property escapes (\p{L}\p{N}) in the regular expression requires the u flag, which is correctly used here. However, ensure that the runtime environment supports this feature, as it may not be available in older JavaScript environments.
| OR: visibilityFilters, | ||
| }); | ||
| } else { | ||
| appendAndCondition(where, { |
There was a problem hiding this comment.
[💡 maintainability]
Using id: -1n as a guard condition to return zero rows is a clever approach. However, ensure that this behavior is well-documented and understood by the team, as it relies on a specific implementation detail that could be overlooked or misunderstood.
| : `${normalized} asc`; | ||
| const [field, direction] = withDirection.split(/\s+/); | ||
|
|
||
| if (!field || !direction || !allowedFields.includes(field)) { |
There was a problem hiding this comment.
[💡 maintainability]
The check for !field || !direction is redundant since withDirection.split(/\s+/) will always return an array with at least one element. Consider simplifying the condition to !allowedFields.includes(field).
| throw new BadRequestException('Invalid sort criteria.'); | ||
| } | ||
|
|
||
| const normalizedDirection = direction.toLowerCase(); |
There was a problem hiding this comment.
[performance]
Consider using a Set for allowedFields to improve the performance of the includes check, especially if the list of allowed fields is large.
| import { extractScopesFromPayload } from './scope.utils'; | ||
|
|
||
| describe('extractScopesFromPayload', () => { | ||
| it('splits string claims on arbitrary whitespace', () => { |
There was a problem hiding this comment.
[correctness]
Consider adding a test case for when the scope field is missing or null to ensure the function handles such cases gracefully.
| ).toEqual(['all:projects', 'write:projects', 'read:projects']); | ||
| }); | ||
|
|
||
| it('merges supported scope claims and deduplicates values', () => { |
There was a problem hiding this comment.
[correctness]
It would be beneficial to test the behavior when scopes or permissions fields contain duplicate values to ensure deduplication logic is robust.
| */ | ||
| export function extractScopesFromPayload( | ||
| payload: ScopePayload, | ||
| normalizeScope: ScopeNormalizer = (scope) => scope.trim(), |
There was a problem hiding this comment.
[design]
The default normalizeScope function uses scope.trim(), which may not handle all normalization needs. Consider documenting expected normalization behavior or making it configurable.
| if (Array.isArray(rawScope)) { | ||
| extractedScopes.push( | ||
| ...rawScope | ||
| .map((scope) => normalizeScope(String(scope))) |
There was a problem hiding this comment.
[❗❗ correctness]
Using String(scope) to convert array elements to strings may lead to unexpected results if the elements are objects or other non-primitive types. Consider adding a check to ensure elements are of expected types before conversion.
| * Returns `-1` for machine principals without a numeric actor id. | ||
| */ | ||
| export function getAuditUserId(user: JwtUser): number { | ||
| const parsedUserId = Number.parseInt(getActorUserId(user), 10); |
There was a problem hiding this comment.
[correctness]
Using Number.parseInt without a radix can lead to unexpected results if the input string starts with '0'. Consider specifying the radix explicitly.
| * Uses the machine-principal actor id when a human `userId` is absent. | ||
| */ | ||
| export function getAuditUserIdOrDefault(user: JwtUser, fallback = -1): number { | ||
| const actorId = |
There was a problem hiding this comment.
[correctness]
Using Number.parseInt without a radix can lead to unexpected results if the input string starts with '0'. Consider specifying the radix explicitly.
| prisma: PrismaService, | ||
| projectId: bigint, | ||
| ): Promise<ProjectPermissionContext> { | ||
| const project = await prisma.project.findFirst({ |
There was a problem hiding this comment.
[performance]
Consider using prisma.project.findUnique instead of findFirst if projectId is a unique identifier. This can improve performance and clarity.
| prisma: PrismaService, | ||
| projectId: bigint, | ||
| ): Promise<ProjectPermissionContextBase> { | ||
| const project = await prisma.project.findFirst({ |
There was a problem hiding this comment.
[performance]
Consider using prisma.project.findUnique instead of findFirst if projectId is a unique identifier. This can improve performance and clarity.
| }, | ||
| }, | ||
| }, | ||
| } as unknown as OpenAPIObject; |
There was a problem hiding this comment.
[maintainability]
Casting the object to OpenAPIObject using as unknown as OpenAPIObject is a workaround that could hide potential type mismatches. Consider defining a more precise type for the operation parameter or restructuring the code to avoid the need for double casting.
| return value | ||
| .map((entry) => stringifyPermission(entry as RequiredPermission)) | ||
| .filter((entry) => entry.length > 0); | ||
| return value as RequiredPermission[]; |
There was a problem hiding this comment.
[correctness]
Casting value directly to RequiredPermission[] without validation could lead to runtime errors if the input is not as expected. Consider adding type checks or validation to ensure the input conforms to RequiredPermission structure.
| function pruneSwaggerAuthExtensions(operation: SwaggerOperation): void { | ||
| const roles = parseStringArray(operation[SWAGGER_REQUIRED_ROLES_KEY]); | ||
|
|
||
| if (isAllKnownUserRoles(roles)) { |
There was a problem hiding this comment.
[design]
The function pruneSwaggerAuthExtensions deletes the SWAGGER_REQUIRED_ROLES_KEY if all known user roles are present. Ensure this behavior is intended, as it may lead to loss of metadata that could be useful for debugging or auditing purposes.
|
|
||
| return { | ||
| isMachine: payload?.gty === 'client-credentials', | ||
| scopes, | ||
| isMachine: payload?.gty === 'client-credentials' || scopes.length > 0, |
There was a problem hiding this comment.
[correctness]
The condition scopes.length > 0 in the isMachine property could lead to incorrect results if payload?.gty is not 'client-credentials' but scopes is non-empty. Ensure this logic aligns with the intended behavior.
| isMachine: payload?.gty === 'client-credentials', | ||
| scopes, | ||
| isMachine: payload?.gty === 'client-credentials' || scopes.length > 0, | ||
| scopes: scopes.filter((scope) => scope.length > 0), |
There was a problem hiding this comment.
[💡 maintainability]
Filtering scopes to remove empty strings is a good practice, but consider handling cases where rawScope is not a string or array more explicitly, as the current logic defaults to an empty array without logging or error handling.
| @@ -128,7 +128,8 @@ describe('Deployment validation', () => { | |||
| 'AUTH0_AUDIENCE', | |||
| 'AUTH0_CLIENT_ID', | |||
| 'AUTH0_CLIENT_SECRET', | |||
| 'KAFKA_URL', | |||
| 'BUSAPI_URL', | |||
There was a problem hiding this comment.
[maintainability]
The addition of BUSAPI_URL and KAFKA_ERROR_TOPIC to the list of required environment variables is correct, but ensure that these variables are documented and properly set in all deployment environments to avoid runtime errors.
| .set('Authorization', 'Bearer user-token') | ||
| .expect(200); | ||
|
|
||
| expect(response.body).toEqual([]); |
There was a problem hiding this comment.
[correctness]
The test expects an empty array [] as the response body for authenticated users. Ensure this is the intended behavior, as it might be more useful to verify the presence of expected project types if applicable.
| }), | ||
| hasRequiredScopes: jest.fn( | ||
| (tokenScopes: string[], requiredScopes: string[]): boolean => { | ||
| if (requiredScopes.length === 0) { |
There was a problem hiding this comment.
[correctness]
The hasRequiredScopes function uses some to check if any required scope is present in the token scopes. This means it will return true even if only one required scope is present, which might not be the intended behavior if all required scopes are needed. Consider using every instead if all required scopes must be present.
| const rawScope = payload?.scope; | ||
| const scopes = | ||
| typeof rawScope === 'string' | ||
| ? rawScope.split(' ').map((scope) => scope.trim().toLowerCase()) |
There was a problem hiding this comment.
[💡 correctness]
The validateMachineToken function splits the rawScope string by spaces and trims each scope. Ensure that the input format is always consistent and does not contain unexpected delimiters or whitespace issues that could lead to incorrect scope parsing.
| @@ -225,7 +225,7 @@ describe('Project Member endpoints (e2e)', () => { | |||
| expect(projectMemberServiceMock.deleteMember).toHaveBeenCalled(); | |||
| }); | |||
|
|
|||
| it('rejects m2m token for member list without named permission match', async () => { | |||
| it('lists members for m2m token with project-member read scope', async () => { | |||
| (jwtServiceMock.validateToken as jest.Mock).mockResolvedValueOnce({ | |||
There was a problem hiding this comment.
[💡 maintainability]
The validateToken mock now resolves with a tokenPayload object, but this object is not used in the test. Consider removing it if it's unnecessary to avoid confusion.
1 participant
Initial release to prod for final functionality.