Refactor reCaptcha code and bypass in report abuse for trustworthy users only (#10502)

* Bypass recaptcha for authenticated users abuse reports

* Moves bypass_recaptcha? to helper method to have one definition of the criteria for this

* User must be 1 month old at least in order to bypass_recaptcha?

* Fixes test

* Better spec message

* Bypasses auditable users

* Adds better test coverage over recaptcha bypass cases

* include Helper removed from FeedbackMessagesController

* Adds some inline comments for clarity and banned check

app/controllers/feedback_messages_controller.rb

@@ -9,7 +9,8 @@ class FeedbackMessagesController < ApplicationController
     params = feedback_message_params.merge(reporter_id: current_user&.id)
     @feedback_message = FeedbackMessage.new(params)
 
-    if (recaptcha_disabled? || recaptcha_verified?) && @feedback_message.save
+    recaptcha_disabled = ReCaptcha.call(current_user).disabled?
+    if (recaptcha_disabled || recaptcha_verified?) && @feedback_message.save
       Slack::Messengers::Feedback.call(
         user: current_user,
         type: feedback_message_params[:feedback_type],
@@ -30,10 +31,6 @@ class FeedbackMessagesController < ApplicationController
 
   private
 
-  def recaptcha_disabled?
-    SiteConfig.recaptcha_site_key.blank? && SiteConfig.recaptcha_secret_key.blank?
-  end
-
   def recaptcha_verified?
     recaptcha_params = { secret_key: SiteConfig.recaptcha_secret_key }
     params["g-recaptcha-response"] && verify_recaptcha(recaptcha_params)

app/controllers/registrations_controller.rb

@@ -17,7 +17,7 @@ class RegistrationsController < Devise::RegistrationsController
     not_authorized if SiteConfig.waiting_on_first_user && ENV["FOREM_OWNER_SECRET"].present? &&
       ENV["FOREM_OWNER_SECRET"] != params[:user][:forem_owner_secret]
 
-    if recaptcha_disabled? || recaptcha_verified?
+    if ReCaptcha.for_registration_disabled? || recaptcha_verified?
       build_resource(sign_up_params)
       resource.saw_onboarding = false
       resource.registered = true
@@ -49,11 +49,6 @@ class RegistrationsController < Devise::RegistrationsController
     Users::CreateMascotAccount.call
   end
 
-  def recaptcha_disabled?
-    (SiteConfig.recaptcha_site_key.blank? && SiteConfig.recaptcha_secret_key.blank?) ||
-      !SiteConfig.require_captcha_for_email_password_registration
-  end
-
   def recaptcha_verified?
     recaptcha_params = { secret_key: SiteConfig.recaptcha_secret_key }
     params["g-recaptcha-response"] && verify_recaptcha(recaptcha_params)

app/helpers/authentication_helper.rb

@@ -23,12 +23,6 @@ module AuthenticationHelper
     Authentication::Providers.available.map(&:to_s)
   end
 
-  def recaptcha_configured_and_enabled?
-    SiteConfig.recaptcha_secret_key.present? &&
-      SiteConfig.recaptcha_site_key.present? &&
-      SiteConfig.require_captcha_for_email_password_registration
-  end
-
   def forem_creator_flow_enabled?
     FeatureFlag.enabled?(:creator_onboarding) && waiting_on_first_user?
   end

app/services/re_captcha.rb

@@ -0,0 +1,52 @@
+# This service encapsulates some logic related to reCAPTCHA.
+#
+# The `enabled?` and `disabled?` methods will tell if the reCAPTCHA is
+# necessary (enabled) or if it's not necessary (disabled) in the current
+# context. This is determined by the `current_user`, or the lack thereof.
+#
+# Example: ReCaptcha.call(current_user).enabled? => true/false
+class ReCaptcha
+  include Devise::Controllers::Helpers
+
+  def self.call(current_user = nil)
+    new(current_user).call
+  end
+
+  def initialize(current_user)
+    @current_user = current_user
+  end
+
+  def call
+    self
+  end
+
+  def disabled?
+    !enabled?
+  end
+
+  def enabled?
+    # recaptcha will not be enabled if site key and secret key aren't set
+    return false unless ReCaptcha.keys_configured?
+    # recaptcha will always be enabled when not logged in
+    return true if @current_user.nil?
+    # recaptcha will not be enabled for trusted/admin/tag mod users
+    return false if @current_user.auditable?
+    # recaptcha will be enabled if the user has been banned
+    return true if @current_user.banned
+
+    # recaptcha will be enabled if the user has a vomit or is too recent
+    @current_user.vomitted_on? || @current_user.created_at > 1.month.ago
+  end
+
+  def self.keys_configured?
+    SiteConfig.recaptcha_site_key.present? && SiteConfig.recaptcha_secret_key.present?
+  end
+
+  def self.for_registration_disabled?
+    !ReCaptcha.for_registration_enabled?
+  end
+
+  def self.for_registration_enabled?
+    ReCaptcha.keys_configured? && SiteConfig.require_captcha_for_email_password_registration
+  end
+end

app/views/feedback_messages/_form.html.erb

@@ -67,11 +67,11 @@
     <%= f.text_area :message, class: "crayons-textfield", placeholder: "...", value: @previous_message, required: true %>
   </div>
 
-<% if SiteConfig.recaptcha_secret_key.present? && SiteConfig.recaptcha_site_key.present? %>
-  <div class="recaptcha-tag-container">
-    <%= recaptcha_tags site_key: SiteConfig.recaptcha_site_key %>
-  </div>
-<% end %>
+  <% if ReCaptcha.call(current_user).enabled? %>
+    <div class="recaptcha-tag-container">
+      <%= recaptcha_tags site_key: SiteConfig.recaptcha_site_key %>
+    </div>
+  <% end %>
 
   <div><button type="submit" name="commit" class="crayons-btn">Send Feedback</button></div>
 <% end %>

app/views/shared/authentication/_email_registration_form.html.erb

@@ -87,7 +87,7 @@
       <% end %>
     <% end %>
 
-    <% if recaptcha_configured_and_enabled? %>
+    <% if ReCaptcha.for_registration_enabled? %>
       <div class="recaptcha-tag-container mt-2">
         <%= recaptcha_tags site_key: SiteConfig.recaptcha_site_key %>
       </div>

spec/helpers/authentication_helper_spec.rb

@@ -38,33 +38,6 @@ RSpec.describe AuthenticationHelper, type: :helper do
     end
   end
 
-  describe "#recaptcha_configured_and_enabled?" do
-    context "when recaptcha is enabled" do
-      before do
-        allow(SiteConfig).to receive(:require_captcha_for_email_password_registration).and_return(true)
-      end
-
-      it "returns true if both site & secret keys present" do
-        allow(SiteConfig).to receive(:recaptcha_secret_key).and_return("someSecretKey")
-        allow(SiteConfig).to receive(:recaptcha_site_key).and_return("someSiteKey")
-
-        expect(recaptcha_configured_and_enabled?).to be(true)
-      end
-
-      it "returns false if site or secret key missing" do
-        allow(SiteConfig).to receive(:recaptcha_site_key).and_return("")
-
-        expect(recaptcha_configured_and_enabled?).to be(false)
-      end
-    end
-
-    it "returns false if recaptcha disabled for email signup" do
-      allow(SiteConfig).to receive(:require_captcha_for_email_password_registration).and_return(false)
-
-      expect(recaptcha_configured_and_enabled?).to be(false)
-    end
-  end
-
   describe "tooltip classes, attributes and content" do
     context "when invite-only-mode enabled and no enabled registration options" do
       before do

spec/requests/feedback_messages_spec.rb

@@ -21,7 +21,7 @@ RSpec.describe "feedback_messages", type: :request do
 
     headers = { "HTTP_FASTLY_CLIENT_IP" => "5.6.7.8" }
 
-    context "with valid params" do
+    context "with valid params and recaptcha passed" do
       before do
         mock_recaptcha_verification
       end
@@ -44,10 +44,10 @@ RSpec.describe "feedback_messages", type: :request do
       end
     end
 
-    context "with no recaptcha keys set" do
+    context "with valid params and recaptcha not configured" do
       before do
-        allow(SiteConfig).to receive(:recaptcha_secret_key).and_return("")
-        allow(SiteConfig).to receive(:recaptcha_site_key).and_return("")
+        allow(SiteConfig).to receive(:recaptcha_secret_key).and_return(nil)
+        allow(SiteConfig).to receive(:recaptcha_site_key).and_return(nil)
       end
 
       it "does not show the recaptcha tag" do
@@ -62,19 +62,6 @@ RSpec.describe "feedback_messages", type: :request do
       end
     end
 
-    context "with invalid recaptcha" do
-      it "rerenders page" do
-        post "/feedback_messages", params: valid_abuse_report_params, headers: headers
-        expect(response.body).to include("Make sure the forms are filled")
-      end
-
-      it "queues a slack message to be sent" do
-        sidekiq_assert_no_enqueued_jobs(only: Slack::Messengers::Worker) do
-          post feedback_messages_path, params: valid_abuse_report_params, headers: headers
-        end
-      end
-    end
-
     context "when rate limit is reached" do
       it "returns a 429" do
         user = create(:user)
@@ -87,57 +74,123 @@ RSpec.describe "feedback_messages", type: :request do
       end
     end
 
-    context "when a user submits a report" do
-      let(:user) { create(:user) }
-
+    describe "detailed scenarios where recaptcha is enabled" do
       before do
-        mock_recaptcha_verification
-
-        sign_in user
+        allow(SiteConfig).to receive(:recaptcha_secret_key).and_return("someSecretKey")
+        allow(SiteConfig).to receive(:recaptcha_site_key).and_return("someSiteKey")
       end
 
-      it "creates a feedback message reported by the user" do
-        post feedback_messages_path, params: valid_abuse_report_params, headers: headers
-
-        expect(FeedbackMessage.exists?(reporter_id: user.id)).to be(true)
-      end
-
-      it "queues a slack message to be sent" do
-        sidekiq_assert_enqueued_jobs(1, only: Slack::Messengers::Worker) do
+      context "with valid params but recaptcha not passed" do
+        it "rerenders page" do
           post feedback_messages_path, params: valid_abuse_report_params, headers: headers
+          expect(response.body).to include("Make sure the forms are filled")
         end
-      end
-    end
 
-    context "when an anonymous user submits a report" do
-      before do
-        mock_recaptcha_verification
-      end
-
-      it "does not add any user as the reporter" do
-        post "/feedback_messages", params: valid_abuse_report_params, headers: headers
-
-        expect(FeedbackMessage.last.reporter).to be(nil)
-      end
-
-      it "queues a slack message to be sent" do
-        sidekiq_assert_enqueued_jobs(1, only: Slack::Messengers::Worker) do
-          post feedback_messages_path, params: valid_abuse_report_params, headers: headers
+        it "doesn't queues a slack message" do
+          sidekiq_assert_no_enqueued_jobs(only: Slack::Messengers::Worker) do
+            post feedback_messages_path, params: valid_abuse_report_params, headers: headers
+          end
         end
       end
 
-      it "redirects to the index page" do
-        post "/feedback_messages", params: valid_abuse_report_params, headers: headers
+      context "when a user qualifies to bypass the recaptcha submits a report" do
+        let(:user) { create(:user, created_at: 3.months.ago) }
 
-        expect(response).to redirect_to(feedback_messages_path)
+        before do
+          sign_in user
+        end
+
+        it "creates a feedback message reported by the user without recaptcha" do
+          post feedback_messages_path, params: valid_abuse_report_params, headers: headers
+
+          expect(FeedbackMessage.exists?(reporter_id: user.id)).to be(true)
+        end
+
+        it "queues a slack message to be sent" do
+          sidekiq_assert_enqueued_jobs(1, only: Slack::Messengers::Worker) do
+            post feedback_messages_path, params: valid_abuse_report_params, headers: headers
+          end
+        end
       end
 
-      it "redirects and continues to the index page with the correct message" do
-        post "/feedback_messages", params: valid_abuse_report_params, headers: headers
+      context "when a user doesn't qualify to bypass the recaptcha submits a report" do
+        let(:user) do
+          user = create(:user, created_at: 2.months.ago)
+          create(:reaction,
+                 category: "vomit",
+                 reactable: user,
+                 user: create(:user, :trusted),
+                 status: "confirmed")
+          user
+        end
 
-        follow_redirect!
+        before do
+          sign_in user
+        end
 
-        expect(response.body).to include("Thank you for your report.")
+        it "fails to create a feedback message reported without recaptcha" do
+          post feedback_messages_path, params: valid_abuse_report_params, headers: headers
+
+          expect(FeedbackMessage.exists?(reporter_id: user.id)).to be(false)
+        end
+
+        it "doesn't queue a slack message to be sent" do
+          sidekiq_assert_enqueued_jobs(0, only: Slack::Messengers::Worker) do
+            post feedback_messages_path, params: valid_abuse_report_params, headers: headers
+          end
+        end
+      end
+
+      context "when a moderator submits a report" do
+        let(:user) { create(:user, :tag_moderator) }
+
+        before do
+          sign_in user
+        end
+
+        it "creates a feedback message reported by the moderator without recaptcha" do
+          post feedback_messages_path, params: valid_abuse_report_params, headers: headers
+
+          expect(FeedbackMessage.exists?(reporter_id: user.id)).to be(true)
+        end
+
+        it "queues a slack message to be sent" do
+          sidekiq_assert_enqueued_jobs(1, only: Slack::Messengers::Worker) do
+            post feedback_messages_path, params: valid_abuse_report_params, headers: headers
+          end
+        end
+      end
+
+      context "when an anonymous user submits a report" do
+        before do
+          mock_recaptcha_verification
+        end
+
+        it "does not add any user as the reporter" do
+          post "/feedback_messages", params: valid_abuse_report_params, headers: headers
+
+          expect(FeedbackMessage.last.reporter).to be(nil)
+        end
+
+        it "queues a slack message to be sent" do
+          sidekiq_assert_enqueued_jobs(1, only: Slack::Messengers::Worker) do
+            post feedback_messages_path, params: valid_abuse_report_params, headers: headers
+          end
+        end
+
+        it "redirects to the index page" do
+          post "/feedback_messages", params: valid_abuse_report_params, headers: headers
+
+          expect(response).to redirect_to(feedback_messages_path)
+        end
+
+        it "redirects and continues to the index page with the correct message" do
+          post "/feedback_messages", params: valid_abuse_report_params, headers: headers
+
+          follow_redirect!
+
+          expect(response.body).to include("Thank you for your report.")
+        end
       end
     end
   end

spec/requests/registrations_spec.rb

@@ -89,6 +89,8 @@ RSpec.describe "Registrations", type: :request do
 
     context "when email registration allowed and captcha required" do
       before do
+        allow(SiteConfig).to receive(:recaptcha_secret_key).and_return("someSecretKey")
+        allow(SiteConfig).to receive(:recaptcha_site_key).and_return("someSiteKey")
         allow(SiteConfig).to receive(:allow_email_password_registration).and_return(true)
         allow(SiteConfig).to receive(:require_captcha_for_email_password_registration).and_return(true)
       end
@@ -268,6 +270,8 @@ RSpec.describe "Registrations", type: :request do
 
     context "when site configured to accept email registration AND require captcha" do
       before do
+        allow(SiteConfig).to receive(:recaptcha_secret_key).and_return("someSecretKey")
+        allow(SiteConfig).to receive(:recaptcha_site_key).and_return("someSiteKey")
         allow(SiteConfig).to receive(:allow_email_password_registration).and_return(true)
         allow(SiteConfig).to receive(:require_captcha_for_email_password_registration).and_return(true)
       end

spec/services/re_captcha_spec.rb

@@ -0,0 +1,102 @@
+require "rails_helper"
+
+RSpec.describe "ReCaptcha", type: :request do
+  let(:admin) { create(:user, :super_admin) }
+  let(:recent_user) { create(:user) }
+  let(:older_user) { create(:user, created_at: 3.months.ago) }
+  let(:trusted_user) { create(:user, :trusted) }
+  let(:vomitted_user) do
+    user = create(:user, created_at: 3.months.ago)
+    create(:vomit_reaction, category: "vomit", reactable: user, user: trusted_user, status: "confirmed")
+    user
+  end
+
+  describe "ReCaptcha for registration" do
+    context "when recaptcha is enabled" do
+      before do
+        allow(SiteConfig).to receive(:require_captcha_for_email_password_registration).and_return(true)
+      end
+
+      it "is enabled if both site & secret keys present" do
+        allow(SiteConfig).to receive(:recaptcha_secret_key).and_return("someSecretKey")
+        allow(SiteConfig).to receive(:recaptcha_site_key).and_return("someSiteKey")
+
+        expect(ReCaptcha.for_registration_enabled?).to be(true)
+        expect(ReCaptcha.for_registration_disabled?).to be(false)
+      end
+
+      it "is disabled if site or secret key missing" do
+        allow(SiteConfig).to receive(:recaptcha_site_key).and_return("")
+
+        expect(ReCaptcha.for_registration_enabled?).to be(false)
+        expect(ReCaptcha.for_registration_disabled?).to be(true)
+      end
+    end
+
+    it "is disabled if recaptcha disabled for email signup" do
+      allow(SiteConfig).to receive(:require_captcha_for_email_password_registration).and_return(false)
+
+      expect(ReCaptcha.for_registration_enabled?).to be(false)
+      expect(ReCaptcha.for_registration_disabled?).to be(true)
+    end
+  end
+
+  describe "ReCaptcha for user actions like Abuse Reports (FeedbackMessages)" do
+    context "when recaptcha keys are not configured" do
+      before do
+        allow(SiteConfig).to receive(:recaptcha_site_key).and_return(nil)
+        allow(SiteConfig).to receive(:recaptcha_secret_key).and_return(nil)
+      end
+
+      it "marks ReCaptcha as enabled" do
+        expect(ReCaptcha.call.enabled?).to be(false)
+        expect(ReCaptcha.call.disabled?).to be(true)
+      end
+    end
+
+    context "when recaptcha keys are configured" do
+      before do
+        allow(SiteConfig).to receive(:recaptcha_site_key).and_return("stub")
+        allow(SiteConfig).to receive(:recaptcha_secret_key).and_return("stub")
+        allow(SiteConfig).to receive(:require_captcha_for_email_password_registration).and_return(true)
+      end
+
+      it "marks ReCaptcha as enabled when logged out" do
+        expect(ReCaptcha.call.enabled?).to be(true)
+        expect(ReCaptcha.call.disabled?).to be(false)
+      end
+
+      it "marks ReCaptcha as disabled when older user is logged in" do
+        sign_in older_user
+        expect(ReCaptcha.call(older_user).enabled?).to be(false)
+        expect(ReCaptcha.call(older_user).disabled?).to be(true)
+      end
+
+      it "marks ReCaptcha as disabled when admin is logged in" do
+        sign_in admin
+        expect(ReCaptcha.call(admin).enabled?).to be(false)
+        expect(ReCaptcha.call(admin).disabled?).to be(true)
+      end
+
+      it "marks ReCaptcha as disabled when trusted user is logged in" do
+        sign_in trusted_user
+        expect(ReCaptcha.call(trusted_user).enabled?).to be(false)
+        expect(ReCaptcha.call(trusted_user).disabled?).to be(true)
+      end
+
+      it "marks ReCaptcha as enabled when user with vomits is logged in" do
+        sign_in vomitted_user
+        expect(ReCaptcha.call(vomitted_user).enabled?).to be(true)
+        expect(ReCaptcha.call(vomitted_user).disabled?).to be(false)
+      end
+
+      it "marks ReCaptcha as enabled when a banned user is logged in" do
+        older_user.add_role(:banned)
+        sign_in older_user
+        expect(ReCaptcha.call(older_user).enabled?).to be(true)
+        expect(ReCaptcha.call(older_user).disabled?).to be(false)
+        older_user.remove_role(:banned)
+      end
+    end
+  end
+end