Add latest changes from gitlab-org/gitlab@13-1-stable-ee

3 files changed, 241 insertions, 29 deletions

diff --git a/spec/services/spam/akismet_service_spec.rb b/spec/services/spam/akismet_service_spec.rb
index a496cd1890e..413b43d0156 100644
--- a/spec/services/spam/akismet_service_spec.rb
+++ b/spec/services/spam/akismet_service_spec.rb
@@ -45,9 +45,7 @@ describe Spam::AkismetService do
       end
 
       it 'logs an error' do
-        logger_spy = double(:logger)
-        expect(Rails).to receive(:logger).and_return(logger_spy)
-        expect(logger_spy).to receive(:error).with(/skipping/)
+        expect(Gitlab::AppLogger).to receive(:error).with(/skipping/)
 
         subject.send(method_call)
       end
@@ -98,9 +96,7 @@ describe Spam::AkismetService do
         end
 
         it 'logs an error' do
-          logger_spy = double(:logger)
-          expect(Rails).to receive(:logger).and_return(logger_spy)
-          expect(logger_spy).to receive(:error).with(/skipping check/)
+          expect(Gitlab::AppLogger).to receive(:error).with(/skipping check/)
 
           subject.spam?
         end
diff --git a/spec/services/spam/spam_action_service_spec.rb b/spec/services/spam/spam_action_service_spec.rb
index 560833aba97..7b6b65c82b1 100644
--- a/spec/services/spam/spam_action_service_spec.rb
+++ b/spec/services/spam/spam_action_service_spec.rb
@@ -24,7 +24,7 @@ describe Spam::SpamActionService do
   end
 
   describe '#initialize' do
-    subject { described_class.new(spammable: issue, request: request) }
+    subject { described_class.new(spammable: issue, request: request, user: user) }
 
     context 'when the request is nil' do
       let(:request) { nil }
@@ -53,7 +53,7 @@ describe Spam::SpamActionService do
 
   shared_examples 'only checks for spam if a request is provided' do
     context 'when request is missing' do
-      subject { described_class.new(spammable: issue, request: nil) }
+      subject { described_class.new(spammable: issue, request: nil, user: user) }
 
       it "doesn't check as spam" do
         subject
@@ -78,9 +78,9 @@ describe Spam::SpamActionService do
     let_it_be(:existing_spam_log) { create(:spam_log, user: user, recaptcha_verified: false) }
 
     subject do
-      described_service = described_class.new(spammable: issue, request: request)
+      described_service = described_class.new(spammable: issue, request: request, user: user)
       allow(described_service).to receive(:allowlisted?).and_return(allowlisted)
-      described_service.execute(user: user, api: nil, recaptcha_verified: recaptcha_verified, spam_log_id: existing_spam_log.id)
+      described_service.execute(api: nil, recaptcha_verified: recaptcha_verified, spam_log_id: existing_spam_log.id)
     end
 
     before do
@@ -163,9 +163,9 @@ describe Spam::SpamActionService do
           end
         end
 
-        context 'when spam verdict service requires reCAPTCHA' do
+        context 'when spam verdict service conditionally allows' do
           before do
-            allow(fake_verdict_service).to receive(:execute).and_return(REQUIRE_RECAPTCHA)
+            allow(fake_verdict_service).to receive(:execute).and_return(CONDITIONAL_ALLOW)
           end
 
           context 'when allow_possible_spam feature flag is false' do
diff --git a/spec/services/spam/spam_verdict_service_spec.rb b/spec/services/spam/spam_verdict_service_spec.rb
index 93460a5e7d7..f6d9cd96da5 100644
--- a/spec/services/spam/spam_verdict_service_spec.rb
+++ b/spec/services/spam/spam_verdict_service_spec.rb
@@ -16,50 +16,266 @@ describe Spam::SpamVerdictService do
   let(:request) { double(:request, env: env) }
 
   let(:check_for_spam) { true }
-  let(:issue) { build(:issue) }
+  let_it_be(:user) { create(:user) }
+  let(:issue) { build(:issue, author: user) }
   let(:service) do
-    described_class.new(target: issue, request: request, options: {})
+    described_class.new(user: user, target: issue, request: request, options: {})
   end
 
   describe '#execute' do
     subject { service.execute }
 
     before do
-      allow_next_instance_of(Spam::AkismetService) do |service|
-        allow(service).to receive(:spam?).and_return(spam_verdict)
+      allow(service).to receive(:akismet_verdict).and_return(nil)
+      allow(service).to receive(:spam_verdict_verdict).and_return(nil)
+    end
+
+    context 'if all services return nil' do
+      it 'renders ALLOW verdict' do
+        expect(subject).to eq ALLOW
       end
     end
 
-    context 'if Akismet considers it spam' do
-      let(:spam_verdict) { true }
+    context 'if only one service returns a verdict' do
+      context 'and it is supported' do
+        before do
+          allow(service).to receive(:akismet_verdict).and_return(DISALLOW)
+        end
 
-      context 'if reCAPTCHA is enabled' do
+        it 'renders that verdict' do
+          expect(subject).to eq DISALLOW
+        end
+      end
+
+      context 'and it is unexpected' do
         before do
-          stub_application_setting(recaptcha_enabled: true)
+          allow(service).to receive(:akismet_verdict).and_return("unexpected")
         end
 
-        it 'requires reCAPTCHA' do
-          expect(subject).to eq REQUIRE_RECAPTCHA
+        it 'allows' do
+          expect(subject).to eq ALLOW
         end
       end
+    end
 
-      context 'if reCAPTCHA is not enabled' do
+    context 'if more than one service returns a verdict' do
+      context 'and they are supported' do
         before do
-          stub_application_setting(recaptcha_enabled: false)
+          allow(service).to receive(:akismet_verdict).and_return(DISALLOW)
+          allow(service).to receive(:spam_verdict).and_return(BLOCK_USER)
         end
 
-        it 'disallows the change' do
-          expect(subject).to eq DISALLOW
+        it 'renders the more restrictive verdict' do
+          expect(subject).to eq BLOCK_USER
+        end
+      end
+
+      context 'and one is supported' do
+        before do
+          allow(service).to receive(:akismet_verdict).and_return('nonsense')
+          allow(service).to receive(:spam_verdict).and_return(BLOCK_USER)
+        end
+
+        it 'renders the more restrictive verdict' do
+          expect(subject).to eq BLOCK_USER
+        end
+      end
+
+      context 'and one is supported' do
+        before do
+          allow(service).to receive(:akismet_verdict).and_return('nonsense')
+          allow(service).to receive(:spam_verdict).and_return(BLOCK_USER)
+        end
+
+        it 'renders the more restrictive verdict' do
+          expect(subject).to eq BLOCK_USER
+        end
+      end
+
+      context 'and none are supported' do
+        before do
+          allow(service).to receive(:akismet_verdict).and_return('nonsense')
+          allow(service).to receive(:spam_verdict).and_return('rubbish')
+        end
+
+        it 'renders the more restrictive verdict' do
+          expect(subject).to eq ALLOW
+        end
+      end
+    end
+  end
+
+  describe '#akismet_verdict' do
+    subject { service.send(:akismet_verdict) }
+
+    context 'if Akismet is enabled' do
+      before do
+        stub_application_setting(akismet_enabled: true)
+        allow_next_instance_of(Spam::AkismetService) do |service|
+          allow(service).to receive(:spam?).and_return(akismet_result)
+        end
+      end
+
+      context 'if Akismet considers it spam' do
+        let(:akismet_result) { true }
+
+        context 'if reCAPTCHA is enabled' do
+          before do
+            stub_application_setting(recaptcha_enabled: true)
+          end
+
+          it 'returns conditionally allow verdict' do
+            expect(subject).to eq CONDITIONAL_ALLOW
+          end
+        end
+
+        context 'if reCAPTCHA is not enabled' do
+          before do
+            stub_application_setting(recaptcha_enabled: false)
+          end
+
+          it 'renders disallow verdict' do
+            expect(subject).to eq DISALLOW
+          end
+        end
+      end
+
+      context 'if Akismet does not consider it spam' do
+        let(:akismet_result) { false }
+
+        it 'renders allow verdict' do
+          expect(subject).to eq ALLOW
         end
       end
     end
 
-    context 'if Akismet does not consider it spam' do
-      let(:spam_verdict) { false }
+    context 'if Akismet is not enabled' do
+      before do
+        stub_application_setting(akismet_enabled: false)
+      end
 
-      it 'allows the change' do
+      it 'renders allow verdict' do
         expect(subject).to eq ALLOW
       end
     end
   end
+
+  describe '#spam_verdict' do
+    subject { service.send(:spam_verdict) }
+
+    context 'if a Spam Check endpoint enabled and set to a URL' do
+      let(:spam_check_body) { {} }
+      let(:spam_check_http_status) { nil }
+
+      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 )
+      end
+
+      context 'if the endpoint is accessible' do
+        let(:spam_check_http_status) { 200 }
+        let(:error) { nil }
+        let(:verdict) { nil }
+        let(:spam_check_body) do
+          { verdict: verdict, error: error }
+        end
+
+        context 'the result is a valid verdict' do
+          let(:verdict) { 'allow' }
+
+          it 'returns the verdict' do
+            expect(subject).to eq ALLOW
+          end
+        end
+
+        context 'the verdict is an unexpected string' do
+          let(:verdict) { 'this is fine' }
+
+          it 'returns nil' do
+            expect(subject).to be_nil
+          end
+        end
+
+        context 'the JSON is malformed' do
+          let(:spam_check_body) { 'this is fine' }
+
+          it 'returns allow' do
+            expect(subject).to eq ALLOW
+          end
+        end
+
+        context 'the verdict is an empty string' do
+          let(:verdict) { '' }
+
+          it 'returns nil' do
+            expect(subject).to be_nil
+          end
+        end
+
+        context 'the verdict is nil' do
+          let(:verdict) { nil }
+
+          it 'returns nil' do
+            expect(subject).to be_nil
+          end
+        end
+
+        context 'there is an error' do
+          let(:error) { "Sorry Dave, I can't do that" }
+
+          it 'returns nil' do
+            expect(subject).to be_nil
+          end
+        end
+
+        context 'the HTTP status is not 200' do
+          let(:spam_check_http_status) { 500 }
+
+          it 'returns nil' do
+            expect(subject).to be_nil
+          end
+        end
+
+        context 'the confused API endpoint returns both an error and a verdict' do
+          let(:verdict) { 'disallow' }
+          let(:error) { 'oh noes!' }
+
+          it 'renders the verdict' do
+            expect(subject).to eq DISALLOW
+          end
+        end
+      end
+
+      context 'if the endpoint times out' do
+        before do
+          stub_request(:post, /.*spamcheckurl.com.*/).to_timeout
+        end
+
+        it 'returns nil' do
+          expect(subject).to be_nil
+        end
+      end
+    end
+
+    context 'if a Spam Check endpoint is not set' do
+      before do
+        stub_application_setting(spam_check_endpoint_url: nil)
+      end
+
+      it 'returns nil' do
+        expect(subject).to be_nil
+      end
+    end
+
+    context 'if Spam Check endpoint is not enabled' do
+      before do
+        stub_application_setting(spam_check_endpoint_enabled: false)
+      end
+
+      it 'returns nil' do
+        expect(subject).to be_nil
+      end
+    end
+  end
 end