From 6883fddb19b1319a378aa5dc2034670c3b27ce39 Mon Sep 17 00:00:00 2001 From: Claire Date: Tue, 24 Jan 2023 19:40:21 +0100 Subject: [PATCH] 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 | 28 ++++++-- spec/models/user_spec.rb | 134 +++++++++++++++++++++++++++++++++++++-- 2 files changed, 152 insertions(+), 10 deletions(-) diff --git a/app/models/user.rb b/app/models/user.rb index 2a42ffaa16..d40044da38 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -195,10 +195,16 @@ class User < ApplicationRecord super - if new_user && approved? - prepare_new_user! - elsif new_user - notify_staff_about_pending_account! + 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 @@ -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? diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index a7da31e606..4b3d6101fd 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -142,10 +142,136 @@ RSpec.describe User, type: :model do end describe '#confirm' do - it 'sets email to unconfirmed_email' do - user = Fabricate.build(:user, confirmed_at: Time.now.utc, unconfirmed_email: 'new-email@example.com') - user.confirm - expect(user.email).to eq 'new-email@example.com' + 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 + 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(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