Merge branch...

Merge branch '321838-spike-move-application-limiting-from-endpoint-level-to-service-level-2' into 'master'

Use new RateLimitedService when creating Issues

See merge request gitlab-org/gitlab!68526

app/controllers/application_controller.rb

@@ -108,6 +108,12 @@ class ApplicationController < ActionController::Base
     head :forbidden, retry_after: Gitlab::Auth::UniqueIpsLimiter.config.unique_ips_limit_time_window
   end
 
+  rescue_from RateLimitedService::RateLimitedError do |e|
+    e.log_request(request, current_user)
+    response.headers.merge!(e.headers)
+    render plain: e.message, status: :too_many_requests
+  end
+
   def redirect_back_or_default(default: root_path, options: {})
     redirect_back(fallback_location: default, **options)
   end

app/controllers/projects/issues_controller.rb

@@ -37,7 +37,7 @@ class Projects::IssuesController < Projects::ApplicationController
   before_action :authorize_download_code!, only: [:related_branches]
 
   # Limit the amount of issues created per minute
-  before_action :create_rate_limit, only: [:create]
+  before_action :create_rate_limit, only: [:create], if: -> { Feature.disabled?('rate_limited_service_issues_create', project, default_enabled: :yaml) }
 
   before_action do
     push_frontend_feature_flag(:tribute_autocomplete, @project)

app/services/concerns/rate_limited_service.rb

+# frozen_string_literal: true
+
+module RateLimitedService
+  extend ActiveSupport::Concern
+
+  RateLimitedNotSetupError = Class.new(StandardError)
+
+  class RateLimitedError < StandardError
+    def initialize(key:, rate_limiter:)
+      @key = key
+      @rate_limiter = rate_limiter
+    end
+
+    def headers
+      # TODO: This will be fleshed out in https://gitlab.com/gitlab-org/gitlab/-/issues/342370
+      {}
+    end
+
+    def log_request(request, current_user)
+      rate_limiter.class.log_request(request, "#{key}_request_limit".to_sym, current_user)
+    end
+
+    private
+
+    attr_reader :key, :rate_limiter
+  end
+
+  class RateLimiterScopedAndKeyed
+    attr_reader :key, :opts, :rate_limiter_klass
+
+    def initialize(key:, opts:, rate_limiter_klass:)
+      @key = key
+      @opts = opts
+      @rate_limiter_klass = rate_limiter_klass
+    end
+
+    def rate_limit!(service)
+      evaluated_scope = evaluated_scope_for(service)
+      return if feature_flag_disabled?(evaluated_scope[:project])
+
+      rate_limiter = new_rate_limiter(evaluated_scope)
+      if rate_limiter.throttled?
+        raise RateLimitedError.new(key: key, rate_limiter: rate_limiter), _('This endpoint has been requested too many times. Try again later.')
+      end
+    end
+
+    private
+
+    def users_allowlist
+      @users_allowlist ||= opts[:users_allowlist] ? opts[:users_allowlist].call : []
+    end
+
+    def evaluated_scope_for(service)
+      opts[:scope].each_with_object({}) do |var, all|
+        all[var] = service.public_send(var) # rubocop: disable GitlabSecurity/PublicSend
+      end
+    end
+
+    def feature_flag_disabled?(project)
+      Feature.disabled?("rate_limited_service_#{key}", project, default_enabled: :yaml)
+    end
+
+    def new_rate_limiter(evaluated_scope)
+      rate_limiter_klass.new(key, **opts.merge(scope: evaluated_scope.values, users_allowlist: users_allowlist))
+    end
+  end
+
+  prepended do
+    attr_accessor :rate_limiter_bypassed
+    cattr_accessor :rate_limiter_scoped_and_keyed
+
+    def self.rate_limit(key:, opts:, rate_limiter_klass: ::Gitlab::ApplicationRateLimiter)
+      self.rate_limiter_scoped_and_keyed = RateLimiterScopedAndKeyed.new(key: key,
+                                                                         opts: opts,
+                                                                         rate_limiter_klass: rate_limiter_klass)
+    end
+  end
+
+  def execute_without_rate_limiting(*args, **kwargs)
+    self.rate_limiter_bypassed = true
+    execute(*args, **kwargs)
+  ensure
+    self.rate_limiter_bypassed = false
+  end
+
+  def execute(*args, **kwargs)
+    raise RateLimitedNotSetupError if rate_limiter_scoped_and_keyed.nil?
+
+    rate_limiter_scoped_and_keyed.rate_limit!(self) unless rate_limiter_bypassed
+
+    super
+  end
+end

app/services/issues/create_service.rb

@@ -3,6 +3,10 @@
 module Issues
   class CreateService < Issues::BaseService
     include ResolveDiscussions
+    prepend RateLimitedService
+
+    rate_limit key: :issues_create,
+               opts: { scope: [:project, :current_user], users_allowlist: -> { [User.support_bot.username] } }
 
     # NOTE: For Issues::CreateService, we require the spam_params and do not default it to nil, because
     # spam_checking is likely to be necessary.  However, if there is not a request available in scope

config/feature_flags/development/rate_limited_service_issues_create.yml

+---
+name: rate_limited_service_issues_create
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/68526
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/342677
+milestone: '14.4'
+type: development
+group: group::project management
+default_enabled: false

lib/api/api.rb

@@ -124,6 +124,11 @@ module API
       handle_api_exception(exception)
     end
 
+    rescue_from RateLimitedService::RateLimitedError do |exception|
+      exception.log_request(context.request, context.current_user)
+      rack_response({ 'message' => { 'error' => exception.message } }.to_json, 429, exception.headers)
+    end
+
     format :json
     formatter :json, Gitlab::Json::GrapeFormatter
     content_type :json, 'application/json'
@@ -132,6 +137,7 @@ module API
     helpers ::API::Helpers
     helpers ::API::Helpers::CommonHelpers
     helpers ::API::Helpers::PerformanceBarHelpers
+    helpers ::API::Helpers::RateLimiter
 
     namespace do
       after do

lib/api/group_export.rb

@@ -2,8 +2,6 @@
 
 module API
   class GroupExport < ::API::Base
-    helpers Helpers::RateLimiter
-
     before do
       not_found! unless Feature.enabled?(:group_import_export, user_group, default_enabled: true)
 

lib/api/issues.rb

@@ -4,7 +4,6 @@ module API
   class Issues < ::API::Base
     include PaginationParams
     helpers Helpers::IssuesHelpers
-    helpers Helpers::RateLimiter
 
     before { authenticate_non_get! }
 
@@ -263,7 +262,7 @@ module API
       post ':id/issues' do
         Gitlab::QueryLimiting.disable!('https://gitlab.com/gitlab-org/gitlab/-/issues/21140')
 
-        check_rate_limit! :issues_create, [current_user]
+        check_rate_limit! :issues_create, [current_user] if Feature.disabled?("rate_limited_service_issues_create", user_project, default_enabled: :yaml)
 
         authorize! :create_issue, user_project
 

lib/api/notes.rb

@@ -4,7 +4,6 @@ module API
   class Notes < ::API::Base
     include PaginationParams
     helpers ::API::Helpers::NotesHelpers
-    helpers Helpers::RateLimiter
 
     before { authenticate! }
 

lib/api/project_export.rb

@@ -2,8 +2,6 @@
 
 module API
   class ProjectExport < ::API::Base
-    helpers Helpers::RateLimiter
-
     feature_category :importers
 
     before do

lib/api/project_import.rb

@@ -6,7 +6,6 @@ module API
 
     helpers Helpers::ProjectsHelpers
     helpers Helpers::FileUploadHelpers
-    helpers Helpers::RateLimiter
 
     feature_category :importers
 

lib/gitlab/application_rate_limiter.rb

@@ -11,6 +11,23 @@ module Gitlab
   #   redirect_to(edit_project_path(@project), status: :too_many_requests)
   # end
   class ApplicationRateLimiter
+    def initialize(key, **options)
+      @key = key
+      @options = options
+    end
+
+    def throttled?
+      self.class.throttled?(key, **options)
+    end
+
+    def threshold_value
+      options[:threshold] || self.class.threshold(key)
+    end
+
+    def interval_value
+      self.class.interval(key)
+    end
+
     class << self
       # Application rate limits
       #
@@ -154,5 +171,9 @@ module Gitlab
         scoped_user.username.downcase.in?(options[:users_allowlist])
       end
     end
+
+    private
+
+    attr_reader :key, :options
   end
 end

lib/quality/seeders/issues.rb

@@ -30,7 +30,8 @@ module Quality
               labels: labels.join(',')
             }
             params[:closed_at] = params[:created_at] + rand(35).days if params[:state] == 'closed'
-            issue = ::Issues::CreateService.new(project: project, current_user: team.sample, params: params, spam_params: nil).execute
+
+            issue = ::Issues::CreateService.new(project: project, current_user: team.sample, params: params, spam_params: nil).execute_without_rate_limiting
 
             if issue.persisted?
               created_issues_count += 1

spec/controllers/projects/issues_controller_spec.rb

@@ -1411,39 +1411,42 @@ RSpec.describe Projects::IssuesController do
         stub_application_setting(issues_create_limit: 5)
       end
 
-      it 'prevents from creating more issues', :request_store do
-        5.times { post_new_issue }
-
-        expect { post_new_issue }
-          .to change { Gitlab::GitalyClient.get_request_count }.by(1) # creates 1 projects and 0 issues
-
-        post_new_issue
-        expect(response.body).to eq(_('This endpoint has been requested too many times. Try again later.'))
-        expect(response).to have_gitlab_http_status(:too_many_requests)
-      end
-
-      it 'logs the event on auth.log' do
-        attributes = {
-          message: 'Application_Rate_Limiter_Request',
-          env: :issues_create_request_limit,
-          remote_ip: '0.0.0.0',
-          request_method: 'POST',
-          path: "/#{project.full_path}/-/issues",
-          user_id: user.id,
-          username: user.username
-        }
+      context 'when issue creation limits imposed' do
+        it 'prevents from creating more issues', :request_store do
+          5.times { post_new_issue }
 
-        expect(Gitlab::AuthLogger).to receive(:error).with(attributes).once
+          expect { post_new_issue }
+            .to change { Gitlab::GitalyClient.get_request_count }.by(1) # creates 1 projects and 0 issues
 
-        project.add_developer(user)
-        sign_in(user)
+          post_new_issue
 
-        6.times do
-          post :create, params: {
-            namespace_id: project.namespace.to_param,
-            project_id: project,
-            issue: { title: 'Title', description: 'Description' }
+          expect(response.body).to eq(_('This endpoint has been requested too many times. Try again later.'))
+          expect(response).to have_gitlab_http_status(:too_many_requests)
+        end
+
+        it 'logs the event on auth.log' do
+          attributes = {
+            message: 'Application_Rate_Limiter_Request',
+            env: :issues_create_request_limit,
+            remote_ip: '0.0.0.0',
+            request_method: 'POST',
+            path: "/#{project.full_path}/-/issues",
+            user_id: user.id,
+            username: user.username
           }
+
+          expect(Gitlab::AuthLogger).to receive(:error).with(attributes).once
+
+          project.add_developer(user)
+          sign_in(user)
+
+          6.times do
+            post :create, params: {
+              namespace_id: project.namespace.to_param,
+              project_id: project,
+              issue: { title: 'Title', description: 'Description' }
+            }
+          end
         end
       end
     end

spec/graphql/mutations/issues/create_spec.rb

@@ -53,7 +53,11 @@ RSpec.describe Mutations::Issues::Create do
       stub_spam_services
     end
 
-    subject { mutation.resolve(**mutation_params) }
+    def resolve
+      mutation.resolve(**mutation_params)
+    end
+
+    subject { resolve }
 
     context 'when the user does not have permission to create an issue' do
       it 'raises an error' do
@@ -61,6 +65,15 @@ RSpec.describe Mutations::Issues::Create do
       end
     end
 
+    context 'when the user has exceeded the rate limit' do
+      it 'raises an error' do
+        allow(::Gitlab::ApplicationRateLimiter).to receive(:throttled?).and_return(true)
+        project.add_developer(user)
+
+        expect { resolve }.to raise_error(RateLimitedService::RateLimitedError, _('This endpoint has been requested too many times. Try again later.'))
+      end
+    end
+
     context 'when the user can create an issue' do
       context 'when creating an issue a developer' do
         before do

spec/lib/gitlab/email/handler/create_issue_handler_spec.rb

@@ -136,6 +136,36 @@ RSpec.describe Gitlab::Email::Handler::CreateIssueHandler do
         expect { handler.execute }.to raise_error(Gitlab::Email::ProjectNotFound)
       end
     end
+
+    context 'rate limiting' do
+      let(:rate_limited_service_feature_enabled) { nil }
+
+      before do
+        stub_feature_flags(rate_limited_service_issues_create: rate_limited_service_feature_enabled)
+      end
+
+      context 'when :rate_limited_service Feature is disabled' do
+        let(:rate_limited_service_feature_enabled) { false }
+
+        it 'does not attempt to throttle' do
+          expect(::Gitlab::ApplicationRateLimiter).not_to receive(:throttled?)
+
+          setup_attachment
+          receiver.execute
+        end
+      end
+
+      context 'when :rate_limited_service Feature is enabled' do
+        let(:rate_limited_service_feature_enabled) { true }
+
+        it 'raises a RateLimitedService::RateLimitedError' do
+          allow(::Gitlab::ApplicationRateLimiter).to receive(:throttled?).and_return(true)
+
+          setup_attachment
+          expect { receiver.execute }.to raise_error(RateLimitedService::RateLimitedError, _('This endpoint has been requested too many times. Try again later.'))
+        end
+      end
+    end
   end
 
   def email_fixture(path)

spec/lib/gitlab/email/handler/service_desk_handler_spec.rb

@@ -243,6 +243,15 @@ RSpec.describe Gitlab::Email::Handler::ServiceDeskHandler do
           end
         end
       end
+
+      context 'when rate limiting is in effect' do
+        it 'allows unlimited new issue creation' do
+          stub_application_setting(issues_create_limit: 1)
+          setup_attachment
+
+          expect { 2.times { receiver.execute } }.to change { Issue.count }.by(2)
+        end
+      end
     end
 
     describe '#can_handle?' do

spec/requests/api/issues/post_projects_issues_spec.rb

@@ -399,16 +399,15 @@ RSpec.describe API::Issues do
     end
 
     context 'when request exceeds the rate limit' do
-      before do
+      it 'prevents users from creating more issues' do
         allow(::Gitlab::ApplicationRateLimiter).to receive(:throttled?).and_return(true)
-      end
 
-      it 'prevents users from creating more issues' do
         post api("/projects/#{project.id}/issues", user),
         params: { title: 'new issue', labels: 'label, label2', weight: 3, assignee_ids: [user2.id] }
 
-        expect(response).to have_gitlab_http_status(:too_many_requests)
         expect(json_response['message']['error']).to eq('This endpoint has been requested too many times. Try again later.')
+
+        expect(response).to have_gitlab_http_status(:too_many_requests)
       end
     end
   end

spec/services/concerns/rate_limited_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe RateLimitedService do
+  let(:key) { :issues_create }
+  let(:scope) { [:project, :current_user] }
+  let(:opts) { { scope: scope, users_allowlist: -> { [User.support_bot.username] } } }
+  let(:rate_limiter_klass) { ::Gitlab::ApplicationRateLimiter }
+  let(:rate_limiter_instance) { rate_limiter_klass.new(key, **opts) }
+
+  describe 'RateLimitedError' do
+    subject { described_class::RateLimitedError.new(key: key, rate_limiter: rate_limiter_instance) }
+
+    describe '#headers' do
+      it 'returns a Hash of HTTP headers' do
+        # TODO: This will be fleshed out in https://gitlab.com/gitlab-org/gitlab/-/issues/342370
+        expected_headers = {}
+
+        expect(subject.headers).to eq(expected_headers)
+      end
+    end
+
+    describe '#log_request' do
+      it 'logs the request' do
+        request = instance_double(Grape::Request)
+        user = instance_double(User)
+
+        expect(rate_limiter_klass).to receive(:log_request).with(request, "#{key}_request_limit".to_sym, user)
+
+        subject.log_request(request, user)
+      end
+    end
+  end
+
+  describe 'RateLimiterScopedAndKeyed' do
+    subject { described_class::RateLimiterScopedAndKeyed.new(key: key, opts: opts, rate_limiter_klass: rate_limiter_klass) }
+
+    describe '#rate_limit!' do
+      let(:project_with_feature_enabled) { create(:project) }
+      let(:project_without_feature_enabled) { create(:project) }
+
+      let(:project) { nil }
+
+      let(:current_user) { create(:user) }
+      let(:service) { instance_double(Issues::CreateService, project: project, current_user: current_user) }
+      let(:evaluated_scope) { [project, current_user] }
+      let(:evaluated_opts) { { scope: evaluated_scope, users_allowlist: %w[support-bot] } }
+      let(:rate_limited_service_issues_create_feature_enabled) { nil }
+
+      before do
+        allow(rate_limiter_klass).to receive(:new).with(key, **evaluated_opts).and_return(rate_limiter_instance)
+        stub_feature_flags(rate_limited_service_issues_create: rate_limited_service_issues_create_feature_enabled)
+      end
+
+      shared_examples 'a service that does not attempt to throttle' do
+        it 'does not attempt to throttle' do
+          expect(rate_limiter_instance).not_to receive(:throttled?)
+
+          expect(subject.rate_limit!(service)).to be_nil
+        end
+      end
+
+      shared_examples 'a service that does attempt to throttle' do
+        before do
+          allow(rate_limiter_instance).to receive(:throttled?).and_return(throttled)
+        end
+
+        context 'when rate limiting is not in effect' do
+          let(:throttled) { false }
+
+          it 'does not raise an exception' do
+            expect(subject.rate_limit!(service)).to be_nil
+          end
+        end
+
+        context 'when rate limiting is in effect' do
+          let(:throttled) { true }
+
+          it 'raises a RateLimitedError exception' do
+            expect { subject.rate_limit!(service) }.to raise_error(described_class::RateLimitedError, 'This endpoint has been requested too many times. Try again later.')
+          end
+        end
+      end
+
+      context 'when :rate_limited_service_issues_create feature is globally disabled' do
+        let(:rate_limited_service_issues_create_feature_enabled) { false }
+
+        it_behaves_like 'a service that does not attempt to throttle'
+      end
+
+      context 'when :rate_limited_service_issues_create feature is globally enabled' do
+        let(:throttled) { nil }
+        let(:rate_limited_service_issues_create_feature_enabled) { true }
+        let(:project) { project_without_feature_enabled }
+
+        it_behaves_like 'a service that does attempt to throttle'
+      end
+
+      context 'when :rate_limited_service_issues_create feature is enabled for project_with_feature_enabled' do
+        let(:throttled) { nil }
+        let(:rate_limited_service_issues_create_feature_enabled) { project_with_feature_enabled }
+
+        context 'for project_without_feature_enabled' do
+          let(:project) { project_without_feature_enabled }
+
+          it_behaves_like 'a service that does not attempt to throttle'
+        end
+
+        context 'for project_with_feature_enabled' do
+          let(:project) { project_with_feature_enabled }
+
+          it_behaves_like 'a service that does attempt to throttle'
+        end
+      end
+    end
+  end
+
+  describe '#execute_without_rate_limiting' do
+    let(:rate_limiter_scoped_and_keyed) { instance_double(RateLimitedService::RateLimiterScopedAndKeyed) }
+    let(:subject) do
+      local_key = key
+      local_opts = opts
+
+      Class.new do
+        prepend RateLimitedService
+
+        rate_limit key: local_key, opts: local_opts
+
+        def execute(*args, **kwargs)
+          'main logic here'
+        end
+      end.new
+    end
+
+    before do
+      allow(RateLimitedService::RateLimiterScopedAndKeyed).to receive(:new).with(key: key, opts: opts, rate_limiter_klass: rate_limiter_klass).and_return(rate_limiter_scoped_and_keyed)
+    end
+
+    context 'bypasses rate limiting' do
+      it 'calls super' do
+        expect(rate_limiter_scoped_and_keyed).not_to receive(:rate_limit!).with(subject)
+
+        expect(subject.execute_without_rate_limiting).to eq('main logic here')
+      end
+    end
+  end
+
+  describe '#execute' do
+    context 'when rate_limit has not been called' do
+      let(:subject) { Class.new { prepend RateLimitedService }.new }
+
+      it 'raises an RateLimitedNotSetupError exception' do
+        expect { subject.execute }.to raise_error(described_class::RateLimitedNotSetupError)
+      end
+    end
+
+    context 'when rate_limit has been called' do
+      let(:rate_limiter_scoped_and_keyed) { instance_double(RateLimitedService::RateLimiterScopedAndKeyed) }
+      let(:subject) do
+        local_key = key
+        local_opts = opts
+
+        Class.new do
+          prepend RateLimitedService
+
+          rate_limit key: local_key, opts: local_opts
+
+          def execute(*args, **kwargs)
+            'main logic here'
+          end
+        end.new
+      end
+
+      before do
+        allow(RateLimitedService::RateLimiterScopedAndKeyed).to receive(:new).with(key: key, opts: opts, rate_limiter_klass: rate_limiter_klass).and_return(rate_limiter_scoped_and_keyed)
+      end
+
+      context 'and applies rate limiting' do
+        it 'raises an RateLimitedService::RateLimitedError exception' do
+          expect(rate_limiter_scoped_and_keyed).to receive(:rate_limit!).with(subject).and_raise(RateLimitedService::RateLimitedError.new(key: key, rate_limiter: rate_limiter_instance))
+
+          expect { subject.execute }.to raise_error(RateLimitedService::RateLimitedError)
+        end
+      end
+
+      context 'but does not apply rate limiting' do
+        it 'calls super' do
+          expect(rate_limiter_scoped_and_keyed).to receive(:rate_limit!).with(subject).and_return(nil)
+
+          expect(subject.execute).to eq('main logic here')
+        end
+      end
+    end
+  end
+end

spec/services/issues/create_service_spec.rb

@@ -10,6 +10,25 @@ RSpec.describe Issues::CreateService do
 
   let(:spam_params) { double }
 
+  describe '.rate_limiter_scoped_and_keyed' do
+    it 'is set via the rate_limit call' do
+      expect(described_class.rate_limiter_scoped_and_keyed).to be_a(RateLimitedService::RateLimiterScopedAndKeyed)
+
+      expect(described_class.rate_limiter_scoped_and_keyed.key).to eq(:issues_create)
+      expect(described_class.rate_limiter_scoped_and_keyed.opts[:scope]).to eq(%i[project current_user])
+      expect(described_class.rate_limiter_scoped_and_keyed.opts[:users_allowlist].call).to eq(%w[support-bot])
+      expect(described_class.rate_limiter_scoped_and_keyed.rate_limiter_klass).to eq(Gitlab::ApplicationRateLimiter)
+    end
+  end
+
+  describe '#rate_limiter_bypassed' do
+    let(:subject) { described_class.new(project: project, spam_params: {}) }
+
+    it 'is nil by default' do
+      expect(subject.rate_limiter_bypassed).to be_nil
+    end
+  end
+
   describe '#execute' do
     let_it_be(:assignee) { create(:user) }
     let_it_be(:milestone) { create(:milestone, project: project) }

spec/workers/email_receiver_worker_spec.rb

@@ -80,6 +80,21 @@ RSpec.describe EmailReceiverWorker, :mailer do
           expect(email).to be_nil
         end
       end
+
+      context 'when the error is RateLimitedService::RateLimitedError' do
+        let(:error) { RateLimitedService::RateLimitedError.new(key: :issues_create, rate_limiter: Gitlab::ApplicationRateLimiter) }
+
+        it 'does not report the error to the sender' do
+          expect(Gitlab::ErrorTracking).to receive(:track_exception).with(error).and_call_original
+
+          perform_enqueued_jobs do
+            described_class.new.perform(raw_message)
+          end
+
+          email = ActionMailer::Base.deliveries.last
+          expect(email).to be_nil
+        end
+      end
     end
   end