fix(test): grantAdmin authorization-decision stub — remainder after #1744 (pair pinning, .Maybe() per-handler, scoped-account conversion) #1596

Activity

1. cristim

   added

   triagedItem has been triagedItem has been triaged

   priority/p1Next up; this sprintNext up; this sprint

   severity/criticalMajor harm when it happensMajor harm when it happens

   urgency/nowDrop other thingsDrop other things

   impact/internalTeam-internal onlyTeam-internal only

   effort/lWeeksWeeks

   type/bugDefectDefect

   on Jul 28, 2026
2. cristim commented on Aug 8, 2026

   

   cristim

   on Aug 8, 2026

   MemberAuthor

   More actions

   Status: partially addressed by PR #1744. This issue stays open for the remainder.

   #1744 delivers the security-critical core and deliberately carries Refs LeanerCloud/cloud-commitments-cli#1596, not Closes. Status against the three fix-direction bullets:

   Bullet	Status
   1. Make grantAdmin assert the specific (action, resource) pairs a handler may ask, and fail on any other	Partial
   2. Drop .Maybe() wherever the permission check is mandatory	Not done — and not doable as written; see below
   3. Add grantScoped(accounts...) and convert the account-scoped handler tests	Helper + shared-seam coverage done; per-handler conversion not

   What #1744 did do

   grantAdmin no longer stubs the authorization decision. It declares that the principal holds exactly {admin, *} and routes every question through the real matcher (auth.AuthContext.HasPermission), which applies adminCarvedOuts.

   That closed a blind spot this issue did not name, and it is the reason the PR was worth landing on its own: adminCarvedOuts could have been deleted entirely and no test in internal/api would have failed. Proven by mutation in both directions — carve-out emptied, pre-#1744 suite exit=0; with #1744's tests, exit=1. The #923 money separation-of-duties control had zero handler-level coverage.

   Empirically, handlers asked execute:purchases 12 times and approve-any:purchases 23 times under grantAdmin, and got true for both. A real Administrators member gets false for all three carved-out verbs. 21 purchase-path tests were repaired (fixture defects, not handler defects — no handler behaviour changed), each mutation-verified individually: 21/21 killed when reverted to the plain-admin principal.

   Corrected counts

   • 235 .grantAdmin() invocations across 26 files on current main. The issue says 227 across 25 files; 227 was correct at the reviewed commit be11bdcb5 if the definition line is counted (226 invocations + 1). handler_plans_test.go is now 38, not 33 — main grew.
   • 17 getAllowedAccounts call sites across 9 files (handler_accounts.go 2, handler_analytics.go 1, handler_dashboard.go 2, handler_history.go 1, handler_ladder.go 1, handler_marketplace.go 1, handler_recommendations.go 1, handler_ri_exchange.go 4, scoping.go 4), plus 1 direct GetAllowedAccountsAPI call in handler_purchases_revoke.go.

   Methodology note, because it produced a wrong number first: grep -rn ... | wc -l returns 205, not 235, because the tooling truncates long grep output and wc -l then counts a shortened list. Sum per-file grep -o '\.grantAdmin()' "$f" | wc -l instead.

   ---

   Remainder, and a correction to bullet 2

   Bullet 1 — partial, and what is left

   Answering correctly per pair fixes the carve-out class completely, but it does not catch a handler asking the wrong non-carved verb: view:purchases where it should ask approve:purchases still passes, because a real admin genuinely holds both. Catching that needs per-handler expected-pair pinning across the 235 sites — each handler declaring the pairs it is contractually allowed to ask, with anything else failing.

   Bullet 2 — this cannot be done as stated, and that is a finding rather than a deferral

   The bullet presumes "mandatory" is a property of the permission check. It is not — it is a per-handler contract, and for many handlers the check is legitimately not reached.

   requirePermission runs after input validation in several handlers, so a request that fails validation returns before any authorization question is asked. updateGroup, getGroup and deleteGroup all call validateUUID(groupID) first; createGroup and updateGroup return 400 invalid request body on a JSON unmarshal failure. Every test covering those paths would fail AssertExpectations if .Maybe() were dropped globally, and none of them is wrong — the handler correctly never asked.

   So a blanket removal is not a cleanup that someone simply has not got round to; it would break correct tests. The real work is to decide, per handler, whether the permission check is unconditional, and to assert the call explicitly in the tests where it is. That is a different and larger task than the bullet describes, and it should be planned as one rather than attempted as a sweep.

   .Maybe() is retained in grantPermissionsScoped with a comment recording this reasoning, so the next reader does not re-derive it.

   Bullet 3 — helper done, conversion not

   grantScoped(accounts...) exists and returns a non-nil allow-list. Four tests pin the shared seam every scoped handler funnels through (getAllowedAccounts -> requireAccountAccess): out-of-scope refused with the enumeration-safe errNotFound, in-scope allowed, an unrestricted admin still reaching the account the scoped principal is refused, and name-based matching.

   What remains is exercising the restricted path per handler across the 17 call sites above, each of which needs its own handler setup.

   ---

   Suggested scoping of this issue going forward

   1. Per-handler expected-pair pinning (bullet 1's remainder).
   2. Per-handler decision on whether the permission check is unconditional, then assert it where it is (bullet 2, re-specified).
   3. Restricted-account coverage for each of the 17 getAllowedAccounts call sites (bullet 3's remainder).

   Each is independently landable and independently reviewable, which the 235-site sweep would not have been.
3. cristim

   changed the title [-]fix(test): grantAdmin stubs the authorization decision under test across 227 call sites[/-] [+]fix(test): grantAdmin authorization-decision stub — remainder after #1744 (pair pinning, .Maybe() per-handler, scoped-account conversion)[/+] on Aug 8, 2026
4. cristim

   added a commit that references this issue on Aug 8, 2026

   fix(test): fail closed on empty constraintSets in the auth mock

   ...

   40325fc
5. cristim

   closed this as completedin #1744on Aug 8, 2026
6. cristim

   added a commit that references this issue on Aug 8, 2026

   fix(test): make grantAdmin model the principal instead of stubbing th…

   ...

   d04cf31
7. cristim

   added a commit that references this issue on Aug 19, 2026

   test(api): model the constraint dimension in MockAuthService (#1859)

   ...

   b9f0d77