maverick/docbrown
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Split ReCaptcha service and use verbs instead of pronouns (#11706)

Browse source 
* Split ReCaptcha service and use verbs instead of pronouns

* Inline comment rewrite for clarify

* after? > >

* Use explicit role check for the user instead of .auditable?
This commit is contained in:
Fernando Valverde 2020-12-02 16:50:42 -06:00 • committed by GitHub
parent 42e10e7b42
commit b02262c563
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
10 changed files with 145 additions and 159 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

4
app/controllers/feedback_messages_controller.rb
View file

@ -9,8 +9,8 @@ class FeedbackMessagesController < ApplicationController
params = feedback_message_params.merge(reporter_id: current_user&.id)
@feedback_message = FeedbackMessage.new(params)
recaptcha_disabled = ReCaptcha.call(current_user).disabled?
if (recaptcha_disabled || recaptcha_verified?) && @feedback_message.save
recaptcha_enabled = ReCaptcha::CheckEnabled.call(current_user)
if (!recaptcha_enabled || recaptcha_verified?) && @feedback_message.save
Slack::Messengers::Feedback.call(
user: current_user,
type: feedback_message_params[:feedback_type],

2
app/controllers/registrations_controller.rb
View file

@ -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.for_registration_disabled? || recaptcha_verified?
if !ReCaptcha::CheckRegistrationEnabled.call || recaptcha_verified?
build_resource(sign_up_params)
resource.saw_onboarding = false
resource.registered = true

52
app/services/re_captcha.rb
View file

@ -1,52 +0,0 @@
# 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

37
app/services/re_captcha/check_enabled.rb Normal file
View file

@ -0,0 +1,37 @@
# This service encapsulates the logic related to validating if reCAPTCHA is
# enabled in the current Forem instance. The decision is based on making
# sure the necessary SiteConfig keys are available and also on the user
# object passed in.
#
# Example use: ReCaptcha::CheckEnabled.call(current_user) => true/false
module ReCaptcha
class CheckEnabled
def self.call(user = nil)
new(user).call
end
def initialize(user)
@user = user
end
def call
# recaptcha will not be enabled if site key and secret key aren't set
return false unless keys_configured?
# recaptcha will always be enabled when not logged in
return true if @user.nil?
# recaptcha will not be enabled for tag moderator/trusted/admin users
return false if @user.tag_moderator? || @user.trusted || @user.any_admin?
# recaptcha will be enabled if the user has been banned
return true if @user.banned
# recaptcha will be enabled if the user has a vomit or is too recent
@user.vomitted_on? || @user.created_at.after?(1.month.ago)
end
private
def keys_configured?
SiteConfig.recaptcha_site_key.present? && SiteConfig.recaptcha_secret_key.present?
end
end
end

13
app/services/re_captcha/check_registration_enabled.rb Normal file
View file

@ -0,0 +1,13 @@
module ReCaptcha
class CheckRegistrationEnabled
def self.call
new.call
end
def call
# ReCaptcha::CheckEnabled.call without a user parameter will return `true`
# if the ReCaptcha SiteConfig keys are configured, and `false` otherwise
ReCaptcha::CheckEnabled.call && SiteConfig.require_captcha_for_email_password_registration
end
end
end

2
app/views/feedback_messages/_form.html.erb
View file

@ -67,7 +67,7 @@
<%= f.text_area :message, class: "crayons-textfield", placeholder: "...", value: @previous_message, required: true %>
</div>
<% if ReCaptcha.call(current_user).enabled? %>
<% if ReCaptcha::CheckEnabled.call(current_user) %>
<div class="recaptcha-tag-container">
<%= recaptcha_tags site_key: SiteConfig.recaptcha_site_key %>
</div>

2
app/views/shared/authentication/_email_registration_form.html.erb
View file

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

63
spec/services/re_captcha/check_enabled_spec.rb Normal file
View file

@ -0,0 +1,63 @@
require "rails_helper"
RSpec.describe ReCaptcha::CheckEnabled, 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 user actions like Abuse Reports (FeedbackMessages)" do
context "when recaptcha SiteConfig keys are not configured" do
it "marks ReCaptcha as not enabled regardless of the param passed in" do
allow(SiteConfig).to receive(:recaptcha_site_key).and_return(nil)
allow(SiteConfig).to receive(:recaptcha_secret_key).and_return(nil)
expect(described_class.call).to be(false)
expect(described_class.call(older_user)).to be(false)
end
end
context "when recaptcha SiteConfig keys are configured" do
before do
allow(SiteConfig).to receive(:recaptcha_site_key).and_return("someSecretKey")
allow(SiteConfig).to receive(:recaptcha_secret_key).and_return("someSiteKey")
end
it "marks ReCaptcha as enabled when logged out (parameter is nil)" do
expect(described_class.call).to be(true)
end
it "marks ReCaptcha as not enabled when older user is logged in" do
sign_in older_user
expect(described_class.call(older_user)).to be(false)
end
it "marks ReCaptcha as not enabled when admin is logged in" do
sign_in admin
expect(described_class.call(admin)).to be(false)
end
it "marks ReCaptcha as not enabled when trusted user is logged in" do
sign_in trusted_user
expect(described_class.call(trusted_user)).to be(false)
end
it "marks ReCaptcha as enabled when user with vomits is logged in" do
sign_in vomitted_user
expect(described_class.call(vomitted_user)).to be(true)
end
it "marks ReCaptcha as enabled when a banned user is logged in" do
older_user.add_role(:banned)
sign_in older_user
expect(described_class.call(older_user)).to be(true)
older_user.remove_role(:banned)
end
end
end
end

27
spec/services/re_captcha/check_registration_enabled_spec.rb Normal file
View file

@ -0,0 +1,27 @@
require "rails_helper"
RSpec.describe ReCaptcha::CheckRegistrationEnabled, type: :request do
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(described_class.call).to be(true)
end
it "is disabled if site or secret key missing" do
allow(SiteConfig).to receive(:recaptcha_site_key).and_return("")
expect(described_class.call).to be(false)
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(described_class.call).to be(false)
end
end
end

102
spec/services/re_captcha_spec.rb
View file

@ -1,102 +0,0 @@
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