Require API key step up for list and revoke - #528
thomasluizon wants to merge 3 commits into
Conversation
|
Approach:
The implementation lands in the existing API key handlers, email challenge service, shared error copy, and their unit tests. I rejected a second grant type because creation already establishes the server-backed mechanism. I rejected consuming on list because opening or refreshing the surface must remain usable. I rejected retaining the grant after revoke because that would weaken the destructive boundary compared with creation. |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes Reviewed the current PR head, which contains no changed files or behavioral surface.
- Bootstrap commit The sole commit establishes the implementation branch without code changes; the described API key step-up behavior will need review after implementation commits land.
GPT Sol ｜ 𝕏
The step up change moved the API key endpoints' shape, so `architecture.json` and `architecture.html` no longer matched the tree and the `drift` check failed on `git diff --exit-code`. Regenerated with `node tools/arch-map.mjs`, which reports 45 entities and 0 untested feature folders. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
There was a problem hiding this comment.
Important
The new grant checks break the advertised chat and MCP API-key list and revoke flows once the feature flag is enabled.
Reviewed changes Reviewed the API key management step-up implementation added since the prior Pullfrog review.
- Shared management grant Renamed the challenge operation from creation to management while retaining the existing cache keys and confirmation endpoints.
- Protected list access Added a non-consuming authorization check after the Pro pay gate and before cached or repository-backed key data is returned.
- Protected revocation Added authorization checks around owned-key lookup and consumes the one-time grant before revoking the key.
- Regression coverage Added absent, valid, repeated-read, spent, expired, and non-Pro cases for list and revoke behavior.
GPT Sol ｜ 𝕏
| false, | ||
| cancellationToken); | ||
| if (requiresStepUp && | ||
| !challengeService.HasAuthorization(EmailChallengeOperation.ApiKeyManagement, request.UserId)) |
There was a problem hiding this comment.
GetApiKeysQuery is also dispatched by chat/MCP get_api_keys, and the same new grant gates manage_api_keys revocation after agent confirmation. Neither agent surface can issue or confirm the email challenge, so enabling the flag makes listing fail and revocation fail even after valid agent step-up unless the user separately completes the HTTP challenge.
Technical details
# Agent API-key operations cannot establish the management grant
## Affected sites
- `src/Orbit.Application/ApiKeys/Queries/GetApiKeysQuery.cs:42` - requires an email grant for the query dispatched directly by chat and MCP `get_api_keys`.
- `src/Orbit.Application/ApiKeys/Commands/RevokeApiKeyCommand.cs:34` and `:48` - requires and consumes that grant after `manage_api_keys` has already passed its separate agent step-up.
- `src/Orbit.Application/Chat/Tools/Implementations/GetApiKeysTool.cs:21` and `ManageApiKeysTool.cs:75` - dispatch the gated handlers but expose no email challenge operation.
- `src/Orbit.Api/Mcp/Tools/ApiKeyTools.cs:25` and `:59` - the read dispatches directly, while the mutation bridge validates agent policy without creating an `EmailChallengeService` grant.
## Required outcome
- Every advertised chat/MCP list and revoke flow must remain usable when `RequireApiKeyCreationStepUp` is enabled, or be rejected as a direct-user-only operation before it is advertised or executed.
- Preserve the intended email security boundary rather than treating agent confirmation as equivalent unless that equivalence is an explicit product decision.
## Open questions for the human
- Should a successful agent step-up satisfy API-key management authorization, or should chat/MCP API-key operations become direct-user-only?1 participant
Summary
Require the existing email challenge grant for API key listing and revocation while preserving the Pro pay gate.
Listing checks the grant without consuming it. Revocation consumes the grant exactly once. The existing 428 challenge response remains distinct from the 403 Pro refusal.
The shared challenge operation now represents API key management. Existing request and confirm endpoint names remain unchanged.
Deploy order
Deploy the API change first. Current store builds lose API key listing and revocation until updated. Thomas accepted this beta break in #529.
Test evidence
dotnet test tests/Orbit.Application.Tests/Orbit.Application.Tests.csproj --filter "FullyQualifiedName~GetApiKeysQueryHandlerTests|FullyQualifiedName~RevokeApiKeyCommandHandlerTests" --logger "console;verbosity=normal"passed all 5 existing tests with the defect present.dotnet test tests/Orbit.Application.Tests/Orbit.Application.Tests.csproj --filter "FullyQualifiedName~GetApiKeysQueryHandlerTests|FullyQualifiedName~RevokeApiKeyCommandHandlerTests" --no-restore --logger "console;verbosity=minimal"failed both new tests. Each observedExpected result.IsFailure to be True, but found False.while the 5 existing tests still passed.dotnet build Orbit.slnxcompleted with 0 errors.dotnet test --no-buildpassed all 6,256 tests.Manual steps
RequireApiKeyCreationStepUptotruein theAppConfigstable, after the server change is deployed. Read the row back and verify its value istrue.Fixes #529