FIX: prevent mixed api auth headers & query params

When using the api and you provide an http header based api key any other
auth based information (username, external_id, or user_id) passed in as
query params will not be used and vice versa.

Followup to f03b293e6a

lib/auth/default_current_user_provider.rb

@@ -45,7 +45,7 @@ class Auth::DefaultCurrentUserProvider
     request = @request
 
     user_api_key = @env[USER_API_KEY]
-    api_key = @env.blank? ? nil : request[API_KEY] || @env[HEADER_API_KEY]
+    api_key = @env.blank? ? nil : @env[HEADER_API_KEY] || request[API_KEY]
 
     auth_token = request.cookies[TOKEN_COOKIE] unless user_api_key || api_key
 
@@ -284,7 +284,7 @@ class Auth::DefaultCurrentUserProvider
 
   def lookup_api_user(api_key_value, request)
     if api_key = ApiKey.where(key: api_key_value).includes(:user).first
-      api_username = request[API_USERNAME] || @env[HEADER_API_USERNAME]
+      api_username = header_api_key? ? @env[HEADER_API_USERNAME] : request[API_USERNAME]
 
       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}")
@@ -295,9 +295,9 @@ class Auth::DefaultCurrentUserProvider
         api_key.user if !api_username || (api_key.user.username_lower == api_username.downcase)
       elsif api_username
         User.find_by(username_lower: api_username.downcase)
-      elsif user_id = request["api_user_id"] || @env[HEADER_API_USER_ID]
+      elsif user_id = header_api_key? ? @env[HEADER_API_USER_ID] : request["api_user_id"]
         User.find_by(id: user_id.to_i)
-      elsif external_id = request["api_user_external_id"] || @env[HEADER_API_USER_EXTERNAL_ID]
+      elsif external_id = header_api_key? ? @env[HEADER_API_USER_EXTERNAL_ID] : request["api_user_external_id"]
         SingleSignOnRecord.find_by(external_id: external_id.to_s).try(:user)
       end
     end
@@ -305,6 +305,10 @@ class Auth::DefaultCurrentUserProvider
 
   private
 
+  def header_api_key?
+    !!@env[HEADER_API_KEY]
+  end
+
   def rate_limit_admin_api_requests(api_key)
     return if Rails.env == "profile"
 

spec/components/auth/default_current_user_provider_spec.rb

@@ -92,6 +92,15 @@ describe Auth::DefaultCurrentUserProvider do
       expect(provider("/?api_key=hello&api_username=#{user.username.downcase}").current_user.id).to eq(user.id)
     end
 
+    it "raises for a mismatched api_key param and header username" do
+      user = Fabricate(:user)
+      ApiKey.create!(key: "hello", created_by_id: -1)
+      params = { "HTTP_API_USERNAME" => user.username.downcase }
+      expect {
+        provider("/?api_key=hello", params).current_user
+      }.to raise_error(Discourse::InvalidAccess)
+    end
+
     it "finds a user for a correct system api key with external id" do
       user = Fabricate(:user)
       ApiKey.create!(key: "hello", created_by_id: -1)
@@ -99,12 +108,31 @@ describe Auth::DefaultCurrentUserProvider do
       expect(provider("/?api_key=hello&api_user_external_id=abc").current_user.id).to eq(user.id)
     end
 
+    it "raises for a mismatched api_key param and header external id" do
+      user = Fabricate(:user)
+      ApiKey.create!(key: "hello", created_by_id: -1)
+      SingleSignOnRecord.create(user_id: user.id, external_id: "abc", last_payload: '')
+      params = { "HTTP_API_USER_EXTERNAL_ID" => "abc" }
+      expect {
+        provider("/?api_key=hello", params).current_user
+      }.to raise_error(Discourse::InvalidAccess)
+    end
+
     it "finds a user for a correct system api key with id" do
       user = Fabricate(:user)
       ApiKey.create!(key: "hello", created_by_id: -1)
       expect(provider("/?api_key=hello&api_user_id=#{user.id}").current_user.id).to eq(user.id)
     end
 
+    it "raises for a mismatched api_key param and header user id" do
+      user = Fabricate(:user)
+      ApiKey.create!(key: "hello", created_by_id: -1)
+      params = { "HTTP_API_USER_ID" => user.id }
+      expect {
+        provider("/?api_key=hello", params).current_user
+      }.to raise_error(Discourse::InvalidAccess)
+    end
+
     context "rate limiting" do
       before do
         RateLimiter.enable
@@ -243,6 +271,15 @@ describe Auth::DefaultCurrentUserProvider do
       expect(provider("/", params).current_user.id).to eq(user.id)
     end
 
+    it "raises for a mismatched api_key header and param username" do
+      user = Fabricate(:user)
+      ApiKey.create!(key: "hello", created_by_id: -1)
+      params = { "HTTP_API_KEY" => "hello" }
+      expect {
+        provider("/?api_username=#{user.username.downcase}", params).current_user
+      }.to raise_error(Discourse::InvalidAccess)
+    end
+
     it "finds a user for a correct system api key with external id" do
       user = Fabricate(:user)
       ApiKey.create!(key: "hello", created_by_id: -1)
@@ -251,6 +288,16 @@ describe Auth::DefaultCurrentUserProvider do
       expect(provider("/", params).current_user.id).to eq(user.id)
     end
 
+    it "raises for a mismatched api_key header and param external id" do
+      user = Fabricate(:user)
+      ApiKey.create!(key: "hello", created_by_id: -1)
+      SingleSignOnRecord.create(user_id: user.id, external_id: "abc", last_payload: '')
+      params = { "HTTP_API_KEY" => "hello" }
+      expect {
+        provider("/?api_user_external_id=abc", params).current_user
+      }.to raise_error(Discourse::InvalidAccess)
+    end
+
     it "finds a user for a correct system api key with id" do
       user = Fabricate(:user)
       ApiKey.create!(key: "hello", created_by_id: -1)
@@ -258,6 +305,15 @@ describe Auth::DefaultCurrentUserProvider do
       expect(provider("/", params).current_user.id).to eq(user.id)
     end
 
+    it "raises for a mismatched api_key header and param user id" do
+      user = Fabricate(:user)
+      ApiKey.create!(key: "hello", created_by_id: -1)
+      params = { "HTTP_API_KEY" => "hello" }
+      expect {
+        provider("/?api_user_id=#{user.id}", params).current_user
+      }.to raise_error(Discourse::InvalidAccess)
+    end
+
     context "rate limiting" do
       before do
         RateLimiter.enable