diff options
Diffstat (limited to 'spec/services/spam')
-rw-r--r-- | spec/services/spam/akismet_service_spec.rb | 8 | ||||
-rw-r--r-- | spec/services/spam/spam_action_service_spec.rb | 12 | ||||
-rw-r--r-- | spec/services/spam/spam_verdict_service_spec.rb | 250 |
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 |