feat(auth): grant the Support role on DEV for support-dashboard measurements - #4441
feat(auth): grant the Support role on DEV for support-dashboard measurements#4441joshuakrueger-dfx wants to merge 9 commits into
Conversation
|
Reviewed across two passes — conformance against Blocking
The On the precedent cited in the description: A wertfrei audit row (migration name, timestamp, affected row count, user id) would satisfy the "when" and "to which" halves without any sensitive content. Minor1. 2. The specs do not pin the "only" semantics they claim — 3. The two "when ENVIRONMENT is not dev" tests only cover 4. The grant is considerably broader than the stated measurement purpose — Checked and fine
Two wording points in the description
|
CONTRIBUTING requires that an overwritten value stay reconstructible from the database: when a field changed, from which value, and to which. The WHERE predicate only pinned the previous value, leaving no record of when the migration ran or whether it matched a row at all. Both directions now run the UPDATE and the audit insert as a single statement, so a failing audit write aborts the role change (fail-closed). The row is written even when no user matched, which is what makes "ran and changed nothing" distinguishable from "never ran". It carries the migration name, direction, affected count, affected user ids and both roles, and no address or other personal data. down() is now a true inverse: it only demotes when the up() audit row reports a non-zero affected count, so a wallet that already held Support before this migration keeps it.
The previous spec asserted address, target role and source role as three separate toContain calls, so a predicate using OR instead of AND would have passed while matching every User row. The conjunction is now asserted as one contiguous fragment, and the suite runs the migration against a throwaway Postgres schema, following the MIGRATION_TEST_PG pattern already used by the custody and realunit migration specs. The behavioural cases cover what string matching cannot: a second User address stays untouched, a target address holding Compliance is left alone, the audit row carries the right affected count, down() after up() restores User, down() without a promoting up() leaves an existing Support role in place, and the address match is case-insensitive. The environment gate is now table-driven over prd, stg, loc, the empty string and an unset value, instead of asserting "not dev" while only ever setting prd.
…head develop has moved on to 1785460000000 since this branch was opened, so the migration was back-dated. TypeORM would still run it, but the immutability check blocks any rename once the file is merged, so the timestamp can only be corrected now. Renames the file, the class, the name property, the migration name carried in both audit log rows and the spec's require path and assertion.
The migration specs have used MIGRATION_TEST_PG since #4418 without it appearing in .env.example, so a fresh checkout gives no hint that the real-Postgres suites exist or why they skip. This branch adds another such spec, so the gap is documented here.
AddTradingOrderCreatedIndex landed on develop carrying 1785470000000, the same prefix this migration had, which leaves the order between the two undefined. ClearDevUserSignatures has since raised the highest timestamp on develop to 1785500000000. Moves to 1785550000000, following the rounded values develop uses rather than a real creation time, and renames the class, the name property, the migration name in both audit rows and the spec's require path and assertion.
…list Each of the ten non-dev cases carried its own set() closure, which spent eighty lines on what is a list of five values used twice. ClearDevUserSignatures writes the same table as it.each over the values themselves, so this follows the spec that sits next to it on develop. %p rather than %s, because the set here includes the empty string, and %s would render that case and an unset value as indistinguishable titles. No assertion changes: the contiguous AND-fragment checks, the remaining SQL assertions and the Postgres block are untouched, and the count stays at 18. Against the migration without the audit row 5 of them fail; the OR mutant in up() (3), the OR mutant in down() (2), LOWER() dropped in both directions (3), the down() ownership guard removed (2), affectedCount pinned to 0 (2), the environment gate inverted (17) and the single statement split into two queries (3) each turn the suite red.
The two comparable audit migrations on develop take the log system from the entity they mutate: FixFiatOutputValutaDateSerials writes 'FiatOutput' for fiat_output, ClearDevUserSignatures writes 'User' for user. This one wrote 'Auth', which appears nowhere else as a log system - the only two occurrences in the repo are a Swagger tag and a notification type. Moves both audit rows and the down() ownership guard to 'User'. The guard matters: it looks up the up() row by system, so leaving it on 'Auth' while up() wrote 'User' would keep down() from ever restoring the role. The down()-after-up() case covers that coupling and fails if the two drift apart. No change in blast radius either way, since every reader of the log table scopes to system 'LogService' and the cleanup job runs per configured pair. Pinning the value to 'Wrong' fails the audit-row test.
2 participants
What changed
One migration that grants the
Supportrole on DEV only to a single wallet address, so the support-dashboard endpoints become reachable there.ENVIRONMENT === 'dev'. The grant is scoped to DEV, and whether the same address exists elsewhere is unverified, so no other environment is ever touched.up()only promotesUser→Support. An address holdingCompliance,Admin,SuperAdminorSupportis left untouched.down()only restoresUserwhere the role is stillSupportand theup()audit row reports a non-zero affected count. An address that already heldSupportbefore this migration keeps it.Auditability
CONTRIBUTING.mdrequires that an overwritten value stay reconstructible from the database: when a field changed, from which value, and to which. Both directions therefore write one row into the existinglogtable.UPDATEand the audit insert are one statement (WITH updated AS (UPDATE … RETURNING id) INSERT INTO "log" … SELECT … FROM updated), so a failing audit write aborts the role change. Both merged precedents use this shape:1783780521095-PurgeRealUnitRegistrationSteps.js:107,155and1785500000000-ClearDevUserSignatures.js:54,61,75.created. No address and no other personal data.fromRoleis the constantUserrather than a value read back from the row. The predicate constrains the update to rows in exactly that role, so the previous value isUserfor every affected row by construction.down()does not delete theup()row; it appends its own rollback row.systemfollows the table, not the domain. The rows carrysystem = 'User',subsystem = 'GrantSupportRoleOnDev'. Both comparable audit migrations ondevelopnamesystemafter the entity they mutate —FixFiatOutputValutaDateSerialswrites'FiatOutput'forfiat_output,ClearDevUserSignatureswrites'User'foruser.down()'s ownership guard looks theup()row up bysystem, so the two are kept in step; thedown()-after-up()case fails if they drift apart.Blast radius of the new rows. No existing reader matches that pair. Line numbers below are as of
develop213d4b5b4.log.repository.tsis scoped bysystemandsubsystem: asfindconditions at:119-120,:131,:141,:149,:176-177and:243-244, through a correlated subquery at:158-159and:206-207, and as raw parameter conditions at:297and:404.systemalone, which is what keeps these rows separate fromClearDevUserSignatures' rows in the same'User'system.log.valid(setFinancialLogValidity,:599) inheritsLogService/FinancialDataLogscoping fromaddFinancialLogValidityConditions(:625-626), so it cannot alter the audit rows.(system, subsystem)pair (:97-98), for which no setting exists here.systemare the pagination-cursor lookups (:228,:262,:569). Each resolves one id taken from an already-scoped page, so it cannot pull these rows into a result set.Why
PR #4344 was closed because the performance premise was never validated against real request timings, with the explicit ask to measure the list query and
getMessageStatsseparately first. That measurement needs Staff access on DEV.Measured on DEV before this change: a wallet-signature token for that address (
role: User) reachesGET /v1/userwith 200, but the dashboard routes onsupport-issue.controller.tsreturn 403. TheGETroutes gated byRoleGuard(UserRole.SUPPORT)arelist(:96),counts(:107),statistics(:115),escalation/telegram-chats(:126),activity(:150),clerks(:161),clerk(:169) and:id/data(:188); the@UseGuardsline sits two to three lines below each route decorator, after the Swagger annotations. Not everyGETon that controller is gated:@Get()requires onlyACCOUNT(:89), and@Get(':id')(:177) plus the message-file route (:224) use optional JWT auth.A mail login did not close the gap:
getMailLoginUser(StaffRoles)resolves onlyCompliance,SupportandRealUnit(shared/auth/user-role.enum.ts:27,user-data.entity.ts:720), the account carried a single address in roleUser, and no other active staff user existed on it — the function searches all users of the account.Consequence worth stating.
getMailLoginUser()matches a user holding a staff role that also has awalletand is not blocked or deleted. Once this migration has run, the granted user meets the role condition, so if it also carries a wallet, the account's mail login yields aSupporttoken and the private key is no longer required for this access. That is acceptable on DEV, but it is a widening of the access path and is called out here rather than left to be discovered.Impact
DEV data only, one row, one column, plus one audit row per direction. No schema change, no endpoint, guard, role enum or role hierarchy change. On every non-DEV environment both directions are a no-op.
The role is carried as a JWT claim, so an already-issued token keeps its old role — a fresh login is required after this runs.
Scope of the grant
Supportis the weakest role that reaches the dashboard endpoints, so the choice of role is minimal. It is not narrow in what else it opens: the same role gates user search and detail views, support notes and templates, mutating user/account/KYC endpoints and owner-independent transaction detail.The grant is not left in place. It exists for the #4344 measurement and is removed again once the timings are recorded. The removal is a separate follow-up migration rather than a rollback:
migrationsRunexecutes pending migrations at boot (config.ts:265) and nothing in the repo invokesdown(), so there is no operational revert path for a merged migration.down()remains the correct TypeORM inverse and is exercised by the specs, but it is not the mechanism that will be used. That follow-up is owed as its own PR and is not implied by this one.Tests
src/subdomains/generic/user/models/user/__tests__/grant-support-role-on-dev.migration.spec.ts— 18 tests: 10 pin the environment gate, 2 pin the statement shape and its SQL, and 6 run the migration against a throwaway Postgres schema viaMIGRATION_TEST_PG. EveryTest (shard n/3)job starts its ownpostgres:16service and sets that variable for the run (api-pr.yaml:174-181,210) — deliberately on all three shards, because Jest distributes the migration suites across them — so all 18 run in CI.The behavioural cases cover what string matching cannot: a second
Useraddress stays untouched, a target address holdingComplianceis left alone, the audit row carries the right affected count,down()afterup()restoresUser,down()without a promotingup()leaves an existingSupportrole in place, and the address match is case-insensitive. The environment gate is table-driven overprd,stg,loc, the empty string and an unset value, per direction;%prather than%skeeps the empty string and the unset case distinguishable in the report.The predicate is additionally asserted as one contiguous
LOWER(…) AND "role" = …fragment rather than as separatetoContaincalls, so anORmutant cannot pass the string layer either.Counter-checks, each measured on this branch against a real Postgres
AND→ORinup()(3 tests), the same indown()(2),LOWER()dropped in both directions (3), thedown()ownership guard removed (2),affectedCountpinned to 0 (2), the environment gate inverted (17), the single statement split into a separateUPDATEandINSERT(3), and the auditsystemvalue changed to something else (2).Migration immutabilitystays green: the migration is an addition againstdevelop, and the gate only blocksM/D/R/Tentries.Not covered: the
nameproperty is not asserted against the file name, so a future edit could desynchronise the two without a test failing. And none of this ran against the real DEV database — whether exactly one row there matches the address in roleUseris not established by these tests.Checks
npm run lintempty output,npm run format:checkclean,npm run type-checkexit 0,npm run buildexit 0, andnpm testexit 0 withTest Suites: 7 skipped, 321 passed, 321 of 328 totalandTests: 159 skipped, 5817 passed, 5976 total. The 7 skipped suites are theMIGRATION_TEST_PGspecs, which that machine has no Postgres for; those 6 tests were verified separately against a realpostgres:16, where the spec reportsTest Suites: 1 passedandTests: 18 passed, 18 totalwith nothing skipped.1785550000000is above the current highest migration ondevelop(1785500000000-ClearDevUserSignatures). It was raised twice on this branch:developmoved past the original1785311300000, and the replacement1785470000000was then taken byAddTradingOrderCreatedIndex, which would have left the order between the two undefined. The immutability check blocks renames once a migration is merged, so the correction is only possible before that.developand reportsMERGEABLE; it is not rebased, because that would mean force-pushing a branch that is already under review.