Merge branch '33143-group-members-domain-whitelist-should-allow-multiple-domains' into 'master'

Resolve "Group members domain allow-list should allow multiple domains"

See merge request gitlab-org/gitlab!33498

doc/user/group/index.md

@@ -528,13 +528,12 @@ the group regardless of the IP restriction.
 
 #### Allowed domain restriction **(PREMIUM)**
 
-> [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/7297) in [GitLab Premium and Silver](https://about.gitlab.com/pricing/) 12.2.
+>- [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/7297) in [GitLab Premium and Silver](https://about.gitlab.com/pricing/) 12.2.
+>- Support for specifying multiple email domains [introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/33143) in GitLab 13.1
 
-You can restrict access to groups by
-allowing only users with email addresses in particular domains to be added to the group.
+You can restrict access to groups by allowing only users with email addresses in particular domains to be added to the group.
 
-Add email domains you want to allow and users with emails from different
-domains won't be allowed to be added to this group.
+Add email domains you want to allow and users with emails from different domains won't be allowed to be added to this group.
 
 Some domains cannot be restricted. These are the most popular public email domains, such as:
 
@@ -552,7 +551,7 @@ Some domains cannot be restricted. These are the most popular public email domai
 To enable this feature:
 
 1. Navigate to the group's **Settings > General** page.
-1. Expand the **Permissions, LFS, 2FA** section, and enter the domain name into **Restrict membership by email** field.
+1. Expand the **Permissions, LFS, 2FA** section, and enter the domain names into **Restrict membership by email** field. You can enter multiple domains by separating each domain with a comma (,).
 1. Click **Save changes**.
 
 This will enable the domain-checking for all new users added to the group from this moment on.

ee/app/controllers/ee/groups_controller.rb

@@ -8,7 +8,6 @@ module EE
     prepended do
       alias_method :ee_authorize_admin_group!, :authorize_admin_group!
 
-      before_action :set_allowed_domain, only: [:edit]
       before_action :ee_authorize_admin_group!, only: [:restore]
     end
 
@@ -64,7 +63,7 @@ module EE
         params_ee << :file_template_project_id if current_group&.feature_available?(:custom_file_templates_for_namespace)
         params_ee << :custom_project_templates_group_id if current_group&.group_project_template_available?
         params_ee << :ip_restriction_ranges if current_group&.feature_available?(:group_ip_restriction)
-        params_ee << { allowed_email_domain_attributes: [:id, :domain] } if current_group&.feature_available?(:group_allowed_email_domains)
+        params_ee << :allowed_email_domains_list if current_group&.feature_available?(:group_allowed_email_domains)
         params_ee << :max_pages_size if can?(current_user, :update_max_pages_size)
         params_ee << :max_personal_access_token_lifetime if current_group&.personal_access_token_expiration_policy_available?
       end
@@ -92,11 +91,5 @@ module EE
     def default_group_view
       EE::User::DEFAULT_GROUP_VIEW
     end
-
-    def set_allowed_domain
-      return if group.allowed_email_domain.present?
-
-      group.build_allowed_email_domain
-    end
   end
 end

ee/app/models/allowed_email_domain.rb

@@ -24,6 +24,12 @@ class AllowedEmailDomain < ApplicationRecord
 
   belongs_to :group, class_name: 'Group', foreign_key: :group_id
 
+  class << self
+    def domain_names
+      pluck(:domain)
+    end
+  end
+
   def allow_root_group_only
     if group&.parent_id
       errors.add(:base, _('Allowed email domain restriction only permitted for top-level groups'))

ee/app/models/ee/group.rb

@@ -32,8 +32,7 @@ module EE
       has_one :dependency_proxy_setting, class_name: 'DependencyProxy::GroupSetting'
       has_many :dependency_proxy_blobs, class_name: 'DependencyProxy::Blob'
 
-      has_one :allowed_email_domain
-      accepts_nested_attributes_for :allowed_email_domain, allow_destroy: true, reject_if: :all_blank
+      has_many :allowed_email_domains, -> { order(id: :asc) }, autosave: true
 
       # We cannot simply set `has_many :audit_events, as: :entity, dependent: :destroy`
       # here since Group inherits from Namespace, the entity_type would be set to `Namespace`.
@@ -171,6 +170,12 @@ module EE
       ip_restrictions.map(&:range).join(",")
     end
 
+    def allowed_email_domains_list
+      return if allowed_email_domains.empty?
+
+      allowed_email_domains.domain_names.join(",")
+    end
+
     def human_ldap_access
       ::Gitlab::Access.options_with_owner.key(ldap_access)
     end
@@ -240,10 +245,10 @@ module EE
       root_ancestor.ip_restrictions
     end
 
-    def root_ancestor_allowed_email_domain
-      return allowed_email_domain if parent_id.nil?
+    def root_ancestor_allowed_email_domains
+      return allowed_email_domains if parent_id.nil?
 
-      root_ancestor.allowed_email_domain
+      root_ancestor.allowed_email_domains
     end
 
     # Overrides a method defined in `::EE::Namespace`

ee/app/models/ee/group_member.rb

@@ -32,7 +32,7 @@ module EE
     end
 
     def group_has_domain_limitations?
-      group.feature_available?(:group_allowed_email_domains) && group.root_ancestor_allowed_email_domain.present?
+      group.feature_available?(:group_allowed_email_domains) && group_allowed_email_domains.any?
     end
 
     def group_domain_limitations
@@ -56,15 +56,15 @@ module EE
     end
 
     def validate_users_email
-      return if group_allowed_email_domain.email_matches_domain?(user.email)
+      return if matches_at_least_one_group_allowed_email_domain?(user.email)
 
-      errors.add(:user, email_no_match_email_domain(user.email))
+      errors.add(:user, email_does_not_match_any_allowed_domains(user.email))
     end
 
     def validate_invitation_email
-      return if group_allowed_email_domain.email_matches_domain?(invite_email)
+      return if matches_at_least_one_group_allowed_email_domain?(invite_email)
 
-      errors.add(:invite_email, email_no_match_email_domain(invite_email))
+      errors.add(:invite_email, email_does_not_match_any_allowed_domains(invite_email))
     end
 
     def group_saml_identity
@@ -79,16 +79,23 @@ module EE
 
     private
 
-    def email_no_match_email_domain(email)
-      _("email '%{email}' does not match the allowed domain of '%{email_domain}'" % { email: email, email_domain: group_allowed_email_domain.domain })
+    def email_does_not_match_any_allowed_domains(email)
+      _("email '%{email}' does not match the allowed domains of %{email_domains}" %
+        { email: email, email_domains: ::Gitlab::Utils.to_exclusive_sentence(group_allowed_email_domains.map(&:domain)) })
     end
 
     def email_not_verified
       _("email '%{email}' is not a verified email." % { email: user.email })
     end
 
-    def group_allowed_email_domain
-      group.root_ancestor_allowed_email_domain
+    def group_allowed_email_domains
+      group.root_ancestor_allowed_email_domains
+    end
+
+    def matches_at_least_one_group_allowed_email_domain?(email)
+      group_allowed_email_domains.any? do |allowed_email_domain|
+        allowed_email_domain.email_matches_domain?(email)
+      end
     end
   end
 end

ee/app/services/ee/allowed_email_domains/update_service.rb

+# frozen_string_literal: true
+
+module EE
+  # This class is responsible for updating the allowed_email_domains of a specific group.
+  # It takes in comma separated domains as input, eg: 'acme.com, acme.co.in'
+
+  # For a group with existing allowed_email_domains records, this service:
+  # marks the `allowed_email_domains` records that exist for the group right now, but are not in `comma_separated_domains` for deletion.
+  # builds new `allowed_email_domains` records that do not exist for the group right now, but exists in `comma_separated_domains`
+  module AllowedEmailDomains
+    class UpdateService
+      include ::Gitlab::Utils::StrongMemoize
+      include ::Gitlab::Allowable
+
+      def initialize(current_user, group, comma_separated_domains)
+        @current_user = current_user
+        @group = group
+        @current_domains = comma_separated_domains.split(",").map(&:strip).uniq
+      end
+
+      def execute
+        return unless domains_changed?
+
+        unless can?(current_user, :admin_group, group)
+          group.errors.add(:allowed_email_domains, s_('GroupSettings|cannot be changed by you'))
+          return
+        end
+
+        mark_deleted_allowed_email_domains_for_destruction
+        build_new_allowed_emails_domains_records
+      end
+
+      private
+
+      attr_reader :current_user, :group, :current_domains
+
+      def mark_deleted_allowed_email_domains_for_destruction
+        return unless domains_to_be_deleted.present?
+
+        group.allowed_email_domains.each do |allowed_email_domain|
+          if domains_to_be_deleted.include?(allowed_email_domain.domain)
+            allowed_email_domain.mark_for_destruction
+          end
+        end
+      end
+
+      def build_new_allowed_emails_domains_records
+        return unless domains_to_be_created.present?
+
+        domains_to_be_created.each do |domain|
+          group.allowed_email_domains.build(domain: domain)
+        end
+      end
+
+      def domains_to_be_deleted
+        strong_memoize(:domains_to_be_deleted) do
+          existing_domains - current_domains
+        end
+      end
+
+      def domains_to_be_created
+        strong_memoize(:domains_to_be_created) do
+          current_domains - existing_domains
+        end
+      end
+
+      def existing_domains
+        strong_memoize(:existing_domains) do
+          group.allowed_email_domains.domain_names
+        end
+      end
+
+      def domains_changed?
+        existing_domains.sort != current_domains.sort
+      end
+    end
+  end
+end

ee/app/services/ee/groups/update_service.rb

@@ -18,6 +18,8 @@ module EE
 
         remove_insight_if_insight_project_absent
 
+        return false if group.errors.present?
+
         super.tap { |success| log_audit_event if success }
       end
 
@@ -63,7 +65,7 @@ module EE
 
       def check_file_template_project_id_change!
         unless can?(current_user, :admin_group, group)
-          group.errors.add(:file_template_project_id, 'cannot be changed by you')
+          group.errors.add(:file_template_project_id, s_('GroupSettings|cannot be changed by you'))
           return
         end
 
@@ -91,7 +93,7 @@ module EE
       end
 
       def handle_changes
-        handle_allowed_domain_deletion
+        handle_allowed_email_domains_update
         handle_ip_restriction_update
       end
 
@@ -103,20 +105,12 @@ module EE
         IpRestrictions::UpdateService.new(group, comma_separated_ranges).execute
       end
 
-      def associations_editable?
-        return false if group.parent_id.present?
-
-        true
-      end
+      def handle_allowed_email_domains_update
+        return unless params.key?(:allowed_email_domains_list)
 
-      def handle_allowed_domain_deletion
-        return unless associations_editable?
-        return unless group.allowed_email_domain.present?
-        return unless allowed_domain_params
+        comma_separated_domains = params.delete(:allowed_email_domains_list)
 
-        if allowed_domain_params[:domain].blank?
-          allowed_domain_params[:_destroy] = 1
-        end
+        AllowedEmailDomains::UpdateService.new(current_user, group, comma_separated_domains).execute
       end
 
       def valid_path_change_with_npm_packages?
@@ -133,10 +127,6 @@ module EE
         true
       end
 
-      def allowed_domain_params
-        @allowed_domain_params ||= params[:allowed_email_domain_attributes]
-      end
-
       def log_audit_event
         EE::Audit::GroupChangesAuditor.new(current_user, group).execute
       end

ee/app/views/groups/settings/_allowed_email_domain.html.haml

@@ -2,9 +2,14 @@
 
 %h5= _('Restrict membership by email')
 
-= f.fields_for :allowed_email_domain do |allowed_email_domain_form|
-  .form-group
-    = allowed_email_domain_form.text_field :domain, class: 'form-control', placeholder: _('Enter domain')
-    .form-text.text-muted
-      - read_more_link = link_to(_('Read more'), help_page_path('user/group/index', anchor: 'allowed-domain-restriction-premium-only'))
-      = _('Only users with an email address in this domain can be added to the group.<br>Example: <code>gitlab.com</code>. Some common domains are not allowed. %{read_more_link}.').html_safe % { read_more_link: read_more_link }
+.form-group
+  = f.text_field :allowed_email_domains_list, class: 'form-control', placeholder: _('Enter domain')
+  .form-text.text-muted
+    - read_more_link = link_to(_('Read more'), help_page_path('user/group/index', anchor: 'allowed-domain-restriction-premium'))
+    = _('Only verified users with an email address in any of these domains can be added to the group.')
+    %br
+    = _('Multiple domains are supported with comma delimiters.')
+    %br
+    = _('Example: <code>acme.com,acme.co.in,acme.uk</code>.').html_safe
+    %br
+    = _('Some common domains are not allowed. %{read_more_link}.').html_safe % { read_more_link: read_more_link }

ee/changelogs/unreleased/33143-group-members-domain-whitelist-should-allow-multiple-domains.yml

+---
+title: Allow to specify multiple domains when restricting group membership by email
+  domain
+merge_request: 33498
+author:
+type: added

ee/spec/models/allowed_email_domain_spec.rb

@@ -7,6 +7,22 @@ RSpec.describe AllowedEmailDomain do
     it { is_expected.to belong_to(:group) }
   end
 
+  describe '.domain_names' do
+    subject { described_class.domain_names }
+
+    let(:domains) { ['gitlab.com', 'acme.com'] }
+
+    before do
+      domains.each do |domain|
+        create(:allowed_email_domain, domain: domain)
+      end
+    end
+
+    it 'returns the array of domain names' do
+      expect(subject).to match_array(domains)
+    end
+  end
+
   describe 'validations' do
     it { is_expected.to validate_presence_of(:domain) }
     it { is_expected.to validate_presence_of(:group_id) }

ee/spec/models/group_member_spec.rb

@@ -9,12 +9,14 @@ RSpec.describe GroupMember do
   describe 'validations' do
     describe 'group domain limitations' do
       let(:group) { create(:group) }
-      let(:user) { create(:user, email: 'test@gitlab.com') }
-      let(:user_2) { create(:user, email: 'test@gmail.com') }
-      let(:user_3) { create(:user, email: 'unverified@gitlab.com', confirmed_at: nil) }
+      let(:gitlab_user) { create(:user, email: 'test@gitlab.com') }
+      let(:gmail_user) { create(:user, email: 'test@gmail.com') }
+      let(:unconfirmed_gitlab_user) { create(:user, :unconfirmed, email: 'unverified@gitlab.com') }
+      let(:acme_user) { create(:user, email: 'user@acme.com') }
 
       before do
-        create(:allowed_email_domain, group: group)
+        create(:allowed_email_domain, group: group, domain: 'gitlab.com')
+        create(:allowed_email_domain, group: group, domain: 'acme.com')
       end
 
       context 'when group has email domain feature switched on' do
@@ -22,18 +24,20 @@ RSpec.describe GroupMember do
           stub_licensed_features(group_allowed_email_domains: true)
         end
 
-        it 'users email must match allowed domain email' do
-          expect(build(:group_member, group: group, user: user_2)).to be_invalid
-          expect(build(:group_member, group: group, user: user)).to be_valid
+        it 'users email must match at least one of the allowed domain emails' do
+          expect(build(:group_member, group: group, user: gmail_user)).to be_invalid
+          expect(build(:group_member, group: group, user: gitlab_user)).to be_valid
+          expect(build(:group_member, group: group, user: acme_user)).to be_valid
         end
 
-        it 'invited email must match allowed domain email' do
+        it 'invited email must match at least one of the allowed domain emails' do
           expect(build(:group_member, group: group, user: nil, invite_email: 'user@gmail.com')).to be_invalid
           expect(build(:group_member, group: group, user: nil, invite_email: 'user@gitlab.com')).to be_valid
+          expect(build(:group_member, group: group, user: nil, invite_email: 'invite@acme.com')).to be_valid
         end
 
         it 'user emails matching allowed domain must be verified' do
-          group_member = build(:group_member, group: group, user: user_3)
+          group_member = build(:group_member, group: group, user: unconfirmed_gitlab_user)
 
           expect(group_member).to be_invalid
           expect(group_member.errors[:user]).to include("email 'unverified@gitlab.com' is not a verified email.")
@@ -43,39 +47,41 @@ RSpec.describe GroupMember do
           let(:saml_provider) { create(:saml_provider, group: group) }
 
           let!(:group_related_identity) do
-            create(:group_saml_identity, user: user_3, saml_provider: saml_provider)
+            create(:group_saml_identity, user: unconfirmed_gitlab_user, saml_provider: saml_provider)
           end
 
           it 'user emails does not have to be verified' do
-            expect(build(:group_member, group: group, user: user_3)).to be_valid
+            expect(build(:group_member, group: group, user: unconfirmed_gitlab_user)).to be_valid
           end
         end
 
         context 'with group SCIM users' do
           let!(:scim_identity) do
-            create(:scim_identity, user: user_3, group: group)
+            create(:scim_identity, user: unconfirmed_gitlab_user, group: group)
           end
 
           it 'user emails does not have to be verified' do
-            expect(build(:group_member, group: group, user: user_3)).to be_valid
+            expect(build(:group_member, group: group, user: unconfirmed_gitlab_user)).to be_valid
           end
         end
 
         context 'when group is subgroup' do
           let(:subgroup) { create(:group, parent: group) }
 
-          it 'users email must match allowed domain email' do
-            expect(build(:group_member, group: subgroup, user: user_2)).to be_invalid
-            expect(build(:group_member, group: subgroup, user: user)).to be_valid
+          it 'users email must match at least one of the allowed domain emails' do
+            expect(build(:group_member, group: subgroup, user: gmail_user)).to be_invalid
+            expect(build(:group_member, group: subgroup, user: gitlab_user)).to be_valid
+            expect(build(:group_member, group: subgroup, user: acme_user)).to be_valid
           end
 
-          it 'invited email must match allowed domain email' do
+          it 'invited email must match at least one of the allowed domain emails' do
             expect(build(:group_member, group: subgroup, user: nil, invite_email: 'user@gmail.com')).to be_invalid
             expect(build(:group_member, group: subgroup, user: nil, invite_email: 'user@gitlab.com')).to be_valid
+            expect(build(:group_member, group: subgroup, user: nil, invite_email: 'invite@acme.com')).to be_valid
           end
 
           it 'user emails matching allowed domain must be verified' do
-            group_member = build(:group_member, group: subgroup, user: user_3)
+            group_member = build(:group_member, group: subgroup, user: unconfirmed_gitlab_user)
 
             expect(group_member).to be_invalid
             expect(group_member.errors[:user]).to include("email 'unverified@gitlab.com' is not a verified email.")
@@ -84,18 +90,20 @@ RSpec.describe GroupMember do
       end
 
       context 'when group has email domain feature switched off' do
-        it 'users email must match allowed domain email' do
-          expect(build(:group_member, group: group, user: user_2)).to be_valid
-          expect(build(:group_member, group: group, user: user)).to be_valid
+        it 'users email need not match allowed domain emails' do
+          expect(build(:group_member, group: group, user: gmail_user)).to be_valid
+          expect(build(:group_member, group: group, user: gitlab_user)).to be_valid
+          expect(build(:group_member, group: group, user: acme_user)).to be_valid
         end
 
-        it 'invited email must match allowed domain email' do
+        it 'invited email need not match allowed domain emails' do
           expect(build(:group_member, group: group, invite_email: 'user@gmail.com')).to be_valid
           expect(build(:group_member, group: group, invite_email: 'user@gitlab.com')).to be_valid
+          expect(build(:group_member, group: group, invite_email: 'user@acme.com')).to be_valid
         end
 
         it 'user emails does not have to be verified' do
-          expect(build(:group_member, group: group, user: user_3)).to be_valid
+          expect(build(:group_member, group: group, user: unconfirmed_gitlab_user)).to be_valid
         end
       end
     end

ee/spec/models/group_spec.rb

@@ -16,6 +16,7 @@ RSpec.describe Group do
     it { is_expected.to have_many(:dependency_proxy_blobs) }
     it { is_expected.to have_many(:cycle_analytics_stages) }
     it { is_expected.to have_many(:ip_restrictions) }
+    it { is_expected.to have_many(:allowed_email_domains) }
     it { is_expected.to have_one(:dependency_proxy_setting) }
     it { is_expected.to have_one(:deletion_schedule) }
     it { is_expected.to have_one(:group_wiki_repository) }
@@ -419,6 +420,62 @@ RSpec.describe Group do
     end
   end
 
+  describe '#root_ancestor_ip_restrictions' do
+    let(:root_group) { create(:group) }
+    let!(:ip_restriction) { create(:ip_restriction, group: root_group) }
+
+    it 'returns the ip restrictions configured for the root group' do
+      nested_group = create(:group, parent: root_group)
+      deep_nested_group = create(:group, parent: nested_group)
+      very_deep_nested_group = create(:group, parent: deep_nested_group)
+
+      expect(root_group.root_ancestor_ip_restrictions).to contain_exactly(ip_restriction)
+      expect(nested_group.root_ancestor_ip_restrictions).to contain_exactly(ip_restriction)
+      expect(deep_nested_group.root_ancestor_ip_restrictions).to contain_exactly(ip_restriction)
+      expect(very_deep_nested_group.root_ancestor_ip_restrictions).to contain_exactly(ip_restriction)
+    end
+  end
+
+  describe '#allowed_email_domains_list' do
+    subject { group.allowed_email_domains_list }
+
+    context 'group with no associated allowed_email_domains records' do
+      it 'returns nil' do
+        expect(subject).to be_nil
+      end
+    end
+
+    context 'group with associated allowed_email_domains records' do
+      let(:domains) { ['acme.com', 'twitter.com'] }
+
+      before do
+        domains.each do |domain|
+          create(:allowed_email_domain, group: group, domain: domain)
+        end
+      end
+
+      it 'returns a comma separated string of domains of its allowed_email_domains records' do
+        expect(subject).to eq(domains.join(","))
+      end
+    end
+  end
+
+  describe '#root_ancestor_allowed_email_domains' do
+    let(:root_group) { create(:group) }
+    let!(:allowed_email_domain) { create(:allowed_email_domain, group: root_group) }
+
+    it 'returns the email domain restrictions configured for the root group' do
+      nested_group = create(:group, parent: root_group)
+      deep_nested_group = create(:group, parent: nested_group)
+      very_deep_nested_group = create(:group, parent: deep_nested_group)
+
+      expect(root_group.root_ancestor_allowed_email_domains).to contain_exactly(allowed_email_domain)
+      expect(nested_group.root_ancestor_allowed_email_domains).to contain_exactly(allowed_email_domain)
+      expect(deep_nested_group.root_ancestor_allowed_email_domains).to contain_exactly(allowed_email_domain)
+      expect(very_deep_nested_group.root_ancestor_allowed_email_domains).to contain_exactly(allowed_email_domain)
+    end
+  end
+
   describe '#predefined_push_rule' do
     context 'group with no associated push_rules record' do
       let!(:sample) { create(:push_rule_sample) }

ee/spec/requests/groups_controller_spec.rb

@@ -10,8 +10,6 @@ RSpec.describe GroupsController, type: :request do
     before do
       group.add_owner(user)
       login_as(user)
-
-      stub_licensed_features(group_ip_restriction: true)
     end
 
     subject do
@@ -19,7 +17,6 @@ RSpec.describe GroupsController, type: :request do
     end
 
     context 'setting ip_restriction' do
-      let(:group) { create(:group) }
       let(:params) { { group: { ip_restriction_ranges: range } } }
       let(:range) { '192.168.0.0/24' }
 
@@ -186,5 +183,155 @@ RSpec.describe GroupsController, type: :request do
         end
       end
     end
+
+    context 'setting email domain restrictions' do
+      let(:params) { { group: { allowed_email_domains_list: domains } } }
+
+      before do
+        stub_licensed_features(group_allowed_email_domains: true)
+      end
+
+      context 'top-level group' do
+        context 'when email domain restriction does not exist' do
+          context 'valid param' do
+            shared_examples 'creates email domain restrictions' do
+              it 'creates email domain restrictions' do
+                subject
+
+                expect(response).to have_gitlab_http_status(:found)
+                expect(group.reload.allowed_email_domains.domain_names).to match_array(domains.split(","))
+              end
+            end
+
+            context 'single domain' do
+              let(:domains) { 'gitlab.com' }
+
+              it_behaves_like 'creates email domain restrictions'
+            end
+
+            context 'multiple domains' do
+              let(:domains) { 'gitlab.com,acme.com' }
+
+              it_behaves_like 'creates email domain restrictions'
+            end
+          end
+
+          context 'invalid param' do
+            let(:domains) { 'boom!' }
+
+            it 'adds error message' do
+              expect { subject }
+                .not_to(change { group.reload.allowed_email_domains.count }.from(0))
+              expect(response).to have_gitlab_http_status(:ok)
+              expect(response.body).to include('The domain you entered is misformatted')
+            end
+          end
+        end
+
+        context 'when email domain restrictions already exists' do
+          let!(:allowed_email_domain) { create(:allowed_email_domain, group: group, domain: 'gitlab.com') }
+
+          context 'allowed email domain param set' do
+            context 'valid param' do
+              shared_examples 'updates allowed email domain restrictions' do
+                it 'updates allowed email domain restrictions' do
+                  subject
+
+                  expect(response).to have_gitlab_http_status(:found)
+                  expect(group.reload.allowed_email_domains.domain_names).to match_array(domains.split(","))
+                end
+              end
+
+              context 'single domain' do
+                let(:domains) { 'hey.com' }
+
+                it_behaves_like 'updates allowed email domain restrictions'
+              end
+
+              context 'multiple domains' do
+                context 'a new domain along with the existing one' do
+                  let(:domains) { 'gitlab.com,hey.com' }
+
+                  it_behaves_like 'updates allowed email domain restrictions'
+                end
+
+                context 'completely new set of domains' do
+                  let(:domains) { 'hey.com,google.com' }
+
+                  it_behaves_like 'updates allowed email domain restrictions'
+                end
+              end
+            end
+
+            context 'invalid param' do
+              shared_examples 'does not update existing email domain restrictions' do
+                it 'does not change allowed_email_domains records' do
+                  expect { subject }
+                    .not_to(change { group.reload.allowed_email_domains.domain_names }
+                      .from(['gitlab.com']))
+                end
+
+                it 'adds error message' do
+                  subject
+
+                  expect(response).to have_gitlab_http_status(:ok)
+                  expect(response.body).to include('The domain you entered is misformatted')
+                end
+              end
+
+              context 'not a valid domain' do
+                let(:domains) { 'boom!' }
+
+                it_behaves_like 'does not update existing email domain restrictions'
+              end
+
+              context 'multiple domains' do
+                context 'any one of them being not a valid' do
+                  let(:domains) { 'acme.com,boom!' }
+
+                  it_behaves_like 'does not update existing email domain restrictions'
+                end
+              end
+            end
+          end
+
+          context 'empty param' do
+            let(:domains) { '' }
+
+            it 'deletes all email domain restrictions' do
+              expect { subject }
+                .to(change { group.reload.allowed_email_domains.count }.to(0))
+              expect(response).to have_gitlab_http_status(:found)
+            end
+          end
+        end
+      end
+
+      context 'subgroup' do
+        let(:group) { create(:group, :nested) }
+        let(:domains) { 'gitlab.com' }
+
+        it 'does not create email domain restriction' do
+          expect { subject }
+            .not_to change { group.reload.allowed_email_domains.count }.from(0)
+          expect(response).to have_gitlab_http_status(:ok)
+          expect(response.body).to include('Allowed email domain restriction only permitted for top-level groups')
+        end
+      end
+
+      context 'feature is disabled' do
+        let(:domains) { 'gitlab.com' }
+
+        before do
+          stub_licensed_features(group_allowed_email_domains: false)
+        end
+
+        it 'does not create email domain restrictions' do
+          expect { subject }
+            .not_to change { group.reload.allowed_email_domains.count }.from(0)
+          expect(response).to have_gitlab_http_status(:found)
+        end
+      end
+    end
   end
 end

ee/spec/services/ee/allowed_email_domains/update_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe EE::AllowedEmailDomains::UpdateService do
+  let(:group) { create(:group) }
+  let(:user) { create(:user) }
+
+  subject { described_class.new(user, group, comma_separated_domains_list).execute }
+
+  describe '#execute' do
+    context 'as a normal user' do
+      context 'for a group that has no email domain restriction' do
+        context 'with valid domains' do
+          let(:comma_separated_domains_list) { 'gitlab.com,acme.com' }
+
+          it 'does not build new allowed_email_domain records' do
+            subject
+
+            expect { group.save! }
+              .not_to(change { group.allowed_email_domains.count }.from(0))
+          end
+
+          it 'registers an error' do
+            subject
+
+            expect(group.errors[:allowed_email_domains]).to include('cannot be changed by you')
+          end
+        end
+      end
+    end
+
+    context 'as a group owner' do
+      before do
+        group.add_owner(user)
+      end
+
+      context 'for a group that has no email domain restriction' do
+        context 'with valid domains' do
+          let(:comma_separated_domains_list) { 'gitlab.com,acme.com' }
+
+          it 'builds new allowed_email_domain records' do
+            subject
+
+            expect { group.save! }
+              .to(change { group.allowed_email_domains.count }.from(0).to(2))
+          end
+
+          it 'builds new allowed_email_domain records with the provided domains' do
+            subject
+
+            expect(group.allowed_email_domains.map(&:domain)).to match_array(comma_separated_domains_list.split(","))
+          end
+        end
+      end
+
+      context 'for a group that already has email domain restriction' do
+        let(:domains) { ['gitlab.com', 'acme.com'] }
+
+        before do
+          domains.each do |domain|
+            create(:allowed_email_domain, group: group, domain: domain)
+          end
+        end
+
+        context 'with empty domain' do
+          let(:comma_separated_domains_list) { '' }
+
+          it 'marks all existing allowed_email_domain records for destruction' do
+            expect { subject }
+              .to(change { group.allowed_email_domains.select(&:marked_for_destruction?).size }.from(0).to(2))
+          end
+        end
+
+        context 'with valid domains' do
+          before do
+            subject
+          end
+
+          context 'with an entirely new set of domains' do
+            shared_examples 'removes all existing allowed_email_domain records' do
+              it 'marks all the existing allowed_email_domain records for destruction' do
+                records_marked_for_destruction = group.allowed_email_domains.select(&:marked_for_destruction?)
+                expect(records_marked_for_destruction.map(&:domain)).to match_array(domains)
+              end
+            end
+
+            context 'each domain in the list is unique' do
+              let(:comma_separated_domains_list) { 'hey.com,google.com,twitter.com' }
+
+              it_behaves_like 'removes all existing allowed_email_domain records'
+
+              it 'builds new allowed_email_domain records with all of the specified domains' do
+                newly_built_allowed_email_domain_records = group.allowed_email_domains.select { |allowed_email_domain| allowed_email_domain.id.nil? }
+
+                expect(newly_built_allowed_email_domain_records.map(&:domain)).to match_array(comma_separated_domains_list.split(",").map(&:strip))
+              end
+
+              context 'list has space around the names of domains' do
+                let(:comma_separated_domains_list) { 'hey.com, google.com,  twitter.com' }
+
+                it_behaves_like 'removes all existing allowed_email_domain records'
+
+                it 'builds new allowed_email_domain records with all of the specified domains without spaces around them' do
+                  newly_built_allowed_email_domain_records = group.allowed_email_domains.select { |allowed_email_domain| allowed_email_domain.id.nil? }
+
+                  expect(newly_built_allowed_email_domain_records.map(&:domain)).to match_array(comma_separated_domains_list.split(",").map(&:strip))
+                end
+              end
+            end
+
+            context 'domains in the list repeats' do
+              let(:comma_separated_domains_list) { 'hey.com,google.com,hey.com' }
+
+              it_behaves_like 'removes all existing allowed_email_domain records'
+
+              it 'builds new allowed_email_domain records with only the unique domains among the specified domains' do
+                newly_built_allowed_email_domain_records = group.allowed_email_domains.select { |allowed_email_domain| allowed_email_domain.id.nil? }
+
+                expect(newly_built_allowed_email_domain_records.map(&:domain)).to match_array(comma_separated_domains_list.split(",").map(&:strip).uniq)
+              end
+            end
+          end
+
+          context 'replacing one of the existing domains with another' do
+            # replacing 'acme.com' with 'hey.com' and retaining 'gitlab.com'
+
+            let(:comma_separated_domains_list) { 'gitlab.com,hey.com' }
+
+            it 'marks the allowed_email_domain record of the replaced domain for destruction' do
+              allowed_email_domain_record_of_replaced_domain = group.allowed_email_domains.find { |allowed_email_domain| allowed_email_domain.domain == 'acme.com' }
+
+              expect(allowed_email_domain_record_of_replaced_domain.marked_for_destruction?).to be_truthy
+            end
+
+            it 'retains the allowed_email_domain record of the other existing domain' do
+              allowed_email_domain_record_of_other_existing_domain = group.allowed_email_domains.find { |allowed_email_domain| allowed_email_domain.domain == 'gitlab.com' }
+
+              expect(allowed_email_domain_record_of_other_existing_domain.marked_for_destruction?).to be_falsey
+            end
+
+            it 'builds a new allowed_email_domain record with the newly specified domain' do
+              newly_built_allowed_email_domain_records = group.allowed_email_domains.select { |allowed_email_domain| allowed_email_domain.id.nil? }
+
+              expect(newly_built_allowed_email_domain_records.size).to eq(1)
+              expect(newly_built_allowed_email_domain_records.map(&:domain)).to match_array(['hey.com'])
+            end
+          end
+        end
+      end
+    end
+  end
+end

ee/spec/services/groups/update_service_spec.rb

@@ -227,7 +227,8 @@ RSpec.describe Groups::UpdateService, '#execute' do
   end
 
   context 'setting allowed email domain' do
-    let(:group) { create(:group) }
+    let(:group) { create(:group, :private) }
+    let(:user) { create(:user, email: 'admin@gitlab.com') }
 
     subject { update_group(group, user, params) }
 
@@ -238,15 +239,59 @@ RSpec.describe Groups::UpdateService, '#execute' do
     context 'when allowed_email_domain already exists' do
       let!(:allowed_domain) { create(:allowed_email_domain, group: group, domain: 'gitlab.com') }
 
-      context 'empty allowed_email_domain param' do
-        let(:params) { { allowed_email_domain_attributes: { id: allowed_domain.id, domain: '' } } }
+      context 'allowed_email_domains_list param is not specified' do
+        let(:params) { {} }
 
-        it 'deletes ip restriction' do
-          expect(group.allowed_email_domain.domain).to eql('gitlab.com')
+        it 'does not call EE::AllowedEmailDomains::UpdateService#execute' do
+          expect_any_instance_of(EE::AllowedEmailDomains::UpdateService).not_to receive(:execute)
 
           subject
+        end
+      end
+
+      context 'allowed_email_domains_list param is blank' do
+        let(:params) { { allowed_email_domains_list: '' } }
+
+        context 'as a group owner' do
+          before do
+            group.add_owner(user)
+          end
 
-          expect(group.reload.allowed_email_domain).to be_nil
+          it 'calls EE::AllowedEmailDomains::UpdateService#execute' do
+            expect_any_instance_of(EE::AllowedEmailDomains::UpdateService).to receive(:execute)
+
+            subject
+          end
+
+          it 'update is successful' do
+            expect(subject).to eq(true)
+          end
+
+          it 'deletes existing allowed_email_domain record' do
+            expect { subject }.to change { group.reload.allowed_email_domains.size }.from(1).to(0)
+          end
+        end
+
+        context 'as a normal user' do
+          it 'calls EE::AllowedEmailDomains::UpdateService#execute' do
+            expect_any_instance_of(EE::AllowedEmailDomains::UpdateService).to receive(:execute)
+
+            subject
+          end
+
+          it 'update is not successful' do
+            expect(subject).to eq(false)
+          end
+
+          it 'registers an error' do
+            subject
+
+            expect(group.errors[:allowed_email_domains]).to include('cannot be changed by you')
+          end
+
+          it 'does not delete existing allowed_email_domain record' do
+            expect { subject }.not_to change { group.reload.allowed_email_domains.size }
+          end
         end
       end
     end

ee/spec/views/groups/edit.html.haml_spec.rb

@@ -14,50 +14,56 @@ RSpec.describe 'groups/edit.html.haml' do
   end
 
   context 'ip_restriction' do
-    before do
-      stub_licensed_features(group_ip_restriction: true)
+    shared_examples_for 'renders ip_restriction setting' do
+      it 'renders ranges in comma separated format' do
+        render
+
+        expect(rendered).to render_template('groups/settings/_ip_restriction')
+        expect(rendered).to(have_field('group_ip_restriction_ranges',
+                                       { disabled: false,
+                                         with: ranges.join(",") }))
+      end
     end
 
-    context 'top-level group' do
-      shared_examples 'renders ip_restriction setting' do
-        it 'renders ranges in comma separated format' do
-          render
+    shared_examples_for 'does not render ip_restriction setting' do
+      it 'does not render the ranges' do
+        render
 
-          expect(rendered).to render_template('groups/settings/_ip_restriction')
-          expect(rendered).to(have_field('group_ip_restriction_ranges',
-                                         { disabled: false,
-                                           with: ranges.join(",") }))
-        end
+        expect(rendered).to render_template('groups/settings/_ip_restriction')
+        expect(rendered).not_to have_field('group_ip_restriction_ranges')
       end
+    end
 
+    context 'feature is enabled' do
       before do
-        ranges.each do |range|
-          create(:ip_restriction, group: group, range: range)
-        end
+        stub_licensed_features(group_ip_restriction: true)
       end
 
-      context 'with single subnet' do
-        let(:ranges) { ['192.168.0.0/24'] }
+      context 'top-level group' do
+        before do
+          ranges.each do |range|
+            create(:ip_restriction, group: group, range: range)
+          end
+        end
 
-        it_behaves_like 'renders ip_restriction setting'
-      end
+        context 'with single subnet' do
+          let(:ranges) { ['192.168.0.0/24'] }
 
-      context 'with multiple subnets' do
-        let(:ranges) { ['192.168.0.0/24', '192.168.1.0/8'] }
+          it_behaves_like 'renders ip_restriction setting'
+        end
+
+        context 'with multiple subnets' do
+          let(:ranges) { ['192.168.0.0/24', '192.168.1.0/8'] }
 
-        it_behaves_like 'renders ip_restriction setting'
+          it_behaves_like 'renders ip_restriction setting'
+        end
       end
     end
 
     context 'subgroup' do
       let(:group) { create(:group, :nested) }
 
-      it 'does not show ip_restriction setting' do
-        render
-
-        expect(rendered).to render_template('groups/settings/_ip_restriction')
-        expect(rendered).not_to have_field('group_ip_restriction_attributes_range')
-      end
+      it_behaves_like 'does not render ip_restriction setting'
     end
 
     context 'feature is disabled' do
@@ -65,58 +71,66 @@ RSpec.describe 'groups/edit.html.haml' do
         stub_licensed_features(group_ip_restriction: false)
       end
 
-      it 'does not show ip_restriction setting' do
-        render
-
-        expect(rendered).to render_template('groups/settings/_ip_restriction')
-        expect(rendered).not_to have_field('group_ip_restriction_attributes_range')
-      end
+      it_behaves_like 'does not render ip_restriction setting'
     end
   end
 
   context 'allowed_email_domain' do
-    before do
-      allow(group).to receive(:feature_available?).and_return(false)
-      allow(group).to receive(:feature_available?).with(:group_allowed_email_domains).and_return(true)
-    end
-
-    context 'top-level group' do
-      before do
-        create(:allowed_email_domain, group: group)
-      end
-
-      it 'renders allowed_email_domain setting' do
+    shared_examples_for 'renders allowed_email_domain setting' do
+      it 'renders domains in comma separated format' do
         render
 
         expect(rendered).to render_template('groups/settings/_allowed_email_domain')
-        expect(rendered).to(have_field('group_allowed_email_domain_attributes_domain',
+        expect(rendered).to(have_field('group_allowed_email_domains_list',
                                        { disabled: false,
-                                         with: 'gitlab.com' }))
+                                         with: domains.join(",") }))
       end
     end
 
-    context 'subgroup' do
-      let(:group) { create(:group, :nested) }
-
-      it 'does not show allowed_email_domain setting' do
+    shared_examples_for 'does not render allowed_email_domain setting' do
+      it 'does not render the domains' do
         render
 
         expect(rendered).to render_template('groups/settings/_allowed_email_domain')
-        expect(rendered).not_to have_field('group_allowed_email_domain_attributes_domain')
+        expect(rendered).not_to have_field('group_allowed_email_domains_list')
       end
     end
 
-    context 'feature is disabled' do
+    context 'feature is enabled' do
       before do
-        stub_licensed_features(group_allowed_email_domains: false)
+        allow(group).to receive(:feature_available?).and_return(false)
+        allow(group).to receive(:feature_available?).with(:group_allowed_email_domains).and_return(true)
       end
 
-      it 'does not show allowed_email_domain setting' do
-        render
+      context 'top-level group' do
+        before do
+          domains.each do |domain|
+            create(:allowed_email_domain, group: group, domain: domain)
+          end
+        end
 
-        expect(rendered).to render_template('groups/settings/_allowed_email_domain')
-        expect(rendered).not_to have_field('group_allowed_email_domain_attributes_domain')
+        context 'with single domain' do
+          let(:domains) { ['acme.com'] }
+
+          it_behaves_like 'renders allowed_email_domain setting'
+        end
+
+        context 'with multiple domain' do
+          let(:domains) { ['acme.com', 'twitter.com'] }
+
+          it_behaves_like 'renders allowed_email_domain setting'
+        end
       end
+
+      context 'subgroup' do
+        let(:group) { create(:group, :nested) }
+
+        it_behaves_like 'does not render allowed_email_domain setting'
+      end
+    end
+
+    context 'feature is disabled' do
+      it_behaves_like 'does not render allowed_email_domain setting'
     end
   end
 end

locale/gitlab.pot

@@ -9050,6 +9050,9 @@ msgstr ""
 msgid "Exactly one of %{attributes} is required"
 msgstr ""
 
+msgid "Example: <code>acme.com,acme.co.in,acme.uk</code>."
+msgstr ""
+
 msgid "Example: @sub\\.company\\.com$"
 msgstr ""
 
@@ -11219,6 +11222,9 @@ msgstr ""
 msgid "GroupSettings|You will need to update your local repositories to point to the new location."
 msgstr ""
 
+msgid "GroupSettings|cannot be changed by you"
+msgstr ""
+
 msgid "GroupSettings|cannot be disabled when the parent group \"Share with group lock\" is enabled, except by the owner of the parent group"
 msgstr ""
 
@@ -14346,6 +14352,9 @@ msgstr ""
 msgid "MrDeploymentActions|Stop environment"
 msgstr ""
 
+msgid "Multiple domains are supported with comma delimiters."
+msgstr ""
+
 msgid "Multiple issue boards"
 msgstr ""
 
@@ -15208,7 +15217,7 @@ msgstr ""
 msgid "Only project members will be imported. Group members will be skipped."
 msgstr ""
 
-msgid "Only users with an email address in this domain can be added to the group.<br>Example: <code>gitlab.com</code>. Some common domains are not allowed. %{read_more_link}."
+msgid "Only verified users with an email address in any of these domains can be added to the group."
 msgstr ""
 
 msgid "Only ‘Reporter’ roles and above on tiers Premium / Silver and above can see Productivity Analytics."
@@ -20470,6 +20479,9 @@ msgstr ""
 msgid "Some child epics may be hidden due to applied filters"
 msgstr ""
 
+msgid "Some common domains are not allowed. %{read_more_link}."
+msgstr ""
+
 msgid "Some email servers do not support overriding the email sender name. Enable this option to include the name of the author of the issue, merge request or comment in the email body instead."
 msgstr ""
 
@@ -26363,7 +26375,7 @@ msgstr ""
 msgid "element is not a hierarchy"
 msgstr ""
 
-msgid "email '%{email}' does not match the allowed domain of '%{email_domain}'"
+msgid "email '%{email}' does not match the allowed domains of %{email_domains}"
 msgstr ""
 
 msgid "email '%{email}' is not a verified email."