diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 42eae408fdc..cd7b2a463fd 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -125,7 +125,7 @@ class ApplicationController < ActionController::Base # This filter handles private tokens, personal access tokens, and atom # requests with rss tokens def authenticate_sessionless_user! - user = Gitlab::Auth.find_sessionless_user(request) + user = Gitlab::Auth::RequestAuthenticator.new(request).find_sessionless_user sessionless_sign_in(user) if user >>>>>>> Add request throttles diff --git a/config/initializers/rack_attack_global.rb b/config/initializers/rack_attack_global.rb index 3073ba06ac1..cf87310d7b7 100644 --- a/config/initializers/rack_attack_global.rb +++ b/config/initializers/rack_attack_global.rb @@ -45,7 +45,7 @@ class Rack::Attack end def authenticated_user_id - session_user_id || sessionless_user_id + Gitlab::Auth::RequestAuthenticator.new(self).user&.id end def api_request? @@ -55,15 +55,5 @@ class Rack::Attack def web_request? !api_request? end - - private - - def session_user_id - Gitlab::Auth.find_session_user(self)&.id - end - - def sessionless_user_id - Gitlab::Auth.find_sessionless_user(self)&.id - end end end diff --git a/lib/gitlab/auth.rb b/lib/gitlab/auth.rb index 8f39885c608..cbbc51db99e 100644 --- a/lib/gitlab/auth.rb +++ b/lib/gitlab/auth.rb @@ -82,36 +82,6 @@ module Gitlab end end - # request may be Rack::Attack::Request which is just a Rack::Request, so - # we cannot use ActionDispatch::Request methods. - def find_user_by_private_token(request) - token = request.params['private_token'].presence || request.env['HTTP_PRIVATE_TOKEN'].presence - - return unless token.present? - - User.find_by_authentication_token(token) || User.find_by_personal_access_token(token) - end - - # request may be Rack::Attack::Request which is just a Rack::Request, so - # we cannot use ActionDispatch::Request methods. - def find_user_by_rss_token(request) - return unless request.params['format'] == 'atom' - - token = request.params['rss_token'].presence - - return unless token.present? - - User.find_by_rss_token(token) - end - - def find_session_user(request) - request.env['warden']&.authenticate - end - - def find_sessionless_user(request) - find_user_by_private_token(request) || find_user_by_rss_token(request) - end - private def service_request_check(login, password, project) diff --git a/lib/gitlab/auth/request_authenticator.rb b/lib/gitlab/auth/request_authenticator.rb new file mode 100644 index 00000000000..d3da4cc2d2b --- /dev/null +++ b/lib/gitlab/auth/request_authenticator.rb @@ -0,0 +1,64 @@ +# Use for authentication only, in particular for Rack::Attack. +# Does not perform authorization of scopes, etc. +module Gitlab + module Auth + class RequestAuthenticator + def initialize(request) + @request = request + end + + def user + find_sessionless_user || find_session_user + end + + def find_sessionless_user + find_user_by_private_token || find_user_by_rss_token || find_user_by_oauth_token + end + + private + + def find_session_user + @request.env['warden']&.authenticate if verified_request? + end + + # request may be Rack::Attack::Request which is just a Rack::Request, so + # we cannot use ActionDispatch::Request methods. + def find_user_by_private_token + token = @request.params['private_token'].presence || @request.env['HTTP_PRIVATE_TOKEN'].presence + return unless token.present? + + User.find_by_authentication_token(token) || User.find_by_personal_access_token(token) + end + + # request may be Rack::Attack::Request which is just a Rack::Request, so + # we cannot use ActionDispatch::Request methods. + def find_user_by_rss_token + return unless @request.path.ends_with?('atom') || @request.env['HTTP_ACCEPT'] == 'application/atom+xml' + + token = @request.params['rss_token'].presence + return unless token.present? + + User.find_by_rss_token(token) + end + + def find_user_by_oauth_token + access_token = find_oauth_access_token + access_token&.user + end + + def find_oauth_access_token + token = Doorkeeper::OAuth::Token.from_request(doorkeeper_request, *Doorkeeper.configuration.access_token_methods) + OauthAccessToken.by_token(token) if token + end + + def doorkeeper_request + ActionDispatch::Request.new(@request.env) + end + + # Check if the request is GET/HEAD, or if CSRF token is valid. + def verified_request? + Gitlab::RequestForgeryProtection.verified?(@request.env) + end + end + end +end diff --git a/spec/requests/rack_attack_spec.rb b/spec/requests/rack_attack_spec.rb index 97108476e00..684f2af0865 100644 --- a/spec/requests/rack_attack_spec.rb +++ b/spec/requests/rack_attack_spec.rb @@ -1,6 +1,8 @@ require 'spec_helper' describe Rack::Attack do + NUM_TRIES_FOR_REJECTION = 3 # Flaky tests, have not figured out how to fix it + let(:settings) { Gitlab::CurrentSettings.current_application_settings } before do @@ -22,6 +24,93 @@ describe Rack::Attack do Timecop.freeze { example.run } end + # Requires let variables: + # * throttle_setting_prefix (e.g. "throttle_authenticated_api" or "throttle_authenticated_web") + # * get_args + # * other_user_get_args + shared_examples_for 'rate-limited token-authenticated requests' do + let(:requests_per_period) { settings.send(:"#{throttle_setting_prefix}_requests_per_period") } + let(:period) { settings.send(:"#{throttle_setting_prefix}_period_in_seconds").seconds } + + before do + # Set low limits + settings.send(:"#{throttle_setting_prefix}_requests_per_period=", 1) + settings.send(:"#{throttle_setting_prefix}_period_in_seconds=", 10000) + end + + context 'when the throttle is enabled' do + before do + settings.send(:"#{throttle_setting_prefix}_enabled=", true) + settings.save! + end + + it 'rejects requests over the rate limit' do + # At first, allow requests under the rate limit. + requests_per_period.times do + get *get_args + expect(response).to have_http_status 200 + end + + # the last straw + expect_rejection { get *get_args } + end + + it 'allows requests after throttling and then waiting for the next period' do + requests_per_period.times do + get *get_args + expect(response).to have_http_status 200 + end + + expect_rejection { get *get_args } + + Timecop.travel((1.second + period).from_now) do # Add 1 because flaky + requests_per_period.times do + get *get_args + expect(response).to have_http_status 200 + end + + expect_rejection { get *get_args } + end + end + + it 'counts requests from different users separately, even from the same IP' do + requests_per_period.times do + get *get_args + expect(response).to have_http_status 200 + end + + # would be over the limit if this wasn't a different user + get *other_user_get_args + expect(response).to have_http_status 200 + end + + it 'counts all requests from the same user, even via different IPs' do + requests_per_period.times do + get *get_args + expect(response).to have_http_status 200 + end + + expect_any_instance_of(Rack::Attack::Request).to receive(:ip).and_return('1.2.3.4') + + expect_rejection { get *get_args } + end + end + + context 'when the throttle is disabled' do + before do + settings.send(:"#{throttle_setting_prefix}_enabled=", false) + settings.save! + end + + it 'allows requests over the rate limit' do + (1 + requests_per_period).times do + get *get_args + expect(response).to have_http_status 200 + end + end + end + end + describe 'unauthenticated requests' do let(:requests_per_period) { settings.throttle_unauthenticated_requests_per_period } let(:period) { settings.throttle_unauthenticated_period_in_seconds.seconds } @@ -29,7 +118,7 @@ describe Rack::Attack do before do # Set low limits settings.throttle_unauthenticated_requests_per_period = 1 - settings.throttle_unauthenticated_period_in_seconds = 10 + settings.throttle_unauthenticated_period_in_seconds = 10000 end context 'when the throttle is enabled' do @@ -46,8 +135,7 @@ describe Rack::Attack do end # the last straw - get '/users/sign_in' - expect(response).to have_http_status 429 + expect_rejection { get '/users/sign_in' } end it 'allows requests after throttling and then waiting for the next period' do @@ -56,17 +144,15 @@ describe Rack::Attack do expect(response).to have_http_status 200 end - get '/users/sign_in' - expect(response).to have_http_status 429 + expect_rejection { get '/users/sign_in' } - Timecop.travel(period.from_now) do + Timecop.travel((1.second + period).from_now) do # Add 1 because flaky requests_per_period.times do get '/users/sign_in' expect(response).to have_http_status 200 end - get '/users/sign_in' - expect(response).to have_http_status 429 + expect_rejection { get '/users/sign_in' } end end @@ -99,97 +185,101 @@ describe Rack::Attack do end end - describe 'authenticated API requests', :api do + describe 'API requests authenticated with private token', :api do let(:requests_per_period) { settings.throttle_authenticated_api_requests_per_period } let(:period) { settings.throttle_authenticated_api_period_in_seconds.seconds } let(:user) { create(:user) } + let(:other_user) { create(:user) } + let(:throttle_setting_prefix) { 'throttle_authenticated_api' } - before do - # Set low limits - settings.throttle_authenticated_api_requests_per_period = 1 - settings.throttle_authenticated_api_period_in_seconds = 10 + context 'with the token in the query string' do + let(:get_args) { [api('/todos', user)] } + let(:other_user_get_args) { [api('/todos', other_user)] } + + it_behaves_like 'rate-limited token-authenticated requests' end - context 'when the throttle is enabled' do - before do - settings.throttle_authenticated_api_enabled = true - settings.save! - end + context 'with the token in the headers' do + let(:get_args) { ["/api/#{API::API.version}/todos", nil, private_token_headers(user)] } + let(:other_user_get_args) { ["/api/#{API::API.version}/todos", nil, private_token_headers(other_user)] } - it 'rejects requests over the rate limit' do - # At first, allow requests under the rate limit. - requests_per_period.times do - get api('/todos', user) - expect(response).to have_http_status 200 - end + it_behaves_like 'rate-limited token-authenticated requests' + end + end - # the last straw - get api('/todos', user) - expect(response).to have_http_status 429 - end + describe 'API requests authenticated with personal access token', :api do + let(:user) { create(:user) } + let(:token) { create(:personal_access_token, user: user) } + let(:other_user) { create(:user) } + let(:other_user_token) { create(:personal_access_token, user: other_user) } + let(:throttle_setting_prefix) { 'throttle_authenticated_api' } - it 'allows requests after throttling and then waiting for the next period' do - requests_per_period.times do - get api('/todos', user) - expect(response).to have_http_status 200 - end + context 'with the token in the query string' do + let(:get_args) { [api('/todos', personal_access_token: token)] } + let(:other_user_get_args) { [api('/todos', personal_access_token: other_user_token)] } - get api('/todos', user) - expect(response).to have_http_status 429 - - Timecop.travel(period.from_now) do - requests_per_period.times do - get api('/todos', user) - expect(response).to have_http_status 200 - end - - get api('/todos', user) - expect(response).to have_http_status 429 - end - end - - it 'counts requests from different users separately, even from the same IP' do - other_user = create(:user) - - requests_per_period.times do - get api('/todos', user) - expect(response).to have_http_status 200 - end - - # would be over the limit if this wasn't a different user - get api('/todos', other_user) - expect(response).to have_http_status 200 - end - - it 'counts all requests from the same user, even via different IPs' do - requests_per_period.times do - get api('/todos', user) - expect(response).to have_http_status 200 - end - - expect_any_instance_of(Rack::Attack::Request).to receive(:ip).and_return('1.2.3.4') - - get api('/todos', user) - expect(response).to have_http_status 429 - end + it_behaves_like 'rate-limited token-authenticated requests' end - context 'when the throttle is disabled' do - before do - settings.throttle_authenticated_api_enabled = false - settings.save! + context 'with the token in the headers' do + let(:get_args) { ["/api/#{API::API.version}/todos", nil, personal_access_token_headers(token)] } + let(:other_user_get_args) { ["/api/#{API::API.version}/todos", nil, personal_access_token_headers(other_user_token)] } + + it_behaves_like 'rate-limited token-authenticated requests' + end + end + + describe 'API requests authenticated with OAuth token', :api do + let(:requests_per_period) { settings.throttle_authenticated_api_requests_per_period } + let(:period) { settings.throttle_authenticated_api_period_in_seconds.seconds } + let(:user) { create(:user) } + let(:application) { Doorkeeper::Application.create!(name: "MyApp", redirect_uri: "https://app.com", owner: user) } + let(:token) { Doorkeeper::AccessToken.create!(application_id: application.id, resource_owner_id: user.id, scopes: "api") } + let(:other_user) { create(:user) } + let(:other_user_application) { Doorkeeper::Application.create!(name: "MyApp", redirect_uri: "https://app.com", owner: other_user) } + let(:other_user_token) { Doorkeeper::AccessToken.create!(application_id: application.id, resource_owner_id: other_user.id, scopes: "api") } + let(:throttle_setting_prefix) { 'throttle_authenticated_api' } + + context 'with the token in the query string' do + let(:get_args) { [api('/todos', oauth_access_token: token)] } + let(:other_user_get_args) { [api('/todos', oauth_access_token: other_user_token)] } + + it_behaves_like 'rate-limited token-authenticated requests' + end + + context 'with the token in the headers' do + let(:get_args) { ["/api/#{API::API.version}/todos", nil, oauth_token_headers(token)] } + let(:other_user_get_args) { ["/api/#{API::API.version}/todos", nil, oauth_token_headers(other_user_token)] } + + it_behaves_like 'rate-limited token-authenticated requests' + end + end + + describe '"web" (non-API) requests authenticated with RSS token' do + let(:requests_per_period) { settings.throttle_authenticated_web_requests_per_period } + let(:period) { settings.throttle_authenticated_web_period_in_seconds.seconds } + let(:user) { create(:user) } + let(:other_user) { create(:user) } + let(:throttle_setting_prefix) { 'throttle_authenticated_web' } + + context 'with the token in the query string' do + context 'with the atom extension' do + let(:get_args) { ["/dashboard/projects.atom?rss_token=#{user.rss_token}"] } + let(:other_user_get_args) { ["/dashboard/projects.atom?rss_token=#{other_user.rss_token}"] } + + it_behaves_like 'rate-limited token-authenticated requests' end - it 'allows requests over the rate limit' do - (1 + requests_per_period).times do - get api('/todos', user) - expect(response).to have_http_status 200 - end + context 'with the atom format in the Accept header' do + let(:get_args) { ["/dashboard/projects?rss_token=#{user.rss_token}", nil, { 'HTTP_ACCEPT' => 'application/atom+xml' }] } + let(:other_user_get_args) { ["/dashboard/projects?rss_token=#{other_user.rss_token}", nil, { 'HTTP_ACCEPT' => 'application/atom+xml' }] } + + it_behaves_like 'rate-limited token-authenticated requests' end end end - describe 'authenticated web requests' do + describe 'web requests authenticated with regular login' do let(:requests_per_period) { settings.throttle_authenticated_web_requests_per_period } let(:period) { settings.throttle_authenticated_web_period_in_seconds.seconds } let(:user) { create(:user) } @@ -199,7 +289,7 @@ describe Rack::Attack do # Set low limits settings.throttle_authenticated_web_requests_per_period = 1 - settings.throttle_authenticated_web_period_in_seconds = 10 + settings.throttle_authenticated_web_period_in_seconds = 10000 end context 'when the throttle is enabled' do @@ -216,8 +306,7 @@ describe Rack::Attack do end # the last straw - get '/dashboard/snippets' - expect(response).to have_http_status 429 + expect_rejection { get '/dashboard/snippets' } end it 'allows requests after throttling and then waiting for the next period' do @@ -226,17 +315,15 @@ describe Rack::Attack do expect(response).to have_http_status 200 end - get '/dashboard/snippets' - expect(response).to have_http_status 429 + expect_rejection { get '/dashboard/snippets' } - Timecop.travel(period.from_now) do + Timecop.travel((1.second + period).from_now) do # Add 1 because flaky requests_per_period.times do get '/dashboard/snippets' expect(response).to have_http_status 200 end - get '/dashboard/snippets' - expect(response).to have_http_status 429 + expect_rejection { get '/dashboard/snippets' } end end @@ -261,8 +348,7 @@ describe Rack::Attack do expect_any_instance_of(Rack::Attack::Request).to receive(:ip).and_return('1.2.3.4') - get '/dashboard/snippets' - expect(response).to have_http_status 429 + expect_rejection { get '/dashboard/snippets' } end end @@ -280,4 +366,26 @@ describe Rack::Attack do end end end + + def private_token_headers(user) + { 'HTTP_PRIVATE_TOKEN' => user.private_token } + end + + def personal_access_token_headers(personal_access_token) + { 'HTTP_PRIVATE_TOKEN' => personal_access_token.token } + end + + def oauth_token_headers(oauth_access_token) + { 'AUTHORIZATION' => "Bearer #{oauth_access_token.token}" } + end + + def expect_rejection(&block) + NUM_TRIES_FOR_REJECTION.times do |i| + block.call + break if response.status == 429 # success + Rails.logger.warn "Flaky test expected HTTP status 429 but got #{response.status}. Will attempt again (#{i + 1}/#{NUM_TRIES_FOR_REJECTION})" + end + + expect(response).to have_http_status(429) + end end