Skip to content

feat: make regular api tokens revocable#1027

Open
matmanna wants to merge 41 commits intohackclub:mainfrom
matmanna:revokable_api_tokens
Open

feat: make regular api tokens revocable#1027
matmanna wants to merge 41 commits intohackclub:mainfrom
matmanna:revokable_api_tokens

Conversation

@matmanna
Copy link
Member

@matmanna matmanna commented Mar 2, 2026

This PR will be landed prior to but alongside a corresponding one for the revoker.

This PR adds:

  • additionally functionality to the /api/internal/revoke endpoint to distinguish between admin and normal user keys in order to roll exposed user tokens
  • properly documents HKA_REVOCATION_KEY in .env.example
  • new revocations controller tests for regular keys

@matmanna matmanna marked this pull request as ready for review March 2, 2026 21:06
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.

@matmanna matmanna force-pushed the revokable_api_tokens branch 3 times, most recently from 4c1f4b2 to 8e5aabd Compare March 5, 2026 14:27
@matmanna matmanna changed the title Make Hackatime Tokens Revocable Revocable API Tokens Mar 7, 2026
@matmanna
Copy link
Member Author

matmanna commented Mar 8, 2026

hmm is it possible for me to get a @greptileai review

@matmanna matmanna force-pushed the revokable_api_tokens branch from 8e5aabd to f588a2e Compare March 8, 2026 00:35
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Mar 8, 2026

Greptile Summary

This PR extends the internal revocation endpoint to support revoking regular user ApiKeys (via token-rolling) in addition to the existing AdminApiKey revocation (via revoked_at timestamp). A regex-based dispatcher distinguishes key types, error handling has been hardened with a non-raising update + rescue ActiveRecord::ActiveRecordError, and a TODO comment pre-emptively flags the missing active scope on ApiKey. The previously undocumented HKA_REVOCATION_KEY env var is now documented in .env.example.

  • Regex constants (ADMIN_KEY_REGEX, REGULAR_KEY_REGEX) cleanly separate the two key formats with no overlap risk.
  • revoke_key! correctly uses update (non-bang) for ApiKey and wraps AdminApiKey#revoke! (which uses update!) in a shared rescue clause — all previously flagged concerns about unhandled exceptions are addressed.
  • A new test file provides thorough coverage: happy-path token rolling, non-existent token, unrecognised format, simulated DB collision failure (via a SecureRandom.uuid_v4 stub), and an admin-key regression test.
  • Minor: the key_name field in the success response returns the post-mutation name (e.g., "Desktop_revoked_abc12345") rather than the original name; this is consistent with the pre-existing admin-key behaviour but may affect how the revoker service formats user-facing notifications.

Confidence Score: 4/5

  • This PR is safe to merge; the new code paths are well-tested and error-handling is solid.
  • All three previously flagged concerns (missing active-scope guard, missing tests, unhandled update! exception) have been successfully addressed in the code. The TODO comment appropriately flags the future need for an active scope on ApiKey, the test file provides comprehensive coverage of both happy paths and edge cases, and the error handler properly rescues ActiveRecord::ActiveRecordError. The only remaining finding is a design consideration regarding the post-mutation key_name in the response, which is consistent with pre-existing behavior but may merit discussion with the revoker service maintainers.
  • No files require special attention; app/controllers/api/internal/revocations_controller.rb is the most critical but is well-implemented.

Sequence Diagram

sequenceDiagram
    participant Revoker as Revoker Service
    participant RC as RevocationsController
    participant DB as Database

    Revoker->>RC: POST /api/internal/revoke (Bearer auth)
    RC->>RC: authenticate! via secure_compare

    alt Matches ADMIN_KEY_REGEX
        RC->>DB: AdminApiKey.active.find_by(key)
        DB-->>RC: admin_key or nil
        RC->>DB: admin_key.revoke! sets revoked_at and renames
        DB-->>RC: ok or raises
    else Matches REGULAR_KEY_REGEX
        RC->>DB: ApiKey.find_by(key)
        DB-->>RC: api_key or nil
        RC->>DB: api_key.update rolls uuid and renames
        DB-->>RC: true or false
    else No regex match
        RC-->>Revoker: success false
    end

    alt Not found or failed
        RC-->>Revoker: success false
    else Succeeded
        RC-->>Revoker: success true plus owner_email and key_name
    end
Loading

Last reviewed commit: 2fb2aff

@matmanna
Copy link
Member Author

@greptileai re-review pls?

Copy link
Member

@skyfallwastaken skyfallwastaken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! Left a couple comments for you to look at

@matmanna matmanna changed the title Revocable API Tokens feat: make regular api tokens revocable Mar 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants