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
This commit is contained in:
David Taylor
GitHub
parent
16137308b0
commit
6230f5c554
|
|
@ -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?
|
||||
|
|
|
|||
|
|
@ -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 {
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
@ -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
|
||||
|
|
@ -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"))
|
||||
|
|
|
|||
Loading…
Reference in New Issue