fix(api/auth): report a user API key's effective permissions on /me/permissions

[IAmJSD]

Root cause

GET /api/auth/me/permissions (getCurrentUserPermissions, internal/api/handler_auth.go) resolved every caller to a user ID through resolveAuthenticatedUserID, then called GetUserPermissionsAPI(userID). For a user API key, that ID belongs to the key's owner, so the endpoint returned the owner's full group permissions and an is_admin value based on the owner. A view:recommendations-only key owned by an administrator was told it held {admin, *} with is_admin: true. Enforcement (requirePermission -> HasAPIKeyPermissionAPI -> ComputeEffectivePermissions) already used the key's effective set, so the endpoint advertised permissions the key could not use. I re-checked this on current main (110a4968) before changing anything: no commit or merged PR references #62, and the handler still had the owner-permissions code.

Fix

• auth.Service.GetAPIKeyEffectivePermissionsAPI(ctx, keyID, userID) (new, internal/auth/service_apikeys_api.go): loads the key by ID and re-checks it: the owner must match, the key must not be revoked or expired, the owner must be active, and the owner's password version must match. It then returns ComputeEffectivePermissions(key, owner), the same intersection HasAPIKeyPermissionAPI enforces. Each entry keeps the key's constraints, not the owner's. Any failure returns an error.
• Handler: when the authenticated principal is a user API key, the handler uses the principal's APIKeyID/UserID, which principalFromUserAPIKey sets on every routed request, and returns the key's effective set. is_admin is derived from that set. A key sent together with a bearer session is reported as the key, because that is how requirePermission evaluates it. For direct (unrouted) calls, if a key validates but no usable key record comes back, the request is refused rather than described with the owner's permissions.
• Fail closed: a lookup error returns 500 and never falls back to the owner's permissions.
• AuthServiceInterface gains the method, and the server adapter and test mocks are wired. The response shape is unchanged.

Before / after (GET /api/auth/me/permissions)

Caller	Before	After
Admin owner's key scoped to view:recommendations	[{admin,*}], is_admin: true	[{view,recommendations}], is_admin: false
Same key + owner's bearer token in one request	owner's set	key's set (matches enforcement)
Key {execute:purchases (max $100, aws), delete:users}; owner lacks delete:users but has more	the owner's whole set	[{execute,purchases}]
Key scoped to {admin,*} on an admin owner	[{admin,*}], true	[{admin,*}], true (follows the effective set)
Unscoped legacy key	owner's set	[], false (deny-by-default, #61)
Key revoked mid-request / store error	owner's set (200)	500, no permissions
Owner bearer session	owner's set	unchanged
Admin API key	[{admin,*}], true	unchanged

Frontend impact

None expected; frontend/ is untouched. The SPA authenticates with a bearer session, and that path is unchanged. It also calls GET /api/auth/me first, which rejects API-key-only callers (see out of scope), so the SPA never shows UI based on a key's permissions today. If an API-key client starts reading this endpoint, it now gets the key's real surface. That fixes the issue's CLI/integration case, where clients would otherwise try admin endpoints and collect a wave of 403s.

Verification (local, macOS, at c6594dfb)

• New internal/api/handler_auth_me_permissions_apikey_test.go drives HandleRequest (middleware, router, handler) into a real auth.Service backed by auth.MockStore. It covers: an admin owner with a view-only key (TestGetCurrentUserPermissions_UserAPIKey_ReturnsEffectiveScope), a key plus the owner's session, the owner's session alone (unchanged), the admin API key (unchanged), an admin-scoped key, a constrained key, and a lookup error or revocation mid-request (refused). For each key case it also asserts that every reported entry is allowed by HasAPIKeyPermissionAPI and that is_admin matches what enforcement grants for admin:*, so the reported and enforced sets cannot drift apart (sec(auth): unscoped user API key silently inherits owner's full permissions #61/fix(api/auth): /me/permissions returns owner's full perms instead of API key's effective scope #62).
• New internal/auth/service_apikeys_effective_test.go checks the intersection, that surviving entries carry the key's constraints (max $100 and aws survive, not the owner's $1000), that an unscoped key gets an empty set, and that 8 failure cases fail closed (key lookup error, key not found, owner mismatch, revoked, expired, inactive owner, password rotated, group lookup error).
• Three existing tests that asserted the old owner-permissions behavior were updated (TestHandler_getCurrentUserPermissions_UserAPIKey, TestRouterAuthUser_UserAPIKeyPrincipalAvoidsSecondValidation, TestHandleRequest_UserAPIKeyPrincipalValidatedOnce), and one was added (..._UserAPIKeyWithoutKeyRecordRefuses).
• Fail-before / pass-after: with the original handler_auth.go, 4 of the 7 new HandleRequest tests fail. The returned values were [{admin,*}]/true instead of the key's set, the owner's three permissions instead of one, and 200 instead of 500. The 3 unchanged-path tests pass both before and after. All pass with the fix.
• Mutation probes, all killed: principal never treated as a key (6 failing tests); fall back to owner permissions on error (3); is_admin forced false (3); nil key record falls through to the owner path (1); owner-match check dropped (2); key status check dropped (3); password-version check dropped (2); service returns the owner's permissions (3).
• go build ./..., go vet ./..., gofmt -l clean.
• go test ./internal/api/... ./internal/auth/... ./internal/server/... ./internal/mocks/... -count=1 passes, run one package at a time with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null. This includes the TestNoUnfailableMockAssertions guard.
• golangci-lint v2.10.1 (--allow-parallel-runners) on ./internal/api/... ./internal/auth/... ./internal/server/...: 0 issues.
• gocyclo -over 10 on the changed non-test files (handler_auth.go, types.go, service_apikeys_api.go, app.go): no output.
• git-secrets: changed files scanned with --scan --cached in a fresh git init repo with an isolated HOME and the repo's setup-git-secrets.sh + .gitallowed: clean. bash scripts/test-git-secrets-allowlist.sh: 42 PASS, 0 FAIL.
• openapi.yaml: no change. /api/auth/me/permissions is not documented there (audit finding listed it as an undocumented route), and the response shape did not change.
• Not verified against a deployed environment or a real database. The store is mocked, but the evidence comes from the production auth.Service code.

Out of scope

• GET /api/auth/me (audit A02-024, appended to fix(api/auth): /me/permissions returns owner's full perms instead of API key's effective scope #62): this handler has the opposite problem. It uses requireSessionPrincipal, so a caller with only a user API key gets 401. When a key and a bearer token are sent together, it returns the owner's id, email and groups, and the group list reveals the owner's privileges (for example Administrators). Fixing it means deciding what /me should return for a key: whether to show the owner's groups and how to handle the admin API key, which has no user row. That is a design decision, not a trivial change in this handler family, so it is left out. Note: closing fix(api/auth): /me/permissions returns owner's full perms instead of API key's effective scope #62 will close the issue together with this appended finding; a maintainer may want to split A02-024 into its own issue.
• Constraints on the wire: PermissionEntry carries only action/resource for every caller. The key's constraints are computed and returned by the new service method, but not serialized, because adding them would also change the bearer-session response.
• Account scoping (GetAllowedAccountsAPI) stays per-owner. Scoping by a key's AccountIDs constraint applies only where constraint checks run. This PR does not touch it, and there is no /me endpoint for allowed accounts.
• internal/api/handler_auth.go was already over the 500-line guideline before this change (668 lines; now 732).

Closes #62

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

• Run on-demand review

_{This review includes 10 billable files and costs up to $2.50.}

Or wait 38 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

• Configuration used: Repository: LeanerCloud/cloud-commitments-platform/.coderabbit.yaml
• Review profile: CHILL
• Plan: Advanced
• Run ID: 579ec760-762b-4c1b-8c9d-85a63c844220

📥 Commits

Reviewing files that changed from the base of the PR and between 110a496 and c6594df.

📒 Files selected for processing: 10

• internal/api/handler_auth.go
• internal/api/handler_auth_me_permissions_apikey_test.go
• internal/api/handler_auth_test.go
• internal/api/handler_ri_exchange_test.go
• internal/api/mocks_test.go
• internal/api/router_authuser_test.go
• internal/api/types.go
• internal/auth/service_apikeys_api.go
• internal/auth/service_apikeys_effective_test.go
• internal/server/app.go

• 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 of c6594df: draft: blocks ready

No code defects found in the auth logic. The blocks are gate items only (CI not finished, closing link not machine-readable).

Findings

1. BLOCKING (gate, not code): closingIssuesReferences is empty. The only Closes #62 in the body sits inside backticks in the "Out of scope" note, which GitHub does not parse as a closing keyword. Put an unquoted Closes #62 on its own line. Issue #62 exists, is OPEN, and its labels (priority/p2, severity/medium, triaged, urgency/this-quarter, impact/few, effort/s, type/bug) match the PR's exactly.
2. NON-BLOCKING, internal/auth/service_apikeys_api.go:462-491: GetAPIKeyEffectivePermissionsAPI repeats the post-hash checks of ValidateUserAPIKey (service_apikeys.go:379-410: validateAPIKeyStatus, lookupAPIKeyUser, password-version compare). Today the two agree line by line, and both end in the same ComputeEffectivePermissions. A later change to one validation list could drift from the other. Suggested follow-up: extract a validateLoadedAPIKey(ctx, key) used by both. Not required for this PR.
3. NON-BLOCKING, internal/api/handler_auth.go:208-216: a key revoked or rotated between authentication and this call returns 500 (generic error mapped at handler.go:846). Enforcement (authorizeAPIKey, handler.go:269-275) treats the same case as "fall through" to 401. Fail-closed either way, nothing leaks; 401 would be the more accurate status.
4. NON-BLOCKING, size: 592 additions (about 128 production, about 464 tests and mock wiring). Over the 400 guideline but mostly tests. handler_auth.go goes from 668 to 732 lines (already over 500 on main).

Reporting vs enforcement (line by line)

• Enforcement: HasAPIKeyPermissionAPI (service_apikeys_api.go:383-401) = ValidateUserAPIKey then ComputeEffectivePermissions then AuthContext.HasPermission over that set.
• Reporting: same ComputeEffectivePermissions (service_apikeys.go:581, via computeEffectivePermissionsFromAuthCtx, which keeps a key entry only if the owner's HasPermission holds it), preceded by the same status, owner-active and password-version checks, plus an owner-id match. The reported set can therefore never exceed what HasPermission grants on the enforced set.
• Admin cases checked: key {admin,*} on an admin owner reports admin (enforcement grants everything except the carved-out money verbs, same as a session admin). Key {execute,purchases} on an admin-only owner is dropped in both paths (carve-out, types.go:174-190). Unscoped legacy key gives an empty list in both paths. Key {view,*} on an owner with only {view,recommendations} is dropped in both.
• Caller selection mirrors requirePermission (handler.go:250-288): a user-key principal wins over a bearer session, an admin key short-circuits first, and an unrouted call with an invalid key falls through to the session. The key ID and owner come from the validated principal (middleware.go:132-168, which rejects an empty key ID), and the service re-loads the key by ID and requires key.UserID == userID, so no cross-user or cross-tenant key can be named by a caller.
• Constraints are returned by the service but not serialized (PermissionEntry has only action and resource), so the wire cannot overstate a limit. Disabled owner, revoked or expired key, rotated password and group-lookup failure all return an error; no path falls back to the owner's set.

What I attacked

• Built and ran go test ./internal/api ./internal/auth ./internal/server at this SHA: all pass; go vet clean.
• Fail-before: restored the base handler_auth.go with the new service method kept: 5 tests fail (..._UserAPIKey_ReturnsEffectiveScope, ..._UserAPIKeyWithOwnerSession_ReportsKey, ..._ConstrainedKey_ReportsKeyEntries, ..._UserAPIKeyLookupFailureRefuses, TestHandler_getCurrentUserPermissions_UserAPIKey).
• Mutation probes in GetAPIKeyEffectivePermissionsAPI, each reverted at once (worktree clean afterwards): return the key's permissions without the owner intersection (killed by ..._KeyConstraintsAndIntersection, ..._FailsClosed, and the constrained-key API test); drop validateAPIKeyStatus (killed); drop the password-version check (killed); drop the owner-id match (killed); swap lookupAPIKeyUser for a raw GetUserByID, so an inactive owner passes (killed). 6 of 6 killed.
• Tried to find a way to report more than is enforced (wildcards, carve-outs, session plus key, admin key, nil permissions, owner without groups, another user's key ID): none survived.

Gate status (origin/main Review gate)

• Independent adversarial review of the full diff naming the SHA: this comment.
• Actionable findings resolved: none open from me. CodeRabbit skipped the draft ("Review skipped: draft pull request"); I did not ping it. No CR threads exist.
• CI on the SHA: not green yet. The pre-commit check passed; "CI - Build & Test" is queued and about 20 checks (Unit Tests, Lint Code, Integration Tests, E2E, Security Scanning, Terraform validations) are pending. No action_required fork runs were seen at review time. BLOCKS READY until all are green.
• Local verification on macOS: real auth.Service over auth.MockStore through HandleRequest; fail-before and mutation evidence above. No real-account verification (none needed for this read-only endpoint).
• Labels: match issue #62 exactly; the closing keyword needs fixing (finding 1).
• Verdict, SHA and evidence recorded here.

Blocks ready

1. CI green on c6594df (queued or pending at review time).
2. Unquoted Closes #62 so GitHub links the issue.
3. Take the PR out of draft; any new commit or rebase restarts this gate.

[cristim]

Gate record for c6594df (origin/main CLAUDE.md "Review gate"):

• Independent review (fresh cold-brief agent, non-author) of c6594df: no code defects (fix(api/auth): report a user API key's effective permissions on /me/permissions #1026 (review)). Its two "blocks ready" items were gate items and are now met: Closes #62 is machine-readable (closingIssuesReferences = fix(api/auth): /me/permissions returns owner's full perms instead of API key's effective scope #62) and CI is green. Non-blocking notes (shared post-hash validation helper, 401 vs 500 for a key revoked mid-request, size mostly tests) are follow-ups.
• CI on c6594df: latest CI - Build & Test and pre-commit runs (refreshed 2026-10-17T04:01Z against current main) completed/success; CI Success and Run pre-commit hooks pass.
• Local verification (by the reviewer): real auth.Service over auth.MockStore through HandleRequest; fail-before with the base handler fails 5 tests; go test ./internal/api ./internal/auth ./internal/server pass. Read-only endpoint, no real-account run.
• CR skipped: quota-limited; covered by the independent review plus local verification.
• Labels mirrored from fix(api/auth): /me/permissions returns owner's full perms instead of API key's effective scope #62.
• Handed over from claude-cc-pr-merge-4 (its session could not merge).

Merging at c6594df with --match-head-commit.