Merge branch 'remove-recaptcha-verification-param' into 'master'

Remove unnecessary recaptcha verification param and unused snippets verify views

See merge request gitlab-org/gitlab!51587

app/controllers/concerns/spammable_actions.rb

@@ -32,10 +32,6 @@ module SpammableActions
     elsif render_recaptcha?
       ensure_spam_config_loaded!
 
-      if params[:recaptcha_verification]
-        flash[:alert] = _('There was an error with the reCAPTCHA. Please solve the reCAPTCHA again.')
-      end
-
       respond_to do |format|
         format.html do
           render :verify
@@ -56,9 +52,9 @@ module SpammableActions
   def spammable_params
     default_params = { request: request }
 
-    recaptcha_check = params[:recaptcha_verification] &&
+    recaptcha_check = recaptcha_response &&
       ensure_spam_config_loaded! &&
-      verify_recaptcha
+      verify_recaptcha(response: recaptcha_response)
 
     return default_params unless recaptcha_check
 
@@ -66,6 +62,23 @@ module SpammableActions
       spam_log_id: params[:spam_log_id] }.merge(default_params)
   end
 
+  def recaptcha_response
+    # NOTE: This field name comes from `Recaptcha::ClientHelper#recaptcha_tags` in the recaptcha
+    # gem, which is called from the HAML `_recaptcha_form.html.haml` form.
+    #
+    # It is used in the `Recaptcha::Verify#verify_recaptcha` if the `response` option is not
+    # passed explicitly.
+    #
+    # Instead of relying on this behavior, we are extracting and passing it explicitly. This will
+    # make it consistent with the newer, modern reCAPTCHA verification process as it will be
+    # implemented via the GraphQL API and in Vue components via the native reCAPTCHA Javascript API,
+    # which requires that the recaptcha response param be obtained and passed explicitly.
+    #
+    # After this newer GraphQL/JS API process is fully supported by the backend, we can remove this
+    # (and other) HAML-specific support.
+    params['g-recaptcha-response']
+  end
+
   def spammable
     raise NotImplementedError, "#{self.class} does not implement #{__method__}"
   end

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

-= render 'layouts/recaptcha_verification', spammable: @snippet
-

app/views/shared/_recaptcha_form.html.haml

@@ -9,8 +9,11 @@
     - 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)
+    -# The reCAPTCHA response value will be returned in the 'g-recaptcha-response' field
     = recaptcha_tags script: script, callback: 'recaptchaDialogCallback' unless Rails.env.test?
+    -# Fake the 'g-recaptcha-response' field in the test environment, so that the feature spec
+    -# can get to the (mocked) SpamVerdictService check.
+    = hidden_field_tag('g-recaptcha-response', 'abc123') if Rails.env.test?
 
     -# Yields a block with given extra params.
     = yield

app/views/snippets/verify.html.haml

-= render 'layouts/recaptcha_verification', spammable: @snippet
-

spec/controllers/concerns/spammable_actions_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe SpammableActions do
+  controller(ActionController::Base) do
+    include SpammableActions
+
+    # #create is used to test spammable_params
+    # for testing purposes
+    def create
+      spam_params = spammable_params
+
+      # replace the actual request with a string in the JSON response, all we care is that it got set
+      spam_params[:request] = 'this is the request' if spam_params[:request]
+
+      # just return the params in the response so they can be verified in this fake controller spec.
+      # Normally, they are processed further by the controller action
+      render json: spam_params.to_json, status: :ok
+    end
+
+    # #update is used to test recaptcha_check_with_fallback
+    # for testing purposes
+    def update
+      should_redirect = params[:should_redirect] == 'true'
+
+      recaptcha_check_with_fallback(should_redirect) { render json: :ok }
+    end
+
+    private
+
+    def spammable_path
+      '/fake_spammable_path'
+    end
+  end
+
+  before do
+    # Ordinarily we would not stub a method on the class under test, but :ensure_spam_config_loaded!
+    # returns false in the test environment, and is also strong_memoized, so we need to stub it
+    allow(controller).to receive(:ensure_spam_config_loaded!) { true }
+  end
+
+  describe '#spammable_params' do
+    subject { post :create, format: :json, params: params }
+
+    shared_examples 'expects request param only' do
+      it do
+        subject
+
+        expect(response).to be_successful
+        expect(json_response).to eq({ 'request' => 'this is the request' })
+      end
+    end
+
+    shared_examples 'expects all spammable params' do
+      it do
+        subject
+
+        expect(response).to be_successful
+        expect(json_response['request']).to eq('this is the request')
+        expect(json_response['recaptcha_verified']).to eq(true)
+        expect(json_response['spam_log_id']).to eq('1')
+      end
+    end
+
+    let(:recaptcha_response) { nil }
+    let(:spam_log_id) { nil }
+
+    context 'when recaptcha response is not present' do
+      let(:params) do
+        {
+          spam_log_id: spam_log_id
+        }
+      end
+
+      it_behaves_like 'expects request param only'
+    end
+
+    context 'when recaptcha response is present' do
+      let(:recaptcha_response) { 'abd123' }
+      let(:params) do
+        {
+          'g-recaptcha-response': recaptcha_response,
+          spam_log_id: spam_log_id
+        }
+      end
+
+      context 'when verify_recaptcha returns falsey' do
+        before do
+          expect(controller).to receive(:verify_recaptcha).with(
+            {
+              response: recaptcha_response
+            }) { false }
+        end
+
+        it_behaves_like 'expects request param only'
+      end
+
+      context 'when verify_recaptcha returns truthy' do
+        let(:spam_log_id) { 1 }
+
+        before do
+          expect(controller).to receive(:verify_recaptcha).with(
+            {
+              response: recaptcha_response
+            }) { true }
+        end
+
+        it_behaves_like 'expects all spammable params'
+      end
+    end
+  end
+
+  describe '#recaptcha_check_with_fallback' do
+    shared_examples 'yields to block' do
+      it do
+        subject
+
+        expect(json_response).to eq({ json: 'ok' })
+      end
+    end
+
+    let(:format) { :html }
+
+    subject { post :update, format: format, params: params }
+
+    let(:spammable) { double(:spammable) }
+    let(:should_redirect) { nil }
+    let(:params) do
+      {
+        should_redirect: should_redirect
+      }
+    end
+
+    before do
+      routes.draw { get 'update' => 'anonymous#update' }
+      allow(controller).to receive(:spammable) { spammable }
+    end
+
+    context 'when should_redirect is true and spammable is valid' do
+      let(:should_redirect) { true }
+
+      before do
+        allow(spammable).to receive(:valid?) { true }
+      end
+
+      it 'redirects to spammable_path' do
+        expect(subject).to redirect_to('/fake_spammable_path')
+      end
+    end
+
+    context 'when should_redirect is false or spammable is not valid' do
+      before 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
+        before do
+          expect(controller).to receive(:render_recaptcha?) { true }
+        end
+
+        context 'when format is :html' do
+          it 'renders :verify' do
+            expect(controller).to receive(:render).with(:verify)
+
+            subject
+          end
+        end
+
+        context 'when format is :json' do
+          let(:format) { :json }
+          let(:recaptcha_html) { '<recaptcha-html/>' }
+
+          it 'renders json with recaptcha_html' do
+            expect(controller).to receive(:render_to_string).with(
+              {
+                partial: 'shared/recaptcha_form',
+                formats: :html,
+                locals: {
+                  spammable: spammable,
+                  script: false,
+                  has_submit: false
+                }
+              }
+            ) { recaptcha_html }
+
+            subject
+
+            expect(json_response).to eq({ 'recaptcha_html' => recaptcha_html })
+          end
+        end
+      end
+    end
+  end
+end

spec/controllers/projects/issues_controller_spec.rb

@@ -1019,7 +1019,7 @@ RSpec.describe Projects::IssuesController do
             def update_verified_issue
               update_issue(
                 issue_params: { title: spammy_title },
-                additional_params: { spam_log_id: spam_logs.last.id, recaptcha_verification: true })
+                additional_params: { spam_log_id: spam_logs.last.id, 'g-recaptcha-response': true })
             end
 
             it 'returns 200 status' do
@@ -1037,7 +1037,7 @@ RSpec.describe Projects::IssuesController do
             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(issue_params: { spam_log_id: spam_log.id, recaptcha_verification: true }) }
+              expect { update_issue(issue_params: { spam_log_id: spam_log.id, 'g-recaptcha-response': true }) }
                 .not_to change { SpamLog.last.recaptcha_verified }
             end
           end
@@ -1314,7 +1314,7 @@ RSpec.describe Projects::IssuesController do
           let!(:last_spam_log) { spam_logs.last }
 
           def post_verified_issue
-            post_new_issue({}, { spam_log_id: last_spam_log.id, recaptcha_verification: true } )
+            post_new_issue({}, { spam_log_id: last_spam_log.id, 'g-recaptcha-response': true } )
           end
 
           before do
@@ -1332,7 +1332,7 @@ RSpec.describe Projects::IssuesController do
           it 'does not mark spam log as recaptcha_verified when it does not belong to current_user' do
             spam_log = create(:spam_log)
 
-            expect { post_new_issue({}, { spam_log_id: spam_log.id, recaptcha_verification: true } ) }
+            expect { post_new_issue({}, { spam_log_id: spam_log.id, 'g-recaptcha-response': true } ) }
               .not_to change { last_spam_log.recaptcha_verified }
           end
         end