summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
Diffstat
-rw-r--r--app/controllers/projects/issues_controller.rb9
-rw-r--r--app/models/concerns/spammable.rb2
-rw-r--r--app/models/issue.rb3
-rw-r--r--app/models/snippet.rb3
-rw-r--r--app/services/spam_check_service.rb3
-rw-r--r--changelogs/unreleased/29483-spam-check-only-title-and-description.yml4
-rw-r--r--spec/controllers/projects/issues_controller_spec.rb23
-rw-r--r--spec/models/issue_spec.rb37
-rw-r--r--spec/models/snippet_spec.rb43
-rw-r--r--spec/services/spam_service_spec.rb71
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
View Lorry Controller Status