From dfdadb92e834cc099d16d7539a661105e1d12390 Mon Sep 17 00:00:00 2001 From: Claire Date: Thu, 4 Jan 2024 10:07:05 +0100 Subject: [PATCH] Add ability to require approval when users sign up using specific email domains (#28468) --- .../admin/email_domain_blocks_controller.rb | 4 ++-- .../admin/email_domain_blocks_controller.rb | 2 +- app/models/email_domain_block.rb | 23 +++++++++++-------- app/models/user.rb | 8 ++++++- .../admin/email_domain_block_serializer.rb | 2 +- .../_email_domain_block.html.haml | 4 ++++ .../admin/email_domain_blocks/new.html.haml | 3 +++ config/locales/en.yml | 1 + ...ow_with_approval_to_email_domain_blocks.rb | 7 ++++++ db/schema.rb | 3 ++- .../email_domain_blocks_controller_spec.rb | 3 ++- .../auth/registrations_controller_spec.rb | 19 +++++++++++++++ spec/services/app_sign_up_service_spec.rb | 21 +++++++++++++++++ 13 files changed, 84 insertions(+), 16 deletions(-) create mode 100644 db/migrate/20231222100226_add_allow_with_approval_to_email_domain_blocks.rb diff --git a/app/controllers/admin/email_domain_blocks_controller.rb b/app/controllers/admin/email_domain_blocks_controller.rb index 4a3228ec3..ff754bc0b 100644 --- a/app/controllers/admin/email_domain_blocks_controller.rb +++ b/app/controllers/admin/email_domain_blocks_controller.rb @@ -40,7 +40,7 @@ module Admin (@email_domain_block.other_domains || []).uniq.each do |domain| next if EmailDomainBlock.where(domain: domain).exists? - other_email_domain_block = EmailDomainBlock.create!(domain: domain, parent: @email_domain_block) + other_email_domain_block = EmailDomainBlock.create!(domain: domain, allow_with_approval: @email_domain_block.allow_with_approval, parent: @email_domain_block) log_action :create, other_email_domain_block end end @@ -65,7 +65,7 @@ module Admin end def resource_params - params.require(:email_domain_block).permit(:domain, other_domains: []) + params.require(:email_domain_block).permit(:domain, :allow_with_approval, other_domains: []) end def form_email_domain_block_batch_params diff --git a/app/controllers/api/v1/admin/email_domain_blocks_controller.rb b/app/controllers/api/v1/admin/email_domain_blocks_controller.rb index 850eda622..df54b9f0a 100644 --- a/app/controllers/api/v1/admin/email_domain_blocks_controller.rb +++ b/app/controllers/api/v1/admin/email_domain_blocks_controller.rb @@ -55,7 +55,7 @@ class Api::V1::Admin::EmailDomainBlocksController < Api::BaseController end def resource_params - params.permit(:domain) + params.permit(:domain, :allow_with_approval) end def insert_pagination_headers diff --git a/app/models/email_domain_block.rb b/app/models/email_domain_block.rb index 60e90208d..f1b14c8b0 100644 --- a/app/models/email_domain_block.rb +++ b/app/models/email_domain_block.rb @@ -4,11 +4,12 @@ # # Table name: email_domain_blocks # -# id :bigint(8) not null, primary key -# domain :string default(""), not null -# created_at :datetime not null -# updated_at :datetime not null -# parent_id :bigint(8) +# id :bigint(8) not null, primary key +# domain :string default(""), not null +# created_at :datetime not null +# updated_at :datetime not null +# parent_id :bigint(8) +# allow_with_approval :boolean default(FALSE), not null # class EmailDomainBlock < ApplicationRecord @@ -42,8 +43,8 @@ class EmailDomainBlock < ApplicationRecord @attempt_ip = attempt_ip end - def match? - blocking? || invalid_uri? + def match?(...) + blocking?(...) || invalid_uri? end private @@ -52,8 +53,8 @@ class EmailDomainBlock < ApplicationRecord @uris.any?(&:nil?) end - def blocking? - blocks = EmailDomainBlock.where(domain: domains_with_variants).order(Arel.sql('char_length(domain) desc')) + def blocking?(allow_with_approval: false) + blocks = EmailDomainBlock.where(domain: domains_with_variants, allow_with_approval: allow_with_approval).order(Arel.sql('char_length(domain) desc')) blocks.each { |block| block.history.add(@attempt_ip) } if @attempt_ip.present? blocks.any? end @@ -86,4 +87,8 @@ class EmailDomainBlock < ApplicationRecord def self.block?(domain_or_domains, attempt_ip: nil) Matcher.new(domain_or_domains, attempt_ip: attempt_ip).match? end + + def self.requires_approval?(domain_or_domains, attempt_ip: nil) + Matcher.new(domain_or_domains, attempt_ip: attempt_ip).match?(allow_with_approval: true) + end end diff --git a/app/models/user.rb b/app/models/user.rb index a1574c02a..be1e84b49 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -418,7 +418,7 @@ class User < ApplicationRecord def set_approved self.approved = begin - if sign_up_from_ip_requires_approval? + if sign_up_from_ip_requires_approval? || sign_up_email_requires_approval? false else open_registrations? || valid_invitation? || external? @@ -430,6 +430,12 @@ class User < ApplicationRecord !sign_up_ip.nil? && IpBlock.where(severity: :sign_up_requires_approval).where('ip >>= ?', sign_up_ip.to_s).exists? end + def sign_up_email_requires_approval? + return false unless email.present? || unconfirmed_email.present? + + EmailDomainBlock.requires_approval?(email.presence || unconfirmed_email, attempt_ip: sign_up_ip) + end + def open_registrations? Setting.registrations_mode == 'open' end diff --git a/app/serializers/rest/admin/email_domain_block_serializer.rb b/app/serializers/rest/admin/email_domain_block_serializer.rb index a026ff680..afe7722cb 100644 --- a/app/serializers/rest/admin/email_domain_block_serializer.rb +++ b/app/serializers/rest/admin/email_domain_block_serializer.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true class REST::Admin::EmailDomainBlockSerializer < ActiveModel::Serializer - attributes :id, :domain, :created_at, :history + attributes :id, :domain, :created_at, :history, :allow_with_approval def id object.id.to_s diff --git a/app/views/admin/email_domain_blocks/_email_domain_block.html.haml b/app/views/admin/email_domain_blocks/_email_domain_block.html.haml index 7cb973c4b..f6a6e8266 100644 --- a/app/views/admin/email_domain_blocks/_email_domain_block.html.haml +++ b/app/views/admin/email_domain_blocks/_email_domain_block.html.haml @@ -12,3 +12,7 @@ · = t('admin.email_domain_blocks.attempts_over_week', count: email_domain_block.history.reduce(0) { |sum, day| sum + day.accounts }) + + - if email_domain_block.allow_with_approval? + · + = t('admin.email_domain_blocks.allow_registrations_with_approval') diff --git a/app/views/admin/email_domain_blocks/new.html.haml b/app/views/admin/email_domain_blocks/new.html.haml index fa1d950ad..3d3148773 100644 --- a/app/views/admin/email_domain_blocks/new.html.haml +++ b/app/views/admin/email_domain_blocks/new.html.haml @@ -7,6 +7,9 @@ .fields-group = f.input :domain, wrapper: :with_block_label, label: t('admin.email_domain_blocks.domain'), input_html: { readonly: defined?(@resolved_records) } + .fields-group + = f.input :allow_with_approval, wrapper: :with_label, hint: false, label: I18n.t('admin.email_domain_blocks.allow_registrations_with_approval') + - if defined?(@resolved_records) %p.hint= t('admin.email_domain_blocks.resolved_dns_records_hint_html') diff --git a/config/locales/en.yml b/config/locales/en.yml index 15d682d17..50f814a81 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -425,6 +425,7 @@ en: view: View domain block email_domain_blocks: add_new: Add new + allow_registrations_with_approval: Allow registrations with approval attempts_over_week: one: "%{count} attempt over the last week" other: "%{count} sign-up attempts over the last week" diff --git a/db/migrate/20231222100226_add_allow_with_approval_to_email_domain_blocks.rb b/db/migrate/20231222100226_add_allow_with_approval_to_email_domain_blocks.rb new file mode 100644 index 000000000..01e8edfed --- /dev/null +++ b/db/migrate/20231222100226_add_allow_with_approval_to_email_domain_blocks.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +class AddAllowWithApprovalToEmailDomainBlocks < ActiveRecord::Migration[7.1] + def change + add_column :email_domain_blocks, :allow_with_approval, :boolean, default: false, null: false + end +end diff --git a/db/schema.rb b/db/schema.rb index 126ed8785..4ea9744f4 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.1].define(version: 2023_12_12_073317) do +ActiveRecord::Schema[7.1].define(version: 2023_12_22_100226) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -435,6 +435,7 @@ ActiveRecord::Schema[7.1].define(version: 2023_12_12_073317) do t.datetime "created_at", precision: nil, null: false t.datetime "updated_at", precision: nil, null: false t.bigint "parent_id" + t.boolean "allow_with_approval", default: false, null: false t.index ["domain"], name: "index_email_domain_blocks_on_domain", unique: true end diff --git a/spec/controllers/admin/email_domain_blocks_controller_spec.rb b/spec/controllers/admin/email_domain_blocks_controller_spec.rb index 428660014..9379fe374 100644 --- a/spec/controllers/admin/email_domain_blocks_controller_spec.rb +++ b/spec/controllers/admin/email_domain_blocks_controller_spec.rb @@ -12,13 +12,14 @@ RSpec.describe Admin::EmailDomainBlocksController do describe 'GET #index' do around do |example| default_per_page = EmailDomainBlock.default_per_page - EmailDomainBlock.paginates_per 1 + EmailDomainBlock.paginates_per 2 example.run EmailDomainBlock.paginates_per default_per_page end it 'returns http success' do 2.times { Fabricate(:email_domain_block) } + Fabricate(:email_domain_block, allow_with_approval: true) get :index, params: { page: 2 } expect(response).to have_http_status(200) end diff --git a/spec/controllers/auth/registrations_controller_spec.rb b/spec/controllers/auth/registrations_controller_spec.rb index 37172f8d2..bd1c61659 100644 --- a/spec/controllers/auth/registrations_controller_spec.rb +++ b/spec/controllers/auth/registrations_controller_spec.rb @@ -135,6 +135,25 @@ RSpec.describe Auth::RegistrationsController do end end + context 'when user has an email address requiring approval' do + subject do + Setting.registrations_mode = 'open' + Fabricate(:email_domain_block, allow_with_approval: true, domain: 'example.com') + request.headers['Accept-Language'] = accept_language + post :create, params: { user: { account_attributes: { username: 'test' }, email: 'test@example.com', password: '12345678', password_confirmation: '12345678', agreement: 'true' } } + end + + it 'creates unapproved user and redirects to setup' do + subject + expect(response).to redirect_to auth_setup_path + + user = User.find_by(email: 'test@example.com') + expect(user).to_not be_nil + expect(user.locale).to eq(accept_language) + expect(user.approved).to be(false) + end + end + context 'with Approval-based registrations without invite' do subject do Setting.registrations_mode = 'approved' diff --git a/spec/services/app_sign_up_service_spec.rb b/spec/services/app_sign_up_service_spec.rb index 0adb473f1..86e64dab2 100644 --- a/spec/services/app_sign_up_service_spec.rb +++ b/spec/services/app_sign_up_service_spec.rb @@ -27,6 +27,27 @@ RSpec.describe AppSignUpService, type: :service do end end + context 'when the email address requires approval' do + before do + Setting.registrations_mode = 'open' + Fabricate(:email_domain_block, allow_with_approval: true, domain: 'email.com') + end + + it 'creates an unapproved user', :aggregate_failures do + access_token = subject.call(app, remote_ip, params) + expect(access_token).to_not be_nil + expect(access_token.scopes.to_s).to eq 'read write' + + user = User.find_by(id: access_token.resource_owner_id) + expect(user).to_not be_nil + expect(user.confirmed?).to be false + expect(user.approved?).to be false + + expect(user.account).to_not be_nil + expect(user.invite_request).to be_nil + end + end + context 'when registrations are closed' do before do Setting.registrations_mode = 'none'