feat: make regular api tokens revocable (#1027)

* add new icon from bounty

* feat: add hackatime normal token revocation

* chore: make linter not hate me (its always whitespace) <3

* fix: combine both revocation apis into one (as requested by mahad)

* chore: add HKA_REVOCATION_KEY to .env.example

* feat: add hackatime normal token revocation

* chore: make linter not hate me (its always whitespace) <3

* fix: combine both revocation apis into one (as requested by mahad)

* chore: add HKA_REVOCATION_KEY to .env.example

* feat: add hackatime normal token revocation

* chore: make linter not hate me (its always whitespace) <3

* fix: combine both revocation apis into one (as requested by mahad)

* chore: add HKA_REVOCATION_KEY to .env.example

* feat: add hackatime normal token revocation

* chore: make linter not hate me (its always whitespace) <3

* fix: combine both revocation apis into one (as requested by mahad)

* fix: stuff greptile suggested

* style: add final newline

* docs: apply .env.example suggestion from @skyfallwastaken

Co-authored-by: Mahad Kalam <55807755+skyfallwastaken@users.noreply.github.com>

* refactor: move apikey rotation to user model

* style: remove unnecessary comment

* fix: tests passing and inappropriate response codes

* refactor: fix response codes

* refactor: move key info request back into separate function

* fix: broken ci because of merge mistake :/

* refactor: remove unnecessary test line and switch to  report_error

* fix: returned name for admin & regular keys

---------

Co-authored-by: Mahad Kalam <55807755+skyfallwastaken@users.noreply.github.com>

.env.example

@@ -60,3 +60,6 @@ S3_ACCESS_KEY_ID=your_s3_access_key_id_here
 S3_SECRET_ACCESS_KEY=your_s3_secret_access_key_here
 S3_BUCKET=your_s3_bucket_name_here
 S3_ENDPOINT=https://<ACCOUNT_ID>.r2.cloudflarestorage.com
+
+# Key for Revoker (https://github.com/hackclub/revoker)
+HKA_REVOCATION_KEY=your_hka_revocation_key_here

app/controllers/api/internal/revocations_controller.rb

@@ -1,24 +1,60 @@
 module Api
   module Internal
     class RevocationsController < Api::Internal::ApplicationController
+      REGULAR_KEY_REGEX = /\A[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}\z/i
+      ADMIN_KEY_REGEX = /\Ahka_[0-9a-f]{64}\z/
+
       def create
         token = params[:token]
 
-        return head 400 unless token.present?
+        return render_error("Token is required") unless token.present?
 
-        admin_api_key = AdminApiKey.active.find_by(token:)
+        key, user, token_type, token_format = find_key_info(token)
+        return render_error("Token doesn't match any supported type") unless token_format
+        return render_error("Token is invalid or already revoked") unless key.present?
+        original_key_name = key.name
+        return render_error("Token is invalid or already revoked") unless revoke_key(key)
 
-        return render json: { success: false } unless admin_api_key.present?
+        response_payload = {
-
-        admin_api_key.revoke!
-
-        user = admin_api_key.user
-
-        render json: {
           success: true,
-          owner_email: user.email_addresses.first&.email,
+          status: "complete",
-          key_name: admin_api_key.name
+          token_type: token_type,
-        }.compact_blank
+          owner_email: user.email_addresses.first&.email
+        }
+        response_payload[:key_name] = original_key_name if token_format == :admin
+
+        render json: response_payload.compact_blank, status: :created
+      end
+
+      private
+
+      def find_key_info(token)
+        if token.match?(ADMIN_KEY_REGEX)
+          key = AdminApiKey.active.find_by(token:)
+          return [ key, key&.user, key&.name, :admin ]
+        end
+
+        if token.match?(REGULAR_KEY_REGEX)
+          key = ApiKey.find_by(token:)
+          return [ key, key&.user, key&.name, :regular ]
+        end
+
+        [ nil, nil, nil, nil ]
+      end
+
+      def revoke_key(key)
+        if key.is_a?(AdminApiKey)
+          key.revoke!
+        else
+          key.user.rotate_single_api_key!(key)
+        end
+      rescue ActiveRecord::ActiveRecordError => e
+        report_error(e)
+        false
+      end
+
+      def render_error(message)
+        render json: { success: false, error: message }, status: :unprocessable_entity
       end
 
       private def authenticate!

app/controllers/settings/access_controller.rb

@@ -14,14 +14,10 @@ class Settings::AccessController < Settings::BaseController
   end
 
   def rotate_api_key
-    @user.api_keys.transaction do
+    new_api_key = @user.rotate_api_keys!
-      @user.api_keys.destroy_all
 
-      new_api_key = @user.api_keys.create!(name: "Hackatime key")
+    PosthogService.capture(@user, "api_key_rotated")
-
+    render json: { token: new_api_key.token }, status: :ok
-      PosthogService.capture(@user, "api_key_rotated")
-      render json: { token: new_api_key.token }, status: :ok
-    end
   rescue => e
     report_error(e, message: "error rotate #{e.class.name}")
     render json: { error: "cant rotate" }, status: :unprocessable_entity

app/models/user.rb

@@ -286,6 +286,20 @@ class User < ApplicationRecord
     sign_in_tokens.create!(auth_type: :email, continue_param: continue_param)
   end
 
+  def rotate_api_keys!
+    api_keys.transaction do
+      api_keys.destroy_all
+      api_keys.create!(name: "Hackatime key")
+    end
+  end
+
+  def rotate_single_api_key!(api_key)
+    raise ActiveRecord::RecordNotFound unless api_key.user_id == id
+
+    api_key.update!(token: SecureRandom.uuid_v4)
+    api_key
+  end
+
   def find_valid_token(token)
     sign_in_tokens.valid.find_by(token: token)
   end

spec/requests/api/internal/internal_spec.rb

@@ -12,14 +12,19 @@ RSpec.describe 'Api::Internal', type: :request do
       parameter name: :payload, in: :body, schema: {
         type: :object,
         properties: {
-          token: { type: :string }
+          token: { type: :string },
+          submitter: { type: :string },
+          comment: { type: :string }
         },
         required: [ 'token' ]
       }
 
-      response(200, 'successful') do
+      response(201, 'created') do
         let(:Authorization) { "Bearer test_revocation_key" }
-        let(:payload) { { token: 'some_token' } }
+        let(:user) { User.create!(timezone: "UTC") }
+        let!(:email_address) { user.email_addresses.create!(email: "internal@example.com", source: :signing_in) }
+        let!(:api_key) { user.api_keys.create!(name: "Desktop") }
+        let(:payload) { { token: api_key.token } }
 
         before do
           ENV["HKA_REVOCATION_KEY"] = "test_revocation_key"
@@ -32,15 +37,25 @@ RSpec.describe 'Api::Internal', type: :request do
         schema type: :object,
           properties: {
             success: { type: :boolean },
+            status: { type: :string },
+            token_type: { type: :string },
             owner_email: { type: :string, nullable: true },
             key_name: { type: :string, nullable: true }
           }
-        run_test!
+        run_test! do |response|
+          body = JSON.parse(response.body)
+
+          expect(body["success"]).to eq(true)
+          expect(body["status"]).to eq("complete")
+          expect(body["token_type"]).to eq("Hackatime API Key")
+          expect(body["owner_email"]).to eq(email_address.email)
+          expect(body["key_name"]).to eq(api_key.name)
+        end
       end
 
-      response(400, 'bad request') do
+      response(422, 'unprocessable entity') do
         let(:Authorization) { "Bearer test_revocation_key" }
-        let(:payload) { { token: nil } }
+        let(:payload) { { token: SecureRandom.uuid_v4 } }
 
         before do
           ENV["HKA_REVOCATION_KEY"] = "test_revocation_key"
@@ -50,6 +65,12 @@ RSpec.describe 'Api::Internal', type: :request do
           ENV.delete("HKA_REVOCATION_KEY")
         end
 
+        schema type: :object,
+          properties: {
+            success: { type: :boolean },
+            error: { type: :string }
+          },
+          required: [ 'success', 'error' ]
         run_test!
       end
     end

swagger/v1/swagger.yaml

@@ -1905,8 +1905,8 @@ paths:
       - InternalToken: []
       parameters: []
       responses:
-        '200':
+        '201':
-          description: successful
+          description: created
           content:
             application/json:
               schema:
@@ -1914,14 +1914,30 @@ paths:
                 properties:
                   success:
                     type: boolean
+                  status:
+                    type: string
+                  token_type:
+                    type: string
                   owner_email:
                     type: string
                     nullable: true
                   key_name:
                     type: string
                     nullable: true
-        '400':
+        '422':
-          description: bad request
+          description: unprocessable entity
+          content:
+            application/json:
+              schema:
+                type: object
+                properties:
+                  success:
+                    type: boolean
+                  error:
+                    type: string
+                required:
+                - success
+                - error
       requestBody:
         content:
           application/json:
@@ -1930,6 +1946,10 @@ paths:
               properties:
                 token:
                   type: string
+                submitter:
+                  type: string
+                comment:
+                  type: string
               required:
               - token
   "/api/summary":

test/controllers/api/internal/revocations_controller_test.rb

@@ -0,0 +1,96 @@
+require "test_helper"
+
+class Api::Internal::RevocationsControllerTest < ActionDispatch::IntegrationTest
+  setup do
+    @previous_revocation_key = ENV["HKA_REVOCATION_KEY"]
+    ENV["HKA_REVOCATION_KEY"] = "test-revocation-key"
+  end
+
+  teardown do
+    ENV["HKA_REVOCATION_KEY"] = @previous_revocation_key
+  end
+
+  test "revokes regular ApiKey by rolling token" do
+    user = User.create!(timezone: "UTC")
+    email_address = user.email_addresses.create!(email: "regular@example.com", source: :signing_in)
+    original_token = SecureRandom.uuid_v4
+    key = user.api_keys.create!(name: "Desktop", token: original_token)
+
+    post "/api/internal/revoke", params: { token: original_token }, headers: auth_headers, as: :json
+
+    assert_response :created
+    assert_equal true, response.parsed_body["success"]
+    assert_equal "complete", response.parsed_body["status"]
+    assert_equal key.name, response.parsed_body["token_type"]
+    assert_equal email_address.email, response.parsed_body["owner_email"]
+    assert_not_includes response.parsed_body.keys, "key_name"
+
+    key.reload
+    assert_not_equal original_token, key.token
+    assert_nil ApiKey.find_by(token: original_token)
+
+    post "/api/internal/revoke", params: { token: original_token }, headers: auth_headers, as: :json
+
+    assert_response :unprocessable_entity
+    assert_equal false, response.parsed_body["success"]
+    assert_equal "Token is invalid or already revoked", response.parsed_body["error"]
+  end
+
+  test "returns success false for valid regular UUID token that does not exist" do
+    token = SecureRandom.uuid_v4
+
+    post "/api/internal/revoke", params: { token: token }, headers: auth_headers, as: :json
+
+    assert_response :unprocessable_entity
+    assert_equal false, response.parsed_body["success"]
+    assert_equal "Token is invalid or already revoked", response.parsed_body["error"]
+  end
+
+  test "returns success false for token that matches neither regex" do
+    post "/api/internal/revoke", params: { token: "not-a-valid-token" }, headers: auth_headers, as: :json
+
+    assert_response :unprocessable_entity
+    assert_equal false, response.parsed_body["success"]
+    assert_equal "Token doesn't match any supported type", response.parsed_body["error"]
+  end
+
+  test "revokes admin key" do
+    user = User.create!(timezone: "UTC")
+    email_address = user.email_addresses.create!(email: "admin@example.com", source: :signing_in)
+    admin_key = user.admin_api_keys.create!(name: "Infra", token: "hka_#{SecureRandom.hex(32)}")
+
+    post "/api/internal/revoke", params: { token: admin_key.token }, headers: auth_headers, as: :json
+
+    assert_response :created
+    assert_equal true, response.parsed_body["success"]
+    assert_equal "complete", response.parsed_body["status"]
+    assert_equal "Infra", response.parsed_body["token_type"]
+
+    admin_key.reload
+    assert_equal email_address.email, response.parsed_body["owner_email"]
+    assert_equal "Infra", response.parsed_body["key_name"]
+    assert_not_nil admin_key.revoked_at
+    assert_includes admin_key.name, "_revoked_"
+  end
+
+  test "returns error for already-revoked admin key" do
+    user = User.create!(timezone: "UTC")
+    original_token = "hka_#{SecureRandom.hex(32)}"
+    admin_key = user.admin_api_keys.create!(name: "Infra", token: original_token)
+    admin_key.revoke!
+
+    post "/api/internal/revoke", params: { token: original_token }, headers: auth_headers, as: :json
+
+    assert_response :unprocessable_entity
+    assert_equal false, response.parsed_body["success"]
+    assert_equal "Token is invalid or already revoked", response.parsed_body["error"]
+  end
+
+  private
+
+  def auth_headers
+    {
+      "Authorization" => ActionController::HttpAuthentication::Token.encode_credentials(ENV.fetch("HKA_REVOCATION_KEY"))
+    }
+  end
+end

test/models/user_test.rb

@@ -30,6 +30,30 @@ class UserTest < ActiveSupport::TestCase
     assert_equal "gruvbox_dark", metadata[:value]
   end
 
+  test "rotate_api_keys! replaces existing api key with a new one" do
+    user = User.create!(timezone: "UTC", slack_uid: "U#{SecureRandom.hex(8)}")
+    user.api_keys.create!(name: "Original key")
+    original_token = user.api_keys.first.token
+
+    new_api_key = user.rotate_api_keys!
+
+    assert_equal user.id, new_api_key.user_id
+    assert_equal "Hackatime key", new_api_key.name
+    assert_nil ApiKey.find_by(token: original_token)
+  end
+
+  test "rotate_api_keys! creates a key when none exists" do
+    user = User.create!(timezone: "UTC", slack_uid: "U#{SecureRandom.hex(8)}")
+
+    assert_equal 0, user.api_keys.count
+
+    new_api_key = user.rotate_api_keys!
+
+    assert_equal user.id, new_api_key.user_id
+    assert_equal "Hackatime key", new_api_key.name
+    assert_equal [ new_api_key.id ], user.api_keys.reload.pluck(:id)
+  end
+
   test "flipper id uses the user id" do
     user = User.create!(timezone: "UTC")