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

[cristim]

Scoped down 2026-08-08. The security-critical core landed in PR #1744: grantAdmin no longer stubs the authorization decision, and the #923 carve-out now has handler-level coverage it previously lacked entirely. This issue now tracks only the remainder. Full status, corrected counts and the mutation evidence are in this comment.

Remaining work

1. Per-handler expected-pair pinning

grantAdmin now answers each (action, resource) correctly through the real matcher, which fully closes the carve-out class. 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 holds both. Each handler needs to declare the pairs it is contractually allowed to ask, with anything else failing. 235 .grantAdmin() invocations across 26 files.

2. Per-handler decision on whether the permission check is unconditional

The original bullet ("drop .Maybe() wherever the permission check is mandatory") cannot be done as written, and that is a finding rather than a task nobody got to.

It presumes "mandatory" is a property of the check. It is not — it is a per-handler contract, and for many handlers the check is legitimately never reached. requirePermission runs after input validation in several handlers: updateGroup, getGroup and deleteGroup all call validateUUID(groupID) first, and createGroup/updateGroup return 400 invalid request body on an unmarshal failure. Every test covering those paths would fail AssertExpectations under a blanket .Maybe() removal, and none of those tests is wrong — the handler correctly never asked.

So the work is to decide per handler whether the check is unconditional, and assert the call explicitly where it is. .Maybe() is retained in grantPermissionsScoped with a comment recording this, so the reasoning is not re-derived from scratch.

3. Restricted-account coverage per handler

grantScoped(accounts...) exists and the shared seam (getAllowedAccounts -> requireAccountAccess) is covered by four tests. What remains is exercising the restricted path per handler across the 17 getAllowedAccounts call sites in 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 one direct GetAllowedAccountsAPI call in handler_purchases_revoke.go.

Each of the three is independently landable and independently reviewable, which the original 235-site sweep would not have been.

---

Original issue text (counts below are as-reviewed at be11bdcb5; see the status comment for corrections)

Reviewed commit: be11bdcb5. Note: origin/main moved to 3e9660d06 during the review; re-verify against current main before changing code, since a finding may have been fixed or moved.

Where

• internal/api/mocks_test.go:285-297 - grantAdmin()
• 227 call sites across 25 files, densest in internal/api/handler_purchases_test.go (45), handler_plans_test.go (33), handler_config_test.go (26), handler_coverage_test.go (24), router_handlers_test.go (24), handler_test.go (19), handler_apikeys_test.go (14)

What

grantAdmin() registers:

• On("HasPermissionAPI", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(true, nil).Maybe()
• the same four-wildcard registration for HasPermissionForConstraintsAPI
• On("GetAllowedAccountsAPI", ...).Return([]string(nil), nil).Maybe()

So every (userID, action, resource) triple returns true, and the allowed-account list is always nil, which the API layer reads as unrestricted. .Maybe() additionally means AssertExpectations can never require that the permission check happened at all.

The authorization decision is the thing under test in a large fraction of these 227 tests, and it is stubbed to a constant.

Failure scenario

Two distinct regression classes are invisible:

1. Wrong or absent check. A handler that asks for the wrong verb/resource pair - say view/purchases where it should ask approve/purchases - is indistinguishable from a correct one, because the wildcard matcher returns true for both. A handler that skips the permission check entirely is also indistinguishable, because .Maybe() means the unconsumed expectation is never reported.

2. Account scoping. With GetAllowedAccountsAPI pinned to nil, no grantAdmin test can ever exercise a restricted account list. That is the exact class of bug (sec(purchases): standard user can pause/resume other users' scheduled purchases (no ownership gate) #950 / fix(api): filter purchases by account UUID AND external id (closes #701, #498, #866) #956) that survived four successive "fixed with green tests" rounds: each fix merged with a green suite and a clean review while the account filter stayed broken in production, because the tests filtered a column the data did not use and ran against a mock that had no restriction to enforce.

This is worth stating plainly, because it explains a pattern rather than just naming a defect: the 2026-07-28 review found several account-scoping holes in the API layer that the test suite did not, and this is why. The suite is structurally unable to see them. Any handler-level scoping fix validated only by grantAdmin tests has not been validated.

Why this is a prerequisite, not a cleanup task

Together with the AssertNotCalled vacuity filed alongside it, this determines whether "fixed, tests are green" means anything on the API authorization surface. It is not exploitable on its own - no production code path is wrong because of it - but it invalidates the safety net that every other authorization fix depends on. Treat it as a precondition for accepting the next "fixed with green tests" claim on a scoping or permission bug.

Fix direction

• Make grantAdmin assert the specific (action, resource) pairs a handler is contractually allowed to ask for, and fail on any other pair, so asking the wrong question is a test failure rather than a pass.
• Drop .Maybe() wherever the permission check is mandatory, so a handler that stops checking fails AssertExpectations.
• Add a companion grantScoped(accounts...) helper returning a non-nil allow-list, and convert the account-scoped handler tests to use it, so the restricted path is exercised at least once per handler that claims to honour it.

Expect the conversion to turn up real failures. Each one is a scoping behaviour that has never been tested.

Related

• sec(purchases): standard user can pause/resume other users' scheduled purchases (no ownership gate) #950 / fix(api): filter purchases by account UUID AND external id (closes #701, #498, #866) #956 - the account-filter regression class this stub makes untestable.
• sec(plans): listPlans does not scope account_ids to caller's allowed_accounts cloud-commitments-platform#29 (sec(plans): listPlans does not scope account_ids to caller's allowed_accounts), bug(api/dashboard): calculateCommitmentMetrics does not intersect allowed_accounts for scoped sessions #959 (calculateCommitmentMetrics does not intersect allowed_accounts), sec(api): listExchangeableAzureRIs returns tenant-wide reservations without allowed-accounts scoping #1532, Azure RI exchange: source reservations are never scoped to the authorized subscription #1527 - live scoping issues in exactly the surface these 227 tests nominally cover.
• sec(auth): enforce per-permission Constraints on execute-any/own, approve-any/own, runPlanned (SEC-01 follow-up to #1210) cloud-commitments-platform#60, fix(api/auth): /me/permissions returns owner's full perms instead of API key's effective scope cloud-commitments-platform#62, sec(auth): unscoped user API key silently inherits owner's full permissions cloud-commitments-platform#61 - permission-constraint enforcement issues whose regression tests would sit on this helper.
• The AssertNotCalled / isExpected critical filed alongside this one: same theme, different mock, and the two together account for most of the API layer's assertion surface.

[cristim]

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.