Add ap:api_key:all:manage scope for cross-user API key administration - #2911
Conversation
…forbidden access in Update and Revoke methods. Introduce new error codes for API key not found and forbidden access in the error catalog.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAPI key management now supports an explicit organization-wide manage scope, centralized creator authorization, trusted event origins, expiry validation, cataloged lookup errors, updated repository filtering, and removal of built-in file-auth admin credentials. ChangesAPI key authorization
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant APIKeyHandler
participant APIKeyService
participant APIKeyRepo
Client->>APIKeyHandler: API key operation with effective scope
APIKeyHandler->>APIKeyService: pass keyAdmin and trustedOrigin
APIKeyService->>APIKeyRepo: load key and enforce ownership
APIKeyRepo-->>APIKeyService: key record or lookup error
APIKeyService-->>APIKeyHandler: result or catalog error
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@platform-api/internal/service/apikey.go`:
- Around line 565-567: The lookup-failure logs in the API-key update flow must
not expose raw resource identifiers. Update the Error calls around
existingKeyErr and the corresponding failure at the second referenced location
to omit, redact, or replace apiHandle and keyName with an approved keyed-hash
correlation value, while preserving the existing error handling and returned
errors.
- Around line 520-521: Update the creator authorization checks in the API-key
update and revoke flows to reject empty userId values before comparing
ownership; require a non-empty identity that matches the key creator. Apply the
same fail-closed logic to both updateKey and revokeKey, preserving creator-only
access and avoiding implicit authorization for system callers.
- Around line 564-573: Update APIKeyRepo.Update and APIKeyRepo.Revoke to include
the expected creator/owner in each SQL WHERE clause alongside artifact_uuid and
handle, preventing delete/recreate races from mutating another key. Check the
affected-row count and return the existing “api key not found” error when no
rows are affected; preserve normal success behavior otherwise.
- Around line 564-570: Update UpdateAPIKey and RevokeAPIKey to resolve apiId,
then immediately load the API key and enforce ownership before gateway lookup,
broadcasting, hashing, or other gateway operations. Preserve request-derived
input validation at that point, and translate sql.ErrNoRows from
GetByArtifactAndName into RESTAPIKeyNotFound while retaining existing error
handling for other repository failures.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c4343b45-b16a-41a3-93d4-38a830fc0cc5
📒 Files selected for processing (3)
platform-api/internal/apperror/catalog.goplatform-api/internal/apperror/codes.goplatform-api/internal/service/apikey.go
Thushani-Jayasekera
changed the title
ap:api_key:all:manage scope for cross-user API key administration
…ith the `ap:api_key:all:manage` scope to manage any API key
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
platform-api/resources/openapi.yaml (1)
4407-4418: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winDocument the organization-wide
/me/api-keysresponse.Callers with
ap:api_key:all:manageare intended to receive every user’s keys in the organization, but the summary and description still say “current user” and “caller.” Update the contract to describe the conditional organization-wide behavior.🤖 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 `@platform-api/resources/openapi.yaml` around lines 4407 - 4418, Update the /me/api-keys GET contract identified by operationId listUserAPIKeys so its summary and description document that ap:api_key:all:manage callers receive all users’ API keys in the organization, while retaining the current-user behavior for callers without that scope.platform-api/internal/service/llm_apikey.go (1)
155-168: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMake API key deletion authorization atomic.
GetByArtifactAndName(...)authorizes delete access, butAPIKeyRepo.Delete(...)only deletesapi_keys WHERE artifact_uuid = ? AND handle = ?. Under theUNIQUE(artifact_uuid, handle)index, an authorized request can revoke/delete a replacement key if this key is removed and recreated before the delete. Use a conditional delete keyed by the original row identity/version/creator, or re-check the authorizing key ownership inside the deletion transaction.🤖 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 `@platform-api/internal/service/llm_apikey.go` around lines 155 - 168, The deletion flow around GetByArtifactAndName and apiKeyRepo.Delete must bind authorization to the originally fetched key so a replacement cannot be deleted between lookup and deletion. Update the repository/API to perform a conditional, transactional delete using the original key’s stable identity, version, or creator fields, and ensure the service treats a failed condition as no longer authorized/not found.platform-api/internal/service/llm_proxy_apikey.go (1)
133-137: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftRe-verify the creator in the delete statement.
Authorization protects the fetched key, but
apiKeyRepo.Delete(proxy.UUID, keyName)can delete a same-name replacement created after that check. ExtendDeletewith the expectedCreatedByand constrain itsWHEREclause accordingly.🤖 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 `@platform-api/internal/service/llm_proxy_apikey.go` around lines 133 - 137, Extend apiKeyRepo.Delete to accept the authorized key’s expected CreatedBy value, pass existingKey.CreatedBy from the delete flow after canManageAPIKey succeeds, and add CreatedBy to the delete statement’s WHERE constraints so a same-name replacement cannot be removed.Source: Coding guidelines
platform-api/internal/service/application.go (1)
783-795: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winFail closed for missing binding identities.
When
keyAdminis false, Lines 790-792 still return success for an empty requester, allowing an unidentified caller to bind another user’s key. UsecanManageAPIKey(after trimming if needed) and returnApplicationAPIKeyForbiddenon any non-admin missing/mismatched identity.As per coding guidelines, creator-scoped resource access must fail closed for missing identities; cross-user access requires an explicit administrative scope.
🤖 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 `@platform-api/internal/service/application.go` around lines 783 - 795, Update the non-admin identity check in the API-key authorization flow around canManageAPIKey to fail closed: after trimming identities as needed, return ApplicationAPIKeyForbidden whenever the creator or requester is missing or they do not match. Preserve the keyAdmin bypass, and use canManageAPIKey rather than allowing empty identities to return success.Source: Coding guidelines
🧹 Nitpick comments (2)
platform-api/internal/service/apikey_user.go (1)
50-65: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftOrganization-wide listing should not paginate only after full materialization. Both admin paths now widen filtering to all keys but still load and identity-resolve the complete result set before slicing the requested page.
platform-api/internal/service/apikey_user.go#L50-L65: add repository-level pagination and total counting for organization-wide/me/api-keyslistings.platform-api/internal/service/llm_apikey.go#L83-L97: apply repository-level filtering, pagination, and counting for provider listings.🤖 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 `@platform-api/internal/service/apikey_user.go` around lines 50 - 65, Update APIKeyUserService.ListAPIKeysByUser in platform-api/internal/service/apikey_user.go (lines 50-65) to use repository-level filtering, pagination, and total counting for organization-wide listings, avoiding full result materialization and identity resolution before slicing. Apply the same repository-level filtering, pagination, and counting change to the provider listing flow in platform-api/internal/service/llm_apikey.go (lines 83-97), preserving the requested page and total-count response behavior.platform-api/internal/repository/apikey_test.go (1)
141-141: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd mismatch coverage for creator re-verification.
Add update and revoke cases with a different
createdBy, asserting an error and that the original row remains unchanged. The current test covers only the matching-owner path.🤖 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 `@platform-api/internal/repository/apikey_test.go` at line 141, Add update and revoke test cases in the existing repository tests around repo.Revoke, using a createdBy value different from the artifact’s original creator. Assert both operations return an error and verify the original API key row remains unchanged after each attempt.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 @.claude/rules/authentication_authorization.md:
- Around line 1502-1515: Update RevokeAPIKey to use the existing handle
parameter consistently wherever the repository calls currently pass the
undefined artifactID, including GetByArtifactAndName and Revoke. Keep the
function signature and authorization behavior unchanged.
In `@platform-api/config/config.toml`:
- Line 39: Remove the functional default admin credential before granting
ap:api_key:all:manage: in platform-api/config/config.toml at lines 39-39,
eliminate the default username/password-hash fallback; in
platform-api/config/default_config.go at lines 96-96, remove the hard-coded
default user/hash and make startup fail closed when no operator-provided
bootstrap user exists, or generate and persist a random bootstrap credential.
In `@platform-api/internal/webhook/handlers_apikey.go`:
- Around line 229-237: Replace the empty actor plus keyAdmin=true authorization
in the update path at platform-api/internal/webhook/handlers_apikey.go:229-237
with an explicit authorizedUpstream context propagated through the shared
API-key service, restricted to the verified webhook path. Apply the same
explicit trusted-upstream authorization mechanism to the revoke path at
platform-api/internal/webhook/handlers_apikey.go:252-254, preserving fail-closed
creator checks for ordinary requests.
In `@platform-api/plugins/eventgateway/handler/webbroker_apikey.go`:
- Line 193: Map RESTAPIKeyForbidden to the catalog 403 response before the
generic error handling in the UpdateAPIKey error branches at
platform-api/plugins/eventgateway/handler/webbroker_apikey.go lines 193-193 and
240-240, and platform-api/plugins/eventgateway/handler/websub_apikey.go lines
193-193 and 241-241; preserve the existing generic handling for other errors.
---
Outside diff comments:
In `@platform-api/internal/service/application.go`:
- Around line 783-795: Update the non-admin identity check in the API-key
authorization flow around canManageAPIKey to fail closed: after trimming
identities as needed, return ApplicationAPIKeyForbidden whenever the creator or
requester is missing or they do not match. Preserve the keyAdmin bypass, and use
canManageAPIKey rather than allowing empty identities to return success.
In `@platform-api/internal/service/llm_apikey.go`:
- Around line 155-168: The deletion flow around GetByArtifactAndName and
apiKeyRepo.Delete must bind authorization to the originally fetched key so a
replacement cannot be deleted between lookup and deletion. Update the
repository/API to perform a conditional, transactional delete using the original
key’s stable identity, version, or creator fields, and ensure the service treats
a failed condition as no longer authorized/not found.
In `@platform-api/internal/service/llm_proxy_apikey.go`:
- Around line 133-137: Extend apiKeyRepo.Delete to accept the authorized key’s
expected CreatedBy value, pass existingKey.CreatedBy from the delete flow after
canManageAPIKey succeeds, and add CreatedBy to the delete statement’s WHERE
constraints so a same-name replacement cannot be removed.
In `@platform-api/resources/openapi.yaml`:
- Around line 4407-4418: Update the /me/api-keys GET contract identified by
operationId listUserAPIKeys so its summary and description document that
ap:api_key:all:manage callers receive all users’ API keys in the organization,
while retaining the current-user behavior for callers without that scope.
---
Nitpick comments:
In `@platform-api/internal/repository/apikey_test.go`:
- Line 141: Add update and revoke test cases in the existing repository tests
around repo.Revoke, using a createdBy value different from the artifact’s
original creator. Assert both operations return an error and verify the original
API key row remains unchanged after each attempt.
In `@platform-api/internal/service/apikey_user.go`:
- Around line 50-65: Update APIKeyUserService.ListAPIKeysByUser in
platform-api/internal/service/apikey_user.go (lines 50-65) to use
repository-level filtering, pagination, and total counting for organization-wide
listings, avoiding full result materialization and identity resolution before
slicing. Apply the same repository-level filtering, pagination, and counting
change to the provider listing flow in
platform-api/internal/service/llm_apikey.go (lines 83-97), preserving the
requested page and total-count response behavior.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b71bbe6e-3906-4ea4-a41b-43cb8d37e757
📒 Files selected for processing (36)
.claude/rules/authentication_authorization.mdgateway/gateway-controller/pkg/utils/api_key.gokubernetes/helm/platform-api-helm-chart/values.yamlplatform-api/config/config-template.tomlplatform-api/config/config.tomlplatform-api/config/default_config.goplatform-api/internal/constants/constants.goplatform-api/internal/handler/api_key.goplatform-api/internal/handler/apikey_user.goplatform-api/internal/handler/application.goplatform-api/internal/handler/llm_apikey.goplatform-api/internal/handler/llm_proxy_apikey.goplatform-api/internal/integration/audit_identity_it_test.goplatform-api/internal/integration/crud_test.goplatform-api/internal/repository/apikey.goplatform-api/internal/repository/apikey_test.goplatform-api/internal/repository/interfaces.goplatform-api/internal/server/server.goplatform-api/internal/service/apikey.goplatform-api/internal/service/apikey_authz_test.goplatform-api/internal/service/apikey_user.goplatform-api/internal/service/application.goplatform-api/internal/service/application_test.goplatform-api/internal/service/llm_apikey.goplatform-api/internal/service/llm_proxy_apikey.goplatform-api/internal/webhook/handlers_apikey.goplatform-api/internal/webhook/receiver.goplatform-api/plugins/eventgateway/handler/webbroker_apikey.goplatform-api/plugins/eventgateway/handler/websub_apikey.goplatform-api/plugins/eventgateway/openapi.yamlplatform-api/plugins/eventgateway/plugin.goplatform-api/resources/openapi.yamlplatform-api/resources/roles.yamltests/ai-workspace-cli-e2e/platform-api-config.tomltests/integration-e2e/platform-api-config.tomltests/integration-e2e/suite_test.go
|
@CodeRabbit review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@platform-api/config/config_test.go`:
- Around line 510-513: Replace the placeholder APIP_CP_ADMIN_PASSWORD_HASH value
in the successful LoadConfig fixture with a known-valid bcrypt digest, while
keeping the existing generated-admin username and successful configuration flow
unchanged.
- Around line 504-505: Update the test setup around the credential environment
variables and the related t.Setenv calls so original values are captured before
any os.Unsetenv mutation, then restored after the test using t.Setenv or
equivalent cleanup. Ensure APIP_CP_ADMIN_USERNAME and
APIP_CP_ADMIN_PASSWORD_HASH retain their pre-test values, including when they
were initially set.
In `@platform-api/README.md`:
- Around line 31-32: Update the README instructions around the htpasswd command
to remove the password positional argument and batch mode, and direct users to
generate the hash interactively so the admin password is not exposed in shell
history, process listings, or CI logs.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d051dcc4-c818-4cb8-9a98-3d8fe2e53bbb
📒 Files selected for processing (20)
.claude/rules/authentication_authorization.mdkubernetes/helm/platform-api-helm-chart/values.yamlplatform-api/README.mdplatform-api/config/config-template.tomlplatform-api/config/config.tomlplatform-api/config/config_test.goplatform-api/config/default_config.goplatform-api/internal/repository/apikey.goplatform-api/internal/repository/apikey_test.goplatform-api/internal/repository/interfaces.goplatform-api/internal/server/server.goplatform-api/internal/service/application.goplatform-api/internal/service/llm_apikey.goplatform-api/internal/service/llm_proxy_apikey.goplatform-api/internal/webhook/receiver.goplatform-api/plugins/eventgateway/handler/webbroker_apikey.goplatform-api/plugins/eventgateway/handler/websub_apikey.goplatform-api/resources/openapi.yamltests/integration-e2e/platform-api-config.tomltests/integration-e2e/suite_test.go
🚧 Files skipped from review as they are similar to previous changes (12)
- platform-api/config/config-template.toml
- kubernetes/helm/platform-api-helm-chart/values.yaml
- tests/integration-e2e/platform-api-config.toml
- .claude/rules/authentication_authorization.md
- platform-api/plugins/eventgateway/handler/webbroker_apikey.go
- platform-api/internal/service/llm_proxy_apikey.go
- platform-api/plugins/eventgateway/handler/websub_apikey.go
- platform-api/internal/server/server.go
- platform-api/internal/webhook/receiver.go
- tests/integration-e2e/suite_test.go
- platform-api/internal/service/llm_apikey.go
- platform-api/resources/openapi.yaml
…ameter to Update and Revoke methods, ensuring proper authorization checks for directly-authenticated callers.
… password generation and ensure environment variables are restored in tests.
| CodeRESTAPIKeyNotFound = "REST_API_KEY_NOT_FOUND" | ||
| CodeRESTAPIKeyForbidden = "REST_API_KEY_FORBIDDEN" |
There was a problem hiding this comment.
| CodeRESTAPIKeyNotFound = "REST_API_KEY_NOT_FOUND" | |
| CodeRESTAPIKeyForbidden = "REST_API_KEY_FORBIDDEN" | |
| CodeRESTAPIKeyNotFound = "REST_API_API_KEY_NOT_FOUND" | |
| CodeRESTAPIKeyForbidden = "REST_API_API_KEY_FORBIDDEN" |
| // before it was broadcast, so the gateway must not re-run its own creator check. | ||
| // Set only on the platform-api event path (see RevokeExternalAPIKeyFromEvent); it is | ||
| // always false for requests arriving on the gateway's own REST API. | ||
| AuthorizedUpstream bool |
There was a problem hiding this comment.
What does "upstream" mean? Do you mean AuthorizedOrigin? (origin of the call) (or TrustedOrigin - bit shorter)
There was a problem hiding this comment.
yes, a flag to identify if the request is from a trusted / pre validated origin. Will update the parameter name to TrustedOrigin as AuthorizedUpstream gives incorrect meaning.
| response, err := h.apiKeyUserService.ListAPIKeysByUser(r.Context(), orgID, callerUserID, types, limit, offset) | ||
| // Holding constants.ScopeAPIKeyAllManage widens this listing from the caller's own keys | ||
| // to every user's keys in the organization. | ||
| keyAdmin := middleware.HasEffectiveScope(r, h.authzMode, constants.ScopeAPIKeyAllManage) |
There was a problem hiding this comment.
This name is bit confusing. "key" is used to indicate a "field". So this can be misread as a generic admin check and can be reused in unwanted places. Shall we be explicit here? Fix other places too.
hasApiKeyAdmin := ..
| // CreatedBy is required so the repository can re-verify the creator in its WHERE | ||
| // clause, guarding against a delete/recreate race between the check above and this | ||
| // write (see APIKeyRepo.Update). | ||
| CreatedBy: existingKey.CreatedBy, |
There was a problem hiding this comment.
Are we always passing the existing Key's CreatedBy? Does the created by check underneath the repository method ever fail in practice?
If not - its better to just drop this as this is confusing. You've already done the authz check earlier.
There was a problem hiding this comment.
Ah yeah, before reaching L629, we have this check
if !trustedOrigin && !canManageAPIKey(existingKey.CreatedBy, userId, keyAdmin) {
return apperror.RESTAPIAPIKeyForbidden.New()
}
created by was passed here as an additional guard when selecting the API Key record to update. But since the validation is already done before reaching the db update state we can remove.
|
|
||
| ## GO-AUTH-019: Ownership-Override Scopes Must Be Named and Service-Layer Enforced — High | ||
|
|
||
| Most resource CRUD here is *creator-scoped*: a caller may act only on rows they created (`api_keys.created_by`). Where cross-user administration is genuinely needed, express it as one explicitly named org-wide scope (`ap:api_key:all:manage`) resolved from verified claims at the handler, passed down as an explicit argument, and enforced by a single shared predicate at the service layer that every entry point funnels through (GO-AUTH-015). Never approximate it with an implicit signal — an empty caller id, or "this call came from an internal path so it must be trusted". An empty identity treated as elevated fails *open* for any path that merely forgot to populate the actor. The override widens which rows within the caller's own organization are reachable, never which organization (GO-AUTH-005). |
There was a problem hiding this comment.
"one explicitly named org-wide scope" - This should not mean that we should use one scope for everything. Scopes are always specific to resources. Nobody should ever create a scope called "ap:admin"
|
|
||
| ## GO-AUTH-019: Ownership-Override Scopes Must Be Named and Service-Layer Enforced — High | ||
|
|
||
| Most resource CRUD here is *creator-scoped*: a caller may act only on rows they created (`api_keys.created_by`). Where cross-user administration is genuinely needed, express it as one explicitly named org-wide scope (`ap:api_key:all:manage`) resolved from verified claims at the handler, passed down as an explicit argument, and enforced by a single shared predicate at the service layer that every entry point funnels through (GO-AUTH-015). Never approximate it with an implicit signal — an empty caller id, or "this call came from an internal path so it must be trusted". An empty identity treated as elevated fails *open* for any path that merely forgot to populate the actor. The override widens which rows within the caller's own organization are reachable, never which organization (GO-AUTH-005). |
There was a problem hiding this comment.
Most resource CRUD here is creator-scoped -
What does "here" mean? Under this topic or platform? Shall we be specific?
In APIP, most CRUD resources are NOT creator-scoped
There was a problem hiding this comment.
Updated Rule
GO-AUTH-019: Ownership-Override Scopes Must Be Named and Service-Layer Enforced — High
Most resources in this repo are organization-scoped: any caller in the org with the right resource scope may act on them, and GO-AUTH-005 alone governs access. A few are additionally creator-scoped — a caller may act only on rows they created — API keys (api_keys.created_by, enforced by canManageAPIKey in platform-api/internal/service) being the canonical case. This rule applies to those creator-scoped resources; note that a created_by/CreatedBy column is usually just an audit field and does not by itself make a resource creator-scoped. Where cross-user administration of a creator-scoped resource is genuinely needed, express it as an explicitly named override scope for that one resource type (ap:api_key:all:manage overrides creator-scoping on API keys and nothing else) — resolved from verified claims at the handler, passed down as an explicit argument, and enforced by a single shared predicate at the service layer that every entry point funnels through (GO-AUTH-015). Never approximate it with an implicit signal — an empty caller id, or "this call came from an internal path so it must be trusted". An empty identity treated as elevated fails open for any path that merely forgot to populate the actor. The override widens which rows within the caller's own organization are reachable, never which organization (GO-AUTH-005).
"One named scope" means one per resource type, not one for the whole platform. Never define a catch-all override like ap:admin or ap:all:manage that a handler for any resource can accept: a scope is always bound to the resource it governs, so granting API-key administration cannot silently confer subscription or application administration. A caller who legitimately administers several resource types holds several scopes. Wildcards are not an exception to this, :* is own-level only — it covers the actions directly at its level and never descends into sub-resources, matches a prefix, or acts transitively (ap:gateway:* satisfies ap:gateway:create but not ap:gateway:token:create). scopeSatisfies in platform-api/internal/middleware/authorization.go implements exactly that expansion. Wildcards live on the grant side only: a handler or service-layer predicate always names the concrete scope it requires and never matches on a wildcard itself.
A scope decision does not survive a broadcast: an event carries the acting user's id, not their scopes, so a downstream created_by re-check rejects every legitimate admin-initiated change. Gate that check behind an explicit "trusted origin" flag (the call came from a pre-validated origin) set only by the event path — deleting it outright would also disable it for that component's own directly-authenticated API.
…stedOrigin`, enhancing clarity in authorization checks for event-driven operations.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
platform-api/internal/service/apikey.go (1)
163-185: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winGuard
expiresInarithmetic against overflow.
expiresIn.Durationis multiplied by unit constants without checking the product. A large request value can wrap to a different duration; a wrapped future value then passesvalidateExpiryInFuture, producing an incorrect expiration. Reject out-of-range values before multiplication and test the boundaries.🤖 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 `@platform-api/internal/service/apikey.go` around lines 163 - 185, Guard the expiration calculation in the API key creation flow around expiresIn and validateExpiryInFuture against duration overflow before multiplying by the unit-specific factor. Reject values whose converted duration would exceed the supported time.Duration range, including each unit’s boundary cases, while preserving the existing unsupported-unit validation and future-expiry checks.platform-api/internal/repository/apikey.go (1)
60-73: 🔒 Security & Privacy | 🟠 MajorBind mutations to the authorized key record.
Update,Revoke, andDeletematch only(artifact_uuid, handle)after a separate ownership lookup. If the key is deleted and recreated between those operations, the authorized request can mutate or delete the replacement key owned by another user. Pass an immutable key UUID/owner into each mutation predicate, or perform authorization and mutation atomically, and map zero affected rows to the catalog not-found error.This is the same ownership-predicate/TOCTOU issue reported previously; the current SQL still lacks the safeguard. As per coding guidelines, authorization invariants must fail closed across the shared service/data path.
Also applies to: 89-96, 199-203
🤖 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 `@platform-api/internal/repository/apikey.go` around lines 60 - 73, Bind the Update, Revoke, and Delete mutations in APIKeyRepo to the immutable authorized key UUID and owner, not only artifact_uuid and handle, so replacements cannot be modified after the ownership lookup. Extend each SQL predicate and arguments with those authorization fields, and map zero affected rows to the catalog not-found error while preserving the existing mutation behavior for authorized records.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.
Outside diff comments:
In `@platform-api/internal/repository/apikey.go`:
- Around line 60-73: Bind the Update, Revoke, and Delete mutations in APIKeyRepo
to the immutable authorized key UUID and owner, not only artifact_uuid and
handle, so replacements cannot be modified after the ownership lookup. Extend
each SQL predicate and arguments with those authorization fields, and map zero
affected rows to the catalog not-found error while preserving the existing
mutation behavior for authorized records.
In `@platform-api/internal/service/apikey.go`:
- Around line 163-185: Guard the expiration calculation in the API key creation
flow around expiresIn and validateExpiryInFuture against duration overflow
before multiplying by the unit-specific factor. Reject values whose converted
duration would exceed the supported time.Duration range, including each unit’s
boundary cases, while preserving the existing unsupported-unit validation and
future-expiry checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1a22472f-f4a8-404c-9c69-3fbbc1179657
📒 Files selected for processing (13)
.claude/rules/authentication_authorization.mdgateway/gateway-controller/pkg/utils/api_key.goplatform-api/internal/apperror/catalog.goplatform-api/internal/apperror/codes.goplatform-api/internal/repository/apikey.goplatform-api/internal/repository/interfaces.goplatform-api/internal/service/apikey.goplatform-api/internal/service/llm_apikey.goplatform-api/internal/service/llm_proxy_apikey.goplatform-api/internal/webhook/handlers_apikey.goplatform-api/internal/webhook/receiver.goplatform-api/plugins/eventgateway/handler/webbroker_apikey.goplatform-api/plugins/eventgateway/handler/websub_apikey.go
🚧 Files skipped from review as they are similar to previous changes (7)
- platform-api/internal/service/llm_proxy_apikey.go
- platform-api/plugins/eventgateway/handler/webbroker_apikey.go
- platform-api/internal/webhook/receiver.go
- .claude/rules/authentication_authorization.md
- platform-api/internal/webhook/handlers_apikey.go
- platform-api/internal/service/llm_apikey.go
- platform-api/plugins/eventgateway/handler/websub_apikey.go
… rules, emphasizing the distinction between organization-scoped and creator-scoped resources, and reinforcing the need for explicit override scopes per resource type.
…eter, streamlining the revoke function calls in integration tests.
3 participants
Summary
API key CRUD is creator-scoped: only the user recorded in
api_keys.created_bymay update orrevoke a key. There is currently no way for an administrator to manage a key created by someone
else — a problem when a user leaves the organization, or when an operator needs to rotate a
compromised key on their behalf.
This PR introduces
ap:api_key:all:manage, an organization-wide scope that lets its holderperform API key CRUD on any user's keys, alongside the existing creator-based access. Every other
caller keeps the creator-scoped default.
It also removes the gateway-controller's ownership re-check on the platform-api event path, which
cannot work for admin-initiated changes: an
apikey.updated/apikey.revokedevent carries onlythe acting user's id, never their scopes, so the gateway has no way to tell an org key admin apart
from an unauthorized third party and rejects every admin-initiated rotation.
Scope model
ap:api_key:all:managegrants two things:securitylist, so a dedicatedkey-admin persona can call them without also holding
ap:rest_api:manage,ap:llm_provider:manage, etc.created_byrestriction inside the service layer.Both are required; neither alone is sufficient. Reaching the endpoint (GO-AUTH-007) and being
allowed to touch another user's row are separate decisions, and the service-layer check is the
authoritative one (GO-AUTH-015).
The override widens which rows within the caller's own organization are reachable — never
which organization. Every query still sources
organization_uuidfrom token claims (GO-AUTH-005).Granted to
platform-adminonly.platform-operator,platform-developer, andplatform-viewerdeliberately do not receive it.
Surfaces covered
PUT/DELETE /rest-apis/{id}/api-keys/{keyId}PUT/DELETE /websub-apis,/webbroker-apis.../api-keys/{keyId}APIKeyService)GET/DELETE /llm-providers/{id}/api-keysGET/DELETE /llm-proxies/{id}/api-keysPOST /applications/{id}/api-keysGET /me/api-keysImplementation
One shared predicate. The ownership rule was previously written out separately in each service,
with three subtly different forms. It is now a single function in
internal/service/apikey.gothat every path calls, so it cannot drift between artifact kinds:
Explicit parameters, not implicit context. Handlers resolve the scope once from the request's
verified claims via
middleware.HasEffectiveScope(the pattern already used inorganization.go), then pass an explicitkeyAdmin booldown. An authorization input travels asa visible argument that a future refactor cannot silently drop.
The repository race guard is unaffected.
Update/Revokestill re-verifycreated_byintheir
WHEREclause, and admin paths pass the key's stored creator, so the delete/recreateprotection holds for admin-initiated writes too.
Gateway side.
APIKeyUpdateParamsandAPIKeyRevocationParamsgained anAuthorizedUpstream bool, set totrueonly byUpdateExternalAPIKeyFromEvent/RevokeExternalAPIKeyFromEvent. The check is skipped there, not deleted — the gateway's own RESTAPI path still enforces it in full, since the flag defaults to
false.Behavior changes worth reviewer attention
1. A latent break in the webhook path is fixed.
internal/webhook/handlers_apikey.gocalls update/revoke withuserID: "". The fail-closeduserId == ""check added in 6598519 made both of those webhook events return403on everydelivery. They now pass
keyAdmin: true, justified by the envelope's HMAC signature (verified inReceiveEventbefore dispatch) rather than by key ownership — the payload carries no useridentity at all. Please confirm this reading matches intent.
2. The LLM services' implicit-admin bypass is removed.
They previously treated an empty caller id as admin (
if userID != "" && key.CreatedBy != userID),which fails open for any path that forgets to populate the actor. The shared predicate fails
closed instead. All callers go through
resolveActorErr, which errors on an unresolvable actor,so no live path is affected.
3.
/me/api-keyswidening is an explicit flag, not an empty string.ListAPIKeysByUsertakesallUsers booland errors if given an empty username without it,rather than silently dropping the creator filter and returning the whole org's keys. The SQL is
unchanged in shape: the creator predicate is one of two code-controlled literals and the username
is always bound through a
?placeholder (GO-AUTH-008).Known limitation
On the webhook path,
keyAdmin: truebypasses ownership for whatever key occupies the target(artifact, handle)— not only keys that channel provisioned. Sinceapi_keysuniqueness isartifact + name, a Developer Portal event could rotate or revoke a key an interactive user created
via the REST API under a colliding handle. This is documented in the code rather than left
implicit. If it is unacceptable, the tighter fix is to assert
created_by == ""(system-owned) onthe webhook path instead of passing a blanket flag — happy to switch.
Documentation
Added GO-AUTH-019: Ownership-Override Scopes Must Be Named, Service-Layer Enforced, and Never
Silently Assumed to
.claude/rules/authentication_authorization.md. This pattern deliberatelycuts against the creator-scoped default and looks like a tenant-isolation hole at a glance, so it
needs a written rationale. The rule covers why an empty caller id must never mean "admin", why the
downstream check is gated rather than deleted, and why the override must not widen past the
caller's own organization. Purely additive — no existing rule renumbered or edited.
Testing
internal/service/apikey_authz_test.go—TestCanManageAPIKey, 7 cases pinning theownership rule including the fail-closed empty-caller behavior. Passes.
go vetclean across bothplatform-apiandgateway-controller, test files included.internal/service/application_test.gocall sites updated for the new parameter.internal/repositoryandplugins/eventgateway/handlercould not be linked. Those two packagesare unverified — CI should be treated as the source of truth for them.
Configuration / upgrade notes
The scope is appended to every shipped admin scope string (
config.toml,config-template.toml,default_config.go, Helmvalues.yaml, and both e2e configs). Operatorsusing IDP role mode get it automatically via
platform-admininroles.yaml. Operators usingscope mode with a hand-maintained scope list must add
ap:api_key:all:manageto their adminclient for cross-user key management to work; without it, behavior is unchanged (creator-scoped).
portals/ai-workspace/configs/config-template.tomlwas deliberately not updated — it requestsneither
ap:api_key:readnor this scope today, and defaulting a user-facing portal to anorg-wide admin grant seemed wrong. Easy to add if you disagree.