Merge branch 'unconfirm-wrongfully-verified-email-records' into 'master'

Unconfirm wrongfully verified email records

See merge request gitlab-org/gitlab!35492

app/views/devise/mailer/_confirmation_instructions_secondary.html.haml

 #content
-  = email_default_heading("#{sanitize_name(@resource.user.name)}, you've added an additional email!")
+  = email_default_heading("#{sanitize_name(@resource.user.name)}, confirm your email address now!")
   %p Click the link below to confirm your email address (#{@resource.email})
   #cta
     = link_to 'Confirm your email address', confirmation_url(@resource, confirmation_token: @token)

app/views/devise/mailer/_confirmation_instructions_secondary.text.erb

-<%= @resource.user.name %>, you've added an additional email!
+<%= @resource.user.name %>, confirm your email address now!
 
 Use the link below to confirm your email address (<%= @resource.email %>)
 

changelogs/unreleased/unconfirm-wrongfully-verified-email-records.yml

+---
+title: Unconfirm wrongfully verified email addresses and user accounts
+merge_request: 35492
+author:
+type: security

db/post_migrate/20200615111857_unconfirm_wrongfully_verified_emails.rb

+# frozen_string_literal: true
+
+class UnconfirmWrongfullyVerifiedEmails < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  INTERVAL = 5.minutes.to_i
+  BATCH_SIZE = 1000
+  MIGRATION = 'WrongfullyConfirmedEmailUnconfirmer'
+  EMAIL_INDEX_NAME = 'tmp_index_for_email_unconfirmation_migration'
+
+  class Email < ActiveRecord::Base
+    include EachBatch
+  end
+
+  def up
+    add_concurrent_index :emails, :id, where: 'confirmed_at IS NOT NULL', name: EMAIL_INDEX_NAME
+
+    queue_background_migration_jobs_by_range_at_intervals(Email,
+                                                          MIGRATION,
+                                                          INTERVAL,
+                                                          batch_size: BATCH_SIZE)
+  end
+
+  def down
+    remove_concurrent_index_by_name(:emails, EMAIL_INDEX_NAME)
+  end
+end

db/structure.sql

@@ -20493,6 +20493,8 @@ CREATE INDEX tmp_index_ci_pipelines_lock_version ON public.ci_pipelines USING bt
 
 CREATE INDEX tmp_index_ci_stages_lock_version ON public.ci_stages USING btree (id) WHERE (lock_version IS NULL);
 
+CREATE INDEX tmp_index_for_email_unconfirmation_migration ON public.emails USING btree (id) WHERE (confirmed_at IS NOT NULL);
+
 CREATE UNIQUE INDEX unique_merge_request_metrics_by_merge_request_id ON public.merge_request_metrics USING btree (merge_request_id);
 
 CREATE UNIQUE INDEX users_security_dashboard_projects_unique_index ON public.users_security_dashboard_projects USING btree (project_id, user_id);
@@ -23538,6 +23540,7 @@ COPY "schema_migrations" (version) FROM STDIN;
 20200610130002
 20200613104045
 20200615083635
+20200615111857
 20200615121217
 20200615123055
 20200615193524

lib/gitlab/background_migration/mailers/unconfirm_mailer.rb

+# frozen_string_literal: true
+# rubocop:disable Style/Documentation
+
+module Gitlab
+  module BackgroundMigration
+    module Mailers
+      class UnconfirmMailer < ::Notify
+        prepend_view_path(File.join(__dir__, 'views'))
+
+        def unconfirm_notification_email(user)
+          @user = user
+          @verification_from_mail = Gitlab.config.gitlab.email_from
+
+          mail(
+            template_path: 'unconfirm_mailer',
+            template_name: 'unconfirm_notification_email',
+            to: @user.notification_email,
+            subject: subject('GitLab email verification request')
+          )
+        end
+      end
+    end
+  end
+end

lib/gitlab/background_migration/mailers/views/unconfirm_mailer/unconfirm_notification_email.html.haml

+-# haml-lint:disable NoPlainNodes
+%p
+  Dear GitLab user,
+
+%p
+  As part of our commitment to keeping GitLab secure, we have identified and addressed a vulnerability in GitLab that allowed some users to bypass the email verification process in a #{link_to("recent security release", "https://about.gitlab.com/releases/2020/05/27/security-release-13-0-1-released", target: '_blank')}.
+
+%p
+  As a precautionary measure, you will need to re-verify some of your account's email addresses before continuing to use GitLab. Sorry for the inconvenience!
+
+%p
+  We have already sent the re-verification email with a subject line of "Confirmation instructions" from #{@verification_from_mail}. Please feel free to contribute any questions or comments to #{link_to("this issue", "https://gitlab.com/gitlab-com/www-gitlab-com/-/issues/7942", target: '_blank')}.
+
+%p
+  If you are not "#{@user.username}", please #{link_to 'report this to our administrator', new_abuse_report_url(user_id: @user.id)}
+
+%p
+  Thank you for being a GitLab user!
+-# haml-lint:enable NoPlainNodes

lib/gitlab/background_migration/mailers/views/unconfirm_mailer/unconfirm_notification_email.text.erb

+Dear GitLab user,
+
+As part of our commitment to keeping GitLab secure, we have identified and addressed a vulnerability in GitLab that allowed some users to bypass the email verification process in a recent security release. 
+
+Security release: https://about.gitlab.com/releases/2020/05/27/security-release-13-0-1-released
+
+As a precautionary measure, you will need to re-verify some of your account's email addresses before continuing to use GitLab. Sorry for the inconvenience!
+
+We have already sent the re-verification email with a subject line of "Confirmation instructions" from <%= @verification_from_mail %>. 
+Please feel free to contribute any questions or comments to this issue: https://gitlab.com/gitlab-com/www-gitlab-com/-/issues/7942
+
+If you are not "<%= @user.username %>", please report this to our administrator. Report link: <%= new_abuse_report_url(user_id: @user.id) %>
+
+Thank you for being a GitLab user!

lib/gitlab/background_migration/wrongfully_confirmed_email_unconfirmer.rb

+# frozen_string_literal: true
+# rubocop:disable Style/Documentation
+
+module Gitlab
+  module BackgroundMigration
+    class WrongfullyConfirmedEmailUnconfirmer
+      class UserModel < ActiveRecord::Base
+        alias_method :reset, :reload
+
+        self.table_name = 'users'
+
+        scope :active, -> { where(state: 'active', user_type: nil) } # only humans, skip bots
+
+        devise :confirmable
+      end
+
+      class EmailModel < ActiveRecord::Base
+        alias_method :reset, :reload
+
+        self.table_name = 'emails'
+
+        belongs_to :user
+
+        devise :confirmable
+
+        def self.wrongfully_confirmed_emails(start_id, stop_id)
+          joins(:user)
+            .merge(UserModel.active)
+            .where(id: (start_id..stop_id))
+            .where('emails.confirmed_at IS NOT NULL')
+            .where('emails.confirmed_at = users.confirmed_at')
+            .where('emails.email <> users.email')
+        end
+      end
+
+      def perform(start_id, stop_id)
+        email_records = EmailModel
+          .wrongfully_confirmed_emails(start_id, stop_id)
+          .to_a
+
+        user_ids = email_records.map(&:user_id).uniq
+
+        ActiveRecord::Base.transaction do
+          update_email_records(start_id, stop_id)
+          update_user_records(user_ids)
+        end
+
+        # Refind the records with the "real" Email model so devise will notice that the user / email is unconfirmed
+        unconfirmed_email_records = ::Email.where(id: email_records.map(&:id))
+        ActiveRecord::Associations::Preloader.new.preload(unconfirmed_email_records, [:user])
+
+        send_emails(unconfirmed_email_records)
+      end
+
+      private
+
+      def update_email_records(start_id, stop_id)
+        EmailModel.connection.execute <<-SQL
+          WITH md5_strings as (
+            #{email_query_for_update(start_id, stop_id).to_sql}
+          )
+          UPDATE #{EmailModel.connection.quote_table_name(EmailModel.table_name)}
+          SET confirmed_at = NULL,
+            confirmation_token = md5_strings.md5_string,
+            confirmation_sent_at = NOW()
+          FROM md5_strings
+          WHERE id = md5_strings.email_id
+        SQL
+      end
+
+      def update_user_records(user_ids)
+        UserModel
+          .where(id: user_ids)
+          .update_all("confirmed_at = NULL, confirmation_sent_at = NOW(), confirmation_token=md5(users.id::varchar || users.created_at || users.encrypted_password || '#{Integer(Time.now.to_i)}')")
+      end
+
+      def email_query_for_update(start_id, stop_id)
+        EmailModel
+          .wrongfully_confirmed_emails(start_id, stop_id)
+          .select('emails.id as email_id', "md5(emails.id::varchar || emails.created_at || users.encrypted_password || '#{Integer(Time.now.to_i)}') as md5_string")
+      end
+
+      def send_emails(email_records)
+        email_records.each do |email|
+          DeviseMailer.confirmation_instructions(email, email.confirmation_token).deliver_later
+        end
+
+        user_records = email_records.map(&:user).uniq
+
+        user_records.each do |user|
+          DeviseMailer.confirmation_instructions(user, user.confirmation_token).deliver_later
+          Gitlab::BackgroundMigration::Mailers::UnconfirmMailer.unconfirm_notification_email(user).deliver_later
+        end
+      end
+    end
+  end
+end

spec/lib/gitlab/background_migration/mailers/unconfirm_mailer_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::BackgroundMigration::Mailers::UnconfirmMailer do
+  let(:user) { User.new(id: 1111) }
+  let(:subject) { described_class.unconfirm_notification_email(user) }
+
+  it 'contains abuse report url' do
+    expect(subject.body.encoded).to include(Rails.application.routes.url_helpers.new_abuse_report_url(user_id: user.id))
+  end
+end

spec/lib/gitlab/background_migration/wrongfully_confirmed_email_unconfirmer_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::BackgroundMigration::WrongfullyConfirmedEmailUnconfirmer, schema: 20200615111857 do
+  let(:users) { table(:users) }
+  let(:emails) { table(:emails) }
+  let(:confirmed_at_2_days_ago) { 2.days.ago }
+  let(:confirmed_at_3_days_ago) { 3.days.ago }
+  let(:one_year_ago) { 1.year.ago }
+
+  let!(:user_needs_migration_1) { users.create!(name: 'user1', email: 'test1@test.com', state: 'active', projects_limit: 1, confirmed_at: confirmed_at_2_days_ago, confirmation_sent_at: one_year_ago) }
+  let!(:user_needs_migration_2) { users.create!(name: 'user2', email: 'test2@test.com', state: 'active', projects_limit: 1, confirmed_at: confirmed_at_3_days_ago, confirmation_sent_at: one_year_ago) }
+  let!(:user_does_not_need_migration) { users.create!(name: 'user3', email: 'test3@test.com', state: 'active', projects_limit: 1) }
+  let!(:inactive_user) { users.create!(name: 'user4', email: 'test4@test.com', state: 'blocked', projects_limit: 1, confirmed_at: confirmed_at_3_days_ago, confirmation_sent_at: one_year_ago) }
+  let!(:alert_bot_user) { users.create!(name: 'user5', email: 'test5@test.com', state: 'active', user_type: 2, projects_limit: 1, confirmed_at: confirmed_at_3_days_ago, confirmation_sent_at: one_year_ago) }
+
+  let!(:bad_email_1) { emails.create!(user_id: user_needs_migration_1.id, email: 'other1@test.com', confirmed_at: confirmed_at_2_days_ago, confirmation_sent_at: one_year_ago) }
+  let!(:bad_email_2) { emails.create!(user_id: user_needs_migration_2.id, email: 'other2@test.com', confirmed_at: confirmed_at_3_days_ago, confirmation_sent_at: one_year_ago) }
+  let!(:bad_email_3_inactive_user) { emails.create!(user_id: inactive_user.id, email: 'other-inactive@test.com', confirmed_at: confirmed_at_3_days_ago, confirmation_sent_at: one_year_ago) }
+  let!(:bad_email_4_bot_user) { emails.create!(user_id: alert_bot_user.id, email: 'other-bot@test.com', confirmed_at: confirmed_at_3_days_ago, confirmation_sent_at: one_year_ago) }
+
+  let!(:good_email_1) { emails.create!(user_id: user_needs_migration_2.id, email: 'other3@test.com', confirmed_at: confirmed_at_2_days_ago, confirmation_sent_at: one_year_ago) }
+  let!(:good_email_2) { emails.create!(user_id: user_needs_migration_2.id, email: 'other4@test.com', confirmed_at: nil) }
+  let!(:good_email_3) { emails.create!(user_id: user_does_not_need_migration.id, email: 'other5@test.com', confirmed_at: confirmed_at_2_days_ago, confirmation_sent_at: one_year_ago) }
+
+  subject do
+    email_ids = [bad_email_1, bad_email_2, good_email_1, good_email_2, good_email_3].map(&:id)
+
+    described_class.new.perform(email_ids.min, email_ids.max)
+  end
+
+  it 'does not change irrelevant email records' do
+    subject
+
+    expect(good_email_1.reload.confirmed_at).to be_within(1.second).of(confirmed_at_2_days_ago)
+    expect(good_email_2.reload.confirmed_at).to be_nil
+    expect(good_email_3.reload.confirmed_at).to be_within(1.second).of(confirmed_at_2_days_ago)
+
+    expect(bad_email_3_inactive_user.reload.confirmed_at).to be_within(1.second).of(confirmed_at_3_days_ago)
+    expect(bad_email_4_bot_user.reload.confirmed_at).to be_within(1.second).of(confirmed_at_3_days_ago)
+
+    expect(good_email_1.reload.confirmation_sent_at).to be_within(1.second).of(one_year_ago)
+    expect(good_email_2.reload.confirmation_sent_at).to be_nil
+    expect(good_email_3.reload.confirmation_sent_at).to be_within(1.second).of(one_year_ago)
+
+    expect(bad_email_3_inactive_user.reload.confirmation_sent_at).to be_within(1.second).of(one_year_ago)
+    expect(bad_email_4_bot_user.reload.confirmation_sent_at).to be_within(1.second).of(one_year_ago)
+  end
+
+  it 'does not change irrelevant user records' do
+    subject
+
+    expect(user_does_not_need_migration.reload.confirmed_at).to be_nil
+    expect(inactive_user.reload.confirmed_at).to be_within(1.second).of(confirmed_at_3_days_ago)
+    expect(alert_bot_user.reload.confirmed_at).to be_within(1.second).of(confirmed_at_3_days_ago)
+
+    expect(user_does_not_need_migration.reload.confirmation_sent_at).to be_nil
+    expect(inactive_user.reload.confirmation_sent_at).to be_within(1.second).of(one_year_ago)
+    expect(alert_bot_user.reload.confirmation_sent_at).to be_within(1.second).of(one_year_ago)
+  end
+
+  it 'updates confirmation_sent_at column' do
+    subject
+
+    expect(user_needs_migration_1.reload.confirmation_sent_at).to be_within(1.minute).of(Time.now)
+    expect(user_needs_migration_2.reload.confirmation_sent_at).to be_within(1.minute).of(Time.now)
+
+    expect(bad_email_1.reload.confirmation_sent_at).to be_within(1.minute).of(Time.now)
+    expect(bad_email_2.reload.confirmation_sent_at).to be_within(1.minute).of(Time.now)
+  end
+
+  it 'unconfirms bad email records' do
+    subject
+
+    expect(bad_email_1.reload.confirmed_at).to be_nil
+    expect(bad_email_2.reload.confirmed_at).to be_nil
+
+    expect(bad_email_1.reload.confirmation_token).not_to be_nil
+    expect(bad_email_2.reload.confirmation_token).not_to be_nil
+  end
+
+  it 'unconfirms user records' do
+    subject
+
+    expect(user_needs_migration_1.reload.confirmed_at).to be_nil
+    expect(user_needs_migration_2.reload.confirmed_at).to be_nil
+
+    expect(user_needs_migration_1.reload.confirmation_token).not_to be_nil
+    expect(user_needs_migration_2.reload.confirmation_token).not_to be_nil
+  end
+
+  context 'enqueued jobs' do
+    let(:user_1_gid) { User.find(user_needs_migration_1.id).to_gid.to_s }
+    let(:user_2_gid) { User.find(user_needs_migration_2.id).to_gid.to_s }
+
+    let(:email_1_gid) { Email.find(bad_email_1.id).to_gid.to_s }
+    let(:email_2_gid) { Email.find(bad_email_2.id).to_gid.to_s }
+
+    it 'enqueues the email confirmation and the unconfirm notification mailer jobs' do
+      subject
+
+      expect(enqueued_jobs.size).to eq(6)
+
+      expected_job_arguments = [
+        [
+          'DeviseMailer',
+          'confirmation_instructions',
+          'deliver_now',
+          { "_aj_globalid" => email_1_gid },
+          bad_email_1.reload.confirmation_token
+        ],
+        [
+          'DeviseMailer',
+          'confirmation_instructions',
+          'deliver_now',
+          { "_aj_globalid" => email_2_gid },
+          bad_email_2.reload.confirmation_token
+        ],
+        [
+          'DeviseMailer',
+          'confirmation_instructions',
+          'deliver_now',
+          { "_aj_globalid" => user_1_gid },
+          user_needs_migration_1.reload.confirmation_token
+        ],
+        [
+          'Gitlab::BackgroundMigration::Mailers::UnconfirmMailer',
+          'unconfirm_notification_email',
+          'deliver_now',
+          { "_aj_globalid" => user_1_gid }
+        ],
+        [
+          'DeviseMailer',
+          'confirmation_instructions',
+          'deliver_now',
+          { "_aj_globalid" => user_2_gid },
+          user_needs_migration_2.reload.confirmation_token
+        ],
+        [
+          'Gitlab::BackgroundMigration::Mailers::UnconfirmMailer',
+          'unconfirm_notification_email',
+          'deliver_now',
+          { "_aj_globalid" => user_2_gid }
+        ]
+      ]
+
+      all_job_arguments = enqueued_jobs.map { |job| job["arguments"] }
+
+      expected_job_arguments.each do |job_arguments|
+        expect(all_job_arguments).to include(job_arguments)
+      end
+    end
+  end
+end

spec/migrations/unconfirm_wrongfully_verified_emails_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require Rails.root.join('db', 'post_migrate', '20200615111857_unconfirm_wrongfully_verified_emails.rb')
+
+RSpec.describe UnconfirmWrongfullyVerifiedEmails do
+  before do
+    user = table(:users).create!(name: 'user1', email: 'test1@test.com', projects_limit: 1)
+    table(:emails).create!(email: 'test2@test.com', user_id: user.id)
+  end
+
+  it 'enqueues WrongullyConfirmedEmailUnconfirmer job' do
+    Sidekiq::Testing.fake! do
+      migrate!
+
+      jobs = BackgroundMigrationWorker.jobs
+      expect(jobs.size).to eq(1)
+      expect(jobs.first["args"].first).to eq(Gitlab::BackgroundMigration::WrongfullyConfirmedEmailUnconfirmer.name.demodulize)
+    end
+  end
+end