FEATURE: Overhaul of admin API key system (#8284)

- Allow revoking keys without deleting them
- Auto-revoke keys after a period of no use (default 6 months)
- Allow multiple keys per user
- Allow attaching a description to each key, for easier auditing
- Log changes to keys in the staff action log
- Move all key management to one place, and improve the UI

spec/requests/admin/api_controller_spec.rb

@@ -10,6 +10,9 @@ describe Admin::ApiController do
 
   fab!(:admin) { Fabricate(:admin) }
 
+  fab!(:key1) { Fabricate(:api_key, description: "my key") }
+  fab!(:key2) { Fabricate(:api_key, user: admin) }
+
   context "as an admin" do
     before do
       sign_in(admin)
@@ -19,60 +22,159 @@ describe Admin::ApiController do
       it "succeeds" do
         get "/admin/api/keys.json"
         expect(response.status).to eq(200)
+        expect(JSON.parse(response.body)["keys"].length).to eq(2)
       end
     end
 
-    describe '#regenerate_key' do
-      fab!(:api_key) { Fabricate(:api_key) }
-
-      it "returns 404 when there is no key" do
-        put "/admin/api/key.json", params: { id: 1234 }
-        expect(response.status).to eq(404)
-      end
-
-      it "delegates to the api key's `regenerate!` method" do
-        prev_value = api_key.key
-        put "/admin/api/key.json", params: { id: api_key.id }
+    describe '#show' do
+      it "succeeds" do
+        get "/admin/api/keys/#{key1.id}.json"
         expect(response.status).to eq(200)
-
-        api_key.reload
-        expect(api_key.key).not_to eq(prev_value)
-        expect(api_key.created_by.id).to eq(admin.id)
+        data = JSON.parse(response.body)["key"]
+        expect(data["id"]).to eq(key1.id)
+        expect(data["key"]).to eq(key1.key)
+        expect(data["description"]).to eq("my key")
       end
     end
 
-    describe '#revoke_key' do
-      fab!(:api_key) { Fabricate(:api_key) }
+    describe '#update' do
+      it "allows updating the description" do
+        original_key = key1.key
 
-      it "returns 404 when there is no key" do
-        delete "/admin/api/key.json", params: { id: 1234 }
-        expect(response.status).to eq(404)
+        put "/admin/api/keys/#{key1.id}.json", params: {
+          key: {
+            description: "my new description",
+            key: "overridekey"
+          }
+        }
+        expect(response.status).to eq(200)
+
+        key1.reload
+        expect(key1.description).to eq("my new description")
+        expect(key1.key).to eq(original_key)
+
+        expect(UserHistory.last.action).to eq(UserHistory.actions[:api_key_update])
+        expect(UserHistory.last.subject).to eq(key1.truncated_key)
       end
 
-      it "delegates to the api key's `regenerate!` method" do
-        delete "/admin/api/key.json", params: { id: api_key.id }
+      it "returns 400 for invalid payloads" do
+        put "/admin/api/keys/#{key1.id}.json", params: {
+          key: "string not a hash"
+        }
+        expect(response.status).to eq(400)
+
+        put "/admin/api/keys/#{key1.id}.json", params: {}
+        expect(response.status).to eq(400)
+      end
+    end
+
+    describe "#destroy" do
+      it "works" do
+        expect(ApiKey.exists?(key1.id)).to eq(true)
+
+        delete "/admin/api/keys/#{key1.id}.json"
+
         expect(response.status).to eq(200)
-        expect(ApiKey.where(key: api_key.key).count).to eq(0)
+        expect(ApiKey.exists?(key1.id)).to eq(false)
+
+        expect(UserHistory.last.action).to eq(UserHistory.actions[:api_key_destroy])
+        expect(UserHistory.last.subject).to eq(key1.truncated_key)
+      end
+    end
+
+    describe "#create" do
+      it "can create a master key" do
+        post "/admin/api/keys.json", params: {
+          key: {
+            description: "master key description"
+          }
+        }
+        expect(response.status).to eq(200)
+
+        data = JSON.parse(response.body)
+
+        expect(data['key']['description']).to eq("master key description")
+        expect(data['key']['user']).to eq(nil)
+        expect(data['key']['key']).to_not eq(nil)
+        expect(data['key']['last_used_at']).to eq(nil)
+
+        key = ApiKey.find(data['key']['id'])
+        expect(key.description).to eq("master key description")
+        expect(key.user).to eq(nil)
+
+        expect(UserHistory.last.action).to eq(UserHistory.actions[:api_key_create])
+        expect(UserHistory.last.subject).to eq(key.truncated_key)
+      end
+
+      it "can create a user-specific key" do
+        user = Fabricate(:user)
+        post "/admin/api/keys.json", params: {
+          key: {
+            description: "restricted key description",
+            username: user.username
+          }
+        }
+        expect(response.status).to eq(200)
+
+        data = JSON.parse(response.body)
+
+        expect(data['key']['description']).to eq("restricted key description")
+        expect(data['key']['user']['username']).to eq(user.username)
+        expect(data['key']['key']).to_not eq(nil)
+        expect(data['key']['last_used_at']).to eq(nil)
+
+        key = ApiKey.find(data['key']['id'])
+        expect(key.description).to eq("restricted key description")
+        expect(key.user.id).to eq(user.id)
+
+        expect(UserHistory.last.action).to eq(UserHistory.actions[:api_key_create])
+        expect(UserHistory.last.subject).to eq(key.truncated_key)
+      end
+    end
+
+    describe "#revoke and #undo_revoke" do
+      it "works correctly" do
+        post "/admin/api/keys/#{key1.id}/revoke.json"
+        expect(response.status).to eq 200
+
+        key1.reload
+        expect(key1.revoked_at).to_not eq(nil)
+        expect(UserHistory.last.action).to eq(UserHistory.actions[:api_key_update])
+        expect(UserHistory.last.subject).to eq(key1.truncated_key)
+        expect(UserHistory.last.details).to eq(I18n.t("staff_action_logs.api_key.revoked"))
+
+        post "/admin/api/keys/#{key1.id}/undo-revoke.json"
+        expect(response.status).to eq 200
+
+        key1.reload
+        expect(key1.revoked_at).to eq(nil)
+        expect(UserHistory.last.action).to eq(UserHistory.actions[:api_key_update])
+        expect(UserHistory.last.subject).to eq(key1.truncated_key)
+        expect(UserHistory.last.details).to eq(I18n.t("staff_action_logs.api_key.restored"))
       end
     end
   end
 
-  describe '#create_master_key' do
-    it "creates a record" do
-      sign_in(admin)
-      expect do
-        post "/admin/api/key.json"
-      end.to change(ApiKey, :count).by(1)
-      expect(response.status).to eq(200)
-    end
-
-    it "doesn't allow moderators to create master keys" do
+  context "as a moderator" do
+    before do
       sign_in(Fabricate(:moderator))
-      expect do
-        post "/admin/api/key.json"
-      end.to change(ApiKey, :count).by(0)
-      expect(response.status).to eq(404)
     end
 
+    it "doesn't allow access" do
+      get "/admin/api/keys.json"
+      expect(response.status).to eq(404)
+
+      get "/admin/api/key/#{key1.id}.json"
+      expect(response.status).to eq(404)
+
+      post "/admin/api/keys.json", params: {
+        key: {
+          description: "master key description"
+        }
+      }
+      expect(response.status).to eq(404)
+
+      expect(ApiKey.count).to eq(2)
+    end
   end
 end

spec/requests/admin/users_controller_spec.rb

@@ -123,25 +123,6 @@ RSpec.describe Admin::UsersController do
     end
   end
 
-  describe '#generate_api_key' do
-    it 'calls generate_api_key' do
-      post "/admin/users/#{user.id}/generate_api_key.json"
-      expect(response.status).to eq(200)
-      json = JSON.parse(response.body)
-      expect(json["api_key"]["user"]["id"]).to eq(user.id)
-      expect(json["api_key"]["key"]).to be_present
-    end
-  end
-
-  describe '#revoke_api_key' do
-    it 'calls revoke_api_key' do
-      ApiKey.create!(user: user, key: SecureRandom.hex)
-      delete "/admin/users/#{user.id}/revoke_api_key.json"
-      expect(response.status).to eq(200)
-      expect(ApiKey.where(user: user).count).to eq(0)
-    end
-  end
-
   describe '#suspend' do
     fab!(:post) { Fabricate(:post) }
     let(:suspend_params) do
@@ -269,15 +250,26 @@ RSpec.describe Admin::UsersController do
       expect(log.details).to match(/long reason/)
     end
 
-    it "also revokes any api keys" do
-      Fabricate(:api_key, user: user)
-      put "/admin/users/#{user.id}/suspend.json", params: suspend_params
+    it "also prevents use of any api keys" do
+      api_key = Fabricate(:api_key, user: user)
 
+      put "/posts/#{Fabricate(:post).id}/bookmark.json", params: {
+        bookmarked: "true",
+        api_key: api_key.key
+      }
       expect(response.status).to eq(200)
-      user.reload
 
+      put "/admin/users/#{user.id}/suspend.json", params: suspend_params
+      expect(response.status).to eq(200)
+
+      user.reload
       expect(user).to be_suspended
-      expect(ApiKey.where(user_id: user.id).count).to eq(0)
+
+      put "/posts/#{Fabricate(:post).id}/bookmark.json", params: {
+        bookmarked: "true",
+        api_key: api_key.key
+      }
+      expect(response.status).to eq(403)
     end
   end
 

spec/requests/embed_controller_spec.rb

@@ -42,7 +42,7 @@ describe EmbedController do
 
     context "with api key" do
 
-      let(:api_key) { ApiKey.create_master_key }
+      let(:api_key) { Fabricate(:api_key) }
 
       context "with valid embed url" do
         let(:topic_embed) { Fabricate(:topic_embed, embed_url: embed_url) }

spec/requests/posts_controller_spec.rb

@@ -524,8 +524,8 @@ describe PostsController do
     end
 
     context "api" do
-      let(:api_key) { user.generate_api_key(user) }
-      let(:master_key) { ApiKey.create_master_key }
+      let(:api_key) { Fabricate(:api_key, user: user) }
+      let(:master_key) { Fabricate(:api_key, user: nil) }
 
       # choosing an arbitrarily easy to mock trusted activity
       it 'allows users with api key to bookmark posts' do
@@ -711,7 +711,7 @@ describe PostsController do
         raw = "this is a test post 123 #{SecureRandom.hash}"
         title = "this is a title #{SecureRandom.hash}"
 
-        master_key = ApiKey.create_master_key.key
+        master_key = Fabricate(:api_key).key
 
         post "/posts.json", params: {
           api_username: user.username,
@@ -740,7 +740,7 @@ describe PostsController do
         Jobs.run_immediately!
         NotificationEmailer.enable
         post_1 = Fabricate(:post)
-        master_key = ApiKey.create_master_key.key
+        master_key = Fabricate(:api_key).key
 
         post "/posts.json", params: {
           api_username: user.username,
@@ -796,7 +796,7 @@ describe PostsController do
 
       it 'will raise an error if specified category cannot be found' do
         user = Fabricate(:admin)
-        master_key = ApiKey.create_master_key.key
+        master_key = Fabricate(:api_key).key
 
         post "/posts.json", params: {
           api_username: user.username,

spec/requests/topics_controller_spec.rb

@@ -1759,7 +1759,7 @@ RSpec.describe TopicsController do
       end
 
       context 'and the user is not logged in' do
-        let(:api_key) { topic.user.generate_api_key(topic.user) }
+        let(:api_key) { Fabricate(:api_key, user: topic.user) }
 
         it 'redirects to the login page' do
           get "/t/#{topic.slug}/#{topic.id}.json"

spec/requests/user_badges_controller_spec.rb

@@ -111,10 +111,10 @@ describe UserBadgesController do
     end
 
     it 'does not grant badges from regular api calls' do
-      Fabricate(:api_key, user: user)
+      api_key = Fabricate(:api_key, user: user)
 
       post "/user_badges.json", params: {
-        badge_id: badge.id, username: user.username, api_key: user.api_key.key
+        badge_id: badge.id, username: user.username, api_key: api_key.key
       }
 
       expect(response.status).to eq(403)