Fix account activation being triggered before email confirmation (#23245)

* Add tests

* Fix account activation being triggered before email confirmation

Fixes #23098

app/models/user.rb

@@ -195,12 +195,18 @@ class User < ApplicationRecord
 
     super
 
-    if new_user && approved?
+    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!
-    elsif new_user
+      else
         notify_staff_about_pending_account!
       end
     end
+  end
 
   def confirm!
     new_user      = !confirmed?
@@ -209,7 +215,13 @@ class User < ApplicationRecord
     skip_confirmation!
     save!
 
-    prepare_new_user! if new_user && approved?
+    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
 
   def update_sign_in!(new_sign_in: false)
@@ -260,7 +272,11 @@ class User < ApplicationRecord
     return if approved?
 
     update!(approved: true)
-    prepare_new_user!
+
+    # Avoid extremely unlikely race condition when approving and confirming
+    # the user at the same time
+    reload unless confirmed?
+    prepare_new_user! if confirmed?
   end
 
   def otp_enabled?

spec/models/user_spec.rb

@@ -142,10 +142,136 @@ RSpec.describe User, type: :model do
   end
 
   describe '#confirm' do
+    let(:new_email) { 'new-email@example.com' }
+
+    subject { user.confirm }
+
+    before do
+      allow(TriggerWebhookWorker).to receive(:perform_async)
+    end
+
+    context 'when the user is already confirmed' do
+      let!(:user) { Fabricate(:user, confirmed_at: Time.now.utc, approved: true, unconfirmed_email: new_email) }
+
       it 'sets email to unconfirmed_email' do
-      user = Fabricate.build(:user, confirmed_at: Time.now.utc, unconfirmed_email: 'new-email@example.com')
+        expect { subject }.to change { user.reload.email }.to(new_email)
+      end
+
+      it 'does not trigger the account.approved Web Hook' do
+        subject
+        expect(TriggerWebhookWorker).not_to have_received(:perform_async).with('account.approved', 'Account', user.account_id)
+      end
+    end
+
+    context 'when the user is a new user' do
+      let(:user) { Fabricate(:user, confirmed_at: nil, unconfirmed_email: new_email) }
+
+      context 'when the user is already approved' do
+        around(:example) do |example|
+          registrations_mode = Setting.registrations_mode
+          Setting.registrations_mode = 'approved'
+
+          example.run
+
+          Setting.registrations_mode = registrations_mode
+        end
+
+        before do
+          user.approve!
+        end
+
+        it 'sets email to unconfirmed_email' do
+          expect { subject }.to change { user.reload.email }.to(new_email)
+        end
+
+        it 'triggers the account.approved Web Hook' do
           user.confirm
-      expect(user.email).to eq 'new-email@example.com'
+          expect(TriggerWebhookWorker).to have_received(:perform_async).with('account.approved', 'Account', user.account_id).once
+        end
+      end
+
+      context 'when the user does not require explicit approval' do
+        around(:example) do |example|
+          registrations_mode = Setting.registrations_mode
+          Setting.registrations_mode = 'open'
+
+          example.run
+
+          Setting.registrations_mode = registrations_mode
+        end
+
+        it 'sets email to unconfirmed_email' do
+          expect { subject }.to change { user.reload.email }.to(new_email)
+        end
+
+        it 'triggers the account.approved Web Hook' do
+          user.confirm
+          expect(TriggerWebhookWorker).to have_received(:perform_async).with('account.approved', 'Account', user.account_id).once
+        end
+      end
+
+      context 'when the user requires explicit approval but is not approved' do
+        around(:example) do |example|
+          registrations_mode = Setting.registrations_mode
+          Setting.registrations_mode = 'approved'
+
+          example.run
+
+          Setting.registrations_mode = registrations_mode
+        end
+
+        it 'sets email to unconfirmed_email' do
+          expect { subject }.to change { user.reload.email }.to(new_email)
+        end
+
+        it 'does not trigger the account.approved Web Hook' do
+          subject
+          expect(TriggerWebhookWorker).to_not have_received(:perform_async).with('account.approved', 'Account', user.account_id)
+        end
+      end
+    end
+  end
+
+  describe '#approve!' do
+    subject { user.approve! }
+
+    around(:example) do |example|
+      registrations_mode = Setting.registrations_mode
+      Setting.registrations_mode = 'approved'
+
+      example.run
+
+      Setting.registrations_mode = registrations_mode
+    end
+
+    before do
+      allow(TriggerWebhookWorker).to receive(:perform_async)
+    end
+
+    context 'when the user is already confirmed' do
+      let(:user) { Fabricate(:user, confirmed_at: Time.now.utc, approved: false) }
+
+      it 'sets the approved flag' do
+        expect { subject }.to change { user.reload.approved? }.to(true)
+      end
+
+      it 'triggers the account.approved Web Hook' do
+        subject
+        expect(TriggerWebhookWorker).to have_received(:perform_async).with('account.approved', 'Account', user.account_id).once
+      end
+    end
+
+    context 'when the user is not confirmed' do
+      let(:user) { Fabricate(:user, confirmed_at: nil, approved: false) }
+
+      it 'sets the approved flag' do
+        expect { subject }.to change { user.reload.approved? }.to(true)
+      end
+
+      it 'does not trigger the account.approved Web Hook' do
+        subject
+        expect(TriggerWebhookWorker).not_to have_received(:perform_async).with('account.approved', 'Account', user.account_id)
+      end
     end
   end