Spam check and reCAPTCHA improvements

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
-    {
-      recaptcha_verified: true,
-      spam_log_id: params[:spam_log_id]
-    }
+  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] }.merge(default_params)
   end
   def spammable

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)
-    @issue = Issues::CreateService.new(project, current_user, issue_params.merge(extra_params)).execute
+    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, 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

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

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

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
       []

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

app/services/create_snippet_service.rb

 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

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
-      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 params.present?
+      issuable.assign_attributes(params.merge(updated_by: current_user))
+
+      before_update(issuable)
+      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

app/services/issues/create_service.rb

 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

app/services/issues/update_service.rb

 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)

app/services/spam_check_service.rb

+# 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

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,

app/services/update_snippet_service.rb

 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

app/views/layouts/_recaptcha_verification.html.haml

+- 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'

app/views/projects/issues/verify.html.haml

-- page_title "Anti-spam verification"
-%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'
+- form = [@project.namespace.becomes(Namespace), @project, @issue]
+= 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])

app/views/projects/snippets/verify.html.haml

+- form = [@project.namespace.becomes(Namespace), @project, @snippet.becomes(Snippet)]
+
+= render 'layouts/recaptcha_verification', spammable: @snippet, form: form
+

app/views/snippets/verify.html.haml

+- form = [@snippet.becomes(Snippet)]
+
+= render 'layouts/recaptcha_verification', spammable: @snippet, form: form
+

changelogs/unreleased/28093-snippet-and-issue-spam-check-on-edit.yml

+---
+title: Spam check and reCAPTCHA improvements
+merge_request:
+author:

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

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