From b7cf3941b3783220e6b3bc9a6d3975ceecdc64cb Mon Sep 17 00:00:00 2001 From: Claire Date: Tue, 25 Jan 2022 23:56:57 +0100 Subject: [PATCH] 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. --- app/controllers/about_controller.rb | 2 -- app/controllers/api/v1/accounts_controller.rb | 4 +-- .../auth/confirmations_controller.rb | 6 ----- .../auth/registrations_controller.rb | 22 --------------- app/controllers/concerns/captcha_concern.rb | 27 ++++--------------- app/models/form/admin_settings.rb | 4 +-- app/serializers/rest/instance_serializer.rb | 6 +---- app/views/about/_registration.html.haml | 3 --- app/views/admin/settings/edit.html.haml | 2 +- .../auth/confirmations/captcha.html.haml | 2 +- app/views/auth/registrations/new.html.haml | 3 --- config/locales-glitch/en.yml | 15 +++-------- config/settings.yml | 2 +- spec/views/about/show.html.haml_spec.rb | 1 - 14 files changed, 15 insertions(+), 84 deletions(-) diff --git a/app/controllers/about_controller.rb b/app/controllers/about_controller.rb index 5a35dbbcb..620c0ff78 100644 --- a/app/controllers/about_controller.rb +++ b/app/controllers/about_controller.rb @@ -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] diff --git a/app/controllers/api/v1/accounts_controller.rb b/app/controllers/api/v1/accounts_controller.rb index 8916c3f96..5c47158e0 100644 --- a/app/controllers/api/v1/accounts_controller.rb +++ b/app/controllers/api/v1/accounts_controller.rb @@ -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? diff --git a/app/controllers/auth/confirmations_controller.rb b/app/controllers/auth/confirmations_controller.rb index e9a646f91..17ad56fa8 100644 --- a/app/controllers/auth/confirmations_controller.rb +++ b/app/controllers/auth/confirmations_controller.rb @@ -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 diff --git a/app/controllers/auth/registrations_controller.rb b/app/controllers/auth/registrations_controller.rb index 0db9cb84d..6b1f3fa82 100644 --- a/app/controllers/auth/registrations_controller.rb +++ b/app/controllers/auth/registrations_controller.rb @@ -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 diff --git a/app/controllers/concerns/captcha_concern.rb b/app/controllers/concerns/captcha_concern.rb index 02069d205..538c1ffb1 100644 --- a/app/controllers/concerns/captcha_concern.rb +++ b/app/controllers/concerns/captcha_concern.rb @@ -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 diff --git a/app/models/form/admin_settings.rb b/app/models/form/admin_settings.rb index 7abb0d6c6..34f14e312 100644 --- a/app/models/form/admin_settings.rb +++ b/app/models/form/admin_settings.rb @@ -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 diff --git a/app/serializers/rest/instance_serializer.rb b/app/serializers/rest/instance_serializer.rb index d343cca20..48bbb55c8 100644 --- a/app/serializers/rest/instance_serializer.rb +++ b/app/serializers/rest/instance_serializer.rb @@ -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 diff --git a/app/views/about/_registration.html.haml b/app/views/about/_registration.html.haml index 5bb5d08a2..e4d614d71 100644 --- a/app/views/about/_registration.html.haml +++ b/app/views/about/_registration.html.haml @@ -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? diff --git a/app/views/admin/settings/edit.html.haml b/app/views/admin/settings/edit.html.haml index fc042f845..49b03a9e3 100644 --- a/app/views/admin/settings/edit.html.haml +++ b/app/views/admin/settings/edit.html.haml @@ -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/ diff --git a/app/views/auth/confirmations/captcha.html.haml b/app/views/auth/confirmations/captcha.html.haml index 850bc1479..0f7cf9c59 100644 --- a/app/views/auth/confirmations/captcha.html.haml +++ b/app/views/auth/confirmations/captcha.html.haml @@ -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') diff --git a/app/views/auth/registrations/new.html.haml b/app/views/auth/registrations/new.html.haml index 5cb558297..6981195ed 100644 --- a/app/views/auth/registrations/new.html.haml +++ b/app/views/auth/registrations/new.html.haml @@ -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 diff --git a/config/locales-glitch/en.yml b/config/locales-glitch/en.yml index 6ad5a5365..ab7f1b976 100644 --- a/config/locales-glitch/en.yml +++ b/config/locales-glitch/en.yml @@ -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 diff --git a/config/settings.yml b/config/settings.yml index b5437caee..7d192f369 100644 --- a/config/settings.yml +++ b/config/settings.yml @@ -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 diff --git a/spec/views/about/show.html.haml_spec.rb b/spec/views/about/show.html.haml_spec.rb index 12c96ea49..d608bbf5d 100644 --- a/spec/views/about/show.html.haml_spec.rb +++ b/spec/views/about/show.html.haml_spec.rb @@ -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