Repository navigation
Add scoped admin API keys - #1795
Merged
Merged
Conversation
Each admin API endpoint now requires one AdminApiKey scope. Existing keys keep every scope but expire 48 days after the migration, and AdminApiKeyExpiryNoticeJob emails their owners once about it.
Contributor
[High impact] The latest change appears safe to merge; the earlier request for revocation-job tests remains open. Findings
Fix with agent prompt### Issue 1
app/jobs/revoke_expired_admin_api_keys_job.rb:undefined-5
The PR deletes the only test for `RevokeExpiredAdminApiKeysJob` while changing how it selects keys. This violates the repository requirement to include tests for new or changed functionality. Before merging, restore coverage that checks expired keys are revoked and renamed, while future and permanent keys stay untouched.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.SummaryThis PR adds scoped admin API keys, expiry for existing keys and expiry notices. Since the last review:
Reviews (3) · Last reviewed commit: "Show the admin API key token preview wit..." · Reviewed by Greptile |
Require heartbeats:read for user stats, which returns raw heartbeats. Mark keys as notified in the same transaction that enqueues the expiry email, and never run two notice jobs at once. Show expired or revoked keys as dead. Trim the mailer, a test and redundant assertions.
Drop the usage card and its copy from the key page.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary of the problem
Every admin API key has full access to the whole admin API. A key made for a read-only tool can also convict users, change admin levels and approve deletions, so a leaked key does as much damage as possible.
Describe your changes
AdminApiKeynow hasscopes. There are 13 scopes such asusers:read,heartbeats:read,bans:writeandpermissions:write, and each/api/admin/v1endpoint requires exactly one. A key without it gets a 403 naming the missing scope. The owner's admin level still applies on top, so a scope never grants more than the owner has.require_admin_api_key_scope. An action with no declared scope rejects keys, and a test fails if a new admin API route is added without one./api/admin/v1/checkworks with any scope and now returns the key's scopes and expiry. The legacy/api/v1/users/lookup_*and/api/v1/statsendpoints needusers:read. OAuth admin tokens are unchanged.expires_atto 48 days later. Expired keys stop authenticating immediately andRevokeExpiredAdminApiKeysJobmarks them revoked every 10 minutes.AdminApiKeyExpiryNoticeJobemails each admin once with their expiring keys and when each will be revoked. Keys are marked as notified in the same transaction that enqueues the email and only one run can happen at a time, so it never sends twice. It is not on cron, so run it once after the migration:Performance
In-process requests through the full Rails stack with a fully scoped key, 3 alternating rounds of 3000 requests per endpoint against
main. Median ms.mainGET /api/admin/v1/checkGET /api/admin/v1/user/infoGET /api/v1/users/lookup_slack_uidThe key lookup still uses the unique
token_bidxindex and the expiry check filters that one row. The scope check is in memory.Screenshots / Media
AI Usage
claude-opus-5-5)