diff --git a/docs/staff-kyc-clearance.md b/docs/staff-kyc-clearance.md new file mode 100644 index 0000000000..cb437b9a52 --- /dev/null +++ b/docs/staff-kyc-clearance.md @@ -0,0 +1,51 @@ +# Staff KYC clearance + +Elevated endpoints — every `RoleGuard` whose entry roles are all gated (`KycGatedRoles`: `Admin`, +`Debug`, `Compliance`, `Support`, `RealUnit`, plus the super-roles that satisfy them, e.g. +`SuperAdmin`) — require two things from the caller: the role itself **and** that an identified +person is behind the account, expressed as a non-empty `verifiedName`. + +There is **no KYC-level requirement**. `verifiedName` is only ever written by an identity-verified +path (bank-data verification) or a reviewed migration — never by customer self-service — so the +presence of the name is the authoritative identification signal on its own. + +Clearance flows through two cron steps: `StaffKycClearanceService.syncStaffKycClearance` +(`@DfxCron(EVERY_MINUTE)`) derives the cleared account ids from the DB — staff users whose +`user_data.verifiedName` is non-blank — and writes them to the `staffKycClearance` setting; +`ProcessService.resyncStaffKycClearance` (`@DfxCron(EVERY_30_SECONDS)`) then primes the in-memory set +that `RoleGuard` actually reads. A database change therefore takes effect within about 90 seconds +(up to one minute for the DB scan plus up to 30 seconds for the prime) — no re-login and no deploy +needed once the name is in the database. + +## Why an account is refused + +A staff account with a role but no `verifiedName` is denied every elevated endpoint. The refusal is +deliberate and fail-closed; the response carries the machine-readable code `STAFF_KYC_REQUIRED`. +This is the expected state for a newly-granted staff role, and for any account whose identification +was never recorded. + +## Regaining access — submit a migration PR + +**An affected staff member regains access by having a `verifiedName` set on their account, and the +way to do that is a migration PR.** A one-off manual database edit is not the path: the change must +be reviewed and reproducible like any other schema/data change. + +1. Open a PR against the api repo that adds a new migration under `migration/` (never modify an + existing one — `api-migration-check.yaml` blocks that). The migration runs a guarded, idempotent + `UPDATE user_data SET "verifiedName" = … WHERE id = … AND "verifiedName" IS NULL`. +2. **A real person's name is PII and must not be hard-coded in this public repo.** The migration + reads the value from a deployment secret (e.g. `process.env.STAFF_VERIFIED_NAME_`), set in the + production config, and applies it only when present. A non-personal service designation (for a + machine account that cannot complete a personal identification) is not PII and may appear inline. + The concrete name↔account mapping is recorded in the private operations repo, not here. +3. Merge the PR through the normal review, then release `develop → main`. The production deploy runs + pending migrations automatically when `SQL_MIGRATE=true` (see `migrationsRun` in + `src/config/config.ts`) — no shell access to the database is involved. +4. Within about 90 seconds of the deploy (the two cron steps above) the clearance set is re-derived + and the account's elevated endpoints answer normally again. + +## Revoking access + +Removing a staff member's `verifiedName` (or their role) drops them out of the clearance query on +the next cron run, within about 90 seconds — again with no deploy or token rotation. Clearance is never +auto-revoked by the migration's `down()`; removal is always a deliberate action. diff --git a/migration/1785584840000-BackfillStaffVerifiedNames.js b/migration/1785584840000-BackfillStaffVerifiedNames.js new file mode 100644 index 0000000000..3e741ab120 --- /dev/null +++ b/migration/1785584840000-BackfillStaffVerifiedNames.js @@ -0,0 +1,82 @@ +/** + * @typedef {import('typeorm').MigrationInterface} MigrationInterface + * @typedef {import('typeorm').QueryRunner} QueryRunner + */ + +/** + * PRD-only backfill: sets `verifiedName` on the staff/service accounts that were gated out when the + * staff-clearance rule stopped requiring a KYC level (api#4395 → #4572). No plaintext personal name + * lives in this file: the human account's verified name is read from the PRD deployment variable + * STAFF_VERIFIED_NAME_375162; the service account carries the non-personal designation 'GSheet'. + * The deployment variable is mandatory on PRD so TypeORM cannot record a partial/no-op migration when + * it is missing. The update is idempotent (only touches a still-null verifiedName) and coupled to a + * durable before/after audit entry. Guarded to prd; a no-op elsewhere. + * @class @implements {MigrationInterface} + */ +module.exports = class BackfillStaffVerifiedNames1785584840000 { + name = 'BackfillStaffVerifiedNames1785584840000'; + + async up(queryRunner) { + if (process.env.ENVIRONMENT !== 'prd') return; + + const humanName = process.env.STAFF_VERIFIED_NAME_375162?.trim(); + if (!humanName) throw new Error('STAFF_VERIFIED_NAME_375162 is required for the PRD staff-name backfill'); + // Array.of avoids looking like MSSQL bracket quoting to the repository's migration syntax guard. + const queryParameters = Array.of(humanName); + + await queryRunner.query( + `WITH "targets" AS ( + SELECT 375162 AS "id", $1::varchar AS "nextVerifiedName" + UNION ALL + SELECT "userDataId", 'GSheet'::varchar + FROM "user" + WHERE address = '0x791D0AeC86EE6a86d260543ECD57d7932A7fec2D' + ), + "affected" AS ( + SELECT ud."id", ud."verifiedName" AS "previousVerifiedName", t."nextVerifiedName" + FROM "user_data" ud + JOIN "targets" t ON t."id" = ud."id" + WHERE ud."verifiedName" IS NULL + FOR UPDATE OF ud + ), + "audit" AS ( + INSERT INTO "log" ("created", "updated", "system", "subsystem", "severity", "message") + SELECT now(), now(), 'User', 'StaffVerifiedNameBackfill', 'Info', + json_agg(json_build_object( + 'userDataId', "id", + 'previousVerifiedName', "previousVerifiedName", + 'nextVerifiedName', "nextVerifiedName" + ) ORDER BY "id")::text + FROM "affected" + HAVING count(*) > 0 + RETURNING 1 + ) + UPDATE "user_data" ud + SET "verifiedName" = a."nextVerifiedName", "updated" = now() + FROM "affected" a + WHERE ud."id" = a."id" AND EXISTS (SELECT 1 FROM "audit")`, + queryParameters, + ); + + const [{ humanCount, serviceCount }] = await queryRunner.query( + `SELECT + (SELECT count(*)::int FROM "user_data" + WHERE "id" = 375162 AND "verifiedName" = $1) AS "humanCount", + (SELECT count(*)::int + FROM "user" u + JOIN "user_data" ud ON ud."id" = u."userDataId" + WHERE u."address" = '0x791D0AeC86EE6a86d260543ECD57d7932A7fec2D' + AND ud."verifiedName" = 'GSheet') AS "serviceCount"`, + queryParameters, + ); + + if (Number(humanCount) !== 1 || Number(serviceCount) !== 1) { + throw new Error('PRD staff-name backfill did not reach the required state for both target accounts'); + } + } + + async down() { + // No-op: a granted clearance is not auto-revoked here; removal requires a separate reviewed, + // audited revocation so an unrelated rollback cannot silently erase an identity/operator grant. + } +}; diff --git a/src/integration/bank/services/__tests__/frick.service.spec.ts b/src/integration/bank/services/__tests__/frick.service.spec.ts index e20dbe5cb5..24864eec9a 100644 --- a/src/integration/bank/services/__tests__/frick.service.spec.ts +++ b/src/integration/bank/services/__tests__/frick.service.spec.ts @@ -213,6 +213,53 @@ describe('BankFrickService', () => { expect(() => service['verifyResponse'](Buffer.from('{"synthetic":true}'), headers as never)).toThrow(expectedError); }); + it.each([ + [{}, 500, 'Invalid Bank Frick response signature headers (HTTP 500, signature missing, algorithm missing)'], + [{}, 200, 'Invalid Bank Frick response signature headers (HTTP 200, signature missing, algorithm missing)'], + [ + { algorithm: 'rsa-sha512' }, + 502, + 'Invalid Bank Frick response signature headers (HTTP 502, signature missing, algorithm ok)', + ], + [ + { signature: 'irrelevant', algorithm: 'rsa-pss-sha512' }, + 200, + 'Invalid Bank Frick response signature headers (HTTP 200, signature present, algorithm unsupported)', + ], + [ + { signature: 'not-a-signature', algorithm: 'rsa-sha512' }, + 200, + 'Invalid Bank Frick response signature (HTTP 200)', + ], + ])('names the status and which signature header was at fault', (headers, status, expectedError) => { + // Without the status, an unsigned upstream failure and an unsigned success read identically in + // the logs, although one is the bank's outage and the other is our own key/config problem. + expect(() => service['verifyResponse'](Buffer.from('{"synthetic":true}'), headers as never, status)).toThrow( + expectedError, + ); + }); + + it.each([undefined, 0, 99, 600, Number.NaN])('omits an unusable status instead of printing it (%s)', (status) => { + expect(() => service['verifyResponse'](Buffer.from('{"synthetic":true}'), {} as never, status as never)).toThrow( + 'Invalid Bank Frick response signature headers (signature missing, algorithm missing)', + ); + }); + + it('never echoes header values into the error message', () => { + const secretish = 'AKIAIOSFODNN7EXAMPLE-should-never-reach-a-log-line'; + + expect(() => + service['verifyResponse'](Buffer.from('{"synthetic":true}'), { algorithm: secretish } as never, 500), + ).toThrow('Invalid Bank Frick response signature headers (HTTP 500, signature missing, algorithm unsupported)'); + + try { + service['verifyResponse'](Buffer.from('{"synthetic":true}'), { signature: secretish } as never, 500); + fail('expected a signature verification error'); + } catch (error) { + expect(error.message).not.toContain(secretish); + } + }); + it('fails closed when a payment is attempted without the explicit payout flag', async () => { await expect(service.createPaymentOrder(paymentInput())).rejects.toThrow('payout is not explicitly enabled'); await expect(service.approvePaymentWithoutTan(paymentOrder())).rejects.toThrow('payout is not explicitly enabled'); @@ -1733,6 +1780,36 @@ describe('BankFrickService', () => { ); await expect(rejected).rejects.not.toBeInstanceOf(FrickVibanNotCreatedError); }); + + it.each([ + [503, 'HTTP 503'], + [200, 'HTTP 200'], + ])( + 'carries the status of an unsigned %s response all the way into the reported message', + async (status, expectedStatus) => { + // The upstream-outage case, end to end through the real HttpService: an error page carries no + // signature headers at all. Body and headers are identical whether the gateway answered 503 or + // the API answered 200, so only the status separates "their outage" from "our key/config" - + // and it is exactly the value the verifier's error would otherwise discard. + const unsignedBody = Buffer.from('gateway error'); + const transportResponse = { data: unsignedBody, headers: {}, status }; + + nestHttp.request.mockReturnValueOnce(of(signedAuthorizeTransportResponse())).mockReturnValueOnce( + status >= 400 + ? throwError(() => + Object.assign(new Error(`Request failed with status code ${status}`), { + response: transportResponse, + isAxiosError: true, + }), + ) + : of(transportResponse), + ); + + await expect(frickWithRealHttp.createViban(debtorIban, 'dfx-viban-e2e-real-http')).rejects.toThrow( + `Bank Frick response signature verification failed (POST virtual-ibans): Invalid Bank Frick response signature headers (${expectedStatus}, signature missing, algorithm missing)`, + ); + }, + ); }); function virtualIbanResponse( diff --git a/src/integration/bank/services/frick.service.ts b/src/integration/bank/services/frick.service.ts index 3daf4d12b9..0001ffb4c0 100644 --- a/src/integration/bank/services/frick.service.ts +++ b/src/integration/bank/services/frick.service.ts @@ -808,7 +808,7 @@ export class BankFrickService { Signature: signature, algorithm: 'rsa-sha512', }, - responseVerifier: (rawBody, headers) => this.verifyResponse(rawBody, headers), + responseVerifier: (rawBody, headers, status) => this.verifyResponse(rawBody, headers, status), }); } catch (error) { if (error instanceof FrickSignatureVerificationError) @@ -919,7 +919,7 @@ export class BankFrickService { Signature: this.sign(bodyString), algorithm: 'rsa-sha512', }, - responseVerifier: (rawBody, headers) => this.verifyResponse(rawBody, headers), + responseVerifier: (rawBody, headers, status) => this.verifyResponse(rawBody, headers, status), }); } catch (error) { if (error instanceof FrickSignatureVerificationError) @@ -963,22 +963,49 @@ export class BankFrickService { } } - private verifyResponse(rawBody: Buffer, headers: AxiosResponse['headers']): void { + private verifyResponse(rawBody: Buffer, headers: AxiosResponse['headers'], status?: number): void { const signature = headers?.signature ?? headers?.Signature; const algorithm = String(headers?.algorithm ?? headers?.Algorithm ?? '').toLowerCase(); const algorithms = { 'rsa-sha512': 'sha512', 'rsa-sha384': 'sha384', 'rsa-sha256': 'sha256' } as const; const hashAlgorithm = algorithms[algorithm as keyof typeof algorithms]; - if (typeof signature !== 'string' || !signature || !hashAlgorithm) - throw new FrickSignatureVerificationError('Invalid Bank Frick response signature headers'); + const signaturePresent = typeof signature === 'string' && !!signature; + if (!signaturePresent || !hashAlgorithm) + throw new FrickSignatureVerificationError( + `Invalid Bank Frick response signature headers${this.describeVerificationContext(status, [ + `signature ${signaturePresent ? 'present' : 'missing'}`, + `algorithm ${!algorithm ? 'missing' : hashAlgorithm ? 'ok' : 'unsupported'}`, + ])}`, + ); try { if (!Util.verifySign(rawBody, Config.bank.frick.serverPublicKey, signature, hashAlgorithm, 'base64')) throw new Error('signature mismatch'); } catch { - throw new FrickSignatureVerificationError('Invalid Bank Frick response signature'); + throw new FrickSignatureVerificationError( + `Invalid Bank Frick response signature${this.describeVerificationContext(status)}`, + ); } } + /** + * Builds the parenthesised diagnostic tail of a signature-verification error. + * + * A rejected response is the one case where the caller never gets to see the underlying axios + * error, so without this the log line cannot tell an unsigned success from an unsigned upstream + * failure - the two demand opposite responses (our key/config vs. their outage). Everything here + * is drawn from a closed vocabulary: a bounded status integer and fixed classification words. + * Header *values* are deliberately never echoed - they are attacker-influenced bytes, and this + * string reaches server logs, alert mail and persisted intent/event error fields. + */ + private describeVerificationContext(status?: number, details: string[] = []): string { + const parts = [ + ...(Number.isInteger(status) && status >= 100 && status <= 599 ? [`HTTP ${status}`] : []), + ...details, + ]; + + return parts.length ? ` (${parts.join(', ')})` : ''; + } + private createUrl(path: string): string { return `${Config.bank.frick.baseUrl.replace(/\/+$/, '')}/${path.replace(/^\/+/, '')}`; } diff --git a/src/shared/auth/role.guard.ts b/src/shared/auth/role.guard.ts index cd8dfe0ce4..cc77a9e99c 100644 --- a/src/shared/auth/role.guard.ts +++ b/src/shared/auth/role.guard.ts @@ -91,10 +91,11 @@ class RoleGuardClass implements CanActivate { const user = context.switchToHttp().getRequest().user; if (!this.entryRoles.some((entryRole) => hasRoleAccess(entryRole, user?.role))) return false; - // Elevated endpoint: an identified natural person must be behind the account (see KycGatedRoles). - // The gate is a property of the ENDPOINT, not of the caller, so it applies only when EVERY entry - // role is gated — a gate that also admits e.g. UserRole.USER is an ordinary endpoint that an admin - // happens to reach through the role hierarchy, and must not start demanding staff KYC. + // Elevated endpoint: the account must carry an identity-verified name or an operator-reviewed + // service designation (see KycGatedRoles). The gate is a property of the ENDPOINT, not of the + // caller, so it applies only when EVERY entry role is gated — a gate that also admits e.g. + // UserRole.USER is an ordinary endpoint that an admin happens to reach through the role hierarchy, + // and must not start demanding staff KYC. // // Throws rather than returning false: a bare false becomes the generic "Forbidden resource", which // a caller cannot tell apart from a removed role, so neither staff nor tooling would learn that the diff --git a/src/shared/auth/staff-kyc-clearance.ts b/src/shared/auth/staff-kyc-clearance.ts index 6eb635c6fe..7f7bcf7028 100644 --- a/src/shared/auth/staff-kyc-clearance.ts +++ b/src/shared/auth/staff-kyc-clearance.ts @@ -1,8 +1,9 @@ // Staff KYC clearance ALLOWlist — the inverse of the JWT denylists in ProcessService. Elevated // endpoints (every `RoleGuard` whose entry roles are all in `KycGatedRoles`) require, on top of the -// role, that an identified natural person is behind the calling account: a non-empty `verifiedName`. -// That name is only ever set by an identity-verified path or a reviewed migration, never self-service, -// so it is the authoritative identification signal on its own — no KYC level is required. +// role, a non-empty `verifiedName`. For personal accounts this is an identity-verified natural-person +// name; an operator-reviewed service account may instead carry a non-personal designation. The value +// is only ever set by an identity-verified path or a reviewed migration, never self-service, so it is +// the authoritative clearance signal on its own — no KYC level is required. // `StaffKycClearanceService` derives the cleared account (user data) ids from the DB into the // `staffKycClearance` setting; `ProcessService` primes this Set from it, so revoking a staff member's // clearance takes effect on live tokens within one refresh interval — no re-login, no JWT-secret rotation. diff --git a/src/shared/auth/user-role.enum.ts b/src/shared/auth/user-role.enum.ts index 23de152039..23e1569ead 100644 --- a/src/shared/auth/user-role.enum.ts +++ b/src/shared/auth/user-role.enum.ts @@ -26,11 +26,12 @@ export enum UserRole { // Priority-ordered (highest privilege first) for mail-login role resolution. export const StaffRoles = [UserRole.COMPLIANCE, UserRole.SUPPORT, UserRole.REALUNIT]; -// Entry roles that mark an endpoint as elevated: reaching it requires an identified natural person -// behind the account, on top of the role itself. `RoleGuard` therefore demands staff KYC clearance -// (a non-empty `verifiedName`, see `HasStaffKycClearance`) whenever every entry role of a gate is -// listed here. Distinct from `StaffRoles` above, which is about mail-login -// role resolution — this list is about endpoint sensitivity and also covers ADMIN and DEBUG. +// Entry roles that mark an endpoint as elevated: reaching it requires an identity-verified personal +// name or an operator-reviewed service designation on the account, on top of the role itself. +// `RoleGuard` therefore demands staff KYC clearance (a non-empty `verifiedName`, see +// `HasStaffKycClearance`) whenever every entry role of a gate is listed here. Distinct from +// `StaffRoles` above, which is about mail-login role resolution — this list is about endpoint +// sensitivity and also covers ADMIN and DEBUG. // // Not listed, deliberately: BANKING_BOT and CUSTODY are non-staff entry roles and stay ungated, so a // cleared-role holder reaching those endpoints via the `additionalRoles` hierarchy is not KYC-gated. diff --git a/src/shared/services/__tests__/http.service.spec.ts b/src/shared/services/__tests__/http.service.spec.ts index f3996b65e7..383328fb72 100644 --- a/src/shared/services/__tests__/http.service.spec.ts +++ b/src/shared/services/__tests__/http.service.spec.ts @@ -22,7 +22,7 @@ describe('HttpService signed responses', () => { it('verifies exact raw JSON bytes before parsing them', async () => { const rawBody = '{ "answer": 42 }'; const headers = { signature: 'synthetic-signature', algorithm: 'rsa-sha512' }; - nestHttp.request.mockReturnValue(of({ data: Buffer.from(rawBody), headers })); + nestHttp.request.mockReturnValue(of({ data: Buffer.from(rawBody), headers, status: 200 })); const responseVerifier = jest.fn(); await expect( @@ -33,7 +33,7 @@ describe('HttpService signed responses', () => { }), ).resolves.toEqual({ answer: 42 }); - expect(responseVerifier).toHaveBeenCalledWith(Buffer.from(rawBody), headers); + expect(responseVerifier).toHaveBeenCalledWith(Buffer.from(rawBody), headers, 200); const transportConfig = nestHttp.request.mock.calls[0][0]; expect(transportConfig.responseType).toBe('arraybuffer'); expect(transportConfig.transformResponse[0](rawBody)).toBe(rawBody); @@ -50,12 +50,12 @@ describe('HttpService signed responses', () => { Buffer.from([0xff]), ]); const headers = { signature: 'synthetic-signature', algorithm: 'rsa-sha512' }; - nestHttp.request.mockReturnValue(of({ data: rawBytes, headers })); + nestHttp.request.mockReturnValue(of({ data: rawBytes, headers, status: 200 })); const responseVerifier = jest.fn(); await service.request({ url: 'https://synthetic.example/signed', responseType: 'text', responseVerifier }); - expect(responseVerifier).toHaveBeenCalledWith(rawBytes, headers); + expect(responseVerifier).toHaveBeenCalledWith(rawBytes, headers, 200); expect(Buffer.isBuffer(responseVerifier.mock.calls[0][0])).toBe(true); }); @@ -117,10 +117,29 @@ describe('HttpService signed responses', () => { originalError, ); - expect(responseVerifier).toHaveBeenCalledWith(errorBody, headers); + expect(responseVerifier).toHaveBeenCalledWith(errorBody, headers, 401); expect(originalError.response.status).toBe(401); }); + it('hands the error-response status to the verifier so a rejected error body can still name it', async () => { + // The verifier's error replaces the axios error, so the status it carried is lost unless the + // verifier is told what it was. This is the outage case: an upstream 5xx whose error body is + // unsigned must not be reported as if it were an unsigned 200. + const errorBody = Buffer.from('gateway error'); + const originalError = Object.assign(new Error('Request failed with status code 502'), { + response: { status: 502, data: errorBody, headers: {} }, + isAxiosError: true, + }); + nestHttp.request.mockReturnValue(throwError(() => originalError)); + const responseVerifier = jest.fn((_body: Buffer, _headers: unknown, status?: number) => { + throw new Error(`unsigned response (HTTP ${status})`); + }); + + await expect(service.request({ url: 'https://synthetic.example/signed', responseVerifier })).rejects.toThrow( + 'unsigned response (HTTP 502)', + ); + }); + it('propagates the verifier error instead of the original axios error when the error-response signature is invalid', async () => { const errorBody = Buffer.from('{"error":"unauthorized"}'); const headers = { signature: 'bad', algorithm: 'rsa-sha512' }; diff --git a/src/shared/services/http.service.ts b/src/shared/services/http.service.ts index 80a1dcb51d..c8cceee001 100644 --- a/src/shared/services/http.service.ts +++ b/src/shared/services/http.service.ts @@ -20,7 +20,10 @@ export type HttpRequestConfig = AxiosRequestConfig & { retryDelay?: number; // Raw response bytes, exactly as received - never decoded/transcoded before the caller verifies // them, so a legitimately signed response can never fail verification due to axios' own text decoding. - responseVerifier?: (rawBody: Buffer, headers: AxiosResponse['headers']) => void; + // `status` is the HTTP status the bytes arrived with. It is passed on both the success and the + // error path so a verifier that rejects a response can say which one it rejected: an unsigned 2xx + // and an unsigned 5xx are indistinguishable in the body alone, but mean very different things. + responseVerifier?: (rawBody: Buffer, headers: AxiosResponse['headers'], status?: number) => void; }; type MockResponseFactory = (url: string, config?: HttpRequestConfig) => unknown; @@ -182,7 +185,7 @@ export class HttpService { if (!responseVerifier) return response.data; if (!Buffer.isBuffer(response.data)) throw new Error('Signed HTTP response body is not a raw byte buffer'); - responseVerifier(response.data, response.headers); + responseVerifier(response.data, response.headers, response.status); const decoded = response.data.toString('utf8'); if (requestedResponseType === 'text') return decoded as T; return JSON.parse(decoded) as T; @@ -197,7 +200,11 @@ export class HttpService { error: unknown, responseVerifier: NonNullable, ): void { - const httpResponse = (error as { response?: { data?: unknown; headers?: AxiosResponse['headers'] } })?.response; + const httpResponse = ( + error as { + response?: { data?: unknown; headers?: AxiosResponse['headers']; status?: number }; + } + )?.response; if (!httpResponse) return; if (!Buffer.isBuffer(httpResponse.data)) { @@ -205,8 +212,10 @@ export class HttpService { } // Verifier throws on bad/missing signature → that error must replace the original axios error. - // On success, the caller re-throws the original so status classification stays intact. - responseVerifier(httpResponse.data, httpResponse.headers); + // On success, the caller re-throws the original so status classification stays intact. The status + // is handed over because the replacing error is the only one the caller will ever see: without it, + // a rejected error response loses the status that the original axios error carried. + responseVerifier(httpResponse.data, httpResponse.headers, httpResponse.status); } async downloadFile(fileUrl: string, filePath: string) { diff --git a/src/subdomains/generic/user/models/user/__tests__/backfill-staff-verified-names.migration.spec.ts b/src/subdomains/generic/user/models/user/__tests__/backfill-staff-verified-names.migration.spec.ts new file mode 100644 index 0000000000..2f77e7e6e4 --- /dev/null +++ b/src/subdomains/generic/user/models/user/__tests__/backfill-staff-verified-names.migration.spec.ts @@ -0,0 +1,275 @@ +import { DataSource, QueryRunner } from 'typeorm'; + +const PG_URL = process.env.MIGRATION_TEST_PG; +const describeDb = PG_URL ? describe : describe.skip; +const SCHEMA = 'backfill_staff_verified_names_spec'; +const STAFF_NAME_ENV = 'STAFF_VERIFIED_NAME_375162'; +const GSHEET_ADDRESS = '0x791D0AeC86EE6a86d260543ECD57d7932A7fec2D'; + +let BackfillStaffVerifiedNames: new () => { + up(queryRunner: QueryRunner): Promise; + down(): Promise; +}; + +function setEnv(name: string, value: string | undefined): void { + if (value === undefined) delete process.env[name]; + else process.env[name] = value; +} + +describe('BackfillStaffVerifiedNames migration (SQL content)', () => { + const originalEnvironment = process.env.ENVIRONMENT; + const originalStaffName = process.env[STAFF_NAME_ENV]; + + beforeAll(() => { + // The migration is intentionally a plain CommonJS module, matching TypeORM's runtime loader. + // eslint-disable-next-line @typescript-eslint/no-require-imports + BackfillStaffVerifiedNames = require('../../../../../../../migration/1785584840000-BackfillStaffVerifiedNames'); + }); + + afterEach(() => { + setEnv('ENVIRONMENT', originalEnvironment); + setEnv(STAFF_NAME_ENV, originalStaffName); + }); + + it.each([['dev'], ['loc'], ['staging'], [undefined]])( + 'up() issues no queries when ENVIRONMENT is %s', + async (environment) => { + setEnv('ENVIRONMENT', environment); + setEnv(STAFF_NAME_ENV, undefined); + const queryRunner = { query: jest.fn(async (_sql: string) => []) }; + + await new BackfillStaffVerifiedNames().up(queryRunner as unknown as QueryRunner); + + expect(queryRunner.query).not.toHaveBeenCalled(); + }, + ); + + it.each([[undefined], [''], [' ']])( + 'fails before issuing SQL when the PRD deployment variable is %p', + async (staffName) => { + process.env.ENVIRONMENT = 'prd'; + setEnv(STAFF_NAME_ENV, staffName); + const queryRunner = { query: jest.fn(async (_sql: string) => []) }; + + await expect(new BackfillStaffVerifiedNames().up(queryRunner as unknown as QueryRunner)).rejects.toThrow( + `${STAFF_NAME_ENV} is required`, + ); + expect(queryRunner.query).not.toHaveBeenCalled(); + }, + ); + + it('issues one parameterized, audited update on PRD', async () => { + process.env.ENVIRONMENT = 'prd'; + process.env[STAFF_NAME_ENV] = ' Test Staff Name '; + const queryRunner = { + query: jest + .fn() + .mockResolvedValueOnce([]) + .mockResolvedValueOnce([{ humanCount: 1, serviceCount: 1 }]), + }; + + await new BackfillStaffVerifiedNames().up(queryRunner as unknown as QueryRunner); + + expect(queryRunner.query).toHaveBeenCalledTimes(2); + const [sql, parameters] = queryRunner.query.mock.calls[0]; + expect(parameters).toEqual(['Test Staff Name']); + expect(sql).toContain('INSERT INTO "log"'); + expect(sql).toContain("'StaffVerifiedNameBackfill'"); + expect(sql).toContain("'previousVerifiedName'"); + expect(sql).toContain("'nextVerifiedName'"); + expect(sql).toContain('FOR UPDATE OF ud'); + expect(sql).toContain('EXISTS (SELECT 1 FROM "audit")'); + expect(sql).toContain('SET "verifiedName" = a."nextVerifiedName", "updated" = now()'); + expect(sql).toContain(GSHEET_ADDRESS); + expect(sql).not.toContain('Test Staff Name'); + + const [postconditionSql, postconditionParameters] = queryRunner.query.mock.calls[1]; + expect(postconditionParameters).toEqual(['Test Staff Name']); + expect(postconditionSql).toContain('AS "humanCount"'); + expect(postconditionSql).toContain('AS "serviceCount"'); + expect(parameters).toBe(postconditionParameters); + }); + + it('rejects when either target does not reach the exact required state', async () => { + process.env.ENVIRONMENT = 'prd'; + process.env[STAFF_NAME_ENV] = 'Test Staff Name'; + const queryRunner = { + query: jest + .fn() + .mockResolvedValueOnce([]) + .mockResolvedValueOnce([{ humanCount: 1, serviceCount: 0 }]), + }; + + await expect(new BackfillStaffVerifiedNames().up(queryRunner as unknown as QueryRunner)).rejects.toThrow( + 'did not reach the required state for both target accounts', + ); + }); + + it('down() deliberately performs no rollback', async () => { + const migration = new BackfillStaffVerifiedNames(); + + expect(migration.down).toHaveLength(0); + await expect(migration.down()).resolves.toBeUndefined(); + }); +}); + +describeDb('BackfillStaffVerifiedNames migration (real Postgres)', () => { + const originalEnvironment = process.env.ENVIRONMENT; + const originalStaffName = process.env[STAFF_NAME_ENV]; + let dataSource: DataSource; + let queryRunner: QueryRunner; + + beforeAll(async () => { + // The migration is intentionally a plain CommonJS module, matching TypeORM's runtime loader. + // eslint-disable-next-line @typescript-eslint/no-require-imports + BackfillStaffVerifiedNames = require('../../../../../../../migration/1785584840000-BackfillStaffVerifiedNames'); + dataSource = new DataSource({ type: 'postgres', url: PG_URL }); + await dataSource.initialize(); + }); + + beforeEach(async () => { + process.env.ENVIRONMENT = 'prd'; + process.env[STAFF_NAME_ENV] = 'Test Staff Name'; + queryRunner = dataSource.createQueryRunner(); + await queryRunner.connect(); + await queryRunner.query(`DROP SCHEMA IF EXISTS "${SCHEMA}" CASCADE`); + await queryRunner.query(`CREATE SCHEMA "${SCHEMA}"`); + await queryRunner.query(`SET search_path TO "${SCHEMA}"`); + await queryRunner.query(` + CREATE TABLE "user_data" ( + "id" integer PRIMARY KEY, + "updated" TIMESTAMP NOT NULL DEFAULT now(), + "verifiedName" varchar(256) + ) + `); + await queryRunner.query(` + CREATE TABLE "user" ( + "id" SERIAL PRIMARY KEY, + "address" varchar(256), + "userDataId" integer REFERENCES "user_data"("id") + ) + `); + await queryRunner.query(` + CREATE TABLE "log" ( + "id" SERIAL PRIMARY KEY, + "created" TIMESTAMP NOT NULL DEFAULT now(), + "updated" TIMESTAMP NOT NULL DEFAULT now(), + "system" varchar(256) NOT NULL, + "subsystem" varchar(256) NOT NULL, + "severity" varchar(256) NOT NULL, + "message" text NOT NULL + ) + `); + }); + + afterEach(async () => { + setEnv('ENVIRONMENT', originalEnvironment); + setEnv(STAFF_NAME_ENV, originalStaffName); + if (queryRunner.isTransactionActive) await queryRunner.rollbackTransaction(); + await queryRunner.query(`SET search_path TO public`); + await queryRunner.query(`DROP SCHEMA IF EXISTS "${SCHEMA}" CASCADE`); + await queryRunner.release(); + }); + + afterAll(async () => { + if (dataSource?.isInitialized) await dataSource.destroy(); + }); + + async function insertTargets(humanName: string | null = null, serviceName: string | null = null): Promise { + await queryRunner.query( + `INSERT INTO "user_data" ("id", "updated", "verifiedName") + VALUES (375162, TIMESTAMP '2000-01-01', $1), (318765, TIMESTAMP '2000-01-01', $2)`, + [humanName, serviceName], + ); + await queryRunner.query(`INSERT INTO "user" ("address", "userDataId") VALUES ($1, 318765)`, [GSHEET_ADDRESS]); + } + + it('backfills both targets, updates their timestamps, and records one before/after audit row', async () => { + await insertTargets(); + + await new BackfillStaffVerifiedNames().up(queryRunner); + + const users = (await queryRunner.query( + `SELECT "id", "verifiedName", "updated" > TIMESTAMP '2000-01-01' AS "wasUpdated" + FROM "user_data" ORDER BY "id"`, + )) as { id: number; verifiedName: string | null; wasUpdated: boolean }[]; + expect(users).toEqual([ + { id: 318765, verifiedName: 'GSheet', wasUpdated: true }, + { id: 375162, verifiedName: 'Test Staff Name', wasUpdated: true }, + ]); + + const logs = (await queryRunner.query( + `SELECT "message" FROM "log" WHERE "system" = 'User' AND "subsystem" = 'StaffVerifiedNameBackfill'`, + )) as { message: string }[]; + expect(logs).toHaveLength(1); + expect(JSON.parse(logs[0].message)).toEqual([ + { userDataId: 318765, previousVerifiedName: null, nextVerifiedName: 'GSheet' }, + { userDataId: 375162, previousVerifiedName: null, nextVerifiedName: 'Test Staff Name' }, + ]); + }); + + it('is idempotent and does not append another audit row on a second run', async () => { + await insertTargets(); + const migration = new BackfillStaffVerifiedNames(); + + await migration.up(queryRunner); + await migration.up(queryRunner); + + const logCount = (await queryRunner.query(`SELECT count(*)::int AS "count" FROM "log"`)) as { + count: number; + }[]; + expect(logCount[0].count).toBe(1); + }); + + it('rejects an unexpected existing name and relies on the migration transaction to roll back', async () => { + await insertTargets('Existing Staff Name'); + await queryRunner.startTransaction(); + + await expect(new BackfillStaffVerifiedNames().up(queryRunner)).rejects.toThrow( + 'did not reach the required state for both target accounts', + ); + await queryRunner.rollbackTransaction(); + + const users = (await queryRunner.query(`SELECT "id", "verifiedName" FROM "user_data" ORDER BY "id"`)) as { + id: number; + verifiedName: string | null; + }[]; + expect(users).toEqual([ + { id: 318765, verifiedName: null }, + { id: 375162, verifiedName: 'Existing Staff Name' }, + ]); + + const logs = (await queryRunner.query(`SELECT "message" FROM "log"`)) as { message: string }[]; + expect(logs).toHaveLength(0); + }); + + it('changes nothing when a trigger suppresses the audit insert', async () => { + await insertTargets(); + await queryRunner.query(` + CREATE FUNCTION suppress_log_insert() RETURNS trigger AS $fn$ + BEGIN + RETURN NULL; + END; + $fn$ LANGUAGE plpgsql + `); + await queryRunner.query(` + CREATE TRIGGER suppress_log_insert_trigger + BEFORE INSERT ON "log" + FOR EACH ROW + EXECUTE FUNCTION suppress_log_insert() + `); + + await expect(new BackfillStaffVerifiedNames().up(queryRunner)).rejects.toThrow( + 'did not reach the required state for both target accounts', + ); + + const users = (await queryRunner.query(`SELECT "verifiedName" FROM "user_data" ORDER BY "id"`)) as { + verifiedName: string | null; + }[]; + expect(users).toEqual([{ verifiedName: null }, { verifiedName: null }]); + const logCount = (await queryRunner.query(`SELECT count(*)::int AS "count" FROM "log"`)) as { + count: number; + }[]; + expect(logCount[0].count).toBe(0); + }); +}); diff --git a/src/subdomains/generic/user/models/user/__tests__/user.service.spec.ts b/src/subdomains/generic/user/models/user/__tests__/user.service.spec.ts index 2fd4e0c017..0a498e7966 100644 --- a/src/subdomains/generic/user/models/user/__tests__/user.service.spec.ts +++ b/src/subdomains/generic/user/models/user/__tests__/user.service.spec.ts @@ -1,36 +1,48 @@ +import { createMock } from '@golevelup/ts-jest'; import { ForbiddenException } from '@nestjs/common'; +import { GeoLocationService } from 'src/integration/geolocation/geo-location.service'; +import { SiftService } from 'src/integration/sift/services/sift.service'; import { UserRole } from 'src/shared/auth/user-role.enum'; +import { AssetService } from 'src/shared/models/asset/asset.service'; +import { FiatService } from 'src/shared/models/fiat/fiat.service'; +import { LanguageService } from 'src/shared/models/language/language.service'; +import { UserDataRepository } from 'src/subdomains/generic/user/models/user-data/user-data.repository'; +import { UserDataService } from 'src/subdomains/generic/user/models/user-data/user-data.service'; +import { WalletService } from 'src/subdomains/generic/user/models/wallet/wallet.service'; +import { FeeService } from 'src/subdomains/supporting/payment/services/fee.service'; +import { UserData } from '../../user-data/user-data.entity'; +import { User } from '../user.entity'; +import { UserRepository } from '../user.repository'; import { UserService } from '../user.service'; // Focused on the write-side clearance invariant in `updateUserInternal`: an account may only be given a -// gated role when a verified name is already behind it. The guard reads only `userRepo`, so the service -// is constructed with a mock repository and inert stubs for the other collaborators. +// gated role when a verified name is already behind it. Only `userRepo` participates in this path; the +// remaining collaborators are typed mocks so a future constructor reorder fails the compiler instead of +// silently wiring the service with dependencies in the wrong slots. describe('UserService — elevated role assignment guard', () => { let service: UserService; - let userRepo: { findOne: jest.Mock; save: jest.Mock }; + let userRepo: jest.Mocked; - function userWith(verifiedName: string | null | undefined, loaded = true): any { - const userData = { id: 7, verifiedName }; - return { id: 42, userData: loaded ? userData : undefined }; + function userWith(verifiedName: string | null | undefined, loaded = true): User { + const userData = createMock({ id: 7, verifiedName: verifiedName ?? undefined }); + return createMock({ id: 42, userData: loaded ? userData : undefined }); } beforeEach(() => { - userRepo = { - findOne: jest.fn(), - save: jest.fn().mockImplementation((u) => Promise.resolve(u)), - }; - // Only userRepo participates in this path; the remaining collaborators are never reached. + userRepo = createMock(); + userRepo.save.mockImplementation((entity) => Promise.resolve(entity as User)); + service = new UserService( - userRepo as any, - {} as any, - {} as any, - {} as any, - {} as any, - {} as any, - {} as any, - {} as any, - {} as any, - {} as any, + userRepo, + createMock(), + createMock(), + createMock(), + createMock(), + createMock(), + createMock(), + createMock(), + createMock(), + createMock(), ); }); @@ -41,8 +53,18 @@ describe('UserService — elevated role assignment guard', () => { expect(userRepo.save).not.toHaveBeenCalled(); }); + // SUPER_ADMIN is not itself in KycGatedRoles but satisfies every gate through the role hierarchy, so the + // guard must cover it exactly as the clearance query does — otherwise the highest privilege of all could + // be assigned to a faceless account. + it('rejects a super-role (SUPER_ADMIN) when the account has no verified name', async () => { + await expect(service.updateUserInternal(userWith(null), { role: UserRole.SUPER_ADMIN })).rejects.toBeInstanceOf( + ForbiddenException, + ); + expect(userRepo.save).not.toHaveBeenCalled(); + }); + it('rejects a gated role when the verified name is blank whitespace', async () => { - await expect(service.updateUserInternal(userWith('  \t'), { role: UserRole.ADMIN })).rejects.toBeInstanceOf( + await expect(service.updateUserInternal(userWith(' \t'), { role: UserRole.ADMIN })).rejects.toBeInstanceOf( ForbiddenException, ); expect(userRepo.save).not.toHaveBeenCalled(); @@ -54,7 +76,9 @@ describe('UserService — elevated role assignment guard', () => { }); it('reloads userData when the relation was not hydrated, then allows on a present name', async () => { - userRepo.findOne.mockResolvedValue({ userData: { id: 7, verifiedName: 'Jane Doe' } }); + userRepo.findOne.mockResolvedValue( + createMock({ userData: createMock({ verifiedName: 'Jane Doe' }) }), + ); await service.updateUserInternal(userWith(undefined, false), { role: UserRole.DEBUG }); diff --git a/src/subdomains/generic/user/models/user/staff-kyc-clearance.service.ts b/src/subdomains/generic/user/models/user/staff-kyc-clearance.service.ts index c6a1c9ad7d..6aec42ed42 100644 --- a/src/subdomains/generic/user/models/user/staff-kyc-clearance.service.ts +++ b/src/subdomains/generic/user/models/user/staff-kyc-clearance.service.ts @@ -11,7 +11,9 @@ import { UserRepository } from './user.repository'; // SUPER_ADMIN, which satisfies every gate but is not itself listed in KycGatedRoles). Derived, not // hand-written — a role added to the hierarchy must not silently fall out of the clearance sync and // lose access. -const ClearanceRelevantRoles = rolesSatisfying(KycGatedRoles); +// Exported so the write-side guard in UserService gates the exact same set this service clears — +// a single source keeps the read and write sides from drifting. +export const ClearanceRelevantRoles = rolesSatisfying(KycGatedRoles); // Every character `String.prototype.trim()` strips (ECMAScript WhiteSpace + LineTerminator). Postgres' // bare `TRIM(x)` removes ASCII space ONLY, so a name of a single tab or a non-breaking space would pass @@ -58,9 +60,10 @@ export class StaffKycClearanceService { userData: { // A non-empty verified name is the sole clearance condition: it is only ever set by an // identity-verified path or a reviewed migration, never self-service (see the write paths of - // `verifiedName`), so it is the authoritative identification signal on its own. A KYC level is - // deliberately NOT required — it is unreachable for the DEBUG role and impossible for the - // service accounts that legitimately hold a gated role. + // `verifiedName`). Personal accounts carry an identity-verified name; operator-reviewed service + // accounts carry a non-personal designation. A KYC level is deliberately NOT required — it is + // unreachable for the DEBUG role and impossible for service accounts that legitimately hold a + // gated role. // // `verifiedName IS NOT NULL` is the stated rule, but an empty or blank name carries no // identification either — the predicate covers both, and NULL drops out on its own because the diff --git a/src/subdomains/generic/user/models/user/user.service.ts b/src/subdomains/generic/user/models/user/user.service.ts index 20bd975106..b5d4d3d72e 100644 --- a/src/subdomains/generic/user/models/user/user.service.ts +++ b/src/subdomains/generic/user/models/user/user.service.ts @@ -12,7 +12,7 @@ import { Config } from 'src/config/config'; import { CryptoService } from 'src/integration/blockchain/shared/services/crypto.service'; import { GeoLocationService } from 'src/integration/geolocation/geo-location.service'; import { SiftService } from 'src/integration/sift/services/sift.service'; -import { KycGatedRoles, UserRole } from 'src/shared/auth/user-role.enum'; +import { UserRole } from 'src/shared/auth/user-role.enum'; import { Active } from 'src/shared/models/active'; import { AssetService } from 'src/shared/models/asset/asset.service'; import { FiatService } from 'src/shared/models/fiat/fiat.service'; @@ -54,6 +54,7 @@ import { ReferralDto, UpdateRefDto, UserV2Dto } from './dto/user-v2.dto'; import { UserDetailDto, UserDetails } from './dto/user.dto'; import { UpdateMailStatus } from './dto/verify-mail.dto'; import { VolumeQuery } from './dto/volume-query.dto'; +import { ClearanceRelevantRoles } from './staff-kyc-clearance.service'; import { User } from './user.entity'; import { RefPayoutFrequency, UserAddressType, UserStatus } from './user.enum'; import { UserRepository } from './user.repository'; @@ -473,11 +474,15 @@ export class UserService { async updateUserInternal(user: User, update: UpdateUserInternalDto): Promise { // Write-side counterpart to the staff KYC clearance: an account may only be given a gated role if a // verified name is already behind it, so a faceless staff account with no identification signal can - // never be created. Blankness matches the clearance definition — `verifiedName.trim()` strips exactly - // the characters `BlankChars` lists (see staff-kyc-clearance.service.ts), so this stays in step with - // the DB predicate that decides clearance. userData is reloaded when absent so a caller that did not - // hydrate the relation cannot slip an elevated role past the check. - if (update.role && KycGatedRoles.includes(update.role)) { + // never be created. It gates the same `ClearanceRelevantRoles` set the clearance query clears — + // that set is the gated roles plus their super-roles (e.g. SUPER_ADMIN, which satisfies every gate + // without being listed in KycGatedRoles), so sharing the one constant keeps write and read in step + // and stops the highest privilege from being granted without an identification signal. Blankness + // matches the clearance definition — `verifiedName.trim()` strips exactly the characters `BlankChars` + // lists (see staff-kyc-clearance.service.ts), so this stays in step with the DB predicate that decides + // clearance. userData is reloaded when absent so a caller that did not hydrate the relation cannot + // slip an elevated role past the check. + if (update.role && ClearanceRelevantRoles.includes(update.role)) { const userData = user.userData ?? (await this.userRepo.findOne({ where: { id: user.id }, relations: { userData: true } }))?.userData;