DEPRECATION: Remove support for api creds in query params (#9106)

* DEPRECATION: Remove support for api creds in query params

This commit removes support for api credentials in query params except
for a few whitelisted routes like rss/json feeds and the handle_mail
route.

Several tests were written to valid these changes, but the bulk of the
spec changes are just switching them over to use header based auth so
that they will pass without changing what they were actually testing.

Original commit that notified admins this change was coming was created
over 3 months ago: 2db2003187

* fix tests

* Also allow iCalendar feeds

Co-authored-by: Rafael dos Santos Silva <xfalcox@gmail.com>
This commit is contained in:
Blake Erickson 2020-04-06 16:55:44 -06:00 committed by GitHub
parent 58bec3b200
commit d04ba4b3b2
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
10 changed files with 170 additions and 143 deletions

View File

@ -286,12 +286,8 @@ 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]
# Check for deprecated api auth
if !header_api_key?
unless is_whitelisted_query_param_auth_route?(request)
# Notify admins of deprecated auth method
AdminDashboardData.add_problem_message('dashboard.deprecated_api_usage', 1.day)
end
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) }
@ -321,12 +317,13 @@ class Auth::DefaultCurrentUserProvider
private
def is_whitelisted_query_param_auth_route?(request)
(is_rss_feed?(request) || is_handle_mail?(request))
(is_user_feed?(request) || is_handle_mail?(request))
end
def is_rss_feed?(request)
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
def is_handle_mail?(request)

View File

@ -33,7 +33,8 @@ describe Auth::DefaultCurrentUserProvider do
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)
good_provider = provider("/?api_key=#{api_key.key}")
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)
@ -67,20 +68,22 @@ describe Auth::DefaultCurrentUserProvider do
it "raises for a revoked key" do
user = Fabricate(:user)
key = ApiKey.create!
api_key = ApiKey.create!
params = { "HTTP_API_USERNAME" => user.username.downcase, "HTTP_API_KEY" => api_key.key }
expect(
provider("/?api_key=#{key.key}&api_username=#{user.username.downcase}").current_user.id
provider("/", params).current_user.id
).to eq(user.id)
key.reload.update(revoked_at: Time.zone.now, last_used_at: nil)
expect(key.reload.last_used_at).to eq(nil)
api_key.reload.update(revoked_at: Time.zone.now, last_used_at: nil)
expect(api_key.reload.last_used_at).to eq(nil)
params = { "HTTP_API_USERNAME" => user.username.downcase, "HTTP_API_KEY" => api_key.key }
expect {
provider("/?api_key=#{key.key}&api_username=#{user.username.downcase}").current_user
provider("/", params).current_user
}.to raise_error(Discourse::InvalidAccess)
key.reload
expect(key.last_used_at).to eq(nil)
api_key.reload
expect(api_key.last_used_at).to eq(nil)
end
it "raises for a user with a mismatching ip" do
@ -97,25 +100,35 @@ describe Auth::DefaultCurrentUserProvider do
freeze_time
user = Fabricate(:user)
key = ApiKey.create!(user_id: user.id, created_by_id: -1, allowed_ips: ['100.0.0.0/24'])
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("/?api_key=#{key.key}&api_username=#{user.username.downcase}",
"REMOTE_ADDR" => "100.0.0.22").current_user
found_user = provider("/", params).current_user
expect(found_user.id).to eq(user.id)
found_user = provider("/?api_key=#{key.key}&api_username=#{user.username.downcase}",
"HTTP_X_FORWARDED_FOR" => "10.1.1.1, 100.0.0.22").current_user
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)
key.reload
expect(key.last_used_at).to eq_time(Time.zone.now)
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)
expect(provider("/?api_key=#{api_key.key}&api_username=#{user.username.downcase}").current_user.id).to eq(user.id)
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
@ -130,8 +143,9 @@ describe Auth::DefaultCurrentUserProvider do
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("/?api_key=#{api_key.key}&api_user_external_id=abc").current_user.id).to eq(user.id)
expect(provider("/", params).current_user.id).to eq(user.id)
end
it "raises for a mismatched api_key param and header external id" do
@ -147,7 +161,8 @@ describe Auth::DefaultCurrentUserProvider do
it "finds a user for a correct system api key with id" do
user = Fabricate(:user)
api_key = ApiKey.create!(created_by_id: -1)
expect(provider("/?api_key=#{api_key.key}&api_user_id=#{user.id}").current_user.id).to eq(user.id)
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
@ -176,36 +191,80 @@ describe Auth::DefaultCurrentUserProvider do
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("/?api_key=#{key}&api_username=#{user.username.downcase}").current_user
provider("/?api_key=#{key}&api_username=system").current_user
provider("/?api_key=#{key}&api_username=#{user.username.downcase}").current_user
provider("/", user_params).current_user
provider("/", system_params).current_user
provider("/", user_params).current_user
expect do
provider("/?api_key=#{key}&api_username=system").current_user
provider("/", system_params).current_user
end.to raise_error(RateLimiter::LimitExceeded)
freeze_time 59.seconds.from_now
expect do
provider("/?api_key=#{key}&api_username=system").current_user
provider("/", system_params).current_user
end.to raise_error(RateLimiter::LimitExceeded)
freeze_time 2.seconds.from_now
# 1 minute elapsed
provider("/?api_key=#{key}&api_username=system").current_user
provider("/", system_params).current_user
# should not rake limit a random key
api_key.destroy
api_key = ApiKey.create!(created_by_id: -1)
provider("/?api_key=#{api_key.key}&api_username=#{user.username.downcase}").current_user
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

View File

@ -19,22 +19,23 @@ describe 'invite only' do
username: 'bob',
password: 'strongpassword',
email: 'bob@bob.com',
api_key: api_key.key,
api_username: admin.username
}, headers: {
HTTP_API_KEY: api_key.key,
HTTP_API_USERNAME: admin.username
}
user_id = JSON.parse(response.body)["user_id"]
expect(user_id).to be > 0
# activate and approve
put "/admin/users/#{user_id}/activate.json", params: {
api_key: api_key.key,
api_username: admin.username
put "/admin/users/#{user_id}/activate.json", headers: {
HTTP_API_KEY: api_key.key,
HTTP_API_USERNAME: admin.username
}
put "/admin/users/#{user_id}/approve.json", params: {
api_key: api_key.key,
api_username: admin.username
put "/admin/users/#{user_id}/approve.json", headers: {
HTTP_API_KEY: api_key.key,
HTTP_API_USERNAME: admin.username
}
u = User.find(user_id)

View File

@ -60,16 +60,16 @@ describe 'rate limiter integration' do
global_setting :max_admin_api_reqs_per_key_per_minute, 1
get '/admin/api/keys.json', params: {
api_key: api_key.key,
api_username: admin.username
get '/admin/api/keys.json', headers: {
HTTP_API_KEY: api_key.key,
HTTP_API_USERNAME: admin.username
}
expect(response.status).to eq(200)
get '/admin/api/keys.json', params: {
api_key: api_key.key,
api_username: admin.username
get '/admin/api/keys.json', headers: {
HTTP_API_KEY: api_key.key,
HTTP_API_USERNAME: admin.username
}
expect(response.status).to eq(429)

View File

@ -254,9 +254,8 @@ RSpec.describe Admin::UsersController do
api_key = Fabricate(:api_key, user: user)
put "/posts/#{Fabricate(:post).id}/bookmark.json", params: {
bookmarked: "true",
api_key: api_key.key
}
bookmarked: "true"
}, headers: { HTTP_API_KEY: api_key.key }
expect(response.status).to eq(200)
put "/admin/users/#{user.id}/suspend.json", params: suspend_params

View File

@ -49,7 +49,8 @@ describe EmbedController do
it "returns information about the topic" do
get '/embed/info.json',
params: { embed_url: topic_embed.embed_url, api_key: api_key.key, api_username: "system" }
params: { embed_url: topic_embed.embed_url },
headers: { HTTP_API_KEY: api_key.key, HTTP_API_USERNAME: "system" }
json = JSON.parse(response.body)
expect(json['topic_id']).to eq(topic_embed.topic.id)
@ -61,7 +62,8 @@ describe EmbedController do
context "without invalid embed url" do
it "returns error response" do
get '/embed/info.json',
params: { embed_url: "http://nope.com", api_key: api_key.key, api_username: "system" }
params: { embed_url: "http://nope.com" },
headers: { HTTP_API_KEY: api_key.key, HTTP_API_USERNAME: "system" }
json = JSON.parse(response.body)
expect(json["error_type"]).to eq("not_found")

View File

@ -556,10 +556,9 @@ describe PostsController do
# choosing an arbitrarily easy to mock trusted activity
it 'allows users with api key to bookmark posts' do
put "/posts/#{post.id}/bookmark.json", params: {
bookmarked: "true",
api_key: api_key.key
}
put "/posts/#{post.id}/bookmark.json",
params: { bookmarked: "true" },
headers: { HTTP_API_KEY: api_key.key }
expect(response.status).to eq(200)
expect(PostAction.where(
@ -570,21 +569,17 @@ describe PostsController do
end
it 'raises an error with a user key that does not match an optionally specified username' do
put "/posts/#{post.id}/bookmark.json", params: {
bookmarked: "true",
api_key: api_key.key,
api_username: 'made_up'
}
put "/posts/#{post.id}/bookmark.json",
params: { bookmarked: "true" },
headers: { HTTP_API_KEY: api_key.key, HTTP_API_USERNAME: 'made_up' }
expect(response.status).to eq(403)
end
it 'allows users with a master api key to bookmark posts' do
put "/posts/#{post.id}/bookmark.json", params: {
bookmarked: "true",
api_key: master_key.key,
api_username: user.username
}
put "/posts/#{post.id}/bookmark.json",
params: { bookmarked: "true" },
headers: { HTTP_API_KEY: master_key.key, HTTP_API_USERNAME: user.username }
expect(response.status).to eq(200)
expect(PostAction.where(
@ -595,21 +590,17 @@ describe PostsController do
end
it 'disallows phonies to bookmark posts' do
put "/posts/#{post.id}/bookmark.json", params: {
bookmarked: "true",
api_key: SecureRandom.hex(32),
api_username: user.username
}
put "/posts/#{post.id}/bookmark.json",
params: { bookmarked: "true" },
headers: { HTTP_API_KEY: SecureRandom.hex(32), HTTP_API_USERNAME: user.username }
expect(response.status).to eq(403)
end
it 'disallows blank api' do
put "/posts/#{post.id}/bookmark.json", params: {
bookmarked: "true",
api_key: "",
api_username: user.username
}
put "/posts/#{post.id}/bookmark.json",
params: { bookmarked: "true" },
headers: { HTTP_API_KEY: "", HTTP_API_USERNAME: user.username }
expect(response.status).to eq(403)
end
@ -740,24 +731,16 @@ describe PostsController do
master_key = Fabricate(:api_key).key
post "/posts.json", params: {
api_username: user.username,
api_key: master_key,
raw: raw,
title: title,
wpid: 1
}
post "/posts.json",
params: { raw: raw, title: title, wpid: 1 },
headers: { HTTP_API_USERNAME: user.username, HTTP_API_KEY: master_key }
expect(response.status).to eq(200)
original = response.body
post "/posts.json", params: {
api_username: user.username_lower,
api_key: master_key,
raw: raw,
title: title,
wpid: 2
}
post "/posts.json",
params: { raw: raw, title: title, wpid: 2 },
headers: { HTTP_API_USERNAME: user.username_lower, HTTP_API_KEY: master_key }
expect(response.status).to eq(200)
expect(response.body).to eq(original)
@ -769,38 +752,24 @@ describe PostsController do
post_1 = Fabricate(:post)
master_key = Fabricate(:api_key).key
post "/posts.json", params: {
api_username: user.username,
api_key: master_key,
raw: 'this is test reply 1',
topic_id: post_1.topic.id,
reply_to_post_number: 1
}
post "/posts.json",
params: { raw: 'this is test reply 1', topic_id: post_1.topic.id, reply_to_post_number: 1 },
headers: { HTTP_API_USERNAME: user.username, HTTP_API_KEY: master_key }
expect(response.status).to eq(200)
expect(post_1.topic.user.notifications.count).to eq(1)
post_1.topic.user.notifications.destroy_all
post "/posts.json", params: {
api_username: user.username,
api_key: master_key,
raw: 'this is test reply 2',
topic_id: post_1.topic.id,
reply_to_post_number: 1,
import_mode: true
}
post "/posts.json",
params: { raw: 'this is test reply 2', topic_id: post_1.topic.id, reply_to_post_number: 1, import_mode: true },
headers: { HTTP_API_USERNAME: user.username, HTTP_API_KEY: master_key }
expect(response.status).to eq(200)
expect(post_1.topic.user.notifications.count).to eq(0)
post "/posts.json", params: {
api_username: user.username,
api_key: master_key,
raw: 'this is test reply 3',
topic_id: post_1.topic.id,
reply_to_post_number: 1,
import_mode: false
}
post "/posts.json",
params: { raw: 'this is test reply 3', topic_id: post_1.topic.id, reply_to_post_number: 1, import_mode: false },
headers: { HTTP_API_USERNAME: user.username, HTTP_API_KEY: master_key }
expect(response.status).to eq(200)
expect(post_1.topic.user.notifications.count).to eq(1)
@ -810,14 +779,10 @@ describe PostsController do
post_1 = Fabricate(:post)
user_key = ApiKey.create!(user: user).key
post "/posts.json", params: {
api_username: user.username,
api_key: user_key,
raw: 'this is test whisper',
topic_id: post_1.topic.id,
reply_to_post_number: 1,
whisper: true
}
post "/posts.json",
params: { raw: 'this is test whisper', topic_id: post_1.topic.id, reply_to_post_number: 1, whisper: true },
headers: { HTTP_API_USERNAME: user.username, HTTP_API_KEY: user_key }
expect(response.status).to eq(403)
end
@ -825,13 +790,9 @@ describe PostsController do
user = Fabricate(:admin)
master_key = Fabricate(:api_key).key
post "/posts.json", params: {
api_username: user.username,
api_key: master_key,
title: 'this is a test title',
raw: 'this is test body',
category: 'invalid'
}
post "/posts.json",
params: { title: 'this is a test title', raw: 'this is test body', category: 'invalid' },
headers: { HTTP_API_USERNAME: user.username, HTTP_API_KEY: master_key }
expect(response.status).to eq(400)

View File

@ -60,7 +60,10 @@ describe UploadsController do
stub_request(:get, url).to_return(status: 200, body: png)
post "/uploads.json", params: { url: url, type: "avatar", api_key: api_key, api_username: user.username }
post "/uploads.json", params: { url: url, type: "avatar" }, headers: {
HTTP_API_KEY: api_key,
HTTP_API_USERNAME: user.username.downcase
}
json = ::JSON.parse(response.body)

View File

@ -123,9 +123,13 @@ describe UserBadgesController do
it 'grants badges from master api calls' do
api_key = Fabricate(:api_key)
post "/user_badges.json", params: {
badge_id: badge.id, username: user.username, api_key: api_key.key, api_username: "system"
}
post "/user_badges.json",
params: {
badge_id: badge.id, username: user.username
},
headers: {
HTTP_API_KEY: api_key.key, HTTP_API_USERNAME: "system"
}
expect(response.status).to eq(200)
user_badge = UserBadge.find_by(user: user, badge: badge)

View File

@ -743,7 +743,7 @@ describe UsersController do
it "won't create the user as active with a regular key" do
post "/u.json",
params: post_user_params.merge(active: true, api_key: api_key.key)
params: post_user_params.merge(active: true), headers: { HTTP_API_KEY: api_key.key }
expect(response.status).to eq(200)
expect(JSON.parse(response.body)['active']).to be_falsey
@ -759,7 +759,7 @@ describe UsersController do
SiteSetting.must_approve_users = true
#Sidekiq::Client.expects(:enqueue).never
post "/u.json", params: post_user_params.merge(approved: true, active: true, api_key: api_key.key)
post "/u.json", params: post_user_params.merge(approved: true, active: true), headers: { HTTP_API_KEY: api_key.key }
expect(Jobs::CriticalUserEmail.jobs.size).to eq(0)
expect(Jobs::SendSystemMessage.jobs.size).to eq(0)
@ -781,7 +781,7 @@ describe UsersController do
Jobs.run_immediately!
SiteSetting.must_approve_users = true
post "/u.json", params: post_user_params.merge(active: true, api_key: api_key.key)
post "/u.json", params: post_user_params.merge(active: true), headers: { HTTP_API_KEY: api_key.key }
expect(response.status).to eq(200)
json = JSON.parse(response.body)
@ -796,7 +796,7 @@ describe UsersController do
Jobs.run_immediately!
SiteSetting.must_approve_users = true
post "/u.json", params: post_user_params.merge(api_key: api_key.key)
post "/u.json", params: post_user_params, headers: { HTTP_API_KEY: api_key.key }
expect(response.status).to eq(200)
json = JSON.parse(response.body)
@ -810,7 +810,7 @@ describe UsersController do
it "won't create the developer as active" do
UsernameCheckerService.expects(:is_developer?).returns(true)
post "/u.json", params: post_user_params.merge(active: true, api_key: api_key.key)
post "/u.json", params: post_user_params.merge(active: true), headers: { HTTP_API_KEY: api_key.key }
expect(response.status).to eq(200)
expect(JSON.parse(response.body)['active']).to be_falsy
end
@ -819,7 +819,7 @@ describe UsersController do
SiteSetting.allow_user_locale = true
admin.update!(locale: :fr)
post "/u.json", params: post_user_params.merge(active: true, api_key: api_key.key)
post "/u.json", params: post_user_params.merge(active: true), headers: { HTTP_API_KEY: api_key.key }
expect(response.status).to eq(200)
json = JSON.parse(response.body)
@ -832,7 +832,7 @@ describe UsersController do
SiteSetting.must_approve_users = true
SiteSetting.auto_approve_email_domains = "example.com"
post "/u.json", params: post_user_params.merge(active: true, api_key: api_key.key)
post "/u.json", params: post_user_params.merge(active: true), headers: { HTTP_API_KEY: api_key.key }
expect(response.status).to eq(200)
json = JSON.parse(response.body)
@ -858,7 +858,7 @@ describe UsersController do
fab!(:api_key, refind: false) { Fabricate(:api_key, user: user) }
it "won't create the user as staged with a regular key" do
post "/u.json", params: post_user_params.merge(staged: true, api_key: api_key.key)
post "/u.json", params: post_user_params.merge(staged: true), headers: { HTTP_API_KEY: api_key.key }
expect(response.status).to eq(200)
new_user = User.where(username: post_user_params[:username]).first
@ -871,7 +871,7 @@ describe UsersController do
fab!(:api_key, refind: false) { Fabricate(:api_key, user: user) }
it "creates the user as staged with a regular key" do
post "/u.json", params: post_user_params.merge(staged: true, api_key: api_key.key)
post "/u.json", params: post_user_params.merge(staged: true), headers: { HTTP_API_KEY: api_key.key }
expect(response.status).to eq(200)
new_user = User.where(username: post_user_params[:username]).first
@ -880,7 +880,7 @@ describe UsersController do
it "won't create the developer as staged" do
UsernameCheckerService.expects(:is_developer?).returns(true)
post "/u.json", params: post_user_params.merge(staged: true, api_key: api_key.key)
post "/u.json", params: post_user_params.merge(staged: true), headers: { HTTP_API_KEY: api_key.key }
expect(response.status).to eq(200)
new_user = User.where(username: post_user_params[:username]).first
@ -1850,8 +1850,9 @@ describe UsersController do
},
user_fields: {
user_field.id.to_s => 'user field value'
},
api_key: api_key.key
}
}, headers: {
HTTP_API_KEY: api_key.key
}
expect(response.status).to eq(200)
u = User.find_by_email('user@mail.com')