From e621c1c44c49dbed58e6961ad868d356f0534490 Mon Sep 17 00:00:00 2001 From: Claire Date: Mon, 15 Jan 2024 18:10:57 +0100 Subject: [PATCH 1/2] Fix registrations not checking MX records for email domain blocks requiring approval (#28608) --- app/models/user.rb | 25 +++++++++-- .../auth/registrations_controller_spec.rb | 41 ++++++++++++++++++- spec/services/app_sign_up_service_spec.rb | 33 +++++++++++++++ 3 files changed, 94 insertions(+), 5 deletions(-) diff --git a/app/models/user.rb b/app/models/user.rb index f5ed3fa462..4bc19b16a6 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -149,6 +149,10 @@ class User < ApplicationRecord end end + def self.skip_mx_check? + Rails.env.local? + end + def role if role_id.nil? UserRole.everyone @@ -436,9 +440,24 @@ class User < ApplicationRecord end def sign_up_email_requires_approval? - return false unless email.present? || unconfirmed_email.present? + return false if email.blank? - EmailDomainBlock.requires_approval?(email.presence || unconfirmed_email, attempt_ip: sign_up_ip) + _, domain = email.split('@', 2) + return false if domain.blank? + + records = [] + + # Doing this conditionally is not very satisfying, but this is consistent + # with the MX records validations we do and keeps the specs tractable. + unless self.class.skip_mx_check? + Resolv::DNS.open do |dns| + dns.timeouts = 5 + + records = dns.getresources(domain, Resolv::DNS::Resource::IN::MX).to_a.map { |e| e.exchange.to_s }.compact_blank + end + end + + EmailDomainBlock.requires_approval?(records + [domain], attempt_ip: sign_up_ip) end def open_registrations? @@ -489,7 +508,7 @@ class User < ApplicationRecord end def validate_email_dns? - email_changed? && !external? && !Rails.env.local? + email_changed? && !external? && !self.class.skip_mx_check? end def validate_role_elevation diff --git a/spec/controllers/auth/registrations_controller_spec.rb b/spec/controllers/auth/registrations_controller_spec.rb index bd1c616595..0b7f02f590 100644 --- a/spec/controllers/auth/registrations_controller_spec.rb +++ b/spec/controllers/auth/registrations_controller_spec.rb @@ -137,12 +137,49 @@ RSpec.describe Auth::RegistrationsController do 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 + before do + Setting.registrations_mode = 'open' + Fabricate(:email_domain_block, allow_with_approval: true, domain: 'example.com') + 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 'when user has an email address requiring approval through a MX record' do + subject do + 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 + + before do + Setting.registrations_mode = 'open' + Fabricate(:email_domain_block, allow_with_approval: true, domain: 'mail.example.com') + allow(User).to receive(:skip_mx_check?).and_return(false) + + resolver = instance_double(Resolv::DNS, :timeouts= => nil) + + allow(resolver).to receive(:getresources) + .with('example.com', Resolv::DNS::Resource::IN::MX) + .and_return([instance_double(Resolv::DNS::Resource::MX, exchange: 'mail.example.com')]) + allow(resolver).to receive(:getresources).with('example.com', Resolv::DNS::Resource::IN::A).and_return([]) + allow(resolver).to receive(:getresources).with('example.com', Resolv::DNS::Resource::IN::AAAA).and_return([]) + allow(resolver).to receive(:getresources).with('mail.example.com', Resolv::DNS::Resource::IN::A).and_return([instance_double(Resolv::DNS::Resource::IN::A, address: '2.3.4.5')]) + allow(resolver).to receive(:getresources).with('mail.example.com', Resolv::DNS::Resource::IN::AAAA).and_return([instance_double(Resolv::DNS::Resource::IN::AAAA, address: 'fd00::2')]) + allow(Resolv::DNS).to receive(:open).and_yield(resolver) + end + it 'creates unapproved user and redirects to setup' do subject expect(response).to redirect_to auth_setup_path diff --git a/spec/services/app_sign_up_service_spec.rb b/spec/services/app_sign_up_service_spec.rb index 86e64dab21..b37b6da1f0 100644 --- a/spec/services/app_sign_up_service_spec.rb +++ b/spec/services/app_sign_up_service_spec.rb @@ -48,6 +48,39 @@ RSpec.describe AppSignUpService, type: :service do end end + context 'when the email address requires approval through MX records' do + before do + Setting.registrations_mode = 'open' + Fabricate(:email_domain_block, allow_with_approval: true, domain: 'smtp.email.com') + allow(User).to receive(:skip_mx_check?).and_return(false) + + resolver = instance_double(Resolv::DNS, :timeouts= => nil) + + allow(resolver).to receive(:getresources) + .with('email.com', Resolv::DNS::Resource::IN::MX) + .and_return([instance_double(Resolv::DNS::Resource::MX, exchange: 'smtp.email.com')]) + allow(resolver).to receive(:getresources).with('email.com', Resolv::DNS::Resource::IN::A).and_return([]) + allow(resolver).to receive(:getresources).with('email.com', Resolv::DNS::Resource::IN::AAAA).and_return([]) + allow(resolver).to receive(:getresources).with('smtp.email.com', Resolv::DNS::Resource::IN::A).and_return([instance_double(Resolv::DNS::Resource::IN::A, address: '2.3.4.5')]) + allow(resolver).to receive(:getresources).with('smtp.email.com', Resolv::DNS::Resource::IN::AAAA).and_return([instance_double(Resolv::DNS::Resource::IN::AAAA, address: 'fd00::2')]) + allow(Resolv::DNS).to receive(:open).and_yield(resolver) + 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' From 98b5f85f10a3af50a54fcd79e09fc9fd88f774fa Mon Sep 17 00:00:00 2001 From: Claire Date: Mon, 15 Jan 2024 19:04:58 +0100 Subject: [PATCH 2/2] Rename and refactor `User#confirm!` to `User#mark_email_as_confirmed!` (#28735) --- .../admin/confirmations_controller.rb | 2 +- app/models/concerns/user/omniauthable.rb | 2 +- app/models/user.rb | 54 +++++++++---------- lib/mastodon/cli/accounts.rb | 2 +- spec/features/oauth_spec.rb | 2 +- spec/models/user_spec.rb | 4 +- spec/policies/user_policy_spec.rb | 2 +- 7 files changed, 33 insertions(+), 35 deletions(-) diff --git a/app/controllers/admin/confirmations_controller.rb b/app/controllers/admin/confirmations_controller.rb index 6f4e426797..7ccf5c9012 100644 --- a/app/controllers/admin/confirmations_controller.rb +++ b/app/controllers/admin/confirmations_controller.rb @@ -7,7 +7,7 @@ module Admin def create authorize @user, :confirm? - @user.confirm! + @user.mark_email_as_confirmed! log_action :confirm, @user redirect_to admin_accounts_path end diff --git a/app/models/concerns/user/omniauthable.rb b/app/models/concerns/user/omniauthable.rb index 6d1d1b8cc3..113bfda230 100644 --- a/app/models/concerns/user/omniauthable.rb +++ b/app/models/concerns/user/omniauthable.rb @@ -61,7 +61,7 @@ module User::Omniauthable user.account.avatar_remote_url = nil end - user.confirm! if email_is_verified + user.mark_email_as_confirmed! if email_is_verified user.save! user end diff --git a/app/models/user.rb b/app/models/user.rb index 4bc19b16a6..5c90af56d2 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -190,37 +190,16 @@ class User < ApplicationRecord end def confirm - new_user = !confirmed? - self.approved = true if grant_approval_on_confirmation? - - super - - if new_user - # Avoid extremely unlikely race condition when approving and confirming - # the user at the same time - reload unless approved? - - if approved? - prepare_new_user! - else - notify_staff_about_pending_account! - end + wrap_email_confirmation do + super end end - def confirm! - new_user = !confirmed? - self.approved = true if grant_approval_on_confirmation? - - skip_confirmation! - save! - - if new_user - # Avoid extremely unlikely race condition when approving and confirming - # the user at the same time - reload unless approved? - - prepare_new_user! if approved? + # Mark current email as confirmed, bypassing Devise + def mark_email_as_confirmed! + wrap_email_confirmation do + skip_confirmation! + save! end end @@ -435,6 +414,25 @@ class User < ApplicationRecord open_registrations? && !sign_up_from_ip_requires_approval? && !sign_up_email_requires_approval? end + def wrap_email_confirmation + new_user = !confirmed? + self.approved = true if grant_approval_on_confirmation? + + yield + + if new_user + # Avoid extremely unlikely race condition when approving and confirming + # the user at the same time + reload unless approved? + + if approved? + prepare_new_user! + else + notify_staff_about_pending_account! + end + end + end + def sign_up_from_ip_requires_approval? !sign_up_ip.nil? && IpBlock.where(severity: :sign_up_requires_approval).where('ip >>= ?', sign_up_ip.to_s).exists? end diff --git a/lib/mastodon/cli/accounts.rb b/lib/mastodon/cli/accounts.rb index 4146753035..9dc65c1477 100644 --- a/lib/mastodon/cli/accounts.rb +++ b/lib/mastodon/cli/accounts.rb @@ -105,7 +105,7 @@ module Mastodon::CLI if user.save if options[:confirmed] user.confirmed_at = nil - user.confirm! + user.mark_email_as_confirmed! end user.approve! if options[:approve] diff --git a/spec/features/oauth_spec.rb b/spec/features/oauth_spec.rb index 967956cc8e..70356784d2 100644 --- a/spec/features/oauth_spec.rb +++ b/spec/features/oauth_spec.rb @@ -49,7 +49,7 @@ describe 'Using OAuth from an external app' do let(:user) { Fabricate(:user, email: email, password: password) } before do - user.confirm! + user.mark_email_as_confirmed! user.approve! end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 7f68671df4..213022e830 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -461,12 +461,12 @@ RSpec.describe User do end end - describe '#confirm!' do + describe '#mark_email_as_confirmed!' do subject(:user) { Fabricate(:user, confirmed_at: confirmed_at) } before do ActionMailer::Base.deliveries.clear - user.confirm! + user.mark_email_as_confirmed! end after { ActionMailer::Base.deliveries.clear } diff --git a/spec/policies/user_policy_spec.rb b/spec/policies/user_policy_spec.rb index fa476a9fc3..7854547d26 100644 --- a/spec/policies/user_policy_spec.rb +++ b/spec/policies/user_policy_spec.rb @@ -64,7 +64,7 @@ RSpec.describe UserPolicy do context 'when record.confirmed?' do it 'denies' do - john.user.confirm! + john.user.mark_email_as_confirmed! expect(subject).to_not permit(admin, john.user) end end