Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| WORKDIR /app | ||
| COPY . . | ||
| COPY --chown=node:node . . |
There was a problem hiding this comment.
[security]
Using COPY --chown=node:node . . is a good practice for setting file ownership, but ensure that all necessary files and directories are accessible to the node user. Verify that there are no permission issues during runtime.
| COPY --chown=node:node . . | ||
| RUN npm install pnpm -g | ||
| RUN pnpm install | ||
| RUN pnpm install --frozen-lockfile |
There was a problem hiding this comment.
[maintainability]
Using --frozen-lockfile with pnpm install ensures that the exact versions in the lockfile are used, which is good for consistency across environments. However, ensure that the lockfile is up-to-date and committed to the repository to avoid potential dependency resolution issues.
|
|
||
| USER node | ||
|
|
||
| HEALTHCHECK --interval=30s --timeout=5s --start-period=30s --retries=3 \ |
There was a problem hiding this comment.
[❗❗ correctness]
The HEALTHCHECK command uses wget to check the health endpoint. Ensure that wget is installed in the image, or consider using curl which might be more commonly available. Also, verify that the health endpoint is correctly implemented and accessible.
|
|
||
| # Start the app | ||
| pnpm start:prod | ||
| exec node dist/src/main |
There was a problem hiding this comment.
[❗❗ correctness]
Replacing pnpm start:prod with exec node dist/src/main changes how the application is started. Ensure that any scripts or environment setups previously handled by pnpm are now correctly managed, as this could affect application initialization and environment configuration.
| expect(statusMock).toHaveBeenCalledWith(201); | ||
| }); | ||
|
|
||
| it('sets 403 when createInvites only returns failures', async () => { |
There was a problem hiding this comment.
[correctness]
The test case for setting status 403 when createInvites only returns failures is correct, but consider verifying that the response object is correctly structured and contains the expected error message. This will ensure that the API consumer receives the necessary information to understand why the request failed.
| ); | ||
|
|
||
| if (response.failed && response.failed.length > 0) { | ||
| const createdInviteCount = Array.isArray(response.success) |
There was a problem hiding this comment.
[correctness]
Consider using Array.isArray(response.failed) to ensure response.failed is an array before accessing its length property. This will prevent potential runtime errors if response.failed is undefined or not an array.
| expect(validateSync(dto)).toEqual([]); | ||
| }); | ||
|
|
||
| it('rejects user ids outside the supported bigint range', () => { |
There was a problem hiding this comment.
[maintainability]
The test case description 'rejects user ids outside the supported bigint range' could be misleading. The test checks for a specific value that is one more than Number.MAX_SAFE_INTEGER, but it does not verify the full range of bigint values. Consider clarifying the description or adding additional test cases to cover a broader range of invalid bigint values.
| } | ||
|
|
||
| return Math.trunc(parsed); | ||
| throw new BadRequestException('User id must be a numeric string.'); |
There was a problem hiding this comment.
[maintainability]
The error message in BadRequestException could be more descriptive by including the actual value that caused the exception. This would aid in debugging and provide more context to the client.
| @IsNumber() | ||
| userId?: number; | ||
| @Transform(({ value }) => parseOptionalUserId(value)) | ||
| @IsString() |
There was a problem hiding this comment.
[correctness]
Using @IsString() for userId might not fully validate the numeric string format. Consider using a custom validator to ensure userId is a valid numeric string, which would enhance data integrity.
| '1001', | ||
| { | ||
| userId: 456, | ||
| userId: '456', |
There was a problem hiding this comment.
[❗❗ correctness]
The userId was changed from a number to a string. Ensure that this change is consistent with the rest of the codebase and that the userId is always expected to be a string. This could potentially lead to issues if other parts of the system expect a numeric userId.
| service.addMember( | ||
| '1001', | ||
| { | ||
| userId: 'invalid' as unknown as string, |
There was a problem hiding this comment.
[💡 style]
Casting 'invalid' as unknown and then as string is unnecessary and could be misleading. Since 'invalid' is already a string, consider removing the redundant type casting.
| service.addMember( | ||
| '1001', | ||
| { | ||
| userId: '10000000000000011111', |
There was a problem hiding this comment.
[correctness]
The userId '10000000000000011111' is a string representation of a large number. Ensure that the system can handle such large numeric values when converted from strings, especially if they are used in contexts expecting numeric types.
| } | ||
|
|
||
| if (typeof userId === 'number') { | ||
| if (!Number.isFinite(userId)) { |
There was a problem hiding this comment.
[correctness]
The check Number.isFinite(userId) is used to ensure the number is finite, but it might be more appropriate to check if userId is a safe integer using Number.isSafeInteger(userId) to avoid potential issues with very large numbers that could lose precision.
| ).toString(); | ||
| } | ||
|
|
||
| if (typeof userId === 'string' || typeof userId === 'bigint') { |
There was a problem hiding this comment.
[correctness]
The resolveTargetUserId method handles userId as a bigint or string, but it doesn't explicitly handle cases where userId might be a non-numeric string. Consider adding validation to ensure that userId is a numeric string before calling parseNumericStringId.
| Scope.CONNECT_PROJECT_ADMIN, | ||
| ) | ||
| @RequirePermission(Permission.VIEW_PROJECT) | ||
| @RequirePermission(Permission.VIEW_PROJECT, { |
There was a problem hiding this comment.
[❗❗ security]
The addition of topcoderRoles: [UserRole.PROJECT_MANAGER] to the @RequirePermission decorator changes the permission requirements. Ensure that this change aligns with the intended access control policy, as it may inadvertently restrict or expand access.
| isMachine: boolean; | ||
| }, | ||
| ) => { | ||
| prismaMock.project.findFirst.mockResolvedValue({ |
There was a problem hiding this comment.
[correctness]
Consider adding a test case for when the prismaMock.project.findFirst returns null to ensure the function handles the absence of a project gracefully.
| }, | ||
| ); | ||
|
|
||
| it('returns a per-user permission matrix for copilot members on the requested project', async () => { |
There was a problem hiding this comment.
[correctness]
The test case for copilot members assumes that the permissionServiceMock.hasNamedPermission implementation correctly handles the role 'copilot'. Ensure that this mock implementation aligns with the actual logic in the service to avoid discrepancies.
| expect(transactionProjectHistoryCreate).toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('assigns Talent Manager creators the manager project role', async () => { |
There was a problem hiding this comment.
[correctness]
The test for assigning Talent Manager creators the manager project role does not verify if the role assignment is conditional on the hasNamedPermission check. Consider adding assertions to ensure that the role assignment logic is correctly gated by permissions.
| }), | ||
| workManagementPermissionsPromise, | ||
| ]); | ||
| if (this.shouldReturnProjectMemberPermissionMatrix(user, projectMembers)) { |
There was a problem hiding this comment.
[maintainability]
The refactoring of the getProjectPermissions method to use shouldReturnProjectMemberPermissionMatrix improves readability and maintainability by clearly separating the logic for determining when to return the permission matrix. However, ensure that the logic in shouldReturnProjectMemberPermissionMatrix is thoroughly tested, as it now plays a critical role in deciding the flow of permission retrieval.
| * @param projectMembers Active members of the requested project. | ||
| * @returns `true` when the route should return the per-user matrix. | ||
| */ | ||
| private shouldReturnProjectMemberPermissionMatrix( |
There was a problem hiding this comment.
[💡 performance]
The shouldReturnProjectMemberPermissionMatrix method uses a combination of role checks and user ID parsing to determine access. Consider caching the normalized roles or user ID parsing results if this method is called frequently, to improve performance.
| project role, otherwise with "${PROJECT_MEMBER_ROLE.CUSTOMER}".`, | ||
| }, | ||
| topcoderRoles: TOPCODER_ROLES_MANAGERS_AND_ADMINS, | ||
| topcoderRoles: [ |
There was a problem hiding this comment.
[❗❗ security]
The change from TOPCODER_ROLES_MANAGERS_AND_ADMINS to a specific list of roles including USER_ROLE.TALENT_MANAGER and USER_ROLE.TOPCODER_TALENT_MANAGER may impact the permissions for creating projects as a manager. Ensure that this change aligns with the intended access control policy and that these roles should indeed have this permission.
| }, | ||
| topcoderRoles: [USER_ROLE.MANAGER, USER_ROLE.TOPCODER_ADMIN], | ||
| scopes: SCOPES_PROJECTS_WRITE_PROJECTS_BILLING_ACCOUNTS, | ||
| topcoderRoles: TOPCODER_ROLES_ADMINS, |
There was a problem hiding this comment.
[❗❗ security]
The change from a specific list of roles to TOPCODER_ROLES_ADMINS for managing the billingAccountId property may broaden the access to this permission. Verify that this is intentional and that all roles included in TOPCODER_ROLES_ADMINS should have this capability.
| service: LoggerService, | ||
| message: unknown, | ||
| ): string => { | ||
| return (service as unknown as LoggerServicePrivate).serializeMessage( |
There was a problem hiding this comment.
[correctness]
Casting service to unknown and then to LoggerServicePrivate is a risky operation that could lead to runtime errors if the serializeMessage method is not present. Consider using a safer approach, such as checking if the method exists before calling it.
| * Global logger service implementing NestJS LoggerService contract. | ||
| */ | ||
| export class LoggerService implements NestLoggerService { | ||
| private static readonly SENSITIVE_VALUE_PATTERN = |
There was a problem hiding this comment.
[security]
The regex patterns for detecting sensitive information are comprehensive, but consider adding tests to ensure they cover all expected cases and do not inadvertently redact non-sensitive data.
| } | ||
|
|
||
| if (Array.isArray(message)) { | ||
| return '[redacted array payload]'; |
There was a problem hiding this comment.
[design]
Returning a fixed placeholder for arrays and objects may prevent useful debugging information from being logged. Consider allowing configurable redaction levels to balance security and debugging needs.
| }, | ||
| ); | ||
|
|
||
| it('blocks managing project billing accounts for machine admin scope without admin user role', () => { |
There was a problem hiding this comment.
[💡 readability]
The test case description mentions 'machine admin scope without admin user role', but the input does not include any user roles. Consider clarifying the test case to ensure it accurately reflects the intended scenario.
| expect(allowed).toBe(false); | ||
| }); | ||
|
|
||
| it.each([UserRole.TALENT_MANAGER, UserRole.TOPCODER_TALENT_MANAGER])( |
There was a problem hiding this comment.
[💡 readability]
The test case description 'allows creating projects as manager for %s Topcoder role' might be misleading as it does not specify the context of the role being a manager. Consider rephrasing for clarity.
| }, | ||
| ); | ||
|
|
||
| it.each([UserRole.TALENT_MANAGER, UserRole.TOPCODER_TALENT_MANAGER])( |
There was a problem hiding this comment.
[💡 readability]
The test case description 'allows full project-owner delete access for %s creators once they are manager members' could be clearer. It might be beneficial to specify the conditions under which this access is granted.
| case NamedPermission.UPDATE_PROJECT_MEMBER_NON_CUSTOMER: | ||
| case NamedPermission.DELETE_PROJECT_MEMBER_TOPCODER: | ||
| return isAdmin || isManagementMember; | ||
| return isAdmin || isManagementMember || hasProjectMemberWriteScope; |
There was a problem hiding this comment.
[❗❗ security]
Adding hasProjectMemberWriteScope to the return condition for CREATE_PROJECT_MEMBER_NOT_OWN, UPDATE_PROJECT_MEMBER_NON_CUSTOMER, and DELETE_PROJECT_MEMBER_TOPCODER is a significant change. Ensure that this scope is correctly assigned and managed, as it now grants write permissions to project members.
| isAdmin || | ||
| isManagementMember || | ||
| this.isCopilot(member?.role) || | ||
| hasProjectMemberWriteScope |
There was a problem hiding this comment.
[❗❗ security]
Including hasProjectMemberWriteScope in the condition for DELETE_PROJECT_MEMBER_CUSTOMER might inadvertently expand the set of users who can delete customer project members. Verify that this scope is intended to have such permissions.
| this.isCopilot(member?.role) || | ||
| this.hasCopilotManagerRole(user) | ||
| this.hasCopilotManagerRole(user) || | ||
| hasProjectMemberWriteScope |
There was a problem hiding this comment.
[❗❗ security]
The addition of hasProjectMemberWriteScope to the condition for DELETE_PROJECT_MEMBER_COPILOT could potentially allow more users to delete copilot project members. Ensure that this aligns with the intended access control policies.
|
|
||
| // Billing-account related permissions. | ||
| case NamedPermission.MANAGE_PROJECT_BILLING_ACCOUNT_ID: | ||
| return ( |
There was a problem hiding this comment.
[❗❗ security]
The change in the return condition for MANAGE_PROJECT_BILLING_ACCOUNT_ID now allows project managers to manage billing accounts. Ensure that this aligns with the intended permissions and that project managers are supposed to have this level of access.
| ...ADMIN_ROLES, | ||
| UserRole.CONNECT_ADMIN, | ||
| UserRole.TALENT_MANAGER, | ||
| UserRole.TOPCODER_TALENT_MANAGER, |
There was a problem hiding this comment.
[❗❗ security]
Adding UserRole.TALENT_MANAGER and UserRole.TOPCODER_TALENT_MANAGER to the roles that can create projects as a manager is a significant change. Verify that these roles should indeed have this capability.
| ...TALENT_MANAGER_ROLES, | ||
| ]; | ||
|
|
||
| const PROJECT_BILLING_MANAGEMENT_USER_ROLES = [...STRICT_ADMIN_ACCESS_ROLES]; |
There was a problem hiding this comment.
[maintainability]
Consider adding ...TALENT_MANAGER_ROLES to PROJECT_BILLING_MANAGEMENT_USER_ROLES to maintain consistency with other role definitions that include talent manager roles. This ensures that any future updates to TALENT_MANAGER_ROLES are automatically reflected in PROJECT_BILLING_MANAGEMENT_USER_ROLES.
| }); | ||
|
|
||
| case NamedPermission.MANAGE_PROJECT_BILLING_ACCOUNT_ID: | ||
| return createSummary({ |
There was a problem hiding this comment.
[correctness]
The addition of the MANAGE_PROJECT_BILLING_ACCOUNT_ID case with PROJECT_BILLING_MANAGEMENT_USER_ROLES seems correct, but ensure that this change aligns with the intended permission model. Verify that only roles defined in PROJECT_BILLING_MANAGEMENT_USER_ROLES should have access to manage project billing account IDs.
| return BigInt(normalized); | ||
| const parsed = BigInt(normalized); | ||
|
|
||
| if (parsed > MAX_SIGNED_BIGINT_ID) { |
There was a problem hiding this comment.
[correctness]
The check parsed > MAX_SIGNED_BIGINT_ID ensures that the parsed value does not exceed the maximum signed 64-bit integer. However, consider also checking for negative values to ensure the ID is within the valid range of positive signed 64-bit integers.
| }); | ||
|
|
||
| it('returns 403 for partial failures', async () => { | ||
| it('returns 201 when at least one invite is created', async () => { |
There was a problem hiding this comment.
[correctness]
The test description has been changed to 'returns 201 when at least one invite is created', but the test does not verify the contents of the failed array. Consider adding assertions to ensure that the failed invites are handled correctly, as this could affect the correctness of the test.
| .expect(201); | ||
| }); | ||
|
|
||
| it('returns 403 when no invites are created', async () => { |
There was a problem hiding this comment.
[correctness]
In the test 'returns 403 when no invites are created', the failed array is checked, but there is no verification of the error message or the specific failure reasons. Consider adding assertions to verify the contents of the failed array to ensure that the failure reasons are as expected.
1 participant
https://topcoder.atlassian.net/browse/PM-4376