diff --git a/app/controllers/concerns/spammable_actions.rb b/app/controllers/concerns/spammable_actions.rb
index a6891149bfa374808372a3d4e93ebcee91d6e3d5..da225d8f1c75cb5fade735f9a1e211817ff4e779 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 744a4af1c51b97fce8b9c751be2bf3441dd0dc70..6ef36771ac12d6d89d24a34b9a33fa36b2040b7c 100644
--- a/app/controllers/projects/issues_controller.rb
+++ b/app/controllers/projects/issues_controller.rb
@@ -94,15 +94,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
@@ -111,7 +111,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])
@@ -123,11 +125,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
@@ -179,20 +177,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 ef5d3d242eb3e6c50453048d883317c17b35f72e..ea1a97b7cf0d6ecaea4e8ec66cc37b4d02c18c6b 100644
--- a/app/controllers/projects/snippets_controller.rb
+++ b/app/controllers/projects/snippets_controller.rb
@@ -38,24 +38,19 @@ 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 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 366804ab17e6a3e8062bbf7429573adad5634c8b..a632c36cfb89526c6fdf0b9f1b46b8e84c4dd243 100644
--- a/app/controllers/snippets_controller.rb
+++ b/app/controllers/snippets_controller.rb
@@ -42,16 +42,19 @@ 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 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 79adc77c9e42dd1894363d492b3762b4acb92beb..107e6764ba2297136a44b9d831da9105f40e03e6 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 9bb456eee24755149b1e6025c2955a5525d3b4d3..25b5d7776411273af6aabdc6f7766113cb2b9441 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 14f5ba064ffdab0835c3851c7b85561b9c2d9568..40286dbf3bfa4c2c1646c1e342e02855b6ce7344 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 5f3ced49665ca1a702f23a7d1b5c6a37026754bc..9500faf2862e8e7379dd024e9203076d2f59f2aa 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 961605a1005caf35ea4f981d55e5325edaa68537..366b3572738c4a0ffbeb82633f9a9fafe3b92464 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)
- 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 78cbf94ec69c03962ed545cf5d5268f689fe1eb1..22e32b1325943ae796d1605c059aefed3c6c3668 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 0000000000000000000000000000000000000000..023e0824e85e4189a8bb77f928f737ac0df6bd8e
--- /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 024a7c19d3320386e356426205ff8e5457194559..3e65b7d31a35fd3fbb471d02bc5e6828156ab90a 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 a6bb36821c3d673db64f2e7f06a2985f36278412..358bca73aec25ab21637f8cfcadb9e7834ff3f61 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 0000000000000000000000000000000000000000..77c77dc675422bb946fb1e747d37a0737128b434
--- /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 1934b18c086a3542bcd1bfc136bb358a6580829f..09aa401e44ab55dd48283b2627d15e10527ba8a4 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 0000000000000000000000000000000000000000..eb56f03b3f4bf408ddbf7ac6ae659dde56c180fb
--- /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 0000000000000000000000000000000000000000..cb623ccab571edd4ff9f3720266b46168958b835
--- /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 0000000000000000000000000000000000000000..d70b5ef8fd560495adcfb631326ed79e9dab737b
--- /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 7b6fae866eb16a35939fcad72d4a5b3c7138767a..32692f19fcdd9a4d62ff2a4967af4bfba59a7116 100644
--- a/lib/api/helpers.rb
+++ b/lib/api/helpers.rb
@@ -215,6 +215,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 2b946bfd34957edf9c2ee44ebcaec49e0bf5eea8..6d30c5d81b12df8d183dbdb4d6a73c734b36e917 100644
--- a/lib/api/issues.rb
+++ b/lib/api/issues.rb
@@ -169,9 +169,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 dcc0c82ee27abb7524cd0c840ab33161c95c85fa..2a1cce73f3f834b8c37e5b13549c47abcb0a4b40 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 eb9ece49e7febc2e7e51985e58743e84c9919c63..ac03fbd2a3dc62f572eb4837731ac7131ca2801d 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 ba5b6fdbe52170782d05578c05b968cd89002d07..d0af09f0e1ec7482ac0d3c21b46628119cd777f6 100644
--- a/lib/api/v3/issues.rb
+++ b/lib/api/v3/issues.rb
@@ -149,9 +149,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
@@ -182,9 +180,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 9f95d4395fa285a6fe7e81693008184e9ca95054..e03e941d30b0ae498ac7fbe303882c0d43a75cc4 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 e576bf9ef79c7f529653286612bec3bea2f14309..7871b6a9e106ece4769a9adbd19f6b8d43578c53 100644
--- a/spec/controllers/projects/issues_controller_spec.rb
+++ b/spec/controllers/projects/issues_controller_spec.rb
@@ -152,6 +152,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,
@@ -384,7 +491,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)
@@ -395,7 +502,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 77ee10a1e151ee953f0ebeecf42a885af07c18c9..8bab094a79efe8c82fe91ac15401eb2c8675c654 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 f90c0d76ceba6626f5186848c3a8d0368da95826..5de3b9890ef3def4bde1d3035f3b7ea337c5b985 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 b6e5c95d18a4614c4bc0967ed23c9de841354db7..fd3b830757148c01bc61563dc075b301d929af0d 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 ece1b43567d3505451582cd001b16def60a2ee83..7a0bd5f9721fad92a8911a59c1d6bd48898f553e 100644
--- a/spec/requests/api/issues_spec.rb
+++ b/spec/requests/api/issues_spec.rb
@@ -1028,6 +1028,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 f56876bcf54223976f6c9944d5be7f387fb5e3dd..da9df56401b02b1841fe218b07c1200c6ef33618 100644
--- a/spec/requests/api/project_snippets_spec.rb
+++ b/spec/requests/api/project_snippets_spec.rb
@@ -78,43 +78,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'
@@ -138,6 +128,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 1ef92930b3c8e02947ed7284ed11a7961c339380..41def7cd1d44c8189246d4ffda81649982378e54 100644
--- a/spec/requests/api/snippets_spec.rb
+++ b/spec/requests/api/snippets_spec.rb
@@ -129,7 +129,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
@@ -141,16 +143,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
@@ -161,7 +167,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')
@@ -172,6 +178,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 33a127de98a7635be7fa1b01fd4df0cb621ff3fc..8e6732fe23e2d2a955466bebe285ffa91ce72d1c 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 3700477f0db3fc850b09e0f57a766e1ec7b6df51..957a3bf97ef51ccd55d4fcd3f1ed9a682241a029 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 e1feeed8a67e48f722ca6939c4cf034836292cdd..6045d00ff097ed6bf4c6522a49780b747e0021d2 100644
--- a/spec/services/issues/create_service_spec.rb
+++ b/spec/services/issues/create_service_spec.rb
@@ -230,16 +230,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 271c17dd8c039b08cdfe4be9bf52edf7255116c9..4ce3b95aa871e266c7fb03f043db129fd42267e2 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