Merge commit '98b5f85f10a3af50a54fcd79e09fc9fd88f774fa' into glitch-soc/merge-upstream

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

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

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
@@ -186,37 +190,16 @@ class User < ApplicationRecord
   end
 
   def confirm
-    new_user      = !confirmed?
+    wrap_email_confirmation do
-    self.approved = true if grant_approval_on_confirmation?
+      super
-
-    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
     end
   end
 
-  def confirm!
+  # Mark current email as confirmed, bypassing Devise
-    new_user      = !confirmed?
+  def mark_email_as_confirmed!
-    self.approved = true if grant_approval_on_confirmation?
+    wrap_email_confirmation do
-
+      skip_confirmation!
-    skip_confirmation!
+      save!
-    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?
     end
   end
 
@@ -431,14 +414,48 @@ 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
 
   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 +506,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

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]

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

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
 

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 }

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

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'