Merge branch '339644-sd-limit' into 'master'

Limit issues one user can quickly create using service desk

See merge request gitlab-org/gitlab!73346

app/services/issues/create_service.rb

@@ -6,7 +6,7 @@ module Issues
     prepend RateLimitedService
 
     rate_limit key: :issues_create,
-               opts: { scope: [:project, :current_user], users_allowlist: -> { [User.support_bot.username] } }
+               opts: { scope: [:project, :current_user, :external_author] }
 
     # 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
@@ -25,6 +25,10 @@ module Issues
       create(@issue, skip_system_notes: skip_system_notes)
     end
 
+    def external_author
+      params[:external_author] # present when creating an issue using service desk (email: from)
+    end
+
     def before_create(issue)
       Spam::SpamActionService.new(
         spammable: issue,

spec/fixtures/emails/service_desk_forwarded.eml

 Delivered-To: incoming+email-test-project_id-issue-@appmail.adventuretime.ooo
-Return-Path: <jake@adventuretime.ooo>
+Return-Path: <jake.g@adventuretime.ooo>
 Received: from iceking.adventuretime.ooo ([unix socket]) by iceking (Cyrus v2.2.13-Debian-2.2.13-19+squeeze3) with LMTPA; Thu, 13 Jun 2013 17:03:50 -0400
 Received: from mail-ie0-x234.google.com (mail-ie0-x234.google.com [IPv6:2607:f8b0:4001:c03::234]) by iceking.adventuretime.ooo (8.14.3/8.14.3/Debian-9.4) with ESMTP id r5DL3nFJ016967 (version=TLSv1/SSLv3 cipher=RC4-SHA bits=128 verify=NOT) for <incoming+gitlabhq/gitlabhq@appmail.adventuretime.ooo>; Thu, 13 Jun 2013 17:03:50 -0400
 Received: by mail-ie0-f180.google.com with SMTP id f4so21977375iea.25 for <incoming+email-test-project_id-issue-@appmail.adventuretime.ooo>; Thu, 13 Jun 2013 14:03:48 -0700
 Received: by 10.0.0.1 with HTTP; Thu, 13 Jun 2013 14:03:48 -0700
 Date: Thu, 13 Jun 2013 17:03:48 -0400
-From: Jake the Dog <jake@adventuretime.ooo>
+From: Jake the Dog <jake.g@adventuretime.ooo>
 To: support@adventuretime.ooo
 Delivered-To: support@adventuretime.ooo
 Message-ID: <CADkmRc+rNGAGGbV2iE5p918UVy4UyJqVcXRO2=otppgzduJSg@mail.gmail.com>

spec/lib/gitlab/application_rate_limiter_spec.rb

@@ -99,6 +99,16 @@ RSpec.describe Gitlab::ApplicationRateLimiter do
         end
 
         it_behaves_like 'action rate limiter'
+
+        context 'when a scope attribute is nil' do
+          let(:scope) { [user, nil] }
+
+          let(:cache_key) do
+            "application_rate_limiter:test_action:user:#{user.id}"
+          end
+
+          it_behaves_like 'action rate limiter'
+        end
       end
 
       context 'when the key is a combination of ActiveRecord models and strings' do
@@ -112,6 +122,16 @@ RSpec.describe Gitlab::ApplicationRateLimiter do
         end
 
         it_behaves_like 'action rate limiter'
+
+        context 'when a scope attribute is nil' do
+          let(:scope) { [project, commit, nil] }
+
+          let(:cache_key) do
+            "application_rate_limiter:test_action:project:#{project.id}:commit:#{commit.sha}"
+          end
+
+          it_behaves_like 'action rate limiter'
+        end
       end
     end
 

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

@@ -11,6 +11,7 @@ RSpec.describe Gitlab::Email::Handler::ServiceDeskHandler do
   end
 
   let(:email_raw) { email_fixture('emails/service_desk.eml') }
+  let(:author_email) { 'jake@adventuretime.ooo' }
   let_it_be(:group) { create(:group, :private, name: "email") }
 
   let(:expected_description) do
@@ -45,7 +46,7 @@ RSpec.describe Gitlab::Email::Handler::ServiceDeskHandler do
         receiver.execute
         new_issue = Issue.last
 
-        expect(new_issue.issue_email_participants.first.email).to eq("jake@adventuretime.ooo")
+        expect(new_issue.issue_email_participants.first.email).to eq(author_email)
       end
 
       it 'sends thank you email' do
@@ -256,13 +257,60 @@ RSpec.describe Gitlab::Email::Handler::ServiceDeskHandler do
           it_behaves_like 'a new issue request'
         end
       end
+    end
+
+    context 'when rate limiting is in effect', :clean_gitlab_redis_cache do
+      let(:receiver) { Gitlab::Email::Receiver.new(email_raw) }
 
-      context 'when rate limiting is in effect' do
-        it 'allows unlimited new issue creation' do
+      subject { 2.times { receiver.execute } }
+
+      before do
+        stub_feature_flags(rate_limited_service_issues_create: true)
         stub_application_setting(issues_create_limit: 1)
-          setup_attachment
+      end
+
+      context 'when too many requests are sent by one user' do
+        it 'raises an error' do
+          freeze_time do
+            expect { subject }.to raise_error(RateLimitedService::RateLimitedError)
+          end
+        end
+
+        it 'creates 1 issue' do
+          freeze_time do
+            expect do
+              subject
+            rescue RateLimitedService::RateLimitedError
+            end.to change { Issue.count }.by(1)
+          end
+        end
+
+        context 'when requests are sent by different users' do
+          let(:email_raw_2) { email_fixture('emails/service_desk_forwarded.eml') }
+          let(:receiver2) { Gitlab::Email::Receiver.new(email_raw_2) }
 
-          expect { 2.times { receiver.execute } }.to change { Issue.count }.by(2)
+          subject do
+            receiver.execute
+            receiver2.execute
+          end
+
+          it 'creates 2 issues' do
+            freeze_time do
+              expect { subject }.to change { Issue.count }.by(2)
+            end
+          end
+        end
+      end
+
+      context 'when limit is higher than sent emails' do
+        before do
+          stub_application_setting(issues_create_limit: 3)
+        end
+
+        it 'creates 2 issues' do
+          freeze_time do
+            expect { subject }.to change { Issue.count }.by(2)
+          end
         end
       end
     end
@@ -336,6 +384,7 @@ RSpec.describe Gitlab::Email::Handler::ServiceDeskHandler do
     end
 
     context 'when the email is forwarded through an alias' do
+      let(:author_email) { 'jake.g@adventuretime.ooo' }
       let(:email_raw) { email_fixture('emails/service_desk_forwarded.eml') }
 
       it_behaves_like 'a new issue request'

spec/services/issues/create_service_spec.rb

@@ -15,8 +15,7 @@ RSpec.describe Issues::CreateService 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.opts[:scope]).to eq(%i[project current_user external_author])
       expect(described_class.rate_limiter_scoped_and_keyed.rate_limiter_klass).to eq(Gitlab::ApplicationRateLimiter)
     end
   end
@@ -289,6 +288,50 @@ 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', :clean_gitlab_redis_cache do
+        let(:user) { create(:user) }
+
+        before do
+          stub_feature_flags(rate_limited_service_issues_create: true)
+          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
+            freeze_time do
+              expect do
+                subject
+              end.to raise_error(RateLimitedService::RateLimitedError)
+            end
+          end
+
+          it 'creates 1 issue' do
+            freeze_time do
+              expect do
+                subject
+              rescue RateLimitedService::RateLimitedError
+              end.to change { Issue.count }.by(1)
+            end
+          end
+        end
+
+        context 'when limit is higher than counf of issues being created' do
+          before do
+            stub_application_setting(issues_create_limit: 3)
+          end
+
+          it 'creates 2 issues' do
+            freeze_time do
+              expect { subject }.to change { Issue.count }.by(2)
+            end
+          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}" } }