From 494af84e83a9349f2aaa35a1a6ffb6e018760739 Mon Sep 17 00:00:00 2001 From: Fernando Valverde Date: Wed, 2 Dec 2020 08:55:47 -0600 Subject: [PATCH] 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 --- .../feedback_messages_controller.rb | 7 +- app/controllers/registrations_controller.rb | 7 +- app/helpers/authentication_helper.rb | 6 - app/services/re_captcha.rb | 52 ++++++ app/views/feedback_messages/_form.html.erb | 10 +- .../_email_registration_form.html.erb | 2 +- spec/helpers/authentication_helper_spec.rb | 27 --- spec/requests/feedback_messages_spec.rb | 161 ++++++++++++------ spec/requests/registrations_spec.rb | 4 + spec/services/re_captcha_spec.rb | 102 +++++++++++ 10 files changed, 274 insertions(+), 104 deletions(-) create mode 100644 app/services/re_captcha.rb create mode 100644 spec/services/re_captcha_spec.rb diff --git a/app/controllers/feedback_messages_controller.rb b/app/controllers/feedback_messages_controller.rb index 15be4f7a8..c53f381da 100644 --- a/app/controllers/feedback_messages_controller.rb +++ b/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) diff --git a/app/controllers/registrations_controller.rb b/app/controllers/registrations_controller.rb index 847c68cb6..4111af40b 100644 --- a/app/controllers/registrations_controller.rb +++ b/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) diff --git a/app/helpers/authentication_helper.rb b/app/helpers/authentication_helper.rb index 068d8f468..3bceeca0b 100644 --- a/app/helpers/authentication_helper.rb +++ b/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 diff --git a/app/services/re_captcha.rb b/app/services/re_captcha.rb new file mode 100644 index 000000000..5917bf97d --- /dev/null +++ b/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 diff --git a/app/views/feedback_messages/_form.html.erb b/app/views/feedback_messages/_form.html.erb index aebf99982..df777a02a 100644 --- a/app/views/feedback_messages/_form.html.erb +++ b/app/views/feedback_messages/_form.html.erb @@ -67,11 +67,11 @@ <%= f.text_area :message, class: "crayons-textfield", placeholder: "...", value: @previous_message, required: true %> -<% if SiteConfig.recaptcha_secret_key.present? && SiteConfig.recaptcha_site_key.present? %> -
- <%= recaptcha_tags site_key: SiteConfig.recaptcha_site_key %> -
-<% end %> + <% if ReCaptcha.call(current_user).enabled? %> +
+ <%= recaptcha_tags site_key: SiteConfig.recaptcha_site_key %> +
+ <% end %>
<% end %> diff --git a/app/views/shared/authentication/_email_registration_form.html.erb b/app/views/shared/authentication/_email_registration_form.html.erb index a839598ce..03fa6d43a 100644 --- a/app/views/shared/authentication/_email_registration_form.html.erb +++ b/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? %>
<%= recaptcha_tags site_key: SiteConfig.recaptcha_site_key %>
diff --git a/spec/helpers/authentication_helper_spec.rb b/spec/helpers/authentication_helper_spec.rb index ba7ba386f..9098dfc2e 100644 --- a/spec/helpers/authentication_helper_spec.rb +++ b/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 diff --git a/spec/requests/feedback_messages_spec.rb b/spec/requests/feedback_messages_spec.rb index 09e971f8e..77aea19d9 100644 --- a/spec/requests/feedback_messages_spec.rb +++ b/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 diff --git a/spec/requests/registrations_spec.rb b/spec/requests/registrations_spec.rb index edd2c3812..8b3ec178c 100644 --- a/spec/requests/registrations_spec.rb +++ b/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 diff --git a/spec/services/re_captcha_spec.rb b/spec/services/re_captcha_spec.rb new file mode 100644 index 000000000..c8e54ba6d --- /dev/null +++ b/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