summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorSean McGivern <sean@mcgivern.me.uk>2017-02-22 10:13:20 +0000
committerRuben Davila <rdavila84@gmail.com>2017-02-27 23:35:54 -0500
commitad6473de66928ff4de3a0b33ba73652048d6ce2a (patch)
treece0536718e4fbfd946ecda5ae240f4d7493eb508
parent26e77c382ddd4afa2d27d9d3488f0c9e802f74ed (diff)
downloadgitlab-ce-ad6473de66928ff4de3a0b33ba73652048d6ce2a.tar.gz
Merge branch '28093-snippet-and-issue-spam-check-on-edit' into 'master'
Spam check and reCAPTCHA improvements Closes #28093 See merge request !9248 Conflicts: app/services/issues/create_service.rb
-rw-r--r--app/controllers/concerns/spammable_actions.rb30
-rw-r--r--app/controllers/projects/issues_controller.rb34
-rw-r--r--app/controllers/projects/snippets_controller.rb21
-rw-r--r--app/controllers/snippets_controller.rb13
-rw-r--r--app/models/concerns/spammable.rb2
-rw-r--r--app/models/project_snippet.rb4
-rw-r--r--app/services/create_snippet_service.rb10
-rw-r--r--app/services/issuable_base_service.rb32
-rw-r--r--app/services/issues/create_service.rb21
-rw-r--r--app/services/issues/update_service.rb8
-rw-r--r--app/services/spam_check_service.rb24
-rw-r--r--app/services/spam_service.rb31
-rw-r--r--app/services/update_snippet_service.rb10
-rw-r--r--app/views/layouts/_recaptcha_verification.html.haml23
-rw-r--r--app/views/projects/issues/verify.html.haml22
-rw-r--r--app/views/projects/snippets/verify.html.haml4
-rw-r--r--app/views/snippets/verify.html.haml4
-rw-r--r--changelogs/unreleased/28093-snippet-and-issue-spam-check-on-edit.yml4
-rw-r--r--lib/api/helpers.rb4
-rw-r--r--lib/api/issues.rb6
-rw-r--r--lib/api/project_snippets.rb8
-rw-r--r--lib/api/snippets.rb7
-rw-r--r--lib/api/v3/issues.rb10
-rw-r--r--lib/api/v3/project_snippets.rb8
-rw-r--r--spec/controllers/projects/issues_controller_spec.rb111
-rw-r--r--spec/controllers/projects/snippets_controller_spec.rb186
-rw-r--r--spec/controllers/snippets_controller_spec.rb159
-rw-r--r--spec/models/concerns/spammable_spec.rb1
-rw-r--r--spec/requests/api/issues_spec.rb27
-rw-r--r--spec/requests/api/project_snippets_spec.rb92
-rw-r--r--spec/requests/api/snippets_spec.rb66
-rw-r--r--spec/requests/api/v3/issues_spec.rb27
-rw-r--r--spec/requests/api/v3/project_snippets_spec.rb92
-rw-r--r--spec/services/issues/create_service_spec.rb10
-rw-r--r--spec/services/spam_service_spec.rb71
35 files changed, 935 insertions, 247 deletions
diff --git a/app/controllers/concerns/spammable_actions.rb b/app/controllers/concerns/spammable_actions.rb
index a6891149bfa..da225d8f1c7 100644
--- a/app/controllers/concerns/spammable_actions.rb
+++ b/app/controllers/concerns/spammable_actions.rb
@@ -17,13 +17,31 @@ module SpammableActions
private
- def recaptcha_params
- return {} unless params[:recaptcha_verification] && Gitlab::Recaptcha.load_configurations! && verify_recaptcha
+ def recaptcha_check_with_fallback(&fallback)
+ if spammable.valid?
+ redirect_to spammable
+ elsif render_recaptcha?
+ if params[:recaptcha_verification]
+ flash[:alert] = 'There was an error with the reCAPTCHA. Please solve the reCAPTCHA again.'
+ end
+
+ render :verify
+ else
+ fallback.call
+ end
+ end
+
+ def spammable_params
+ default_params = { request: request }
+
+ recaptcha_check = params[:recaptcha_verification] &&
+ Gitlab::Recaptcha.load_configurations! &&
+ verify_recaptcha
+
+ return default_params unless recaptcha_check
- {
- recaptcha_verified: true,
- spam_log_id: params[:spam_log_id]
- }
+ { recaptcha_verified: true,
+ spam_log_id: params[:spam_log_id] }.merge(default_params)
end
def spammable
diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb
index c75b8987a4b..3794585ce35 100644
--- a/app/controllers/projects/issues_controller.rb
+++ b/app/controllers/projects/issues_controller.rb
@@ -91,15 +91,15 @@ class Projects::IssuesController < Projects::ApplicationController
end
def create
- extra_params = { request: request,
- merge_request_for_resolving_discussions: merge_request_for_resolving_discussions }
- extra_params.merge!(recaptcha_params)
+ create_params = issue_params
+ .merge(merge_request_for_resolving_discussions: merge_request_for_resolving_discussions)
+ .merge(spammable_params)
- @issue = Issues::CreateService.new(project, current_user, issue_params.merge(extra_params)).execute
+ @issue = Issues::CreateService.new(project, current_user, create_params).execute
respond_to do |format|
format.html do
- html_response_create
+ recaptcha_check_with_fallback { render :new }
end
format.js do
@link = @issue.attachment.url.to_js
@@ -108,7 +108,9 @@ class Projects::IssuesController < Projects::ApplicationController
end
def update
- @issue = Issues::UpdateService.new(project, current_user, issue_params).execute(issue)
+ update_params = issue_params.merge(spammable_params)
+
+ @issue = Issues::UpdateService.new(project, current_user, update_params).execute(issue)
if params[:move_to_project_id].to_i > 0
new_project = Project.find(params[:move_to_project_id])
@@ -120,11 +122,7 @@ class Projects::IssuesController < Projects::ApplicationController
respond_to do |format|
format.html do
- if @issue.valid?
- redirect_to issue_path(@issue)
- else
- render :edit
- end
+ recaptcha_check_with_fallback { render :edit }
end
format.json do
@@ -176,20 +174,6 @@ class Projects::IssuesController < Projects::ApplicationController
protected
- def html_response_create
- if @issue.valid?
- redirect_to issue_path(@issue)
- elsif render_recaptcha?
- if params[:recaptcha_verification]
- flash[:alert] = 'There was an error with the reCAPTCHA. Please solve the reCAPTCHA again.'
- end
-
- render :verify
- else
- render :new
- end
- end
-
def issue
# The Sortable default scope causes performance issues when used with find_by
@noteable = @issue ||= @project.issues.where(iid: params[:id]).reorder(nil).take || redirect_old
diff --git a/app/controllers/projects/snippets_controller.rb b/app/controllers/projects/snippets_controller.rb
index 5d193f26a8e..181c6bbb1eb 100644
--- a/app/controllers/projects/snippets_controller.rb
+++ b/app/controllers/projects/snippets_controller.rb
@@ -37,27 +37,22 @@ class Projects::SnippetsController < Projects::ApplicationController
end
def create
- create_params = snippet_params.merge(request: request)
+ create_params = snippet_params.merge(spammable_params)
+
@snippet = CreateSnippetService.new(@project, current_user, create_params).execute
- if @snippet.valid?
- respond_with(@snippet,
- location: namespace_project_snippet_path(@project.namespace,
- @project, @snippet))
- else
- render :new
- end
+ recaptcha_check_with_fallback { render :new }
end
def edit
end
def update
- UpdateSnippetService.new(project, current_user, @snippet,
- snippet_params).execute
- respond_with(@snippet,
- location: namespace_project_snippet_path(@project.namespace,
- @project, @snippet))
+ update_params = snippet_params.merge(spammable_params)
+
+ UpdateSnippetService.new(project, current_user, @snippet, update_params).execute
+
+ recaptcha_check_with_fallback { render :edit }
end
def show
diff --git a/app/controllers/snippets_controller.rb b/app/controllers/snippets_controller.rb
index b169d993688..02ae2b9b383 100644
--- a/app/controllers/snippets_controller.rb
+++ b/app/controllers/snippets_controller.rb
@@ -41,19 +41,22 @@ class SnippetsController < ApplicationController
end
def create
- create_params = snippet_params.merge(request: request)
+ create_params = snippet_params.merge(spammable_params)
+
@snippet = CreateSnippetService.new(nil, current_user, create_params).execute
- respond_with @snippet.becomes(Snippet)
+ recaptcha_check_with_fallback { render :new }
end
def edit
end
def update
- UpdateSnippetService.new(nil, current_user, @snippet,
- snippet_params).execute
- respond_with @snippet.becomes(Snippet)
+ update_params = snippet_params.merge(spammable_params)
+
+ UpdateSnippetService.new(nil, current_user, @snippet, update_params).execute
+
+ recaptcha_check_with_fallback { render :edit }
end
def show
diff --git a/app/models/concerns/spammable.rb b/app/models/concerns/spammable.rb
index 79adc77c9e4..107e6764ba2 100644
--- a/app/models/concerns/spammable.rb
+++ b/app/models/concerns/spammable.rb
@@ -13,7 +13,7 @@ module Spammable
attr_accessor :spam
attr_accessor :spam_log
- after_validation :check_for_spam, on: :create
+ after_validation :check_for_spam, on: [:create, :update]
cattr_accessor :spammable_attrs, instance_accessor: false do
[]
diff --git a/app/models/project_snippet.rb b/app/models/project_snippet.rb
index 9bb456eee24..25b5d777641 100644
--- a/app/models/project_snippet.rb
+++ b/app/models/project_snippet.rb
@@ -9,8 +9,4 @@ class ProjectSnippet < Snippet
participant :author
participant :notes_with_associations
-
- def check_for_spam?
- super && project.public?
- end
end
diff --git a/app/services/create_snippet_service.rb b/app/services/create_snippet_service.rb
index 14f5ba064ff..40286dbf3bf 100644
--- a/app/services/create_snippet_service.rb
+++ b/app/services/create_snippet_service.rb
@@ -1,7 +1,8 @@
class CreateSnippetService < BaseService
+ include SpamCheckService
+
def execute
- request = params.delete(:request)
- api = params.delete(:api)
+ filter_spam_check_params
snippet = if project
project.snippets.build(params)
@@ -15,10 +16,11 @@ class CreateSnippetService < BaseService
end
snippet.author = current_user
- snippet.spam = SpamService.new(snippet, request).check(api)
+
+ spam_check(snippet, current_user)
if snippet.save
- UserAgentDetailService.new(snippet, request).create
+ UserAgentDetailService.new(snippet, @request).create
end
snippet
diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb
index 5f3ced49665..9500faf2862 100644
--- a/app/services/issuable_base_service.rb
+++ b/app/services/issuable_base_service.rb
@@ -191,14 +191,12 @@ class IssuableBaseService < BaseService
# To be overridden by subclasses
end
- def after_update(issuable)
+ def before_update(issuable)
# To be overridden by subclasses
end
- def update_issuable(issuable, attributes)
- issuable.with_transaction_returning_status do
- issuable.update(attributes.merge(updated_by: current_user))
- end
+ def after_update(issuable)
+ # To be overridden by subclasses
end
def update(issuable)
@@ -212,16 +210,22 @@ class IssuableBaseService < BaseService
label_ids = process_label_ids(params, existing_label_ids: issuable.label_ids)
params[:label_ids] = label_ids if labels_changing?(issuable.label_ids, label_ids)
- if params.present? && update_issuable(issuable, params)
- # We do not touch as it will affect a update on updated_at field
- ActiveRecord::Base.no_touching do
- handle_common_system_notes(issuable, old_labels: old_labels)
- end
+ if params.present?
+ issuable.assign_attributes(params.merge(updated_by: current_user))
+
+ before_update(issuable)
- handle_changes(issuable, old_labels: old_labels, old_mentioned_users: old_mentioned_users)
- after_update(issuable)
- issuable.create_new_cross_references!(current_user)
- execute_hooks(issuable, 'update')
+ if issuable.with_transaction_returning_status { issuable.save }
+ # We do not touch as it will affect a update on updated_at field
+ ActiveRecord::Base.no_touching do
+ handle_common_system_notes(issuable, old_labels: old_labels)
+ end
+
+ handle_changes(issuable, old_labels: old_labels, old_mentioned_users: old_mentioned_users)
+ after_update(issuable)
+ issuable.create_new_cross_references!(current_user)
+ execute_hooks(issuable, 'update')
+ end
end
issuable
diff --git a/app/services/issues/create_service.rb b/app/services/issues/create_service.rb
index c9168f74249..366b3572738 100644
--- a/app/services/issues/create_service.rb
+++ b/app/services/issues/create_service.rb
@@ -1,10 +1,9 @@
module Issues
class CreateService < Issues::BaseService
+ include SpamCheckService
+
def execute
- @request = params.delete(:request)
- @api = params.delete(:api)
- @recaptcha_verified = params.delete(:recaptcha_verified)
- @spam_log_id = params.delete(:spam_log_id)
+ filter_spam_check_params
issue_attributes = params.merge(merge_request_for_resolving_discussions: merge_request_for_resolving_discussions)
@issue = BuildService.new(project, current_user, issue_attributes).execute
@@ -12,14 +11,8 @@ module Issues
create(@issue)
end
- def before_create(issuable)
- if @recaptcha_verified
- spam_log = current_user.spam_logs.find_by(id: @spam_log_id, title: issuable.title)
- spam_log.update!(recaptcha_verified: true) if spam_log
- else
- issuable.spam = spam_service.check(@api)
- issuable.spam_log = spam_service.spam_log
- end
+ def before_create(issue)
+ spam_check(issue, current_user)
end
def after_create(issuable)
@@ -42,10 +35,6 @@ module Issues
private
- def spam_service
- @spam_service ||= SpamService.new(@issue, @request)
- end
-
def user_agent_detail_service
UserAgentDetailService.new(@issue, @request)
end
diff --git a/app/services/issues/update_service.rb b/app/services/issues/update_service.rb
index 78cbf94ec69..22e32b13259 100644
--- a/app/services/issues/update_service.rb
+++ b/app/services/issues/update_service.rb
@@ -1,9 +1,17 @@
module Issues
class UpdateService < Issues::BaseService
+ include SpamCheckService
+
def execute(issue)
+ filter_spam_check_params
+
update(issue)
end
+ def before_update(issue)
+ spam_check(issue, current_user)
+ end
+
def handle_changes(issue, old_labels: [], old_mentioned_users: [])
if has_changes?(issue, old_labels: old_labels)
todo_service.mark_pending_todos_as_done(issue, current_user)
diff --git a/app/services/spam_check_service.rb b/app/services/spam_check_service.rb
new file mode 100644
index 00000000000..023e0824e85
--- /dev/null
+++ b/app/services/spam_check_service.rb
@@ -0,0 +1,24 @@
+# SpamCheckService
+#
+# Provide helper methods for checking if a given spammable object has
+# potential spam data.
+#
+# Dependencies:
+# - params with :request
+#
+module SpamCheckService
+ def filter_spam_check_params
+ @request = params.delete(:request)
+ @api = params.delete(:api)
+ @recaptcha_verified = params.delete(:recaptcha_verified)
+ @spam_log_id = params.delete(:spam_log_id)
+ end
+
+ def spam_check(spammable, user)
+ spam_service = SpamService.new(spammable, @request)
+
+ spam_service.when_recaptcha_verified(@recaptcha_verified, @api) do
+ user.spam_logs.find_by(id: @spam_log_id)&.update!(recaptcha_verified: true)
+ end
+ end
+end
diff --git a/app/services/spam_service.rb b/app/services/spam_service.rb
index 024a7c19d33..3e65b7d31a3 100644
--- a/app/services/spam_service.rb
+++ b/app/services/spam_service.rb
@@ -17,15 +17,6 @@ class SpamService
end
end
- def check(api = false)
- return false unless request && check_for_spam?
-
- return false unless akismet.is_spam?
-
- create_spam_log(api)
- true
- end
-
def mark_as_spam!
return false unless spammable.submittable_as_spam?
@@ -36,8 +27,30 @@ class SpamService
end
end
+ def when_recaptcha_verified(recaptcha_verified, api = false)
+ # In case it's a request which is already verified through recaptcha, yield
+ # block.
+ if recaptcha_verified
+ yield
+ 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)
+ spammable.spam_log = spam_log
+ end
+ end
+
private
+ def check(api)
+ return false unless request && check_for_spam?
+
+ return false unless akismet.is_spam?
+
+ create_spam_log(api)
+ true
+ end
+
def akismet
@akismet ||= AkismetService.new(
spammable_owner,
diff --git a/app/services/update_snippet_service.rb b/app/services/update_snippet_service.rb
index a6bb36821c3..358bca73aec 100644
--- a/app/services/update_snippet_service.rb
+++ b/app/services/update_snippet_service.rb
@@ -1,4 +1,6 @@
class UpdateSnippetService < BaseService
+ include SpamCheckService
+
attr_accessor :snippet
def initialize(project, user, snippet, params)
@@ -9,7 +11,7 @@ class UpdateSnippetService < BaseService
def execute
# check that user is allowed to set specified visibility_level
new_visibility = params[:visibility_level]
-
+
if new_visibility && new_visibility.to_i != snippet.visibility_level
unless Gitlab::VisibilityLevel.allowed_for?(current_user, new_visibility)
deny_visibility_level(snippet, new_visibility)
@@ -17,6 +19,10 @@ class UpdateSnippetService < BaseService
end
end
- snippet.update_attributes(params)
+ filter_spam_check_params
+ snippet.assign_attributes(params)
+ spam_check(snippet, current_user)
+
+ snippet.save
end
end
diff --git a/app/views/layouts/_recaptcha_verification.html.haml b/app/views/layouts/_recaptcha_verification.html.haml
new file mode 100644
index 00000000000..77c77dc6754
--- /dev/null
+++ b/app/views/layouts/_recaptcha_verification.html.haml
@@ -0,0 +1,23 @@
+- humanized_resource_name = spammable.class.model_name.human.downcase
+- resource_name = spammable.class.model_name.singular
+
+%h3.page-title
+ Anti-spam verification
+%hr
+
+%p
+ #{"We detected potential spam in the #{humanized_resource_name}. Please solve the reCAPTCHA to proceed."}
+
+= form_for form do |f|
+ .recaptcha
+ - params[resource_name].each do |field, value|
+ = hidden_field(resource_name, field, value: value)
+ = hidden_field_tag(:spam_log_id, spammable.spam_log.id)
+ = hidden_field_tag(:recaptcha_verification, true)
+ = recaptcha_tags
+
+ -# Yields a block with given extra params.
+ = yield
+
+ .row-content-block.footer-block
+ = f.submit "Submit #{humanized_resource_name}", class: 'btn btn-create'
diff --git a/app/views/projects/issues/verify.html.haml b/app/views/projects/issues/verify.html.haml
index 1934b18c086..09aa401e44a 100644
--- a/app/views/projects/issues/verify.html.haml
+++ b/app/views/projects/issues/verify.html.haml
@@ -1,20 +1,4 @@
-- page_title "Anti-spam verification"
+- form = [@project.namespace.becomes(Namespace), @project, @issue]
-%h3.page-title
- Anti-spam verification
-%hr
-
-%p
- We detected potential spam in the issue description. Please verify that you are not a robot to submit the issue.
-
-= form_for [@project.namespace.becomes(Namespace), @project, @issue] do |f|
- .recaptcha
- - params[:issue].each do |field, value|
- = hidden_field(:issue, field, value: value)
- = hidden_field_tag(:merge_request_for_resolving_discussions, params[:merge_request_for_resolving_discussions])
- = hidden_field_tag(:spam_log_id, @issue.spam_log.id)
- = hidden_field_tag(:recaptcha_verification, true)
- = recaptcha_tags
-
- .row-content-block.footer-block
- = f.submit "Submit #{@issue.class.model_name.human.downcase}", class: 'btn btn-create'
+= render layout: 'layouts/recaptcha_verification', locals: { spammable: @issue, form: form } do
+ = hidden_field_tag(:merge_request_for_resolving_discussions, params[:merge_request_for_resolving_discussions])
diff --git a/app/views/projects/snippets/verify.html.haml b/app/views/projects/snippets/verify.html.haml
new file mode 100644
index 00000000000..eb56f03b3f4
--- /dev/null
+++ b/app/views/projects/snippets/verify.html.haml
@@ -0,0 +1,4 @@
+- form = [@project.namespace.becomes(Namespace), @project, @snippet.becomes(Snippet)]
+
+= render 'layouts/recaptcha_verification', spammable: @snippet, form: form
+
diff --git a/app/views/snippets/verify.html.haml b/app/views/snippets/verify.html.haml
new file mode 100644
index 00000000000..cb623ccab57
--- /dev/null
+++ b/app/views/snippets/verify.html.haml
@@ -0,0 +1,4 @@
+- form = [@snippet.becomes(Snippet)]
+
+= render 'layouts/recaptcha_verification', spammable: @snippet, form: form
+
diff --git a/changelogs/unreleased/28093-snippet-and-issue-spam-check-on-edit.yml b/changelogs/unreleased/28093-snippet-and-issue-spam-check-on-edit.yml
new file mode 100644
index 00000000000..d70b5ef8fd5
--- /dev/null
+++ b/changelogs/unreleased/28093-snippet-and-issue-spam-check-on-edit.yml
@@ -0,0 +1,4 @@
+---
+title: Spam check and reCAPTCHA improvements
+merge_request:
+author:
diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb
index dfab60f7fa5..868db241dab 100644
--- a/lib/api/helpers.rb
+++ b/lib/api/helpers.rb
@@ -231,6 +231,10 @@ module API
end
end
+ def render_spam_error!
+ render_api_error!({ error: 'Spam detected' }, 400)
+ end
+
def render_api_error!(message, status)
error!({ 'message' => message }, status, header)
end
diff --git a/lib/api/issues.rb b/lib/api/issues.rb
index 90fca20d4fa..d641865a6e2 100644
--- a/lib/api/issues.rb
+++ b/lib/api/issues.rb
@@ -177,9 +177,13 @@ module API
params.delete(:updated_at)
end
+ update_params = declared_params(include_missing: false).merge(request: request, api: true)
+
issue = ::Issues::UpdateService.new(user_project,
current_user,
- declared_params(include_missing: false)).execute(issue)
+ update_params).execute(issue)
+
+ render_spam_error! if issue.spam?
if issue.valid?
present issue, with: Entities::Issue, current_user: current_user, project: user_project
diff --git a/lib/api/project_snippets.rb b/lib/api/project_snippets.rb
index dcc0c82ee27..2a1cce73f3f 100644
--- a/lib/api/project_snippets.rb
+++ b/lib/api/project_snippets.rb
@@ -63,6 +63,8 @@ module API
snippet = CreateSnippetService.new(user_project, current_user, snippet_params).execute
+ render_spam_error! if snippet.spam?
+
if snippet.persisted?
present snippet, with: Entities::ProjectSnippet
else
@@ -92,12 +94,16 @@ module API
authorize! :update_project_snippet, snippet
snippet_params = declared_params(include_missing: false)
+ .merge(request: request, api: true)
+
snippet_params[:content] = snippet_params.delete(:code) if snippet_params[:code].present?
UpdateSnippetService.new(user_project, current_user, snippet,
snippet_params).execute
- if snippet.persisted?
+ render_spam_error! if snippet.spam?
+
+ if snippet.valid?
present snippet, with: Entities::ProjectSnippet
else
render_validation_error!(snippet)
diff --git a/lib/api/snippets.rb b/lib/api/snippets.rb
index eb9ece49e7f..ac03fbd2a3d 100644
--- a/lib/api/snippets.rb
+++ b/lib/api/snippets.rb
@@ -67,6 +67,8 @@ module API
attrs = declared_params(include_missing: false).merge(request: request, api: true)
snippet = CreateSnippetService.new(nil, current_user, attrs).execute
+ render_spam_error! if snippet.spam?
+
if snippet.persisted?
present snippet, with: Entities::PersonalSnippet
else
@@ -93,9 +95,12 @@ module API
return not_found!('Snippet') unless snippet
authorize! :update_personal_snippet, snippet
- attrs = declared_params(include_missing: false)
+ attrs = declared_params(include_missing: false).merge(request: request, api: true)
UpdateSnippetService.new(nil, current_user, snippet, attrs).execute
+
+ render_spam_error! if snippet.spam?
+
if snippet.persisted?
present snippet, with: Entities::PersonalSnippet
else
diff --git a/lib/api/v3/issues.rb b/lib/api/v3/issues.rb
index 081d45165e8..32f2dbded31 100644
--- a/lib/api/v3/issues.rb
+++ b/lib/api/v3/issues.rb
@@ -148,9 +148,7 @@ module API
issue = ::Issues::CreateService.new(user_project,
current_user,
issue_params.merge(request: request, api: true)).execute
- if issue.spam?
- render_api_error!({ error: 'Spam detected' }, 400)
- end
+ render_spam_error! if issue.spam?
if issue.valid?
present issue, with: ::API::Entities::Issue, current_user: current_user, project: user_project
@@ -181,9 +179,13 @@ module API
params.delete(:updated_at)
end
+ update_params = declared_params(include_missing: false).merge(request: request, api: true)
+
issue = ::Issues::UpdateService.new(user_project,
current_user,
- declared_params(include_missing: false)).execute(issue)
+ update_params).execute(issue)
+
+ render_spam_error! if issue.spam?
if issue.valid?
present issue, with: ::API::Entities::Issue, current_user: current_user, project: user_project
diff --git a/lib/api/v3/project_snippets.rb b/lib/api/v3/project_snippets.rb
index 9f95d4395fa..e03e941d30b 100644
--- a/lib/api/v3/project_snippets.rb
+++ b/lib/api/v3/project_snippets.rb
@@ -64,6 +64,8 @@ module API
snippet = CreateSnippetService.new(user_project, current_user, snippet_params).execute
+ render_spam_error! if snippet.spam?
+
if snippet.persisted?
present snippet, with: ::API::V3::Entities::ProjectSnippet
else
@@ -93,12 +95,16 @@ module API
authorize! :update_project_snippet, snippet
snippet_params = declared_params(include_missing: false)
+ .merge(request: request, api: true)
+
snippet_params[:content] = snippet_params.delete(:code) if snippet_params[:code].present?
UpdateSnippetService.new(user_project, current_user, snippet,
snippet_params).execute
- if snippet.persisted?
+ render_spam_error! if snippet.spam?
+
+ if snippet.valid?
present snippet, with: ::API::V3::Entities::ProjectSnippet
else
render_validation_error!(snippet)
diff --git a/spec/controllers/projects/issues_controller_spec.rb b/spec/controllers/projects/issues_controller_spec.rb
index 4b89381eb96..ea70d518e9b 100644
--- a/spec/controllers/projects/issues_controller_spec.rb
+++ b/spec/controllers/projects/issues_controller_spec.rb
@@ -150,6 +150,113 @@ describe Projects::IssuesController do
end
end
+ context 'Akismet is enabled' do
+ let(:project) { create(:project_empty_repo, :public) }
+
+ 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(:is_spam?).and_return(false)
+ end
+
+ it 'normally updates the issue' do
+ expect { update_issue(title: 'Foo') }.to change { issue.reload.title }.to('Foo')
+ end
+ end
+
+ context 'when an issue is identified as spam' do
+ before { allow_any_instance_of(AkismetService).to receive(:is_spam?).and_return(true) }
+
+ context 'when captcha is not verified' do
+ def update_spam_issue
+ update_issue(title: 'Spam Title', description: 'Spam lives here')
+ end
+
+ before { allow_any_instance_of(described_class).to receive(:verify_recaptcha).and_return(false) }
+
+ it 'rejects an issue recognized as a spam' do
+ expect { update_spam_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)
+
+ expect { update_spam_issue }.not_to change{ issue.reload.title }
+ end
+
+ it 'creates a spam log' do
+ update_spam_issue
+
+ spam_logs = SpamLog.all
+
+ 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 verify template' do
+ update_spam_issue
+
+ expect(response).to render_template(:verify)
+ end
+ end
+
+ context 'when captcha is verified' do
+ let(:spammy_title) { 'Whatever' }
+ let!(:spam_logs) { create_list(:spam_log, 2, user: user, title: spammy_title) }
+
+ def update_verified_issue
+ update_issue({ title: spammy_title },
+ { 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 'redirect to issue page' do
+ update_verified_issue
+
+ expect(response).
+ to redirect_to(namespace_project_issue_path(project.namespace, project, issue))
+ end
+
+ it 'accepts an issue after recaptcha is verified' do
+ expect{ update_verified_issue }.to change{ issue.reload.title }.to(spammy_title)
+ end
+
+ it 'marks spam log as recaptcha_verified' do
+ expect { update_verified_issue }.to change { SpamLog.last.recaptcha_verified }.from(false).to(true)
+ end
+
+ it 'does not mark spam log as recaptcha_verified when it does not belong to current_user' do
+ spam_log = create(:spam_log)
+
+ expect { update_issue(spam_log_id: spam_log.id, recaptcha_verification: true) }.
+ not_to change { SpamLog.last.recaptcha_verified }
+ end
+ end
+ end
+ end
+
+ def update_issue(issue_params = {}, additional_params = {})
+ params = {
+ namespace_id: project.namespace.to_param,
+ project_id: project.to_param,
+ id: issue.iid,
+ issue: issue_params
+ }.merge(additional_params)
+
+ put :update, params
+ end
+
def move_issue
put :update,
namespace_id: project.namespace.to_param,
@@ -382,7 +489,7 @@ describe Projects::IssuesController do
allow_any_instance_of(SpamService).to receive(:check_for_spam?).and_return(true)
end
- context 'when an issue is not identified as a spam' do
+ 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(:is_spam?).and_return(false)
@@ -393,7 +500,7 @@ describe Projects::IssuesController do
end
end
- context 'when an issue is identified as a spam' do
+ context 'when an issue is identified as spam' do
before { allow_any_instance_of(AkismetService).to receive(:is_spam?).and_return(true) }
context 'when captcha is not verified' do
diff --git a/spec/controllers/projects/snippets_controller_spec.rb b/spec/controllers/projects/snippets_controller_spec.rb
index 19e948d8fb8..f5694cea7f8 100644
--- a/spec/controllers/projects/snippets_controller_spec.rb
+++ b/spec/controllers/projects/snippets_controller_spec.rb
@@ -70,7 +70,7 @@ describe Projects::SnippetsController do
end
describe 'POST #create' do
- def create_snippet(project, snippet_params = {})
+ def create_snippet(project, snippet_params = {}, additional_params = {})
sign_in(user)
project.add_developer(user)
@@ -79,7 +79,7 @@ describe Projects::SnippetsController do
namespace_id: project.namespace.to_param,
project_id: project.to_param,
project_snippet: { title: 'Title', content: 'Content' }.merge(snippet_params)
- }
+ }.merge(additional_params)
end
context 'when the snippet is spam' do
@@ -87,35 +87,179 @@ describe Projects::SnippetsController do
allow_any_instance_of(AkismetService).to receive(:is_spam?).and_return(true)
end
- context 'when the project is private' do
- let(:private_project) { create(:project_empty_repo, :private) }
+ context 'when the snippet is private' do
+ it 'creates the snippet' do
+ expect { create_snippet(project, visibility_level: Snippet::PRIVATE) }.
+ to change { Snippet.count }.by(1)
+ end
+ end
+
+ context 'when the snippet is public' do
+ it 'rejects the shippet' do
+ expect { create_snippet(project, visibility_level: Snippet::PUBLIC) }.
+ not_to change { Snippet.count }
+ expect(response).to render_template(:new)
+ end
+
+ it 'creates a spam log' do
+ expect { create_snippet(project, visibility_level: Snippet::PUBLIC) }.
+ to change { SpamLog.count }.by(1)
+ end
+
+ it 'renders :new with recaptcha disabled' do
+ stub_application_setting(recaptcha_enabled: false)
+
+ create_snippet(project, visibility_level: Snippet::PUBLIC)
+
+ expect(response).to render_template(:new)
+ end
- context 'when the snippet is public' do
- it 'creates the snippet' do
- expect { create_snippet(private_project, visibility_level: Snippet::PUBLIC) }.
- to change { Snippet.count }.by(1)
+ context 'recaptcha enabled' do
+ before do
+ stub_application_setting(recaptcha_enabled: true)
end
+
+ it 'renders :verify with recaptcha enabled' do
+ create_snippet(project, visibility_level: Snippet::PUBLIC)
+
+ expect(response).to render_template(:verify)
+ end
+
+ it 'renders snippet page when recaptcha verified' do
+ spammy_title = 'Whatever'
+
+ spam_logs = create_list(:spam_log, 2, user: user, title: spammy_title)
+ create_snippet(project,
+ { visibility_level: Snippet::PUBLIC },
+ { spam_log_id: spam_logs.last.id,
+ recaptcha_verification: true })
+
+ expect(response).to redirect_to(Snippet.last)
+ end
+ end
+ end
+ end
+ end
+
+ describe 'PUT #update' do
+ let(:project) { create :project, :public }
+ let(:snippet) { create :project_snippet, author: user, project: project, visibility_level: visibility_level }
+
+ def update_snippet(snippet_params = {}, additional_params = {})
+ sign_in(user)
+
+ project.add_developer(user)
+
+ put :update, {
+ namespace_id: project.namespace.to_param,
+ project_id: project.to_param,
+ id: snippet.id,
+ project_snippet: { title: 'Title', content: 'Content' }.merge(snippet_params)
+ }.merge(additional_params)
+
+ snippet.reload
+ end
+
+ context 'when the snippet is spam' do
+ before do
+ allow_any_instance_of(AkismetService).to receive(:is_spam?).and_return(true)
+ end
+
+ context 'when the snippet is private' do
+ let(:visibility_level) { Snippet::PRIVATE }
+
+ it 'updates the snippet' do
+ expect { update_snippet(title: 'Foo') }.
+ to change { snippet.reload.title }.to('Foo')
end
end
- context 'when the project is public' do
- context 'when the snippet is private' do
- it 'creates the snippet' do
- expect { create_snippet(project, visibility_level: Snippet::PRIVATE) }.
- to change { Snippet.count }.by(1)
+ context 'when the snippet is public' do
+ let(:visibility_level) { Snippet::PUBLIC }
+
+ it 'rejects the shippet' do
+ expect { update_snippet(title: 'Foo') }.
+ not_to change { snippet.reload.title }
+ end
+
+ it 'creates a spam log' do
+ expect { update_snippet(title: 'Foo') }.
+ to change { SpamLog.count }.by(1)
+ end
+
+ it 'renders :edit with recaptcha disabled' do
+ stub_application_setting(recaptcha_enabled: false)
+
+ update_snippet(title: 'Foo')
+
+ expect(response).to render_template(:edit)
+ end
+
+ context 'recaptcha enabled' do
+ before do
+ stub_application_setting(recaptcha_enabled: true)
+ end
+
+ it 'renders :verify with recaptcha enabled' do
+ update_snippet(title: 'Foo')
+
+ expect(response).to render_template(:verify)
+ end
+
+ it 'renders snippet page when recaptcha verified' do
+ spammy_title = 'Whatever'
+
+ spam_logs = create_list(:spam_log, 2, user: user, title: spammy_title)
+ snippet = update_snippet({ title: spammy_title },
+ { spam_log_id: spam_logs.last.id,
+ recaptcha_verification: true })
+
+ expect(response).to redirect_to(snippet)
end
end
+ end
+
+ context 'when the private snippet is made public' do
+ let(:visibility_level) { Snippet::PRIVATE }
+
+ it 'rejects the shippet' do
+ expect { update_snippet(title: 'Foo', visibility_level: Snippet::PUBLIC) }.
+ not_to change { snippet.reload.title }
+ end
+
+ it 'creates a spam log' do
+ expect { update_snippet(title: 'Foo', visibility_level: Snippet::PUBLIC) }.
+ to change { SpamLog.count }.by(1)
+ end
+
+ it 'renders :edit with recaptcha disabled' do
+ stub_application_setting(recaptcha_enabled: false)
- context 'when the snippet is public' do
- it 'rejects the shippet' do
- expect { create_snippet(project, visibility_level: Snippet::PUBLIC) }.
- not_to change { Snippet.count }
- expect(response).to render_template(:new)
+ update_snippet(title: 'Foo', visibility_level: Snippet::PUBLIC)
+
+ expect(response).to render_template(:edit)
+ end
+
+ context 'recaptcha enabled' do
+ before do
+ stub_application_setting(recaptcha_enabled: true)
+ end
+
+ it 'renders :verify with recaptcha enabled' do
+ update_snippet(title: 'Foo', visibility_level: Snippet::PUBLIC)
+
+ expect(response).to render_template(:verify)
end
- it 'creates a spam log' do
- expect { create_snippet(project, visibility_level: Snippet::PUBLIC) }.
- to change { SpamLog.count }.by(1)
+ it 'renders snippet page when recaptcha verified' do
+ spammy_title = 'Whatever'
+
+ spam_logs = create_list(:spam_log, 2, user: user, title: spammy_title)
+ snippet = update_snippet({ title: spammy_title, visibility_level: Snippet::PUBLIC },
+ { spam_log_id: spam_logs.last.id,
+ recaptcha_verification: true })
+
+ expect(response).to redirect_to(snippet)
end
end
end
diff --git a/spec/controllers/snippets_controller_spec.rb b/spec/controllers/snippets_controller_spec.rb
index dadcb90cfc2..4b1d50f2b3b 100644
--- a/spec/controllers/snippets_controller_spec.rb
+++ b/spec/controllers/snippets_controller_spec.rb
@@ -139,12 +139,14 @@ describe SnippetsController do
end
describe 'POST #create' do
- def create_snippet(snippet_params = {})
+ def create_snippet(snippet_params = {}, additional_params = {})
sign_in(user)
post :create, {
personal_snippet: { title: 'Title', content: 'Content' }.merge(snippet_params)
- }
+ }.merge(additional_params)
+
+ Snippet.last
end
context 'when the snippet is spam' do
@@ -163,13 +165,164 @@ describe SnippetsController do
it 'rejects the shippet' do
expect { create_snippet(visibility_level: Snippet::PUBLIC) }.
not_to change { Snippet.count }
- expect(response).to render_template(:new)
end
it 'creates a spam log' do
expect { create_snippet(visibility_level: Snippet::PUBLIC) }.
to change { SpamLog.count }.by(1)
end
+
+ it 'renders :new with recaptcha disabled' do
+ stub_application_setting(recaptcha_enabled: false)
+
+ create_snippet(visibility_level: Snippet::PUBLIC)
+
+ expect(response).to render_template(:new)
+ end
+
+ context 'recaptcha enabled' do
+ before do
+ stub_application_setting(recaptcha_enabled: true)
+ end
+
+ it 'renders :verify with recaptcha enabled' do
+ create_snippet(visibility_level: Snippet::PUBLIC)
+
+ expect(response).to render_template(:verify)
+ end
+
+ it 'renders snippet page when recaptcha verified' do
+ spammy_title = 'Whatever'
+
+ spam_logs = create_list(:spam_log, 2, user: user, title: spammy_title)
+ snippet = create_snippet({ title: spammy_title },
+ { spam_log_id: spam_logs.last.id,
+ recaptcha_verification: true })
+
+ expect(response).to redirect_to(snippet_path(snippet))
+ end
+ end
+ end
+ end
+ end
+
+ describe 'PUT #update' do
+ let(:project) { create :project }
+ let(:snippet) { create :personal_snippet, author: user, project: project, visibility_level: visibility_level }
+
+ def update_snippet(snippet_params = {}, additional_params = {})
+ sign_in(user)
+
+ put :update, {
+ id: snippet.id,
+ personal_snippet: { title: 'Title', content: 'Content' }.merge(snippet_params)
+ }.merge(additional_params)
+
+ snippet.reload
+ end
+
+ context 'when the snippet is spam' do
+ before do
+ allow_any_instance_of(AkismetService).to receive(:is_spam?).and_return(true)
+ end
+
+ context 'when the snippet is private' do
+ let(:visibility_level) { Snippet::PRIVATE }
+
+ it 'updates the snippet' do
+ expect { update_snippet(title: 'Foo') }.
+ to change { snippet.reload.title }.to('Foo')
+ end
+ end
+
+ context 'when a private snippet is made public' do
+ let(:visibility_level) { Snippet::PRIVATE }
+
+ it 'rejects the snippet' do
+ expect { update_snippet(title: 'Foo', visibility_level: Snippet::PUBLIC) }.
+ not_to change { snippet.reload.title }
+ end
+
+ it 'creates a spam log' do
+ expect { update_snippet(title: 'Foo', visibility_level: Snippet::PUBLIC) }.
+ to change { SpamLog.count }.by(1)
+ end
+
+ it 'renders :edit with recaptcha disabled' do
+ stub_application_setting(recaptcha_enabled: false)
+
+ update_snippet(title: 'Foo', visibility_level: Snippet::PUBLIC)
+
+ expect(response).to render_template(:edit)
+ end
+
+ context 'recaptcha enabled' do
+ before do
+ stub_application_setting(recaptcha_enabled: true)
+ end
+
+ it 'renders :verify with recaptcha enabled' do
+ update_snippet(title: 'Foo', visibility_level: Snippet::PUBLIC)
+
+ expect(response).to render_template(:verify)
+ end
+
+ it 'renders snippet page when recaptcha verified' do
+ spammy_title = 'Whatever'
+
+ spam_logs = create_list(:spam_log, 2, user: user, title: spammy_title)
+ snippet = update_snippet({ title: spammy_title, visibility_level: Snippet::PUBLIC },
+ { spam_log_id: spam_logs.last.id,
+ recaptcha_verification: true })
+
+ expect(response).to redirect_to(snippet)
+ end
+ end
+ end
+
+ context 'when the snippet is public' do
+ let(:visibility_level) { Snippet::PUBLIC }
+
+ it 'rejects the shippet' do
+ expect { update_snippet(title: 'Foo') }.
+ not_to change { snippet.reload.title }
+ end
+
+ it 'creates a spam log' do
+ expect { update_snippet(title: 'Foo') }.
+ to change { SpamLog.count }.by(1)
+ end
+
+ it 'renders :edit with recaptcha disabled' do
+ stub_application_setting(recaptcha_enabled: false)
+
+ update_snippet(title: 'Foo')
+
+ expect(response).to render_template(:edit)
+ end
+
+ context 'recaptcha enabled' do
+ before do
+ stub_application_setting(recaptcha_enabled: true)
+ end
+
+ it 'renders :verify with recaptcha enabled' do
+ update_snippet(title: 'Foo')
+
+ expect(response).to render_template(:verify)
+ end
+
+ it 'renders snippet page when recaptcha verified' do
+ spammy_title = 'Whatever'
+
+ spam_logs = create_list(:spam_log, 2, user: user, title: spammy_title)
+ snippet = update_snippet({ title: spammy_title },
+ { spam_log_id: spam_logs.last.id,
+ recaptcha_verification: true })
+
+ expect(response).to redirect_to(snippet_path(snippet))
+ end
+ end
end
end
end
diff --git a/spec/models/concerns/spammable_spec.rb b/spec/models/concerns/spammable_spec.rb
index b6e5c95d18a..fd3b8307571 100644
--- a/spec/models/concerns/spammable_spec.rb
+++ b/spec/models/concerns/spammable_spec.rb
@@ -23,6 +23,7 @@ describe Issue, 'Spammable' do
describe '#check_for_spam?' do
it 'returns true for public project' do
issue.project.update_attribute(:visibility_level, Gitlab::VisibilityLevel::PUBLIC)
+
expect(issue.check_for_spam?).to eq(true)
end
diff --git a/spec/requests/api/issues_spec.rb b/spec/requests/api/issues_spec.rb
index cca00df9591..d78332be22b 100644
--- a/spec/requests/api/issues_spec.rb
+++ b/spec/requests/api/issues_spec.rb
@@ -919,6 +919,33 @@ describe API::Issues, api: true do
end
end
+ describe 'PUT /projects/:id/issues/:issue_id with spam filtering' do
+ let(:params) do
+ {
+ title: 'updated title',
+ description: 'content here',
+ labels: 'label, label2'
+ }
+ 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(is_spam?: true)
+
+ put api("/projects/#{project.id}/issues/#{issue.id}", user), params
+
+ expect(response).to have_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')
+ end
+ end
+
describe 'PUT /projects/:id/issues/:issue_id to update labels' do
let!(:label) { create(:label, title: 'dummy', project: project) }
let!(:label_link) { create(:label_link, label: label, target: issue) }
diff --git a/spec/requests/api/project_snippets_spec.rb b/spec/requests/api/project_snippets_spec.rb
index eea76c7bb94..a9f8bf6b4e0 100644
--- a/spec/requests/api/project_snippets_spec.rb
+++ b/spec/requests/api/project_snippets_spec.rb
@@ -73,43 +73,33 @@ describe API::ProjectSnippets, api: true do
allow_any_instance_of(AkismetService).to receive(:is_spam?).and_return(true)
end
- context 'when the project is private' do
- let(:private_project) { create(:project_empty_repo, :private) }
-
- context 'when the snippet is public' do
- it 'creates the snippet' do
- expect { create_snippet(private_project, visibility_level: Snippet::PUBLIC) }.
- to change { Snippet.count }.by(1)
- end
+ context 'when the snippet is private' do
+ it 'creates the snippet' do
+ expect { create_snippet(project, visibility_level: Snippet::PRIVATE) }.
+ to change { Snippet.count }.by(1)
end
end
- context 'when the project is public' do
- context 'when the snippet is private' do
- it 'creates the snippet' do
- expect { create_snippet(project, visibility_level: Snippet::PRIVATE) }.
- to change { Snippet.count }.by(1)
- end
+ context 'when the snippet is public' do
+ it 'rejects the shippet' do
+ expect { create_snippet(project, visibility_level: Snippet::PUBLIC) }.
+ not_to change { Snippet.count }
+
+ expect(response).to have_http_status(400)
+ expect(json_response['message']).to eq({ "error" => "Spam detected" })
end
- context 'when the snippet is public' do
- it 'rejects the shippet' do
- expect { create_snippet(project, visibility_level: Snippet::PUBLIC) }.
- not_to change { Snippet.count }
- expect(response).to have_http_status(400)
- end
-
- it 'creates a spam log' do
- expect { create_snippet(project, visibility_level: Snippet::PUBLIC) }.
- to change { SpamLog.count }.by(1)
- end
+ it 'creates a spam log' do
+ expect { create_snippet(project, visibility_level: Snippet::PUBLIC) }.
+ to change { SpamLog.count }.by(1)
end
end
end
end
describe 'PUT /projects/:project_id/snippets/:id/' do
- let(:snippet) { create(:project_snippet, author: admin) }
+ let(:visibility_level) { Snippet::PUBLIC }
+ let(:snippet) { create(:project_snippet, author: admin, visibility_level: visibility_level) }
it 'updates snippet' do
new_content = 'New content'
@@ -133,6 +123,56 @@ describe API::ProjectSnippets, api: true do
expect(response).to have_http_status(400)
end
+
+ context 'when the snippet is spam' do
+ def update_snippet(snippet_params = {})
+ put api("/projects/#{snippet.project.id}/snippets/#{snippet.id}", admin), snippet_params
+ end
+
+ before do
+ allow_any_instance_of(AkismetService).to receive(:is_spam?).and_return(true)
+ end
+
+ context 'when the snippet is private' do
+ let(:visibility_level) { Snippet::PRIVATE }
+
+ it 'creates the snippet' do
+ expect { update_snippet(title: 'Foo') }.
+ to change { snippet.reload.title }.to('Foo')
+ end
+ end
+
+ context 'when the snippet is public' do
+ let(:visibility_level) { Snippet::PUBLIC }
+
+ it 'rejects the snippet' do
+ expect { update_snippet(title: 'Foo') }.
+ not_to change { snippet.reload.title }
+ end
+
+ it 'creates a spam log' do
+ expect { update_snippet(title: 'Foo') }.
+ to change { SpamLog.count }.by(1)
+ end
+ end
+
+ context 'when the private snippet is made public' do
+ let(:visibility_level) { Snippet::PRIVATE }
+
+ it 'rejects the snippet' do
+ expect { update_snippet(title: 'Foo', visibility_level: Snippet::PUBLIC) }.
+ not_to change { snippet.reload.title }
+
+ expect(response).to have_http_status(400)
+ expect(json_response['message']).to eq({ "error" => "Spam detected" })
+ end
+
+ it 'creates a spam log' do
+ expect { update_snippet(title: 'Foo', visibility_level: Snippet::PUBLIC) }.
+ to change { SpamLog.count }.by(1)
+ end
+ end
+ end
end
describe 'DELETE /projects/:project_id/snippets/:id/' do
diff --git a/spec/requests/api/snippets_spec.rb b/spec/requests/api/snippets_spec.rb
index 6b9a739b439..bdd694abc85 100644
--- a/spec/requests/api/snippets_spec.rb
+++ b/spec/requests/api/snippets_spec.rb
@@ -122,7 +122,9 @@ describe API::Snippets, api: true do
it 'rejects the shippet' do
expect { create_snippet(visibility_level: Snippet::PUBLIC) }.
not_to change { Snippet.count }
+
expect(response).to have_http_status(400)
+ expect(json_response['message']).to eq({ "error" => "Spam detected" })
end
it 'creates a spam log' do
@@ -134,16 +136,20 @@ describe API::Snippets, api: true do
end
describe 'PUT /snippets/:id' do
+ let(:visibility_level) { Snippet::PUBLIC }
let(:other_user) { create(:user) }
- let(:public_snippet) { create(:personal_snippet, :public, author: user) }
+ let(:snippet) do
+ create(:personal_snippet, author: user, visibility_level: visibility_level)
+ end
+
it 'updates snippet' do
new_content = 'New content'
- put api("/snippets/#{public_snippet.id}", user), content: new_content
+ put api("/snippets/#{snippet.id}", user), content: new_content
expect(response).to have_http_status(200)
- public_snippet.reload
- expect(public_snippet.content).to eq(new_content)
+ snippet.reload
+ expect(snippet.content).to eq(new_content)
end
it 'returns 404 for invalid snippet id' do
@@ -154,7 +160,7 @@ describe API::Snippets, api: true do
end
it "returns 404 for another user's snippet" do
- put api("/snippets/#{public_snippet.id}", other_user), title: 'fubar'
+ put api("/snippets/#{snippet.id}", other_user), title: 'fubar'
expect(response).to have_http_status(404)
expect(json_response['message']).to eq('404 Snippet Not Found')
@@ -165,6 +171,56 @@ describe API::Snippets, api: true do
expect(response).to have_http_status(400)
end
+
+ context 'when the snippet is spam' do
+ def update_snippet(snippet_params = {})
+ put api("/snippets/#{snippet.id}", user), snippet_params
+ end
+
+ before do
+ allow_any_instance_of(AkismetService).to receive(:is_spam?).and_return(true)
+ end
+
+ context 'when the snippet is private' do
+ let(:visibility_level) { Snippet::PRIVATE }
+
+ it 'updates the snippet' do
+ expect { update_snippet(title: 'Foo') }.
+ to change { snippet.reload.title }.to('Foo')
+ end
+ end
+
+ context 'when the snippet is public' do
+ let(:visibility_level) { Snippet::PUBLIC }
+
+ it 'rejects the shippet' do
+ expect { update_snippet(title: 'Foo') }.
+ not_to change { snippet.reload.title }
+
+ expect(response).to have_http_status(400)
+ expect(json_response['message']).to eq({ "error" => "Spam detected" })
+ end
+
+ it 'creates a spam log' do
+ expect { update_snippet(title: 'Foo') }.
+ to change { SpamLog.count }.by(1)
+ end
+ end
+
+ context 'when a private snippet is made public' do
+ let(:visibility_level) { Snippet::PRIVATE }
+
+ it 'rejects the snippet' do
+ expect { update_snippet(title: 'Foo', visibility_level: Snippet::PUBLIC) }.
+ not_to change { snippet.reload.title }
+ end
+
+ it 'creates a spam log' do
+ expect { update_snippet(title: 'Foo', visibility_level: Snippet::PUBLIC) }.
+ to change { SpamLog.count }.by(1)
+ end
+ end
+ end
end
describe 'DELETE /snippets/:id' do
diff --git a/spec/requests/api/v3/issues_spec.rb b/spec/requests/api/v3/issues_spec.rb
index 33a127de98a..8e6732fe23e 100644
--- a/spec/requests/api/v3/issues_spec.rb
+++ b/spec/requests/api/v3/issues_spec.rb
@@ -986,6 +986,33 @@ describe API::V3::Issues, api: true do
end
end
+ describe 'PUT /projects/:id/issues/:issue_id with spam filtering' do
+ let(:params) do
+ {
+ title: 'updated title',
+ description: 'content here',
+ labels: 'label, label2'
+ }
+ 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(is_spam?: true)
+
+ put v3_api("/projects/#{project.id}/issues/#{issue.id}", user), params
+
+ expect(response).to have_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')
+ end
+ end
+
describe 'PUT /projects/:id/issues/:issue_id to update labels' do
let!(:label) { create(:label, title: 'dummy', project: project) }
let!(:label_link) { create(:label_link, label: label, target: issue) }
diff --git a/spec/requests/api/v3/project_snippets_spec.rb b/spec/requests/api/v3/project_snippets_spec.rb
index 3700477f0db..957a3bf97ef 100644
--- a/spec/requests/api/v3/project_snippets_spec.rb
+++ b/spec/requests/api/v3/project_snippets_spec.rb
@@ -85,43 +85,33 @@ describe API::ProjectSnippets, api: true do
allow_any_instance_of(AkismetService).to receive(:is_spam?).and_return(true)
end
- context 'when the project is private' do
- let(:private_project) { create(:project_empty_repo, :private) }
-
- context 'when the snippet is public' do
- it 'creates the snippet' do
- expect { create_snippet(private_project, visibility_level: Snippet::PUBLIC) }.
- to change { Snippet.count }.by(1)
- end
+ context 'when the snippet is private' do
+ it 'creates the snippet' do
+ expect { create_snippet(project, visibility_level: Snippet::PRIVATE) }.
+ to change { Snippet.count }.by(1)
end
end
- context 'when the project is public' do
- context 'when the snippet is private' do
- it 'creates the snippet' do
- expect { create_snippet(project, visibility_level: Snippet::PRIVATE) }.
- to change { Snippet.count }.by(1)
- end
+ context 'when the snippet is public' do
+ it 'rejects the shippet' do
+ expect { create_snippet(project, visibility_level: Snippet::PUBLIC) }.
+ not_to change { Snippet.count }
+
+ expect(response).to have_http_status(400)
+ expect(json_response['message']).to eq({ "error" => "Spam detected" })
end
- context 'when the snippet is public' do
- it 'rejects the shippet' do
- expect { create_snippet(project, visibility_level: Snippet::PUBLIC) }.
- not_to change { Snippet.count }
- expect(response).to have_http_status(400)
- end
-
- it 'creates a spam log' do
- expect { create_snippet(project, visibility_level: Snippet::PUBLIC) }.
- to change { SpamLog.count }.by(1)
- end
+ it 'creates a spam log' do
+ expect { create_snippet(project, visibility_level: Snippet::PUBLIC) }.
+ to change { SpamLog.count }.by(1)
end
end
end
end
describe 'PUT /projects/:project_id/snippets/:id/' do
- let(:snippet) { create(:project_snippet, author: admin) }
+ let(:visibility_level) { Snippet::PUBLIC }
+ let(:snippet) { create(:project_snippet, author: admin, visibility_level: visibility_level) }
it 'updates snippet' do
new_content = 'New content'
@@ -145,6 +135,56 @@ describe API::ProjectSnippets, api: true do
expect(response).to have_http_status(400)
end
+
+ context 'when the snippet is spam' do
+ def update_snippet(snippet_params = {})
+ put v3_api("/projects/#{snippet.project.id}/snippets/#{snippet.id}", admin), snippet_params
+ end
+
+ before do
+ allow_any_instance_of(AkismetService).to receive(:is_spam?).and_return(true)
+ end
+
+ context 'when the snippet is private' do
+ let(:visibility_level) { Snippet::PRIVATE }
+
+ it 'creates the snippet' do
+ expect { update_snippet(title: 'Foo') }.
+ to change { snippet.reload.title }.to('Foo')
+ end
+ end
+
+ context 'when the snippet is public' do
+ let(:visibility_level) { Snippet::PUBLIC }
+
+ it 'rejects the snippet' do
+ expect { update_snippet(title: 'Foo') }.
+ not_to change { snippet.reload.title }
+ end
+
+ it 'creates a spam log' do
+ expect { update_snippet(title: 'Foo') }.
+ to change { SpamLog.count }.by(1)
+ end
+ end
+
+ context 'when the private snippet is made public' do
+ let(:visibility_level) { Snippet::PRIVATE }
+
+ it 'rejects the snippet' do
+ expect { update_snippet(title: 'Foo', visibility_level: Snippet::PUBLIC) }.
+ not_to change { snippet.reload.title }
+
+ expect(response).to have_http_status(400)
+ expect(json_response['message']).to eq({ "error" => "Spam detected" })
+ end
+
+ it 'creates a spam log' do
+ expect { update_snippet(title: 'Foo', visibility_level: Snippet::PUBLIC) }.
+ to change { SpamLog.count }.by(1)
+ end
+ end
+ end
end
describe 'DELETE /projects/:project_id/snippets/:id/' do
diff --git a/spec/services/issues/create_service_spec.rb b/spec/services/issues/create_service_spec.rb
index 30578ee4c7d..98aab32d209 100644
--- a/spec/services/issues/create_service_spec.rb
+++ b/spec/services/issues/create_service_spec.rb
@@ -229,16 +229,6 @@ describe Issues::CreateService, services: true do
expect { issue }.not_to change{SpamLog.last.recaptcha_verified}
end
end
-
- context 'when spam log title does not match the issue title' do
- before do
- opts[:title] = 'Another issue'
- end
-
- it 'does not mark spam_log as recaptcha_verified' do
- expect { issue }.not_to change{SpamLog.last.recaptcha_verified}
- end
- end
end
context 'when recaptcha was not verified' do
diff --git a/spec/services/spam_service_spec.rb b/spec/services/spam_service_spec.rb
index 271c17dd8c0..4ce3b95aa87 100644
--- a/spec/services/spam_service_spec.rb
+++ b/spec/services/spam_service_spec.rb
@@ -1,46 +1,61 @@
require 'spec_helper'
describe SpamService, services: true do
- describe '#check' do
- let(:project) { create(:project, :public) }
- let(:issue) { create(:issue, project: project) }
- let(:request) { double(:request, env: {}) }
+ describe '#when_recaptcha_verified' do
+ def check_spam(issue, request, recaptcha_verified)
+ described_class.new(issue, request).when_recaptcha_verified(recaptcha_verified) do
+ 'yielded'
+ end
+ end
+
+ it 'yields block when recaptcha was already verified' do
+ issue = build_stubbed(:issue)
- def check_spam(issue, request)
- described_class.new(issue, request).check
+ expect(check_spam(issue, nil, true)).to eql('yielded')
end
- context 'when indicated as spam by akismet' do
- before { allow(AkismetService).to receive(:new).and_return(double(is_spam?: true)) }
+ context 'when recaptcha was not verified' do
+ let(:project) { create(:project, :public) }
+ let(:issue) { create(:issue, project: project) }
+ let(:request) { double(:request, env: {}) }
- it 'returns false when request is missing' do
- expect(check_spam(issue, nil)).to be_falsey
- end
+ context 'when indicated as spam by akismet' do
+ before { allow(AkismetService).to receive(:new).and_return(double(is_spam?: true)) }
- it 'returns false when issue is not public' do
- issue = create(:issue, project: create(:project, :private))
+ it 'doesnt check as spam when request is missing' do
+ check_spam(issue, nil, false)
- expect(check_spam(issue, request)).to be_falsey
- end
+ expect(issue.spam).to be_falsey
+ end
- it 'returns true' do
- expect(check_spam(issue, request)).to be_truthy
- end
+ it 'checks as spam' do
+ check_spam(issue, request, false)
- it 'creates a spam log' do
- expect { check_spam(issue, request) }.to change { SpamLog.count }.from(0).to(1)
- end
- end
+ expect(issue.spam).to be_truthy
+ end
- context 'when not indicated as spam by akismet' do
- before { allow(AkismetService).to receive(:new).and_return(double(is_spam?: false)) }
+ it 'creates a spam log' do
+ expect { check_spam(issue, request, false) }
+ .to change { SpamLog.count }.from(0).to(1)
+ end
- it 'returns false' do
- expect(check_spam(issue, request)).to be_falsey
+ 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) }.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