From f03b293e6a5f12f12ba2a61ab2bc2cfb8a7f1a63 Mon Sep 17 00:00:00 2001 From: Blake Erickson Date: Fri, 8 Mar 2019 09:13:31 -0700 Subject: [PATCH] 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: ``` text 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. --- lib/auth/default_current_user_provider.rb | 13 +- .../default_current_user_provider_spec.rb | 156 ++++++++++++++++++ 2 files changed, 165 insertions(+), 4 deletions(-) diff --git a/lib/auth/default_current_user_provider.rb b/lib/auth/default_current_user_provider.rb index 7f1bf243ce8..8404ee6694b 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 1f6ac6bb2d3..ed582333d82 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",