Repository navigation
test(api): model the constraint dimension in MockAuthService - #1859
Merged
Merged
Conversation
grantPermissionsScoped wired HasPermissionForConstraintsAPI to the same decide(action, resource) closure as HasPermissionAPI, so the SEC-01 execution-time check answered without ever reading the constraint arguments. Measured against the real matchers, the mock allowed requests outside the permission's constraints on all five dimensions: Providers, Regions, Services, AccountIDs and MaxPurchaseAmount, the last of which is the spend guard. A handler test asserting a constraint refusal was green whether or not the handler enforced anything. Extract auth.PermissionsAllowForConstraintSets, the decision half of Service.HasPermissionForConstraintsAPI with the permission fetch removed, and answer the mock from it. permissionsAllow and the four constraint matchers become package functions: they read nothing off the *Service receiver, and the exported entry point needs them without one. Replace the justifying comment. It claimed a principal holding only admin:* satisfies any constraint set for the verbs it holds, which is true and stayed true through #1758: both halves short-circuit on the wildcard before any constraint comparison, so #1758's carve-out only changed which verbs they deny in lockstep. What it never covered is the helper it was attached to, since grantPermissionsScoped takes an arbitrary permission set and a permission carrying Constraints is exactly what the check bounds. Registering a constraint-blind func on that method now panics, so the divergence cannot be reintroduced there silently. Scope: this covers the session branch of requirePermissionConstraints. The user-API-key branch calls HasAPIKeyPermissionForConstraintsAPI, which the mock still stubs to a constant via allowConstraintChecks; modelling it faithfully needs the key-and-owner permission intersection production computes, so it is left for a follow-up. Closes #1762
cristim
added
priority/p1
Contributor
|
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 (7)
Included review availability: 0 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 1 review per hour. 📝 WalkthroughWalkthroughChangesThe change centralizes permission constraint evaluation in package-level helpers. It updates service and mock authorization paths to preserve constraint sets. New regression tests cover constraint dimensions, administrator carve-outs, and handler-level 403 responses. Constraint-aware authorization
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to The change makes the mock authorization path honor constrained permissions while preserving production behavior, with targeted regression coverage and successful verification. No actionable merge-blocking risk remains beyond normal checks and review. Sequence Diagram(s)sequenceDiagram
participant Handler
participant MockAuthService
participant PermissionsAllowForConstraintSets
participant permissionsAllow
Handler->>MockAuthService: request constrained permission
MockAuthService->>PermissionsAllowForConstraintSets: permissions, action, resource, constraint sets
PermissionsAllowForConstraintSets->>permissionsAllow: evaluate constraint set
permissionsAllow-->>MockAuthService: allow or deny
MockAuthService-->>Handler: authorization result
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Member
Author
|
@coderabbitai full review The push at 06:42Z was skipped with the included review limit reached, so this head has never been reviewed. Checking whether the allowance has refilled. This PR changes production authorization code in internal/auth, not only the test mock, so an independent pass matters here. |
Contributor
|
🧠 Learnings used✅ Action performedFull review finished. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
MockAuthService.grantPermissionsScopedwired bothHasPermissionAPIandHasPermissionForConstraintsAPIto the same closure:decidetakes(action, resource)and never reads the constraint arguments, so the SEC-01 execution-time check answered without them. The constraint dimension (AccountIDs,Providers,Services,Regions,MaxPurchaseAmount) was not modelled at all.Measurement
Taken before any edit, on unmodified
d9d1c1302, comparing realpermissionsAllowagainst the mock's exactdecideclosure for the same principal:execute:ri-exchangeconstrainedProviders:[aws], requestProviders:[azure]Providers:[aws]execute:purchasescappedMaxPurchaseAmount:1000, request5000500Regions:[eastus]permitted, requestRegions:[eastus,westus]AccountIDs:[acct-a]permitted, requestAccountIDs:[acct-b]Services:[ec2]permitted, requestServices:[rds]admin:*alone onexecute:ri-exchangeadmin:*on a non-carved verb with a constraintadmin:*plus constrainedri-exchange, requestProviders:[azure]10 cases, 6 divergent. Two things the table shows that matter more than the count:
Providers. Row 3 is the money one: aMaxPurchaseAmountof 1000 did not stop a request for 5000.Two corrections to the issue
Recording these because a reviewer checking the issue's reasoning against this diff will otherwise find a mismatch. The issue's central claim is correct and understated; two supporting arguments do not hold. Both are also on the issue itself.
1. The
#1758argument does not hold. The issue says #1758 falsifies the comment by putting a carved-out verb within reach. It does not, and row 8 of the table is the direct disproof:admin:*alone onexecute:ri-exchangeisfalse/false, same. Adding a pair toadminCarvedOutsmakes "admin holds none of the carved-out verbs" more true. For an admin-only principal the two sides are structurally identical:AuthContext.HasPermissiontakes no constraint parameter, andpermissionsAllowshort-circuits oncheckAdminPermissionbeforecheckPermissionConstraintsis reached. Bothcontinueon a carve-out hit and fall through tofalse. #1758 only changed which verbs they deny in lockstep.Worth stating precisely, since it is security-relevant: they ignore the constraint arguments for an
admin:*permission unconditionally, not because that permission happens to carry no Constraints. Attaching Constraints to anadmin:*grant would not make the check bind.What actually made the comment false: its premise ("a principal holding only
admin:*") describesgrantAdmin, but it sits ongrantPermissionsScoped, whichgrantPermissionsalso drives with an arbitrary permission set. It was false from the day it was written, for a reason unrelated to #1758. Related: migration000096_seed_ri_exchanger_group.up.sql:48-56documents the seeded RI Exchanger grant as deliberately UNCONSTRAINED.2. "Every handler test that appears to exercise it is measuring the mock" overstates it. No pre-existing test changed behaviour; all 2170
internal/apitests passed unchanged on the first run after the rewiring. Verified againstd9d1c1302rather than a working tree: concatenating everyinternal/api/*_test.goat that commit and grepping for the struct fieldConstraints:returns 0 hits across the whole file set. All 259grant*call sites passConstraints == nilpermissions, for whichcheckPermissionConstraintsreturns true unconditionally, so old and new mock answer identically for every principal any existing test builds.This was already known:
grantadmin_carveout_test.go:176records it during #1596 review, calling it "a trap for the first constrained-permission test." The divergence was latent, not fired. Worth being accurate about, because describing it as fired invites dismissal by counter-example, which would leave the trap in place.The change
auth.PermissionsAllowForConstraintSets, the decision half ofService.HasPermissionForConstraintsAPIwith the permission fetch removed, and answer the mock from it. The mock now models the principal and lets production logic derive the answer, as fix(test): make grantAdmin model the principal instead of stubbing the authorization decision #1744 did for the permission half.permissionsAllowand the four constraint matchers become package functions. Verified against the parent commit that none reads a field off the receiver;sexisted only to reach the next method.errNoConstraintSets, now shared by all three fail-closed guards that previously duplicated the format string.Registering a constraint-blind
func(action, resource string) boolon that method now panics, naming the issue. This is the part that keeps the fix from decaying: the original defect was invisible except by measurement, and a future inline registration would have reintroduced it just as silently. Now it fails loudly at the point of the mistake.Tests
Three tests in
grantadmin_carveout_test.go. Each constraint case holds aninsideand anoutsiderequest in the same case, deliberately: an always-allow mock passes an inside-only assertion and an always-deny mock passes an outside-only one, so only the pair distinguishes them.Pre-fix proof, with all six source changes stashed so the tests ran against genuine
d9d1c1302code:Every failure is by assertion, not by panic. Every
insideassertion passed pre-fix, which is what rules out the refusal-only trap. Post-fix 11/11 pass.Reviewers additionally mutation-tested the committed tree; every mutant was killed by the expected assertion. Neutering
matchPurchaseAmountConstraintkills exactly the spend-cap rows; regressingmatchAllRegionsConstraintto any-overlap kills exactly the regions row; removing{ActionExecute, ResourceRIExchange}fromadminCarvedOutskills the admin test.One pre-existing test moved during implementation:
service_api_test.go:756"empty constraint sets fail loud". My first cut left the empty-constraintSetsguard only in the shared function, reordering the method to fetch-then-guard. That subtest registers no store expectations, so it asserts not merely "returns an error" but "a caller bug never reaches the store". The test was right; I reverted and the guard is back before the fetch, with error precedence byte-for-byte as atd9d1c1302. The duplication across both entry points is load-bearing, not sloppy: removing either half breaks a different test.Verification
go build ./...clean;go test ./...7083 passed across 43 packages;go vetclean;golangci-lintat the CI pin v2.10.1 exit 0 with non-empty0 issues.;gocyclo -over 10clean, sanity-checked at-over 5(884 hits) to confirm the tool actually ran. All re-run after rebasing ontof8ed6bff5; the pre- and post-rebase patch-ids are identical.Scope
This covers the session branch of
requirePermissionConstraints. The user-API-key branch callsHasAPIKeyPermissionForConstraintsAPI, which the mock still stubs to a constant viaallowConstraintChecks. Same defect one branch over, on the same money paths; modelling it faithfully needs the key-and-owner permission intersection production computes, so it is deliberately left out.Separately filed: issue LeanerCloud/cloud-commitments-platform#208, for the one test helper that re-implements
matchAllRegionsConstraint's all-match rule rather than asserting it.Closes #1762
Summary by CodeRabbit