Merge branch '205383-confidential-issue' into 'master'

Add canonical version of user's primary email addresses

Closes #205383

See merge request gitlab-org/gitlab!27722

app/models/user.rb

@@ -168,6 +168,7 @@ class User < ApplicationRecord
   has_one :user_preference
   has_one :user_detail
   has_one :user_highest_role
+  has_one :user_canonical_email
 
   #
   # Validations

app/models/user_canonical_email.rb

+# frozen_string_literal: true
+
+class UserCanonicalEmail < ApplicationRecord
+  validates :canonical_email, presence: true
+  validates :canonical_email, format: { with: Devise.email_regexp }
+
+  belongs_to :user, inverse_of: :user_canonical_email
+end

app/services/users/build_service.rb

@@ -30,6 +30,8 @@ module Users
 
       build_identity(user)
 
+      Users::UpdateCanonicalEmailService.new(user: user).execute
+
       user
     end
 

app/services/users/update_canonical_email_service.rb

+# frozen_string_literal: true
+
+module Users
+  class UpdateCanonicalEmailService
+    extend ActiveSupport::Concern
+
+    INCLUDED_DOMAINS_PATTERN = [/gmail.com/].freeze
+
+    def initialize(user:)
+      raise ArgumentError.new("Please provide a user") unless user&.is_a?(User)
+
+      @user = user
+    end
+
+    def execute
+      return unless user.email
+      return unless user.email.match? Devise.email_regexp
+
+      canonical_email = canonicalize_email
+
+      unless canonical_email
+        # the canonical email doesn't exist, probably because the domain doesn't match
+        # destroy any UserCanonicalEmail record associated with this user
+        user.user_canonical_email&.delete
+        # nothing else to do here
+        return
+      end
+
+      if user.user_canonical_email
+        # update to the new value
+        user.user_canonical_email.canonical_email = canonical_email
+      else
+        user.build_user_canonical_email(canonical_email: canonical_email)
+      end
+    end
+
+    private
+
+    attr_reader :user
+
+    def canonicalize_email
+      email = user.email
+
+      portions = email.split('@')
+      username = portions.shift
+      rest = portions.join
+
+      regex = Regexp.union(INCLUDED_DOMAINS_PATTERN)
+      return unless regex.match?(rest)
+
+      no_dots = username.tr('.', '')
+      before_plus = no_dots.split('+')[0]
+      "#{before_plus}@#{rest}"
+    end
+  end
+end

app/services/users/update_service.rb

@@ -21,6 +21,7 @@ module Users
       discard_read_only_attributes
       assign_attributes
       assign_identity
+      build_canonical_email
 
       if @user.save(validate: validate) && update_status
         notify_success(user_exists)
@@ -40,6 +41,12 @@ module Users
 
     private
 
+    def build_canonical_email
+      return unless @user.email_changed?
+
+      Users::UpdateCanonicalEmailService.new(user: @user).execute
+    end
+
     def update_status
       return true unless @status_params
 

db/migrate/20200214025454_add_canonical_emails.rb

+# frozen_string_literal: true
+
+class AddCanonicalEmails < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def up
+    with_lock_retries do
+      create_table :user_canonical_emails do |t|
+        t.timestamps_with_timezone
+        t.references :user, index: false, null: false, foreign_key: { on_delete: :cascade }
+        t.string :canonical_email, null: false, index: true # rubocop:disable Migration/AddLimitToStringColumns
+      end
+    end
+
+    add_index :user_canonical_emails, [:user_id, :canonical_email], unique: true
+    add_index :user_canonical_emails, :user_id, unique: true
+  end
+
+  def down
+    with_lock_retries do
+      drop_table(:user_canonical_emails)
+    end
+  end
+end

db/structure.sql

@@ -6110,6 +6110,23 @@ CREATE SEQUENCE public.user_callouts_id_seq
 
 ALTER SEQUENCE public.user_callouts_id_seq OWNED BY public.user_callouts.id;
 
+CREATE TABLE public.user_canonical_emails (
+    id bigint NOT NULL,
+    created_at timestamp with time zone NOT NULL,
+    updated_at timestamp with time zone NOT NULL,
+    user_id bigint NOT NULL,
+    canonical_email character varying NOT NULL
+);
+
+CREATE SEQUENCE public.user_canonical_emails_id_seq
+    START WITH 1
+    INCREMENT BY 1
+    NO MINVALUE
+    NO MAXVALUE
+    CACHE 1;
+
+ALTER SEQUENCE public.user_canonical_emails_id_seq OWNED BY public.user_canonical_emails.id;
+
 CREATE TABLE public.user_custom_attributes (
     id integer NOT NULL,
     created_at timestamp without time zone NOT NULL,
@@ -7302,6 +7319,8 @@ ALTER TABLE ONLY public.user_agent_details ALTER COLUMN id SET DEFAULT nextval('
 
 ALTER TABLE ONLY public.user_callouts ALTER COLUMN id SET DEFAULT nextval('public.user_callouts_id_seq'::regclass);
 
+ALTER TABLE ONLY public.user_canonical_emails ALTER COLUMN id SET DEFAULT nextval('public.user_canonical_emails_id_seq'::regclass);
+
 ALTER TABLE ONLY public.user_custom_attributes ALTER COLUMN id SET DEFAULT nextval('public.user_custom_attributes_id_seq'::regclass);
 
 ALTER TABLE ONLY public.user_details ALTER COLUMN user_id SET DEFAULT nextval('public.user_details_user_id_seq'::regclass);
@@ -8206,6 +8225,9 @@ ALTER TABLE ONLY public.user_agent_details
 ALTER TABLE ONLY public.user_callouts
     ADD CONSTRAINT user_callouts_pkey PRIMARY KEY (id);
 
+ALTER TABLE ONLY public.user_canonical_emails
+    ADD CONSTRAINT user_canonical_emails_pkey PRIMARY KEY (id);
+
 ALTER TABLE ONLY public.user_custom_attributes
     ADD CONSTRAINT user_custom_attributes_pkey PRIMARY KEY (id);
 
@@ -9963,6 +9985,12 @@ CREATE INDEX index_user_callouts_on_user_id ON public.user_callouts USING btree
 
 CREATE UNIQUE INDEX index_user_callouts_on_user_id_and_feature_name ON public.user_callouts USING btree (user_id, feature_name);
 
+CREATE INDEX index_user_canonical_emails_on_canonical_email ON public.user_canonical_emails USING btree (canonical_email);
+
+CREATE UNIQUE INDEX index_user_canonical_emails_on_user_id ON public.user_canonical_emails USING btree (user_id);
+
+CREATE UNIQUE INDEX index_user_canonical_emails_on_user_id_and_canonical_email ON public.user_canonical_emails USING btree (user_id, canonical_email);
+
 CREATE INDEX index_user_custom_attributes_on_key_and_value ON public.user_custom_attributes USING btree (key, value);
 
 CREATE UNIQUE INDEX index_user_custom_attributes_on_user_id_and_key ON public.user_custom_attributes USING btree (user_id, key);
@@ -11484,6 +11512,9 @@ ALTER TABLE ONLY public.labels
 ALTER TABLE ONLY public.project_feature_usages
     ADD CONSTRAINT fk_rails_c22a50024b FOREIGN KEY (project_id) REFERENCES public.projects(id) ON DELETE CASCADE;
 
+ALTER TABLE ONLY public.user_canonical_emails
+    ADD CONSTRAINT fk_rails_c2bd828b51 FOREIGN KEY (user_id) REFERENCES public.users(id) ON DELETE CASCADE;
+
 ALTER TABLE ONLY public.project_repositories
     ADD CONSTRAINT fk_rails_c3258dc63b FOREIGN KEY (shard_id) REFERENCES public.shards(id) ON DELETE RESTRICT;
 
@@ -12614,6 +12645,7 @@ INSERT INTO "schema_migrations" (version) VALUES
 ('20200213204737'),
 ('20200213220159'),
 ('20200213220211'),
+('20200214025454'),
 ('20200214034836'),
 ('20200214085940'),
 ('20200214214934'),

spec/factories/user_canonical_emails.rb

+# frozen_string_literal: true
+
+FactoryBot.define do
+  factory :user_canonical_email do
+    user
+    canonical_email { user.email }
+  end
+end

spec/models/user_canonical_email_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe UserCanonicalEmail do
+  it { is_expected.to belong_to(:user) }
+
+  describe 'validations' do
+    describe 'canonical_email' do
+      it { is_expected.to validate_presence_of(:canonical_email) }
+
+      it 'validates email address', :aggregate_failures do
+        expect(build(:user_canonical_email, canonical_email: 'nonsense')).not_to be_valid
+        expect(build(:user_canonical_email, canonical_email: '@nonsense')).not_to be_valid
+        expect(build(:user_canonical_email, canonical_email: '@nonsense@')).not_to be_valid
+        expect(build(:user_canonical_email, canonical_email: 'nonsense@')).not_to be_valid
+      end
+    end
+  end
+end

spec/services/users/build_service_spec.rb

@@ -16,6 +16,14 @@ describe Users::BuildService do
         expect(service.execute).to be_valid
       end
 
+      context 'calls the UpdateCanonicalEmailService' do
+        specify do
+          expect(Users::UpdateCanonicalEmailService).to receive(:new).and_call_original
+
+          service.execute
+        end
+      end
+
       context 'allowed params' do
         let(:params) do
           {

spec/services/users/create_service_spec.rb

@@ -8,10 +8,11 @@ describe Users::CreateService do
 
     context 'with an admin user' do
       let(:service) { described_class.new(admin_user, params) }
+      let(:email) { 'jd@example.com' }
 
       context 'when required parameters are provided' do
         let(:params) do
-          { name: 'John Doe', username: 'jduser', email: 'jd@example.com', password: 'mydummypass' }
+          { name: 'John Doe', username: 'jduser', email: email, password: 'mydummypass' }
         end
 
         it 'returns a persisted user' do

spec/services/users/update_canonical_email_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Users::UpdateCanonicalEmailService do
+  let(:other_email) { "differentaddress@includeddomain.com" }
+
+  before do
+    stub_const("Users::UpdateCanonicalEmailService::INCLUDED_DOMAINS_PATTERN", [/includeddomain/])
+  end
+
+  describe '#initialize' do
+    context 'unsuccessful' do
+      it 'raises an error if there is no user' do
+        expect { described_class.new(user: nil) }.to raise_error(ArgumentError, /Please provide a user/)
+      end
+
+      it 'raises an error if the object is not a User' do
+        expect { described_class.new(user: 123) }.to raise_error(ArgumentError, /Please provide a user/)
+      end
+    end
+
+    context 'when a user is provided' do
+      it 'does not error' do
+        user = build(:user)
+
+        expect { described_class.new(user: user) }.not_to raise_error
+      end
+    end
+  end
+
+  describe "#canonicalize_email" do
+    let(:user) { build(:user) }
+    let(:subject) { described_class.new(user: user) }
+
+    context 'when the email domain is included' do
+      context 'strips out any . or anything after + in the agent for included domains' do
+        using RSpec::Parameterized::TableSyntax
+
+        let(:expected_result) { 'user@includeddomain.com' }
+
+        where(:raw_email, :expected_result) do
+          'user@includeddomain.com'      | 'user@includeddomain.com'
+          'u.s.e.r@includeddomain.com'   | 'user@includeddomain.com'
+          'user+123@includeddomain.com'  | 'user@includeddomain.com'
+          'us.er+123@includeddomain.com' | 'user@includeddomain.com'
+        end
+
+        with_them do
+          before do
+            user.email = raw_email
+          end
+
+          specify do
+            subject.execute
+
+            expect(user.user_canonical_email).not_to be_nil
+            expect(user.user_canonical_email.canonical_email).to eq expected_result
+          end
+        end
+      end
+
+      context 'when the user has an existing canonical email' do
+        it 'updates the user canonical email record' do
+          user.user_canonical_email = build(:user_canonical_email, canonical_email: other_email)
+          user.email = "us.er+123@includeddomain.com"
+
+          subject.execute
+
+          expect(user.user_canonical_email.canonical_email).to eq "user@includeddomain.com"
+        end
+      end
+    end
+
+    context 'when the email domain is not included' do
+      it 'returns nil' do
+        user.email = "u.s.er+343@excludeddomain.com"
+
+        subject.execute
+
+        expect(user.user_canonical_email).to be_nil
+      end
+
+      it 'destroys any existing UserCanonicalEmail record' do
+        user.email = "u.s.er+343@excludeddomain.com"
+        user.user_canonical_email = build(:user_canonical_email, canonical_email: other_email)
+        expect(user.user_canonical_email).to receive(:delete)
+
+        subject.execute
+      end
+    end
+
+    context 'when the user email is not processable' do
+      [nil, 'nonsense'].each do |invalid_address|
+        before do
+          user.email = invalid_address
+        end
+
+        specify do
+          subject.execute
+
+          expect(user.user_canonical_email).to be_nil
+        end
+
+        it 'preserves any existing record' do
+          user.email = nil
+          user.user_canonical_email = build(:user_canonical_email, canonical_email: other_email)
+
+          subject.execute
+
+          expect(user.user_canonical_email.canonical_email).to eq other_email
+        end
+      end
+    end
+  end
+end

spec/services/users/update_service_spec.rb

@@ -71,6 +71,32 @@ describe Users::UpdateService do
       expect(user.job_title).to eq('Backend Engineer')
     end
 
+    context 'updating canonical email' do
+      context 'if email was changed' do
+        subject do
+          update_user(user, email: 'user+extrastuff@example.com')
+        end
+
+        it 'calls canonicalize_email' do
+          expect_next_instance_of(Users::UpdateCanonicalEmailService) do |service|
+            expect(service).to receive(:execute)
+          end
+
+          subject
+        end
+      end
+
+      context 'if email was NOT changed' do
+        subject do
+          update_user(user, job_title: 'supreme leader of the universe')
+        end
+
+        it 'skips update canonicalize email service call' do
+          expect { subject }.not_to change { user.user_canonical_email }
+        end
+      end
+    end
+
     def update_user(user, opts)
       described_class.new(user, opts.merge(user: user)).execute
     end