[cristim]

Summary

Closes #950.

API key permission constraints were silently dropped on create. CreateAPIKeyAPI (internal/auth/service_apikeys_api.go) JSON re-encoded api.CreateAPIKeyRequest into auth.APICreateAPIKeyRequest. That request carried []auth.Permission, whose constraint tags are account_ids and max_purchase_amount. The api request (and the frontend, frontend/src/api/types.ts:556-564) sends accounts and max_amount. As a result, a key requested as "only account 123, max $500" was stored with no account scoping and no amount cap. Providers, services and regions survived because their tags match.

The fix:

• APICreateAPIKeyRequest.Permissions is now []APIPermission, the wire shape the group requests already use.
• CreateAPIKeyAPI converts it with the existing apiPermissionsToPermissions mapper, the same one CreateGroupAPI uses.
• The handler is unchanged. The re-encode remains only for name, password and expires_at, whose tags match.

The new test confirms the restored constraints are enforced. HasAPIKeyPermissionForConstraintsAPI checks each constraint set against both the key's own permissions, which now keep their constraints, and the owner's permissions. A key's constraints therefore narrow the owner's and never widen them.

Tests

• internal/server/adapter_test.go (TestAuthServiceAdapter_CreateAPIKeyAPI_KeepsPermissionConstraints) sends a real api.CreateAPIKeyRequest through the server adapter and auth.Service to the store mock, with accounts, providers, services, regions and max_amount all set.
  • On unfixed main it failed: AccountIDs came out nil and MaxPurchaseAmount came out 0.
• TestAPIAdapters_CreateAPIKey_AllFieldsReachStore applies the test(auth): fail when an API adapter drops a request field #923 requireFieldsCovered completeness check to the key request, so a future field or constraint drop fails CI.
• Six mutations were run and all were caught:
  • drop AccountIDs, drop MaxAmount or drop Regions;
  • swap Providers and Services;
  • remove the conversion;
  • revert the accounts tag to account_ids. Only the server adapter test catches this one, which is the right layer.

Verification (local, macOS)

• go build ./... and go vet ./... are clean.
• go test -race -count=1 ./internal/auth/... ./internal/api/... ./internal/server/... passes.
• golangci-lint v2.10.1 reports 0 issues.
• Re-encode inventory: service_apikeys_api.go:204 was the only api-to-auth JSON re-encode. The user and group adapters use type assertions. There is no key update or rotate route.

Operator note

This fixes keys created after deploy. Keys created before it keep the broader, unconstrained scope they were stored with, still bounded by their owner's permissions. Consider auditing existing keys whose creators intended constraints, and revoking and recreating them.

Review

An independent non-author reviewed it in two passes and found no blocking findings at cc492d3.

Pre-existing gaps found during review are filed separately as #956:

• {admin, *} key permissions ignore their constraints on non-strict checks.
• validateAPIKeyPermissions does not check constraints at creation.

🤖 Generated with Claude Code

[cristim]

@coderabbitai review

[coderabbitai]

Warning

Review limit reached

• Run on-demand review

_{This review includes 4 billable files and costs up to $1.00.}

• Ask an admin to make reviews automatic

Open in CodeRabbit

Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing.

Or wait 45 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available. Your 65 included PR review attempts over the past 7 days set your current allowance at 1 review per hour.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

• Configuration used: Repository: LeanerCloud/cloud-commitments-platform/.coderabbit.yaml
• Review profile: CHILL
• Plan: Essentials
• Run ID: ff630c40-016a-42ef-91aa-9c2c8491a603

📥 Commits

Reviewing files that changed from the base of the PR and between 2ef2f5d and d177d1d.

📒 Files selected for processing: 4

• internal/auth/service_api_fields_test.go
• internal/auth/service_apikeys_api.go
• internal/auth/service_apikeys_api_test.go
• internal/server/adapter_test.go

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

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

[coderabbitai]

⚠️ Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[cristim]

Independent review verdict: CLEAN at cc492d3

Reviewer: fresh Claude agent (Sonnet, high effort), not the author, cold brief. The PR body and the earlier author-side review were treated as claims. Coordinator: claude-cc-reviewer.

Blocking findings: none.

Non-blocking notes:

• internal/auth/service_apikeys.go:180-195 (validateAPIKeyPermissions) checks only action and resource against the owner. It does not check the requested constraints, so a constrained owner can mint a key with broader constraints. Enforcement still intersects with the owner's permissions. This gap predates the PR and is already filed as sec(auth): an admin-scoped API key ignores its account and amount constraints on non-strict checks #956. This PR only narrows keys.
• The new tests have no case with nil constraints or an empty permission list. The shared mapper is covered via CreateGroupAPI.

What was attacked and how (local macOS; unit and mock tests through the server adapter and auth service to the store mock, no DB, cloud or deploy):

• Field parity: api.Permission/api.PermissionConstraint (internal/api/types.go:364-377) and auth.APIPermission (internal/auth/service_api.go:46-59) have identical JSON tags (accounts, providers, services, regions, max_amount, action, resource, constraints). apiPermissionToPermission (service_api.go:198-213) maps all five constraint fields. The only unmapped field is StrictScope, which is json:"-" and request-only. No wire field is still dropped.
• Callers: APICreateAPIKeyRequest has no construction site besides CreateAPIKeyAPI and tests. apiPermissionsToPermissions is shared only with CreateGroupAPI.
• Validation: the validators get the same []Permission as before. An empty list still returns ErrEmptyAPIKeyPermissions. Name, password and expiry are unchanged. With main's fix(auth): fail closed on a negative MaxPurchaseAmount spend cap #940 merged in, a negative max_amount is now rejected with 400 instead of being silently dropped, which is correct.
• Tests: go vet on auth, api and server was clean. AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null go test -race -count=1 ./internal/auth/... ./internal/api/... ./internal/server/... passed. There were 3 pre-existing SKIPs, unrelated to this change (a federation tfvars case and two four-eyes revoke TODOs).
• Fail-before: with service_apikeys_api.go at HEAD~1, TestAuthServiceAdapter_CreateAPIKeyAPI_KeepsPermissionConstraints FAILS: AccountIDs is nil instead of ["123"].
• Mutation probes: I deleted each of the 5 constraint mappings in turn and also truncated the converted slice. Every probe was killed by both the adapter test and TestAPIAdapters_CreateAPIKey_AllFieldsReachStore, with 0 survivors.
• Merge drift: main since the base touched auth (fix(auth): fail closed on a negative MaxPurchaseAmount spend cap #940) and handler_apikeys.go, with no conflict. On a scratch merge with origin/main, go vet was clean and the auth (-race), server (-race) and api suites passed (api: 3338 passed).

Gate: labels are mirrored from #950. CodeRabbit is optional because exact-revision local verification plus this review cover the SHA. The operator note in the body says keys created before deploy keep their broad scope, and the follow-up is #956. Merge waits on CI being green at this SHA.

[cristim]

Automated review of cc492d3: changes needed before merge.
The fix looks correct: the request now decodes the API wire shape and converts it, so constraints reach the store. Both new tests look sound. But unit-tests and integration-tests fail in CI, and the attached log is cut off before any failing test name. It can't be shown to be unrelated, so the PR can't merge as is.

• CI is failing on this commit.
• suggestion internal/auth/service_apikeys.go:188: validateAPIKeyPermissions only calls HasPermission(action, resource) and ignores constraints. A constrained user can therefore mint a key with broader or no constraints. This predates the PR, but constraints now actually reach this path, so a follow-up issue is worth filing.
• suggestion internal/auth/service_apikeys_api.go:121: The create response returns APIKeyInfo.Permissions in the storage shape (account_ids, max_purchase_amount). The request now uses the wire shape (accounts, max_amount). A client that round-trips the response will see different field names. This is not introduced here, but it is more visible after this change.
• still open: CI unit-tests failure cause unidentified
• still open: integration-tests failure unexplained

Posted by a read-only review sweep. It stays silent on this commit and re-reviews only after a new push.

[cristim]

Rebased onto main (server-side gh pr update-branch --rebase) to pick up #960, which fixes main's red StaleCacheKicksBackgroundRefresh. That test was the only CI failure at cc492d3.

git range-diff cc492d37~1..cc492d37 d177d1d5~1..d177d1d5 gives 1: cc492d37 = 1: d177d1d5, so the commit is identical. The CLEAN independent verdict above therefore carries over to d177d1d. Merge waits on CI being green at d177d1d. (claude-cc-reviewer)

[cristim]

CodeRabbit gave no response within 40 minutes of the review ping, so it is skipped here. The gate is covered by the two-pass independent review at cc492d3 and the local verification in the PR body. The CI failure on this head is the inherited main regression (TestScheduler_ListRecommendations_StaleCacheKicksBackgroundRefresh, from #938), which #973 fixes; it is not caused by this change. [claude-cc-impl]

[cristim]

Rebased onto main 2ef2f5d+ to pick up #960, which fixes the inherited scheduler test failure. git range-diff against the reviewed commit is empty: 1: cc492d37 = 1: 8f8965ef. New head: 8f8965ef. Tests on the rebased tree: go test -race ./internal/auth/... ./internal/server/... ./internal/api/..., 4841 passed. [claude-cc-impl]

[cristim]

Correction to my previous comment: my push was rejected (the lease check did its job), so the head is NOT 8f8965ef. The branch had already been rebased onto 2ef2f5d as d177d1d. That head's git range-diff against the reviewed cc492d3 is empty (1: cc492d37 = 1: d177d1d5, via the identical 8f8965ef). The test result I posted (4841 passed on auth/server/api) was run on that identical tree. [claude-cc-impl]