Merge branch 'jwanjohi-spamcheck-monitor-mode' into 'master'

Get extra attributes from Spamcheck response and determine verdict [RUN ALL RSPEC][RUN AS-IF-FOSS]

See merge request gitlab-org/gitlab!60915

app/services/spam/spam_verdict_service.rb

@@ -15,11 +15,17 @@ module Spam
 
     def execute
       spamcheck_result = nil
+      spamcheck_attribs = {}
 
       external_spam_check_round_trip_time = Benchmark.realtime do
-        spamcheck_result = spamcheck_verdict
+        spamcheck_result, spamcheck_attribs = spamcheck_verdict
       end
 
+      # assign result to a var and log it before reassigning to nil when monitorMode is true
+      original_spamcheck_result = spamcheck_result
+
+      spamcheck_result = nil if spamcheck_attribs&.fetch("monitorMode", "false") == "true"
+
       akismet_result = akismet_verdict
 
       # filter out anything we don't recognise, including nils.
@@ -33,7 +39,8 @@ module Spam
 
       logger.info(class: self.class.name,
                   akismet_verdict: akismet_verdict,
-                  spam_check_verdict: spamcheck_result,
+                  spam_check_verdict: original_spamcheck_result,
+                  extra_attributes: spamcheck_attribs,
                   spam_check_rtt: external_spam_check_round_trip_time.real,
                   final_verdict: final_verdict,
                   username: user.username,
@@ -61,21 +68,23 @@ module Spam
       return unless Gitlab::CurrentSettings.spam_check_endpoint_enabled
 
       begin
-        result, _error = spamcheck_client.issue_spam?(spam_issue: target, user: user, context: context)
-        return unless result
+        result, attribs, _error = spamcheck_client.issue_spam?(spam_issue: target, user: user, context: context)
+        return [nil, attribs] unless result
 
         # @TODO log if error is not nil https://gitlab.com/gitlab-org/gitlab/-/issues/329545
 
+        return [result, attribs] if result == NOOP || attribs["monitorMode"] == "true"
+
         # Duplicate logic with Akismet logic in #akismet_verdict
         if Gitlab::Recaptcha.enabled? && result != ALLOW
-          CONDITIONAL_ALLOW
+          [CONDITIONAL_ALLOW, attribs]
         else
-          result
+          [result, attribs]
         end
       rescue StandardError => e
         Gitlab::ErrorTracking.log_exception(e)
         # Default to ALLOW if any errors occur
-        ALLOW
+        [ALLOW, attribs]
       end
     end
 

lib/gitlab/spamcheck/client.rb

@@ -45,7 +45,7 @@ module Gitlab
                                               metadata: { 'authorization' =>
                                                            Gitlab::CurrentSettings.spam_check_api_key })
         verdict = convert_verdict_to_gitlab_constant(response.verdict)
-        [verdict, response.error]
+        [verdict, response.extra_attributes.to_h, response.error]
       end
 
       private

spec/lib/gitlab/spamcheck/client_spec.rb

@@ -9,12 +9,20 @@ RSpec.describe Gitlab::Spamcheck::Client do
   let_it_be(:user) { create(:user, organization: 'GitLab') }
   let(:verdict_value) { nil }
   let(:error_value) { "" }
+
+  let(:attribs_value) do
+    extra_attributes = Google::Protobuf::Map.new(:string, :string)
+    extra_attributes["monitorMode"] = "false"
+    extra_attributes
+  end
+
   let_it_be(:issue) { create(:issue, description: 'Test issue description') }
 
   let(:response) do
     verdict = ::Spamcheck::SpamVerdict.new
     verdict.verdict = verdict_value
     verdict.error = error_value
+    verdict.extra_attributes = attribs_value
     verdict
   end
 
@@ -45,7 +53,7 @@ RSpec.describe Gitlab::Spamcheck::Client do
       let(:verdict_value) { verdict }
 
       it "returns expected spam constant" do
-        expect(subject).to eq([expected, ""])
+        expect(subject).to eq([expected, { "monitorMode" => "false" }, ""])
       end
     end
   end

spec/services/spam/spam_verdict_service_spec.rb

@@ -23,12 +23,17 @@ RSpec.describe Spam::SpamVerdictService 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(:spamcheck_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(:spamcheck_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(:spamcheck_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,29 @@ 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(:spamcheck_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
   end
 
@@ -170,16 +191,42 @@ RSpec.describe Spam::SpamVerdictService do
         let(:error) { '' }
         let(:verdict) { nil }
 
+        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, error])
+          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 }
 
           it 'returns the verdict' do
-            expect(subject).to eq ALLOW
+            expect(subject).to eq([ALLOW, attribs])
           end
         end
 
@@ -203,7 +250,7 @@ RSpec.describe Spam::SpamVerdictService do
             let(:verdict) { verdict_value }
 
             it "returns expected spam constant" do
-              expect(subject).to eq(expected)
+              expect(subject).to eq([expected, attribs])
             end
           end
         end
@@ -218,7 +265,7 @@ RSpec.describe Spam::SpamVerdictService do
            ::Spam::SpamConstants::DISALLOW,
            ::Spam::SpamConstants::BLOCK_USER].each do |verdict_value|
             let(:verdict) { verdict_value }
-            let(:expected) { verdict_value }
+            let(:expected) { [verdict_value, attribs] }
 
             it "returns expected spam constant" do
               expect(subject).to eq(expected)
@@ -230,7 +277,7 @@ RSpec.describe Spam::SpamVerdictService do
           let(:verdict) { :this_is_fine }
 
           it 'returns the string' do
-            expect(subject).to eq verdict
+            expect(subject).to eq([verdict, attribs])
           end
         end
 
@@ -238,7 +285,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
 
@@ -246,7 +293,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
 
@@ -254,17 +301,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 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(ALLOW)
+            expect(subject).to eq([ALLOW, attribs])
           end
         end
 
@@ -273,18 +322,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
           allow(spam_client).to receive(:issue_spam?).and_raise(GRPC::DeadlineExceeded)
         end
 
         it 'returns nil' do
-          expect(subject).to be(ALLOW)
+          expect(subject).to eq([ALLOW, attribs])
         end
       end
     end