FEATURE: Header based auth for API requests (#7129)

FEATURE: Header based auth for API requests (#7129)

Now you can also make authenticated API requests by passing the api_key and api_username in the HTTP header instead of query params.

The new header values are: Api-key and Api-Username.

Here is an example in cURL:

curl -i -sS -X POST "http://127.0.0.1:3000/categories" \
  -H "Content-Type: multipart/form-data;" \
  -H "Api-Key: 7aa202bec1ff70563bc0a3d102feac0a7dd2af96b5b772a9feaf27485f9d31a2" \
  -H "Api-Username: system" \
  -F "name=7c1c0ed93583cba7124b745d1bd56b32" \
  -F "color=49d9e9" \
  -F "text_color=f0fcfd"

There is also support for Api-User-Id and Api-User-External-Id instead of specifying the username along with the key.

diff --git a/lib/auth/default_current_user_provider.rb b/lib/auth/default_current_user_provider.rb
index 7f1bf24..8404ee6 100644
--- a/lib/auth/default_current_user_provider.rb
+++ b/lib/auth/default_current_user_provider.rb
@@ -7,6 +7,11 @@ class Auth::DefaultCurrentUserProvider
 
   CURRENT_USER_KEY ||= "_DISCOURSE_CURRENT_USER"
   API_KEY ||= "api_key"
+  API_USERNAME ||= "api_username"
+  HEADER_API_KEY ||= "HTTP_API_KEY"
+  HEADER_API_USERNAME ||= "HTTP_API_USERNAME"
+  HEADER_API_USER_EXTERNAL_ID ||= "HTTP_API_USER_EXTERNAL_ID"
+  HEADER_API_USER_ID ||= "HTTP_API_USER_ID"
   USER_API_KEY ||= "HTTP_USER_API_KEY"
   USER_API_CLIENT_ID ||= "HTTP_USER_API_CLIENT_ID"
   API_KEY_ENV ||= "_DISCOURSE_API"
@@ -40,7 +45,7 @@ class Auth::DefaultCurrentUserProvider
     request = @request
 
     user_api_key = @env[USER_API_KEY]
-    api_key = @env.blank? ? nil : request[API_KEY]
+    api_key = @env.blank? ? nil : request[API_KEY] || @env[HEADER_API_KEY]
 
     auth_token = request.cookies[TOKEN_COOKIE] unless user_api_key || api_key
 
@@ -279,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"]
+      api_username = request[API_USERNAME] || @env[HEADER_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}")
@@ -290,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"]
+      elsif user_id = request["api_user_id"] || @env[HEADER_API_USER_ID]
         User.find_by(id: user_id.to_i)
-      elsif external_id = request["api_user_external_id"]
+      elsif external_id = request["api_user_external_id"] || @env[HEADER_API_USER_EXTERNAL_ID]
         SingleSignOnRecord.find_by(external_id: external_id.to_s).try(:user)
       end
     end
diff --git a/spec/components/auth/default_current_user_provider_spec.rb b/spec/components/auth/default_current_user_provider_spec.rb
index 1f6ac6b..ed58233 100644
--- a/spec/components/auth/default_current_user_provider_spec.rb
+++ b/spec/components/auth/default_current_user_provider_spec.rb
@@ -153,6 +153,162 @@ describe Auth::DefaultCurrentUserProvider do
 
   end
 
+  context "server header api" do
+
+    it "raises errors for incorrect api_key" do
+      params = { "HTTP_API_KEY" => "INCORRECT" }
+      expect {
+        provider("/", params).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)
+      ApiKey.create!(key: "hello", user_id: user.id, created_by_id: -1)
+      params = { "HTTP_API_KEY" => "hello" }
+
+      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("/", params).current_user
+      }.to raise_error(Discourse::InvalidAccess)
+
+      user.update_columns(active: true, suspended_till: 1.day.from_now)
+
+      expect {
+        provider("/", params).current_user
+      }.to raise_error(Discourse::InvalidAccess)
+    end
+
+    it "raises for a user pretending" do
+      user = Fabricate(:user)
+      user2 = Fabricate(:user)
+      ApiKey.create!(key: "hello", user_id: user.id, created_by_id: -1)
+      params = { "HTTP_API_KEY" => "hello", "HTTP_API_USERNAME" => user2.username.downcase }
+
+      expect {
+        provider("/", params).current_user
+      }.to raise_error(Discourse::InvalidAccess)
+    end
+
+    it "raises for a user with a mismatching ip" do
+      user = Fabricate(:user)
+      ApiKey.create!(key: "hello", user_id: user.id, created_by_id: -1, allowed_ips: ['10.0.0.0/24'])
+      params = {
+        "HTTP_API_KEY" => "hello",
+        "HTTP_API_USERNAME" => user.username.downcase,
+        "REMOTE_ADDR" => "10.1.0.1"
+      }
+
+      expect {
+        provider("/", params).current_user
+      }.to raise_error(Discourse::InvalidAccess)
+
+    end
+
+    it "allows a user with a matching ip" do
+      user = Fabricate(:user)
+      ApiKey.create!(key: "hello", user_id: user.id, created_by_id: -1, allowed_ips: ['100.0.0.0/24'])
+      params = {
+        "HTTP_API_KEY" => "hello",
+        "HTTP_API_USERNAME" => user.username.downcase,
+        "REMOTE_ADDR" => "100.0.0.22",
+      }
+
+      found_user = provider("/", params).current_user
+
+      expect(found_user.id).to eq(user.id)
+
+      params = {
+        "HTTP_API_KEY" => "hello",
+        "HTTP_API_USERNAME" => user.username.downcase,
+        "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)
+
+    end
+
+    it "finds a user for a correct system api key" do
+      user = Fabricate(:user)
+      ApiKey.create!(key: "hello", created_by_id: -1)
+      params = { "HTTP_API_KEY" => "hello", "HTTP_API_USERNAME" => user.username.downcase }
+      expect(provider("/", params).current_user.id).to eq(user.id)
+    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)
+      SingleSignOnRecord.create(user_id: user.id, external_id: "abc", last_payload: '')
+      params = { "HTTP_API_KEY" => "hello", "HTTP_API_USER_EXTERNAL_ID" => "abc" }
+      expect(provider("/", params).current_user.id).to eq(user.id)
+    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)
+      params = { "HTTP_API_KEY" => "hello", "HTTP_API_USER_ID" => user.id }
+      expect(provider("/", params).current_user.id).to eq(user.id)
+    end
+
+    context "rate limiting" do
+      before do
+        RateLimiter.enable
+      end
+
+      after do
+        RateLimiter.disable
+      end
+
+      it "rate limits api requests per api key" do
+        global_setting :max_admin_api_reqs_per_key_per_minute, 3
+
+        freeze_time
+
+        user = Fabricate(:user)
+        key = SecureRandom.hex
+        api_key = ApiKey.create!(key: key, created_by_id: -1)
+        params = { "HTTP_API_KEY" => key, "HTTP_API_USERNAME" => user.username.downcase }
+        system_params = params.merge("HTTP_API_USERNAME" => "system")
+
+        provider("/", params).current_user
+        provider("/", system_params).current_user
+        provider("/", params).current_user
+
+        expect do
+          provider("/", system_params).current_user
+        end.to raise_error(RateLimiter::LimitExceeded)
+
+        freeze_time 59.seconds.from_now
+
+        expect do
+          provider("/", system_params).current_user
+        end.to raise_error(RateLimiter::LimitExceeded)
+
+        freeze_time 2.seconds.from_now
+
+        # 1 minute elapsed
+        provider("/", system_params).current_user
+
+        # should not rate limit a random key
+        api_key.destroy
+        key = SecureRandom.hex
+        ApiKey.create!(key: key, created_by_id: -1)
+        params = { "HTTP_API_KEY" => key, "HTTP_API_USERNAME" => user.username.downcase }
+        provider("/", params).current_user
+
+      end
+    end
+
+  end
+
   it "should not update last seen for ajax calls without Discourse-Visible header" do
     expect(provider("/topic/anything/goes",
                     :method => "POST",

GitHub sha: f03b293e

One thing I think about here is how much support do we want for mix-and-match where part is coming from headers and another part is coming from query params.

My call on this is that we should simply not allow mix-and-match. Everything should either come from headers OR query params, not both.

So after you grabbed HEADER_API_KEY you should switch this to header mode and further down only use headers.

Other than this is looks great!!

2 Likes

FIX: prevent mixed api auth headers & query params