diff options
author | GitLab Bot <gitlab-bot@gitlab.com> | 2021-05-19 18:44:42 +0300 |
---|---|---|
committer | GitLab Bot <gitlab-bot@gitlab.com> | 2021-05-19 18:44:42 +0300 |
commit | 4555e1b21c365ed8303ffb7a3325d773c9b8bf31 (patch) | |
tree | 5423a1c7516cffe36384133ade12572cf709398d /spec/services/spam | |
parent | e570267f2f6b326480d284e0164a6464ba4081bc (diff) |
Add latest changes from gitlab-org/gitlab@13-12-stable-eev13.12.0-rc42
Diffstat (limited to 'spec/services/spam')
-rw-r--r-- | spec/services/spam/spam_action_service_spec.rb | 54 | ||||
-rw-r--r-- | spec/services/spam/spam_verdict_service_spec.rb | 189 |
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 |