summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorJarka KoÅ¡anovĂ¡ <jarka@gitlab.com>2019-08-13 20:55:04 +0200
committerJarka KoÅ¡anovĂ¡ <jarka@gitlab.com>2019-09-17 13:52:27 +0200
commit78f67c560813dc36fda2b8061cac55fa3cef7a1b (patch)
treec921c223db54b61882c79dfd4421cc056208a2bc
parent5975f55c555330423aab49e7e0d2da2049a39200 (diff)
downloadgitlab-ce-64023-disable-recaptcha.tar.gz
Add feature flag for disabling reCaptcha64023-disable-recaptcha
- only for issues and merge requests - snippets still require solving reCaptcha
-rw-r--r--app/models/concerns/spammable.rb5
-rw-r--r--app/models/snippet.rb7
-rw-r--r--app/services/spam_service.rb3
-rw-r--r--spec/controllers/projects/issues_controller_spec.rb188
-rw-r--r--spec/controllers/projects/snippets_controller_spec.rb6
-rw-r--r--spec/controllers/snippets_controller_spec.rb8
-rw-r--r--spec/features/issues/spam_issues_spec.rb48
-rw-r--r--spec/features/snippets/spam_snippets_spec.rb73
-rw-r--r--spec/requests/api/issues/post_projects_issues_spec.rb60
-rw-r--r--spec/requests/api/issues/put_projects_issues_spec.rb61
-rw-r--r--spec/requests/api/project_snippets_spec.rb6
-rw-r--r--spec/requests/api/snippets_spec.rb7
-rw-r--r--spec/services/create_snippet_service_spec.rb72
-rw-r--r--spec/services/issues/create_service_spec.rb62
-rw-r--r--spec/services/spam_service_spec.rb48
-rw-r--r--spec/support/matchers/log_spam.rb34
16 files changed, 529 insertions, 159 deletions
diff --git a/app/models/concerns/spammable.rb b/app/models/concerns/spammable.rb
index 3ff4b4046d3..10bbeecc2f7 100644
--- a/app/models/concerns/spammable.rb
+++ b/app/models/concerns/spammable.rb
@@ -80,4 +80,9 @@ module Spammable
def check_for_spam?
true
end
+
+ # Override in Spammable if differs
+ def allow_possible_spam?
+ Feature.enabled?(:allow_possible_spam, project)
+ end
end
diff --git a/app/models/snippet.rb b/app/models/snippet.rb
index 00931457344..f85f53cae48 100644
--- a/app/models/snippet.rb
+++ b/app/models/snippet.rb
@@ -14,6 +14,7 @@ class Snippet < ApplicationRecord
include Editable
include Gitlab::SQL::Pattern
include FromUnion
+ extend ::Gitlab::Utils::Override
cache_markdown_field :title, pipeline: :single_line
cache_markdown_field :description
@@ -190,6 +191,12 @@ class Snippet < ApplicationRecord
(public? && (title_changed? || content_changed?))
end
+ # snippers are the biggest sources of spam
+ override :allow_possible_spam?
+ def allow_possible_spam?
+ false
+ end
+
def spammable_entity_type
'snippet'
end
diff --git a/app/services/spam_service.rb b/app/services/spam_service.rb
index f2f133dae28..babe69cfdc8 100644
--- a/app/services/spam_service.rb
+++ b/app/services/spam_service.rb
@@ -37,7 +37,8 @@ class SpamService
else
# Otherwise, it goes to Akismet and check if it's a spam. If that's the
# case, it assigns spammable record as "spam" and create a SpamLog record.
- spammable.spam = check(api)
+ possible_spam = check(api)
+ spammable.spam = possible_spam unless spammable.allow_possible_spam?
spammable.spam_log = spam_log
end
end
diff --git a/spec/controllers/projects/issues_controller_spec.rb b/spec/controllers/projects/issues_controller_spec.rb
index 608131dcbc8..dacf5ab36c2 100644
--- a/spec/controllers/projects/issues_controller_spec.rb
+++ b/spec/controllers/projects/issues_controller_spec.rb
@@ -417,6 +417,7 @@ describe Projects::IssuesController do
context 'when user has access to update issue' do
before do
+ project.update!(visibility_level: Gitlab::VisibilityLevel::PUBLIC)
project.add_developer(user)
end
@@ -430,14 +431,30 @@ describe Projects::IssuesController do
context 'when Akismet is enabled and the issue is identified as spam' do
before do
stub_application_setting(recaptcha_enabled: true)
- allow_any_instance_of(SpamService).to receive(:check_for_spam?).and_return(true)
- allow_any_instance_of(AkismetService).to receive(:spam?).and_return(true)
+ expect_next_instance_of(AkismetService) do |akismet_service|
+ expect(akismet_service).to receive_messages(spam?: true)
+ end
+ end
+
+ context 'when allow_possible_spam feature flag is false' do
+ before do
+ stub_feature_flags(allow_possible_spam: false)
+ end
+
+ it 'renders json with recaptcha_html' do
+ subject
+
+ expect(json_response).to have_key('recaptcha_html')
+ end
end
- it 'renders json with recaptcha_html' do
- subject
+ context 'when allow_possible_spam feature flag is true' do
+ it 'updates the issue' do
+ subject
- expect(json_response).to have_key('recaptcha_html')
+ expect(response).to have_http_status(:ok)
+ expect(issue.reload.title).to eq('New title')
+ end
end
end
end
@@ -690,13 +707,13 @@ describe Projects::IssuesController do
before do
project.update!(visibility_level: Gitlab::VisibilityLevel::PUBLIC)
stub_application_setting(recaptcha_enabled: true)
- allow_any_instance_of(SpamService).to receive(:check_for_spam?).and_return(true)
end
context 'when an issue is not identified as spam' do
before do
- allow_any_instance_of(described_class).to receive(:verify_recaptcha).and_return(false)
- allow_any_instance_of(AkismetService).to receive(:spam?).and_return(false)
+ expect_next_instance_of(AkismetService) do |akismet_service|
+ expect(akismet_service).to receive_messages(spam?: false)
+ end
end
it 'normally updates the issue' do
@@ -705,45 +722,64 @@ describe Projects::IssuesController do
end
context 'when an issue is identified as spam' do
- before do
- allow_any_instance_of(AkismetService).to receive(:spam?).and_return(true)
- end
-
context 'when captcha is not verified' do
before do
- allow_any_instance_of(described_class).to receive(:verify_recaptcha).and_return(false)
+ expect_next_instance_of(AkismetService) do |akismet_service|
+ expect(akismet_service).to receive_messages(spam?: true)
+ end
end
- it 'rejects an issue recognized as a spam' do
- expect { update_issue }.not_to change { issue.reload.title }
- end
+ context 'when allow_possible_spam feature flag is false' do
+ before do
+ stub_feature_flags(allow_possible_spam: false)
+ end
- it 'rejects an issue recognized as a spam when recaptcha disabled' do
- stub_application_setting(recaptcha_enabled: false)
+ it 'rejects an issue recognized as a spam' do
+ expect { update_issue }.not_to change { issue.reload.title }
+ end
- expect { update_issue }.not_to change { issue.reload.title }
- end
+ it 'rejects an issue recognized as a spam when recaptcha disabled' do
+ stub_application_setting(recaptcha_enabled: false)
- it 'creates a spam log' do
- update_issue(issue_params: { title: 'Spam title' })
+ expect { update_issue }.not_to change { issue.reload.title }
+ end
- spam_logs = SpamLog.all
+ it 'creates a spam log' do
+ expect { update_issue(issue_params: { title: 'Spam title' }) }
+ .to log_spam(title: 'Spam title', noteable_type: 'Issue')
+ end
- expect(spam_logs.count).to eq(1)
- expect(spam_logs.first.title).to eq('Spam title')
- expect(spam_logs.first.recaptcha_verified).to be_falsey
- end
+ it 'renders recaptcha_html json response' do
+ update_issue
+
+ expect(json_response).to have_key('recaptcha_html')
+ end
- it 'renders recaptcha_html json response' do
- update_issue
+ it 'returns 200 status' do
+ update_issue
- expect(json_response).to have_key('recaptcha_html')
+ expect(response).to have_gitlab_http_status(200)
+ end
end
- it 'returns 200 status' do
- update_issue
+ context 'when allow_possible_spam feature flag is true' do
+ it 'updates the issue recognized as spam' do
+ expect { update_issue }.to change { issue.reload.title }
+ end
- expect(response).to have_gitlab_http_status(200)
+ it 'creates a spam log' do
+ expect { update_issue(issue_params: { title: 'Spam title' }) }
+ .to log_spam(
+ title: 'Spam title', description: issue.description,
+ noteable_type: 'Issue', recaptcha_verified: false
+ )
+ end
+
+ it 'returns 200 status' do
+ update_issue
+
+ expect(response).to have_gitlab_http_status(200)
+ end
end
end
@@ -757,11 +793,6 @@ describe Projects::IssuesController do
additional_params: { spam_log_id: spam_logs.last.id, recaptcha_verification: true })
end
- before do
- allow_any_instance_of(described_class).to receive(:verify_recaptcha)
- .and_return(true)
- end
-
it 'returns 200 status' do
expect(response).to have_gitlab_http_status(200)
end
@@ -926,55 +957,72 @@ describe Projects::IssuesController do
context 'Akismet is enabled' do
before do
stub_application_setting(recaptcha_enabled: true)
- allow_any_instance_of(SpamService).to receive(:check_for_spam?).and_return(true)
end
context 'when an issue is not identified as spam' do
before do
- allow_any_instance_of(described_class).to receive(:verify_recaptcha).and_return(false)
- allow_any_instance_of(AkismetService).to receive(:spam?).and_return(false)
+ stub_feature_flags(allow_possible_spam: false)
+
+ expect_next_instance_of(AkismetService) do |akismet_service|
+ expect(akismet_service).to receive_messages(spam?: false)
+ end
end
- it 'does not create an issue' do
- expect { post_new_issue(title: '') }.not_to change(Issue, :count)
+ it 'creates an issue' do
+ expect { post_new_issue(title: 'Some title') }.to change(Issue, :count)
end
end
context 'when an issue is identified as spam' do
- before do
- allow_any_instance_of(AkismetService).to receive(:spam?).and_return(true)
- end
-
context 'when captcha is not verified' do
def post_spam_issue
post_new_issue(title: 'Spam Title', description: 'Spam lives here')
end
before do
- allow_any_instance_of(described_class).to receive(:verify_recaptcha).and_return(false)
+ expect_next_instance_of(AkismetService) do |akismet_service|
+ expect(akismet_service).to receive_messages(spam?: true)
+ end
end
- it 'rejects an issue recognized as a spam' do
- expect { post_spam_issue }.not_to change(Issue, :count)
- end
+ context 'when allow_possible_spam feature flag is false' do
+ before do
+ stub_feature_flags(allow_possible_spam: false)
+ end
- it 'creates a spam log' do
- post_spam_issue
- spam_logs = SpamLog.all
+ it 'rejects an issue recognized as a spam' do
+ expect { post_spam_issue }.not_to change(Issue, :count)
+ end
- expect(spam_logs.count).to eq(1)
- expect(spam_logs.first.title).to eq('Spam Title')
- expect(spam_logs.first.recaptcha_verified).to be_falsey
- end
+ it 'creates a spam log' do
+ expect { post_spam_issue }
+ .to log_spam(title: 'Spam Title', noteable_type: 'Issue', recaptcha_verified: false)
+ end
- it 'does not create an issue when it is not valid' do
- expect { post_new_issue(title: '') }.not_to change(Issue, :count)
+ it 'does not create an issue when it is not valid' do
+ expect { post_new_issue(title: '') }.not_to change(Issue, :count)
+ end
+
+ it 'does not create an issue when recaptcha is not enabled' do
+ stub_application_setting(recaptcha_enabled: false)
+
+ expect { post_spam_issue }.not_to change(Issue, :count)
+ end
end
- it 'does not create an issue when recaptcha is not enabled' do
- stub_application_setting(recaptcha_enabled: false)
+ context 'when allow_possible_spam feature flag is true' do
+ it 'creates an issue recognized as spam' do
+ expect { post_spam_issue }.to change(Issue, :count)
+ end
- expect { post_spam_issue }.not_to change(Issue, :count)
+ it 'creates a spam log' do
+ expect { post_spam_issue }
+ .to log_spam(title: 'Spam Title', noteable_type: 'Issue', recaptcha_verified: false)
+ end
+
+ it 'does not create an issue when it is not valid' do
+ expect { post_new_issue(title: '') }.not_to change(Issue, :count)
+ end
end
end
@@ -986,7 +1034,7 @@ describe Projects::IssuesController do
end
before do
- allow_any_instance_of(described_class).to receive(:verify_recaptcha).and_return(true)
+ expect(controller).to receive_messages(verify_recaptcha: true)
end
it 'accepts an issue after recaptcha is verified' do
@@ -1039,8 +1087,12 @@ describe Projects::IssuesController do
describe 'POST #mark_as_spam' do
context 'properly submits to Akismet' do
before do
- allow_any_instance_of(AkismetService).to receive_messages(submit_spam: true)
- allow_any_instance_of(ApplicationSetting).to receive_messages(akismet_enabled: true)
+ expect_next_instance_of(AkismetService) do |akismet_service|
+ expect(akismet_service).to receive_messages(submit_spam: true)
+ end
+ expect_next_instance_of(ApplicationSetting) do |setting|
+ expect(setting).to receive_messages(akismet_enabled: true)
+ end
end
def post_spam
@@ -1275,7 +1327,9 @@ describe Projects::IssuesController do
end
it "shows error when upload fails" do
- allow_any_instance_of(UploadService).to receive(:execute).and_return(nil)
+ expect_next_instance_of(UploadService) do |upload_service|
+ expect(upload_service).to receive(:execute).and_return(nil)
+ end
import_csv
diff --git a/spec/controllers/projects/snippets_controller_spec.rb b/spec/controllers/projects/snippets_controller_spec.rb
index 9b5d7317c11..cef003c2f78 100644
--- a/spec/controllers/projects/snippets_controller_spec.rb
+++ b/spec/controllers/projects/snippets_controller_spec.rb
@@ -125,7 +125,7 @@ describe Projects::SnippetsController do
it 'creates a spam log' do
expect { create_snippet(project, visibility_level: Snippet::PUBLIC) }
- .to change { SpamLog.count }.by(1)
+ .to log_spam(title: 'Title', user_id: user.id, noteable_type: 'ProjectSnippet')
end
it 'renders :new with recaptcha disabled' do
@@ -206,7 +206,7 @@ describe Projects::SnippetsController do
it 'creates a spam log' do
expect { update_snippet(title: 'Foo') }
- .to change { SpamLog.count }.by(1)
+ .to log_spam(title: 'Foo', user_id: user.id, noteable_type: 'ProjectSnippet')
end
it 'renders :edit with recaptcha disabled' do
@@ -251,7 +251,7 @@ describe Projects::SnippetsController do
it 'creates a spam log' do
expect { update_snippet(title: 'Foo', visibility_level: Snippet::PUBLIC) }
- .to change { SpamLog.count }.by(1)
+ .to log_spam(title: 'Foo', user_id: user.id, noteable_type: 'ProjectSnippet')
end
it 'renders :edit with recaptcha disabled' do
diff --git a/spec/controllers/snippets_controller_spec.rb b/spec/controllers/snippets_controller_spec.rb
index b0092bc8994..f89c790ae1e 100644
--- a/spec/controllers/snippets_controller_spec.rb
+++ b/spec/controllers/snippets_controller_spec.rb
@@ -260,7 +260,7 @@ describe SnippetsController do
it 'creates a spam log' do
expect { create_snippet(visibility_level: Snippet::PUBLIC) }
- .to change { SpamLog.count }.by(1)
+ .to log_spam(title: 'Title', user: user, noteable_type: 'PersonalSnippet')
end
it 'renders :new with recaptcha disabled' do
@@ -336,7 +336,7 @@ describe SnippetsController do
it 'creates a spam log' do
expect { update_snippet(title: 'Foo', visibility_level: Snippet::PUBLIC) }
- .to change { SpamLog.count }.by(1)
+ .to log_spam(title: 'Foo', user: user, noteable_type: 'PersonalSnippet')
end
it 'renders :edit with recaptcha disabled' do
@@ -380,8 +380,8 @@ describe SnippetsController do
end
it 'creates a spam log' do
- expect { update_snippet(title: 'Foo') }
- .to change { SpamLog.count }.by(1)
+ expect {update_snippet(title: 'Foo') }
+ .to log_spam(title: 'Foo', user: user, noteable_type: 'PersonalSnippet')
end
it 'renders :edit with recaptcha disabled' do
diff --git a/spec/features/issues/spam_issues_spec.rb b/spec/features/issues/spam_issues_spec.rb
index 0d009f47fff..18245f249fd 100644
--- a/spec/features/issues/spam_issues_spec.rb
+++ b/spec/features/issues/spam_issues_spec.rb
@@ -30,21 +30,47 @@ describe 'New issue', :js do
visit new_project_issue_path(project)
end
- it 'creates an issue after solving reCaptcha' do
- fill_in 'issue_title', with: 'issue title'
- fill_in 'issue_description', with: 'issue description'
+ context 'when allow_possible_spam feature flag is false' do
+ before do
+ stub_feature_flags(allow_possible_spam: false)
+ end
- click_button 'Submit issue'
+ it 'creates an issue after solving reCaptcha' do
+ fill_in 'issue_title', with: 'issue title'
+ fill_in 'issue_description', with: 'issue description'
- # it is impossible to test recaptcha automatically and there is no possibility to fill in recaptcha
- # recaptcha verification is skipped in test environment and it always returns true
- expect(page).not_to have_content('issue title')
- expect(page).to have_css('.recaptcha')
+ click_button 'Submit issue'
- click_button 'Submit issue'
+ # it is impossible to test recaptcha automatically and there is no possibility to fill in recaptcha
+ # recaptcha verification is skipped in test environment and it always returns true
+ expect(page).not_to have_content('issue title')
+ expect(page).to have_css('.recaptcha')
- expect(page.find('.issue-details h2.title')).to have_content('issue title')
- expect(page.find('.issue-details .description')).to have_content('issue description')
+ click_button 'Submit issue'
+
+ expect(page.find('.issue-details h2.title')).to have_content('issue title')
+ expect(page.find('.issue-details .description')).to have_content('issue description')
+ end
+ end
+
+ context 'when allow_possible_spam feature flag is true' do
+ before do
+ fill_in 'issue_title', with: 'issue title'
+ fill_in 'issue_description', with: 'issue description'
+ end
+
+ it 'creates an issue without a need to solve reCaptcha' do
+ click_button 'Submit issue'
+
+ expect(page).not_to have_css('.recaptcha')
+ expect(page.find('.issue-details h2.title')).to have_content('issue title')
+ expect(page.find('.issue-details .description')).to have_content('issue description')
+ end
+
+ it 'creates a spam log record' do
+ expect { click_button 'Submit issue' }
+ .to log_spam(title: 'issue title', description: 'issue description', user_id: user.id, noteable_type: 'Issue')
+ end
end
end
diff --git a/spec/features/snippets/spam_snippets_spec.rb b/spec/features/snippets/spam_snippets_spec.rb
new file mode 100644
index 00000000000..3e71a4e7879
--- /dev/null
+++ b/spec/features/snippets/spam_snippets_spec.rb
@@ -0,0 +1,73 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe 'User creates snippet', :js do
+ let(:user) { create(:user) }
+
+ before do
+ stub_feature_flags(allow_possible_spam: false)
+ stub_env('IN_MEMORY_APPLICATION_SETTINGS', 'false')
+
+ Gitlab::CurrentSettings.update!(
+ akismet_enabled: true,
+ akismet_api_key: 'testkey',
+ recaptcha_enabled: true,
+ recaptcha_site_key: 'test site key',
+ recaptcha_private_key: 'test private key'
+ )
+
+ sign_in(user)
+ visit new_snippet_path
+
+ fill_in 'personal_snippet_title', with: 'My Snippet Title'
+ fill_in 'personal_snippet_description', with: 'My Snippet **Description**'
+ find('#personal_snippet_visibility_level_20').set(true)
+ page.within('.file-editor') do
+ find('.ace_text-input', visible: false).send_keys 'Hello World!'
+ end
+ end
+
+ shared_examples 'solve recaptcha' do
+ it 'creates a snippet after solving reCaptcha' do
+ click_button('Create snippet')
+ wait_for_requests
+
+ # it is impossible to test recaptcha automatically and there is no possibility to fill in recaptcha
+ # recaptcha verification is skipped in test environment and it always returns true
+ expect(page).not_to have_content('My Snippet Title')
+ expect(page).to have_css('.recaptcha')
+ click_button('Submit personal snippet')
+
+ expect(page).to have_content('My Snippet Title')
+ end
+ end
+
+ context 'when identified as a spam' do
+ before do
+ WebMock.stub_request(:any, /.*akismet.com.*/).to_return(body: "true", status: 200)
+ end
+
+ context 'when allow_possible_spam feature flag is false' do
+ it_behaves_like 'solve recaptcha'
+ end
+
+ context 'when allow_possible_spam feature flag is true' do
+ it_behaves_like 'solve recaptcha'
+ end
+ end
+
+ context 'when not identified as a spam' do
+ before do
+ WebMock.stub_request(:any, /.*akismet.com.*/).to_return(body: "false", status: 200)
+ end
+
+ it 'creates a snippet' do
+ click_button('Create snippet')
+ wait_for_requests
+
+ expect(page).not_to have_css('.recaptcha')
+ expect(page).to have_content('My Snippet Title')
+ end
+ end
+end
diff --git a/spec/requests/api/issues/post_projects_issues_spec.rb b/spec/requests/api/issues/post_projects_issues_spec.rb
index b74e8867310..3a55b437ead 100644
--- a/spec/requests/api/issues/post_projects_issues_spec.rb
+++ b/spec/requests/api/issues/post_projects_issues_spec.rb
@@ -374,9 +374,17 @@ describe API::Issues do
end
describe 'POST /projects/:id/issues with spam filtering' do
+ def post_issue
+ post api("/projects/#{project.id}/issues", user), params: params
+ end
+
before do
- allow_any_instance_of(SpamService).to receive(:check_for_spam?).and_return(true)
- allow_any_instance_of(AkismetService).to receive_messages(spam?: true)
+ expect_next_instance_of(SpamService) do |spam_service|
+ expect(spam_service).to receive_messages(check_for_spam?: true)
+ end
+ expect_next_instance_of(AkismetService) do |akismet_service|
+ expect(akismet_service).to receive_messages(spam?: true)
+ end
end
let(:params) do
@@ -387,17 +395,43 @@ describe API::Issues do
}
end
- it 'does not create a new project issue' do
- expect { post api("/projects/#{project.id}/issues", user), params: params }.not_to change(Issue, :count)
- expect(response).to have_gitlab_http_status(400)
- expect(json_response['message']).to eq({ 'error' => 'Spam detected' })
-
- spam_logs = SpamLog.all
- expect(spam_logs.count).to eq(1)
- expect(spam_logs[0].title).to eq('new issue')
- expect(spam_logs[0].description).to eq('content here')
- expect(spam_logs[0].user).to eq(user)
- expect(spam_logs[0].noteable_type).to eq('Issue')
+ context 'when allow_possible_spam feature flag is false' do
+ before do
+ stub_feature_flags(allow_possible_spam: false)
+ end
+
+ it 'does not create a new project issue' do
+ expect { post_issue }.not_to change(Issue, :count)
+ end
+
+ it 'returns correct status and message' do
+ post_issue
+
+ expect(response).to have_gitlab_http_status(400)
+ expect(json_response['message']).to eq({ 'error' => 'Spam detected' })
+ end
+
+ it 'creates a new spam log entry' do
+ expect { post_issue }
+ .to log_spam(title: 'new issue', description: 'content here', user_id: user.id, noteable_type: 'Issue')
+ end
+ end
+
+ context 'when allow_possible_spam feature flag is true' do
+ it 'does creates a new project issue' do
+ expect { post_issue }.to change(Issue, :count).by(1)
+ end
+
+ it 'returns correct status' do
+ post_issue
+
+ expect(response).to have_gitlab_http_status(201)
+ end
+
+ it 'creates a new spam log entry' do
+ expect { post_issue }
+ .to log_spam(title: 'new issue', description: 'content here', user_id: user.id, noteable_type: 'Issue')
+ end
end
end
diff --git a/spec/requests/api/issues/put_projects_issues_spec.rb b/spec/requests/api/issues/put_projects_issues_spec.rb
index 267cba93713..43f302ed194 100644
--- a/spec/requests/api/issues/put_projects_issues_spec.rb
+++ b/spec/requests/api/issues/put_projects_issues_spec.rb
@@ -181,6 +181,10 @@ describe API::Issues do
end
describe 'PUT /projects/:id/issues/:issue_iid with spam filtering' do
+ def update_issue
+ put api("/projects/#{project.id}/issues/#{issue.iid}", user), params: params
+ end
+
let(:params) do
{
title: 'updated title',
@@ -189,21 +193,52 @@ describe API::Issues do
}
end
- it 'does not create a new project issue' do
- allow_any_instance_of(SpamService).to receive_messages(check_for_spam?: true)
- allow_any_instance_of(AkismetService).to receive_messages(spam?: true)
+ before do
+ expect_next_instance_of(SpamService) do |spam_service|
+ expect(spam_service).to receive_messages(check_for_spam?: true)
+ end
+ expect_next_instance_of(AkismetService) do |akismet_service|
+ expect(akismet_service).to receive_messages(spam?: true)
+ end
+ end
- put api("/projects/#{project.id}/issues/#{issue.iid}", user), params: params
+ context 'when allow_possible_spam feature flag is false' do
+ before do
+ stub_feature_flags(allow_possible_spam: false)
+ end
- expect(response).to have_gitlab_http_status(400)
- expect(json_response['message']).to eq({ 'error' => 'Spam detected' })
-
- spam_logs = SpamLog.all
- expect(spam_logs.count).to eq(1)
- expect(spam_logs[0].title).to eq('updated title')
- expect(spam_logs[0].description).to eq('content here')
- expect(spam_logs[0].user).to eq(user)
- expect(spam_logs[0].noteable_type).to eq('Issue')
+ it 'does not update a project issue' do
+ expect { update_issue }.not_to change { issue.reload.title }
+ end
+
+ it 'returns correct status and message' do
+ update_issue
+
+ expect(response).to have_gitlab_http_status(400)
+ expect(json_response).to include('message' => { 'error' => 'Spam detected' })
+ end
+
+ it 'creates a new spam log entry' do
+ expect { update_issue }
+ .to log_spam(title: 'updated title', description: 'content here', user_id: user.id, noteable_type: 'Issue')
+ end
+ end
+
+ context 'when allow_possible_spam feature flag is true' do
+ it 'updates a project issue' do
+ expect { update_issue }.to change { issue.reload.title }
+ end
+
+ it 'returns correct status and message' do
+ update_issue
+
+ expect(response).to have_gitlab_http_status(200)
+ end
+
+ it 'creates a new spam log entry' do
+ expect { update_issue }
+ .to log_spam(title: 'updated title', description: 'content here', user_id: user.id, noteable_type: 'Issue')
+ end
end
end
diff --git a/spec/requests/api/project_snippets_spec.rb b/spec/requests/api/project_snippets_spec.rb
index 2e6e13aa927..ef0cabad4b0 100644
--- a/spec/requests/api/project_snippets_spec.rb
+++ b/spec/requests/api/project_snippets_spec.rb
@@ -198,7 +198,7 @@ describe API::ProjectSnippets do
it 'creates a spam log' do
expect { create_snippet(project, visibility: 'public') }
- .to change { SpamLog.count }.by(1)
+ .to log_spam(title: 'Test Title', user_id: user.id, noteable_type: 'ProjectSnippet')
end
end
end
@@ -289,7 +289,7 @@ describe API::ProjectSnippets do
it 'creates a spam log' do
expect { update_snippet(title: 'Foo') }
- .to change { SpamLog.count }.by(1)
+ .to log_spam(title: 'Foo', user_id: admin.id, noteable_type: 'ProjectSnippet')
end
end
@@ -306,7 +306,7 @@ describe API::ProjectSnippets do
it 'creates a spam log' do
expect { update_snippet(title: 'Foo', visibility: 'public') }
- .to change { SpamLog.count }.by(1)
+ .to log_spam(title: 'Foo', user_id: admin.id, noteable_type: 'ProjectSnippet')
end
end
end
diff --git a/spec/requests/api/snippets_spec.rb b/spec/requests/api/snippets_spec.rb
index 515912cb305..e7eaaea2418 100644
--- a/spec/requests/api/snippets_spec.rb
+++ b/spec/requests/api/snippets_spec.rb
@@ -254,7 +254,7 @@ describe API::Snippets do
it 'creates a spam log' do
expect { create_snippet(visibility: 'public') }
- .to change { SpamLog.count }.by(1)
+ .to log_spam(title: 'Test Title', user_id: user.id, noteable_type: 'PersonalSnippet')
end
end
end
@@ -344,8 +344,7 @@ describe API::Snippets do
end
it 'creates a spam log' do
- expect { update_snippet(title: 'Foo') }
- .to change { SpamLog.count }.by(1)
+ expect { update_snippet(title: 'Foo') }.to log_spam(title: 'Foo', user_id: user.id, noteable_type: 'PersonalSnippet')
end
end
@@ -359,7 +358,7 @@ describe API::Snippets do
it 'creates a spam log' do
expect { update_snippet(title: 'Foo', visibility: 'public') }
- .to change { SpamLog.count }.by(1)
+ .to log_spam(title: 'Foo', user_id: user.id, noteable_type: 'PersonalSnippet')
end
end
end
diff --git a/spec/services/create_snippet_service_spec.rb b/spec/services/create_snippet_service_spec.rb
index 7d2491b3a49..1751029a78c 100644
--- a/spec/services/create_snippet_service_spec.rb
+++ b/spec/services/create_snippet_service_spec.rb
@@ -3,26 +3,28 @@
require 'spec_helper'
describe CreateSnippetService do
- before do
- @user = create :user
- @admin = create :user, admin: true
- @opts = {
+ let(:user) { create(:user) }
+ let(:admin) { create(:user, :admin) }
+ let(:opts) { base_opts.merge(extra_opts) }
+ let(:base_opts) do
+ {
title: 'Test snippet',
file_name: 'snippet.rb',
content: 'puts "hello world"',
visibility_level: Gitlab::VisibilityLevel::PRIVATE
}
end
+ let(:extra_opts) { {} }
context 'When public visibility is restricted' do
+ let(:extra_opts) { { visibility_level: Gitlab::VisibilityLevel::PUBLIC } }
+
before do
stub_application_setting(restricted_visibility_levels: [Gitlab::VisibilityLevel::PUBLIC])
-
- @opts.merge!(visibility_level: Gitlab::VisibilityLevel::PUBLIC)
end
it 'non-admins are not able to create a public snippet' do
- snippet = create_snippet(nil, @user, @opts)
+ snippet = create_snippet(nil, user, opts)
expect(snippet.errors.messages).to have_key(:visibility_level)
expect(snippet.errors.messages[:visibility_level].first).to(
match('has been restricted')
@@ -30,37 +32,81 @@ describe CreateSnippetService do
end
it 'admins are able to create a public snippet' do
- snippet = create_snippet(nil, @admin, @opts)
+ snippet = create_snippet(nil, admin, opts)
expect(snippet.errors.any?).to be_falsey
expect(snippet.visibility_level).to eq(Gitlab::VisibilityLevel::PUBLIC)
end
describe "when visibility level is passed as a string" do
+ let(:extra_opts) { { visibility: 'internal' } }
+
before do
- @opts[:visibility] = 'internal'
- @opts.delete(:visibility_level)
+ base_opts.delete(:visibility_level)
end
it "assigns the correct visibility level" do
- snippet = create_snippet(nil, @user, @opts)
+ snippet = create_snippet(nil, user, opts)
expect(snippet.errors.any?).to be_falsey
expect(snippet.visibility_level).to eq(Gitlab::VisibilityLevel::INTERNAL)
end
end
end
+ context 'checking spam' do
+ shared_examples 'marked as spam' do
+ let(:snippet) { create_snippet(nil, admin, opts) }
+
+ it 'marks a snippet as a spam ' do
+ expect(snippet).to be_spam
+ end
+
+ it 'invalidates the snippet' do
+ expect(snippet).to be_invalid
+ end
+
+ it 'creates a new spam_log' do
+ expect { snippet }
+ .to log_spam(title: snippet.title, noteable_type: 'PersonalSnippet')
+ end
+
+ it 'assigns a spam_log to an issue' do
+ expect(snippet.spam_log).to eq(SpamLog.last)
+ end
+ end
+
+ let(:extra_opts) do
+ { visibility_level: Gitlab::VisibilityLevel::PUBLIC, request: double(:request, env: {}) }
+ end
+
+ before do
+ expect_next_instance_of(AkismetService) do |akismet_service|
+ expect(akismet_service).to receive_messages(spam?: true)
+ end
+ end
+
+ [true, false, nil].each do |allow_possible_spam|
+ context "when recaptcha_disabled flag is #{allow_possible_spam.inspect}" do
+ before do
+ stub_feature_flags(allow_possible_spam: allow_possible_spam) unless allow_possible_spam.nil?
+ end
+
+ it_behaves_like 'marked as spam'
+ end
+ end
+ end
+
describe 'usage counter' do
let(:counter) { Gitlab::UsageDataCounters::SnippetCounter }
it 'increments count' do
expect do
- create_snippet(nil, @admin, @opts)
+ create_snippet(nil, admin, opts)
end.to change { counter.read(:create) }.by 1
end
it 'does not increment count if create fails' do
expect do
- create_snippet(nil, @admin, {})
+ create_snippet(nil, admin, {})
end.not_to change { counter.read(:create) }
end
end
diff --git a/spec/services/issues/create_service_spec.rb b/spec/services/issues/create_service_spec.rb
index b7bedc2f97e..5dc6b6176ee 100644
--- a/spec/services/issues/create_service_spec.rb
+++ b/spec/services/issues/create_service_spec.rb
@@ -344,7 +344,7 @@ describe Issues::CreateService do
end
before do
- allow_any_instance_of(SpamService).to receive(:check_for_spam?).and_return(true)
+ stub_feature_flags(allow_possible_spam: false)
end
context 'when recaptcha was verified' do
@@ -384,31 +384,67 @@ describe Issues::CreateService do
end
context 'when recaptcha was not verified' do
+ before do
+ expect_next_instance_of(SpamService) do |spam_service|
+ expect(spam_service).to receive_messages(check_for_spam?: true)
+ end
+ end
+
context 'when akismet detects spam' do
before do
- allow_any_instance_of(AkismetService).to receive(:spam?).and_return(true)
+ expect_next_instance_of(AkismetService) do |akismet_service|
+ expect(akismet_service).to receive_messages(spam?: true)
+ end
end
- it 'marks an issue as a spam ' do
- expect(issue).to be_spam
- end
+ context 'when issuables_recaptcha_enabled feature flag is true' do
+ it 'marks an issue as a spam ' do
+ expect(issue).to be_spam
+ end
- it 'an issue is not valid ' do
- expect(issue.valid?).to be_falsey
- end
+ it 'invalidates the issue' do
+ expect(issue).to be_invalid
+ end
+
+ it 'creates a new spam_log' do
+ expect { issue }
+ .to log_spam(title: issue.title, description: issue.description, user_id: user.id, noteable_type: 'Issue')
+ end
- it 'creates a new spam_log' do
- expect {issue}.to change {SpamLog.count}.from(0).to(1)
+ it 'assigns a spam_log to an issue' do
+ expect(issue.spam_log).to eq(SpamLog.last)
+ end
end
- it 'assigns a spam_log to an issue' do
- expect(issue.spam_log).to eq(SpamLog.last)
+ context 'when issuable_recaptcha_enabled feature flag is false' do
+ before do
+ stub_feature_flags(allow_possible_spam: true)
+ end
+
+ it 'does not mark an issue as a spam ' do
+ expect(issue).not_to be_spam
+ end
+
+ it 'accepts the ​issue as valid' do
+ expect(issue).to be_valid
+ end
+
+ it 'creates a new spam_log' do
+ expect { issue }
+ .to log_spam(title: issue.title, description: issue.description, user_id: user.id, noteable_type: 'Issue')
+ end
+
+ it 'assigns a spam_log to an issue' do
+ expect(issue.spam_log).to eq(SpamLog.last)
+ end
end
end
context 'when akismet does not detect spam' do
before do
- allow_any_instance_of(AkismetService).to receive(:spam?).and_return(false)
+ expect_next_instance_of(AkismetService) do |akismet_service|
+ expect(akismet_service).to receive_messages(spam?: false)
+ end
end
it 'does not mark an issue as a spam ' do
diff --git a/spec/services/spam_service_spec.rb b/spec/services/spam_service_spec.rb
index b9e5e844c1f..76f77583612 100644
--- a/spec/services/spam_service_spec.rb
+++ b/spec/services/spam_service_spec.rb
@@ -44,30 +44,50 @@ describe SpamService do
end
context 'when indicated as spam by akismet' do
+ shared_examples 'akismet spam' do
+ it 'doesnt check as spam when request is missing' do
+ check_spam(issue, nil, false)
+
+ expect(issue).not_to be_spam
+ end
+
+ it 'creates a spam log' do
+ expect { check_spam(issue, request, false) }
+ .to log_spam(title: issue.title, description: issue.description, noteable_type: 'Issue')
+ end
+
+ it 'does not yield to the block' do
+ expect(check_spam(issue, request, false))
+ .to eql(SpamLog.last)
+ end
+ end
+
before do
allow(AkismetService).to receive(:new).and_return(double(spam?: true))
end
- it 'doesnt check as spam when request is missing' do
- check_spam(issue, nil, false)
+ context 'when allow_possible_spam feature flag is false' do
+ before do
+ stub_feature_flags(allow_possible_spam: false)
+ end
- expect(issue.spam).to be_falsey
- end
+ it_behaves_like 'akismet spam'
- it 'checks as spam' do
- check_spam(issue, request, false)
+ it 'checks as spam' do
+ check_spam(issue, request, false)
- expect(issue.spam).to be_truthy
+ expect(issue.spam).to be_truthy
+ end
end
- it 'creates a spam log' do
- expect { check_spam(issue, request, false) }
- .to change { SpamLog.count }.from(0).to(1)
- end
+ context 'when allow_possible_spam feature flag is true' do
+ it_behaves_like 'akismet spam'
+
+ it 'does not check as spam' do
+ check_spam(issue, request, false)
- it 'doesnt yield block' do
- expect(check_spam(issue, request, false))
- .to eql(SpamLog.last)
+ expect(issue.spam).to be_nil
+ end
end
end
diff --git a/spec/support/matchers/log_spam.rb b/spec/support/matchers/log_spam.rb
new file mode 100644
index 00000000000..541cacf558c
--- /dev/null
+++ b/spec/support/matchers/log_spam.rb
@@ -0,0 +1,34 @@
+# frozen_string_literal: true
+
+# This matcher checkes if one spam log with provided attributes was created
+#
+# Example:
+#
+# expect { create_issue }.to log_spam
+RSpec::Matchers.define :log_spam do |expected|
+ def spam_logs
+ SpamLog.all
+ end
+
+ match do |block|
+ block.call
+
+ expect(spam_logs).to contain_exactly(
+ have_attributes(expected)
+ )
+ end
+
+ description do
+ count = spam_logs.count
+
+ if count == 1
+ keys = expected.keys.map(&:to_s)
+ actual = spam_logs.first.attributes.slice(*keys)
+ "create a spam log with #{expected} attributes. #{actual} created instead."
+ else
+ "create exactly 1 spam log with #{expected} attributes. #{count} spam logs created instead."
+ end
+ end
+
+ supports_block_expectations
+end