Merge branch '329835-restrict-group-project-selections' into 'master'

Exclude self and ancestor in Invite a group to project/group modal

See merge request gitlab-org/gitlab!79036

app/assets/javascripts/invite_members/components/group_select.vue

@@ -38,6 +38,10 @@ export default {
       required: false,
       default: null,
     },
+    invalidGroups: {
+      type: Array,
+      required: true,
+    },
   },
   data() {
     return {
@@ -75,18 +79,26 @@ export default {
       this.isFetching = true;
       return this.fetchGroups()
         .then((response) => {
-          this.groups = response.map((group) => ({
-            id: group.id,
-            name: group.full_name,
-            path: group.path,
-            avatarUrl: group.avatar_url,
-          }));
+          this.groups = this.processGroups(response);
           this.isFetching = false;
         })
         .catch(() => {
           this.isFetching = false;
         });
     }, SEARCH_DELAY),
+    processGroups(response) {
+      const rawGroups = response.map((group) => ({
+        id: group.id,
+        name: group.full_name,
+        path: group.path,
+        avatarUrl: group.avatar_url,
+      }));
+
+      return this.filterOutInvalidGroups(rawGroups);
+    },
+    filterOutInvalidGroups(groups) {
+      return groups.filter((group) => this.invalidGroups.indexOf(group.id) === -1);
+    },
     selectGroup(group) {
       this.selectedGroup = group;
 

app/assets/javascripts/invite_members/components/invite_members_modal.vue

@@ -107,6 +107,10 @@ export default {
       type: Array,
       required: true,
     },
+    invalidGroups: {
+      type: Array,
+      required: true,
+    },
   },
   data() {
     return {
@@ -431,6 +435,7 @@ export default {
           :access-levels="accessLevels"
           :groups-filter="groupSelectFilter"
           :parent-group-id="groupSelectParentId"
+          :invalid-groups="invalidGroups"
           @input="handleMembersTokenSelectClear"
         />
       </gl-form-group>

app/assets/javascripts/invite_members/init_invite_members_modal.js

@@ -40,6 +40,7 @@ export default function initInviteMembersModal() {
           defaultAccessLevel: parseInt(el.dataset.defaultAccessLevel, 10),
           groupSelectFilter: el.dataset.groupsFilter,
           groupSelectParentId: parseInt(el.dataset.parentId, 10),
+          invalidGroups: JSON.parse(el.dataset.invalidGroups || '[]'),
           tasksToBeDoneOptions: JSON.parse(el.dataset.tasksToBeDoneOptions || '[]'),
           projects: JSON.parse(el.dataset.projects || '[]'),
           usersFilter: el.dataset.usersFilter,

app/controllers/projects/project_members_controller.rb

@@ -13,8 +13,7 @@ class Projects::ProjectMembersController < Projects::ApplicationController
   def index
     @sort = params[:sort].presence || sort_value_name
 
-    @skip_groups = @project.invited_group_ids
-    @skip_groups += @project.group.self_and_ancestors_ids if @project.group
+    @skip_groups = @project.related_group_ids
 
     @group_links = @project.project_group_links
     @group_links = @group_links.search(params[:search_groups]) if params[:search_groups].present?

app/helpers/invite_members_helper.rb

@@ -21,6 +21,11 @@ module InviteMembersHelper
   end
 
   def group_select_data(group)
+    # This should only be used for groups to load the invite group modal.
+    # For instance the invite groups modal should not call this from a project scope
+    # this is only to be called in scope of a group context as noted in this thread
+    # https://gitlab.com/gitlab-org/gitlab/-/merge_requests/79036#note_821465513
+    # the group sharing in projects disabling is explained there as well
     if group.root_ancestor.namespace_settings.prevent_sharing_groups_outside_hierarchy
       { groups_filter: 'descendant_groups', parent_id: group.root_ancestor.id }
     else
@@ -32,7 +37,8 @@ module InviteMembersHelper
     dataset = {
       id: source.id,
       name: source.name,
-      default_access_level: Gitlab::Access::GUEST
+      default_access_level: Gitlab::Access::GUEST,
+      invalid_groups: source.related_group_ids
     }
 
     if show_invite_members_for_task?(source)

app/models/project.rb

@@ -2618,6 +2618,14 @@ class Project < ApplicationRecord
     [project&.id, root_group&.id]
   end
 
+  def related_group_ids
+    ids = invited_group_ids
+
+    ids += group.self_and_ancestors_ids if group
+
+    ids
+  end
+
   def package_already_taken?(package_name, package_version, package_type:)
     Packages::Package.with_name(package_name)
       .with_version(package_version)

spec/features/groups/members/manage_groups_spec.rb

@@ -156,6 +156,26 @@ RSpec.describe 'Groups > Members > Manage groups', :js do
       group_outside_hierarchy.add_owner(user)
     end
 
+    context 'when the invite members group modal is enabled' do
+      it 'does not show self or ancestors', :aggregate_failures do
+        group_sibbling = create(:group, parent: group)
+        group_sibbling.add_owner(user)
+
+        visit group_group_members_path(group_within_hierarchy)
+
+        click_on 'Invite a group'
+        click_on 'Select a group'
+        wait_for_requests
+
+        page.within('[data-testid="group-select-dropdown"]') do
+          expect(page).to have_selector("[entity-id='#{group_outside_hierarchy.id}']")
+          expect(page).to have_selector("[entity-id='#{group_sibbling.id}']")
+          expect(page).not_to have_selector("[entity-id='#{group.id}']")
+          expect(page).not_to have_selector("[entity-id='#{group_within_hierarchy.id}']")
+        end
+      end
+    end
+
     context 'when sharing with groups outside the hierarchy is enabled' do
       context 'when the invite members group modal is disabled' do
         before do

spec/features/projects/members/invite_group_spec.rb

@@ -8,7 +8,7 @@ RSpec.describe 'Project > Members > Invite group', :js do
   include Spec::Support::Helpers::Features::MembersHelpers
   include Spec::Support::Helpers::Features::InviteMembersModalHelper
 
-  let(:maintainer) { create(:user) }
+  let_it_be(:maintainer) { create(:user) }
 
   using RSpec::Parameterized::TableSyntax
 
@@ -190,17 +190,26 @@ RSpec.describe 'Project > Members > Invite group', :js do
   end
 
   describe 'the groups dropdown' do
-    context 'with multiple groups to choose from' do
-      let(:project) { create(:project) }
+    let_it_be(:parent_group) { create(:group, :public) }
+    let_it_be(:project_group) { create(:group, :public, parent: parent_group) }
+    let_it_be(:public_sub_subgroup) { create(:group, :public, parent: project_group) }
+    let_it_be(:public_sibbling_group) { create(:group, :public, parent: parent_group) }
+    let_it_be(:private_sibbling_group) { create(:group, :private, parent: parent_group) }
+    let_it_be(:private_membership_group) { create(:group, :private) }
+    let_it_be(:public_membership_group) { create(:group, :public) }
+    let_it_be(:project) { create(:project, group: project_group) }
 
-      it 'includes multiple groups' do
-        project.add_maintainer(maintainer)
-        sign_in(maintainer)
+    before do
+      private_membership_group.add_guest(maintainer)
+      public_membership_group.add_maintainer(maintainer)
 
-        group1 = create(:group)
-        group1.add_owner(maintainer)
-        group2 = create(:group)
-        group2.add_owner(maintainer)
+      sign_in(maintainer)
+    end
+
+    context 'for a project in a nested group' do
+      it 'does not show the groups inherited from projects' do
+        project.add_maintainer(maintainer)
+        public_sibbling_group.add_maintainer(maintainer)
 
         visit project_project_members_path(project)
 
@@ -208,57 +217,36 @@ RSpec.describe 'Project > Members > Invite group', :js do
         click_on 'Select a group'
         wait_for_requests
 
-        expect(page).to have_button(group1.name)
-        expect(page).to have_button(group2.name)
-      end
-    end
-
-    context 'for a project in a nested group' do
-      let!(:parent_group) { create(:group, :public) }
-      let!(:public_subgroup) { create(:group, :public, parent: parent_group) }
-      let!(:public_sub_subgroup) { create(:group, :public, parent: public_subgroup) }
-      let!(:private_subgroup) { create(:group, :private, parent: parent_group) }
-      let!(:project) { create(:project, :public, namespace: public_subgroup) }
+        page.within('[data-testid="group-select-dropdown"]') do
+          expect_to_have_group(public_membership_group)
+          expect_to_have_group(public_sibbling_group)
+          expect_to_have_group(private_membership_group)
 
-      let!(:membership_group) { create(:group, :public) }
-
-      before do
-        project.add_maintainer(maintainer)
-        membership_group.add_guest(maintainer)
-
-        sign_in(maintainer)
+          expect_not_to_have_group(public_sub_subgroup)
+          expect_not_to_have_group(private_sibbling_group)
+          expect_not_to_have_group(parent_group)
+          expect_not_to_have_group(project_group)
+        end
       end
 
-      context 'when invite_members_group_modal feature enabled' do
-        it 'does not show the groups inherited from projects' do
-          visit project_project_members_path(project)
-
-          click_on 'Invite a group'
-          click_on 'Select a group'
-          wait_for_requests
-
-          expect(page).to have_button(membership_group.name)
-          expect(page).not_to have_button(parent_group.name)
-          expect(page).not_to have_button(public_subgroup.name)
-          expect(page).not_to have_button(public_sub_subgroup.name)
-          expect(page).not_to have_button(private_subgroup.name)
-        end
+      it 'does not show the ancestors or project group', :aggregate_failures do
+        parent_group.add_maintainer(maintainer)
 
-        # This behavior should be changed to exclude the ancestor and project
-        # group from the options once issue is fixed for the modal:
-        # https://gitlab.com/gitlab-org/gitlab/-/issues/329835
-        it 'does show ancestors and the project group' do
-          parent_group.add_maintainer(maintainer)
+        visit project_project_members_path(project)
 
-          visit project_project_members_path(project)
+        click_on 'Invite a group'
+        click_on 'Select a group'
+        wait_for_requests
 
-          click_on 'Invite a group'
-          click_on 'Select a group'
-          wait_for_requests
+        page.within('[data-testid="group-select-dropdown"]') do
+          expect_to_have_group(public_membership_group)
+          expect_to_have_group(public_sibbling_group)
+          expect_to_have_group(private_membership_group)
+          expect_to_have_group(public_sub_subgroup)
+          expect_to_have_group(private_sibbling_group)
 
-          expect(page).to have_button(membership_group.name)
-          expect(page).to have_button(parent_group.name)
-          expect(page).to have_button(public_subgroup.name)
+          expect_not_to_have_group(parent_group)
+          expect_not_to_have_group(project_group)
         end
       end
 
@@ -269,21 +257,26 @@ RSpec.describe 'Project > Members > Invite group', :js do
           stub_feature_flags(invite_members_group_modal: false)
         end
 
-        it 'does not show the groups inherited from projects' do
+        it 'does not show the groups inherited from projects', :aggregate_failures do
+          project.add_maintainer(maintainer)
+          public_sibbling_group.add_maintainer(maintainer)
+
           visit project_project_members_path(project)
 
           click_on 'Invite group'
           click_on 'Search for a group'
           wait_for_requests
 
-          expect(group_invite_dropdown).to have_text(membership_group.name)
-          expect(group_invite_dropdown).not_to have_text(parent_group.name)
-          expect(group_invite_dropdown).not_to have_text(public_subgroup.name)
-          expect(group_invite_dropdown).not_to have_text(public_sub_subgroup.name)
-          expect(group_invite_dropdown).not_to have_text(private_subgroup.name)
+          expect(group_invite_dropdown).to have_text(public_membership_group.full_path)
+          expect(group_invite_dropdown).to have_text(public_sibbling_group.full_path)
+          expect(group_invite_dropdown).to have_text(private_membership_group.full_path)
+          expect(group_invite_dropdown).not_to have_text(public_sub_subgroup.full_path)
+          expect(group_invite_dropdown).not_to have_text(private_sibbling_group.full_path)
+          expect(group_invite_dropdown).not_to have_text(parent_group.full_path, exact: true)
+          expect(group_invite_dropdown).not_to have_text(project_group.full_path, exact: true)
         end
 
-        it 'does not show ancestors and the project group' do
+        it 'does not show the ancestors or project group', :aggregate_failures do
           parent_group.add_maintainer(maintainer)
 
           visit project_project_members_path(project)
@@ -292,11 +285,23 @@ RSpec.describe 'Project > Members > Invite group', :js do
           click_on 'Search for a group'
           wait_for_requests
 
-          expect(group_invite_dropdown).to have_text(membership_group.name)
-          expect(group_invite_dropdown).not_to have_text(parent_group.name, exact: true)
-          expect(group_invite_dropdown).not_to have_text(public_subgroup.name, exact: true)
+          expect(group_invite_dropdown).to have_text(public_membership_group.full_path)
+          expect(group_invite_dropdown).to have_text(public_sub_subgroup.full_path)
+          expect(group_invite_dropdown).to have_text(public_sibbling_group.full_path)
+          expect(group_invite_dropdown).to have_text(private_sibbling_group.full_path)
+          expect(group_invite_dropdown).to have_text(private_membership_group.full_path)
+          expect(group_invite_dropdown).not_to have_text(parent_group.full_path, exact: true)
+          expect(group_invite_dropdown).not_to have_text(project_group.full_path, exact: true)
         end
       end
+
+      def expect_to_have_group(group)
+        expect(page).to have_selector("[entity-id='#{group.id}']")
+      end
+
+      def expect_not_to_have_group(group)
+        expect(page).not_to have_selector("[entity-id='#{group.id}']")
+      end
     end
   end
 end

spec/frontend/invite_members/components/group_select_spec.js

@@ -5,19 +5,20 @@ import * as groupsApi from '~/api/groups_api';
 import GroupSelect from '~/invite_members/components/group_select.vue';
 
 const accessLevels = { Guest: 10, Reporter: 20, Developer: 30, Maintainer: 40, Owner: 50 };
+const group1 = { id: 1, full_name: 'Group One', avatar_url: 'test' };
+const group2 = { id: 2, full_name: 'Group Two', avatar_url: 'test' };
+const allGroups = [group1, group2];
 
-const createComponent = () => {
+const createComponent = (props = {}) => {
   return mount(GroupSelect, {
     propsData: {
+      invalidGroups: [],
       accessLevels,
+      ...props,
     },
   });
 };
 
-const group1 = { id: 1, full_name: 'Group One', avatar_url: 'test' };
-const group2 = { id: 2, full_name: 'Group Two', avatar_url: 'test' };
-const allGroups = [group1, group2];
-
 describe('GroupSelect', () => {
   let wrapper;
 
@@ -90,6 +91,20 @@ describe('GroupSelect', () => {
         size: '32',
       });
     });
+
+    describe('when filtering out the group from results', () => {
+      beforeEach(() => {
+        wrapper = createComponent({ invalidGroups: [group1.id] });
+      });
+
+      it('does not find an invalid group', () => {
+        expect(findAvatarByLabel(group1.full_name)).toBe(undefined);
+      });
+
+      it('finds a group that is valid', () => {
+        expect(findAvatarByLabel(group2.full_name).exists()).toBe(true);
+      });
+    });
   });
 
   describe('when group is selected from the dropdown', () => {

spec/frontend/invite_members/components/invite_members_modal_spec.js

@@ -46,6 +46,7 @@ jest.mock('~/lib/utils/url_utility', () => ({
 const id = '1';
 const name = 'test name';
 const isProject = false;
+const invalidGroups = [];
 const inviteeType = 'members';
 const accessLevels = { Guest: 10, Reporter: 20, Developer: 30, Maintainer: 40, Owner: 50 };
 const defaultAccessLevel = 10;
@@ -93,6 +94,7 @@ const createComponent = (data = {}, props = {}) => {
       tasksToBeDoneOptions,
       projects,
       helpLink,
+      invalidGroups,
       ...props,
     },
     data() {

spec/helpers/invite_members_helper_spec.rb

@@ -20,7 +20,8 @@ RSpec.describe InviteMembersHelper do
       attributes = {
         id: project.id,
         name: project.name,
-        default_access_level: Gitlab::Access::GUEST
+        default_access_level: Gitlab::Access::GUEST,
+        invalid_groups: project.related_group_ids
       }
 
       expect(helper.common_invite_modal_dataset(project)).to include(attributes)
@@ -155,4 +156,28 @@ RSpec.describe InviteMembersHelper do
       end
     end
   end
+
+  describe '#group_select_data' do
+    let_it_be(:group) { create(:group) }
+
+    context 'when sharing with groups outside the hierarchy is disabled' do
+      before do
+        group.namespace_settings.update!(prevent_sharing_groups_outside_hierarchy: true)
+      end
+
+      it 'provides the correct attributes' do
+        expect(helper.group_select_data(group)).to eq({ groups_filter: 'descendant_groups', parent_id: group.id })
+      end
+    end
+
+    context 'when sharing with groups outside the hierarchy is enabled' do
+      before do
+        group.namespace_settings.update!(prevent_sharing_groups_outside_hierarchy: false)
+      end
+
+      it 'returns an empty hash' do
+        expect(helper.group_select_data(project.group)).to eq({})
+      end
+    end
+  end
 end

spec/models/project_spec.rb

@@ -7118,6 +7118,29 @@ RSpec.describe Project, factory_default: :keep do
     it { is_expected.to be true }
   end
 
+  describe '#related_group_ids' do
+    let_it_be(:group) { create(:group) }
+    let_it_be(:sub_group) { create(:group, parent: group) }
+
+    context 'when associated with a namespace' do
+      let(:project) { create(:project, namespace: create(:namespace)) }
+      let!(:linked_group) { create(:project_group_link, project: project).group }
+
+      it 'only includes linked groups' do
+        expect(project.related_group_ids).to contain_exactly(linked_group.id)
+      end
+    end
+
+    context 'when associated with a group' do
+      let(:project) { create(:project, group: sub_group) }
+      let!(:linked_group) { create(:project_group_link, project: project).group }
+
+      it 'includes self, ancestors and linked groups' do
+        expect(project.related_group_ids).to contain_exactly(group.id, sub_group.id, linked_group.id)
+      end
+    end
+  end
+
   describe '#package_already_taken?' do
     let_it_be(:namespace) { create(:namespace, path: 'test') }
     let_it_be(:project) { create(:project, :public, namespace: namespace) }