Fix OAuth API and RSS rate limiting

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

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

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)

lib/gitlab/auth/request_authenticator.rb

+# 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

spec/requests/rack_attack_spec.rb

 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,174 +24,262 @@ describe Rack::Attack do
     Timecop.freeze { example.run }
   end
 
-  describe 'unauthenticated requests' do
-    let(:requests_per_period) { settings.throttle_unauthenticated_requests_per_period }
-    let(:period) { settings.throttle_unauthenticated_period_in_seconds.seconds }
+  # 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.throttle_unauthenticated_requests_per_period = 1
-      settings.throttle_unauthenticated_period_in_seconds = 10
+      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.throttle_unauthenticated_enabled = true
+        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 '/users/sign_in'
+          get *get_args
           expect(response).to have_http_status 200
         end
 
         # the last straw
-        get '/users/sign_in'
-        expect(response).to have_http_status 429
+        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 '/users/sign_in'
+          get *get_args
           expect(response).to have_http_status 200
         end
 
-        get '/users/sign_in'
-        expect(response).to have_http_status 429
+        expect_rejection { get *get_args }
 
-        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'
+            get *get_args
             expect(response).to have_http_status 200
           end
 
-          get '/users/sign_in'
-          expect(response).to have_http_status 429
+          expect_rejection { get *get_args }
         end
       end
 
-      it 'counts requests from different IPs separately' do
+      it 'counts requests from different users separately, even from the same IP' do
         requests_per_period.times do
-          get '/users/sign_in'
+          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')
 
-        # would be over limit for the same IP
-        get '/users/sign_in'
-        expect(response).to have_http_status 200
+        expect_rejection { get *get_args }
       end
     end
 
     context 'when the throttle is disabled' do
       before do
-        settings.throttle_unauthenticated_enabled = false
+        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 '/users/sign_in'
+          get *get_args
           expect(response).to have_http_status 200
         end
       end
     end
   end
 
-  describe 'authenticated API requests', :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) }
+  describe 'unauthenticated requests' do
+    let(:requests_per_period) { settings.throttle_unauthenticated_requests_per_period }
+    let(:period) { settings.throttle_unauthenticated_period_in_seconds.seconds }
 
     before do
       # Set low limits
-      settings.throttle_authenticated_api_requests_per_period = 1
-      settings.throttle_authenticated_api_period_in_seconds = 10
+      settings.throttle_unauthenticated_requests_per_period = 1
+      settings.throttle_unauthenticated_period_in_seconds = 10000
     end
 
     context 'when the throttle is enabled' do
       before do
-        settings.throttle_authenticated_api_enabled = true
+        settings.throttle_unauthenticated_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 api('/todos', user)
+          get '/users/sign_in'
           expect(response).to have_http_status 200
         end
 
         # the last straw
-        get api('/todos', user)
-        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
         requests_per_period.times do
-          get api('/todos', user)
+          get '/users/sign_in'
           expect(response).to have_http_status 200
         end
 
-        get api('/todos', user)
-        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 api('/todos', user)
+            get '/users/sign_in'
             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
+          expect_rejection { get '/users/sign_in' }
         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
+      it 'counts requests from different IPs separately' do
         requests_per_period.times do
-          get api('/todos', user)
+          get '/users/sign_in'
           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
+        # would be over limit for the same IP
+        get '/users/sign_in'
+        expect(response).to have_http_status 200
       end
     end
 
     context 'when the throttle is disabled' do
       before do
-        settings.throttle_authenticated_api_enabled = false
+        settings.throttle_unauthenticated_enabled = false
         settings.save!
       end
 
       it 'allows requests over the rate limit' do
         (1 + requests_per_period).times do
-          get api('/todos', user)
+          get '/users/sign_in'
           expect(response).to have_http_status 200
         end
       end
     end
   end
 
-  describe 'authenticated web requests' 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' }
+
+    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 '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_behaves_like 'rate-limited token-authenticated requests'
+    end
+  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' }
+
+    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)] }
+
+      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, 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
+
+      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 '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