From be0fd5b4cc3329b78a4431a31e87ea9c7d7f8d2d Mon Sep 17 00:00:00 2001
From: Sam <sam.saffron@gmail.com>
Date: Fri, 2 Sep 2016 16:57:41 +1000
Subject: [PATCH] FEATURE: allow user api key revocation for read only keys

---
 app/controllers/user_api_keys_controller.rb   | 11 +++++++++-
 lib/auth/default_current_user_provider.rb     | 10 +++++++--
 .../user_api_keys_controller_spec.rb          | 21 +++++++++++++++++++
 spec/fabricators/user_api_key_fabricator.rb   |  9 ++++++++
 4 files changed, 48 insertions(+), 3 deletions(-)
 create mode 100644 spec/fabricators/user_api_key_fabricator.rb

diff --git a/app/controllers/user_api_keys_controller.rb b/app/controllers/user_api_keys_controller.rb
index 175623751..3875e982b 100644
--- a/app/controllers/user_api_keys_controller.rb
+++ b/app/controllers/user_api_keys_controller.rb
@@ -90,7 +90,16 @@ class UserApiKeysController < ApplicationController
   end
 
   def revoke
-    find_key.update_columns(revoked_at: Time.zone.now)
+    revoke_key = find_key
+    if current_key = request.env['HTTP_USER_API_KEY']
+      request_key = UserApiKey.find_by(key: current_key)
+      if request_key && request_key.id != revoke_key.id && !request_key.write
+        raise Discourse::InvalidAccess
+      end
+    end
+
+    revoke_key.update_columns(revoked_at: Time.zone.now)
+
     render json: success_json
   end
 
diff --git a/lib/auth/default_current_user_provider.rb b/lib/auth/default_current_user_provider.rb
index a4224d823..a840b2770 100644
--- a/lib/auth/default_current_user_provider.rb
+++ b/lib/auth/default_current_user_provider.rb
@@ -176,10 +176,16 @@ class Auth::DefaultCurrentUserProvider
 
   protected
 
+  WHITELISTED_WRITE_PATHS ||= [/^\/message-bus\/.*\/poll/, /^\/user-api-key\/revoke$/]
   def lookup_user_api_user(user_api_key)
     if api_key = UserApiKey.where(key: user_api_key, revoked_at: nil).includes(:user).first
-      if !api_key.write && (@env["REQUEST_METHOD"] != "GET" && @env["PATH_INFO"] !~ /^\/message-bus\/.*\/poll/)
-        raise Discourse::InvalidAccess
+      unless api_key.write
+        if @env["REQUEST_METHOD"] != "GET"
+          path = @env["PATH_INFO"]
+          unless WHITELISTED_WRITE_PATHS.any?{|whitelisted| path =~ whitelisted}
+            raise Discourse::InvalidAccess
+          end
+        end
       end
 
       api_key.user
diff --git a/spec/controllers/user_api_keys_controller_spec.rb b/spec/controllers/user_api_keys_controller_spec.rb
index 9884eff07..9d0324c9a 100644
--- a/spec/controllers/user_api_keys_controller_spec.rb
+++ b/spec/controllers/user_api_keys_controller_spec.rb
@@ -94,6 +94,27 @@ TXT
 
     end
 
+    it "will not allow readonly api keys to revoke others" do
+      key1 = Fabricate(:readonly_user_api_key)
+      key2 = Fabricate(:readonly_user_api_key)
+
+      request.env['HTTP_USER_API_KEY'] = key1.key
+      post :revoke, id: key2.id
+
+      expect(response.status).to eq(403)
+    end
+
+    it "will allow readonly api keys to revoke self" do
+      key = Fabricate(:readonly_user_api_key)
+      request.env['HTTP_USER_API_KEY'] = key.key
+      post :revoke, id: key.id
+
+      expect(response.status).to eq(200)
+
+      key.reload
+      expect(key.revoked_at).not_to eq(nil)
+    end
+
     it "will not return p access if not yet configured" do
       SiteSetting.min_trust_level_for_user_api_key = 0
       SiteSetting.allowed_user_api_auth_redirects = args[:auth_redirect]
diff --git a/spec/fabricators/user_api_key_fabricator.rb b/spec/fabricators/user_api_key_fabricator.rb
new file mode 100644
index 000000000..5658a4981
--- /dev/null
+++ b/spec/fabricators/user_api_key_fabricator.rb
@@ -0,0 +1,9 @@
+Fabricator(:readonly_user_api_key, from: :user_api_key) do
+  user
+  read true
+  write false
+  push false
+  client_id { SecureRandom.hex }
+  key { SecureRandom.hex }
+  application_name 'some app'
+end