Merge branch 'fix-rack-attack-with-read-api-scope' into 'master'

Allow read_api scope in rate limiter

See merge request gitlab-org/gitlab!50685

lib/gitlab/auth/auth_finders.rb

@@ -92,10 +92,10 @@ module Gitlab
       # We only allow Private Access Tokens with `api` scope to be used by web
       # requests on RSS feeds or ICS files for backwards compatibility.
       # It is also used by GraphQL/API requests.
-      def find_user_from_web_access_token(request_format)
+      def find_user_from_web_access_token(request_format, scopes: [:api])
         return unless access_token && valid_web_access_format?(request_format)
 
-        validate_access_token!(scopes: [:api])
+        validate_access_token!(scopes: scopes)
 
         ::PersonalAccessTokens::LastUsedService.new(access_token).execute
 

lib/gitlab/auth/request_authenticator.rb

@@ -30,7 +30,7 @@ module Gitlab
       end
 
       def find_sessionless_user(request_format)
-        find_user_from_web_access_token(request_format) ||
+        find_user_from_web_access_token(request_format, scopes: [:api, :read_api]) ||
           find_user_from_feed_token(request_format) ||
           find_user_from_static_object_token(request_format) ||
           find_user_from_basic_auth_job ||

spec/lib/gitlab/auth/auth_finders_spec.rb

@@ -6,7 +6,8 @@ RSpec.describe Gitlab::Auth::AuthFinders do
   include described_class
   include HttpBasicAuthHelpers
 
-  let(:user) { create(:user) }
+  # Create the feed_token and static_object_token for the user
+  let_it_be(:user) { create(:user).tap(&:feed_token).tap(&:static_object_token) }
   let(:env) do
     {
       'rack.input' => ''
@@ -65,7 +66,7 @@ RSpec.describe Gitlab::Auth::AuthFinders do
   end
 
   describe '#find_user_from_bearer_token' do
-    let(:job) { create(:ci_build, user: user) }
+    let_it_be_with_reload(:job) { create(:ci_build, user: user) }
 
     subject { find_user_from_bearer_token }
 
@@ -91,7 +92,7 @@ RSpec.describe Gitlab::Auth::AuthFinders do
     end
 
     context 'with a personal access token' do
-      let(:pat) { create(:personal_access_token, user: user) }
+      let_it_be(:pat) { create(:personal_access_token, user: user) }
       let(:token) { pat.token }
 
       before do
@@ -148,7 +149,7 @@ RSpec.describe Gitlab::Auth::AuthFinders do
         end
 
         it 'returns nil if valid feed_token and disabled' do
-          allow(Gitlab::CurrentSettings).to receive(:disable_feed_token).and_return(true)
+          stub_application_setting(disable_feed_token: true)
           set_param(:feed_token, user.feed_token)
 
           expect(find_user_from_feed_token(:rss)).to be_nil
@@ -166,7 +167,7 @@ RSpec.describe Gitlab::Auth::AuthFinders do
       end
 
       context 'when rss_token param is provided' do
-        it 'returns user if valid rssd_token' do
+        it 'returns user if valid rss_token' do
           set_param(:rss_token, user.feed_token)
 
           expect(find_user_from_feed_token(:rss)).to eq user
@@ -347,7 +348,7 @@ RSpec.describe Gitlab::Auth::AuthFinders do
   end
 
   describe '#find_user_from_access_token' do
-    let(:personal_access_token) { create(:personal_access_token, user: user) }
+    let_it_be(:personal_access_token) { create(:personal_access_token, user: user) }
 
     before do
       set_header('SCRIPT_NAME', 'url.atom')
@@ -386,7 +387,7 @@ RSpec.describe Gitlab::Auth::AuthFinders do
       end
 
       context 'when using a non-prefixed access token' do
-        let(:personal_access_token) { create(:personal_access_token, :no_prefix, user: user) }
+        let_it_be(:personal_access_token) { create(:personal_access_token, :no_prefix, user: user) }
 
         it 'returns user' do
           set_header('HTTP_AUTHORIZATION', "Bearer #{personal_access_token.token}")
@@ -398,7 +399,7 @@ RSpec.describe Gitlab::Auth::AuthFinders do
   end
 
   describe '#find_user_from_web_access_token' do
-    let(:personal_access_token) { create(:personal_access_token, user: user) }
+    let_it_be_with_reload(:personal_access_token) { create(:personal_access_token, user: user) }
 
     before do
       set_header(described_class::PRIVATE_TOKEN_HEADER, personal_access_token.token)
@@ -449,6 +450,22 @@ RSpec.describe Gitlab::Auth::AuthFinders do
         expect(find_user_from_web_access_token(:api)).to be_nil
       end
 
+      context 'when the token has read_api scope' do
+        before do
+          personal_access_token.update!(scopes: ['read_api'])
+
+          set_header('SCRIPT_NAME', '/api/endpoint')
+        end
+
+        it 'raises InsufficientScopeError by default' do
+          expect { find_user_from_web_access_token(:api) }.to raise_error(Gitlab::Auth::InsufficientScopeError)
+        end
+
+        it 'finds the user when the read_api scope is passed' do
+          expect(find_user_from_web_access_token(:api, scopes: [:api, :read_api])).to eq(user)
+        end
+      end
+
       context 'when relative_url_root is set' do
         before do
           stub_config_setting(relative_url_root: '/relative_root')
@@ -464,7 +481,7 @@ RSpec.describe Gitlab::Auth::AuthFinders do
   end
 
   describe '#find_personal_access_token' do
-    let(:personal_access_token) { create(:personal_access_token, user: user) }
+    let_it_be(:personal_access_token) { create(:personal_access_token, user: user) }
 
     before do
       set_header('SCRIPT_NAME', 'url.atom')
@@ -534,7 +551,7 @@ RSpec.describe Gitlab::Auth::AuthFinders do
     end
 
     context 'access token is valid' do
-      let(:personal_access_token) { create(:personal_access_token, user: user) }
+      let_it_be(:personal_access_token) { create(:personal_access_token, user: user) }
       let(:route_authentication_setting) { { basic_auth_personal_access_token: true } }
 
       it 'finds the token from basic auth' do
@@ -555,7 +572,7 @@ RSpec.describe Gitlab::Auth::AuthFinders do
     end
 
     context 'route_setting is not set' do
-      let(:personal_access_token) { create(:personal_access_token, user: user) }
+      let_it_be(:personal_access_token) { create(:personal_access_token, user: user) }
 
       it 'returns nil' do
         auth_header_with(personal_access_token.token)
@@ -565,7 +582,7 @@ RSpec.describe Gitlab::Auth::AuthFinders do
     end
 
     context 'route_setting is not correct' do
-      let(:personal_access_token) { create(:personal_access_token, user: user) }
+      let_it_be(:personal_access_token) { create(:personal_access_token, user: user) }
       let(:route_authentication_setting) { { basic_auth_personal_access_token: false } }
 
       it 'returns nil' do
@@ -611,8 +628,9 @@ RSpec.describe Gitlab::Auth::AuthFinders do
 
     context 'with CI username' do
       let(:username) { ::Gitlab::Auth::CI_JOB_USER }
-      let(:user) { create(:user) }
-      let(:build) { create(:ci_build, user: user, status: :running) }
+
+      let_it_be(:user) { create(:user) }
+      let_it_be(:build) { create(:ci_build, user: user, status: :running) }
 
       it 'returns nil without password' do
         set_basic_auth_header(username, nil)
@@ -645,11 +663,11 @@ RSpec.describe Gitlab::Auth::AuthFinders do
   describe '#validate_access_token!' do
     subject { validate_access_token! }
 
-    let(:personal_access_token) { create(:personal_access_token, user: user) }
+    let_it_be_with_reload(:personal_access_token) { create(:personal_access_token, user: user) }
 
     context 'with a job token' do
+      let_it_be(:job) { create(:ci_build, user: user, status: :running) }
       let(:route_authentication_setting) { { job_token_allowed: true } }
-      let(:job) { create(:ci_build, user: user, status: :running) }
 
       before do
         env['HTTP_AUTHORIZATION'] = "Bearer #{job.token}"
@@ -671,7 +689,7 @@ RSpec.describe Gitlab::Auth::AuthFinders do
       end
 
       it 'returns Gitlab::Auth::ExpiredError if token expired' do
-        personal_access_token.expires_at = 1.day.ago
+        personal_access_token.update!(expires_at: 1.day.ago)
 
         expect { validate_access_token! }.to raise_error(Gitlab::Auth::ExpiredError)
       end
@@ -688,7 +706,7 @@ RSpec.describe Gitlab::Auth::AuthFinders do
     end
 
     context 'with impersonation token' do
-      let(:personal_access_token) { create(:personal_access_token, :impersonation, user: user) }
+      let_it_be(:personal_access_token) { create(:personal_access_token, :impersonation, user: user) }
 
       context 'when impersonation is disabled' do
         before do
@@ -704,7 +722,7 @@ RSpec.describe Gitlab::Auth::AuthFinders do
   end
 
   describe '#find_user_from_job_token' do
-    let(:job) { create(:ci_build, user: user, status: :running) }
+    let_it_be(:job) { create(:ci_build, user: user, status: :running) }
     let(:route_authentication_setting) { { job_token_allowed: true } }
 
     subject { find_user_from_job_token }
@@ -866,7 +884,7 @@ RSpec.describe Gitlab::Auth::AuthFinders do
   end
 
   describe '#find_runner_from_token' do
-    let(:runner) { create(:ci_runner) }
+    let_it_be(:runner) { create(:ci_runner) }
 
     context 'with API requests' do
       before do

spec/lib/gitlab/auth/request_authenticator_spec.rb

@@ -47,7 +47,10 @@ RSpec.describe Gitlab::Auth::RequestAuthenticator do
     let!(:job_token_user) { build(:user) }
 
     it 'returns access_token user first' do
-      allow_any_instance_of(described_class).to receive(:find_user_from_web_access_token).and_return(access_token_user)
+      allow_any_instance_of(described_class).to receive(:find_user_from_web_access_token)
+                                                  .with(anything, scopes: [:api, :read_api])
+                                                  .and_return(access_token_user)
+
       allow_any_instance_of(described_class).to receive(:find_user_from_feed_token).and_return(feed_token_user)
 
       expect(subject.find_sessionless_user(:api)).to eq access_token_user

spec/requests/rack_attack_global_spec.rb

@@ -188,10 +188,10 @@ RSpec.describe 'Rack Attack global throttles' do
   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_it_be(:user) { create(:user) }
+    let_it_be(:token) { create(:personal_access_token, user: user) }
+    let_it_be(:other_user) { create(:user) }
+    let_it_be(:other_user_token) { create(:personal_access_token, user: other_user) }
     let(:throttle_setting_prefix) { 'throttle_authenticated_api' }
     let(:api_partial_url) { '/todos' }
 
@@ -208,6 +208,41 @@ RSpec.describe 'Rack Attack global throttles' do
 
       it_behaves_like 'rate-limited token-authenticated requests'
     end
+
+    context 'with the token in the OAuth headers' do
+      let(:request_args) { api_get_args_with_token_headers(api_partial_url, oauth_token_headers(token)) }
+      let(:other_user_request_args) { api_get_args_with_token_headers(api_partial_url, oauth_token_headers(other_user_token)) }
+
+      it_behaves_like 'rate-limited token-authenticated requests'
+    end
+
+    context 'with the token in basic auth' do
+      let(:request_args) { api_get_args_with_token_headers(api_partial_url, basic_auth_headers(user, token)) }
+      let(:other_user_request_args) { api_get_args_with_token_headers(api_partial_url, basic_auth_headers(other_user, other_user_token)) }
+
+      it_behaves_like 'rate-limited token-authenticated requests'
+    end
+
+    context 'with a read_api scope' do
+      before do
+        token.update!(scopes: ['read_api'])
+        other_user_token.update!(scopes: ['read_api'])
+      end
+
+      context 'with the token in the headers' do
+        let(:request_args) { api_get_args_with_token_headers(api_partial_url, personal_access_token_headers(token)) }
+        let(:other_user_request_args) { api_get_args_with_token_headers(api_partial_url, personal_access_token_headers(other_user_token)) }
+
+        it_behaves_like 'rate-limited token-authenticated requests'
+      end
+
+      context 'with the token in the OAuth headers' do
+        let(:request_args) { api_get_args_with_token_headers(api_partial_url, oauth_token_headers(token)) }
+        let(:other_user_request_args) { api_get_args_with_token_headers(api_partial_url, oauth_token_headers(other_user_token)) }
+
+        it_behaves_like 'rate-limited token-authenticated requests'
+      end
+    end
   end
 
   describe 'API requests authenticated with OAuth token', :api do
@@ -235,6 +270,15 @@ RSpec.describe 'Rack Attack global throttles' do
 
       it_behaves_like 'rate-limited token-authenticated requests'
     end
+
+    context 'with a read_api scope' do
+      let(:read_token) { Doorkeeper::AccessToken.create!(application_id: application.id, resource_owner_id: user.id, scopes: "read_api") }
+      let(:other_user_read_token) { Doorkeeper::AccessToken.create!(application_id: other_user_application.id, resource_owner_id: other_user.id, scopes: "read_api") }
+      let(:request_args) { api_get_args_with_token_headers(api_partial_url, oauth_token_headers(read_token)) }
+      let(:other_user_request_args) { api_get_args_with_token_headers(api_partial_url, oauth_token_headers(other_user_read_token)) }
+
+      it_behaves_like 'rate-limited token-authenticated requests'
+    end
   end
 
   describe '"web" (non-API) requests authenticated with RSS token' do

spec/support/helpers/rack_attack_spec_helpers.rb

@@ -21,6 +21,11 @@ module RackAttackSpecHelpers
     { 'AUTHORIZATION' => "Bearer #{oauth_access_token.token}" }
   end
 
+  def basic_auth_headers(user, personal_access_token)
+    encoded_login = ["#{user.username}:#{personal_access_token.token}"].pack('m0')
+    { 'AUTHORIZATION' => "Basic #{encoded_login}" }
+  end
+
   def expect_rejection(&block)
     yield