Merge branch 'caw-captcha-refactor-1' into 'master'

Refactor CAPTCHA internals

See merge request gitlab-org/gitlab!80986

app/controllers/concerns/issuable_actions.rb

@@ -43,18 +43,18 @@ module IssuableActions
     if updated_issuable.is_a?(Spammable)
       respond_to do |format|
         format.html do
-          # NOTE: This redirect is intentionally only performed in the case where the updated
-          # issuable is a spammable, and intentionally is not performed in the non-spammable case.
-          # This preserves the legacy behavior of this action.
           if updated_issuable.valid?
+            # NOTE: This redirect is intentionally only performed in the case where the valid updated
+            # issuable is a spammable, and intentionally is not performed below in the
+            # valid non-spammable case. This preserves the legacy behavior of this action.
             redirect_to spammable_path
           else
-            with_captcha_check_html_format { render :edit }
+            with_captcha_check_html_format(spammable: spammable) { render :edit }
           end
         end
 
         format.json do
-          with_captcha_check_json_format { render_entity_json }
+          with_captcha_check_json_format(spammable: spammable) { render_entity_json }
         end
       end
     else

app/controllers/concerns/spammable_actions/akismet_mark_as_spam_action.rb

@@ -2,7 +2,6 @@
 
 module SpammableActions::AkismetMarkAsSpamAction
   extend ActiveSupport::Concern
-  include SpammableActions::Attributes
 
   included do
     before_action :authorize_submit_spammable!, only: :mark_as_spam
@@ -22,7 +21,15 @@ module SpammableActions::AkismetMarkAsSpamAction
     access_denied! unless current_user.can_admin_all_resources?
   end
 
+  def spammable
+    # The class extending this module should define the #spammable method to return
+    # the Spammable model instance via: `alias_method :spammable , <:model_name>`
+    raise NotImplementedError, "#{self.class} should implement #{__method__}"
+  end
+
   def spammable_path
-    raise NotImplementedError, "#{self.class} does not implement #{__method__}"
+    # The class extending this module should define the #spammable_path method to return
+    # the route helper pointing to the action to show the Spammable instance
+    raise NotImplementedError, "#{self.class} should implement #{__method__}"
   end
 end

app/controllers/concerns/spammable_actions/attributes.rb

-# frozen_string_literal: true
-
-module SpammableActions
-  module Attributes
-    extend ActiveSupport::Concern
-
-    private
-
-    def spammable
-      raise NotImplementedError, "#{self.class} does not implement #{__method__}"
-    end
-  end
-end

app/controllers/concerns/spammable_actions/captcha_check/common.rb

 # frozen_string_literal: true
 
-module SpammableActions::CaptchaCheck
-  module Common
-    extend ActiveSupport::Concern
+module SpammableActions
+  module CaptchaCheck
+    module Common
+      extend ActiveSupport::Concern
 
-    private
+      private
 
-    def with_captcha_check_common(captcha_render_lambda:, &block)
-      # If the Spammable indicates that CAPTCHA is not necessary (either due to it not being flagged
-      # as spam, or if spam/captcha is disabled for some reason), then we will go ahead and
-      # yield to the block containing the action's original behavior, then return.
-      return yield unless spammable.render_recaptcha?
+      def with_captcha_check_common(spammable:, captcha_render_lambda:, &block)
+        # If the Spammable indicates that CAPTCHA is not necessary (either due to it not being flagged
+        # as spam, or if spam/captcha is disabled for some reason), then we will go ahead and
+        # yield to the block containing the action's original behavior, then return.
+        return yield unless spammable.render_recaptcha?
 
-      # If we got here, we need to render the CAPTCHA instead of yielding to action's original
-      # behavior. We will present a CAPTCHA to be solved by executing the lambda which was passed
-      # as the `captcha_render_lambda:` argument. This lambda contains either the HTML-specific or
-      # JSON-specific behavior to cause the CAPTCHA modal to be rendered.
-      Gitlab::Recaptcha.load_configurations!
-      captcha_render_lambda.call
+        # If we got here, we need to render the CAPTCHA instead of yielding to action's original
+        # behavior. We will present a CAPTCHA to be solved by executing the lambda which was passed
+        # as the `captcha_render_lambda:` argument. This lambda contains either the HTML-specific or
+        # JSON-specific behavior to cause the CAPTCHA modal to be rendered.
+        Gitlab::Recaptcha.load_configurations!
+        captcha_render_lambda.call
+      end
     end
   end
 end

app/controllers/concerns/spammable_actions/captcha_check/html_format_actions_support.rb

@@ -8,7 +8,6 @@
 # which supports JSON format should be used instead.
 module SpammableActions::CaptchaCheck::HtmlFormatActionsSupport
   extend ActiveSupport::Concern
-  include SpammableActions::Attributes
   include SpammableActions::CaptchaCheck::Common
 
   included do
@@ -17,9 +16,9 @@ module SpammableActions::CaptchaCheck::HtmlFormatActionsSupport
 
   private
 
-  def with_captcha_check_html_format(&block)
+  def with_captcha_check_html_format(spammable:, &block)
     captcha_render_lambda = -> { render :captcha_check }
-    with_captcha_check_common(captcha_render_lambda: captcha_render_lambda, &block)
+    with_captcha_check_common(spammable: spammable, captcha_render_lambda: captcha_render_lambda, &block)
   end
 
   # Convert spam/CAPTCHA values from form field params to headers, because all spam-related services

app/controllers/concerns/spammable_actions/captcha_check/json_format_actions_support.rb

@@ -9,17 +9,16 @@
 # supports HTML format should be used instead.
 module SpammableActions::CaptchaCheck::JsonFormatActionsSupport
   extend ActiveSupport::Concern
-  include SpammableActions::Attributes
   include SpammableActions::CaptchaCheck::Common
   include Spam::Concerns::HasSpamActionResponseFields
 
   private
 
-  def with_captcha_check_json_format(&block)
+  def with_captcha_check_json_format(spammable:, &block)
     # NOTE: "409 - Conflict" seems to be the most appropriate HTTP status code for a response
     # which requires a CAPTCHA to be solved in order for the request to be resubmitted.
     # https://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html#sec10.4.10
     captcha_render_lambda = -> { render json: spam_action_response_fields(spammable), status: :conflict }
-    with_captcha_check_common(captcha_render_lambda: captcha_render_lambda, &block)
+    with_captcha_check_common(spammable: spammable, captcha_render_lambda: captcha_render_lambda, &block)
   end
 end

app/controllers/projects/issues_controller.rb

@@ -150,7 +150,7 @@ class Projects::IssuesController < Projects::ApplicationController
       redirect_to project_issue_path(@project, @issue)
     else
       # NOTE: this CAPTCHA support method is indirectly included via IssuableActions
-      with_captcha_check_html_format { render :new }
+      with_captcha_check_html_format(spammable: spammable) { render :new }
     end
   end
 

app/graphql/mutations/concerns/mutations/spam_protection.rb

@@ -16,30 +16,16 @@ module Mutations
 
     private
 
-    def spam_action_response(object)
-      fields = spam_action_response_fields(object)
-
-      # If the SpamActionService detected something as spam,
-      # this is non-recoverable and the needs_captcha_response
-      # should not be considered
-      kind = if fields[:spam]
-               :spam
-             elsif fields[:needs_captcha_response]
-               :needs_captcha_response
-             end
-
-      [kind, fields]
-    end
-
     def check_spam_action_response!(object)
-      kind, fields = spam_action_response(object)
+      fields = spam_action_response_fields(object)
 
-      case kind
-      when :needs_captcha_response
+      if fields[:spam]
+        # If the SpamActionService detected something as spam, this is non-recoverable and the
+        # needs_captcha_response and other CAPTCHA-related fields should not be returned
+        raise SpamDisallowedError.new(SPAM_DISALLOWED_MESSAGE, extensions: { spam: true })
+      elsif fields[:needs_captcha_response]
         fields.delete :spam
         raise NeedsCaptchaResponseError.new(NEEDS_CAPTCHA_RESPONSE_MESSAGE, extensions: fields)
-      when :spam
-        raise SpamDisallowedError.new(SPAM_DISALLOWED_MESSAGE, extensions: { spam: true })
       else
         nil
       end

app/models/concerns/spammable.rb

@@ -12,7 +12,7 @@ module Spammable
   included do
     has_one :user_agent_detail, as: :subject, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
 
-    attr_accessor :spam
+    attr_writer :spam
     attr_accessor :needs_recaptcha
     attr_accessor :spam_log
 
@@ -29,6 +29,10 @@ module Spammable
     delegate :ip_address, :user_agent, to: :user_agent_detail, allow_nil: true
   end
 
+  def spam
+    !!@spam # rubocop:disable Gitlab/ModuleWithInstanceVariables
+  end
+
   def submittable_as_spam_by?(current_user)
     current_user && current_user.admin? && submittable_as_spam?
   end

lib/spam/concerns/has_spam_action_response_fields.rb

@@ -7,7 +7,7 @@ module Spam
     module HasSpamActionResponseFields
       extend ActiveSupport::Concern
 
-      # spam_action_response_fields(spammable)    -> hash
+      # spam_action_response_fields(spammable) -> hash
       #
       # Takes a Spammable as an argument and returns response fields necessary to display a CAPTCHA on
       # the client.

spec/controllers/concerns/spammable_actions/akismet_mark_as_spam_action_spec.rb

@@ -7,12 +7,6 @@ RSpec.describe SpammableActions::AkismetMarkAsSpamAction do
 
   controller(ActionController::Base) do
     include SpammableActions::AkismetMarkAsSpamAction
-
-    private
-
-    def spammable_path
-      '/fake_spammable_path'
-    end
   end
 
   let(:spammable_type) { 'SpammableType' }
@@ -22,7 +16,6 @@ RSpec.describe SpammableActions::AkismetMarkAsSpamAction do
   before do
     allow(Gitlab::Recaptcha).to receive(:load_configurations!) { true }
     routes.draw { get 'mark_as_spam' => 'anonymous#mark_as_spam' }
-    allow(controller).to receive(:spammable) { spammable }
     allow(controller).to receive(:current_user) { double(:current_user, admin?: admin) }
     allow(controller).to receive(:current_user).and_return(current_user)
   end
@@ -31,6 +24,9 @@ RSpec.describe SpammableActions::AkismetMarkAsSpamAction do
     subject { post :mark_as_spam }
 
     before do
+      allow(controller).to receive(:spammable) { spammable }
+      allow(controller).to receive(:spammable_path) { '/fake_spammable_path' }
+
       expect_next(Spam::AkismetMarkAsSpamService, target: spammable)
         .to receive(:execute).and_return(execute_result)
     end
@@ -68,4 +64,16 @@ RSpec.describe SpammableActions::AkismetMarkAsSpamAction do
       end
     end
   end
+
+  describe '#spammable' do
+    it 'raises when unimplemented' do
+      expect { controller.send(:spammable) }.to raise_error(NotImplementedError)
+    end
+  end
+
+  describe '#spammable_path' do
+    it 'raises when unimplemented' do
+      expect { controller.send(:spammable_path) }.to raise_error(NotImplementedError)
+    end
+  end
 end

spec/controllers/concerns/spammable_actions/captcha_check/html_format_actions_support_spec.rb

@@ -7,7 +7,7 @@ RSpec.describe SpammableActions::CaptchaCheck::HtmlFormatActionsSupport do
     include SpammableActions::CaptchaCheck::HtmlFormatActionsSupport
 
     def create
-      with_captcha_check_html_format { render :some_rendered_view }
+      with_captcha_check_html_format(spammable: spammable) { render :some_rendered_view }
     end
   end
 

spec/controllers/concerns/spammable_actions/captcha_check/json_format_actions_support_spec.rb

@@ -7,7 +7,7 @@ RSpec.describe SpammableActions::CaptchaCheck::JsonFormatActionsSupport do
     include SpammableActions::CaptchaCheck::JsonFormatActionsSupport
 
     def some_action
-      with_captcha_check_json_format { render :some_rendered_view }
+      with_captcha_check_json_format(spammable: spammable) { render :some_rendered_view }
     end
   end