diff options
| -rw-r--r-- | app/models/user.rb | 9 | ||||
| -rw-r--r-- | changelogs/unreleased/security-fix-email-confirmation-bug.yml | 5 | ||||
| -rw-r--r-- | spec/factories/users.rb | 4 | ||||
| -rw-r--r-- | spec/models/user_spec.rb | 102 |
4 files changed, 117 insertions, 3 deletions
diff --git a/app/models/user.rb b/app/models/user.rb index b2d3978551e..81316f81818 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -237,9 +237,10 @@ class User < ApplicationRecord if previous_changes.key?('email') # Grab previous_email here since previous_changes changes after # #update_emails_with_primary_email and #update_notification_email are called + previous_confirmed_at = previous_changes.key?('confirmed_at') ? previous_changes['confirmed_at'][0] : confirmed_at previous_email = previous_changes[:email][0] - update_emails_with_primary_email(previous_email) + update_emails_with_primary_email(previous_confirmed_at, previous_email) update_invalid_gpg_signatures if previous_email == notification_email @@ -811,13 +812,15 @@ class User < ApplicationRecord # By using an `after_commit` instead of `after_update`, we avoid the recursive callback # scenario, though it then requires us to use the `previous_changes` hash # rubocop: disable CodeReuse/ServiceClass - def update_emails_with_primary_email(previous_email) + def update_emails_with_primary_email(previous_confirmed_at, previous_email) primary_email_record = emails.find_by(email: email) Emails::DestroyService.new(self, user: self).execute(primary_email_record) if primary_email_record # the original primary email was confirmed, and we want that to carry over. We don't # have access to the original confirmation values at this point, so just set confirmed_at - Emails::CreateService.new(self, user: self, email: previous_email).execute(confirmed_at: confirmed_at) + Emails::CreateService.new(self, user: self, email: previous_email).execute(confirmed_at: previous_confirmed_at) + + update_columns(confirmed_at: primary_email_record.confirmed_at) if primary_email_record&.confirmed_at end # rubocop: enable CodeReuse/ServiceClass diff --git a/changelogs/unreleased/security-fix-email-confirmation-bug.yml b/changelogs/unreleased/security-fix-email-confirmation-bug.yml new file mode 100644 index 00000000000..ce66a255616 --- /dev/null +++ b/changelogs/unreleased/security-fix-email-confirmation-bug.yml @@ -0,0 +1,5 @@ +--- +title: Fix confirming unverified emails with soft email confirmation flow enabled +merge_request: +author: +type: security diff --git a/spec/factories/users.rb b/spec/factories/users.rb index 2f5cc404143..7e121b10632 100644 --- a/spec/factories/users.rb +++ b/spec/factories/users.rb @@ -48,6 +48,10 @@ FactoryBot.define do after(:build) { |user, _| user.block! } end + trait :unconfirmed do + confirmed_at { nil } + end + trait :with_avatar do avatar { fixture_file_upload('spec/fixtures/dk.png') } end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 94a3f6bafea..abc52263298 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -916,6 +916,108 @@ describe User do expect(@user.emails.count).to eq 1 expect(@user.emails.first.confirmed_at).not_to eq nil end + + context 'when the first email was unconfirmed and the second email gets confirmed' do + let_it_be(:user) { create(:user, :unconfirmed, email: 'should-be-unconfirmed@test.com') } + + before do + user.update!(email: 'should-be-confirmed@test.com') + user.confirm + end + + it 'updates user.email' do + expect(user.email).to eq('should-be-confirmed@test.com') + end + + it 'confirms user.email' do + expect(user).to be_confirmed + end + + it 'keeps the unconfirmed email unconfirmed' do + email = user.emails.first + + expect(email.email).to eq('should-be-unconfirmed@test.com') + expect(email).not_to be_confirmed + end + + it 'has only one email association' do + expect(user.emails.size).to be(1) + end + end + end + + context 'when an existing email record is set as primary' do + let(:user) { create(:user, email: 'confirmed@test.com') } + + context 'when it is unconfirmed' do + let(:originally_unconfirmed_email) { 'should-stay-unconfirmed@test.com' } + + before do + user.emails << create(:email, email: originally_unconfirmed_email, confirmed_at: nil) + + user.update!(email: originally_unconfirmed_email) + end + + it 'keeps the user confirmed' do + expect(user).to be_confirmed + end + + it 'keeps the original email' do + expect(user.email).to eq('confirmed@test.com') + end + + context 'when the email gets confirmed' do + before do + user.confirm + end + + it 'keeps the user confirmed' do + expect(user).to be_confirmed + end + + it 'updates the email' do + expect(user.email).to eq(originally_unconfirmed_email) + end + end + end + + context 'when it is confirmed' do + let!(:old_confirmed_email) { user.email } + let(:confirmed_email) { 'already-confirmed@test.com' } + + before do + user.emails << create(:email, :confirmed, email: confirmed_email) + + user.update!(email: confirmed_email) + end + + it 'keeps the user confirmed' do + expect(user).to be_confirmed + end + + it 'updates the email' do + expect(user.email).to eq(confirmed_email) + end + + it 'moves the old email' do + email = user.reload.emails.first + + expect(email.email).to eq(old_confirmed_email) + expect(email).to be_confirmed + end + end + end + + context 'when unconfirmed user deletes a confirmed additional email' do + let(:user) { create(:user, :unconfirmed) } + + before do + user.emails << create(:email, :confirmed) + end + + it 'does not affect the confirmed status' do + expect { user.emails.confirmed.destroy_all }.not_to change { user.confirmed? } # rubocop: disable Cop/DestroyAll + end end describe '#update_notification_email' do |