[IAmJSD]

Summary

permissionsAllow (internal/auth/service_group.go) treated an {admin, *} permission as allowing everything and applied its Constraints only when the request set StrictScope. A key created as {admin, *} with accounts=[A] and/or max_amount therefore passed every non-strict constraint check for account B or for any amount. #957 made those constraints survive key creation, so this became reachable through the API.

The non-strict path is the one the API layer runs for most user-API-key money checks: requirePermissionConstraints → HasAPIKeyPermissionForConstraintsAPI, e.g. enforcePurchaseConstraints for execute-any / execute-own / approve-own, none of which are in the admin carve-out set. The owner gate does not help here: an Administrators owner holds an unconstrained {admin, *} and passes everything, so the key's own constraints were the only thing that could refuse the request, and they were skipped.

Fix

I took the issue's first option ("apply constraints on an admin permission everywhere"), because it fails closed for keys already stored with constrained admin permissions. Rejecting such permissions at creation would leave those keys unenforced.

• In permissionsAllow, an admin permission is now checked with checkPermissionConstraints(perm, constraints) on every check, not only under StrictScope. This is the same rule every other permission already follows.
• Unchanged:
  • An unconstrained {admin, *} (the seeded Administrators group) still passes, because checkPermissionConstraints passes a permission that has no Constraints.
  • The money-verb carve-outs still run first.
  • Callers that pass nil constraints (grant ceilings, HasPermission) behave as before.
  • The empty "holds verb" probe (sessionHoldsVerb, []PermissionConstraints{{}}) still reports the verb as held, because an empty request dimension counts as unspecified.
• On the issue's second point: validateAPIKeyPermissions still checks only action and resource at creation. Its doc comment now says that constraints are enforced at check time, against both the key and the owner, and that this includes {admin, *}. After this change, admin-key constraints are no longer silently ignored on these checks.

The same rule now also narrows a group permission {admin, *} that carries Constraints on the session path (HasPermissionForConstraintsAPI). Strict checks already narrowed it, so this only makes the non-strict path consistent.

No migration and no DB change, so there is no concurrency surface.

Tests

• internal/auth/admin_constraints_test.go
  • TestPermissionsAllow_ConstrainedAdminNonStrict: non-strict checks against a constrained admin permission, one row per dimension (account, provider, service, region, amount), plus allow controls and the empty probe.
  • TestPermissionsAllow_AdminWithoutConstraintsUnchanged: an unconstrained admin still passes, and carve-outs still deny.
  • TestPermissionsAllow_ConstrainedAdminAlongsideExplicitGrant: a constrained admin permission next to an explicit grant.
  • TestHasAPIKeyPermissionForConstraintsAPI_ConstrainedAdminKey: a {admin,*} key with accounts=[acct-a] and max 500 under an Administrators owner. Account B is refused, an amount over the cap is refused, and an in-scope request is allowed.
  • TestHasPermissionForConstraintsAPI_ConstrainedAdminGroup: the session path with a group-granted constrained admin permission.
• internal/server/apikey_admin_constraints_test.go (TestAuthServiceAdapter_ConstrainedAdminKeyRefusedOutOfScope): creates the key through the real CreateAPIKeyAPI adapter and auth.Service, with the store mocked, as {admin,*} + accounts=[acct-a] + max_amount=500. It then checks the stored key through HasAPIKeyPermissionForConstraintsAPI. Account B and $501 are refused; account A at $500 is allowed.

Fail before / pass after: I reverted only service_group.go to origin/main with the new tests in place. These failed:

• 4 of the 5 new auth tests. The failures were the five deny rows of ConstrainedAdminNonStrict (other account, provider, service, region, over cap), the two deny rows of ConstrainedAdminKey (account B, over the key cap), ConstrainedAdminGroup and ConstrainedAdminAlongsideExplicitGrant. The fifth test, AdminWithoutConstraintsUnchanged, is a control and passed, as intended;
• the server test, on both deny subtests.

Every allow control passed. All pass with the fix applied.

Verification (local, macOS)

All evidence here is unit, mock and fixture evidence. I ran tests with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null because this machine's default AWS profile is invalid. No real cloud calls were made.

• go build ./... and go vet ./...: clean.
• go test -count=1 ./internal/auth/ ./internal/server/ ./internal/api/ ./internal/mocks/: pass.
• go test -tags integration -count=1 ./internal/server/ -run TestConfigPermissionConstraintsHTTP and ./internal/api/ -run TestMarketplacePermissionConstraintsHTTP: pass. These run on real Postgres via testcontainers and include the existing admin / admin-bounded cases.
• golangci-lint v2.10.1 (--allow-parallel-runners) on ./internal/auth/... ./internal/server/...: 0 issues.
• gocyclo -over 10 -ignore "_test\.go" internal/auth/: silent.
• scripts/gosec-hook.sh on the changed files: exit 0.
• git-secrets (--register-aws) scan of all tracked files, run in an isolated fresh repo: clean. scripts/test-git-secrets-allowlist.sh: 42 PASS, 0 FAIL.

Real-environment gap: I did not run an end-to-end HTTP request with a constrained admin key against a deployed instance. The server-level test exercises the real create path and the real check, with a mocked store.

Out of scope (noted, not changed)

• API-key read scoping: getAccountScope derives the account scope for read endpoints from the owner's group allowed_accounts, for every key type. A key's permission AccountIDs constraint therefore never narrows what the key can read. Permission Constraints in general (for users and keys alike) are enforced only where handlers call requirePermissionConstraints. That is a broader design question than this issue.
• UserHasAdminCapability and permissionsResponse (IsAdmin) still treat any {admin, *} as admin regardless of its Constraints. They answer action/resource-level questions only, so I left them alone.
• Operator note: keys created between fix(auth): keep API key permission constraints on create #957 and this deploy with constrained admin permissions are narrowed automatically once this deploys. There is nothing to recreate.

Closes #956

Maintainer: please mirror the issue's triage labels onto this PR: priority/p2, severity/medium, triaged, urgency/this-quarter, impact/few, effort/s, type/security.

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes

  • Administrator wildcard permissions now consistently honor account, provider, service, region, and purchase-amount constraints during authorization.
  • Constrained administrator API keys and group or session access are denied for out-of-scope requests while remaining available for permitted requests.
  • Unconstrained administrator permissions and explicit permission carve-outs retain their existing behavior.

• Tests

  • Added coverage for constrained administrator permissions across API-key, group, session, and multi-dimension authorization scenarios.

[coderabbitai]

Note

Currently processing new changes in this PR. This may take a few minutes, please wait...

⚙️ Run configuration

• Configuration used: Repository: LeanerCloud/cloud-commitments-platform/.coderabbit.yaml
• Review profile: CHILL
• Plan: Advanced
• Run ID: bd52568d-730c-4c09-bcd5-0b62b3c9cbf1

📥 Commits

Reviewing files that changed from the base of the PR and between 8caed39 and dcb8f58.

📒 Files selected for processing: 4

• internal/auth/admin_constraints_test.go
• internal/auth/service_apikeys.go
• internal/auth/service_group.go
• internal/server/apikey_admin_constraints_test.go

+---------------------------------------------------------------------------------------------------------+
| If a system is to serve the creative spirit, it must be entirely comprehensible to a single individual. |
+---------------------------------------------------------------------------------------------------------+
  \
   \     /)/)
        ( '')
        /  |
      o(___)

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

_{Comment @coderabbitai help to get the list of available commands.}

[cristim]

Independent review (claude-cc-platform reviewer, cold brief): APPROVE dcb8f58d2564b4b7f52d5c2f6523ffc831d92abf

The production change is one line in permissionsAllow (internal/auth/service_group.go:337). Every constraint-aware entry point routes through it: HasPermission, HasPermissionForConstraintsAPI, PermissionsAllowForConstraintSets, and the key and owner gates of HasAPIKeyPermissionForConstraintsAPI. One fix therefore covers them all.

Real admins are unaffected:

• The seeded Administrators group {admin,*} carries no Constraints (migration 000024), and nil or empty constraints pass every matcher.
• The admin API key bypasses the auth service.
• Group permissions are a pure union, so an Administrators member is not narrowed by a constrained admin grant in another group.
• The only behaviour change is the intended one: a constrained {admin,*} grant is now narrowed on non-strict checks.

Fail-closed is intact: every store or lookup error still returns (false, err). The change only adds a deny condition.

Interactions:

• fix: rate-limiter worker context, pause/resume update-any constraints, IAM parity wildcards #1134's update-any checks now also narrow a constrained admin, which is intended.
• The approve-any and execute carve-outs still need explicit grants, so the four-eyes paths are unchanged.

Mutation probes (all killed):

• reverting to the StrictScope-only condition: 4 auth tests and the server test fail;
• dropping the purchase-amount term;
• dropping the AccountIDs term: 5 tests fail;
• making the key gate ignore request constraints.

Tests and drift: go test -race -count=1 ./internal/auth ./internal/api ./internal/server passes at the head and on the tree merged with current main (clean merge; the 20 newer main commits touch none of these files).

Nit: a request that omits a dimension (for example accounts) passes a constrained admin under the non-strict matcher. That is the same rule as every other permission, and only matters for handlers that omit the dimension.

A separate pre-existing follow-up from this review has been reported to the owner directly.

[cristim]

Independent review (claude-fleet): APPROVE dcb8f58

Findings: none blocking. A non-blocking follow-up outside this diff was reported privately to the maintainers, per SECURITY.md.

• [non-blocking nit] A non-strict request that omits a dimension still passes a constrained admin. matchStringListConstraints (internal/auth/service_group.go:432-437) treats an empty list as "unspecified", and every other permission follows the same rule. Only handlers that drop a dimension are affected.

Check sites:

Site	Constraint-aware	Constrained admin now
permissionsAllow (service_group.go:337-358)	yes	narrowed on every check (the fix, :343)
HasPermissionForConstraintsAPI (service_api.go:488) and PermissionsAllowForConstraintSets (:521)	yes, via permissionsAllow	narrowed
HasAPIKeyPermissionForConstraintsAPI (service_apikeys_api.go:416, key gate :450, owner gate :453)	yes	narrowed on both gates
HasPermission (:306) and HasPermissionAPI (service_api.go:470)	nil constraints	unchanged
sessionHoldsVerb probe (handler_purchases.go:533)	empty request	still allowed (amount 0 is valid, purchase_amount_cap.go:28-36)
StrictScope callers (handler_config.go:154; ladder, marketplace and revoke handlers)	strict	unchanged (already narrowed)

Attacked:

• Back-compat: an unconstrained admin with nil constraints still passes checkPermissionConstraints.
• Union semantics: when a constrained admin grant fails, the loop continues. An unconstrained admin from another group, or an in-scope explicit grant, still allows, so there is no over-denial.
• The money-verb carve-out still runs before the constraint check.
• Wildcards: only {admin, ResourceAll} hits the admin branch. {X, *} takes the normal match path, which already applied constraints.
• API keys: the effective set is the key intersected with the owner. Constraints come from the key, and the owner gate is checked separately.
• Groups and caching: groups are covered by the session-path test. These paths have no permission cache.
• Every caller of the changed function was grepped. None loses a legitimately allowed action; the only new denials are out-of-scope requests against constrained admin grants.
• Mutation probes on service_group.go, each restored and confirmed byte-identical with cmp:
  • M1, revert to the StrictScope-only condition: 5 tests FAIL.
  • M2, the admin branch checks AccountIDs only: 4 tests FAIL, including the server test.
  • M3, an empty request list denies a constrained permission: 2 tests FAIL.
  • M4, an unconstrained admin no longer short-circuits: 15+ tests FAIL, including AdminWithoutConstraintsUnchanged.
  • M5, the admin branch ignores MaxPurchaseAmount: 3 tests FAIL.

Verified locally, on a git archive export of HEAD with no network or credentials:

• go build ./... and go vet ./internal/auth/ ./internal/server/: clean. gofmt -l: clean.
• go test -count=1 -race ./internal/auth/ ./internal/server/ ./internal/api/: all ok.
• The new tests against the base export: 4 of 5 auth tests FAIL, and so does the server test. AdminWithoutConstraintsUnchanged is a control and passes, as expected.

Reviewed by an independent agent with no author context. Review only: nothing was pushed, and the GitHub review state is unchanged.

[cristim]

Local verification at dcb8f58d2564b4b7f52d5c2f6523ffc831d92abf merged onto current main (d16a94f5; merge-tree 7bc940bd; macOS):

• go build ./..., go vet -tags integration ./... and go test -race -short ./... pass (full suite).
• Nothing was applied to any cloud.

[cristim]

Marked ready and merged by the owner's agent after 2 independent reviews and local verification (see the review and verification comments above, all at dcb8f58d2564b4b7f52d5c2f6523ffc831d92abf).