feat(auth): require staff KYC clearance on elevated endpoints - #4395
Conversation
|
Six review passes were needed before both independent reviews came back clean. What changed along the way: Correctness of the blank-name filter. Moving the A manual override on the allowlist. The generic Smaller items. Import ordering, a stale class reference in a comment, argument pinning in the CI is green on the final commit, including the dedicated coverage gate (100% on the three files that Note on CI history: the last commit did not trigger the PR workflow automatically, so the pipeline was |
Access to endpoints gated on Admin, Debug, Compliance, Support or RealUnit now additionally requires an identified natural person behind the calling account: kycLevel >= 50 and a non-empty verifiedName. The check lives in RoleGuard, so it covers all 235 elevated gates at once. It is a property of the endpoint, not of the caller: it applies only when every entry role of a gate is elevated, so an admin using ordinary customer functionality is unaffected. StaffKycClearanceService derives the cleared account ids from the DB every minute, ProcessService primes an in-memory Set from the resulting setting, and RoleGuard reads it synchronously. This mirrors the existing JWT denylists: the requirement takes effect on already-issued tokens within one refresh interval, with no DB lookup per request. The allowlist is deliberately fail-closed - an empty or not-yet-primed Set denies every elevated endpoint, so a DB or cron outage cannot silently re-open admin access. Three privilege checks that live in business logic rather than a gate are covered too, via the new hasStaffAccess helper. All three sit on routes that are not role-gated (OptionalJwtAuthGuard), so the requirement would otherwise be bypassable: protected KYC file downloads, ownership-independent access to any customer's transaction history, and posting official support replies. Privilege checks behind already-gated endpoints (the isAdmin distinctions in the support note services) are left unchanged - the gate has run by then. The three files deciding elevated access are pinned at 100% coverage on all four metrics via a dedicated CI step, following the existing Frick coverage gate.
Restores the alphabetical import order in user.module.ts and fixes a comment in setting.service.ts that referenced a class name that does not exist.
Moves the verifiedName condition out of JS and into the query, per the
"filter in SQL, not JS" rule: TRIM(...) <> '' covers empty and whitespace-only names and
drops NULL on its own, since the comparison yields NULL.
The raw fragment interpolates the alias TypeORM passes in, which is already quoted - an
unquoted camelCase identifier would be folded to lowercase by Postgres and fail at
runtime. Verified against a real Postgres instance: the emitted SQL is
TRIM("..."."verifiedName") <> '' and only genuinely verified staff rows survive, with
NULL, empty and whitespace-only names all filtered out. The unit test pins the rendered
predicate, since mocked repositories never execute the fragment.
Also restores the alphabetical import order in process.service.ts.
Two holes found in review, both in the path that decides elevated access. Blank names: Postgres TRIM() with no character argument strips ASCII space only, while the JS trim() it replaced also strips tabs, newlines and Unicode spaces. A verifiedName of a single tab or a non-breaking space therefore passed the filter and cleared an account carrying no identification. The predicate now uses BTRIM with the explicit character set that trim() strips, spelled out rather than left to [[:space:]], whose meaning depends on the database locale. A mocked repository never executes raw SQL, so a new suite runs the predicate against a real Postgres and pins it to the semantics it replaced: every blank variant is excluded, padded real names are kept, and the result is asserted to equal the JS filter on every input. It follows the existing MIGRATION_TEST_PG pattern, so CI runs it against its throwaway database. Manual override: the generic PUT /setting/:key route accepted staffKycClearance, which contradicted this branch's own comment and let a cleared admin write uncleared accounts into the allowlist, live until the next sync overwrote it. Sync-owned settings are now rejected there; the sync path writes through setObj and is unaffected.
Moves the system-managed check from the controller into SettingService.set, where it covers every caller rather than only the HTTP route, and keeps the controller thin as the contributing guide requires. The sync path writes through setObj, which goes to the repository directly, so it stays unaffected - now pinned by a test. Widens the Postgres suite so the character set itself is guarded: the blank fixtures are derived from the JS runtime rather than from BlankChars, because fixtures generated from the constant under test would shrink along with it and could never catch a character dropped from it. Verified by removing one character from BlankChars, which turns the suite red.
Pins key and value on the generic setter instead of only asserting that something was written - a swapped argument pair would otherwise pass, and this is the only place the pair is checked since the controller test was folded into the service test. Records why U+200B is absent from BlankChars: trim() does not strip it either, so adding it would make the predicate stricter than the check it replaced. Whether a name made of invisible characters counts as identification is a question about how verifiedName is written, not about this gate.
9d8c5bc to
d084bea
Compare
|
Correction to the CI note above: the branch has since been rebased onto the current |
* feat(gs): grant staff clearance to the Debug account The staff-clearance rule (#4395 -> #4572) requires a non-empty user_data.verifiedName on top of the role for every elevated RoleGuard, including the Debug gate on POST /gs/debug. The backfill in #4574 covered two accounts; user data 403938 was not among them and still answers every elevated endpoint with STAFF_KYC_REQUIRED. Adds a PRD-only, idempotent, audited migration in the shape of #4574. The personal name is read from the deployment variable STAFF_VERIFIED_NAME_403938 and never appears in this repository. The closing assertion checks the clearance predicate itself — the BlankChars-aware BTRIM that StaffKycClearanceService uses — instead of equality with the supplied value. Should an identity-verified path have written a different, correct name in the meantime, that account is cleared and the deploy must not fail over the spelling. * fix(gs): make the backfill precondition the exact negation of its assertion The update repaired only a NULL verifiedName while the closing assertion demanded a non-blank one, so a present-but-blank name (a lone tab, a non-breaking space) was a state the migration refused to repair and then refused to accept. Because migrationsTransactionMode defaults to 'all', that throw rolls back the whole release's migration batch and fails DataSource.initialize() — a PRD crash-loop, not a failed tool. The precondition now uses the same BTRIM predicate as the assertion, so the two are complementary by construction and the blank case becomes a repair. Keeping a divergent existing name is no longer silent either: the audit entry records the outcome, so the deployed state cannot differ from the reviewed one without a trace. A re-run after a successful backfill stays a true no-op. Adds the migration spec the two preceding data migrations ship, including the blank-name cases that were the defect, so the verification runs in CI on the postgres:16 service instead of living in a throwaway container. * test(gs): pin the assertion's account scope and the duplicated BlankChars The spec left the non-target account's verifiedName NULL in every database case, so `count(*) = 1` held whether or not the closing assertion was scoped to the account. Dropping `"id" = 403938 AND` from the postcondition passed all 20 tests while in production it would count every cleared account and throw. The fixture now gives that account a cleared name, which kills the mutant with 10 failures. The blank-name cases covered four hand-picked characters, so the migration's copy of BlankChars could lose any of the other 21 code points unnoticed. The set is now derived from the runtime — every character String.prototype.trim() strips, which is what BlankChars is defined as — and the migration must repair a name built from all of them. Also drops a vacuous assertion: the log count was read after the rollback, where it holds regardless of what the migration wrote. docs/staff-kyc-clearance.md still prescribed the `verifiedName IS NULL` precondition this branch removed, so the next author following it would reproduce the boot-fatal gap. The recipe now states the exact-negation requirement, and step 2 no longer says the value is applied "only when present" when PRD in fact throws without it. * docs(gs): correct the JSDoc precondition and state the assertion the recipe requires The migration's JSDoc still said the update "only touches a still-null verifiedName", carried over from the migration this one is modelled on and false since the precondition became the negation of the closing assertion. This has to be fixed before the merge: api-migration-check.yaml allows comment-only edits to an existing migration by stripping whole lines starting with `//`, and JSDoc lines start with `*`, so afterwards the sentence could never be corrected. The runbook demanded the precondition be the exact negation of the closing assertion without saying what that assertion must be. The only precedent in the repo asserts equality with the supplied name, so an author following both would write an equality assertion — which throws when an identity-verified path wrote a different but valid name, and under the 'all' transaction mode takes the release's whole migration batch and the boot with it. The recipe now states the clearance-predicate assertion explicitly, and names `develop` rather than `main` as the last point where the deploy order can still be arranged. Two spec corrections: the BlankChars pin claimed a drifted copy would fail the migration's own assertion, when in fact the postcondition shares the drifted constant and reports success — the drift is silent, which is precisely why the test asserts the repaired state. And a test name still promised a post-rollback assertion that no longer exists. Adds the sanity floor on the derived character set that the sibling spec already carries. * docs(gs): make the recipe's negation NULL-total and require the audit coupling Taken literally, "the exact negation of that assertion" produced `BTRIM("verifiedName", <BlankChars>) = ''` — without the COALESCE that the printed precondition one paragraph earlier does carry. That is not the negation over NULL: a NULL name yields NULL rather than true, so the ordinary un-backfilled account is neither repaired nor accepted, which is the same boot-fatal shape the paragraph exists to prevent. Verified as a mutant against the shipped SQL: three tests fail with the postcondition error. The instruction now spells out both forms and says which is wrong. The ban on equality assertions was justified with only one of its two failure modes, and that one is unreachable once the negation rule is applied — inviting the conclusion that the ban is redundant. Both branches are now stated: against a blankness precondition an equality assertion throws over a spelling difference, against its own negation it silently overwrites an identity-verified name. The recipe also never mentioned the audit coupling, though both shipped backfills gate the write on EXISTS (SELECT 1 FROM "audit") and CONTRIBUTING treats unaudited mutation of a PII column as blocking. An author following only the runbook produced an unaudited migration. Corrects the release-PR mechanism too: auto-release-pr.yaml checks for an open PR first and creates one only when none exists, so it keeps a release PR open continuously rather than opening one per push. The conclusion — develop is the last point where the deploy order can be arranged — is unchanged.
What
Access to endpoints gated on
Admin,Debug,Compliance,SupportorRealUnitnowadditionally requires an identified natural person behind the calling account:
kycLevel >= 50and a non-blankverifiedName. The role alone is no longer sufficient.How
The check lives in
RoleGuard, so it covers all 235 elevated gates at once.It is a property of the endpoint, not of the caller: it applies only when every entry role
of a gate is elevated. A gate that also admits
UserRole.USERstays an ordinary endpoint that astaff member happens to reach through the role hierarchy, so ordinary customer functionality is
unaffected.
Data path, mirroring the existing JWT denylists:
StaffKycClearanceServicederives the cleared account ids from the DB every minuteProcessServiceprimes an in-memorySetfrom the resultingstaffKycClearancesettingRoleGuardreads it synchronously — no DB lookup per requestBecause the clearance is read from live data rather than the token, revoking a staff member's KYC
takes effect on already-issued tokens within one refresh interval — no re-login, no JWT secret
rotation.
The allowlist is deliberately fail-closed, unlike the denylists it mirrors: an empty or
not-yet-primed
Setdenies every elevated endpoint. A DB or cron outage must never silentlyre-open admin access.
ProcessService.onModuleInittherefore awaits the first prime before HTTPstarts, and a failing resync keeps the last known
Setrather than clearing it.staffKycClearanceis derived data, so it is listed inSystemManagedSettingsand rejected bySettingService.set. Without that, the genericPUT /setting/:keyroute would let a cleared adminwrite uncleared accounts into the allowlist, live until the next sync overwrote it. The sync path
writes through
setObjand is unaffected — blocking it would freeze the allowlist.Blank names
The
verifiedNamecondition is filtered in SQL. Postgres' bareTRIM()strips ASCII space only,unlike JS
trim(), so a name consisting of a tab or a non-breaking space would otherwise clear anaccount carrying no identification. The predicate uses
BTRIMwith the character settrim()strips,spelled out explicitly rather than left to
[[:space:]], whose meaning depends on the database locale.A mocked repository never executes raw SQL, so
staff-kyc-clearance.pg.spec.tsruns the predicateagainst a real Postgres and pins it to the semantics it replaced: every blank variant is excluded,
padded real names are kept, and the outcome is asserted to equal the JS filter on every input. The
blank fixtures are derived from the JS runtime rather than from the constant under test, so removing
a character from that constant turns the suite red. It follows the existing
MIGRATION_TEST_PGpattern and runs in CI against the throwaway Postgres service.
Bypass paths closed
Three privilege checks live in business logic rather than behind a gate, on routes that are
not role-gated (
OptionalJwtAuthGuard). Without these the requirement would be triviallybypassable — an account losing
/adminwould keep:KycService.getFileByUid)HistoryAccessService)SupportIssueController.createSupportMessage)All three now use the new
hasStaffAccesshelper. Privilege checks behind already-gatedendpoints (the
isAdmindistinctions in the support note services) are deliberately leftunchanged — the gate has already run by then.
Scope notes
BankingBotandCustodyare not staff entry roles and stay ungated, so a staff caller reachingthose endpoints through the role hierarchy is not KYC-gated. Out of scope here; happy to extend.
trim()does not strip it either, soincluding it would make the predicate stricter than the check it replaced. Whether a name made of
invisible characters counts as identification is a question about how
verifiedNameis written.Tests
access, enforced by a dedicated CI step (
test:staff-gate:cov) following the existing Frickcoverage gate — including the full-compilation transform, so phantom branches from
transpile-only decorator-metadata emit cannot skew the gate
Before merging
This changes access for existing staff accounts on deploy: any account not meeting the requirement
loses every elevated endpoint, including the admin endpoints used to correct KYC data. Please
confirm the staff account data is in the expected state first — recovering afterwards means a
direct DB correction (the setting is sync-derived and will be overwritten) or a rollback. There is
also a short window after boot, until the first sync runs, in which no account is cleared.