DEPRECATION: Remove support for api creds in query params (#9106)

DEPRECATION: Remove support for api creds in query params (#9106)

  • DEPRECATION: Remove support for api creds in query params This commit removes support for api credentials in query params except for a few whitelisted routes like rss/json feeds and the handle_mail route.

Several tests were written to valid these changes, but the bulk of the spec changes are just switching them over to use header based auth so that they will pass without changing what they were actually testing.

Original commit that notified admins this change was coming was created over 3 months ago: 2db20031879dbafd1a90cbb1a43bca55d51c1b08

  • fix tests

  • Also allow iCalendar feeds

Co-authored-by: Rafael dos Santos Silva xfalcox@gmail.com

diff --git a/lib/auth/default_current_user_provider.rb b/lib/auth/default_current_user_provider.rb
index 3e293dc..c43580c 100644
--- a/lib/auth/default_current_user_provider.rb
+++ b/lib/auth/default_current_user_provider.rb
@@ -286,12 +286,8 @@ 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]
 
-      # Check for deprecated api auth
       if !header_api_key?
-        unless is_whitelisted_query_param_auth_route?(request)
-          # Notify admins of deprecated auth method
-          AdminDashboardData.add_problem_message('dashboard.deprecated_api_usage', 1.day)
-        end
+        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) }
@@ -321,12 +317,13 @@ class Auth::DefaultCurrentUserProvider
   private
 
   def is_whitelisted_query_param_auth_route?(request)
-    (is_rss_feed?(request) || is_handle_mail?(request))
+    (is_user_feed?(request) || is_handle_mail?(request))
   end
 
-  def is_rss_feed?(request)
+  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
 
   def is_handle_mail?(request)
diff --git a/spec/components/auth/default_current_user_provider_spec.rb b/spec/components/auth/default_current_user_provider_spec.rb
index f0c6fbc..621ece6 100644
--- a/spec/components/auth/default_current_user_provider_spec.rb
+++ b/spec/components/auth/default_current_user_provider_spec.rb
@@ -33,7 +33,8 @@ describe Auth::DefaultCurrentUserProvider do
     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)
-      good_provider = provider("/?api_key=#{api_key.key}")
+      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)
@@ -67,20 +68,22 @@ describe Auth::DefaultCurrentUserProvider do
 
     it "raises for a revoked key" do
       user = Fabricate(:user)
-      key = ApiKey.create!
+      api_key = ApiKey.create!
+      params = { "HTTP_API_USERNAME" => user.username.downcase, "HTTP_API_KEY" => api_key.key }
       expect(
-        provider("/?api_key=#{key.key}&api_username=#{user.username.downcase}").current_user.id
+        provider("/", params).current_user.id
       ).to eq(user.id)
 
-      key.reload.update(revoked_at: Time.zone.now, last_used_at: nil)
-      expect(key.reload.last_used_at).to eq(nil)
+      api_key.reload.update(revoked_at: Time.zone.now, last_used_at: nil)
+      expect(api_key.reload.last_used_at).to eq(nil)
+      params = { "HTTP_API_USERNAME" => user.username.downcase, "HTTP_API_KEY" => api_key.key }
 
       expect {
-        provider("/?api_key=#{key.key}&api_username=#{user.username.downcase}").current_user
+        provider("/", params).current_user
       }.to raise_error(Discourse::InvalidAccess)
 
-      key.reload
-      expect(key.last_used_at).to eq(nil)
+      api_key.reload
+      expect(api_key.last_used_at).to eq(nil)
     end
 
     it "raises for a user with a mismatching ip" do
@@ -97,25 +100,35 @@ describe Auth::DefaultCurrentUserProvider do
       freeze_time
 
       user = Fabricate(:user)
-      key = ApiKey.create!(user_id: user.id, created_by_id: -1, allowed_ips: ['100.0.0.0/24'])
+      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("/?api_key=#{key.key}&api_username=#{user.username.downcase}",
-                            "REMOTE_ADDR" => "100.0.0.22").current_user
+      found_user = provider("/", params).current_user
 
       expect(found_user.id).to eq(user.id)
 
-      found_user = provider("/?api_key=#{key.key}&api_username=#{user.username.downcase}",
-                            "HTTP_X_FORWARDED_FOR" => "10.1.1.1, 100.0.0.22").current_user
+      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)
 
-      key.reload
-      expect(key.last_used_at).to eq_time(Time.zone.now)
+      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)
-      expect(provider("/?api_key=#{api_key.key}&api_username=#{user.username.downcase}").current_user.id).to eq(user.id)
+      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
@@ -130,8 +143,9 @@ describe Auth::DefaultCurrentUserProvider do
     it "finds a user for a correct system api key with external id" do
       user = Fabricate(:user)
       api_key = ApiKey.create!(created_by_id: -1)
+      params = { "HTTP_API_KEY" => api_key.key, "HTTP_API_USER_EXTERNAL_ID" => "abc" }
       SingleSignOnRecord.create(user_id: user.id, external_id: "abc", last_payload: '')
-      expect(provider("/?api_key=#{api_key.key}&api_user_external_id=abc").current_user.id).to eq(user.id)
+      expect(provider("/", params).current_user.id).to eq(user.id)
     end
 
     it "raises for a mismatched api_key param and header external id" do
@@ -147,7 +161,8 @@ describe Auth::DefaultCurrentUserProvider do
     it "finds a user for a correct system api key with id" do
       user = Fabricate(:user)
       api_key = ApiKey.create!(created_by_id: -1)
-      expect(provider("/?api_key=#{api_key.key}&api_user_id=#{user.id}").current_user.id).to eq(user.id)
+      params = { "HTTP_API_USER_ID" => user.id, "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 user id" do
@@ -176,36 +191,80 @@ describe Auth::DefaultCurrentUserProvider do
         user = Fabricate(:user)
         api_key = ApiKey.create!(created_by_id: -1)
         key = api_key.key
+        user_params = { "HTTP_API_KEY" => key, "HTTP_API_USERNAME" => user.username.downcase }
+        system_params = { "HTTP_API_KEY" => key, "HTTP_API_USERNAME" => "system" }
 
-        provider("/?api_key=#{key}&api_username=#{user.username.downcase}").current_user
-        provider("/?api_key=#{key}&api_username=system").current_user
-        provider("/?api_key=#{key}&api_username=#{user.username.downcase}").current_user
+        provider("/", user_params).current_user
+        provider("/", system_params).current_user
+        provider("/", user_params).current_user
 
         expect do
-          provider("/?api_key=#{key}&api_username=system").current_user
+          provider("/", system_params).current_user
         end.to raise_error(RateLimiter::LimitExceeded)
 
         freeze_time 59.seconds.from_now
 
         expect do
-          provider("/?api_key=#{key}&api_username=system").current_user
+          provider("/", system_params).current_user

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

GitHub sha: d04ba4b3

This commit appears in #9106 which was merged by blake.

I’m slightly concerned that tests were updated at the same time as the removal. I fully expect a request soon for a site setting to allow param keys for all API requests on a site, default off. This would ensure people are actively aware of the problem because they were forced to take an action to fix it.