Repository navigation
fix(test): make grantAdmin model the principal instead of stubbing the authorization decision - #1744
Conversation
…e answer grantAdmin registered HasPermissionAPI returning a constant true for every (userID, action, resource) triple. HasPermissionAPI is the authorization decision, so 235 tests across 26 files asserted downstream behaviour with the gate already answered. The consequence is sharper than "too permissive". Two of the pairs handlers actually ask for under grantAdmin are money verbs carved out of the admin:* wildcard by issue #923: 12 calls execute:purchases grantAdmin answered TRUE 23 calls approve-any:purchases grantAdmin answered TRUE A real Administrators member gets false for all three carved-out verbs. So grantAdmin modeled a principal production cannot have: an admin who may spend money. Verified by mutation: with adminCarvedOuts emptied, internal/api passed green. The separation-of-duties control that #923, #1550 and #1737 exist to defend had no handler-level regression barrier at all. grantAdmin now models the principal's STATE -- it holds exactly {admin, *} -- and every question is answered by the real matcher, auth.AuthContext .HasPermission, which applies the carve-out. The decision function is read after m.Called so the invocation is still recorded; short-circuiting ahead of m.Called is what made 20 assertions vacuous in #1595 and is not reintroduced. 21 purchase-path tests failed once the answer became real, all with "permission denied: requires execute on purchases". Each is a fixture defect, not a handler defect: the test's principal was under-specified. They now use grantAdminPurchaser, modeling an admin who is also in the Purchaser group, which migrations 000059/000064 backfill onto every admin, so it is the default real operator on the money paths. No handler behaviour changed. Adds grantScoped(accounts...) for the restricted allow-list. grantAdmin pins GetAllowedAccountsAPI to nil, read as unrestricted, so no test could exercise account scoping, the structural reason the #950/#956 filter regressions survived four "fixed, tests are green" rounds. New tests pin both properties and fail when the control is removed: TestGrantAdmin_CarveOutIsEnforcedAtHandler, TestExecutePurchase_ PlainAdminIsRefused, and the grantScoped seam tests, each with a negative control so a guard that refused everything would not pass. Refs #1596.
cristim
added
triaged
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR updates authorization test mocks to support dynamic permission decisions. It adds regression coverage for purchase permission carve-outs and scoped account access. Existing purchase and approval tests now use purchaser-specific administrator grants. ChangesAuthorization regressions
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
internal/api/grantscoped_test.go (1)
34-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert
MockConfigStoreexpectations.Line 36 asserts only
mockAuthexpectations. ThemockStore.On("GetCloudAccount", ...)expectations can remain uncalled without failing these tests. Add cleanup formockStore, or remove an expectation when that lookup is not part of the test contract.Proposed fix
mockStore := new(MockConfigStore) mockAuth := new(MockAuthService) t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + t.Cleanup(func() { mockStore.AssertExpectations(t) })As per coding guidelines, new Go tests should use mock-first tests.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/api/grantscoped_test.go` around lines 34 - 36, Update the test cleanup near MockConfigStore and MockAuthService so it also calls mockStore.AssertExpectations(t), ensuring configured GetCloudAccount expectations are verified; retain the existing mockAuth assertion and mock-first test structure.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/api/middleware_test.go`:
- Line 364: Update the authorization fixture in
TestApproveViaSession_RequiresCSRF from grantAdmin to grantAdminPurchaser so the
test reaches ValidateCSRFToken and verifies CSRF rejection rather than failing
permission checks.
In `@internal/api/mocks_test.go`:
- Around line 384-395: Update MockAuthService.grantPermissionsScoped so its
HasPermissionForConstraintsAPI handler rejects empty constraintSets instead of
delegating them to permissionDecision. Preserve the existing decision behavior
for non-empty constraint sets, and retain explicit mock expectations for tests
that exercise constrained permissions.
---
Nitpick comments:
In `@internal/api/grantscoped_test.go`:
- Around line 34-36: Update the test cleanup near MockConfigStore and
MockAuthService so it also calls mockStore.AssertExpectations(t), ensuring
configured GetCloudAccount expectations are verified; retain the existing
mockAuth assertion and mock-first test structure.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2fd9a760-d82e-4032-98cf-81303d2895b3
📒 Files selected for processing (8)
internal/api/executed_notification_flow_test.gointernal/api/grantadmin_carveout_test.gointernal/api/grantscoped_test.gointernal/api/handler_purchases_guards_test.gointernal/api/handler_purchases_test.gointernal/api/handler_ri_exchange_test.gointernal/api/middleware_test.gointernal/api/mocks_test.go
Adversarial review, head
|
| gate | result |
|---|---|
go build ./... / go vet ./... |
clean |
go test -race -count=1 ./... |
exit 0, 31 packages ok, 0 FAIL |
gocyclo -over 10 -ignore "_test\.go" . |
clean |
golangci-lint run at CI-pinned v2.10.1 |
0 issues. |
CI at this head, read from the raw check-runs API rather than the summary: 19 checks, all completed, all success, zero null or in-progress conclusions. MERGEABLE / CLEAN (it reported UNKNOWN briefly while GitHub was still computing).
The headline claim, verified in both directions
The claim only means something if the pre-#1596 baseline is real, so I ran that first, in a separate worktree at origin/main. Emptying adminCarvedOuts deletes the #923 separation-of-duties control outright:
PRE-PR BASELINE (origin/main + adminCarvedOuts emptied): internal/api exit=0 ok 11.826s
Green. The entire control could have been deleted and not one test in internal/api would have noticed. Same mutation at this head:
PR HEAD (b9109a56a + adminCarvedOuts emptied): internal/api exit=1
--- FAIL: TestGrantAdmin_CarveOutIsEnforcedAtHandler/execute:purchases
--- FAIL: TestGrantAdmin_CarveOutIsEnforcedAtHandler/approve-any:purchases
--- FAIL: TestGrantAdmin_CarveOutIsEnforcedAtHandler/retry-any:purchases
--- FAIL: TestExecutePurchase_PlainAdminIsRefused
Four named failures, exactly as claimed. The barrier is real.
"No handler behaviour changed" is structurally true
git diff --name-only origin/main...b9109a56a filtered to non-test files returns nothing. All 8 changed files are _test.go. There is no production code in this changeset, so no handler behaviour can have changed. That half of the claim needs no further argument.
The 21: adjudicated individually, not as an aggregate
Each of the 21 was flipped back to plain grantAdmin() alone and run alone. All 21 fail. I spot-checked three myself end-to-end (executePurchase_InvalidBody, executePurchase_Success, ApproveRIExchange_SessionAdmin) — each fails in isolation, confirming the isolation was genuine rather than an aggregate misread.
The count was never the interesting part. On the adjudication: all 21 are reading (a), fixture defects. Not one is arguably (b). Every one is an execute, approve, or run-purchase endpoint, and each demanded exactly the verb matching its own operation — execute:purchases on the execute/run paths (handler_purchases.go:2128, :307), approve-any:purchases on the approve paths (handler_purchases.go:753, handler_ri_exchange.go:2168/2202). None is a read, list, or validation-only endpoint, and no approve endpoint demands execute. There is no site where a handler over-demands, so nothing is being papered over.
One subtlety worth recording, because the naive reading of the 11 approve rows is wrong: admin:* does grant approve-own — only approve-any is carved out. So the admin-only principal passes the own-check and is then denied by the ownership comparison (handler_purchases.go:770, handler_ri_exchange.go:2220), which is why those denials read "cannot approve another user's pending purchase" rather than the expected requires approve-any on purchases. The effective demand really is approve-any, because the row belongs to someone else — confirmed in the fixtures (SessionAdmin has creatorID ≠ session; the purchase fixtures leave CreatedByUserID nil; FourEyesOn_DifferentApproverSucceeds requires a different approver by construction). So approve-any is load-bearing in all 11, which is exactly the #923 semantic.
A bonus the mutation run proves: authorization strictly precedes body validation. validateExecutePurchaseRequest calls requirePermission at handler_purchases.go:2128 and only reaches json.Unmarshal at :2133. The seven validation tests (InvalidBody, EmptyRecommendations, NegativeUpfrontCost, NegativeSavings, TooManyRecommendations, ExceedsMaxAmount, SaveError) all returned permission denied: requires execute on purchases instead of their own 400 messages. That is the correct secure ordering — an unauthorized caller learns nothing about which field was malformed or what the configured cap is — and granting the Purchaser verb is precisely what lets those tests reach the validation they are actually about.
The sharpest question: does grantAdminPurchaser model a principal production permits?
Yes, and I verified both halves rather than accepting the migration argument.
It is the default operator. 000064:86-99 is an unconditional admin-backfill: every user whose group_ids contains the Administrators UUID (…0001) gets the Purchaser UUID appended. So admin+Purchaser is what a real deployment produces, not an exotic pairing.
It does not over-grant. adminCarvedOuts (internal/auth/types.go:115-119) is exactly {execute, approve-any, retry-any} × purchases — precisely the three verbs grantAdminPurchaser adds on top of admin:*. The seeded Purchaser group grants those three plus four view:* verbs, and admin:* already covers the views. So {admin:*} ∪ {the three} and {admin:*} ∪ {Purchaser's seven} denote the same permission set. The fixture is exactly faithful; it cannot be over-privileged relative to the principal it models.
The matcher is used honestly. grantPermissionsScoped builds &auth.AuthContext{Permissions: perms} and delegates to the real AuthContext.HasPermission. I checked that function reads only ctx.Permissions (types.go:131-151) — no dependence on Groups, User or AllowedAccounts — so the bare struct cannot behave differently from production through a silently-defaulted field.
The design note holds on every path
permissionDecision is called after m.Called in both readers (HasPermissionAPI, HasPermissionForConstraintsAPI), so the invocation is always recorded first. I checked this is not just true on the common path: every method on MockAuthService reaches m.Called, with no early return anywhere in the type. The #1595 defect is not reintroduced.
retry-any:purchases is covered in both directions
It appears in carvedOutVerbs, which drives both TestGrantAdmin_CarveOutIsEnforcedAtHandler (plain admin refused) and TestGrantAdminPurchaser_GrantsCarvedOutVerbs (admin+Purchaser allowed). So the refusal-only failure mode you would expect here does not apply.
TestGrantAdmin_NonCarvedVerbsStillGranted is a genuine negative control, and a well-chosen one: it includes view:purchases and update-any:purchases, the same resource as the carved-out verbs, so it pins the specific (action, resource) pair rather than "purchases". Without it a mock that denied everything would satisfy the refusal test.
Applying the #1735 arity lesson: mockStore.AssertNotCalled(t, "SavePurchaseExecution", mock.Anything, mock.Anything) passes 2 matchers and SavePurchaseExecution records (ctx, exec) = 2. Arity matches, so that assertion is genuinely failable.
N1 (not a defect here, but a trap for the work #1596 has left)
Exactly one test in the package layers a specific HasPermissionAPI expectation after a grant* call: coverage_gaps_test.go:370, three lines below grantAdmin() at :367. testify's findExpectedCall returns the first registered expectation whose arguments match, and grantAdmin's generic mock.Anything matcher is registered first, so the specific one never serves the call. AssertExpectations still passes because methodWasCalled matches on the actual arguments regardless of which expectation object answered.
Proven by execution rather than by reciting testify semantics — I flipped that line from Return(true, nil) to Return(false, nil):
go test -run '^TestHandler_requireAdmin_AdminRole$' ./internal/api/ EXIT=0
The test passes either way. The expectation is provably inert.
This is benign today: the line asserts admin:* is granted, decide() answers true, and the outcome is the same. It was equally inert before this PR, when the generic returned a constant true. But it is a live trap for the 235 remaining grantAdmin sites that #1596 still has to convert. Anyone who layers a specific Return(false, ...) after grantAdmin to model a denial will have it silently ignored and the test will pass for the wrong reason — the same vacuous-assertion class this whole line of work exists to eliminate. The safe patterns are to register the specific expectation before grant*, or to call grantPermissions([...]) with the exact permission set instead of layering on top. Worth one sentence in the helper's doc comment, since that comment is what the next 235 conversions will be read against.
Does the boundary leave anything unsafe?
No. The change is strictly tightening: grantAdmin went from answering true for everything to answering true for everything except the three carved-out verbs. No test can newly pass that previously failed; the only possible effect is new failures, and all 21 were found and adjudicated. The remaining #1596 work (per-handler expected-pair pinning, grantScoped conversion at the 17 getAllowedAccounts sites) is genuinely unfinished rather than unsafe — account scoping is exactly as untested as it was before, not newly so.
The one inherent limitation worth naming: grantAdminPurchaser models a principal holding every permission, which is correct for admin+Purchaser but means tests using it cannot detect a handler demanding a wrong-but-held verb. That is precisely the per-handler pinning left to #1596, so the boundary is drawn in the right place.
Two pre-existing observations outside this diff, flagged for completeness only: runPlannedPurchase validates the UUID syntactically before authorizing (discloses nothing), and approvePurchaseViaSession returns a 409 carrying status=<X> before authorizeSessionApprove runs, so an authenticated session holding no approve rights can probe existence and status of an execution whose exact UUID it already knows. Low severity, unrelated to this changeset.
Verdict
Every claim I was asked to attack holds, and the two that mattered most — the pre-PR baseline being silently green, and all 21 being fixture rather than handler defects — I re-derived independently rather than accepting the aggregate. The principal grantAdminPurchaser models is exactly the one the migrations create, and it is provably not over-privileged. No production defect is being masked.
Nothing blocking. N1 is a doc-comment sentence, and it is worth adding before the remaining 235 conversions rather than after.
Execution-verified: both directions of the carve-out mutation including the origin/main baseline, three of the 21 flipped and run in isolation by me plus all 21 by an independent pass, the shadowing proof, and all five gates. Reading-derived: the AuthContext.HasPermission field-dependence analysis, the permission-set equivalence argument for grantAdminPurchaser, and the migration backfill (read at 000064:86-99).
HasPermissionForConstraintsAPI's decision-function path (registered by grantPermissionsScoped, used by grantAdmin/grantAdminPurchaser/ grantScoped) answered purely from action/resource and never inspected constraintSets, so it allowed an empty slice for any verb the mocked principal holds. Production (auth.Service.HasPermissionForConstraintsAPI) fails closed on an empty constraintSets slice: it's a caller bug, not a grant. Harmless today since every current grant helper passes only Constraints == nil permissions and every constrained-permission test registers its own explicit mock.On(...) expectation, but a trap for a future test that reaches the constrained check through the auto-answering path instead. Guards the decision-function branch on len(constraintSets) == 0 and fails closed with an error, matching production exactly. Explicit mock.On(...).Return(bool, err) expectations are untouched. Adds TestGrantPermissionsScoped_ConstrainedCheckFailsClosedOnEmptyConstraintSets, covering both the fail-closed empty-set case and a positive control. Verified fail-then-pass: reverting the guard, the empty-set assertion fails with "An error is expected but got nil" (mock returned (true, nil)); with the guard, it returns (false, err) as production would. Also documents the ordering trap CodeRabbit's earlier N1 finding flagged on grantAdmin: a test-specific mock.On(...) expectation registered AFTER grantAdmin is silently shadowed by the generic catch-all grantAdmin registers first (testify serves the first matching expectation), so it must be registered before grantAdmin or expressed via grantPermissions instead. One sentence added to grantAdmin's doc comment ahead of the ~235 remaining grantAdmin conversions #1596 has left. Declined the third finding (middleware_test.go:321, grantAdmin -> grantAdminPurchaser on TestApproveViaSession_RequiresCSRF): verified by execution that the test passes identically with either principal, since approvePurchase's dispatch falls through unconditionally to approvePurchaseViaSession when token=="", and that function checks CSRF before re-running the RBAC check. Declined in a reply on the review thread with the execution evidence; no source change.
|
Addressed the two CodeRabbit findings and the N1 doc-comment ask from the adversarial review. Pushed Fixed: constrained-check fail-closed trap (
|
|
Merging. Recording what the independent review established, and one finding that was declined rather than fixed. The headline claim was re-derived from the pre-PR baseline, not accepted. An independent reviewer ran the mutation against Deleting the #923 separation-of-duties control outright used to be silent. "No handler behaviour changed" is structurally true, not a judgment: All 21 repairs adjudicated individually, flipped back alone and run alone: 21/21 fail. Not one is arguably a handler defect — every case is an endpoint demanding exactly the verb matching its own operation. A subtlety worth recording:
One CodeRabbit finding was declined with evidence rather than applied. The suggestion to move That trace surfaced a separate production finding, now tracked as #1753: the outer dispatch discards an authorization denial and falls through to the same call. Verified latent, not live by execution with valid CSRF tokens — safe today on two independent facts (inner and outer call the same A real mock/production divergence was found and fixed here: the decision-function path answered A latent trap is documented rather than restructured. Scope stays as ruled: #1596 remains open for the remainder — per-handler expected-pair pinning across 235 invocations in 26 files, and Gates: build, vet, |
1 participant
Refs #1596. Scope question for the reviewer at the bottom — this deliberately does not say
Closes.adminCarvedOutscould be deleted entirely and no test ininternal/apiwould failThat is the finding. Verified by mutation, not inferred:
The #923 money separation-of-duties control — the one #1550 and #1737 just spent three commits defending at the service layer — had zero handler-level coverage. Deleting it outright was a silent mutation. This PR makes it a loud one.
Why:
grantAdminstubbed the decision, not the stateHasPermissionAPI(ctx, userID, action, resource) (bool, error)is the authorization decision.grantAdminregistered it returning a constanttruefor every triple, andMockAuthServicereplaces the whole auth service, so no resolution ran. 235 tests across 26 files asserted downstream behaviour with the gate pre-answered.I instrumented
grantAdminto record every pair the suite actually asks it, rather than guessing. 30 distinct pairs; two are carved-out money verbs:execute:purchasesapprove-any:purchasesretry-any:purchasesview:purchasesupdate-any:purchasesProduction column verified by execution against the real matcher (
AuthContext{Permissions: [{admin,*}]}).grantAdminmodeled a principal production cannot have: an admin who may spend money.The fix: model state, let production logic answer
grantAdminnow declares that the principal holds exactly{admin, *}and routes every question through the real exportedauth.AuthContext.HasPermission, which appliesadminCarvedOuts. Same instinct aspermissionsForGroupsin #1737: ask the production question instead of asserting the expected answer.The decision function is read after
m.Called, so the invocation is still recorded — short-circuiting ahead ofm.Calledis exactly what made 20 assertions vacuous in #1595, and this does not reintroduce it.Adds
grantAdminPurchaser()(admin + Purchaser membership) andgrantScoped(accounts...)(restricted allow-list).The 21 repaired tests are fixture defects, not handler defects
Making the answer real broke 21 purchase-path tests, every one with
permission denied: requires execute on purchases. In each case the fix is "grant the test's principal the membership a real operator needs", never "change what the handler does" — the tell for a fixture gap. No handler behaviour changed anywhere in this PR. No production defects found.They now use
grantAdminPurchaser, modeling an admin who is also in the Purchaser group. Migrations000059/000064backfill exactly that pairing onto every existing admin, so it is the default real operator on the money paths, not an exotic one.Per-test mutation, not aggregate
Each repaired test had its principal flipped back to plain
grantAdminindividually and was re-run alone. A test that still passed would mean the repair was cosmetic:All 21 genuinely depend on the money-verb grant.
Restricted account access was untestable
grantAdminpinsGetAllowedAccountsAPItonil, which the API layer reads as unrestricted, so no test in this package could exercise a restricted allow-list. That is the structural reason the #950/#956 account-filter regressions survived four rounds of "fixed, tests are green": the suite had no restriction to enforce.grantScopedfixes the mechanism, and four new tests pin the shared seam (getAllowedAccounts->requireAccountAccess) that every scoped handler funnels through: out-of-scope account refused with the enumeration-safeerrNotFound, in-scope account allowed, an unrestricted admin still reaching the same account the scoped principal is refused, and name-based matching.Counts, corrected
be11bdcb5: 226 invocations. The issue's 227 counted the definition line.main: 235 invocations across 26 files (issue said 25 files);handler_plans_test.gois 38, not 33 — main grew.grep -rn ... | wc -lreturned 205, because this tooling truncates grep output (+13 more in ...) andwc -lcounted a shortened list. The reliable figure comes from summing per-filegrep -ocounts, which reconciles exactly to 235.Gates
Two findings fixed locally rather than shipped to review: a
unparamon an unused return and twomisspellhits.Scope: why this says
Refsand notCloses#1596's fix direction has three bullets. This PR delivers the security-critical core and not all of it:
view:purchaseswhere it should askapprove:purchases) still passes, because a real admin holds both. Catching that needs per-handler expected-pair pinning across all 235 sites..Maybe()wherever the permission check is mandatory." — Not done. "Mandatory" is a per-handler contract; many handlers legitimately return before the check (bad body, bad UUID). Retained with a comment explaining why.grantScopedand convert the account-scoped handler tests." — Helper and shared-seam coverage done; per-handler conversion not. There are 17getAllowedAccountscall sites across 9 files; converting each needs full per-handler setup.I would rather land the core — which closes the carve-out blind spot and repairs 21 tests — than grow this into an unreviewable auth PR. Reviewer's call: merge as-is and keep #1596 open for bullets 1-2 and the per-handler half of 3, or split those into a follow-up and close #1596 here. Happy either way; flag it and I will do the follow-up.
Separately,
retry-any:purchasesis asked zero times and has no non-test reference ininternal/api— a carved-out verb with no handler coverage at all. Being filed separately.Summary by CodeRabbit
Bug Fixes
Tests