From fe1b85f7efa6f055e454c7052ed7581322a4b8bf Mon Sep 17 00:00:00 2001 From: Justin Gasper Date: Tue, 10 Mar 2026 11:25:57 +1100 Subject: [PATCH 1/8] Security fixes --- Dockerfile | 11 ++- package.json | 13 ++- pnpm-lock.yaml | 95 +++++++++---------- .../modules/global/logger.service.spec.ts | 56 +++++++++++ src/shared/modules/global/logger.service.ts | 68 +++++++++++-- 5 files changed, 176 insertions(+), 67 deletions(-) create mode 100644 src/shared/modules/global/logger.service.spec.ts diff --git a/Dockerfile b/Dockerfile index 9224fe5..559daad 100644 --- a/Dockerfile +++ b/Dockerfile @@ -3,7 +3,6 @@ FROM node:22.13.1-alpine RUN apk add --no-cache bash git -RUN apk update ARG RESET_DB_ARG=false ENV RESET_DB=$RESET_DB_ARG @@ -12,9 +11,15 @@ ENV SEED_DATA=$SEED_DATA_ARG ENV PRISMA_CLI_BINARY_TARGETS=linux-musl-openssl-3.0.x WORKDIR /app -COPY . . +COPY --chown=node:node . . RUN npm install pnpm -g RUN pnpm install RUN pnpm run build RUN chmod +x appStartUp.sh -CMD ./appStartUp.sh + +USER node + +HEALTHCHECK --interval=30s --timeout=5s --start-period=30s --retries=3 \ + CMD wget -q -O /dev/null "http://127.0.0.1:${PORT:-3000}/v6/projects/health" || exit 1 + +CMD ["./appStartUp.sh"] diff --git a/package.json b/package.json index 1138e08..a45fdc6 100644 --- a/package.json +++ b/package.json @@ -12,7 +12,7 @@ "start:dev": "nest start --watch", "start:debug": "nest start --debug --watch", "start:prod": "node dist/src/main", - "lint": "eslint \"{src,apps,libs,test,prisma}/**/*.ts\" --fix", + "lint": "eslint \"{src,apps,libs,test,prisma}/**/*.ts\" --fix --no-error-on-unmatched-pattern", "test": "jest --config ./jest.config.js", "test:watch": "jest --config ./jest.config.js --watch", "test:cov": "jest --config ./jest.config.js --coverage", @@ -104,14 +104,17 @@ "packageManager": "pnpm@10.28.2+sha512.41872f037ad22f7348e3b1debbaf7e867cfd448f2726d9cf74c08f19507c31d2c8e7a11525b983febc2df640b5438dee6023ebb1f84ed43cc2d654d2bc326264", "pnpm": { "overrides": { + "@hono/node-server": "1.19.10", "ajv": "8.18.0", "axios": "1.13.5", - "fast-xml-parser": "5.3.6", - "hono": "4.11.10", + "fast-xml-parser": "5.3.8", + "hono": "4.12.4", "jws": ">=3.2.3 <4.0.0 || >=4.0.1", "lodash": "4.17.23", - "minimatch": "10.2.1", - "qs": "6.14.2" + "minimatch": "10.2.3", + "multer": "2.1.1", + "qs": "6.14.2", + "serialize-javascript": "7.0.3" }, "patchedDependencies": { "@eslint/eslintrc@3.3.3": "patches/@eslint__eslintrc@3.3.3.patch", diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index b822065..483cfb1 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -5,14 +5,17 @@ settings: excludeLinksFromLockfile: false overrides: + '@hono/node-server': 1.19.10 ajv: 8.18.0 axios: 1.13.5 - fast-xml-parser: 5.3.6 - hono: 4.11.10 + fast-xml-parser: 5.3.8 + hono: 4.12.4 jws: '>=3.2.3 <4.0.0 || >=4.0.1' lodash: 4.17.23 - minimatch: 10.2.1 + minimatch: 10.2.3 + multer: 2.1.1 qs: 6.14.2 + serialize-javascript: 7.0.3 patchedDependencies: '@eslint/eslintrc@3.3.3': @@ -667,11 +670,11 @@ packages: '@hapi/topo@6.0.2': resolution: {integrity: sha512-KR3rD5inZbGMrHmgPxsJ9dbi6zEK+C3ZwUwTa+eMwWLz7oijWUTWD2pMSNNYJAU6Qq+65NkxXjqHr/7LM2Xkqg==} - '@hono/node-server@1.19.9': - resolution: {integrity: sha512-vHL6w3ecZsky+8P5MD+eFfaGTyCeOHUIFYMGpQGbrBTSmNNoxv0if69rEZ5giu36weC5saFuznL411gRX7bJDw==} + '@hono/node-server@1.19.10': + resolution: {integrity: sha512-hZ7nOssGqRgyV3FVVQdfi+U4q02uB23bpnYpdvNXkYTRRyWx84b7yf1ans+dnJ/7h41sGL3CeQTfO+ZGxuO+Iw==} engines: {node: '>=18.14.1'} peerDependencies: - hono: 4.11.10 + hono: 4.12.4 '@humanfs/core@0.19.1': resolution: {integrity: sha512-5DyQ4+1JEUzejeK1JGICcideyfUbGixgS9jNgex5nqkW+cY7WZhxBigmieN5Qnw9ZosSNVC9KQKyb+GUaGyKUA==} @@ -2649,8 +2652,8 @@ packages: fast-uri@3.1.0: resolution: {integrity: sha512-iPeeDKJSWf4IEOasVVrknXpaBV0IApz/gp7S2bb7Z4Lljbl2MGJRqInZiUrQwV16cpzw/D3S5j5Julj/gT52AA==} - fast-xml-parser@5.3.6: - resolution: {integrity: sha512-QNI3sAvSvaOiaMl8FYU4trnEzCwiRr8XMWgAHzlrWpTSj+QaCSvOf1h82OEP1s4hiAXhnbXSyFWCf4ldZzZRVA==} + fast-xml-parser@5.3.8: + resolution: {integrity: sha512-53jIF4N6u/pxvaL1eb/hEZts/cFLWZ92eCfLrNyCI0k38lettCG/Bs40W9pPwoPXyHQlKu2OUbQtiEIZK/J6Vw==} hasBin: true fastq@1.20.1: @@ -2895,8 +2898,8 @@ packages: hdr-histogram-percentiles-obj@3.0.0: resolution: {integrity: sha512-7kIufnBqdsBGcSZLPJwqHT3yhk1QTsSlFsVD3kx5ixH/AlgBs9yM1q6DPhXZ8f8gtdqgh7N7/5btRLpQsS2gHw==} - hono@4.11.10: - resolution: {integrity: sha512-kyWP5PAiMooEvGrA9jcD3IXF7ATu8+o7B3KCbPXid5se52NPqnOpM/r9qeW2heMnOekF4kqR1fXJqCYeCLKrZg==} + hono@4.12.4: + resolution: {integrity: sha512-ooiZW1Xy8rQ4oELQ++otI2T9DsKpV0M6c6cO6JGx4RTfav9poFFLlet9UMXHZnoM1yG0HWGlQLswBGX3RZmHtg==} engines: {node: '>=16.9.0'} html-escaper@2.0.2: @@ -3458,9 +3461,9 @@ packages: resolution: {integrity: sha512-e5ISH9xMYU0DzrT+jl8q2ze9D6eWBto+I8CNpe+VI+K2J/F/k3PdkdTdz4wvGVH4NTpo+NRYTVIuMQEMMcsLqg==} engines: {node: ^12.20.0 || ^14.13.1 || >=16.0.0} - minimatch@10.2.1: - resolution: {integrity: sha512-MClCe8IL5nRRmawL6ib/eT4oLyeKMGCghibcDWK+J0hh0Q8kqSdia6BvbRMVk6mPa6WqUa5uR2oxt6C5jd533A==} - engines: {node: 20 || >=22} + minimatch@10.2.3: + resolution: {integrity: sha512-Rwi3pnapEqirPSbWbrZaa6N3nmqq4Xer/2XooiOKyV3q12ML06f7MOuc5DVH8ONZIFhwIYQ3yzPH4nt7iWHaTg==} + engines: {node: 18 || 20 || >=22} minimist@1.2.8: resolution: {integrity: sha512-2yyAR8qBkN3YuheJanUpWC5U3bb5osDywNB8RzDVlDwDHbocAJveqqj1u8+SVD7jkWT4yvsHCpWqqWqAxb0zCA==} @@ -3479,8 +3482,8 @@ packages: ms@2.1.3: resolution: {integrity: sha512-6FlzubTLZG3J2a/NVCAleEhjzq5oxgHyaCU9yYXvcLsvoVaHJq/s5xXI6/XXP6tz7R9xAOtHnSO/tXtF3WRTlA==} - multer@2.0.2: - resolution: {integrity: sha512-u7f2xaZ/UG8oLXHvtF/oWTRvT44p9ecwBBqTwgJVq0+4BW1g8OW01TyMEGWBHbyMOYVHXslaut7qEQ1meATXgw==} + multer@2.1.1: + resolution: {integrity: sha512-mo+QTzKlx8R7E5ylSXxWzGoXoZbOsRMpyitcht8By2KHvMbf3tjwosZ/Mu/XYU6UuJ3VZnODIrak5ZrPiPyB6A==} engines: {node: '>= 10.16.0'} mute-stream@2.0.0: @@ -3824,9 +3827,6 @@ packages: resolution: {integrity: sha512-xx0kgFxSHWY9aG1109uv4w2b+JLwHseSowOWo1bzCTDBpUk3er2rZdtQ90mAjUYbkh6Hus9DAwWvmHsX5pHaIQ==} engines: {node: ^8.10.0 || ^10.13.0 || >=11.10.1} - randombytes@2.1.0: - resolution: {integrity: sha512-vYl3iOX+4CKUWuxGi9Ukhie6fsqXqS9FE2Zaic4tNFD2N2QQaXOMFbuKK4QmDHC0JO6B1Zp41J0LpT0oR68amQ==} - range-parser@1.2.1: resolution: {integrity: sha512-Hrgsx+orqoygnmhFbKaHE6c296J+HTAQXoxEF6gNupROmmGJRoyzfG3ccAveqCBrwr/2yxQ5BVd/GTl5agOwSg==} engines: {node: '>= 0.6'} @@ -3998,8 +3998,9 @@ packages: seq-queue@0.0.5: resolution: {integrity: sha512-hr3Wtp/GZIc/6DAGPDcV4/9WoZhjrkXsi5B/07QgX8tsdc6ilr7BFM6PM6rbdAX1kFSDYeZGLipIZZKyQP0O5Q==} - serialize-javascript@6.0.2: - resolution: {integrity: sha512-Saa1xPByTTq2gdeFZYLLo+RFE35NHZkAbqZeWNd3BpzppeVisAqpDjcp8dyf6uIvEqJRd46jemmyA4iFIeVk8g==} + serialize-javascript@7.0.3: + resolution: {integrity: sha512-h+cZ/XXarqDgCjo+YSyQU/ulDEESGGf8AMK9pPNmhNSl/FzPl6L8pMp1leca5z6NuG6tvV/auC8/43tmovowww==} + engines: {node: '>=20.0.0'} serve-static@2.2.1: resolution: {integrity: sha512-xRXBn0pPqQTVQiC8wyQrKs2MOlX24zQ0POGaj0kultvoOCstBQM5yvOhAVSUwOMjQtTvsPWoNCHfPGwaaQJhTw==} @@ -5087,7 +5088,7 @@ snapshots: '@aws-sdk/xml-builder@3.972.4': dependencies: '@smithy/types': 4.12.0 - fast-xml-parser: 5.3.6 + fast-xml-parser: 5.3.8 tslib: 2.8.1 '@aws/lambda-invoke-store@0.2.3': {} @@ -5334,7 +5335,7 @@ snapshots: dependencies: '@eslint/object-schema': 2.1.7 debug: 4.4.3 - minimatch: 10.2.1 + minimatch: 10.2.3 transitivePeerDependencies: - supports-color @@ -5355,7 +5356,7 @@ snapshots: ignore: 5.3.2 import-fresh: 3.3.1 js-yaml: 4.1.1 - minimatch: 10.2.1 + minimatch: 10.2.3 strip-json-comments: 3.1.1 transitivePeerDependencies: - supports-color @@ -5385,9 +5386,9 @@ snapshots: dependencies: '@hapi/hoek': 11.0.7 - '@hono/node-server@1.19.9(hono@4.11.10)': + '@hono/node-server@1.19.10(hono@4.12.4)': dependencies: - hono: 4.11.10 + hono: 4.12.4 '@humanfs/core@0.19.1': {} @@ -5904,7 +5905,7 @@ snapshots: '@nestjs/core': 11.1.13(@nestjs/common@11.1.13(class-transformer@0.5.1)(class-validator@0.14.3)(reflect-metadata@0.2.2)(rxjs@7.8.2))(@nestjs/platform-express@11.1.13)(reflect-metadata@0.2.2)(rxjs@7.8.2) cors: 2.8.6 express: 5.2.1 - multer: 2.0.2 + multer: 2.1.1 path-to-regexp: 8.3.0 tslib: 2.8.1 transitivePeerDependencies: @@ -6003,13 +6004,13 @@ snapshots: '@electric-sql/pglite': 0.3.15 '@electric-sql/pglite-socket': 0.0.20(@electric-sql/pglite@0.3.15) '@electric-sql/pglite-tools': 0.2.20(@electric-sql/pglite@0.3.15) - '@hono/node-server': 1.19.9(hono@4.11.10) + '@hono/node-server': 1.19.10(hono@4.12.4) '@mrleebo/prisma-ast': 0.13.1 '@prisma/get-platform': 7.2.0 '@prisma/query-plan-executor': 7.2.0 foreground-child: 3.3.1 get-port-please: 3.2.0 - hono: 4.11.10 + hono: 4.12.4 http-status-codes: 2.3.0 pathe: 2.0.3 proper-lockfile: 4.1.2 @@ -6421,7 +6422,7 @@ snapshots: '@xhmikosr/bin-wrapper': 13.2.0 commander: 8.3.0 fast-glob: 3.3.3 - minimatch: 10.2.1 + minimatch: 10.2.3 piscina: 4.9.2 semver: 7.7.4 slash: 3.0.0 @@ -6733,7 +6734,7 @@ snapshots: '@typescript-eslint/types': 8.54.0 '@typescript-eslint/visitor-keys': 8.54.0 debug: 4.4.3 - minimatch: 10.2.1 + minimatch: 10.2.3 semver: 7.7.4 tinyglobby: 0.2.15 ts-api-utils: 2.4.0(typescript@5.9.3) @@ -7610,7 +7611,7 @@ snapshots: is-glob: 4.0.3 json-stable-stringify-without-jsonify: 1.0.1 lodash.merge: 4.6.2 - minimatch: 10.2.1 + minimatch: 10.2.3 natural-compare: 1.4.0 optionator: 0.9.4 optionalDependencies: @@ -7742,7 +7743,7 @@ snapshots: fast-uri@3.1.0: {} - fast-xml-parser@5.3.6: + fast-xml-parser@5.3.8: dependencies: strnum: 2.1.2 @@ -7844,7 +7845,7 @@ snapshots: deepmerge: 4.3.1 fs-extra: 10.1.0 memfs: 3.5.3 - minimatch: 10.2.1 + minimatch: 10.2.3 node-abort-controller: 3.1.1 schema-utils: 3.3.0 semver: 7.7.4 @@ -7940,7 +7941,7 @@ snapshots: glob@13.0.0: dependencies: - minimatch: 10.2.1 + minimatch: 10.2.3 minipass: 7.1.2 path-scurry: 2.0.1 @@ -7948,7 +7949,7 @@ snapshots: dependencies: inflight: 1.0.6 inherits: 2.0.4 - minimatch: 10.2.1 + minimatch: 10.2.3 once: 1.4.0 path-is-absolute: 1.0.1 optional: true @@ -7958,7 +7959,7 @@ snapshots: fs.realpath: 1.0.0 inflight: 1.0.6 inherits: 2.0.4 - minimatch: 10.2.1 + minimatch: 10.2.3 once: 1.4.0 path-is-absolute: 1.0.1 @@ -8019,7 +8020,7 @@ snapshots: hdr-histogram-percentiles-obj@3.0.0: {} - hono@4.11.10: {} + hono@4.12.4: {} html-escaper@2.0.2: {} @@ -8715,7 +8716,7 @@ snapshots: mimic-response@4.0.0: {} - minimatch@10.2.1: + minimatch@10.2.3: dependencies: brace-expansion: 5.0.2 @@ -8726,21 +8727,19 @@ snapshots: mkdirp@0.5.6: dependencies: minimist: 1.2.8 + optional: true moment@2.30.1: optional: true ms@2.1.3: {} - multer@2.0.2: + multer@2.1.1: dependencies: append-field: 1.0.0 busboy: 1.6.0 concat-stream: 2.0.0 - mkdirp: 0.5.6 - object-assign: 4.1.1 type-is: 1.6.18 - xtend: 4.0.2 mute-stream@2.0.0: {} @@ -9054,10 +9053,6 @@ snapshots: lodash: 4.17.23 reconnect-core: 1.3.0 - randombytes@2.1.0: - dependencies: - safe-buffer: 5.2.1 - range-parser@1.2.1: {} raw-body@3.0.2: @@ -9223,9 +9218,7 @@ snapshots: seq-queue@0.0.5: {} - serialize-javascript@6.0.2: - dependencies: - randombytes: 2.1.0 + serialize-javascript@7.0.3: {} serve-static@2.2.1: dependencies: @@ -9440,7 +9433,7 @@ snapshots: '@jridgewell/trace-mapping': 0.3.31 jest-worker: 27.5.1 schema-utils: 4.3.3 - serialize-javascript: 6.0.2 + serialize-javascript: 7.0.3 terser: 5.46.0 webpack: 5.104.1(@swc/core@1.15.11) optionalDependencies: @@ -9457,7 +9450,7 @@ snapshots: dependencies: '@istanbuljs/schema': 0.1.3 glob: 7.2.3 - minimatch: 10.2.1 + minimatch: 10.2.3 text-decoder@1.2.3: dependencies: diff --git a/src/shared/modules/global/logger.service.spec.ts b/src/shared/modules/global/logger.service.spec.ts new file mode 100644 index 0000000..2892f1f --- /dev/null +++ b/src/shared/modules/global/logger.service.spec.ts @@ -0,0 +1,56 @@ +import { LoggerService } from './logger.service'; + +type LoggerServicePrivate = { + serializeMessage: (message: unknown) => string; +}; + +describe('LoggerService', () => { + const serializeMessage = ( + service: LoggerService, + message: unknown, + ): string => { + return (service as unknown as LoggerServicePrivate).serializeMessage( + message, + ); + }; + + it('redacts secret-like values embedded in string messages', () => { + const service = new LoggerService('LoggerServiceSpec'); + + expect( + serializeMessage( + service, + 'AUTH_SECRET=super-secret Authorization: Bearer abc.def.ghi password:topsecret', + ), + ).toBe( + 'AUTH_SECRET=[REDACTED] Authorization: [REDACTED] password:[REDACTED]', + ); + }); + + it('omits object payload contents from log output', () => { + const service = new LoggerService('LoggerServiceSpec'); + + expect( + serializeMessage(service, { + AUTH0_CLIENT_SECRET: 'super-secret', + harmless: 'value', + }), + ).toBe('[redacted object payload]'); + }); + + it('omits array payload contents from log output', () => { + const service = new LoggerService('LoggerServiceSpec'); + + expect(serializeMessage(service, ['secret-token', 'another-value'])).toBe( + '[redacted array payload]', + ); + }); + + it('sanitizes error messages before logging them', () => { + const service = new LoggerService('LoggerServiceSpec'); + + expect( + serializeMessage(service, new Error('client_secret=my-secret-value')), + ).toBe('Error: client_secret=[REDACTED]'); + }); +}); diff --git a/src/shared/modules/global/logger.service.ts b/src/shared/modules/global/logger.service.ts index fddff34..2b24a1b 100644 --- a/src/shared/modules/global/logger.service.ts +++ b/src/shared/modules/global/logger.service.ts @@ -16,11 +16,18 @@ import { createLogger, format, Logger, transports } from 'winston'; * Global logger service implementing NestJS LoggerService contract. */ export class LoggerService implements NestLoggerService { + private static readonly SENSITIVE_VALUE_PATTERN = + /\b([A-Za-z0-9_-]*(?:api[_-]?key|client[_-]?secret|cookie|pass(?:word)?|private[_-]?key|secret|session|token)[A-Za-z0-9_-]*\b\s*[:=]\s*)([^,\s;]+)/gi; + private static readonly SENSITIVE_JSON_VALUE_PATTERN = + /("(?:[A-Za-z0-9_-]*(?:api[_-]?key|client[_-]?secret|cookie|pass(?:word)?|private[_-]?key|secret|session|token)[A-Za-z0-9_-]*)"\s*:\s*")([^"]+)(")/gi; + private static readonly AUTHORIZATION_HEADER_PATTERN = + /\b(Authorization\s*:\s*)(?:Bearer|Basic)\s+[^,\s;]+/gi; + private static readonly BEARER_TOKEN_PATTERN = + /\b(Bearer\s+)[A-Za-z0-9\-._~+/]+=*\b/gi; private context?: string; private readonly logger: Logger; // TODO (quality): Each LoggerService instance creates its own Winston logger instance. For high-throughput services, consider sharing a single Winston logger instance and passing context as metadata to reduce overhead. - // TODO (security): Log messages are not sanitized. Sensitive values (tokens, passwords, PII) passed as message arguments will appear in logs. Consider adding a sanitization step in serializeMessage(). // TODO (quality): LOG_LEVEL from env is not validated against Winston's accepted levels. An invalid value will silently default to Winston's behaviour. Add validation on startup. /** * Creates a context-aware logger instance. @@ -99,7 +106,7 @@ export class LoggerService implements NestLoggerService { if (trace) { this.printMessage( 'error', - `${this.serializeMessage(message)} | trace=${trace}`, + `${this.serializeMessage(message)} | trace=${this.sanitizeString(trace)}`, context || this.context, ); return; @@ -162,22 +169,67 @@ export class LoggerService implements NestLoggerService { } /** - * Converts non-string message payloads to string form. + * Converts message payloads into a safe string form for logging. * - * Uses JSON serialization when possible with `String(...)` fallback. + * Primitive values are stringified, string content is sanitized for common + * secret patterns, and object/array payloads are replaced with a fixed + * placeholder to avoid leaking environment/configuration data to logs. * * @param {any} message Message payload. * @returns {string} Serialized message. */ private serializeMessage(message: any): string { if (typeof message === 'string') { - return message; + return this.sanitizeString(message); } - try { - return JSON.stringify(message); - } catch { + if ( + typeof message === 'number' || + typeof message === 'bigint' || + typeof message === 'boolean' + ) { return String(message); } + + if (message instanceof Error) { + return this.serializeError(message); + } + + if (Array.isArray(message)) { + return '[redacted array payload]'; + } + + if (message && typeof message === 'object') { + return '[redacted object payload]'; + } + + return this.sanitizeString(String(message)); + } + + /** + * Converts an error object into a single sanitized log string. + * + * @param {Error} error Error instance being logged. + * @returns {string} Sanitized error summary. + */ + private serializeError(error: Error): string { + return `${error.name}: ${this.sanitizeString(error.message)}`; + } + + /** + * Masks common secret/token formats embedded in string log messages. + * + * @param {string} value Raw string message or stack trace. + * @returns {string} Sanitized string with secret values redacted. + */ + private sanitizeString(value: string): string { + return value + .replace( + LoggerService.AUTHORIZATION_HEADER_PATTERN, + '$1[REDACTED]', + ) + .replace(LoggerService.BEARER_TOKEN_PATTERN, '$1[REDACTED]') + .replace(LoggerService.SENSITIVE_JSON_VALUE_PATTERN, '$1[REDACTED]$3') + .replace(LoggerService.SENSITIVE_VALUE_PATTERN, '$1[REDACTED]'); } } From 4f14fb938abda83ca83a3ebf14f766316b992a85 Mon Sep 17 00:00:00 2001 From: Justin Gasper Date: Tue, 10 Mar 2026 11:39:28 +1100 Subject: [PATCH 2/8] Build fix --- .dockerignore | 9 +++++++++ Dockerfile | 2 +- appStartUp.sh | 2 +- 3 files changed, 11 insertions(+), 2 deletions(-) create mode 100644 .dockerignore diff --git a/.dockerignore b/.dockerignore new file mode 100644 index 0000000..2b93ae4 --- /dev/null +++ b/.dockerignore @@ -0,0 +1,9 @@ +.git +.github +node_modules +dist +coverage +.env +.env.* +npm-debug.log* +pnpm-debug.log* diff --git a/Dockerfile b/Dockerfile index 559daad..882dd3b 100644 --- a/Dockerfile +++ b/Dockerfile @@ -13,7 +13,7 @@ ENV PRISMA_CLI_BINARY_TARGETS=linux-musl-openssl-3.0.x WORKDIR /app COPY --chown=node:node . . RUN npm install pnpm -g -RUN pnpm install +RUN pnpm install --frozen-lockfile RUN pnpm run build RUN chmod +x appStartUp.sh diff --git a/appStartUp.sh b/appStartUp.sh index c60b4b9..d2a491d 100755 --- a/appStartUp.sh +++ b/appStartUp.sh @@ -4,4 +4,4 @@ set -eo pipefail export DATABASE_URL=$(echo -e ${DATABASE_URL}) # Start the app -pnpm start:prod +exec node dist/src/main From bb644fb6c8b67bdd493af5b6d451d86158ea8327 Mon Sep 17 00:00:00 2001 From: Justin Gasper Date: Tue, 10 Mar 2026 16:37:10 +1100 Subject: [PATCH 3/8] QA fixes --- README.md | 6 +- docs/DIFFERENCES_FROM_V5.md | 2 +- docs/MIGRATION_FROM_TC_PROJECT_SERVICE.md | 3 +- docs/api-usage-analysis.md | 2 +- .../project-invite.controller.spec.ts | 71 +++++++ .../project-invite.controller.ts | 18 +- .../dto/create-member.dto.spec.ts | 26 +++ .../project-member/dto/create-member.dto.ts | 13 +- .../project-member.service.spec.ts | 23 ++- .../project-member/project-member.service.ts | 43 +++- src/api/project/project.controller.ts | 14 +- src/api/project/project.service.spec.ts | 189 ++++++++++++++++++ src/api/project/project.service.ts | 146 +++++++++----- test/project-invite.e2e-spec.ts | 35 +++- 14 files changed, 510 insertions(+), 81 deletions(-) create mode 100644 src/api/project-member/dto/create-member.dto.spec.ts diff --git a/README.md b/README.md index 0b6d826..56ead77 100644 --- a/README.md +++ b/README.md @@ -96,7 +96,7 @@ For the full v5 -> v6 mapping table, see `docs/api-usage-analysis.md`. | `DELETE` | `/v6/projects/:projectId` | Admin only | Soft-delete project | | `GET` | `/v6/projects/:projectId/billingAccount` | JWT / M2M | Default billing account (Salesforce) | | `GET` | `/v6/projects/:projectId/billingAccounts` | JWT / M2M | All billing accounts for project | -| `GET` | `/v6/projects/:projectId/permissions` | JWT / M2M | JWT: caller work-management policy map. M2M: per-member permission matrix with project permissions and template policies | +| `GET` | `/v6/projects/:projectId/permissions` | JWT / M2M | Regular human JWT: caller work-management policy map. M2M, admins, project managers, and project copilots on the project: per-member permission matrix with project permissions and template policies | ### Members @@ -114,7 +114,7 @@ For the full v5 -> v6 mapping table, see `docs/api-usage-analysis.md`. | --- | --- | --- | --- | | `GET` | `/v6/projects/:projectId/invites` | JWT / M2M | List invites | | `GET` | `/v6/projects/:projectId/invites/:inviteId` | JWT / M2M | Get invite | -| `POST` | `/v6/projects/:projectId/invites` | JWT / M2M | Create invite(s) - partial-success response `{ success[], failed[] }` | +| `POST` | `/v6/projects/:projectId/invites` | JWT / M2M | Create invite(s) - returns `201` when any invite is created and includes `{ success[], failed[] }` for rejected targets | | `PATCH` | `/v6/projects/:projectId/invites/:inviteId` | JWT / M2M | Accept / decline invite | | `DELETE` | `/v6/projects/:projectId/invites/:inviteId` | JWT / M2M | Delete invite | @@ -463,7 +463,7 @@ Open `TODO (quality)` findings from prior phases: - Elasticsearch removed; all reads use PostgreSQL via Prisma. - Timeline/milestone CRUD intentionally not migrated (see `docs/timeline-milestone-migration.md`). - Deprecated endpoints not ported (scope change requests, reports, customer payments, phase members/approvals, estimation items). -- Invite creation uses partial-success semantics: `{ success: Invite[], failed: ErrorInfo[] }`. +- Invite creation returns `201` when any invite is created and still uses `{ success: Invite[], failed: ErrorInfo[] }` for rejected targets. - Event originator changed from `tc-project-service` to `project-service-v6`. Full details: `docs/DIFFERENCES_FROM_V5.md` and `docs/MIGRATION_FROM_TC_PROJECT_SERVICE.md`. diff --git a/docs/DIFFERENCES_FROM_V5.md b/docs/DIFFERENCES_FROM_V5.md index b68dbf0..feaf2aa 100644 --- a/docs/DIFFERENCES_FROM_V5.md +++ b/docs/DIFFERENCES_FROM_V5.md @@ -20,7 +20,7 @@ This document summarizes intentional differences and improvements in `project-se - Work management permission routes use query-parameter lookup patterns for consistency: - `/v6/projects/metadata/workManagementPermission?projectTemplateId=:id` - `/v6/projects/metadata/workManagementPermission?id=:id` -- Invite creation uses partial-success response semantics: +- Invite creation returns `201` when at least one invite is created and keeps partial-success response semantics: - `{ success: Invite[], failed: ErrorInfo[] }` ## Authorization Improvements diff --git a/docs/MIGRATION_FROM_TC_PROJECT_SERVICE.md b/docs/MIGRATION_FROM_TC_PROJECT_SERVICE.md index 3dda4e0..44761d4 100644 --- a/docs/MIGRATION_FROM_TC_PROJECT_SERVICE.md +++ b/docs/MIGRATION_FROM_TC_PROJECT_SERVICE.md @@ -102,6 +102,7 @@ This document maps legacy authorization logic from Express middleware to NestJS ## Consumer Migration Notes - Continue using `fields` query parameter for optional member/invite enrichment. -- Handle `403` responses from invite create as partial-failure responses with body: +- Invite create now returns `201` when at least one invite is created and still includes: - `{ success: Invite[], failed: ErrorInfo[] }` +- Compatibility `403` responses remain possible when no invite is created and the response only contains failures. - If your integration relied on ES stale fields, switch to Member/Identity APIs for user metadata. diff --git a/docs/api-usage-analysis.md b/docs/api-usage-analysis.md index fb58650..8592699 100644 --- a/docs/api-usage-analysis.md +++ b/docs/api-usage-analysis.md @@ -112,7 +112,7 @@ | GET | `/v5/projects/:projectId/attachments` | **unused** | none | none | Attachment array (read-access filtered) | | GET | `/v5/projects/:projectId/phases/:phaseId/products` | **unused** | none | none | Phase product array | | GET | `/v5/projects/:projectId/phases/:phaseId/products/:productId` | **unused** | none | none | Single phase product | -| GET | `/v5/projects/:projectId/permissions` | **unused** | none | none | JWT: policy map `{ [policyName]: true }` for allowed work-management actions. M2M in `/v6`: per-member permission matrix with memberships, project permissions, and template policies | +| GET | `/v5/projects/:projectId/permissions` | **unused** | none | none | JWT: policy map `{ [policyName]: true }` for allowed work-management actions. In `/v6`, M2M/admin/project-manager/project-copilot callers receive a per-member permission matrix with memberships, project permissions, and template policies | | DELETE | `/v5/projects/:projectId` | **unused** | none | none | `204` | | GET | `/v5/projects/:projectId/phases/:phaseId` | **unused** | none | none | Phase object (includes members/approvals where present) | | POST | `/v5/projects/:projectId/phases` | **unused** | none | `{name,status,description?,requirements?,startDate?,endDate?,duration?,budget?,spentBudget?,progress?,details?,order?,productTemplateId?,members?}` | Created phase | diff --git a/src/api/project-invite/project-invite.controller.spec.ts b/src/api/project-invite/project-invite.controller.spec.ts index 39213d9..c85fad5 100644 --- a/src/api/project-invite/project-invite.controller.spec.ts +++ b/src/api/project-invite/project-invite.controller.spec.ts @@ -67,6 +67,77 @@ describe('ProjectInviteController', () => { expect(statusMock).toHaveBeenCalledWith(201); }); + it('keeps 201 when createInvites returns successes and failures', async () => { + serviceMock.createInvites.mockResolvedValue({ + success: [{ id: '1' }], + failed: [ + { + email: 'member@example.com', + message: 'Emails can only be used for customer', + }, + ], + }); + + const statusMock = jest.fn(); + const resMock = { + status: statusMock, + } as unknown as Response; + + const response = await controller.createInvites( + '123', + { + handles: ['member'], + emails: ['member@example.com'], + role: ProjectMemberRole.observer, + }, + undefined, + { + userId: '10', + isMachine: false, + }, + resMock, + ); + + expect(response.success).toHaveLength(1); + expect(response.failed).toHaveLength(1); + expect(statusMock).toHaveBeenCalledWith(201); + }); + + it('sets 403 when createInvites only returns failures', async () => { + serviceMock.createInvites.mockResolvedValue({ + success: [], + failed: [ + { + handle: 'missing-user', + message: 'Unable to invite user', + }, + ], + }); + + const statusMock = jest.fn(); + const resMock = { + status: statusMock, + } as unknown as Response; + + const response = await controller.createInvites( + '123', + { + handles: ['missing-user'], + role: ProjectMemberRole.customer, + }, + undefined, + { + userId: '10', + isMachine: false, + }, + resMock, + ); + + expect(response.success).toHaveLength(0); + expect(response.failed).toHaveLength(1); + expect(statusMock).toHaveBeenCalledWith(403); + }); + it('updates invite', async () => { serviceMock.updateInvite.mockResolvedValue({ id: '2' }); diff --git a/src/api/project-invite/project-invite.controller.ts b/src/api/project-invite/project-invite.controller.ts index c6aef99..bfbb383 100644 --- a/src/api/project-invite/project-invite.controller.ts +++ b/src/api/project-invite/project-invite.controller.ts @@ -45,8 +45,10 @@ import { ProjectInviteService } from './project-invite.service'; /** * REST controller for `/projects/:projectId/invites`. * - * `createInvites` can return HTTP 403 for partial failures while still - * returning successful records in `InviteBulkResponseDto`. + * `createInvites` returns HTTP 201 whenever at least one invite is created, + * even if `InviteBulkResponseDto.failed` contains rejected targets. + * Compatibility `403` responses are reserved for bulk requests that create no + * invites and only report failures. * * `deleteInvite` performs a soft cancel by setting `status = canceled`. */ @@ -93,8 +95,10 @@ export class ProjectInviteController { /** * Creates invites by handles/emails with partial-failure semantics. * - * Full success returns HTTP 201. Partial success returns HTTP 403 with both - * `success` and `failed` payload sections. + * Full success returns HTTP 201. Mixed success/failure also returns HTTP 201 + * with both `success` and `failed` payload sections. Compatibility `403` + * responses are reserved for requests that create no invites and only + * populate `failed`. * * @param projectId Project identifier from the route. * @param dto Invite creation payload. @@ -141,7 +145,11 @@ export class ProjectInviteController { fields, ); - if (response.failed && response.failed.length > 0) { + const createdInviteCount = Array.isArray(response.success) + ? response.success.length + : 0; + + if (createdInviteCount === 0 && response.failed?.length) { res.status(403); return response; } diff --git a/src/api/project-member/dto/create-member.dto.spec.ts b/src/api/project-member/dto/create-member.dto.spec.ts new file mode 100644 index 0000000..98807e8 --- /dev/null +++ b/src/api/project-member/dto/create-member.dto.spec.ts @@ -0,0 +1,26 @@ +import { BadRequestException } from '@nestjs/common'; +import { ProjectMemberRole } from '@prisma/client'; +import { plainToInstance } from 'class-transformer'; +import { validateSync } from 'class-validator'; +import { CreateMemberDto } from './create-member.dto'; + +describe('CreateMemberDto', () => { + it('rejects non-numeric user ids during transformation', () => { + expect(() => + plainToInstance(CreateMemberDto, { + userId: 'invalid', + role: ProjectMemberRole.customer, + }), + ).toThrow(BadRequestException); + }); + + it('accepts numeric-string user ids', () => { + const dto = plainToInstance(CreateMemberDto, { + userId: '456', + role: ProjectMemberRole.customer, + }); + + expect(dto.userId).toBe(456); + expect(validateSync(dto)).toEqual([]); + }); +}); diff --git a/src/api/project-member/dto/create-member.dto.ts b/src/api/project-member/dto/create-member.dto.ts index 4e8b1cf..24eaf78 100644 --- a/src/api/project-member/dto/create-member.dto.ts +++ b/src/api/project-member/dto/create-member.dto.ts @@ -2,25 +2,28 @@ import { ProjectMemberRole } from '@prisma/client'; import { ApiProperty, ApiPropertyOptional } from '@nestjs/swagger'; import { Transform } from 'class-transformer'; import { IsEnum, IsNumber, IsOptional } from 'class-validator'; +import { parseNumericStringId } from 'src/shared/utils/service.utils'; /** * Parses optional integer-like input values from query/body payloads. * * @param value Raw unknown value from the incoming payload. * @returns A truncated number when parseable, otherwise `undefined`. - * @throws {TypeError} Propagates only if value conversion throws unexpectedly. + * @throws {BadRequestException} If a provided value is not a numeric string or + * finite number. */ function parseOptionalInteger(value: unknown): number | undefined { if (typeof value === 'undefined' || value === null || value === '') { return undefined; } - const parsed = Number(value); - if (Number.isNaN(parsed)) { - return undefined; + if (typeof value === 'number') { + return Number( + parseNumericStringId(String(Math.trunc(value)), 'User id'), + ); } - return Math.trunc(parsed); + return Number(parseNumericStringId(String(value), 'User id')); } /** diff --git a/src/api/project-member/project-member.service.spec.ts b/src/api/project-member/project-member.service.spec.ts index 8193ed8..3ac641d 100644 --- a/src/api/project-member/project-member.service.spec.ts +++ b/src/api/project-member/project-member.service.spec.ts @@ -1,4 +1,4 @@ -import { ForbiddenException } from '@nestjs/common'; +import { BadRequestException, ForbiddenException } from '@nestjs/common'; import { ProjectMemberRole } from '@prisma/client'; import { Permission } from 'src/shared/constants/permissions'; import { KAFKA_TOPIC } from 'src/shared/config/kafka.config'; @@ -177,6 +177,27 @@ describe('ProjectMemberService', () => { }); }); + it('rejects invalid target user ids before querying the project', async () => { + await expect( + service.addMember( + '1001', + { + userId: 'invalid' as unknown as number, + role: ProjectMemberRole.customer, + }, + { + userId: '123', + roles: ['Topcoder User'], + isMachine: false, + }, + undefined, + ), + ).rejects.toBeInstanceOf(BadRequestException); + + expect(prismaMock.project.findFirst).not.toHaveBeenCalled(); + expect(prismaMock.$transaction).not.toHaveBeenCalled(); + }); + it('updates a project member for machine principals inferred from token claims', async () => { prismaMock.project.findFirst.mockResolvedValue({ id: BigInt(1001), diff --git a/src/api/project-member/project-member.service.ts b/src/api/project-member/project-member.service.ts index 110cee3..8da8baa 100644 --- a/src/api/project-member/project-member.service.ts +++ b/src/api/project-member/project-member.service.ts @@ -86,6 +86,7 @@ export class ProjectMemberService { const parsedProjectId = this.parseId(projectId, 'Project'); const auditUserId = this.getAuditUserId(user); const actorUserId = this.getActorUserId(user); + const targetUserId = this.resolveTargetUserId(dto.userId, actorUserId); const project = await this.prisma.project.findFirst({ where: { @@ -107,11 +108,6 @@ export class ProjectMemberService { ); } - const targetUserId = - typeof dto.userId === 'number' && Number.isFinite(dto.userId) - ? String(Math.trunc(dto.userId)) - : actorUserId; - const isOwnMember = targetUserId === actorUserId; if ( @@ -820,6 +816,43 @@ export class ProjectMemberService { return parseNumericStringId(value, `${label} id`); } + /** + * Resolves the target project-member user id from request payload state. + * + * Defaults to the authenticated actor when `dto.userId` is omitted, and + * rejects provided values that are not numeric. + * + * @param userId Raw `CreateMemberDto.userId` payload value. + * @param actorUserId Authenticated caller id used as the default target. + * @returns Normalized target user id string. + * @throws {BadRequestException} If a provided `userId` is not numeric. + */ + private resolveTargetUserId( + userId: CreateMemberDto['userId'] | string | bigint | null | undefined, + actorUserId: string, + ): string { + if (typeof userId === 'undefined' || userId === null) { + return actorUserId; + } + + if (typeof userId === 'number') { + if (!Number.isFinite(userId)) { + throw new BadRequestException('User id must be a numeric string.'); + } + + return parseNumericStringId( + String(Math.trunc(userId)), + 'User id', + ).toString(); + } + + if (typeof userId === 'string' || typeof userId === 'bigint') { + return parseNumericStringId(String(userId), 'User id').toString(); + } + + throw new BadRequestException('User id must be a numeric string.'); + } + /** * Resolves the authenticated actor id as a trimmed string. * diff --git a/src/api/project/project.controller.ts b/src/api/project/project.controller.ts index 62ea401..7a46170 100644 --- a/src/api/project/project.controller.ts +++ b/src/api/project/project.controller.ts @@ -315,9 +315,10 @@ export class ProjectController { * * @param projectId Project id path parameter. * @param user Authenticated caller context. - * @returns JWT callers receive a caller policy map. M2M callers receive a - * per-user matrix containing memberships, Topcoder roles, named project - * permissions, and template work-management policies. + * @returns Non-privileged human callers receive a caller policy map. M2M, + * admins, global project managers, and project copilots on the requested + * project receive a per-user matrix containing memberships, Topcoder roles, + * named project permissions, and template work-management policies. * @throws BadRequestException When `projectId` is not numeric. * @throws UnauthorizedException When the caller is unauthenticated. * @throws ForbiddenException When caller cannot access the project. @@ -332,7 +333,9 @@ export class ProjectController { Scope.PROJECTS_ALL, Scope.CONNECT_PROJECT_ADMIN, ) - @RequirePermission(Permission.VIEW_PROJECT) + @RequirePermission(Permission.VIEW_PROJECT, { + topcoderRoles: [UserRole.PROJECT_MANAGER], + }) @ApiOperation({ summary: 'Get user permissions for project' }) @ApiParam({ name: 'projectId', @@ -341,7 +344,8 @@ export class ProjectController { }) @ApiResponse({ status: 200, - description: 'JWT caller policy map or M2M per-user permission matrix', + description: + 'Caller policy map for regular human JWTs, or a per-user permission matrix for M2M/admin/project-manager/project-copilot callers', schema: { oneOf: [ { diff --git a/src/api/project/project.service.spec.ts b/src/api/project/project.service.spec.ts index 380671f..3793c28 100644 --- a/src/api/project/project.service.spec.ts +++ b/src/api/project/project.service.spec.ts @@ -2,6 +2,7 @@ import { ForbiddenException, NotFoundException } from '@nestjs/common'; import { Permission } from 'src/shared/constants/permissions'; import { KAFKA_TOPIC } from 'src/shared/config/kafka.config'; import { Scope } from 'src/shared/enums/scopes.enum'; +import { UserRole } from 'src/shared/enums/userRole.enum'; import { PermissionService } from 'src/shared/services/permission.service'; import { ProjectService } from './project.service'; @@ -814,6 +815,194 @@ describe('ProjectService', () => { expect(memberServiceMock.getUserRoles).toHaveBeenCalledWith('200'); }); + it.each([ + [ + 'administrator', + { + userId: '999', + roles: [UserRole.TOPCODER_ADMIN], + isMachine: false, + }, + ], + [ + 'project manager', + { + userId: '999', + roles: [UserRole.PROJECT_MANAGER], + isMachine: false, + }, + ], + ])( + 'returns a per-user permission matrix for %s callers on all projects', + async ( + _label: string, + caller: { + userId: string; + roles: string[]; + isMachine: boolean; + }, + ) => { + prismaMock.project.findFirst.mockResolvedValue({ + templateId: null, + }); + prismaMock.projectMember.findMany.mockResolvedValue([ + { + id: BigInt(1), + projectId: BigInt(1001), + userId: BigInt(100), + role: 'manager', + isPrimary: true, + deletedAt: null, + }, + { + id: BigInt(2), + projectId: BigInt(1001), + userId: BigInt(200), + role: 'customer', + isPrimary: false, + deletedAt: null, + }, + ]); + permissionServiceMock.isNamedPermissionRequireProjectMembers.mockImplementation( + (permission: Permission) => + [Permission.VIEW_PROJECT, Permission.EDIT_PROJECT].includes( + permission, + ), + ); + permissionServiceMock.hasNamedPermission.mockImplementation( + ( + permission: Permission, + _matrixUser: { userId?: string }, + members: any[], + ) => + permission === Permission.VIEW_PROJECT || + (permission === Permission.EDIT_PROJECT && + members[0]?.role === 'manager'), + ); + memberServiceMock.getUserRoles.mockResolvedValue([]); + + const result = await service.getProjectPermissions('1001', caller); + + expect(result).toEqual({ + '100': { + memberships: [ + { + memberId: '1', + role: 'manager', + isPrimary: true, + }, + ], + topcoderRoles: [], + projectPermissions: { + VIEW_PROJECT: true, + EDIT_PROJECT: true, + }, + workManagementPolicies: {}, + }, + '200': { + memberships: [ + { + memberId: '2', + role: 'customer', + isPrimary: false, + }, + ], + topcoderRoles: [], + projectPermissions: { + VIEW_PROJECT: true, + }, + workManagementPolicies: {}, + }, + }); + expect(prismaMock.workManagementPermission.findMany).not.toHaveBeenCalled(); + expect(memberServiceMock.getUserRoles).toHaveBeenCalledWith('100'); + expect(memberServiceMock.getUserRoles).toHaveBeenCalledWith('200'); + }, + ); + + it('returns a per-user permission matrix for copilot members on the requested project', async () => { + prismaMock.project.findFirst.mockResolvedValue({ + templateId: null, + }); + prismaMock.projectMember.findMany.mockResolvedValue([ + { + id: BigInt(1), + projectId: BigInt(1001), + userId: BigInt(100), + role: 'manager', + isPrimary: true, + deletedAt: null, + }, + { + id: BigInt(3), + projectId: BigInt(1001), + userId: BigInt(300), + role: 'copilot', + isPrimary: false, + deletedAt: null, + }, + ]); + permissionServiceMock.isNamedPermissionRequireProjectMembers.mockImplementation( + (permission: Permission) => + [Permission.VIEW_PROJECT, Permission.EDIT_PROJECT].includes( + permission, + ), + ); + permissionServiceMock.hasNamedPermission.mockImplementation( + ( + permission: Permission, + _matrixUser: { userId?: string }, + members: any[], + ) => + permission === Permission.VIEW_PROJECT || + (permission === Permission.EDIT_PROJECT && + ['manager', 'copilot'].includes(members[0]?.role)), + ); + memberServiceMock.getUserRoles.mockResolvedValue([]); + + const result = await service.getProjectPermissions('1001', { + userId: '300', + roles: [UserRole.TOPCODER_USER], + isMachine: false, + }); + + expect(result).toEqual({ + '100': { + memberships: [ + { + memberId: '1', + role: 'manager', + isPrimary: true, + }, + ], + topcoderRoles: [], + projectPermissions: { + VIEW_PROJECT: true, + EDIT_PROJECT: true, + }, + workManagementPolicies: {}, + }, + '300': { + memberships: [ + { + memberId: '3', + role: 'copilot', + isPrimary: false, + }, + ], + topcoderRoles: [], + projectPermissions: { + VIEW_PROJECT: true, + EDIT_PROJECT: true, + }, + workManagementPolicies: {}, + }, + }); + expect(prismaMock.workManagementPermission.findMany).not.toHaveBeenCalled(); + expect(memberServiceMock.getUserRoles).toHaveBeenCalledWith('100'); + expect(memberServiceMock.getUserRoles).toHaveBeenCalledWith('300'); + }); + it('creates projects for machine principals inferred from token claims without creating a synthetic owner member', async () => { const transactionProjectCreate = jest.fn().mockResolvedValue({ id: BigInt(1001), diff --git a/src/api/project/project.service.ts b/src/api/project/project.service.ts index 8eae430..e378c48 100644 --- a/src/api/project/project.service.ts +++ b/src/api/project/project.service.ts @@ -17,7 +17,7 @@ import { import { Permission } from 'src/shared/constants/permissions'; import { KAFKA_TOPIC } from 'src/shared/config/kafka.config'; import { Scope } from 'src/shared/enums/scopes.enum'; -import { ADMIN_ROLES } from 'src/shared/enums/userRole.enum'; +import { ADMIN_ROLES, UserRole } from 'src/shared/enums/userRole.enum'; import { Permission as JsonPermission, PermissionRule as JsonPermissionRule, @@ -875,7 +875,9 @@ export class ProjectService { * Returns project permissions for the caller or, for M2M, every project user. * * Human JWT callers keep the legacy v5/v6 behavior and receive the - * work-management policy map allowed for the authenticated caller. + * work-management policy map allowed for the authenticated caller unless + * they are an admin, a global `Project Manager`, or a `copilot` member on + * the requested project. * * M2M callers receive a per-user matrix built from active project members. * Each entry includes the user's active memberships, fetched Topcoder roles, @@ -908,39 +910,35 @@ export class ProjectService { ); } - if (this.isMachinePrincipal(user)) { - const workManagementPermissionsPromise: Promise< - WorkManagementPermissionRecord[] - > = project.templateId - ? this.prisma.workManagementPermission.findMany({ - where: { - projectTemplateId: project.templateId, - deletedAt: null, - }, - select: { - policy: true, - permission: true, - }, - }) - : Promise.resolve([]); + const projectMembers = await this.prisma.projectMember.findMany({ + where: { + projectId: parsedProjectId, + deletedAt: null, + }, + select: { + id: true, + projectId: true, + userId: true, + role: true, + isPrimary: true, + deletedAt: true, + }, + }); - const [projectMembers, workManagementPermissions] = await Promise.all([ - this.prisma.projectMember.findMany({ - where: { - projectId: parsedProjectId, - deletedAt: null, - }, - select: { - id: true, - projectId: true, - userId: true, - role: true, - isPrimary: true, - deletedAt: true, - }, - }), - workManagementPermissionsPromise, - ]); + if (this.shouldReturnProjectMemberPermissionMatrix(user, projectMembers)) { + const workManagementPermissions: WorkManagementPermissionRecord[] = + project.templateId + ? await this.prisma.workManagementPermission.findMany({ + where: { + projectTemplateId: project.templateId, + deletedAt: null, + }, + select: { + policy: true, + permission: true, + }, + }) + : []; return this.buildProjectMemberPermissionMatrix( projectMembers, @@ -952,22 +950,8 @@ export class ProjectService { return {}; } - const [projectMembers, workManagementPermissions] = await Promise.all([ - this.prisma.projectMember.findMany({ - where: { - projectId: parsedProjectId, - deletedAt: null, - }, - select: { - id: true, - projectId: true, - userId: true, - role: true, - isPrimary: true, - deletedAt: true, - }, - }), - this.prisma.workManagementPermission.findMany({ + const workManagementPermissions = + await this.prisma.workManagementPermission.findMany({ where: { projectTemplateId: project.templateId, deletedAt: null, @@ -976,8 +960,7 @@ export class ProjectService { policy: true, permission: true, }, - }), - ]); + }); const policyMap: ProjectPolicyMap = {}; @@ -1763,6 +1746,65 @@ export class ProjectService { return permissionMatrix; } + /** + * Determines whether the caller should receive the per-user permission matrix. + * + * Matrix access is granted to: + * - machine principals + * - admins + * - global `Project Manager` role holders + * - callers who are a `copilot` member on the current project + * + * @param user Authenticated caller context. + * @param projectMembers Active members of the requested project. + * @returns `true` when the route should return the per-user matrix. + */ + private shouldReturnProjectMemberPermissionMatrix( + user: JwtUser, + projectMembers: Array>, + ): boolean { + if (this.isMachinePrincipal(user)) { + return true; + } + + const normalizedUserRoles = new Set( + (user.roles || []) + .map((role) => String(role).trim().toLowerCase()) + .filter((role) => role.length > 0), + ); + const hasGlobalMatrixRole = [...ADMIN_ROLES, UserRole.PROJECT_MANAGER].some( + (role) => + normalizedUserRoles.has(String(role).trim().toLowerCase()), + ); + + if (hasGlobalMatrixRole) { + return true; + } + + const parsedUserId = this.parseUserIdValue(user.userId); + + if (!parsedUserId) { + return false; + } + + const normalizedCopilotRole = String(ProjectMemberRole.copilot) + .trim() + .toLowerCase(); + + return projectMembers.some((member) => { + const parsedMemberUserId = this.parseUserIdValue(member.userId); + + if (!parsedMemberUserId) { + return false; + } + + return ( + parsedMemberUserId.toString() === parsedUserId.toString() && + String(member.role).trim().toLowerCase() === normalizedCopilotRole + ); + }); + } + /** * Returns the authenticated actor user id as trimmed string. * diff --git a/test/project-invite.e2e-spec.ts b/test/project-invite.e2e-spec.ts index 1e9ab19..be65b1f 100644 --- a/test/project-invite.e2e-spec.ts +++ b/test/project-invite.e2e-spec.ts @@ -220,9 +220,40 @@ describe('Project Invite endpoints (e2e)', () => { ); }); - it('returns 403 for partial failures', async () => { + it('returns 201 when at least one invite is created', async () => { + (jwtServiceMock.validateToken as jest.Mock).mockResolvedValueOnce({ + scopes: [Scope.PROJECT_INVITES_WRITE], + isMachine: true, + tokenPayload: { + gty: 'client-credentials', + scope: Scope.PROJECT_INVITES_WRITE, + }, + }); + projectInviteServiceMock.createInvites.mockResolvedValueOnce({ success: [{ id: '1' }], + failed: [ + { + email: 'dilhanigunawardhana+1@gmail.com', + message: 'Emails can only be used for customer', + }, + ], + }); + + await request(app.getHttpServer()) + .post('/v6/projects/1001/invites') + .set('Authorization', 'Bearer m2m-invite-write') + .send({ + handles: ['sdguntcqa'], + emails: ['dilhanigunawardhana+1@gmail.com'], + role: ProjectMemberRole.observer, + }) + .expect(201); + }); + + it('returns 403 when no invites are created', async () => { + projectInviteServiceMock.createInvites.mockResolvedValueOnce({ + success: [], failed: [ { handle: 'missing-user', @@ -234,7 +265,7 @@ describe('Project Invite endpoints (e2e)', () => { await request(app.getHttpServer()) .post('/v6/projects/1001/invites') .set('Authorization', 'Bearer manager-token') - .send({ handles: ['member'], role: ProjectMemberRole.customer }) + .send({ handles: ['missing-user'], role: ProjectMemberRole.customer }) .expect(403); }); From 1490cee2e8df9bc482f52dc833782b040a000396 Mon Sep 17 00:00:00 2001 From: Justin Gasper Date: Tue, 10 Mar 2026 17:43:05 +1100 Subject: [PATCH 4/8] Handle int overflow in provided userID --- .../dto/create-member.dto.spec.ts | 23 ++++++++++- .../project-member/dto/create-member.dto.ts | 41 ++++++++++++------- .../project-member.service.spec.ts | 25 ++++++++++- .../project-member/project-member.service.ts | 6 ++- src/shared/utils/service.utils.ts | 15 ++++++- 5 files changed, 88 insertions(+), 22 deletions(-) diff --git a/src/api/project-member/dto/create-member.dto.spec.ts b/src/api/project-member/dto/create-member.dto.spec.ts index 98807e8..5002db3 100644 --- a/src/api/project-member/dto/create-member.dto.spec.ts +++ b/src/api/project-member/dto/create-member.dto.spec.ts @@ -14,13 +14,32 @@ describe('CreateMemberDto', () => { ).toThrow(BadRequestException); }); - it('accepts numeric-string user ids', () => { + it('accepts numeric-string user ids without coercing them to numbers', () => { const dto = plainToInstance(CreateMemberDto, { userId: '456', role: ProjectMemberRole.customer, }); - expect(dto.userId).toBe(456); + expect(dto.userId).toBe('456'); expect(validateSync(dto)).toEqual([]); }); + + it('preserves numeric-string user ids above Number.MAX_SAFE_INTEGER', () => { + const dto = plainToInstance(CreateMemberDto, { + userId: '9007199254740993', + role: ProjectMemberRole.customer, + }); + + expect(dto.userId).toBe('9007199254740993'); + expect(validateSync(dto)).toEqual([]); + }); + + it('rejects user ids outside the supported bigint range', () => { + expect(() => + plainToInstance(CreateMemberDto, { + userId: '9223372036854775808', + role: ProjectMemberRole.customer, + }), + ).toThrow(BadRequestException); + }); }); diff --git a/src/api/project-member/dto/create-member.dto.ts b/src/api/project-member/dto/create-member.dto.ts index 24eaf78..6e60a29 100644 --- a/src/api/project-member/dto/create-member.dto.ts +++ b/src/api/project-member/dto/create-member.dto.ts @@ -1,43 +1,54 @@ +import { BadRequestException } from '@nestjs/common'; import { ProjectMemberRole } from '@prisma/client'; import { ApiProperty, ApiPropertyOptional } from '@nestjs/swagger'; import { Transform } from 'class-transformer'; -import { IsEnum, IsNumber, IsOptional } from 'class-validator'; +import { IsEnum, IsOptional, IsString } from 'class-validator'; import { parseNumericStringId } from 'src/shared/utils/service.utils'; /** - * Parses optional integer-like input values from query/body payloads. + * Parses optional user ids from query/body payloads while preserving exact + * digits. * * @param value Raw unknown value from the incoming payload. - * @returns A truncated number when parseable, otherwise `undefined`. + * @returns A normalized numeric-string id when parseable, otherwise + * `undefined`. * @throws {BadRequestException} If a provided value is not a numeric string or - * finite number. + * finite number within the supported bigint range. */ -function parseOptionalInteger(value: unknown): number | undefined { +function parseOptionalUserId(value: unknown): string | undefined { if (typeof value === 'undefined' || value === null || value === '') { return undefined; } if (typeof value === 'number') { - return Number( - parseNumericStringId(String(Math.trunc(value)), 'User id'), - ); + return parseNumericStringId( + String(Math.trunc(value)), + 'User id', + ).toString(); } - return Number(parseNumericStringId(String(value), 'User id')); + if (typeof value === 'string' || typeof value === 'bigint') { + return parseNumericStringId(String(value), 'User id').toString(); + } + + throw new BadRequestException('User id must be a numeric string.'); } /** * DTO for creating a project member. * - * Validation requires `role`, while the service still defensively falls back - * to `getDefaultProjectRole` when role is missing in runtime payloads. + * Validation requires `role`. `userId` is normalized to a numeric string so + * values beyond JavaScript's safe integer range are preserved exactly. */ export class CreateMemberDto { - @ApiPropertyOptional({ description: 'User id. Defaults to current user.' }) + @ApiPropertyOptional({ + description: 'User id. Defaults to current user.', + type: String, + }) @IsOptional() - @Transform(({ value }) => parseOptionalInteger(value)) - @IsNumber() - userId?: number; + @Transform(({ value }) => parseOptionalUserId(value)) + @IsString() + userId?: string; @ApiProperty({ enum: ProjectMemberRole, enumName: 'ProjectMemberRole' }) @IsEnum(ProjectMemberRole) diff --git a/src/api/project-member/project-member.service.spec.ts b/src/api/project-member/project-member.service.spec.ts index 3ac641d..2ef87cf 100644 --- a/src/api/project-member/project-member.service.spec.ts +++ b/src/api/project-member/project-member.service.spec.ts @@ -151,7 +151,7 @@ describe('ProjectMemberService', () => { await service.addMember( '1001', { - userId: 456, + userId: '456', role: ProjectMemberRole.customer, }, { @@ -182,7 +182,28 @@ describe('ProjectMemberService', () => { service.addMember( '1001', { - userId: 'invalid' as unknown as number, + userId: 'invalid' as unknown as string, + role: ProjectMemberRole.customer, + }, + { + userId: '123', + roles: ['Topcoder User'], + isMachine: false, + }, + undefined, + ), + ).rejects.toBeInstanceOf(BadRequestException); + + expect(prismaMock.project.findFirst).not.toHaveBeenCalled(); + expect(prismaMock.$transaction).not.toHaveBeenCalled(); + }); + + it('rejects out-of-range target user ids before querying the project', async () => { + await expect( + service.addMember( + '1001', + { + userId: '10000000000000011111', role: ProjectMemberRole.customer, }, { diff --git a/src/api/project-member/project-member.service.ts b/src/api/project-member/project-member.service.ts index 8da8baa..c71dcec 100644 --- a/src/api/project-member/project-member.service.ts +++ b/src/api/project-member/project-member.service.ts @@ -820,12 +820,14 @@ export class ProjectMemberService { * Resolves the target project-member user id from request payload state. * * Defaults to the authenticated actor when `dto.userId` is omitted, and - * rejects provided values that are not numeric. + * rejects provided values that are not numeric or exceed the supported + * signed 64-bit bigint range. * * @param userId Raw `CreateMemberDto.userId` payload value. * @param actorUserId Authenticated caller id used as the default target. * @returns Normalized target user id string. - * @throws {BadRequestException} If a provided `userId` is not numeric. + * @throws {BadRequestException} If a provided `userId` is not numeric or is + * outside the supported bigint range. */ private resolveTargetUserId( userId: CreateMemberDto['userId'] | string | bigint | null | undefined, diff --git a/src/shared/utils/service.utils.ts b/src/shared/utils/service.utils.ts index 9a72309..40032cf 100644 --- a/src/shared/utils/service.utils.ts +++ b/src/shared/utils/service.utils.ts @@ -15,6 +15,8 @@ import { PrismaService } from 'src/shared/modules/global/prisma.service'; import { PermissionService } from 'src/shared/services/permission.service'; import { hasAdminRole } from 'src/shared/utils/permission.utils'; +const MAX_SIGNED_BIGINT_ID = 9223372036854775807n; + /** * Parses an id using `BigInt(...)` semantics and throws on invalid input. */ @@ -28,6 +30,9 @@ export function parseBigIntId(value: string, entityName: string): bigint { /** * Parses a strictly numeric-string id (`/^[0-9]+$/`) as bigint. + * + * Rejects values that exceed the signed 64-bit bigint range supported by the + * backing database schema. */ export function parseNumericStringId(value: string, fieldName: string): bigint { const normalized = String(value || '').trim(); @@ -36,7 +41,15 @@ export function parseNumericStringId(value: string, fieldName: string): bigint { throw new BadRequestException(`${fieldName} must be a numeric string.`); } - return BigInt(normalized); + const parsed = BigInt(normalized); + + if (parsed > MAX_SIGNED_BIGINT_ID) { + throw new BadRequestException( + `${fieldName} must be less than or equal to ${MAX_SIGNED_BIGINT_ID.toString()}.`, + ); + } + + return parsed; } /** From 2c137f10fddd76bd193cc033ea552fa805acc980 Mon Sep 17 00:00:00 2001 From: Justin Gasper Date: Tue, 10 Mar 2026 18:11:43 +1100 Subject: [PATCH 5/8] Check scopes for project member write --- .../services/permission.service.spec.ts | 60 +++++++++++++++++++ src/shared/services/permission.service.ts | 20 ++++++- 2 files changed, 77 insertions(+), 3 deletions(-) diff --git a/src/shared/services/permission.service.spec.ts b/src/shared/services/permission.service.spec.ts index d878512..74af72c 100644 --- a/src/shared/services/permission.service.spec.ts +++ b/src/shared/services/permission.service.spec.ts @@ -240,6 +240,66 @@ describe('PermissionService', () => { expect(allowed).toBe(true); }); + it('allows creating other project members for machine token with project-member write scope', () => { + const allowed = service.hasNamedPermission( + Permission.CREATE_PROJECT_MEMBER_NOT_OWN, + { + scopes: [Scope.PROJECT_MEMBERS_WRITE], + isMachine: true, + }, + ); + + expect(allowed).toBe(true); + }); + + it('allows updating project members for machine token with project-member write scope', () => { + const allowed = service.hasNamedPermission( + Permission.UPDATE_PROJECT_MEMBER_NON_CUSTOMER, + { + scopes: [Scope.PROJECT_MEMBERS_WRITE], + isMachine: true, + }, + ); + + expect(allowed).toBe(true); + }); + + it('allows deleting topcoder project members for machine token with project-member write scope', () => { + const allowed = service.hasNamedPermission( + Permission.DELETE_PROJECT_MEMBER_TOPCODER, + { + scopes: [Scope.PROJECT_MEMBERS_WRITE], + isMachine: true, + }, + ); + + expect(allowed).toBe(true); + }); + + it('allows deleting customer project members for machine token with project-member write scope', () => { + const allowed = service.hasNamedPermission( + Permission.DELETE_PROJECT_MEMBER_CUSTOMER, + { + scopes: [Scope.PROJECT_MEMBERS_WRITE], + isMachine: true, + }, + ); + + expect(allowed).toBe(true); + }); + + it('allows deleting copilot project members for machine token with project-member write scope', () => { + const allowed = service.hasNamedPermission( + Permission.DELETE_PROJECT_MEMBER_COPILOT, + { + scopes: [Scope.PROJECT_MEMBERS_WRITE], + isMachine: true, + }, + ); + + expect(allowed).toBe(true); + }); + it('allows reading other users project invites for machine token with invite read scope', () => { const allowed = service.hasNamedPermission( Permission.READ_PROJECT_INVITE_NOT_OWN, diff --git a/src/shared/services/permission.service.ts b/src/shared/services/permission.service.ts index b1558f5..6a70285 100644 --- a/src/shared/services/permission.service.ts +++ b/src/shared/services/permission.service.ts @@ -195,6 +195,14 @@ export class PermissionService { Scope.PROJECT_MEMBERS_READ, ], ); + const hasProjectMemberWriteScope = this.m2mService.hasRequiredScopes( + effectiveScopes, + [ + Scope.CONNECT_PROJECT_ADMIN, + Scope.PROJECT_MEMBERS_ALL, + Scope.PROJECT_MEMBERS_WRITE, + ], + ); const hasProjectInviteReadScope = this.m2mService.hasRequiredScopes( effectiveScopes, [ @@ -288,17 +296,23 @@ export class PermissionService { case NamedPermission.CREATE_PROJECT_MEMBER_NOT_OWN: case NamedPermission.UPDATE_PROJECT_MEMBER_NON_CUSTOMER: case NamedPermission.DELETE_PROJECT_MEMBER_TOPCODER: - return isAdmin || isManagementMember; + return isAdmin || isManagementMember || hasProjectMemberWriteScope; case NamedPermission.DELETE_PROJECT_MEMBER_CUSTOMER: - return isAdmin || isManagementMember || this.isCopilot(member?.role); + return ( + isAdmin || + isManagementMember || + this.isCopilot(member?.role) || + hasProjectMemberWriteScope + ); case NamedPermission.DELETE_PROJECT_MEMBER_COPILOT: return ( isAdmin || isManagementMember || this.isCopilot(member?.role) || - this.hasCopilotManagerRole(user) + this.hasCopilotManagerRole(user) || + hasProjectMemberWriteScope ); // Project invite read/write permissions. From 618c26eea8271a326550b4c492de53125f5a56db Mon Sep 17 00:00:00 2001 From: Justin Gasper Date: Wed, 18 Mar 2026 07:30:57 +1100 Subject: [PATCH 6/8] Talent manager permissions for BAs (https://topcoder.atlassian.net/browse/PM-4376) --- README.md | 3 + docs/PERMISSIONS.md | 6 ++ src/api/project/project.service.spec.ts | 90 +++++++++++++++++++ src/api/project/project.service.ts | 4 +- src/shared/constants/permissions.constants.ts | 13 ++- .../services/permission.service.spec.ts | 54 +++++++++++ src/shared/services/permission.service.ts | 18 +++- src/shared/utils/permission-docs.utils.ts | 24 ++++- 8 files changed, 205 insertions(+), 7 deletions(-) diff --git a/README.md b/README.md index 56ead77..3fbd5c8 100644 --- a/README.md +++ b/README.md @@ -98,6 +98,9 @@ For the full v5 -> v6 mapping table, see `docs/api-usage-analysis.md`. | `GET` | `/v6/projects/:projectId/billingAccounts` | JWT / M2M | All billing accounts for project | | `GET` | `/v6/projects/:projectId/permissions` | JWT / M2M | Regular human JWT: caller work-management policy map. M2M, admins, project managers, and project copilots on the project: per-member permission matrix with project permissions and template policies | +Talent Manager note: +- `Talent Manager` and `Topcoder Talent Manager` callers create projects as primary `manager` members and are allowed to update project `billingAccountId`. + ### Members | Method | Path | Auth | Description | diff --git a/docs/PERMISSIONS.md b/docs/PERMISSIONS.md index 206942b..ef69400 100644 --- a/docs/PERMISSIONS.md +++ b/docs/PERMISSIONS.md @@ -28,6 +28,12 @@ Swagger auth notes: - `src/shared/enums/projectMemberRole.enum.ts` - `src/shared/enums/scopes.enum.ts` +## Talent Manager Behavior + +- `Talent Manager` and `Topcoder Talent Manager` satisfy `CREATE_PROJECT_AS_MANAGER`, so project creation persists them as the primary `manager` project member. +- That primary `manager` membership then unlocks the standard manager-level project-owner paths, such as edit and delete checks that rely on project-member context. +- The same roles also satisfy `MANAGE_PROJECT_BILLING_ACCOUNT_ID`, which allows them to set or update a project's `billingAccountId`. + ## Permission Rule Shape Supported shapes: diff --git a/src/api/project/project.service.spec.ts b/src/api/project/project.service.spec.ts index 3793c28..5066c2d 100644 --- a/src/api/project/project.service.spec.ts +++ b/src/api/project/project.service.spec.ts @@ -1092,6 +1092,96 @@ describe('ProjectService', () => { expect(transactionProjectHistoryCreate).toHaveBeenCalled(); }); + it('assigns Talent Manager creators the manager project role', async () => { + const transactionProjectCreate = jest.fn().mockResolvedValue({ + id: BigInt(1001), + status: 'in_review', + }); + const transactionProjectMemberCreate = jest.fn().mockResolvedValue({}); + const transactionProjectHistoryCreate = jest.fn().mockResolvedValue({}); + + prismaMock.projectType.findFirst.mockResolvedValue({ + key: 'app', + }); + prismaMock.$transaction.mockImplementation( + async (callback: (tx: unknown) => Promise) => + callback({ + project: { + create: transactionProjectCreate, + }, + projectMember: { + create: transactionProjectMemberCreate, + createMany: jest.fn().mockResolvedValue({ count: 0 }), + }, + projectHistory: { + create: transactionProjectHistoryCreate, + }, + }), + ); + prismaMock.project.findFirst.mockResolvedValue({ + id: BigInt(1001), + name: 'Talent Managed Project', + description: null, + type: 'app', + status: 'in_review', + billingAccountId: null, + directProjectId: null, + estimatedPrice: null, + actualPrice: null, + terms: [], + groups: [], + external: null, + bookmarks: null, + utm: null, + details: null, + challengeEligibility: null, + cancelReason: null, + templateId: null, + version: 'v3', + lastActivityAt: new Date(), + lastActivityUserId: '100', + createdAt: new Date(), + updatedAt: new Date(), + createdBy: 100, + updatedBy: 100, + members: [ + { + userId: BigInt(100), + role: 'manager', + deletedAt: null, + }, + ], + invites: [], + attachments: [], + phases: [], + }); + permissionServiceMock.hasNamedPermission.mockImplementation( + (permission: Permission): boolean => + permission === Permission.CREATE_PROJECT_AS_MANAGER, + ); + + await service.createProject( + { + name: 'Talent Managed Project', + type: 'app', + }, + { + userId: '100', + roles: [UserRole.TALENT_MANAGER], + isMachine: false, + }, + ); + + expect(transactionProjectMemberCreate).toHaveBeenCalledWith({ + data: expect.objectContaining({ + userId: BigInt(100), + role: 'manager', + isPrimary: true, + }), + }); + expect(transactionProjectHistoryCreate).toHaveBeenCalled(); + }); + it('updates projects for machine principals inferred from token claims using fallback audit identity', async () => { const transactionUpdate = jest.fn().mockResolvedValue({ id: BigInt(1001), diff --git a/src/api/project/project.service.ts b/src/api/project/project.service.ts index e378c48..6bc5236 100644 --- a/src/api/project/project.service.ts +++ b/src/api/project/project.service.ts @@ -298,7 +298,9 @@ export class ProjectService { * @throws BadRequestException For invalid type/template/building-block keys. * @throws ConflictException When post-transaction re-fetch fails. * @security Caller permissions determine the primary member role - * (`manager` vs `customer`) assigned on creation. + * (`manager` vs `customer`) assigned on creation. Talent Manager creators + * satisfy `CREATE_PROJECT_AS_MANAGER`, so they are persisted as the primary + * `manager` member for the new project. * @todo Sequential `for...of` loops for estimations and template phases in * the transaction can be optimized with `Promise.all` for larger payloads. */ diff --git a/src/shared/constants/permissions.constants.ts b/src/shared/constants/permissions.constants.ts index f3ea70a..b7d149d 100644 --- a/src/shared/constants/permissions.constants.ts +++ b/src/shared/constants/permissions.constants.ts @@ -203,7 +203,11 @@ export const PERMISSION = { If user has this permission they would join project with "${PROJECT_MEMBER_ROLE.MANAGER}" project role, otherwise with "${PROJECT_MEMBER_ROLE.CUSTOMER}".`, }, - topcoderRoles: TOPCODER_ROLES_MANAGERS_AND_ADMINS, + topcoderRoles: [ + ...TOPCODER_ROLES_ADMINS, + USER_ROLE.TALENT_MANAGER, + USER_ROLE.TOPCODER_TALENT_MANAGER, + ], scopes: SCOPES_PROJECTS_WRITE, }, @@ -369,7 +373,12 @@ export const PERMISSION = { group: 'Project', description: 'Who can set or update the "billingAccountId" property.', }, - topcoderRoles: [USER_ROLE.MANAGER, USER_ROLE.TOPCODER_ADMIN], + topcoderRoles: [ + USER_ROLE.MANAGER, + USER_ROLE.TOPCODER_ADMIN, + USER_ROLE.TALENT_MANAGER, + USER_ROLE.TOPCODER_TALENT_MANAGER, + ], scopes: SCOPES_PROJECTS_WRITE_PROJECTS_BILLING_ACCOUNTS, }, diff --git a/src/shared/services/permission.service.spec.ts b/src/shared/services/permission.service.spec.ts index 74af72c..a9a13ed 100644 --- a/src/shared/services/permission.service.spec.ts +++ b/src/shared/services/permission.service.spec.ts @@ -222,6 +222,60 @@ describe('PermissionService', () => { expect(allowed).toBe(true); }); + it.each([UserRole.TALENT_MANAGER, UserRole.TOPCODER_TALENT_MANAGER])( + 'allows managing project billing accounts for %s Topcoder role', + (role) => { + const allowed = service.hasNamedPermission( + Permission.MANAGE_PROJECT_BILLING_ACCOUNT_ID, + { + userId: '555', + roles: [role], + isMachine: false, + }, + ); + + expect(allowed).toBe(true); + }, + ); + + it.each([UserRole.TALENT_MANAGER, UserRole.TOPCODER_TALENT_MANAGER])( + 'allows creating projects as manager for %s Topcoder role', + (role) => { + const allowed = service.hasNamedPermission( + Permission.CREATE_PROJECT_AS_MANAGER, + { + userId: '555', + roles: [role], + isMachine: false, + }, + ); + + expect(allowed).toBe(true); + }, + ); + + it.each([UserRole.TALENT_MANAGER, UserRole.TOPCODER_TALENT_MANAGER])( + 'allows full project-owner delete access for %s creators once they are manager members', + (role) => { + const allowed = service.hasNamedPermission( + Permission.DELETE_PROJECT, + { + userId: '555', + roles: [role], + isMachine: false, + }, + [ + { + userId: '555', + role: ProjectMemberRole.MANAGER, + }, + ], + ); + + expect(allowed).toBe(true); + }, + ); + it('allows viewing project for machine token with project read scope', () => { const allowed = service.hasNamedPermission(Permission.VIEW_PROJECT, { scopes: [Scope.PROJECTS_READ], diff --git a/src/shared/services/permission.service.ts b/src/shared/services/permission.service.ts index 6a70285..f9b904c 100644 --- a/src/shared/services/permission.service.ts +++ b/src/shared/services/permission.service.ts @@ -376,6 +376,8 @@ export class PermissionService { // Billing-account related permissions. case NamedPermission.MANAGE_PROJECT_BILLING_ACCOUNT_ID: + return isAdmin || this.hasTalentManagerRole(user); + case NamedPermission.MANAGE_PROJECT_DIRECT_PROJECT_ID: return isAdmin; @@ -417,7 +419,8 @@ export class PermissionService { case NamedPermission.CREATE_PROJECT_AS_MANAGER: return this.hasIntersection(user.roles || [], [ ...ADMIN_ROLES, - UserRole.CONNECT_ADMIN, + UserRole.TALENT_MANAGER, + UserRole.TOPCODER_TALENT_MANAGER, ]); // Project attachment permissions. @@ -811,6 +814,19 @@ export class PermissionService { ]); } + /** + * Checks whether user has one of the Talent Manager Topcoder roles. + * + * @param user authenticated JWT user context + * @returns `true` when user has Talent Manager access + */ + private hasTalentManagerRole(user: JwtUser): boolean { + return this.hasIntersection(user.roles || [], [ + UserRole.TALENT_MANAGER, + UserRole.TOPCODER_TALENT_MANAGER, + ]); + } + /** * Checks Topcoder roles allowed to edit projects. * diff --git a/src/shared/utils/permission-docs.utils.ts b/src/shared/utils/permission-docs.utils.ts index 403053c..9370ee8 100644 --- a/src/shared/utils/permission-docs.utils.ts +++ b/src/shared/utils/permission-docs.utils.ts @@ -58,6 +58,11 @@ const ADMIN_AND_MANAGER_ROLES = [ const STRICT_ADMIN_ACCESS_ROLES = [...ADMIN_ROLES]; +const TALENT_MANAGER_ROLES = [ + UserRole.TALENT_MANAGER, + UserRole.TOPCODER_TALENT_MANAGER, +]; + const PROJECT_UPDATE_TOPCODER_ROLES = [ ...ADMIN_AND_MANAGER_ROLES, UserRole.TALENT_MANAGER, @@ -70,8 +75,17 @@ const PROJECT_BILLING_TOPCODER_ROLES = [ UserRole.PROJECT_MANAGER, UserRole.TASK_MANAGER, UserRole.TOPCODER_TASK_MANAGER, - UserRole.TALENT_MANAGER, - UserRole.TOPCODER_TALENT_MANAGER, + ...TALENT_MANAGER_ROLES, +]; + +const PROJECT_BILLING_MANAGEMENT_USER_ROLES = [ + ...ADMIN_AND_MANAGER_ROLES, + ...TALENT_MANAGER_ROLES, +]; + +const PROJECT_CREATOR_MANAGER_USER_ROLES = [ + ...STRICT_ADMIN_ACCESS_ROLES, + ...TALENT_MANAGER_ROLES, ]; const PROJECT_MEMBER_MANAGEMENT_ROLES = [...PROJECT_MEMBER_MANAGER_ROLES]; @@ -355,6 +369,10 @@ function getNamedPermissionDocumentation( }); case NamedPermission.MANAGE_PROJECT_BILLING_ACCOUNT_ID: + return createSummary({ + userRoles: PROJECT_BILLING_MANAGEMENT_USER_ROLES, + }); + case NamedPermission.MANAGE_PROJECT_DIRECT_PROJECT_ID: return createSummary({ userRoles: ADMIN_AND_MANAGER_ROLES, @@ -389,7 +407,7 @@ function getNamedPermissionDocumentation( case NamedPermission.CREATE_PROJECT_AS_MANAGER: return createSummary({ - userRoles: STRICT_ADMIN_ACCESS_ROLES, + userRoles: PROJECT_CREATOR_MANAGER_USER_ROLES, }); case NamedPermission.VIEW_PROJECT_ATTACHMENT: From b620db9700c54906855272720fd9add9bb148438 Mon Sep 17 00:00:00 2001 From: Justin Gasper Date: Wed, 18 Mar 2026 15:53:00 +1100 Subject: [PATCH 7/8] Permissions for managing BAs to be much narrower. --- README.md | 3 +- docs/PERMISSIONS.md | 7 +- src/shared/constants/permissions.constants.ts | 9 +-- .../services/permission.service.spec.ts | 72 ++++++++++++++++++- src/shared/services/permission.service.ts | 9 ++- src/shared/utils/permission-docs.utils.ts | 6 +- 6 files changed, 89 insertions(+), 17 deletions(-) diff --git a/README.md b/README.md index 3fbd5c8..18113a3 100644 --- a/README.md +++ b/README.md @@ -99,7 +99,8 @@ For the full v5 -> v6 mapping table, see `docs/api-usage-analysis.md`. | `GET` | `/v6/projects/:projectId/permissions` | JWT / M2M | Regular human JWT: caller work-management policy map. M2M, admins, project managers, and project copilots on the project: per-member permission matrix with project permissions and template policies | Talent Manager note: -- `Talent Manager` and `Topcoder Talent Manager` callers create projects as primary `manager` members and are allowed to update project `billingAccountId`. +- `Talent Manager` and `Topcoder Talent Manager` callers create projects as primary `manager` members. +- Updating `billingAccountId` is restricted to human administrators and project members whose role on that project is `manager` (`Full Access`). ### Members diff --git a/docs/PERMISSIONS.md b/docs/PERMISSIONS.md index ef69400..ee234f3 100644 --- a/docs/PERMISSIONS.md +++ b/docs/PERMISSIONS.md @@ -32,7 +32,12 @@ Swagger auth notes: - `Talent Manager` and `Topcoder Talent Manager` satisfy `CREATE_PROJECT_AS_MANAGER`, so project creation persists them as the primary `manager` project member. - That primary `manager` membership then unlocks the standard manager-level project-owner paths, such as edit and delete checks that rely on project-member context. -- The same roles also satisfy `MANAGE_PROJECT_BILLING_ACCOUNT_ID`, which allows them to set or update a project's `billingAccountId`. + +## Billing Account Editing + +- `MANAGE_PROJECT_BILLING_ACCOUNT_ID` is intentionally narrower than general project edit access. +- A caller may set or update `billingAccountId` only when they are a human admin (`Connect Admin`, `administrator`, or `tgadmin`) or they are an active `manager` member on that specific project. +- Global manager, project-manager, task-manager, talent-manager, or M2M-only access is not enough on its own to edit a project's billing account. ## Permission Rule Shape diff --git a/src/shared/constants/permissions.constants.ts b/src/shared/constants/permissions.constants.ts index b7d149d..e335c7f 100644 --- a/src/shared/constants/permissions.constants.ts +++ b/src/shared/constants/permissions.constants.ts @@ -373,13 +373,8 @@ export const PERMISSION = { group: 'Project', description: 'Who can set or update the "billingAccountId" property.', }, - topcoderRoles: [ - USER_ROLE.MANAGER, - USER_ROLE.TOPCODER_ADMIN, - USER_ROLE.TALENT_MANAGER, - USER_ROLE.TOPCODER_TALENT_MANAGER, - ], - scopes: SCOPES_PROJECTS_WRITE_PROJECTS_BILLING_ACCOUNTS, + topcoderRoles: TOPCODER_ROLES_ADMINS, + projectRoles: [PROJECT_MEMBER_ROLE.MANAGER], }, /** diff --git a/src/shared/services/permission.service.spec.ts b/src/shared/services/permission.service.spec.ts index a9a13ed..58bad08 100644 --- a/src/shared/services/permission.service.spec.ts +++ b/src/shared/services/permission.service.spec.ts @@ -222,8 +222,45 @@ describe('PermissionService', () => { expect(allowed).toBe(true); }); - it.each([UserRole.TALENT_MANAGER, UserRole.TOPCODER_TALENT_MANAGER])( - 'allows managing project billing accounts for %s Topcoder role', + it('allows managing project billing accounts for admin role', () => { + const allowed = service.hasNamedPermission( + Permission.MANAGE_PROJECT_BILLING_ACCOUNT_ID, + { + userId: '555', + roles: [UserRole.TOPCODER_ADMIN], + isMachine: false, + }, + ); + + expect(allowed).toBe(true); + }); + + it('allows managing project billing accounts for manager project members', () => { + const allowed = service.hasNamedPermission( + Permission.MANAGE_PROJECT_BILLING_ACCOUNT_ID, + { + userId: '555', + roles: [UserRole.PROJECT_MANAGER], + isMachine: false, + }, + [ + { + userId: '555', + role: ProjectMemberRole.MANAGER, + }, + ], + ); + + expect(allowed).toBe(true); + }); + + it.each([ + UserRole.MANAGER, + UserRole.PROJECT_MANAGER, + UserRole.TALENT_MANAGER, + UserRole.TOPCODER_TALENT_MANAGER, + ])( + 'blocks managing project billing accounts for %s without full-access membership', (role) => { const allowed = service.hasNamedPermission( Permission.MANAGE_PROJECT_BILLING_ACCOUNT_ID, @@ -232,12 +269,36 @@ describe('PermissionService', () => { roles: [role], isMachine: false, }, + [ + { + userId: '555', + role: ProjectMemberRole.CUSTOMER, + }, + ], ); - expect(allowed).toBe(true); + expect(allowed).toBe(false); }, ); + it('blocks managing project billing accounts for machine admin scope without admin user role', () => { + const allowed = service.hasNamedPermission( + Permission.MANAGE_PROJECT_BILLING_ACCOUNT_ID, + { + scopes: [Scope.CONNECT_PROJECT_ADMIN], + isMachine: true, + }, + [ + { + userId: '555', + role: ProjectMemberRole.MANAGER, + }, + ], + ); + + expect(allowed).toBe(false); + }); + it.each([UserRole.TALENT_MANAGER, UserRole.TOPCODER_TALENT_MANAGER])( 'allows creating projects as manager for %s Topcoder role', (role) => { @@ -515,6 +576,11 @@ describe('PermissionService', () => { ); it('marks billing account permissions as requiring project member context', () => { + expect( + service.isNamedPermissionRequireProjectMembers( + Permission.MANAGE_PROJECT_BILLING_ACCOUNT_ID, + ), + ).toBe(true); expect( service.isNamedPermissionRequireProjectMembers( Permission.READ_AVL_PROJECT_BILLING_ACCOUNTS, diff --git a/src/shared/services/permission.service.ts b/src/shared/services/permission.service.ts index f9b904c..4eb3fc0 100644 --- a/src/shared/services/permission.service.ts +++ b/src/shared/services/permission.service.ts @@ -376,7 +376,14 @@ export class PermissionService { // Billing-account related permissions. case NamedPermission.MANAGE_PROJECT_BILLING_ACCOUNT_ID: - return isAdmin || this.hasTalentManagerRole(user); + return ( + this.hasIntersection(user.roles || [], ADMIN_ROLES) || + Boolean( + member && + this.normalizeRole(member.role) === + this.normalizeRole(ProjectMemberRole.MANAGER), + ) + ); case NamedPermission.MANAGE_PROJECT_DIRECT_PROJECT_ID: return isAdmin; diff --git a/src/shared/utils/permission-docs.utils.ts b/src/shared/utils/permission-docs.utils.ts index 9370ee8..950afaf 100644 --- a/src/shared/utils/permission-docs.utils.ts +++ b/src/shared/utils/permission-docs.utils.ts @@ -78,10 +78,7 @@ const PROJECT_BILLING_TOPCODER_ROLES = [ ...TALENT_MANAGER_ROLES, ]; -const PROJECT_BILLING_MANAGEMENT_USER_ROLES = [ - ...ADMIN_AND_MANAGER_ROLES, - ...TALENT_MANAGER_ROLES, -]; +const PROJECT_BILLING_MANAGEMENT_USER_ROLES = [...STRICT_ADMIN_ACCESS_ROLES]; const PROJECT_CREATOR_MANAGER_USER_ROLES = [ ...STRICT_ADMIN_ACCESS_ROLES, @@ -371,6 +368,7 @@ function getNamedPermissionDocumentation( case NamedPermission.MANAGE_PROJECT_BILLING_ACCOUNT_ID: return createSummary({ userRoles: PROJECT_BILLING_MANAGEMENT_USER_ROLES, + projectRoles: [ProjectMemberRole.MANAGER], }); case NamedPermission.MANAGE_PROJECT_DIRECT_PROJECT_ID: From 1c01ce72adad467f3e5d61ed70be9d2a6aa3c031 Mon Sep 17 00:00:00 2001 From: Justin Gasper Date: Wed, 18 Mar 2026 16:12:45 +1100 Subject: [PATCH 8/8] Add a test to ensure project managers can create projects --- src/shared/services/permission.service.spec.ts | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/shared/services/permission.service.spec.ts b/src/shared/services/permission.service.spec.ts index 58bad08..ba7a35f 100644 --- a/src/shared/services/permission.service.spec.ts +++ b/src/shared/services/permission.service.spec.ts @@ -346,6 +346,16 @@ describe('PermissionService', () => { expect(allowed).toBe(true); }); + it('allows creating projects for Project Manager role', () => { + const allowed = service.hasNamedPermission(Permission.CREATE_PROJECT, { + userId: '555', + roles: [UserRole.PROJECT_MANAGER], + isMachine: false, + }); + + expect(allowed).toBe(true); + }); + it('allows reading project members for machine token with project-member read scope', () => { const allowed = service.hasNamedPermission(Permission.READ_PROJECT_MEMBER, { scopes: [Scope.PROJECT_MEMBERS_READ],