Merge branch 'ia-limit-users-by-normalized-email' into 'master'

Add a limit for the number of duplicate detumbed emails

See merge request https://gitlab.com/gitlab-org/gitlab/-/merge_requests/167357



Merged-by: Aditya Tiwari <atiwari@gitlab.com>
Approved-by: Aditya Tiwari <atiwari@gitlab.com>
Reviewed-by: Terri Chu <tchu@gitlab.com>
Reviewed-by: Aditya Tiwari <atiwari@gitlab.com>
Co-authored-by: imand3r <ianderson@gitlab.com>

app/models/email.rb

@@ -11,6 +11,12 @@ class Email < ApplicationRecord
   validate :unique_email, if: ->(email) { email.email_changed? }
+  scope :users_by_detumbled_email_count, ->(email) do
+    normalized_email = ::Gitlab::Utils::Email.normalize_email(email)
+
+    where(detumbled_email: normalized_email).distinct.count(:user_id)
+  end
+
   scope :confirmed, -> { where.not(confirmed_at: nil) }
   scope :unconfirmed, -> { where(confirmed_at: nil) }
   scope :unconfirmed_and_created_before, ->(created_cut_off) { unconfirmed.where('created_at < ?', created_cut_off) }

app/models/user.rb

@@ -341,6 +341,7 @@ def update_tracked_fields!(request)
   validate :namespace_move_dir_allowed, if: :username_changed?, unless: :new_record?
   validate :unique_email, if: :email_changed?
+  validates_with AntiAbuse::UniqueDetumbledEmailValidator, if: :email_changed?
   validate :notification_email_verified, if: :notification_email_changed?
   validate :public_email_verified, if: :public_email_changed?
   validate :commit_email_verified, if: :commit_email_changed?
@@ -1253,15 +1254,6 @@ def unique_email
       errors.add(:email, _('is linked to an account pending deletion.'), help_page_url: help_page_url)
     end
-
-    banned_user_email_reuse_check unless errors.include?(:email)
-  end
-
-  def banned_user_email_reuse_check
-    return unless ::Feature.enabled?(:block_banned_user_normalized_email_reuse, ::Feature.current_request)
-    return unless ::Users::BannedUser.by_detumbled_email(email).exists?
-
-    errors.add(:email, _('is not allowed. Please enter a different email address and try again.'))
   end
   def commit_email_or_default

app/validators/anti_abuse/unique_detumbled_email_validator.rb

+# frozen_string_literal: true
+
+module AntiAbuse
+  class UniqueDetumbledEmailValidator < ActiveModel::Validator
+    NORMALIZED_EMAIL_ACCOUNT_LIMIT = 5
+
+    def validate(record)
+      return if record.errors.include?(:email)
+
+      email = record.email
+
+      return unless prevent_banned_user_email_reuse?(email) || limit_normalized_email_reuse?(email)
+
+      record.errors.add(:email, _('is not allowed. Please enter a different email address and try again.'))
+    end
+
+    private
+
+    def prevent_banned_user_email_reuse?(email)
+      return false unless ::Feature.enabled?(:block_banned_user_normalized_email_reuse, ::Feature.current_request)
+
+      ::Users::BannedUser.by_detumbled_email(email).exists?
+    end
+
+    def limit_normalized_email_reuse?(email)
+      return false unless ::Feature.enabled?(:limit_normalized_email_reuse, ::Feature.current_request)
+
+      Email.users_by_detumbled_email_count(email) >= NORMALIZED_EMAIL_ACCOUNT_LIMIT
+    end
+  end
+end
+
+AntiAbuse::UniqueDetumbledEmailValidator.prepend_mod

config/feature_flags/gitlab_com_derisk/limit_normalized_email_reuse.yml

+---
+name: limit_normalized_email_reuse
+feature_issue_url: https://gitlab.com/gitlab-org/modelops/anti-abuse/team-tasks/-/issues/812
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/167357
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/495124
+milestone: '17.6'
+group: group::anti-abuse
+type: gitlab_com_derisk
+default_enabled: false

ee/app/validators/ee/anti_abuse/unique_detumbled_email_validator.rb

+# frozen_string_literal: true
+
+module EE
+  module AntiAbuse
+    module UniqueDetumbledEmailValidator
+      extend ::Gitlab::Utils::Override
+
+      private
+
+      override :limit_normalized_email_reuse?
+      def limit_normalized_email_reuse?(email)
+        super && !paid_verified_domain?(email)
+      end
+
+      def paid_verified_domain?(email)
+        return false unless ::Gitlab::Saas.feature_available?(:limit_normalized_email_reuse)
+
+        email_domain = Mail::Address.new(email).domain.downcase
+
+        return true if email_domain == ::Gitlab::Saas.root_domain
+
+        root_group = ::PagesDomain.verified.find_by_domain_case_insensitive(email_domain)&.root_group
+
+        !!root_group&.paid?
+      end
+    end
+  end
+end

ee/config/saas_features/limit_normalized_email_reuse.yml

+---
+name: limit_normalized_email_reuse
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/167357
+milestone: '17.6'
+group: group::anti-abuse

ee/lib/ee/gitlab/saas.rb

@@ -32,6 +32,7 @@ module Saas
           gitlab_duo_saas_only
           pipl_compliance
           ci_runners_allowed_plans
+          limit_normalized_email_reuse
         ].freeze
       CONFIG_FILE_ROOT = 'ee/config/saas_features'

ee/spec/models/ee/user_spec.rb

@@ -3643,4 +3643,78 @@
       end
     end
   end
+
+  context 'normalized email reuse check' do
+    let(:error_message) { 'Email is not allowed. Please enter a different email address and try again.' }
+    let(:tumbled_email) { 'user+inbox1@test.com' }
+    let(:normalized_email) { 'user@test.com' }
+
+    subject(:new_user) { build(:user, email: tumbled_email).tap(&:valid?) }
+
+    before do
+      stub_const("::AntiAbuse::UniqueDetumbledEmailValidator::NORMALIZED_EMAIL_ACCOUNT_LIMIT", 1)
+      create(:user, email: normalized_email)
+    end
+
+    context 'when the user has a gitlab.com email address' do
+      let(:tumbled_email) { 'user+inbox1@gitlab.com' }
+      let(:normalized_email) { 'user@gitlab.com' }
+
+      context 'when running in saas', :saas do
+        it 'does not add an error' do
+          expect(new_user.errors).to be_empty
+        end
+      end
+
+      context 'when not running in saas' do
+        it 'adds a validation error' do
+          expect(new_user.errors.full_messages).to include(error_message)
+        end
+      end
+    end
+
+    context 'when a saas user has a an email associated with a verified domain', :saas do
+      let(:verified_domain) { 'verified.com' }
+      let(:tumbled_email) { "user+inbox1@#{verified_domain}" }
+      let(:normalized_email) { "user@#{verified_domain}" }
+
+      context 'when the group is paid' do
+        let_it_be(:ultimate_group) { create(:group_with_plan, plan: :ultimate_plan) }
+        let_it_be(:ultimate_project) { create(:project, group: ultimate_group) }
+
+        it 'does not add an error' do
+          create(:pages_domain, domain: verified_domain, project: ultimate_project)
+
+          expect(new_user.errors).to be_empty
+        end
+      end
+
+      context 'when the root group is not paid' do
+        let_it_be(:free_group) { create(:group) }
+        let_it_be(:free_project) { create(:project, group: free_group) }
+
+        it 'adds a validation error' do
+          create(:pages_domain, domain: verified_domain, project: free_project)
+
+          expect(new_user.errors.full_messages).to include(error_message)
+        end
+      end
+
+      context 'when the root group does not exist' do
+        let_it_be(:project) { create(:project) }
+
+        it 'adds a validation error' do
+          create(:pages_domain, domain: verified_domain, project: project)
+
+          expect(new_user.errors.full_messages).to include(error_message)
+        end
+      end
+    end
+
+    context 'when user is on a self-managed instance' do
+      it 'does not check domain verification' do
+        expect(::PagesDomain).not_to receive(:verified)
+      end
+    end
+  end
 end

lib/gitlab/saas.rb

@@ -5,6 +5,10 @@
 module Gitlab
   module Saas
+    def self.root_domain
+      'gitlab.com'
+    end
+
     def self.com_url
       'https://gitlab.com'
     end

spec/lib/gitlab/saas_spec.rb

@@ -6,6 +6,12 @@
 RSpec.describe Gitlab::Saas, feature_category: :shared do
   include SaasTestHelper
+  describe '.root_domain' do
+    subject { described_class.root_domain }
+
+    it { is_expected.to eq('gitlab.com') }
+  end
+
   describe '.canary_toggle_com_url' do
     subject { described_class.canary_toggle_com_url }

spec/models/email_spec.rb

@@ -59,6 +59,22 @@
     let_it_be(:unconfirmed_secondary_email) { create(:email, user: confirmed_user) }
     let_it_be(:confirmed_secondary_email) { create(:email, :confirmed, user: confirmed_user) }
+    describe '.users_by_detumbled_email_count' do
+      before do
+        # Create users with primary and secondary emails that detumble to the same email: user@example.com.
+        user = create(:user, email: 'user+A@example.com') # New user with a matching primary email
+        create(:user, email: 'user+B@example.com') # New user with a matching primary email
+        create(:email, user: user, email: 'user+C@example.com') # Duplicate user with a matching secondary email
+        create(:email, email: 'user+D@example.com') # New user with a matching secondary email
+      end
+
+      # We created 4 emails but this method should only return a count of 3 since one user
+      # has a primary and secondary email that detumble to the same email address.
+      it 'return the count of unique users with the same detumbled email address' do
+        expect(described_class.users_by_detumbled_email_count('user@example.com')).to eq(3)
+      end
+    end
+
     describe '.confirmed' do
       it 'returns confirmed emails' do
         expect(described_class.confirmed).to contain_exactly(

spec/models/user_spec.rb

@@ -8834,51 +8834,148 @@ def owner_class_attribute_default; end
     end
   end
-  context 'banned user normalized email reuse check' do
-    let_it_be(:existing_user) { create(:user) }
-    shared_examples 'does not perform the check' do
+  context 'normalized email reuse check' do
+    let(:error_message) { 'Email is not allowed. Please enter a different email address and try again.' }
+
+    subject(:new_user) { build(:user, email: tumbled_email).tap(&:valid?) }
+    shared_examples 'adds a validation error' do
       specify do
+        subject
+
+        expect(new_user.errors.full_messages).to include(error_message)
+      end
+    end
+
+    shared_examples 'checking normalized email reuse limit' do
+      before do
+        stub_const("AntiAbuse::UniqueDetumbledEmailValidator::NORMALIZED_EMAIL_ACCOUNT_LIMIT", 2)
+      end
+
+      context 'when the normalized email limit has been reached by unique users' do
+        before do
+          create(:user, email: tumbled_email.split('@').join('1@'))
+        end
+
+        it_behaves_like 'adds a validation error'
+
+        it 'performs the normalized email limit check' do
+          expect(Email).to receive(:users_by_detumbled_email_count).and_call_original
+
+          subject
+        end
+      end
+
+      context 'when the normalized email limit has been reached by non-unique users' do
+        before do
+          user = described_class.find_by(email: normalized_email)
+          create(:email, user: user, email: tumbled_email.split('@').join('1@'))
+        end
+
+        it 'does not add an error' do
+          expect(new_user.errors).to be_empty
+        end
+      end
+
+      context 'when the normalized email limit has not been reached' do
+        it 'does not add an error' do
+          expect(new_user.errors).to be_empty
+        end
+      end
+
+      context 'when the feature flag is disabled' do
+        before do
+          stub_feature_flags(limit_normalized_email_reuse: false)
+        end
+
+        it 'does not perform the check' do
+          expect(Email).not_to receive(:users_by_detumbled_email_count)
+
+          subject
+        end
+      end
+    end
+
+    context 'when email has other validation errors' do
+      subject(:new_user) { build(:user, email: 'invalid-email').tap(&:valid?) }
+
+      it 'does not perform the normalized email checks' do
         expect(::Users::BannedUser).not_to receive(:by_detumbled_email)
+        expect(Email).not_to receive(:users_by_detumbled_email_count)
         subject
       end
     end
-    context 'when email has other validation errors' do
-      subject(:new_user) { build(:user, email: existing_user.email).tap(&:valid?) }
-      it_behaves_like 'does not perform the check'
+    context 'when the email has not changed' do
+      it 'does not perform the normalized email checks' do
+        user = create(:user)
+        expect(::Users::BannedUser).not_to receive(:by_detumbled_email)
+        expect(Email).not_to receive(:users_by_detumbled_email_count)
+
+        user.valid?
+      end
     end
     context 'when email has no other validation errors' do
-      let(:error_message) { 'Email is not allowed. Please enter a different email address and try again.' }
-      let(:tumbled_email) { 'person+inbox1@test.com' }
-      let(:normalized_email) { 'person@test.com' }
-      let!(:banned_user) { create(:user, :banned, email: normalized_email) }
-      subject(:new_user) { build(:user, email: tumbled_email).tap(&:valid?) }
-      it 'performs the check and adds an error' do
-        subject
-        expect(new_user.errors.full_messages).to include(error_message)
-      end
-      context 'and does not match normalized email of a banned user' do
-        let(:tumbled_email) { 'unique+tumbled@email.com' }
-        it 'does not add an error' do
-          expect(new_user.errors.full_messages).not_to include(error_message)
+      context 'when the email is associated with a banned user' do
+        let(:tumbled_email) { 'banned+inbox1@test.com' }
+        let(:normalized_email) { 'banned@test.com' }
+        before do
+          create(:user, :banned, email: normalized_email)
+        end
+        it_behaves_like 'adds a validation error'
+        it 'performs the banned user check' do
+          expect(::Users::BannedUser).to receive(:by_detumbled_email).and_call_original
+          subject
+        end
+        it 'does not perform the normalized email limit check' do
+          expect(Email).not_to receive(:users_by_detumbled_email_count)
+
+          subject
+        end
+
+        context 'and does not match normalized email of a banned user' do
+          let(:tumbled_email) { 'unique+tumbled@email.com' }
+
+          it 'does not add an error' do
+            expect(new_user.errors).to be_empty
+          end
+        end
+
+        context 'when feature flag is disabled' do
+          before do
+            stub_feature_flags(block_banned_user_normalized_email_reuse: false)
+          end
+
+          it 'does not perform the check' do
+            expect(::Users::BannedUser).not_to receive(:by_detumbled_email)
+          end
+
+          it_behaves_like 'checking normalized email reuse limit'
         end
       end
-      context 'when feature flag is disabled' do
+      context 'when the email is not associated with a banned user' do
+        let(:tumbled_email) { 'active+inbox1@test.com' }
+        let(:normalized_email) { 'active@test.com' }
+
         before do
-          stub_feature_flags(block_banned_user_normalized_email_reuse: false)
+          create(:user, email: normalized_email)
+        end
+
+        it 'performs the check and does not add an error' do
+          expect(::Users::BannedUser).to receive(:by_detumbled_email).and_call_original
+          expect(new_user.errors).to be_empty
         end
-        it_behaves_like 'does not perform the check'
+        it_behaves_like 'checking normalized email reuse limit'
       end
     end
   end