Merge branch 'refactor-spammable-recaptcha-mutations' into 'master'

Refactor spam/recaptcha mutations/services for better captcha support

See merge request gitlab-org/gitlab!51537

app/controllers/concerns/spammable_actions.rb

@@ -17,15 +17,11 @@ module SpammableActions
 
   private
 
-  def ensure_spam_config_loaded!
-    Gitlab::Recaptcha.load_configurations!
-  end
-
   def recaptcha_check_with_fallback(should_redirect = true, &fallback)
     if should_redirect && spammable.valid?
       redirect_to spammable_path
-    elsif render_recaptcha?
-      ensure_spam_config_loaded!
+    elsif spammable.render_recaptcha?
+      Gitlab::Recaptcha.load_configurations!
 
       respond_to do |format|
         format.html do
@@ -82,11 +78,4 @@ module SpammableActions
   def authorize_submit_spammable!
     access_denied! unless current_user.admin?
   end
-
-  def render_recaptcha?
-    return false if spammable.errors.count > 1 # re-render "new" template in case there are other errors
-    return false unless Gitlab::Recaptcha.enabled?
-
-    spammable.needs_recaptcha?
-  end
 end

app/graphql/mutations/concerns/mutations/can_mutate_spammable.rb

+# frozen_string_literal: true
+
+module Mutations
+  # This concern can be mixed into a mutation to provide support for spam checking,
+  # and optionally support the workflow to allow clients to display and solve recaptchas.
+  module CanMutateSpammable
+    extend ActiveSupport::Concern
+
+    included do
+      field :spam,
+            GraphQL::BOOLEAN_TYPE,
+            null: true,
+            description: 'Indicates whether the operation returns a record detected as spam.'
+    end
+
+    # additional_spam_params    -> hash
+    #
+    # Used from a spammable mutation's #resolve method to generate
+    # the required additional spam/recaptcha params which must be merged into the params
+    # passed to the constructor of a service, where they can then be used in the service
+    # to perform spam checking via SpamActionService.
+    #
+    # Also accesses the #context of the mutation's Resolver superclass to obtain the request.
+    #
+    # Example:
+    #
+    # existing_args.merge!(additional_spam_params)
+    def additional_spam_params
+      {
+        api: true,
+        request: context[:request]
+      }
+    end
+
+    # with_spam_action_fields(spammable) { {other_fields: true} }    -> hash
+    #
+    # Takes a Spammable and a block as arguments.
+    #
+    # The block passed should be a hash, which the spam action fields will be merged into.
+    def with_spam_action_fields(spammable)
+      spam_action_fields = {
+        spam: spammable.spam?,
+        # NOTE: These fields are intentionally named with 'captcha' instead of 'recaptcha', so
+        # that they can be applied to future alternative captcha implementations other than
+        # reCAPTCHA (such as FriendlyCaptcha) without having to change the response field name
+        # in the API.
+        needs_captcha_response: spammable.render_recaptcha?,
+        spam_log_id: spammable.spam_log&.id,
+        captcha_site_key: Gitlab::CurrentSettings.recaptcha_site_key
+      }
+
+      yield.merge(spam_action_fields)
+    end
+  end
+end

app/graphql/mutations/concerns/mutations/spammable_mutation_fields.rb

-# frozen_string_literal: true
-
-module Mutations
-  module SpammableMutationFields
-    extend ActiveSupport::Concern
-
-    included do
-      field :spam,
-            GraphQL::BOOLEAN_TYPE,
-            null: true,
-            description: 'Indicates whether the operation returns a record detected as spam.'
-    end
-
-    def with_spam_params(&block)
-      request = Feature.enabled?(:snippet_spam) ? context[:request] : nil
-
-      yield.merge({ api: true, request: request })
-    end
-
-    def with_spam_fields(spammable, &block)
-      { spam: spammable.spam? }.merge!(yield)
-    end
-  end
-end

app/graphql/mutations/snippets/create.rb

@@ -3,7 +3,8 @@
 module Mutations
   module Snippets
     class Create < BaseMutation
-      include SpammableMutationFields
+      include ServiceCompatibility
+      include CanMutateSpammable
 
       authorize :create_snippet
 
@@ -45,18 +46,17 @@ module Mutations
           authorize!(:global)
         end
 
-        service_response = ::Snippets::CreateService.new(project,
-                                                         current_user,
-                                                         create_params(args)).execute
+        process_args_for_params!(args)
 
-        snippet = service_response.payload[:snippet]
+        service_response = ::Snippets::CreateService.new(project, current_user, args).execute
 
         # Only when the user is not an api user and the operation was successful
         if !api_user? && service_response.success?
           ::Gitlab::UsageDataCounters::EditorUniqueCounter.track_snippet_editor_edit_action(author: current_user)
         end
 
-        with_spam_fields(snippet) do
+        snippet = service_response.payload[:snippet]
+        with_spam_action_fields(snippet) do
           {
             snippet: service_response.success? ? snippet : nil,
             errors: errors_on_object(snippet)
@@ -70,18 +70,25 @@ module Mutations
         Project.find_by_full_path(full_path)
       end
 
-      def create_params(args)
-        with_spam_params do
-          args.tap do |create_args|
-            # We need to rename `blob_actions` into `snippet_actions` because
-            # it's the expected key param
-            create_args[:snippet_actions] = create_args.delete(:blob_actions)&.map(&:to_h)
+      # process_args_for_params!(args)    -> nil
+      #
+      # Modifies/adds/deletes mutation resolve args as necessary to be passed as params to service layer.
+      def process_args_for_params!(args)
+        convert_blob_actions_to_snippet_actions!(args)
 
         # We need to rename `uploaded_files` into `files` because
         # it's the expected key param
-            create_args[:files] = create_args.delete(:uploaded_files)
-          end
+        args[:files] = args.delete(:uploaded_files)
+
+        if Feature.enabled?(:snippet_spam)
+          args.merge!(additional_spam_params)
+        else
+          args[:disable_spam_action_service] = true
         end
+
+        # Return nil to make it explicit that this method is mutating the args parameter, and that
+        # the return value is not relevant and is not to be used.
+        nil
       end
     end
   end

app/graphql/mutations/snippets/service_compatibility.rb

+# frozen_string_literal: true
+
+module Mutations
+  module Snippets
+    # Translates graphql mutation field params to be compatible with those expected by the service layer
+    module ServiceCompatibility
+      extend ActiveSupport::Concern
+
+      # convert_blob_actions_to_snippet_actions!(args)    -> nil
+      #
+      # Converts the blob_actions mutation argument into the
+      # snippet_actions hash which the service layer expects
+      def convert_blob_actions_to_snippet_actions!(args)
+        # We need to rename `blob_actions` into `snippet_actions` because
+        # it's the expected key param
+        args[:snippet_actions] = args.delete(:blob_actions)&.map(&:to_h)
+
+        # Return nil to make it explicit that this method is mutating the args parameter
+        nil
+      end
+    end
+  end
+end

app/graphql/mutations/snippets/update.rb

@@ -3,7 +3,8 @@
 module Mutations
   module Snippets
     class Update < Base
-      include SpammableMutationFields
+      include ServiceCompatibility
+      include CanMutateSpammable
 
       graphql_name 'UpdateSnippet'
 
@@ -30,19 +31,23 @@ module Mutations
       def resolve(id:, **args)
         snippet = authorized_find!(id: id)
 
-        result = ::Snippets::UpdateService.new(snippet.project,
-                                               current_user,
-                                               update_params(args)).execute(snippet)
-        snippet = result.payload[:snippet]
+        process_args_for_params!(args)
+
+        service_response = ::Snippets::UpdateService.new(snippet.project, current_user, args).execute(snippet)
+
+        # TODO: DRY this up - From here down, this is all duplicated with Mutations::Snippets::Create#resolve, except for
+        #    `snippet.reset`, which is required in order to return the object in its non-dirty, unmodified, database state
+        #    See issue here: https://gitlab.com/gitlab-org/gitlab/-/issues/300250
 
         # Only when the user is not an api user and the operation was successful
-        if !api_user? && result.success?
+        if !api_user? && service_response.success?
           ::Gitlab::UsageDataCounters::EditorUniqueCounter.track_snippet_editor_edit_action(author: current_user)
         end
 
-        with_spam_fields(snippet) do
+        snippet = service_response.payload[:snippet]
+        with_spam_action_fields(snippet) do
           {
-            snippet: result.success? ? snippet : snippet.reset,
+            snippet: service_response.success? ? snippet : snippet.reset,
             errors: errors_on_object(snippet)
           }
         end
@@ -50,18 +55,25 @@ module Mutations
 
       private
 
-      def ability_name
-        'update'
-      end
+      # process_args_for_params!(args)    -> nil
+      #
+      # Modifies/adds/deletes mutation resolve args as necessary to be passed as params to service layer.
+      def process_args_for_params!(args)
+        convert_blob_actions_to_snippet_actions!(args)
 
-      def update_params(args)
-        with_spam_params do
-          args.tap do |update_args|
-            # We need to rename `blob_actions` into `snippet_actions` because
-            # it's the expected key param
-            update_args[:snippet_actions] = update_args.delete(:blob_actions)&.map(&:to_h)
+        if Feature.enabled?(:snippet_spam)
+          args.merge!(additional_spam_params)
+        else
+          args[:disable_spam_action_service] = true
         end
+
+        # Return nil to make it explicit that this method is mutating the args parameter, and that
+        # the return value is not relevant and is not to be used.
+        nil
       end
+
+      def ability_name
+        'update'
       end
     end
   end

app/models/concerns/spammable.rb

@@ -45,6 +45,17 @@ module Spammable
     self.needs_recaptcha = true
   end
 
+  ##
+  # Indicates if a recaptcha should be rendered before allowing this model to be saved.
+  #
+  def render_recaptcha?
+    return false unless Gitlab::Recaptcha.enabled?
+
+    return false if self.errors.count > 1 # captcha should not be rendered if are still other errors
+
+    self.needs_recaptcha?
+  end
+
   def spam!
     self.spam = true
   end

app/services/snippets/create_service.rb

@@ -3,6 +3,8 @@
 module Snippets
   class CreateService < Snippets::BaseService
     def execute
+      # NOTE: disable_spam_action_service can be removed when the ':snippet_spam' feature flag is removed.
+      disable_spam_action_service = params.delete(:disable_spam_action_service) == true
       @request = params.delete(:request)
       @spam_params = Spam::SpamActionService.filter_spam_params!(params)
 
@@ -16,12 +18,14 @@ module Snippets
 
       @snippet.author = current_user
 
+      unless disable_spam_action_service
         Spam::SpamActionService.new(
           spammable: @snippet,
           request: request,
           user: current_user,
           action: :create
         ).execute(spam_params: spam_params)
+      end
 
       if save_and_commit
         UserAgentDetailService.new(@snippet, request).create

app/services/snippets/update_service.rb

@@ -7,6 +7,8 @@ module Snippets
     UpdateError = Class.new(StandardError)
 
     def execute(snippet)
+      # NOTE: disable_spam_action_service can be removed when the ':snippet_spam' feature flag is removed.
+      disable_spam_action_service = params.delete(:disable_spam_action_service) == true
       @request = params.delete(:request)
       @spam_params = Spam::SpamActionService.filter_spam_params!(params)
 
@@ -17,17 +19,20 @@ module Snippets
       end
 
       update_snippet_attributes(snippet)
+
+      unless disable_spam_action_service
         Spam::SpamActionService.new(
           spammable: snippet,
           request: request,
           user: current_user,
           action: :update
         ).execute(spam_params: spam_params)
+      end
 
       if save_and_commit(snippet)
         Gitlab::UsageDataCounters::SnippetCounter.count(:update)
 
-        ServiceResponse.success(payload: { snippet: snippet } )
+        ServiceResponse.success(payload: { snippet: snippet })
       else
         snippet_error_response(snippet, 400)
       end

spec/controllers/concerns/spammable_actions_spec.rb

@@ -68,12 +68,9 @@ RSpec.describe SpammableActions do
         allow(spammable).to receive(:valid?) { false }
       end
 
-      # NOTE: Not adding coverage of details of render_recaptcha?, the plan is to refactor it out
-      # of this module anyway as part of adding support for the GraphQL reCAPTCHA flow.
-
-      context 'when render_recaptcha? is true' do
+      context 'when spammable.render_recaptcha? is true' do
         before do
-          expect(controller).to receive(:render_recaptcha?) { true }
+          expect(spammable).to receive(:render_recaptcha?) { true }
         end
 
         context 'when format is :html' do

spec/graphql/mutations/concerns/mutations/can_mutate_spammable_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Mutations::CanMutateSpammable do
+  let(:mutation_class) do
+    Class.new(Mutations::BaseMutation) do
+      include Mutations::CanMutateSpammable
+    end
+  end
+
+  let(:request) { double(:request) }
+  let(:query) { double(:query, schema: GitlabSchema) }
+  let(:context) { GraphQL::Query::Context.new(query: query, object: nil, values: { request: request }) }
+
+  subject(:mutation) { mutation_class.new(object: nil, context: context, field: nil) }
+
+  describe '#additional_spam_params' do
+    it 'returns additional spam-related params' do
+      expect(subject.additional_spam_params).to eq({ api: true, request: request })
+    end
+  end
+
+  describe '#with_spam_action_fields' do
+    let(:spam_log) { double(:spam_log, id: 1) }
+    let(:spammable) { double(:spammable, spam?: true, render_recaptcha?: true, spam_log: spam_log) }
+
+    before do
+      allow(Gitlab::CurrentSettings).to receive(:recaptcha_site_key) { 'abc123' }
+    end
+
+    it 'merges in spam action fields from spammable' do
+      result = subject.with_spam_action_fields(spammable) do
+        { other_field: true }
+      end
+      expect(result)
+        .to eq({
+                 spam: true,
+                 needs_captcha_response: true,
+                 spam_log_id: 1,
+                 captcha_site_key: 'abc123',
+                 other_field: true
+               })
+    end
+  end
+end

spec/models/concerns/spammable_spec.rb

@@ -120,6 +120,61 @@ RSpec.describe Spammable do
         end
       end
 
+      describe '#render_recaptcha?' do
+        before do
+          allow(Gitlab::Recaptcha).to receive(:enabled?) { recaptcha_enabled }
+        end
+
+        context 'when recaptcha is not enabled' do
+          let(:recaptcha_enabled) { false }
+
+          it 'returns false' do
+            expect(issue.render_recaptcha?).to eq(false)
+          end
+        end
+
+        context 'when recaptcha is enabled' do
+          let(:recaptcha_enabled) { true }
+
+          context 'when there are two or more errors' do
+            before do
+              issue.errors.add(:base, 'a spam error')
+              issue.errors.add(:base, 'some other error')
+            end
+
+            it 'returns false' do
+              expect(issue.render_recaptcha?).to eq(false)
+            end
+          end
+
+          context 'when there are less than two errors' do
+            before do
+              issue.errors.add(:base, 'a spam error')
+            end
+
+            context 'when spammable does not need recaptcha' do
+              before do
+                issue.needs_recaptcha = false
+              end
+
+              it 'returns false' do
+                expect(issue.render_recaptcha?).to eq(false)
+              end
+            end
+
+            context 'when spammable needs recaptcha' do
+              before do
+                issue.needs_recaptcha!
+              end
+
+              it 'returns false' do
+                expect(issue.render_recaptcha?).to eq(true)
+              end
+            end
+          end
+        end
+      end
+
       describe '#clear_spam_flags!' do
         it 'clears spam and recaptcha flags' do
           issue.spam = true

spec/requests/api/graphql/mutations/snippets/create_spec.rb

@@ -78,6 +78,21 @@ RSpec.describe 'Creating a Snippet' do
       end
 
       it_behaves_like 'spam flag is present'
+
+      context 'when snippet_spam flag is disabled' do
+        before do
+          stub_feature_flags(snippet_spam: false)
+        end
+
+        it 'passes disable_spam_action_service param to service' do
+          expect(::Snippets::CreateService)
+            .to receive(:new)
+                  .with(anything, anything, hash_including(disable_spam_action_service: true))
+                  .and_call_original
+
+          subject
+        end
+      end
     end
 
     shared_examples 'creates snippet' do

spec/requests/api/graphql/mutations/snippets/update_spec.rb

@@ -87,6 +87,21 @@ RSpec.describe 'Updating a Snippet' do
 
       it_behaves_like 'spam flag is present'
 
+      context 'when snippet_spam flag is disabled' do
+        before do
+          stub_feature_flags(snippet_spam: false)
+        end
+
+        it 'passes disable_spam_action_service param to service' do
+          expect(::Snippets::UpdateService)
+            .to receive(:new)
+                  .with(anything, anything, hash_including(disable_spam_action_service: true))
+                  .and_call_original
+
+          subject
+        end
+      end
+
       context 'when there are ActiveRecord validation errors' do
         let(:updated_title) { '' }
 

spec/support/shared_examples/graphql/mutations/spammable_mutation_fields_examples.rb → spec/support/shared_examples/graphql/mutations/can_mutate_spammable_examples.rb

@@ -33,18 +33,4 @@ RSpec.shared_examples 'can raise spam flag' do
       expect(mutation_response['errors']).to include("Your snippet has been recognized as spam and has been discarded.")
     end
   end
-
-  context 'when :snippet_spam flag is disabled' do
-    before do
-      stub_feature_flags(snippet_spam: false)
-    end
-
-    it 'request parameter is not passed to the service' do
-      expect(service).to receive(:new)
-        .with(anything, anything, hash_not_including(request: instance_of(ActionDispatch::Request)))
-        .and_call_original
-
-      subject
-    end
-  end
 end

spec/support/shared_examples/services/snippets_shared_examples.rb

@@ -5,13 +5,15 @@ RSpec.shared_examples 'checking spam' do
   let(:api) { true }
   let(:captcha_response) { 'abc123' }
   let(:spam_log_id) { 1 }
+  let(:disable_spam_action_service) { false }
 
   let(:extra_opts) do
     {
       request: request,
       api: api,
       captcha_response: captcha_response,
-      spam_log_id: spam_log_id
+      spam_log_id: spam_log_id,
+      disable_spam_action_service: disable_spam_action_service
     }
   end
 
@@ -41,6 +43,16 @@ RSpec.shared_examples 'checking spam' do
 
     subject
   end
+
+  context 'when spam action service is disabled' do
+    let(:disable_spam_action_service) { true }
+
+    it 'request parameter is not passed to the service' do
+      expect(Spam::SpamActionService).not_to receive(:new)
+
+      subject
+    end
+  end
 end
 
 shared_examples 'invalid params error response' do