diff options
-rw-r--r-- | app/controllers/projects/issues_controller.rb | 9 | ||||
-rw-r--r-- | app/models/concerns/spammable.rb | 2 | ||||
-rw-r--r-- | app/models/issue.rb | 3 | ||||
-rw-r--r-- | app/models/snippet.rb | 3 | ||||
-rw-r--r-- | app/services/spam_check_service.rb | 3 | ||||
-rw-r--r-- | changelogs/unreleased/29483-spam-check-only-title-and-description.yml | 4 | ||||
-rw-r--r-- | spec/controllers/projects/issues_controller_spec.rb | 23 | ||||
-rw-r--r-- | spec/models/issue_spec.rb | 37 | ||||
-rw-r--r-- | spec/models/snippet_spec.rb | 43 | ||||
-rw-r--r-- | spec/services/spam_service_spec.rb | 71 |
10 files changed, 167 insertions, 31 deletions
diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb index cdb5b4173d3..0d6d9f492c1 100644 --- a/app/controllers/projects/issues_controller.rb +++ b/app/controllers/projects/issues_controller.rb @@ -148,7 +148,14 @@ class Projects::IssuesController < Projects::ApplicationController end format.json do - render json: @issue.to_json(include: { milestone: {}, assignee: { only: [:name, :username], methods: [:avatar_url] }, labels: { methods: :text_color } }, methods: [:task_status, :task_status_short]) + if @issue.valid? + render json: @issue.to_json(methods: [:task_status, :task_status_short], + include: { milestone: {}, + assignee: { only: [:name, :username], methods: [:avatar_url] }, + labels: { methods: :text_color } }) + else + render json: { errors: @issue.errors.full_messages }, status: :unprocessable_entity + end end end diff --git a/app/models/concerns/spammable.rb b/app/models/concerns/spammable.rb index 107e6764ba2..647a6cad3d7 100644 --- a/app/models/concerns/spammable.rb +++ b/app/models/concerns/spammable.rb @@ -41,7 +41,7 @@ module Spammable def check_for_spam error_msg = if Gitlab::Recaptcha.enabled? "Your #{spammable_entity_type} has been recognized as spam. "\ - "You can still submit it by solving Captcha." + "Please, change the content or solve the reCAPTCHA to proceed." else "Your #{spammable_entity_type} has been recognized as spam and has been discarded." end diff --git a/app/models/issue.rb b/app/models/issue.rb index 602eed86d9e..10a5d9d2a24 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -211,9 +211,8 @@ class Issue < ActiveRecord::Base due_date.try(:past?) || false end - # Only issues on public projects should be checked for spam def check_for_spam? - project.public? + project.public? && (title_changed? || description_changed?) end def as_json(options = {}) diff --git a/app/models/snippet.rb b/app/models/snippet.rb index dbd564e5e7d..30aca62499c 100644 --- a/app/models/snippet.rb +++ b/app/models/snippet.rb @@ -132,7 +132,8 @@ class Snippet < ActiveRecord::Base end def check_for_spam? - public? + visibility_level_changed?(to: Snippet::PUBLIC) || + (public? && (title_changed? || content_changed?)) end def spammable_entity_type diff --git a/app/services/spam_check_service.rb b/app/services/spam_check_service.rb index 023e0824e85..11030bee8f1 100644 --- a/app/services/spam_check_service.rb +++ b/app/services/spam_check_service.rb @@ -14,6 +14,9 @@ module SpamCheckService @spam_log_id = params.delete(:spam_log_id) end + # In order to be proceed to the spam check process, @spammable has to be + # a dirty instance, which means it should be already assigned with the new + # attribute values. def spam_check(spammable, user) spam_service = SpamService.new(spammable, @request) diff --git a/changelogs/unreleased/29483-spam-check-only-title-and-description.yml b/changelogs/unreleased/29483-spam-check-only-title-and-description.yml new file mode 100644 index 00000000000..de8cacb250d --- /dev/null +++ b/changelogs/unreleased/29483-spam-check-only-title-and-description.yml @@ -0,0 +1,4 @@ +--- +title: Spam check only when spammable attributes have changed +merge_request: +author: diff --git a/spec/controllers/projects/issues_controller_spec.rb b/spec/controllers/projects/issues_controller_spec.rb index 57a921e3676..c467ab9fb8a 100644 --- a/spec/controllers/projects/issues_controller_spec.rb +++ b/spec/controllers/projects/issues_controller_spec.rb @@ -241,10 +241,27 @@ describe Projects::IssuesController do expect(spam_logs.first.recaptcha_verified).to be_falsey end - it 'renders verify template' do - update_spam_issue + context 'as HTML' do + it 'renders verify template' do + update_spam_issue + + expect(response).to render_template(:verify) + end + end + + context 'as JSON' do + before do + update_issue({ title: 'Spam Title', description: 'Spam lives here' }, format: :json) + end + + it 'renders json errors' do + expect(json_response) + .to eql("errors" => ["Your issue has been recognized as spam. Please, change the content or solve the reCAPTCHA to proceed."]) + end - expect(response).to render_template(:verify) + it 'returns 422 status' do + expect(response).to have_http_status(422) + end end end diff --git a/spec/models/issue_spec.rb b/spec/models/issue_spec.rb index 73977d031f9..b8584301baa 100644 --- a/spec/models/issue_spec.rb +++ b/spec/models/issue_spec.rb @@ -670,4 +670,41 @@ describe Issue, models: true do expect(attrs_hash).to include('time_estimate') end end + + describe '#check_for_spam' do + let(:project) { create :project, visibility_level: visibility_level } + let(:issue) { create :issue, project: project } + + subject do + issue.assign_attributes(description: description) + issue.check_for_spam? + end + + context 'when project is public and spammable attributes changed' do + let(:visibility_level) { Gitlab::VisibilityLevel::PUBLIC } + let(:description) { 'woo' } + + it 'returns true' do + is_expected.to be_truthy + end + end + + context 'when project is private' do + let(:visibility_level) { Gitlab::VisibilityLevel::PRIVATE } + let(:description) { issue.description } + + it 'returns false' do + is_expected.to be_falsey + end + end + + context 'when spammable attributes have not changed' do + let(:visibility_level) { Gitlab::VisibilityLevel::PUBLIC } + let(:description) { issue.description } + + it 'returns false' do + is_expected.to be_falsey + end + end + end end diff --git a/spec/models/snippet_spec.rb b/spec/models/snippet_spec.rb index 219ab1989ea..8095d01b69e 100644 --- a/spec/models/snippet_spec.rb +++ b/spec/models/snippet_spec.rb @@ -198,4 +198,47 @@ describe Snippet, models: true do expect(snippet.participants).to include(note1.author, note2.author) end end + + describe '#check_for_spam' do + let(:snippet) { create :snippet, visibility_level: visibility_level } + + subject do + snippet.assign_attributes(title: title) + snippet.check_for_spam? + end + + context 'when public and spammable attributes changed' do + let(:visibility_level) { Snippet::PUBLIC } + let(:title) { 'woo' } + + it 'returns true' do + is_expected.to be_truthy + end + end + + context 'when private' do + let(:visibility_level) { Snippet::PRIVATE } + let(:title) { snippet.title } + + it 'returns false' do + is_expected.to be_falsey + end + + it 'returns true when switching to public' do + snippet.save! + snippet.visibility_level = Snippet::PUBLIC + + expect(snippet.check_for_spam?).to be_truthy + end + end + + context 'when spammable attributes have not changed' do + let(:visibility_level) { Snippet::PUBLIC } + let(:title) { snippet.title } + + it 'returns false' do + is_expected.to be_falsey + end + end + end end diff --git a/spec/services/spam_service_spec.rb b/spec/services/spam_service_spec.rb index 4ce3b95aa87..e09c05ccf32 100644 --- a/spec/services/spam_service_spec.rb +++ b/spec/services/spam_service_spec.rb @@ -19,42 +19,67 @@ describe SpamService, services: true do let(:issue) { create(:issue, project: project) } let(:request) { double(:request, env: {}) } - context 'when indicated as spam by akismet' do - before { allow(AkismetService).to receive(:new).and_return(double(is_spam?: true)) } + context 'when spammable attributes have not changed' do + before do + issue.closed_at = Time.zone.now - it 'doesnt check as spam when request is missing' do - check_spam(issue, nil, false) - - expect(issue.spam).to be_falsey + allow(AkismetService).to receive(:new).and_return(double(is_spam?: true)) end - it 'checks as spam' do - check_spam(issue, request, false) - - expect(issue.spam).to be_truthy + it 'returns false' do + expect(check_spam(issue, request, false)).to be_falsey end - it 'creates a spam log' do + it 'does not create a spam log' do expect { check_spam(issue, request, false) } - .to change { SpamLog.count }.from(0).to(1) + .not_to change { SpamLog.count } end + end - it 'doesnt yield block' do - expect(check_spam(issue, request, false)) - .to eql(SpamLog.last) + context 'when spammable attributes have changed' do + before do + issue.description = 'SPAM!' end - end - context 'when not indicated as spam by akismet' do - before { allow(AkismetService).to receive(:new).and_return(double(is_spam?: false)) } + context 'when indicated as spam by akismet' do + before do + allow(AkismetService).to receive(:new).and_return(double(is_spam?: true)) + end - it 'returns false' do - expect(check_spam(issue, request, false)).to be_falsey + it 'doesnt check as spam when request is missing' do + check_spam(issue, nil, false) + + expect(issue.spam).to be_falsey + end + + it 'checks as spam' do + check_spam(issue, request, false) + + expect(issue.spam).to be_truthy + end + + it 'creates a spam log' do + expect { check_spam(issue, request, false) } + .to change { SpamLog.count }.from(0).to(1) + end + + it 'doesnt yield block' do + expect(check_spam(issue, request, false)) + .to eql(SpamLog.last) + end end - it 'does not create a spam log' do - expect { check_spam(issue, request, false) } - .not_to change { SpamLog.count } + context 'when not indicated as spam by akismet' do + before { allow(AkismetService).to receive(:new).and_return(double(is_spam?: false)) } + + it 'returns false' do + expect(check_spam(issue, request, false)).to be_falsey + end + + it 'does not create a spam log' do + expect { check_spam(issue, request, false) } + .not_to change { SpamLog.count } + end end end end |