Change CAPTCHA handling to be only on email verification
This simplifies the implementation considerably, and while not providing ideal UX, it's the most flexible approach.
This commit is contained in:
		
							parent
							
								
									0fb907441c
								
							
						
					
					
						commit
						b7cf3941b3
					
				
					 14 changed files with 15 additions and 84 deletions
				
			
		|  | @ -2,7 +2,6 @@ | |||
| 
 | ||||
| class AboutController < ApplicationController | ||||
|   include RegistrationSpamConcern | ||||
|   include CaptchaConcern | ||||
| 
 | ||||
|   before_action :set_pack | ||||
| 
 | ||||
|  | @ -13,7 +12,6 @@ class AboutController < ApplicationController | |||
|   before_action :set_instance_presenter | ||||
|   before_action :set_expires_in, only: [:more, :terms] | ||||
|   before_action :set_registration_form_time, only: :show | ||||
|   before_action :extend_csp_for_captcha!, only: :show | ||||
| 
 | ||||
|   skip_before_action :require_functional!, only: [:more, :terms] | ||||
| 
 | ||||
|  |  | |||
|  | @ -1,8 +1,6 @@ | |||
| # frozen_string_literal: true | ||||
| 
 | ||||
| class Api::V1::AccountsController < Api::BaseController | ||||
|   include CaptchaConcern | ||||
| 
 | ||||
|   before_action -> { authorize_if_got_token! :read, :'read:accounts' }, except: [:create, :follow, :unfollow, :remove_from_followers, :block, :unblock, :mute, :unmute] | ||||
|   before_action -> { doorkeeper_authorize! :follow, :'write:follows' }, only: [:follow, :unfollow, :remove_from_followers] | ||||
|   before_action -> { doorkeeper_authorize! :follow, :'write:mutes' }, only: [:mute, :unmute] | ||||
|  | @ -85,7 +83,7 @@ class Api::V1::AccountsController < Api::BaseController | |||
|   end | ||||
| 
 | ||||
|   def check_enabled_registrations | ||||
|     forbidden if single_user_mode? || omniauth_only? || !allowed_registrations? || captcha_enabled? | ||||
|     forbidden if single_user_mode? || omniauth_only? || !allowed_registrations? | ||||
|   end | ||||
| 
 | ||||
|   def allowed_registrations? | ||||
|  |  | |||
|  | @ -22,8 +22,6 @@ class Auth::ConfirmationsController < Devise::ConfirmationsController | |||
|   end | ||||
| 
 | ||||
|   def show | ||||
|     clear_captcha! | ||||
| 
 | ||||
|     old_session_values = session.to_hash | ||||
|     reset_session | ||||
|     session.update old_session_values.except('session_id') | ||||
|  | @ -63,10 +61,6 @@ class Auth::ConfirmationsController < Devise::ConfirmationsController | |||
|     invite.present? && !invite.max_uses.nil? | ||||
|   end | ||||
| 
 | ||||
|   def captcha_context | ||||
|     'email-confirmation' | ||||
|   end | ||||
| 
 | ||||
|   def set_pack | ||||
|     use_pack 'auth' | ||||
|   end | ||||
|  |  | |||
|  | @ -2,7 +2,6 @@ | |||
| 
 | ||||
| class Auth::RegistrationsController < Devise::RegistrationsController | ||||
|   include RegistrationSpamConcern | ||||
|   include CaptchaConcern | ||||
| 
 | ||||
|   layout :determine_layout | ||||
| 
 | ||||
|  | @ -16,8 +15,6 @@ class Auth::RegistrationsController < Devise::RegistrationsController | |||
|   before_action :require_not_suspended!, only: [:update] | ||||
|   before_action :set_cache_headers, only: [:edit, :update] | ||||
|   before_action :set_registration_form_time, only: :new | ||||
|   before_action :extend_csp_for_captcha!, only: [:new, :create] | ||||
|   before_action :check_captcha!, only: :create | ||||
| 
 | ||||
|   skip_before_action :require_functional!, only: [:edit, :update] | ||||
| 
 | ||||
|  | @ -138,23 +135,4 @@ class Auth::RegistrationsController < Devise::RegistrationsController | |||
|   def set_cache_headers | ||||
|     response.headers['Cache-Control'] = 'no-cache, no-store, max-age=0, must-revalidate' | ||||
|   end | ||||
| 
 | ||||
|   def sign_up(resource_name, resource) | ||||
|     clear_captcha! | ||||
| 
 | ||||
|     old_session_values = session.to_hash | ||||
|     reset_session | ||||
|     session.update old_session_values.except('session_id') | ||||
| 
 | ||||
|     super | ||||
|   end | ||||
| 
 | ||||
|   def check_captcha! | ||||
|     super do |error| | ||||
|       build_resource(sign_up_params) | ||||
|       resource.validate | ||||
|       resource.errors.add(:base, error) | ||||
|       respond_with resource | ||||
|     end | ||||
|   end | ||||
| end | ||||
|  |  | |||
|  | @ -4,10 +4,8 @@ module CaptchaConcern | |||
|   extend ActiveSupport::Concern | ||||
|   include Hcaptcha::Adapters::ViewMethods | ||||
| 
 | ||||
|   CAPTCHA_TIMEOUT = 2.hours.freeze | ||||
| 
 | ||||
|   included do | ||||
|     helper_method :render_captcha_if_needed | ||||
|     helper_method :render_captcha | ||||
|   end | ||||
| 
 | ||||
|   def captcha_available? | ||||
|  | @ -15,32 +13,21 @@ module CaptchaConcern | |||
|   end | ||||
| 
 | ||||
|   def captcha_enabled? | ||||
|     captcha_available? && Setting.captcha_mode == captcha_context | ||||
|   end | ||||
| 
 | ||||
|   def captcha_recently_passed? | ||||
|     session[:captcha_passed_at].present? && session[:captcha_passed_at] >= CAPTCHA_TIMEOUT.ago | ||||
|     captcha_available? && Setting.captcha_enabled | ||||
|   end | ||||
| 
 | ||||
|   def captcha_user_bypass? | ||||
|     current_user.present? || (@invite.present? && @invite.valid_for_use? && !@invite.max_uses.nil?) | ||||
|     false | ||||
|   end | ||||
| 
 | ||||
|   def captcha_required? | ||||
|     return false if ENV['OMNIAUTH_ONLY'] == 'true' | ||||
|     return false unless Setting.registrations_mode != 'none' || @invite&.valid_for_use? | ||||
|     captcha_enabled? && !captcha_user_bypass? && !captcha_recently_passed? | ||||
|   end | ||||
| 
 | ||||
|   def clear_captcha! | ||||
|     session.delete(:captcha_passed_at) | ||||
|     captcha_enabled? && !captcha_user_bypass? | ||||
|   end | ||||
| 
 | ||||
|   def check_captcha! | ||||
|     return true unless captcha_required? | ||||
| 
 | ||||
|     if verify_hcaptcha | ||||
|       session[:captcha_passed_at] = Time.now.utc | ||||
|       true | ||||
|     else | ||||
|       if block_given? | ||||
|  | @ -64,13 +51,9 @@ module CaptchaConcern | |||
|     end | ||||
|   end | ||||
| 
 | ||||
|   def render_captcha_if_needed | ||||
|   def render_captcha | ||||
|     return unless captcha_required? | ||||
| 
 | ||||
|     hcaptcha_tags | ||||
|   end | ||||
| 
 | ||||
|   def captcha_context | ||||
|     'registration-form' | ||||
|   end | ||||
| end | ||||
|  |  | |||
|  | @ -40,7 +40,7 @@ class Form::AdminSettings | |||
|     noindex | ||||
|     outgoing_spoilers | ||||
|     require_invite_text | ||||
|     captcha_mode | ||||
|     captcha_enabled | ||||
|   ).freeze | ||||
| 
 | ||||
|   BOOLEAN_KEYS = %i( | ||||
|  | @ -59,6 +59,7 @@ class Form::AdminSettings | |||
|     trendable_by_default | ||||
|     noindex | ||||
|     require_invite_text | ||||
|     captcha_enabled | ||||
|   ).freeze | ||||
| 
 | ||||
|   UPLOAD_KEYS = %i( | ||||
|  | @ -82,7 +83,6 @@ class Form::AdminSettings | |||
|   validates :bootstrap_timeline_accounts, existing_username: { multiple: true } | ||||
|   validates :show_domain_blocks, inclusion: { in: %w(disabled users all) } | ||||
|   validates :show_domain_blocks_rationale, inclusion: { in: %w(disabled users all) } | ||||
|   validates :captcha_mode, inclusion: { in: %w(disabled registration-form email-confirmation) } | ||||
| 
 | ||||
|   def initialize(_attributes = {}) | ||||
|     super | ||||
|  |  | |||
|  | @ -98,7 +98,7 @@ class REST::InstanceSerializer < ActiveModel::Serializer | |||
|   end | ||||
| 
 | ||||
|   def registrations | ||||
|     Setting.registrations_mode != 'none' && !Rails.configuration.x.single_user_mode && !captcha_enabled? | ||||
|     Setting.registrations_mode != 'none' && !Rails.configuration.x.single_user_mode | ||||
|   end | ||||
| 
 | ||||
|   def approval_required | ||||
|  | @ -114,8 +114,4 @@ class REST::InstanceSerializer < ActiveModel::Serializer | |||
|   def instance_presenter | ||||
|     @instance_presenter ||= InstancePresenter.new | ||||
|   end | ||||
| 
 | ||||
|   def captcha_enabled? | ||||
|     ENV['HCAPTCHA_SECRET_KEY'].present? && ENV['HCAPTCHA_SITE_KEY'].present? && Setting.captcha_mode == 'registration-form' | ||||
|   end | ||||
| end | ||||
|  |  | |||
|  | @ -21,9 +21,6 @@ | |||
|     .fields-group | ||||
|       = f.input :agreement, as: :boolean, wrapper: :with_label, label: t('auth.checkbox_agreement_html', rules_path: about_more_path, terms_path: terms_path), required: true, disabled: closed_registrations? | ||||
| 
 | ||||
|     .fields-group | ||||
|       = render_captcha_if_needed | ||||
| 
 | ||||
|     .actions | ||||
|       = f.button :button, sign_up_message, type: :submit, class: 'button button-primary', disabled: closed_registrations? | ||||
| 
 | ||||
|  |  | |||
|  | @ -45,7 +45,7 @@ | |||
| 
 | ||||
|   - if captcha_available? | ||||
|     .fields-group | ||||
|       = f.input :captcha_mode, as: :radio_buttons, collection: %w(disabled registration-form email-confirmation), include_blank: false, wrapper: :with_block_label, label_method: ->(type) { safe_join([t("admin.settings.captcha.#{type}.title"), content_tag(:span, t("admin.settings.captcha.#{type}.desc_html"), class: 'hint')])}, label: t('admin.settings.captcha.title'), hint: t('admin.settings.captcha.desc_html') | ||||
|       = f.input :captcha_enabled, as: :boolean, wrapper: :with_label, label: t('admin.settings.captcha_enabled.title'), hint: t('admin.settings.captcha_enabled.desc_html') | ||||
| 
 | ||||
|   %hr.spacer/ | ||||
| 
 | ||||
|  |  | |||
|  | @ -5,7 +5,7 @@ | |||
|   = hidden_field_tag :confirmation_token, params[:confirmation_token] | ||||
| 
 | ||||
|   .field-group | ||||
|     = render_captcha_if_needed | ||||
|     = render_captcha | ||||
| 
 | ||||
|   .actions | ||||
|     %button.button= t('challenge.continue') | ||||
|  |  | |||
|  | @ -38,9 +38,6 @@ | |||
|   .fields-group | ||||
|     = f.input :agreement, as: :boolean, wrapper: :with_label, label: whitelist_mode? ? t('auth.checkbox_agreement_without_rules_html', terms_path: terms_path) : t('auth.checkbox_agreement_html', rules_path: about_more_path, terms_path: terms_path), required: true | ||||
| 
 | ||||
|   .field-group | ||||
|     = render_captcha_if_needed | ||||
| 
 | ||||
|   .actions | ||||
|     = f.button :button, @invite.present? ? t('auth.register') : sign_up_message, type: :submit | ||||
| 
 | ||||
|  |  | |||
|  | @ -2,18 +2,9 @@ | |||
| en: | ||||
|   admin: | ||||
|     settings: | ||||
|       captcha: | ||||
|         desc_html: Configure hCaptcha integration, relying on third-party scripts. This may have security and privacy implications. | ||||
|         email-confirmation: | ||||
|           desc_html: Require new users to go through hCaptcha at the e-mail validation step. Bots will not be deterred from creating accounts, but they may be prevented from confirming them, leaving them to be automatically cleaned up after a couple days. This does not interfere with app-based registrations. | ||||
|           title: CAPTCHA on email validation | ||||
|         disabled: | ||||
|           desc_html: Do not require a CAPTCHA | ||||
|           title: Disabled | ||||
|         registration-form: | ||||
|           desc_html: Require new users to go through hCaptcha on the registration form, so that CAPTCHA requirement is immediately apparent to them. This disables app-based registrations and may prevent your instance from being listed as having open registrations. | ||||
|           title: CAPTCHA on registration forms | ||||
|         title: CAPTCHA configuration | ||||
|       captcha_enabled: | ||||
|         desc_html: Enable hCaptcha integration, requiring new users to solve a challenge when confirming their email address. This requires third-party scripts from hCaptcha to be embedded in the email verification page, which may have security and privacy concerns. Users that have been invited through a limited-use invite will not need to solve a CAPTCHA challenge. | ||||
|         title: Require new users to go through a CAPTCHA to confirm their account | ||||
|       enable_keybase: | ||||
|         desc_html: Allow your users to prove their identity via keybase | ||||
|         title: Enable keybase integration | ||||
|  |  | |||
|  | @ -77,7 +77,7 @@ defaults: &defaults | |||
|   show_domain_blocks_rationale: 'disabled' | ||||
|   outgoing_spoilers: '' | ||||
|   require_invite_text: false | ||||
|   captcha_mode: 'disabled' | ||||
|   captcha_enabled: false | ||||
| 
 | ||||
| development: | ||||
|   <<: *defaults | ||||
|  |  | |||
|  | @ -10,7 +10,6 @@ describe 'about/show.html.haml', without_verify_partial_doubles: true do | |||
|     allow(view).to receive(:site_title).and_return('example site') | ||||
|     allow(view).to receive(:new_user).and_return(User.new) | ||||
|     allow(view).to receive(:use_seamless_external_login?).and_return(false) | ||||
|     allow(view).to receive(:render_captcha_if_needed).and_return(nil) | ||||
|   end | ||||
| 
 | ||||
|   it 'has valid open graph tags' do | ||||
|  |  | |||
		Loading…
	
		Reference in a new issue