FEATURE: Allow parameter authentication for UserApiKeys (#9742)

FEATURE: Allow parameter authentication for UserApiKeys (#9742)

This refactors default_current_user_provider in a few ways:

  • Introduce a generic api_parameter_allowed? method which checks for whitelisted routes/formats
  • Only read the api_key parameter on allowed routes. It is now completely ignored on other routes (previously it would raise a 403)
  • Start reading user_api_key parameter on allowed routes
  • Refactor tests as end-end integration tests

A plugin API for PARAMETER_API_PATTERNS will be added soon

diff --git a/lib/auth/default_current_user_provider.rb b/lib/auth/default_current_user_provider.rb
index 8121724..ad94861 100644
--- a/lib/auth/default_current_user_provider.rb
+++ b/lib/auth/default_current_user_provider.rb
@@ -9,6 +9,7 @@ class Auth::DefaultCurrentUserProvider
   HEADER_API_USERNAME ||= "HTTP_API_USERNAME"
   HEADER_API_USER_EXTERNAL_ID ||= "HTTP_API_USER_EXTERNAL_ID"
   HEADER_API_USER_ID ||= "HTTP_API_USER_ID"
+  PARAMETER_USER_API_KEY ||= "user_api_key"
   USER_API_KEY ||= "HTTP_USER_API_KEY"
   USER_API_CLIENT_ID ||= "HTTP_USER_API_CLIENT_ID"
   API_KEY_ENV ||= "_DISCOURSE_API"
@@ -18,6 +19,35 @@ class Auth::DefaultCurrentUserProvider
   COOKIE_ATTEMPTS_PER_MIN ||= 10
   BAD_TOKEN ||= "_DISCOURSE_BAD_TOKEN"
 
+  PARAMETER_API_PATTERNS = [
+    {
+      method: :get,
+      route: [
+        "posts#latest",
+        "posts#user_topics_feed",
+        "posts#user_posts_feed",
+        "groups#posts_feed",
+        "groups#mentions_feed",
+        "list#category_feed",
+        "list#latest_feed",
+        "list#new_feed",
+        "list#unread_feed",
+        "topics#feed"
+      ],
+      format: :rss
+    },
+    {
+      method: :get,
+      route: "users#bookmarks",
+      format: :ics
+    },
+    {
+      method: :post,
+      route: "admin/email#handle_mail",
+      format: "*"
+    }
+  ]
+
   # do all current user initialization here
   def initialize(env)
     @env = env
@@ -42,7 +72,15 @@ class Auth::DefaultCurrentUserProvider
     request = @request
 
     user_api_key = @env[USER_API_KEY]
-    api_key = @env.blank? ? nil : @env[HEADER_API_KEY] || request[API_KEY]
+    api_key = @env[HEADER_API_KEY]
+
+    if !@env.blank? && request[PARAMETER_USER_API_KEY] && api_parameter_allowed?
+      user_api_key ||= request[PARAMETER_USER_API_KEY]
+    end
+
+    if !@env.blank? && request[API_KEY] && api_parameter_allowed?
+      api_key ||= request[API_KEY]
+    end
 
     auth_token = request.cookies[TOKEN_COOKIE] unless user_api_key || api_key
 
@@ -283,10 +321,6 @@ class Auth::DefaultCurrentUserProvider
     if api_key = ApiKey.active.with_key(api_key_value).includes(:user).first
       api_username = header_api_key? ? @env[HEADER_API_USERNAME] : request[API_USERNAME]
 
-      if !header_api_key?
-        raise Discourse::InvalidAccess unless is_whitelisted_query_param_auth_route?(request)
-      end
-
       if api_key.allowed_ips.present? && !api_key.allowed_ips.any? { |ip| ip.include?(request.ip) }
         Rails.logger.warn("[Unauthorized API Access] username: #{api_username}, IP address: #{request.ip}")
         return nil
@@ -313,18 +347,21 @@ class Auth::DefaultCurrentUserProvider
 
   private
 
-  def is_whitelisted_query_param_auth_route?(request)
-    (is_user_feed?(request) || is_handle_mail?(request))
-  end
+  # By default we only allow headers for sending API credentials
+  # However, in some scenarios it is essential to send them via url parameters
+  # so we need to add some exceptions
+  def api_parameter_allowed?
+    request_method = @env["REQUEST_METHOD"]&.downcase&.to_sym
+    request_format = @env['action_dispatch.request.formats']&.first&.symbol
 
-  def is_user_feed?(request)
-    return true if request.path.match?(/\/(c|t){1}\/\S*.(rss|json)/) && request.get? # topic or category route
-    return true if request.path.match?(/\/(latest|top|categories).(rss|json)/) && request.get? # specific routes with rss
-    return true if request.path.match?(/\/u\/\S*\/bookmarks.(ics|json)/) && request.get? # specific routes with ics
-  end
+    path_params = @env['action_dispatch.request.path_parameters']
+    request_route = "#{path_params[:controller]}##{path_params[:action]}" if path_params
 
-  def is_handle_mail?(request)
-    return true if request.path == "/admin/email/handle_mail" && request.post?
+    PARAMETER_API_PATTERNS.any? do |p|
+      (p[:method] == "*" || Array(p[:method]).include?(request_method)) &&
+      (p[:format] == "*" || Array(p[:format]).include?(request_format)) &&
+      (p[:route] == "*" || Array(p[:route]).include?(request_route))
+    end
   end
 
   def header_api_key?
diff --git a/spec/components/auth/default_current_user_provider_spec.rb b/spec/components/auth/default_current_user_provider_spec.rb
index 141382d..1b698cb 100644
--- a/spec/components/auth/default_current_user_provider_spec.rb
+++ b/spec/components/auth/default_current_user_provider_spec.rb
@@ -22,50 +22,7 @@ describe Auth::DefaultCurrentUserProvider do
     expect(provider.current_user).to eq(nil)
   end
 
-  context "server api" do
-
-    it "raises errors for incorrect api_key" do
-      expect {
-        provider("/?api_key=INCORRECT").current_user
-      }.to raise_error(Discourse::InvalidAccess, /API username or key is invalid/)
-    end
-
-    it "finds a user for a correct per-user api key" do
-      user = Fabricate(:user)
-      api_key = ApiKey.create!(user_id: user.id, created_by_id: -1)
-      params = { "HTTP_API_KEY" => api_key.key }
-      good_provider = provider("/", params)
-      expect(good_provider.current_user.id).to eq(user.id)
-      expect(good_provider.is_api?).to eq(true)
-      expect(good_provider.is_user_api?).to eq(false)
-      expect(good_provider.should_update_last_seen?).to eq(false)
-
-      user.update_columns(active: false)
-
-      expect {
-        provider("/?api_key=#{api_key.key}").current_user
-      }.to raise_error(Discourse::InvalidAccess)
-
-      user.update_columns(active: true, suspended_till: 1.day.from_now)
-
-      expect {
-        provider("/?api_key=#{api_key.key}").current_user
-      }.to raise_error(Discourse::InvalidAccess)
-    end
-
-    it "raises for a user pretending" do
-      user = Fabricate(:user)
-      user2 = Fabricate(:user)
-      key = ApiKey.create!(user_id: user.id, created_by_id: -1)
-
-      expect {
-        provider("/?api_key=#{key.key}&api_username=#{user2.username.downcase}").current_user
-      }.to raise_error(Discourse::InvalidAccess)
-
-      key.reload
-      expect(key.last_used_at).to eq(nil)
-    end
-
+  context "server header api" do
     it "raises for a revoked key" do
       user = Fabricate(:user)
       api_key = ApiKey.create!
@@ -86,187 +43,6 @@ describe Auth::DefaultCurrentUserProvider do
       expect(api_key.last_used_at).to eq(nil)
     end
 
-    it "raises for a user with a mismatching ip" do
-      user = Fabricate(:user)
-      api_key = ApiKey.create!(user_id: user.id, created_by_id: -1, allowed_ips: ['10.0.0.0/24'])
-
-      expect {
-        provider("/?api_key=#{api_key.key}&api_username=#{user.username.downcase}", "REMOTE_ADDR" => "10.1.0.1").current_user
-      }.to raise_error(Discourse::InvalidAccess)
-
-    end
-
-    it "allows a user with a matching ip" do
-      freeze_time
-
-      user = Fabricate(:user)
-      api_key = ApiKey.create!(user_id: user.id, created_by_id: -1, allowed_ips: ['100.0.0.0/24'])
-      params = {
-        "HTTP_API_USERNAME" => user.username.downcase,
-        "HTTP_API_KEY" => api_key.key,
-        "REMOTE_ADDR" => "100.0.0.22"
-      }
-
-      found_user = provider("/", params).current_user
-
-      expect(found_user.id).to eq(user.id)
-
-      params = {
-        "HTTP_API_USERNAME" => user.username.downcase,
-        "HTTP_API_KEY" => api_key.key,
-        "HTTP_X_FORWARDED_FOR" => "10.1.1.1, 100.0.0.22"
-      }
-
-      found_user = provider("/", params).current_user
-      expect(found_user.id).to eq(user.id)
-
-      api_key.reload
-      expect(api_key.last_used_at).to eq_time(Time.zone.now)
-    end
-
-    it "finds a user for a correct system api key" do
-      user = Fabricate(:user)
-      api_key = ApiKey.create!(created_by_id: -1)
-      params = { "HTTP_API_USERNAME" => user.username.downcase, "HTTP_API_KEY" => api_key.key }
-      expect(provider("/", params).current_user.id).to eq(user.id)
-    end
-
-    it "raises for a mismatched api_key param and header username" do
-      user = Fabricate(:user)

[... diff too long, it was truncated ...]

GitHub sha: 6230f5c5

This commit appears in #9742 which was merged by davidtaylorhq.