Merge branch 'security-validate-allowed-email-domains-on-group-invite-14-10' into '14-10-stable-ee'

Allow inviting only groups with subset of allowed domains to groups

See merge request gitlab-org/security/gitlab!2512

.rubocop_todo/layout/line_length.yml

@@ -1979,7 +1979,6 @@ Layout/LineLength:
     - 'ee/spec/features/groups/iterations/user_edits_iteration_spec.rb'
     - 'ee/spec/features/groups/iterations/user_views_iteration_cadence_spec.rb'
     - 'ee/spec/features/groups/iterations/user_views_iteration_spec.rb'
-    - 'ee/spec/features/groups/members/manage_groups_spec.rb'
     - 'ee/spec/features/groups/members/manage_members_spec.rb'
     - 'ee/spec/features/groups/members/override_ldap_memberships_spec.rb'
     - 'ee/spec/features/groups/saml_providers_spec.rb'

app/models/group_group_link.rb

@@ -41,3 +41,5 @@ class GroupGroupLink < ApplicationRecord
     Gitlab::Access.human_access(self.group_access)
   end
 end
+
+GroupGroupLink.prepend_mod_with('GroupGroupLink')

doc/user/group/index.md

@@ -640,6 +640,7 @@ To restrict group access by IP address:
 > - [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/7297) in GitLab 12.2.
 > - Support for specifying multiple email domains [added](https://gitlab.com/gitlab-org/gitlab/-/issues/33143) in GitLab 13.1.
 > - Support for restricting access to projects in the group [added](https://gitlab.com/gitlab-org/gitlab/-/issues/14004) in GitLab 14.1.2.
+> - Support for restricting group memberships to groups with a subset of the allowed email domains [added](https://gitlab.com/gitlab-org/gitlab/-/issues/354791) in GitLab 15.0.1
 
 You can prevent users with email addresses in specific domains from being added to a group and its projects.
 
@@ -662,6 +663,8 @@ The most popular public email domains cannot be restricted, such as:
 - `hotmail.com`, `hotmail.co.uk`, `hotmail.fr`
 - `msn.com`, `live.com`, `outlook.com`
 
+When you share a group, both the source and target namespaces must allow the domains of the members' email addresses.
+
 ## Group file templates **(PREMIUM)**
 
 Use group file templates to share a set of templates for common file

ee/app/models/ee/group_group_link.rb

+# frozen_string_literal: true
+
+module EE
+  module GroupGroupLink
+    extend ActiveSupport::Concern
+
+    prepended do
+      scope :in_shared_group, -> (shared_groups) { where(shared_group: shared_groups) }
+      scope :not_in_shared_with_group, -> (shared_with_groups) { where.not(shared_with_group: shared_with_groups) }
+
+      validate :group_with_allowed_email_domains
+    end
+
+    private
+
+    def group_with_allowed_email_domains
+      return unless shared_group && shared_with_group
+
+      shared_group_domains = shared_group.root_ancestor_allowed_email_domains.pluck(:domain).to_set
+      return if shared_group_domains.empty?
+
+      shared_with_group_domains = shared_with_group.root_ancestor_allowed_email_domains.pluck(:domain).to_set
+
+      if shared_with_group_domains.empty? || !shared_with_group_domains.subset?(shared_group_domains)
+        errors.add(:group_id, _("Invited group allowed email domains must contain a subset of the allowed"\
+          " email domains of the root ancestor group. Go to the group's 'Settings > General' page"\
+          " and check 'Restrict membership by email domain'."))
+      end
+    end
+  end
+end

ee/app/models/ee/project_group_link.rb

@@ -6,6 +6,8 @@ module EE
 
     prepended do
       before_destroy :delete_related_access_levels
+
+      validate :group_with_allowed_email_domains
     end
 
     def delete_related_access_levels
@@ -21,5 +23,21 @@ module EE
       # For protected environments
       project.protected_environments.revoke_group(group)
     end
+
+    def group_with_allowed_email_domains
+      return unless shared_from&.group && shared_with_group
+
+      root_ancestor_group_domains = shared_from.root_ancestor.allowed_email_domains.pluck(:domain).to_set
+      return if root_ancestor_group_domains.empty?
+
+      shared_with_group_domains = shared_with_group.root_ancestor_allowed_email_domains.pluck(:domain).to_set
+
+      if shared_with_group_domains.empty? || !shared_with_group_domains.subset?(root_ancestor_group_domains)
+        errors.add(:group_id,
+        _("Invited group allowed email domains must contain a subset of the allowed email domains"\
+          " of the root ancestor group. Go to the group's 'Settings > General' page"\
+          " and check 'Restrict membership by email domain'."))
+      end
+    end
   end
 end

ee/spec/features/groups/members/manage_groups_spec.rb

@@ -9,22 +9,38 @@ RSpec.describe 'Groups > Members > Manage groups', :js, :saas do
 
   let_it_be(:user) { create(:user) }
   let_it_be(:user2) { create(:user) }
-  let_it_be(:group) { create(:group) }
-  let_it_be(:group_to_add) { create(:group) }
+
+  let(:group) { create(:group) }
+  let(:group_to_add) { create(:group) }
 
   let(:premium_plan) { create(:premium_plan) }
-  let(:ultimate_plan) { create(:premium_plan) }
+  let(:ultimate_plan) { create(:ultimate_plan) }
+
+  shared_examples 'adds group without validation error' do
+    it_behaves_like "doesn't trigger an overage modal when adding a group with a given role", 'Maintainer'
+  end
+
+  shared_examples 'inviting group fails with allowed email domain error' do
+    specify do
+      group.add_owner(user)
+      group_to_add.add_owner(user)
+
+      visit group_group_members_path(group)
+      add_group(group_to_add.name, 'Maintainer')
+
+      expect(page).to have_content('Invited group allowed email domains must contain a subset of the'\
+        ' allowed email domains of the root ancestor group')
+    end
+  end
 
   shared_examples "doesn't trigger an overage modal when adding a group with a given role" do |role|
-    it do
+    specify do
       group.add_owner(user)
       group_to_add.add_owner(user)
 
       visit group_group_members_path(group)
       add_group(group_to_add.name, role)
 
-      wait_for_requests
-
       expect(page).not_to have_button 'Continue'
 
       page.refresh
@@ -39,7 +55,7 @@ RSpec.describe 'Groups > Members > Manage groups', :js, :saas do
   end
 
   shared_examples "triggers an overage modal when adding a group as Reporter" do
-    it do
+    specify do
       add_group_with_one_extra_user
       click_button 'Continue'
 
@@ -98,6 +114,88 @@ RSpec.describe 'Groups > Members > Manage groups', :js, :saas do
     it_behaves_like "doesn't trigger an overage modal when adding a group with a given role", 'Guest'
   end
 
+  describe 'inviting group with restricted email domain' do
+    shared_examples 'restricted membership by email domain' do
+      context 'shared group has membership restricted by allowed email domains' do
+        before do
+          create(:allowed_email_domain, group: group.root_ancestor, domain: 'gitlab.com')
+        end
+
+        context 'shared with group with a subset of allowed email domains' do
+          before do
+            create(:allowed_email_domain, group: group_to_add.root_ancestor, domain: 'gitlab.com')
+          end
+
+          it_behaves_like 'adds group without validation error'
+        end
+
+        context 'shared with group containing domains outside the shared group allowed email domains' do
+          before do
+            create(:allowed_email_domain, group: group_to_add.root_ancestor, domain: 'example.com')
+          end
+
+          it_behaves_like 'inviting group fails with allowed email domain error'
+        end
+
+        context 'shared with group does not have membership restricted by allowed domains' do
+          it_behaves_like 'inviting group fails with allowed email domain error'
+        end
+      end
+
+      context 'shared group does not have membership restricted by allowed domains' do
+        context 'shared with group has membership restricted by allowed email domains' do
+          before do
+            create(:allowed_email_domain, group: group_to_add.root_ancestor, domain: 'example.com')
+          end
+
+          it_behaves_like 'adds group without validation error'
+        end
+
+        context 'shared with group does not have membership restricted by allowed domains' do
+          it_behaves_like 'adds group without validation error'
+        end
+      end
+    end
+
+    context 'shared group is the root ancestor' do
+      let(:group) { create(:group) }
+      let(:group_to_add) { create(:group) }
+
+      it_behaves_like 'restricted membership by email domain'
+    end
+
+    context 'shared group is a subgroup' do
+      let(:parent_group) { create(:group) }
+      let(:group) { create(:group, parent: parent_group) }
+      let(:group_to_add) { create(:group) }
+
+      before do
+        parent_group.add_owner(user)
+      end
+
+      it_behaves_like 'restricted membership by email domain'
+    end
+
+    context 'shared with group is a subgroup' do
+      let(:group) { create(:group) }
+      let(:group_to_add) { create(:group, parent: create(:group)) }
+
+      it_behaves_like 'restricted membership by email domain'
+    end
+
+    context 'shared and shared with group are subgroups' do
+      let(:parent_group) { create(:group) }
+      let(:group) { create(:group, parent: parent_group) }
+      let(:group_to_add) { create(:group, parent: create(:group)) }
+
+      before do
+        parent_group.add_owner(user)
+      end
+
+      it_behaves_like 'restricted membership by email domain'
+    end
+  end
+
   def add_group(name, role, expires_at: nil)
     click_on 'Invite a group'
 
@@ -107,6 +205,7 @@ RSpec.describe 'Groups > Members > Manage groups', :js, :saas do
     choose_options(role, expires_at)
 
     click_button 'Invite'
+    wait_for_requests
   end
 
   def add_group_with_one_extra_user
@@ -117,8 +216,7 @@ RSpec.describe 'Groups > Members > Manage groups', :js, :saas do
     visit group_group_members_path(group)
     add_group(group_to_add.name, 'Reporter')
 
-    wait_for_requests
-
-    expect(page).to have_content("Your subscription includes 1 seat. If you continue, the #{group.name} group will have 2 seats in use and will be billed for the overage. Learn more.")
+    expect(page).to have_content("Your subscription includes 1 seat. If you continue, the #{group.name} group will"\
+      " have 2 seats in use and will be billed for the overage. Learn more.")
   end
 end

ee/spec/features/projects/members/manage_groups_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe 'Projects > Members > Manage groups' do
+  include Spec::Support::Helpers::Features::MembersHelpers
+  include Spec::Support::Helpers::Features::InviteMembersModalHelper
+
+  shared_examples 'adds group without validation error' do
+    specify do
+      group.add_owner(user)
+      group_to_add.add_owner(user)
+      sign_in(user)
+
+      role = 'Maintainer'
+      visit project_project_members_path(project)
+      add_group(group_to_add.name, role)
+
+      page.refresh
+
+      click_groups_tab
+
+      page.within(first_row) do
+        expect(page).to have_content(group_to_add.name)
+        expect(page).to have_content(role)
+      end
+    end
+  end
+
+  shared_examples 'inviting group fails with allowed email domain error' do
+    specify do
+      group.add_owner(user)
+      group_to_add.add_owner(user)
+      sign_in(user)
+
+      visit project_project_members_path(project)
+      add_group(group_to_add.name, 'Maintainer')
+
+      error_msg = 'Invited group allowed email domains must contain a subset of the allowed email domains'\
+      ' of the root ancestor group'
+      expect(page).to have_content(error_msg)
+    end
+  end
+
+  describe 'inviting group with restricted email domain', :js do
+    shared_examples 'restricted membership by email domain' do
+      let(:user) { create(:user) }
+      let(:project) { create(:project, group: group) }
+
+      context 'shared project group has membership restricted by allowed email domains' do
+        before do
+          create(:allowed_email_domain, group: group.root_ancestor, domain: 'gitlab.com')
+        end
+
+        context 'shared with group with a subset of allowed email domains' do
+          before do
+            create(:allowed_email_domain, group: group_to_add.root_ancestor, domain: 'gitlab.com')
+          end
+
+          it_behaves_like 'adds group without validation error'
+        end
+
+        context 'shared with group containing domains outside the shared group allowed email domains' do
+          before do
+            create(:allowed_email_domain, group: group_to_add.root_ancestor, domain: 'example.com')
+          end
+
+          it_behaves_like 'inviting group fails with allowed email domain error'
+        end
+
+        context 'shared with group does not have membership restricted by allowed domains' do
+          it_behaves_like 'inviting group fails with allowed email domain error'
+        end
+      end
+
+      context 'shared project group does not have membership restricted by allowed domains' do
+        context 'shared with group has membership restricted by allowed email domains' do
+          before do
+            create(:allowed_email_domain, group: group_to_add.root_ancestor, domain: 'example.com')
+          end
+
+          it_behaves_like 'adds group without validation error'
+        end
+
+        context 'shared with group does not have membership restricted by allowed domains' do
+          it_behaves_like 'adds group without validation error'
+        end
+      end
+    end
+
+    context 'shared project group is the root ancestor' do
+      let(:group) { create(:group) }
+      let(:group_to_add) { create(:group) }
+
+      it_behaves_like 'restricted membership by email domain'
+    end
+
+    context 'shared project group is a subgroup' do
+      let(:group) { create(:group, parent: create(:group)) }
+      let(:group_to_add) { create(:group) }
+
+      it_behaves_like 'restricted membership by email domain'
+    end
+
+    context 'shared with group is a subgroup' do
+      let(:group) { create(:group) }
+      let(:group_to_add) { create(:group, parent: create(:group)) }
+
+      it_behaves_like 'restricted membership by email domain'
+    end
+
+    context 'shared project group and shared with group are subgroups' do
+      let(:group) { create(:group, parent: create(:group)) }
+      let(:group_to_add) { create(:group, parent: create(:group)) }
+
+      it_behaves_like 'restricted membership by email domain'
+    end
+  end
+
+  def add_group(name, role, expires_at: nil)
+    click_on 'Invite a group'
+
+    click_on 'Select a group'
+    wait_for_requests
+    click_button name
+    choose_options(role, expires_at)
+
+    click_button 'Invite'
+    wait_for_requests
+  end
+end

ee/spec/models/ee/group_group_link_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe GroupGroupLink do
+  let_it_be(:group) { create(:group) }
+  let_it_be(:group_group_link) { create(:group_group_link, shared_group: group, shared_with_group: create(:group)) }
+
+  describe 'scopes' do
+    describe '.in_shared_group' do
+      it 'provides correct link records' do
+        create(:group_group_link)
+
+        expect(described_class.in_shared_group(group)).to match_array([group_group_link])
+      end
+    end
+
+    describe '.not_in_shared_with_group' do
+      it 'provides correct link records' do
+        not_shared_with_group = create(:group)
+        create(:group_group_link, shared_with_group: not_shared_with_group)
+
+        expect(described_class.not_in_shared_with_group(not_shared_with_group)).to match_array([group_group_link])
+      end
+    end
+  end
+
+  describe 'validations' do
+    describe '#group_with_allowed_email_domains' do
+      shared_examples 'restricted membership by email domain' do
+        subject do
+          build(:group_group_link, shared_group: shared_group, shared_with_group: shared_with_group)
+        end
+
+        context 'shared group has membership restricted by allowed email domains' do
+          before do
+            create(:allowed_email_domain, group: shared_group.root_ancestor, domain: 'gitlab.com')
+            create(:allowed_email_domain, group: shared_group.root_ancestor, domain: 'gitlab.cn')
+          end
+
+          context 'shared with group with a subset of allowed email domains' do
+            before do
+              create(:allowed_email_domain, group: shared_with_group.root_ancestor, domain: 'gitlab.com')
+            end
+
+            it { is_expected.to be_valid }
+          end
+
+          context 'shared with group containing domains outside the shared group allowed email domains' do
+            before do
+              create(:allowed_email_domain, group: shared_with_group.root_ancestor, domain: 'example.com')
+            end
+
+            it { is_expected.to be_invalid }
+          end
+
+          context 'shared with group does not have membership restricted by allowed domains' do
+            it { is_expected.to be_invalid }
+          end
+        end
+
+        context 'shared group does not have membership restricted by allowed domains' do
+          context 'shared with group has membership restricted by allowed email domains' do
+            before do
+              create(:allowed_email_domain, group: shared_with_group.root_ancestor, domain: 'example.com')
+            end
+
+            it { is_expected.to be_valid }
+          end
+
+          context 'shared with group does not have membership restricted by allowed domains' do
+            it { is_expected.to be_valid }
+          end
+        end
+      end
+
+      context 'shared group is the root ancestor' do
+        let_it_be(:shared_group) { create(:group) }
+        let_it_be(:shared_with_group) { create(:group) }
+
+        it_behaves_like 'restricted membership by email domain'
+      end
+
+      context 'shared group is a subgroup' do
+        let_it_be(:shared_group) { create(:group, parent: create(:group)) }
+        let_it_be(:shared_with_group) { create(:group) }
+
+        it_behaves_like 'restricted membership by email domain'
+      end
+
+      context 'shared with group is a subgroup' do
+        let_it_be(:shared_group) { create(:group) }
+        let_it_be(:shared_with_group) { create(:group, parent: create(:group)) }
+
+        it_behaves_like 'restricted membership by email domain'
+      end
+
+      context 'shared and shared with group are subgroups' do
+        let_it_be(:shared_group) { create(:group, parent: create(:group)) }
+        let_it_be(:shared_with_group) { create(:group, parent: create(:group)) }
+
+        it_behaves_like 'restricted membership by email domain'
+      end
+    end
+  end
+end

ee/spec/models/ee/project_group_link_spec.rb

@@ -66,4 +66,83 @@ RSpec.describe ProjectGroupLink do
       end
     end
   end
+
+  describe '#group_with_allowed_email_domains' do
+    let(:shared_project) { create(:project, group: shared_project_group) }
+
+    subject do
+      build(:project_group_link, project: shared_project, group: shared_with_group)
+    end
+
+    shared_examples 'restricted membership by email domain' do
+      context 'shared project group has membership restricted by allowed email domains' do
+        before do
+          create(:allowed_email_domain, group: shared_project_group.root_ancestor, domain: 'gitlab.com')
+          create(:allowed_email_domain, group: shared_project_group.root_ancestor, domain: 'gitlab.cn')
+        end
+
+        context 'shared with group with a subset of allowed email domains' do
+          before do
+            create(:allowed_email_domain, group: shared_with_group.root_ancestor, domain: 'gitlab.com')
+          end
+
+          it { is_expected.to be_valid }
+        end
+
+        context 'shared with group containing domains outside the shared group allowed email domains' do
+          before do
+            create(:allowed_email_domain, group: shared_with_group.root_ancestor, domain: 'example.com')
+          end
+
+          it { is_expected.to be_invalid }
+        end
+
+        context 'shared with group does not have membership restricted by allowed domains' do
+          it { is_expected.to be_invalid }
+        end
+      end
+
+      context 'shared project group does not have membership restricted by allowed domains' do
+        context 'shared with group has membership restricted by allowed email domains' do
+          before do
+            create(:allowed_email_domain, group: shared_with_group.root_ancestor, domain: 'example.com')
+          end
+
+          it { is_expected.to be_valid }
+        end
+
+        context 'shared with group does not have membership restricted by allowed domains' do
+          it { is_expected.to be_valid }
+        end
+      end
+    end
+
+    context 'shared project group is the root ancestor' do
+      let_it_be(:shared_project_group) { create(:group) }
+      let_it_be(:shared_with_group) { create(:group) }
+
+      it_behaves_like 'restricted membership by email domain'
+    end
+
+    context 'shared project group is a subgroup' do
+      let_it_be(:shared_project_group) { create(:group, parent: create(:group)) }
+      let_it_be(:shared_with_group) { create(:group) }
+
+      it_behaves_like 'restricted membership by email domain'
+    end
+
+    context 'shared with group is a subgroup' do
+      let_it_be(:shared_project_group) { create(:group) }
+      let_it_be(:shared_with_group) { create(:group, parent: create(:group)) }
+
+      it_behaves_like 'restricted membership by email domain'
+    end
+
+    context 'shared project group and shared with group are subgroups' do
+      let_it_be(:shared_project_group) { create(:group, parent: create(:group)) }
+      let_it_be(:shared_with_group) { create(:group, parent: create(:group)) }
+
+      it_behaves_like 'restricted membership by email domain'
+    end
+  end
 end

locale/gitlab.pot

@@ -20839,6 +20839,9 @@ msgstr ""
 msgid "Invited"
 msgstr ""
 
+msgid "Invited group allowed email domains must contain a subset of the allowed email domains of the root ancestor group. Go to the group's 'Settings > General' page and check 'Restrict membership by email domain'."
+msgstr ""
+
 msgid "Invocations"
 msgstr ""