Merge branch 'add-rate-limiting-to-users-get-by-id-endpoint' into 'master'

Add rate limiting to users get by id endpoint

See merge request gitlab-org/gitlab!73069

app/controllers/concerns/check_rate_limit.rb

@@ -8,6 +8,7 @@
 # See lib/api/helpers/rate_limiter.rb for API version
 module CheckRateLimit
   def check_rate_limit!(key, scope:, redirect_back: false, **options)
+    return if bypass_header_set?
     return unless rate_limiter.throttled?(key, scope: scope, **options)
 
     rate_limiter.log_request(request, "#{key}_request_limit".to_sym, current_user)
@@ -28,4 +29,8 @@ module CheckRateLimit
   def rate_limiter
     ::Gitlab::ApplicationRateLimiter
   end
+
+  def bypass_header_set?
+    ::Gitlab::Throttle.bypass_header.present? && request.get_header(Gitlab::Throttle.bypass_header) == '1'
+  end
 end

config/feature_flags/development/rate_limit_user_by_id_endpoint.yml

+---
+name: rate_limit_user_by_id_endpoint
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/73069
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/348796
+milestone: '14.6'
+type: development
+group: group::optimize
+default_enabled: false

lib/api/helpers/rate_limiter.rb

@@ -10,6 +10,7 @@ module API
     # See app/controllers/concerns/check_rate_limit.rb for Rails controllers version
     module RateLimiter
       def check_rate_limit!(key, scope:, **options)
+        return if bypass_header_set?
         return unless rate_limiter.throttled?(key, scope: scope, **options)
 
         rate_limiter.log_request(request, "#{key}_request_limit".to_sym, current_user)
@@ -24,6 +25,10 @@ module API
       def rate_limiter
         ::Gitlab::ApplicationRateLimiter
       end
+
+      def bypass_header_set?
+        ::Gitlab::Throttle.bypass_header.present? && request.get_header(Gitlab::Throttle.bypass_header) == '1'
+      end
     end
   end
 end

lib/api/users.rb

@@ -142,11 +142,15 @@ module API
       get ":id", feature_category: :users do
         forbidden!('Not authorized!') unless current_user
 
+        if Feature.enabled?(:rate_limit_user_by_id_endpoint, type: :development)
+          check_rate_limit! :users_get_by_id, scope: current_user unless current_user.admin?
+        end
+
         user = User.find_by(id: params[:id])
 
         not_found!('User') unless user && can?(current_user, :read_user, user)
 
-        opts = { with: current_user&.admin? ? Entities::UserDetailsWithAdmin : Entities::User, current_user: current_user }
+        opts = { with: current_user.admin? ? Entities::UserDetailsWithAdmin : Entities::User, current_user: current_user }
         user, opts = with_custom_attributes(user, opts)
 
         present user, opts

lib/gitlab/application_rate_limiter.rb

@@ -49,6 +49,7 @@ module Gitlab
           group_testing_hook:           { threshold: 5, interval: 1.minute },
           profile_add_new_email:        { threshold: 5, interval: 1.minute },
           web_hook_calls:               { interval: 1.minute },
+          users_get_by_id:              { threshold: 10, interval: 1.minute },
           profile_resend_email_confirmation:  { threshold: 5, interval: 1.minute },
           update_environment_canary_ingress:  { threshold: 1, interval: 1.minute },
           auto_rollback_deployment:           { threshold: 1, interval: 3.minutes }

spec/controllers/concerns/check_rate_limit_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe CheckRateLimit do
+  let(:key) { :some_key }
+  let(:scope) { [:some, :scope] }
+  let(:request) { instance_double('Rack::Request') }
+  let(:user) { build_stubbed(:user) }
+
+  let(:controller_class) do
+    Class.new do
+      include CheckRateLimit
+
+      attr_reader :request, :current_user
+
+      def initialize(request, current_user)
+        @request = request
+        @current_user = current_user
+      end
+
+      def redirect_back_or_default(**args)
+      end
+
+      def render(**args)
+      end
+    end
+  end
+
+  subject { controller_class.new(request, user) }
+
+  before do
+    allow(::Gitlab::ApplicationRateLimiter).to receive(:throttled?)
+    allow(::Gitlab::ApplicationRateLimiter).to receive(:log_request)
+  end
+
+  describe '#check_rate_limit!' do
+    it 'calls ApplicationRateLimiter#throttled? with the right arguments' do
+      expect(::Gitlab::ApplicationRateLimiter).to receive(:throttled?).with(key, scope: scope).and_return(false)
+      expect(subject).not_to receive(:render)
+
+      subject.check_rate_limit!(key, scope: scope)
+    end
+
+    it 'renders error and logs request if throttled' do
+      expect(::Gitlab::ApplicationRateLimiter).to receive(:throttled?).with(key, scope: scope).and_return(true)
+      expect(::Gitlab::ApplicationRateLimiter).to receive(:log_request).with(request, "#{key}_request_limit".to_sym, user)
+      expect(subject).to receive(:render).with({ plain: _('This endpoint has been requested too many times. Try again later.'), status: :too_many_requests })
+
+      subject.check_rate_limit!(key, scope: scope)
+    end
+
+    it 'redirects back if throttled and redirect_back option is set to true' do
+      expect(::Gitlab::ApplicationRateLimiter).to receive(:throttled?).with(key, scope: scope).and_return(true)
+      expect(::Gitlab::ApplicationRateLimiter).to receive(:log_request).with(request, "#{key}_request_limit".to_sym, user)
+      expect(subject).not_to receive(:render)
+      expect(subject).to receive(:redirect_back_or_default).with(options: { alert: _('This endpoint has been requested too many times. Try again later.') })
+
+      subject.check_rate_limit!(key, scope: scope, redirect_back: true)
+    end
+
+    context 'when the bypass header is set' do
+      before do
+        allow(Gitlab::Throttle).to receive(:bypass_header).and_return('SOME_HEADER')
+      end
+
+      it 'skips rate limit if set to "1"' do
+        allow(request).to receive(:get_header).with(Gitlab::Throttle.bypass_header).and_return('1')
+
+        expect(::Gitlab::ApplicationRateLimiter).not_to receive(:throttled?)
+        expect(subject).not_to receive(:render)
+
+        subject.check_rate_limit!(key, scope: scope)
+      end
+
+      it 'does not skip rate limit if set to something else than "1"' do
+        allow(request).to receive(:get_header).with(Gitlab::Throttle.bypass_header).and_return('0')
+
+        expect(::Gitlab::ApplicationRateLimiter).to receive(:throttled?)
+
+        subject.check_rate_limit!(key, scope: scope)
+      end
+    end
+  end
+end

spec/lib/api/helpers/rate_limiter_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe API::Helpers::RateLimiter do
+  let(:key) { :some_key }
+  let(:scope) { [:some, :scope] }
+  let(:request) { instance_double('Rack::Request') }
+  let(:user) { build_stubbed(:user) }
+
+  let(:api_class) do
+    Class.new do
+      include API::Helpers::RateLimiter
+
+      attr_reader :request, :current_user
+
+      def initialize(request, current_user)
+        @request = request
+        @current_user = current_user
+      end
+
+      def render_api_error!(**args)
+      end
+    end
+  end
+
+  subject { api_class.new(request, user) }
+
+  before do
+    allow(::Gitlab::ApplicationRateLimiter).to receive(:throttled?)
+    allow(::Gitlab::ApplicationRateLimiter).to receive(:log_request)
+  end
+
+  describe '#check_rate_limit!' do
+    it 'calls ApplicationRateLimiter#throttled? with the right arguments' do
+      expect(::Gitlab::ApplicationRateLimiter).to receive(:throttled?).with(key, scope: scope).and_return(false)
+      expect(subject).not_to receive(:render_api_error!)
+
+      subject.check_rate_limit!(key, scope: scope)
+    end
+
+    it 'renders api error and logs request if throttled' do
+      expect(::Gitlab::ApplicationRateLimiter).to receive(:throttled?).with(key, scope: scope).and_return(true)
+      expect(::Gitlab::ApplicationRateLimiter).to receive(:log_request).with(request, "#{key}_request_limit".to_sym, user)
+      expect(subject).to receive(:render_api_error!).with({ error: _('This endpoint has been requested too many times. Try again later.') }, 429)
+
+      subject.check_rate_limit!(key, scope: scope)
+    end
+
+    context 'when the bypass header is set' do
+      before do
+        allow(Gitlab::Throttle).to receive(:bypass_header).and_return('SOME_HEADER')
+      end
+
+      it 'skips rate limit if set to "1"' do
+        allow(request).to receive(:get_header).with(Gitlab::Throttle.bypass_header).and_return('1')
+
+        expect(::Gitlab::ApplicationRateLimiter).not_to receive(:throttled?)
+        expect(subject).not_to receive(:render_api_error!)
+
+        subject.check_rate_limit!(key, scope: scope)
+      end
+
+      it 'does not skip rate limit if set to something else than "1"' do
+        allow(request).to receive(:get_header).with(Gitlab::Throttle.bypass_header).and_return('0')
+
+        expect(::Gitlab::ApplicationRateLimiter).to receive(:throttled?)
+
+        subject.check_rate_limit!(key, scope: scope)
+      end
+    end
+  end
+end

spec/requests/api/users_spec.rb

@@ -498,6 +498,10 @@ RSpec.describe API::Users do
   describe "GET /users/:id" do
     let_it_be(:user2, reload: true) { create(:user, username: 'another_user') }
 
+    before do
+      allow(Gitlab::ApplicationRateLimiter).to receive(:throttled?).with(:users_get_by_id, scope: user).and_return(false)
+    end
+
     it "returns a user by id" do
       get api("/users/#{user.id}", user)
 
@@ -593,6 +597,55 @@ RSpec.describe API::Users do
       expect(json_response).not_to have_key('sign_in_count')
     end
 
+    context 'when the rate limit is not exceeded' do
+      it 'returns a success status' do
+        expect(Gitlab::ApplicationRateLimiter)
+          .to receive(:throttled?).with(:users_get_by_id, scope: user)
+          .and_return(false)
+
+        get api("/users/#{user.id}", user)
+
+        expect(response).to have_gitlab_http_status(:ok)
+      end
+    end
+
+    context 'when the rate limit is exceeded' do
+      context 'when feature flag is enabled' do
+        it 'returns "too many requests" status' do
+          expect(Gitlab::ApplicationRateLimiter)
+            .to receive(:throttled?).with(:users_get_by_id, scope: user)
+            .and_return(true)
+
+          get api("/users/#{user.id}", user)
+
+          expect(response).to have_gitlab_http_status(:too_many_requests)
+        end
+
+        it 'still allows admin users' do
+          expect(Gitlab::ApplicationRateLimiter)
+            .not_to receive(:throttled?)
+
+          get api("/users/#{user.id}", admin)
+
+          expect(response).to have_gitlab_http_status(:ok)
+        end
+      end
+
+      context 'when feature flag is disabled' do
+        before do
+          stub_feature_flags(rate_limit_user_by_id_endpoint: false)
+        end
+
+        it 'does not throttle the request' do
+          expect(Gitlab::ApplicationRateLimiter).not_to receive(:throttled?)
+
+          get api("/users/#{user.id}", user)
+
+          expect(response).to have_gitlab_http_status(:ok)
+        end
+      end
+    end
+
     context 'when job title is present' do
       let(:job_title) { 'Fullstack Engineer' }