Spam check and reCAPTCHA improvements28093-snippet-and-issue-spam-check-on-edit

7 files changed, 91 insertions, 45 deletions

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 961605a1005..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)
-      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