Merge branch 'jp-epic-rlimit' into 'master'

Rate limit epic create service

See merge request gitlab-org/gitlab!80909

ee/app/controllers/groups/epics_controller.rb

@@ -16,9 +16,6 @@ class Groups::EpicsController < Groups::ApplicationController
   before_action :verify_group_bulk_edit_enabled!, only: [:bulk_update]
   after_action :log_epic_show, only: :show
 
-  # Limit the amount of epics created per minute. Epics share the issue creation rate limit
-  before_action -> { check_rate_limit!(:issues_create, scope: current_user) }, only: [:create]
-
   before_action do
     push_frontend_feature_flag(:improved_emoji_picker, @group, type: :development, default_enabled: :yaml)
   end

ee/app/services/epics/create_service.rb

@@ -2,6 +2,10 @@
 
 module Epics
   class CreateService < Epics::BaseService
+    prepend RateLimitedService
+
+    rate_limit key: :issues_create, opts: { scope: [:current_user] }
+
     def execute
       set_date_params
 

ee/spec/graphql/mutations/epics/create_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Mutations::Epics::Create do
+  let_it_be(:group) { create(:group) }
+  let_it_be(:epic) { create(:epic, group: group) }
+  let_it_be(:user) { create(:user) }
+
+  subject(:mutation) { described_class.new(object: group, context: { current_user: user }, field: nil) }
+
+  describe '#resolve' do
+    before do
+      stub_licensed_features(epics: true)
+    end
+
+    subject { mutation.resolve(group_path: group.full_path, title: 'new epic title') }
+
+    it 'raises a not accessible error' do
+      expect { subject }.to raise_error(Gitlab::Graphql::Errors::ResourceNotAvailable)
+    end
+
+    context 'when the user can create epics' do
+      before do
+        group.add_developer(user)
+      end
+
+      it 'creates a new epic' do
+        expect(subject[:epic][:title]).to eq('new epic title')
+        expect(subject[:errors]).to be_empty
+      end
+
+      context 'with rate limiter', :freeze_time, :clean_gitlab_redis_rate_limiting do
+        before do
+          stub_application_setting(issues_create_limit: 1)
+        end
+
+        it 'prevents users from creating more epics' do
+          result = mutation.resolve(group_path: group.full_path, title: 'new epic title')
+
+          expect(result[:errors]).to be_empty
+
+          expect do
+            mutation.resolve(group_path: group.full_path, title: 'new epic title')
+          end.to raise_error(RateLimitedService::RateLimitedError)
+        end
+      end
+    end
+  end
+end

ee/spec/graphql/mutations/requirements_management/create_requirement_spec.rb

@@ -25,7 +25,7 @@ RSpec.describe Mutations::RequirementsManagement::CreateRequirement do
 
     it_behaves_like 'requirements not available'
 
-    context 'when the user can update the epic' do
+    context 'when the user can create requirements' do
       before do
         project.add_developer(user)
       end

ee/spec/requests/api/epics_spec.rb

@@ -727,6 +727,23 @@ RSpec.describe API::Epics do
         end
       end
 
+      context 'with rate limiter', :freeze_time, :clean_gitlab_redis_rate_limiting do
+        before do
+          stub_application_setting(issues_create_limit: 1)
+        end
+
+        it 'prevents users from creating more epics' do
+          post api(url, user), params: params
+
+          expect(response).to have_gitlab_http_status(:created)
+
+          post api(url, user), params: params
+
+          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.')
+        end
+      end
+
       context 'setting created_at' do
         let(:creation_time) { 2.weeks.ago }
         let(:params) { { title: 'new epic', created_at: creation_time } }

ee/spec/services/epics/create_service_spec.rb

@@ -11,6 +11,14 @@ RSpec.describe Epics::CreateService do
 
   subject { described_class.new(group: group, current_user: user, params: params).execute }
 
+  it_behaves_like 'rate limited service' do
+    let(:key) { :issues_create }
+    let(:key_scope) { %i[current_user] }
+    let(:application_limit_key) { :issues_create_limit }
+    let(:created_model) { Epic }
+    let(:service) { described_class.new(group: group, current_user: user, params: params) }
+  end
+
   describe '#execute' do
     before do
       group.add_reporter(user)

spec/services/issues/create_service_spec.rb

@@ -11,22 +11,12 @@ 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 external_author])
-      expect(described_class.rate_limiter_scoped_and_keyed.rate_limiter).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
+  it_behaves_like 'rate limited service' do
+    let(:key) { :issues_create }
+    let(:key_scope) { %i[project current_user external_author] }
+    let(:application_limit_key) { :issues_create_limit }
+    let(:created_model) { Issue }
+    let(:service) { described_class.new(project: project, current_user: user, params: { title: 'title' }, spam_params: double) }
   end
 
   describe '#execute' do
@@ -331,43 +321,6 @@ RSpec.describe Issues::CreateService do
         described_class.new(project: project, current_user: user, params: opts, spam_params: spam_params).execute
       end
 
-      context 'when rate limiting is in effect', :freeze_time, :clean_gitlab_redis_rate_limiting do
-        let(:user) { create(:user) }
-
-        before do
-          stub_application_setting(issues_create_limit: 1)
-        end
-
-        subject do
-          2.times { described_class.new(project: project, current_user: user, params: opts, spam_params: double).execute }
-        end
-
-        context 'when too many requests are sent by one user' do
-          it 'raises an error' do
-            expect do
-              subject
-            end.to raise_error(RateLimitedService::RateLimitedError)
-          end
-
-          it 'creates 1 issue' do
-            expect do
-              subject
-            rescue RateLimitedService::RateLimitedError
-            end.to change { Issue.count }.by(1)
-          end
-        end
-
-        context 'when limit is higher than count of issues being created' do
-          before do
-            stub_application_setting(issues_create_limit: 2)
-          end
-
-          it 'creates 2 issues' do
-            expect { subject }.to change { Issue.count }.by(2)
-          end
-        end
-      end
-
       context 'after_save callback to store_mentions' do
         context 'when mentionable attributes change' do
           let(:opts) { { title: 'Title', description: "Description with #{user.to_reference}" } }

spec/support/shared_examples/services/rate_limited_service_shared_examples.rb

+# frozen_string_literal: true
+
+# shared examples for testing rate limited functionality of a service
+#
+# following resources are expected to be set (example):
+#  it_behaves_like 'rate limited service' do
+#    let(:key) { :issues_create }
+#    let(:key_scope) { %i[project current_user external_author] }
+#    let(:application_limit_key) { :issues_create_limit }
+#    let(:service) { described_class.new(project: project, current_user: user, params: { title: 'title' }, spam_params: double) }
+#    let(:created_model) { Issue }
+#  end
+
+RSpec.shared_examples 'rate limited service' do
+  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(key)
+      expect(described_class.rate_limiter_scoped_and_keyed.opts[:scope]).to eq(key_scope)
+      expect(described_class.rate_limiter_scoped_and_keyed.rate_limiter).to eq(Gitlab::ApplicationRateLimiter)
+    end
+  end
+
+  describe '#rate_limiter_bypassed' do
+    it 'is nil by default' do
+      expect(service.rate_limiter_bypassed).to be_nil
+    end
+  end
+
+  describe '#execute' do
+    before do
+      stub_spam_services
+    end
+
+    context 'when rate limiting is in effect', :freeze_time, :clean_gitlab_redis_rate_limiting do
+      let(:user) { create(:user) }
+
+      before do
+        stub_application_setting(application_limit_key => 1)
+      end
+
+      subject do
+        2.times { service.execute }
+      end
+
+      context 'when too many requests are sent by one user' do
+        it 'raises an error' do
+          expect do
+            subject
+          end.to raise_error(RateLimitedService::RateLimitedError)
+        end
+
+        it 'creates 1 issue' do
+          expect do
+            subject
+          rescue RateLimitedService::RateLimitedError
+          end.to change { created_model.count }.by(1)
+        end
+      end
+
+      context 'when limit is higher than count of issues being created' do
+        before do
+          stub_application_setting(issues_create_limit: 2)
+        end
+
+        it 'creates 2 issues' do
+          expect { subject }.to change { created_model.count }.by(2)
+        end
+      end
+    end
+  end
+end