Add latest changes from gitlab-org/gitlab@master

7 files changed, 222 insertions, 10 deletions

diff --git a/spec/lib/gitlab/import_export/all_models.yml b/spec/lib/gitlab/import_export/all_models.yml
index 13330f56a31..f4f13896837 100644
--- a/spec/lib/gitlab/import_export/all_models.yml
+++ b/spec/lib/gitlab/import_export/all_models.yml
@@ -94,6 +94,7 @@ notes:
 - diff_note_positions
 - review
 - note_metadata
+- user_agent_detail
 note_metadata:
   - note
   - email_participant
@@ -112,6 +113,7 @@ commit_notes:
 - diff_note_positions
 - review
 - note_metadata
+- user_agent_detail
 label_links:
 - target
 - label
diff --git a/spec/models/concerns/spammable_spec.rb b/spec/models/concerns/spammable_spec.rb
index 44cf87aa1c1..457c831e27c 100644
--- a/spec/models/concerns/spammable_spec.rb
+++ b/spec/models/concerns/spammable_spec.rb
@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-RSpec.describe Spammable do
+RSpec.describe Spammable, feature_category: :instance_resiliency do
   let(:issue) { create(:issue, description: 'Test Desc.') }
 
   describe 'Associations' do
@@ -59,6 +59,18 @@ RSpec.describe Spammable do
         end
       end
 
+      context 'when the model needs recaptcha but does not support it' do
+        subject { invalidate_if_spam(needs_recaptcha: true) }
+
+        before do
+          allow(issue).to receive(:supports_recaptcha?).and_return(false)
+        end
+
+        it 'has an error that discards the spammable' do
+          expect(subject.errors.messages[:base]).to match_array /has been discarded/
+        end
+      end
+
       context 'if the model is spam and also needs recaptcha' do
         subject { invalidate_if_spam(is_spam: true, needs_recaptcha: true) }
 
@@ -112,11 +124,26 @@ RSpec.describe Spammable do
       end
 
       describe '#needs_recaptcha!' do
-        it 'adds `needs_recaptcha` flag' do
-          issue.needs_recaptcha!
+        context 'when recaptcha is supported' do
+          it 'adds `needs_recaptcha` flag' do
+            issue.needs_recaptcha!
+
+            expect(issue.spam).to be_falsey
+            expect(issue.needs_recaptcha).to be_truthy
+          end
+        end
 
-          expect(issue.spam).to be_falsey
-          expect(issue.needs_recaptcha).to be_truthy
+        context 'when recaptcha is not supported' do
+          before do
+            allow(issue).to receive(:supports_recaptcha?).and_return(false)
+          end
+
+          it 'marks the object as spam' do
+            issue.needs_recaptcha!
+
+            expect(issue.spam).to be_truthy
+            expect(issue.needs_recaptcha).to be_falsey
+          end
         end
       end
 
diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb
index 1453ce9709f..f82235916f2 100644
--- a/spec/models/note_spec.rb
+++ b/spec/models/note_spec.rb
@@ -933,6 +933,107 @@ RSpec.describe Note, feature_category: :team_planning do
     end
   end
 
+  describe '#check_for_spam' do
+    let_it_be(:project) { create(:project, :public) }
+    let_it_be(:group)   { create(:group, :public) }
+    let(:issue)     { create(:issue, project: project) }
+    let(:note)      { create(:note, note: "test", noteable: issue, project: project) }
+    let(:note_text) { 'content changed' }
+
+    subject do
+      note.assign_attributes(note: note_text)
+      note.check_for_spam?(user: note.author)
+    end
+
+    before do
+      allow(issue).to receive(:group).and_return(group)
+    end
+
+    context 'when note is public' do
+      it 'returns true' do
+        is_expected.to be_truthy
+      end
+    end
+
+    context 'when note is public and spammable attributes are not changed' do
+      let(:note_text) { 'test' }
+
+      it 'returns false' do
+        is_expected.to be_falsey
+      end
+    end
+
+    context 'when project does not exist' do
+      before do
+        allow(note).to receive(:project).and_return(nil)
+      end
+
+      it 'returns true' do
+        is_expected.to be_truthy
+      end
+    end
+
+    context 'when project is not public' do
+      before do
+        allow(project).to receive(:public?).and_return(false)
+      end
+
+      it 'returns false' do
+        is_expected.to be_falsey
+      end
+    end
+
+    context 'when group is not public' do
+      before do
+        allow(group).to receive(:public?).and_return(false)
+      end
+
+      it 'returns false' do
+        is_expected.to be_falsey
+      end
+    end
+
+    context 'when note is confidential' do
+      before do
+        allow(note).to receive(:confidential?).and_return(true)
+      end
+
+      it 'returns false' do
+        is_expected.to be_falsey
+      end
+    end
+
+    context 'when noteable is confidential' do
+      before do
+        allow(issue).to receive(:confidential?).and_return(true)
+      end
+
+      it 'returns false' do
+        is_expected.to be_falsey
+      end
+    end
+
+    context 'when noteable is not public' do
+      before do
+        allow(issue).to receive(:public?).and_return(false)
+      end
+
+      it 'returns false' do
+        is_expected.to be_falsey
+      end
+    end
+
+    context 'when note is a system note' do
+      before do
+        allow(note).to receive(:system?).and_return(true)
+      end
+
+      it 'returns false' do
+        is_expected.to be_falsey
+      end
+    end
+  end
+
   describe ".grouped_diff_discussions" do
     let!(:merge_request) { create(:merge_request) }
     let(:project) { merge_request.project }
diff --git a/spec/models/sent_notification_spec.rb b/spec/models/sent_notification_spec.rb
index 8194150532d..5b31e8e5e3c 100644
--- a/spec/models/sent_notification_spec.rb
+++ b/spec/models/sent_notification_spec.rb
@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-RSpec.describe SentNotification do
+RSpec.describe SentNotification, :request_store do
   let_it_be(:user) { create(:user) }
   let_it_be(:project) { create(:project) }
 
@@ -46,21 +46,31 @@ RSpec.describe SentNotification do
     end
   end
 
+  shared_examples 'a non-sticky write' do
+    it 'writes without sticking to primary' do
+      subject
+
+      expect(Gitlab::Database::LoadBalancing::Session.current.use_primary?).to be false
+    end
+  end
+
   describe '.record' do
-    let(:issue) { create(:issue) }
+    let_it_be(:issue) { create(:issue) }
 
     subject { described_class.record(issue, user.id) }
 
     it_behaves_like 'a successful sent notification'
+    it_behaves_like 'a non-sticky write'
   end
 
   describe '.record_note' do
     subject { described_class.record_note(note, note.author.id) }
 
     context 'for a discussion note' do
-      let(:note) { create(:diff_note_on_merge_request) }
+      let_it_be(:note) { create(:diff_note_on_merge_request) }
 
       it_behaves_like 'a successful sent notification'
+      it_behaves_like 'a non-sticky write'
 
       it 'sets in_reply_to_discussion_id' do
         expect(subject.in_reply_to_discussion_id).to eq(note.discussion_id)
@@ -68,9 +78,10 @@ RSpec.describe SentNotification do
     end
 
     context 'for an individual note' do
-      let(:note) { create(:note_on_merge_request) }
+      let_it_be(:note) { create(:note_on_merge_request) }
 
       it_behaves_like 'a successful sent notification'
+      it_behaves_like 'a non-sticky write'
 
       it 'sets in_reply_to_discussion_id' do
         expect(subject.in_reply_to_discussion_id).to eq(note.discussion_id)
diff --git a/spec/services/notes/create_service_spec.rb b/spec/services/notes/create_service_spec.rb
index 5a5e8691a76..89114bf3fd0 100644
--- a/spec/services/notes/create_service_spec.rb
+++ b/spec/services/notes/create_service_spec.rb
@@ -30,6 +30,24 @@ RSpec.describe Notes::CreateService, feature_category: :team_planning do
         expect(note).to be_persisted
       end
 
+      it 'checks for spam' do
+        expect_next_instance_of(Note) do |instance|
+          expect(instance).to receive(:check_for_spam).with(action: :create, user: user)
+        end
+
+        note
+      end
+
+      it 'does not persist when spam' do
+        expect_next_instance_of(Note) do |instance|
+          expect(instance).to receive(:check_for_spam).with(action: :create, user: user) do
+            instance.spam!
+          end
+        end
+
+        expect(note).not_to be_persisted
+      end
+
       context 'with internal parameter' do
         context 'when confidential' do
           let(:opts) { base_opts.merge(internal: true) }
@@ -511,7 +529,7 @@ RSpec.describe Notes::CreateService, feature_category: :team_planning do
         end
       end
 
-      context 'when note only have commands' do
+      context 'when note only has commands' do
         it 'adds commands applied message to note errors' do
           note_text = %(/close)
           service = double(:service)
@@ -540,6 +558,15 @@ RSpec.describe Notes::CreateService, feature_category: :team_planning do
 
           expect(note.errors[:commands_only]).to contain_exactly('Closed this issue. Could not apply reopen command.')
         end
+
+        it 'does not check for spam' do
+          expect_next_instance_of(Note) do |instance|
+            expect(instance).not_to receive(:check_for_spam).with(action: :create, user: user)
+          end
+
+          note_text = %(/close)
+          described_class.new(project, user, opts.merge(note: note_text)).execute
+        end
       end
     end
 
diff --git a/spec/services/notes/update_service_spec.rb b/spec/services/notes/update_service_spec.rb
index 808ab46ef99..01b61acb476 100644
--- a/spec/services/notes/update_service_spec.rb
+++ b/spec/services/notes/update_service_spec.rb
@@ -90,6 +90,11 @@ RSpec.describe Notes::UpdateService, feature_category: :team_planning do
         end
       end
 
+      it 'checks for spam' do
+        expect(note).to receive(:check_for_spam).with(action: :update, user: user)
+        edit_note_text
+      end
+
       context 'when quick action only update' do
         it "delete note and return commands_only error" do
           updated_note = described_class.new(project, user, { note: "/close\n" }).execute(note)
@@ -118,6 +123,11 @@ RSpec.describe Notes::UpdateService, feature_category: :team_planning do
           expect { does_not_edit_note_text }.not_to change { note.reload.updated_by }
         end
       end
+
+      it 'does not check for spam' do
+        expect(note).not_to receive(:check_for_spam)
+        does_not_edit_note_text
+      end
     end
 
     context 'when the notable is a merge request' do
diff --git a/spec/services/spam/spam_action_service_spec.rb b/spec/services/spam/spam_action_service_spec.rb
index 7072f5879ea..15cb4977b61 100644
--- a/spec/services/spam/spam_action_service_spec.rb
+++ b/spec/services/spam/spam_action_service_spec.rb
@@ -55,6 +55,21 @@ RSpec.describe Spam::SpamActionService, feature_category: :instance_resiliency d
         expect(issue).not_to be_spam
       end
     end
+
+    context 'when user is nil' do
+      let(:spam_params) { true }
+      let(:user) { nil }
+      let(:expected_service_user_not_present_message) do
+        /Skipped spam check because user was not present/
+      end
+
+      it "returns success with a messaage" do
+        response = subject
+
+        expect(response.message).to match(expected_service_user_not_present_message)
+        expect(issue).not_to be_spam
+      end
+    end
   end
 
   shared_examples 'allows user' do
@@ -120,6 +135,7 @@ RSpec.describe Spam::SpamActionService, feature_category: :instance_resiliency d
     before do
       allow(Captcha::CaptchaVerificationService).to receive(:new).with(spam_params: spam_params) { fake_captcha_verification_service }
       allow(Spam::SpamVerdictService).to receive(:new).with(verdict_service_args).and_return(fake_verdict_service)
+      allow(fake_verdict_service).to receive(:execute).and_return({})
     end
 
     context 'when captcha response verification returns true' do
@@ -170,6 +186,24 @@ RSpec.describe Spam::SpamActionService, feature_category: :instance_resiliency d
           target.description = 'Lovely Spam! Wonderful Spam!'
         end
 
+        context 'when captcha is not supported' do
+          before do
+            allow(target).to receive(:supports_recaptcha?).and_return(false)
+          end
+
+          it "does not execute with captcha support" do
+            expect(Captcha::CaptchaVerificationService).not_to receive(:new)
+
+            subject
+          end
+
+          it "executes a spam check" do
+            expect(fake_verdict_service).to receive(:execute)
+
+            subject
+          end
+        end
+
         context 'when user is a gitlab bot' do
           before do
             allow(user).to receive(:gitlab_bot?).and_return(true)