From 6230f5c5543a294b701290e913d8db9cf777970f Mon Sep 17 00:00:00 2001 From: David Taylor Date: Tue, 12 May 2020 13:35:36 +0100 Subject: [PATCH] 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 --- lib/auth/default_current_user_provider.rb | 67 ++++-- .../default_current_user_provider_spec.rb | 226 +----------------- spec/integration/api_keys_spec.rb | 89 +++++++ spec/integration/user_api_keys_spec.rb | 18 -- spec/requests/topics_controller_spec.rb | 4 +- 5 files changed, 144 insertions(+), 260 deletions(-) create mode 100644 spec/integration/api_keys_spec.rb delete mode 100644 spec/integration/user_api_keys_spec.rb diff --git a/lib/auth/default_current_user_provider.rb b/lib/auth/default_current_user_provider.rb index 81217248a5d..ad94861cf7e 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 141382da8cd..1b698cbf368 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) - api_key = ApiKey.create!(created_by_id: -1) - params = { "HTTP_API_USERNAME" => user.username.downcase } - expect { - provider("/?api_key=#{api_key.key}", 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) - 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("/", params).current_user.id).to eq(user.id) - end - - it "raises for a mismatched api_key param and header external id" do - user = Fabricate(:user) - api_key = ApiKey.create!(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=#{api_key.key}", 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) - api_key = ApiKey.create!(created_by_id: -1) - 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 - user = Fabricate(:user) - api_key = ApiKey.create!(created_by_id: -1) - params = { "HTTP_API_USER_ID" => user.id } - expect { - provider("/?api_key=#{api_key.key}", params).current_user - }.to raise_error(Discourse::InvalidAccess) - 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) - 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("/", user_params).current_user - provider("/", system_params).current_user - provider("/", user_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 rake limit a random key - api_key.destroy - api_key = ApiKey.create!(created_by_id: -1) - user_params = { "HTTP_API_KEY" => api_key.key, "HTTP_API_USERNAME" => user.username.downcase } - provider("/", user_params).current_user - - end - end - - end - - context "whitelisted api auth query param routes" do - - it "allows rss feeds" do - user = Fabricate(:user) - api_key = ApiKey.create!(user_id: user.id, created_by_id: -1) - url = "/latest.rss?api_key=#{api_key.key}&api_username=#{user.username.downcase}" - expect(provider(url).current_user.id).to eq(user.id) - end - - it "allows ics feeds" do - user = Fabricate(:user) - api_key = ApiKey.create!(user_id: user.id, created_by_id: -1) - url = "/u/#{user.username}/bookmarks.ics?api_key=#{api_key.key}&api_username=#{user.username.downcase}" - expect(provider(url).current_user.id).to eq(user.id) - end - - it "allows handle mail route" do - user = Fabricate(:user) - api_key = ApiKey.create!(user_id: user.id, created_by_id: -1) - url = "/admin/email/handle_mail?api_key=#{api_key.key}&api_username=#{user.username.downcase}" - opts = { method: "POST" } - expect(provider(url, opts).current_user.id).to eq(user.id) - end - - it "raises errors for non whitlisted routes" do - user = Fabricate(:user) - api_key = ApiKey.create!(user_id: user.id, created_by_id: -1) - url = "/u?api_key=#{api_key.key}&api_username=#{user.username.downcase}" - expect { - provider(url).current_user - }.to raise_error(Discourse::InvalidAccess) - end - - it "still allows header based auth" do - user = Fabricate(:user) - api_key = ApiKey.create!(user_id: user.id, created_by_id: -1) - params = { "HTTP_API_KEY" => api_key.key, "HTTP_API_USERNAME" => user.username.downcase } - expect(provider("/", params).current_user.id).to eq(user.id) - end - end - - context "server header api" do - it "raises errors for incorrect api_key" do params = { "HTTP_API_KEY" => "INCORRECT" } expect { diff --git a/spec/integration/api_keys_spec.rb b/spec/integration/api_keys_spec.rb new file mode 100644 index 00000000000..451768718fc --- /dev/null +++ b/spec/integration/api_keys_spec.rb @@ -0,0 +1,89 @@ +# frozen_string_literal: true + +require 'rails_helper' + +describe 'api keys' do + let(:user) { Fabricate(:user) } + let(:api_key) { ApiKey.create!(user_id: user.id, created_by_id: Discourse.system_user) } + + it 'works in headers' do + get '/session/current.json', headers: { + HTTP_API_KEY: api_key.key + } + expect(response.status).to eq(200) + expect(response.parsed_body["current_user"]["username"]).to eq(user.username) + end + + it 'does not work in parameters' do + get '/session/current.json', params: { + api_key: api_key.key + } + expect(response.status).to eq(404) + end + + it 'allows parameters on ics routes' do + get "/u/#{user.username}/bookmarks.ics?api_key=#{api_key.key}&api_username=#{user.username.downcase}" + expect(response.status).to eq(200) + + # Confirm not for JSON + get "/u/#{user.username}/bookmarks.json?api_key=#{api_key.key}&api_username=#{user.username.downcase}" + expect(response.status).to eq(403) + end + + it 'allows parameters for handle mail' do + admin_api_key = ApiKey.create!(user: Fabricate(:admin), created_by_id: Discourse.system_user) + + post "/admin/email/handle_mail.json?api_key=#{admin_api_key.key}", params: { email: "blah" } + expect(response.status).to eq(200) + end + + it 'allows parameters for rss feeds' do + SiteSetting.login_required = true + + get "/latest.rss?api_key=#{api_key.key}&api_username=#{user.username.downcase}" + expect(response.status).to eq(200) + + # Confirm not allowed for json + get "/latest.json?api_key=#{api_key.key}&api_username=#{user.username.downcase}" + expect(response.status).to eq(302) + end + +end + +describe 'user api keys' do + let(:user) { Fabricate(:user) } + let(:user_api_key) { Fabricate(:readonly_user_api_key, user: user) } + + it 'updates last used time on use' do + freeze_time + + user_api_key.update_columns(last_used_at: 7.days.ago) + + get '/session/current.json', headers: { + HTTP_USER_API_KEY: user_api_key.key, + } + + expect(user_api_key.reload.last_used_at).to eq_time(Time.zone.now) + end + + it 'allows parameters on ics routes' do + get "/u/#{user.username}/bookmarks.ics?user_api_key=#{user_api_key.key}" + expect(response.status).to eq(200) + + # Confirm not for JSON + get "/u/#{user.username}/bookmarks.json?user_api_key=#{user_api_key.key}" + expect(response.status).to eq(403) + end + + it 'allows parameters for rss feeds' do + SiteSetting.login_required = true + + get "/latest.rss?user_api_key=#{user_api_key.key}" + expect(response.status).to eq(200) + + # Confirm not allowed for json + get "/latest.json?user_api_key=#{user_api_key.key}" + expect(response.status).to eq(302) + end + +end diff --git a/spec/integration/user_api_keys_spec.rb b/spec/integration/user_api_keys_spec.rb deleted file mode 100644 index 5bbdff319ec..00000000000 --- a/spec/integration/user_api_keys_spec.rb +++ /dev/null @@ -1,18 +0,0 @@ -# frozen_string_literal: true - -require 'rails_helper' - -describe 'user api keys integration' do - it 'updates last used time on use' do - freeze_time - - user_api_key = Fabricate(:readonly_user_api_key) - user_api_key.update_columns(last_used_at: 7.days.ago) - - get '/session/current.json', headers: { - HTTP_USER_API_KEY: user_api_key.key, - } - - expect(user_api_key.reload.last_used_at).to eq_time(Time.zone.now) - end -end diff --git a/spec/requests/topics_controller_spec.rb b/spec/requests/topics_controller_spec.rb index b2c9b5ebdae..cf4f5f15b4f 100644 --- a/spec/requests/topics_controller_spec.rb +++ b/spec/requests/topics_controller_spec.rb @@ -1817,7 +1817,7 @@ RSpec.describe TopicsController do end it 'shows the topic if valid api key is provided' do - get "/t/#{topic.slug}/#{topic.id}.json", params: { api_key: api_key.key } + get "/t/#{topic.slug}/#{topic.id}.json", headers: { "HTTP_API_KEY" => api_key.key } expect(response.status).to eq(200) topic.reload @@ -1826,7 +1826,7 @@ RSpec.describe TopicsController do it 'returns 403 for an invalid key' do [:json, :html].each do |format| - get "/t/#{topic.slug}/#{topic.id}.#{format}", params: { api_key: "bad" } + get "/t/#{topic.slug}/#{topic.id}.#{format}", headers: { "HTTP_API_KEY" => "bad" } expect(response.code.to_i).to eq(403) expect(response.body).to include(I18n.t("invalid_access"))