Add latest changes from gitlab-org/gitlab@13-12-stable-eev13.12.0-rc42

2 files changed, 205 insertions, 38 deletions

diff --git a/spec/services/spam/spam_action_service_spec.rb b/spec/services/spam/spam_action_service_spec.rb
index e8ac826df1c..9ca52b92267 100644
--- a/spec/services/spam/spam_action_service_spec.rb
+++ b/spec/services/spam/spam_action_service_spec.rb
@@ -9,11 +9,11 @@ RSpec.describe Spam::SpamActionService do
   let(:issue) { create(:issue, project: project, author: user) }
   let(:fake_ip) { '1.2.3.4' }
   let(:fake_user_agent) { 'fake-user-agent' }
-  let(:fake_referrer) { 'fake-http-referrer' }
+  let(:fake_referer) { 'fake-http-referer' }
   let(:env) do
     { 'action_dispatch.remote_ip' => fake_ip,
       'HTTP_USER_AGENT' => fake_user_agent,
-      'HTTP_REFERRER' => fake_referrer }
+      'HTTP_REFERER' => fake_referer }
   end
 
   let_it_be(:project) { create(:project, :public) }
@@ -80,7 +80,7 @@ RSpec.describe Spam::SpamActionService do
       {
         ip_address: fake_ip,
         user_agent: fake_user_agent,
-        referrer: fake_referrer
+        referer: fake_referer
       }
     end
 
@@ -222,6 +222,38 @@ RSpec.describe Spam::SpamActionService do
           end
         end
 
+        context 'spam verdict service advises to block the user' do
+          before do
+            allow(fake_verdict_service).to receive(:execute).and_return(BLOCK_USER)
+          end
+
+          context 'when allow_possible_spam feature flag is false' do
+            before do
+              stub_feature_flags(allow_possible_spam: false)
+            end
+
+            it_behaves_like 'only checks for spam if a request is provided'
+
+            it 'marks as spam' do
+              response = subject
+
+              expect(response.message).to match(expected_service_check_response_message)
+              expect(issue).to be_spam
+            end
+          end
+
+          context 'when allow_possible_spam feature flag is true' do
+            it_behaves_like 'only checks for spam if a request is provided'
+
+            it 'does not mark as spam' do
+              response = subject
+
+              expect(response.message).to match(expected_service_check_response_message)
+              expect(issue).not_to be_spam
+            end
+          end
+        end
+
         context 'when spam verdict service conditionally allows' do
           before do
             allow(fake_verdict_service).to receive(:execute).and_return(CONDITIONAL_ALLOW)
@@ -281,6 +313,22 @@ RSpec.describe Spam::SpamActionService do
           end
         end
 
+        context 'when spam verdict service returns noop' do
+          before do
+            allow(fake_verdict_service).to receive(:execute).and_return(NOOP)
+          end
+
+          it 'does not create a spam log' do
+            expect { subject }.not_to change(SpamLog, :count)
+          end
+
+          it 'clears spam flags' do
+            expect(issue).to receive(:clear_spam_flags!)
+
+            subject
+          end
+        end
+
         context 'with spam verdict service options' do
           before do
             allow(fake_verdict_service).to receive(:execute).and_return(ALLOW)
diff --git a/spec/services/spam/spam_verdict_service_spec.rb b/spec/services/spam/spam_verdict_service_spec.rb
index 14b788e3a86..215df81de63 100644
--- a/spec/services/spam/spam_verdict_service_spec.rb
+++ b/spec/services/spam/spam_verdict_service_spec.rb
@@ -7,28 +7,33 @@ RSpec.describe Spam::SpamVerdictService do
 
   let(:fake_ip) { '1.2.3.4' }
   let(:fake_user_agent) { 'fake-user-agent' }
-  let(:fake_referrer) { 'fake-http-referrer' }
+  let(:fake_referer) { 'fake-http-referer' }
   let(:env) do
     { 'action_dispatch.remote_ip' => fake_ip,
       'HTTP_USER_AGENT' => fake_user_agent,
-      'HTTP_REFERRER' => fake_referrer }
+      'HTTP_REFERER' => fake_referer }
   end
 
   let(:request) { double(:request, env: env) }
 
   let(:check_for_spam) { true }
   let_it_be(:user) { create(:user) }
-  let(:issue) { build(:issue, author: user) }
+  let_it_be(:issue) { create(:issue, author: user) }
   let(:service) do
     described_class.new(user: user, target: issue, request: request, options: {})
   end
 
+  let(:attribs) do
+    extra_attributes = { "monitorMode" => "false" }
+    extra_attributes
+  end
+
   describe '#execute' do
     subject { service.execute }
 
     before do
       allow(service).to receive(:akismet_verdict).and_return(nil)
-      allow(service).to receive(:external_verdict).and_return(nil)
+      allow(service).to receive(:spamcheck_verdict).and_return([nil, attribs])
     end
 
     context 'if all services return nil' do
@@ -63,7 +68,7 @@ RSpec.describe Spam::SpamVerdictService do
       context 'and they are supported' do
         before do
           allow(service).to receive(:akismet_verdict).and_return(DISALLOW)
-          allow(service).to receive(:external_verdict).and_return(BLOCK_USER)
+          allow(service).to receive(:spamcheck_verdict).and_return([BLOCK_USER, attribs])
         end
 
         it 'renders the more restrictive verdict' do
@@ -74,7 +79,7 @@ RSpec.describe Spam::SpamVerdictService do
       context 'and one is supported' do
         before do
           allow(service).to receive(:akismet_verdict).and_return('nonsense')
-          allow(service).to receive(:external_verdict).and_return(BLOCK_USER)
+          allow(service).to receive(:spamcheck_verdict).and_return([BLOCK_USER, attribs])
         end
 
         it 'renders the more restrictive verdict' do
@@ -85,13 +90,56 @@ RSpec.describe Spam::SpamVerdictService do
       context 'and none are supported' do
         before do
           allow(service).to receive(:akismet_verdict).and_return('nonsense')
-          allow(service).to receive(:external_verdict).and_return('rubbish')
+          allow(service).to receive(:spamcheck_verdict).and_return(['rubbish', attribs])
         end
 
         it 'renders the more restrictive verdict' do
           expect(subject).to eq ALLOW
         end
       end
+
+      context 'and attribs - monitorMode is true' do
+        let(:attribs) do
+          extra_attributes = { "monitorMode" => "true" }
+          extra_attributes
+        end
+
+        before do
+          allow(service).to receive(:akismet_verdict).and_return(DISALLOW)
+          allow(service).to receive(:spamcheck_verdict).and_return([BLOCK_USER, attribs])
+        end
+
+        it 'renders the more restrictive verdict' do
+          expect(subject).to eq(DISALLOW)
+        end
+      end
+    end
+
+    context 'records metrics' do
+      let(:histogram) { instance_double(Prometheus::Client::Histogram) }
+
+      using RSpec::Parameterized::TableSyntax
+
+      where(:verdict, :error, :label) do
+        Spam::SpamConstants::ALLOW             |  false |  'ALLOW'
+        Spam::SpamConstants::ALLOW             |  true  |  'ERROR'
+        Spam::SpamConstants::CONDITIONAL_ALLOW |  false |  'CONDITIONAL_ALLOW'
+        Spam::SpamConstants::BLOCK_USER        |  false |  'BLOCK'
+        Spam::SpamConstants::DISALLOW          |  false |  'DISALLOW'
+        Spam::SpamConstants::NOOP              |  false |  'NOOP'
+      end
+
+      with_them do
+        before do
+          allow(Gitlab::Metrics).to receive(:histogram).with(:gitlab_spamcheck_request_duration_seconds, anything).and_return(histogram)
+          allow(service).to receive(:spamcheck_verdict).and_return([verdict, attribs, error])
+        end
+
+        it 'records duration with labels' do
+          expect(histogram).to receive(:observe).with(a_hash_including(result: label), anything)
+          subject
+        end
+      end
     end
   end
 
@@ -150,48 +198,113 @@ RSpec.describe Spam::SpamVerdictService do
     end
   end
 
-  describe '#external_verdict' do
-    subject { service.send(:external_verdict) }
+  describe '#spamcheck_verdict' do
+    subject { service.send(:spamcheck_verdict) }
 
     context 'if a Spam Check endpoint enabled and set to a URL' do
       let(:spam_check_body) { {} }
-      let(:spam_check_http_status) { nil }
+      let(:endpoint_url) { "grpc://www.spamcheckurl.com/spam_check" }
+
+      let(:spam_client) do
+        Gitlab::Spamcheck::Client.new
+      end
 
       before do
         stub_application_setting(spam_check_endpoint_enabled: true)
-        stub_application_setting(spam_check_endpoint_url: "http://www.spamcheckurl.com/spam_check")
-        stub_request(:post, /.*spamcheckurl.com.*/).to_return( body: spam_check_body.to_json, status: spam_check_http_status )
+        stub_application_setting(spam_check_endpoint_url: endpoint_url)
       end
 
       context 'if the endpoint is accessible' do
-        let(:spam_check_http_status) { 200 }
-        let(:error) { nil }
+        let(:error) { '' }
         let(:verdict) { nil }
-        let(:spam_check_body) do
-          { verdict: verdict, error: error }
+
+        let(:attribs) do
+          extra_attributes = { "monitorMode" => "false" }
+          extra_attributes
+        end
+
+        before do
+          allow(service).to receive(:spamcheck_client).and_return(spam_client)
+          allow(spam_client).to receive(:issue_spam?).and_return([verdict, attribs, error])
+        end
+
+        context 'if the result is a NOOP verdict' do
+          let(:verdict) { NOOP }
+
+          it 'returns the verdict' do
+            expect(subject).to eq([NOOP, attribs])
+          end
+        end
+
+        context 'if attribs - monitorMode is true' do
+          let(:attribs) do
+            extra_attributes = { "monitorMode" => "true" }
+            extra_attributes
+          end
+
+          let(:verdict) { ALLOW }
+
+          it 'returns the verdict' do
+            expect(subject).to eq([ALLOW, attribs])
+          end
         end
 
         context 'the result is a valid verdict' do
-          let(:verdict) { 'allow' }
+          let(:verdict) { ALLOW }
 
           it 'returns the verdict' do
-            expect(subject).to eq ALLOW
+            expect(subject).to eq([ALLOW, attribs])
           end
         end
 
-        context 'the verdict is an unexpected string' do
-          let(:verdict) { 'this is fine' }
+        context 'when recaptcha is enabled' do
+          before do
+            allow(Gitlab::Recaptcha).to receive(:enabled?).and_return(true)
+          end
 
-          it 'returns the string' do
-            expect(subject).to eq verdict
+          using RSpec::Parameterized::TableSyntax
+
+          # rubocop: disable Lint/BinaryOperatorWithIdenticalOperands
+          where(:verdict_value, :expected) do
+            ::Spam::SpamConstants::ALLOW               | ::Spam::SpamConstants::ALLOW
+            ::Spam::SpamConstants::CONDITIONAL_ALLOW   | ::Spam::SpamConstants::CONDITIONAL_ALLOW
+            ::Spam::SpamConstants::DISALLOW            | ::Spam::SpamConstants::CONDITIONAL_ALLOW
+            ::Spam::SpamConstants::BLOCK_USER          | ::Spam::SpamConstants::CONDITIONAL_ALLOW
+          end
+          # rubocop: enable Lint/BinaryOperatorWithIdenticalOperands
+
+          with_them do
+            let(:verdict) { verdict_value }
+
+            it "returns expected spam constant" do
+              expect(subject).to eq([expected, attribs])
+            end
           end
         end
 
-        context 'the JSON is malformed' do
-          let(:spam_check_body) { 'this is fine' }
+        context 'when recaptcha is disabled' do
+          before do
+            allow(Gitlab::Recaptcha).to receive(:enabled?).and_return(false)
+          end
+
+          [::Spam::SpamConstants::ALLOW,
+           ::Spam::SpamConstants::CONDITIONAL_ALLOW,
+           ::Spam::SpamConstants::DISALLOW,
+           ::Spam::SpamConstants::BLOCK_USER].each do |verdict_value|
+            let(:verdict) { verdict_value }
+            let(:expected) { [verdict_value, attribs] }
 
-          it 'returns allow' do
-            expect(subject).to eq ALLOW
+            it "returns expected spam constant" do
+              expect(subject).to eq(expected)
+            end
+          end
+        end
+
+        context 'the verdict is an unexpected value' do
+          let(:verdict) { :this_is_fine }
+
+          it 'returns the string' do
+            expect(subject).to eq([verdict, attribs])
           end
         end
 
@@ -199,7 +312,7 @@ RSpec.describe Spam::SpamVerdictService do
           let(:verdict) { '' }
 
           it 'returns nil' do
-            expect(subject).to eq verdict
+            expect(subject).to eq([verdict, attribs])
           end
         end
 
@@ -207,7 +320,7 @@ RSpec.describe Spam::SpamVerdictService do
           let(:verdict) { nil }
 
           it 'returns nil' do
-            expect(subject).to be_nil
+            expect(subject).to eq([nil, attribs])
           end
         end
 
@@ -215,15 +328,19 @@ RSpec.describe Spam::SpamVerdictService do
           let(:error) { "Sorry Dave, I can't do that" }
 
           it 'returns nil' do
-            expect(subject).to be_nil
+            expect(subject).to eq([nil, attribs])
           end
         end
 
-        context 'the HTTP status is not 200' do
-          let(:spam_check_http_status) { 500 }
+        context 'the requested is aborted' do
+          let(:attribs) { nil }
+
+          before do
+            allow(spam_client).to receive(:issue_spam?).and_raise(GRPC::Aborted)
+          end
 
           it 'returns nil' do
-            expect(subject).to be_nil
+            expect(subject).to eq([ALLOW, attribs, true])
           end
         end
 
@@ -232,18 +349,20 @@ RSpec.describe Spam::SpamVerdictService do
           let(:error) { 'oh noes!' }
 
           it 'renders the verdict' do
-            expect(subject).to eq DISALLOW
+            expect(subject).to eq [DISALLOW, attribs]
           end
         end
       end
 
       context 'if the endpoint times out' do
+        let(:attribs) { nil }
+
         before do
-          stub_request(:post, /.*spamcheckurl.com.*/).to_timeout
+          allow(spam_client).to receive(:issue_spam?).and_raise(GRPC::DeadlineExceeded)
         end
 
         it 'returns nil' do
-          expect(subject).to be_nil
+          expect(subject).to eq([ALLOW, attribs, true])
         end
       end
     end