From 8db55cdbc9c5307c06852b1e0d64b9535b028a2f Mon Sep 17 00:00:00 2001 From: Luke Duncalfe Date: Fri, 29 Mar 2024 12:55:02 +1300 Subject: [PATCH 1/3] Preserve indirect memberships We import "shared" and "inherited shared" memberships from the source https://gitlab.com/gitlab-org/gitlab/-/blob/39cae3e7c50bd508c9be35444b1f20159c051cbd/lib/bulk_imports/common/graphql/get_members_query.rb#L83-93 but previously we would create direct memberships on the destination even if the user had a shared and inherited shared membership within the destination. This change preserves existing shared and inherited shared memberships on the destination, unless the existing membership is of a lower permission level than the source. Changelog: fixed --- doc/user/group/import/index.md | 15 +++- .../common/pipelines/members_pipeline.rb | 29 +++++-- .../common/pipelines/members_pipeline_spec.rb | 84 ++++++++++++++++++- 3 files changed, 115 insertions(+), 13 deletions(-) diff --git a/doc/user/group/import/index.md b/doc/user/group/import/index.md index 9ab786eec02921..fc75ff2a72f02b 100644 --- a/doc/user/group/import/index.md +++ b/doc/user/group/import/index.md @@ -349,7 +349,7 @@ Group items that are migrated to the destination GitLab instance include: | Group labels 2 | [GitLab 13.9](https://gitlab.com/gitlab-org/gitlab/-/issues/292429) | | Iterations | [GitLab 13.10](https://gitlab.com/gitlab-org/gitlab/-/issues/292428) | | Iteration cadences | [GitLab 15.4](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/96570) | -| Members 3 | [GitLab 13.9](https://gitlab.com/gitlab-org/gitlab/-/issues/299415) | +| Members 3 4 | [GitLab 13.9](https://gitlab.com/gitlab-org/gitlab/-/issues/299415) | | Group milestones | [GitLab 13.10](https://gitlab.com/gitlab-org/gitlab/-/issues/292427) | | Namespace settings | [GitLab 14.10](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/85128) | | Release milestones | [GitLab 15.0](https://gitlab.com/gitlab-org/gitlab/-/issues/339422) | @@ -365,6 +365,10 @@ Group items that are migrated to the destination GitLab instance include: 1. Group members are associated with the imported group if the user: - Already exists in the destination GitLab instance. - Has a public email in the source GitLab instance that matches a confirmed email in the destination GitLab instance. +1. All [direct and indirect](../../../user/project/members/index.md#membership-types) group members are imported. + Importing of shared and inherited shared members [introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/129017) + in GitLab 16.3. Group members are imported as [direct members](../../project/members/index.md#membership-types), unless + they are already an indirect member of the imported group with the same or higher [permission](../../../user/permissions.md). ### Excluded items @@ -425,7 +429,7 @@ Project items that are migrated to the destination GitLab instance include: | Issue boards | [GitLab 14.4](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/71661) | | Labels | [GitLab 14.4](https://gitlab.com/gitlab-org/gitlab/-/issues/339419) | | LFS Objects | [GitLab 14.8](https://gitlab.com/gitlab-org/gitlab/-/issues/339405) | -| Members | [GitLab 14.8](https://gitlab.com/gitlab-org/gitlab/-/issues/341886) | +| Members 2 3 | [GitLab 14.8](https://gitlab.com/gitlab-org/gitlab/-/issues/341886) | | Merge requests | [GitLab 14.5](https://gitlab.com/gitlab-org/gitlab/-/issues/339403) | | Push rules | [GitLab 14.6](https://gitlab.com/gitlab-org/gitlab/-/issues/339403) | | Milestones | [GitLab 14.5](https://gitlab.com/gitlab-org/gitlab/-/issues/339417) | @@ -445,6 +449,13 @@ Project items that are migrated to the destination GitLab instance include: 1. Imported branches respect the [default branch protection settings](../../project/protected_branches.md) of the destination group, which could cause an unprotected branch to be imported as protected. +1. Project members are associated with the imported project if the user: + - Already exists in the destination GitLab instance. + - Has a public email in the source GitLab instance that matches a confirmed email in the destination GitLab instance. +1. All [direct and indirect](../../../user/project/members/index.md#membership-types) project members are imported. +Importing of shared and inherited shared members [introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/129017) +in GitLab 16.3. Project members are imported as [direct members](../../project/members/index.md#membership-types), unless +they are already an indirect member of the imported project with the same or higher [permission](../../../user/permissions.md). ### Issue-related items diff --git a/lib/bulk_imports/common/pipelines/members_pipeline.rb b/lib/bulk_imports/common/pipelines/members_pipeline.rb index 90df8453d7789c..dbaae8f6ef23af 100644 --- a/lib/bulk_imports/common/pipelines/members_pipeline.rb +++ b/lib/bulk_imports/common/pipelines/members_pipeline.rb @@ -6,6 +6,9 @@ module Pipelines class MembersPipeline include Pipeline + GROUP_MEMBER_RELATIONS = %i[direct inherited shared_from_groups].freeze + PROJECT_MEMBER_RELATIONS = %i[direct inherited invited_groups shared_into_ancestors].freeze + transformer Common::Transformers::ProhibitedAttributesTransformer transformer Common::Transformers::MemberAttributesTransformer @@ -40,15 +43,27 @@ def graphql_extractor end def existing_user_membership(user_id) - members_finder.execute.find_by_user_id(user_id) + execute_finder.find_by_user_id(user_id) + end + + def finder + @finder ||= if context.entity.group? + ::GroupMembersFinder.new(portable, current_user) + else + ::MembersFinder.new(portable, current_user) + end + end + + def execute_finder + finder.execute(include_relations: finder_relations) end - def members_finder - @members_finder ||= if context.entity.group? - ::GroupMembersFinder.new(portable, current_user) - else - ::MembersFinder.new(portable, current_user) - end + def finder_relations + if context.entity.group? + GROUP_MEMBER_RELATIONS + else + PROJECT_MEMBER_RELATIONS + end end end end diff --git a/spec/lib/bulk_imports/common/pipelines/members_pipeline_spec.rb b/spec/lib/bulk_imports/common/pipelines/members_pipeline_spec.rb index 5fc0c8fa239561..39ccdeb344d064 100644 --- a/spec/lib/bulk_imports/common/pipelines/members_pipeline_spec.rb +++ b/spec/lib/bulk_imports/common/pipelines/members_pipeline_spec.rb @@ -128,7 +128,7 @@ def extracted_data(email:, has_next_page: false) end end - context 'when membership with higher access level exists in parent group' do + context 'when membership has higher access level than membership in parent group' do it 'creates new direct membership' do data = member_data.merge(access_level: Gitlab::Access::MAINTAINER) @@ -140,7 +140,7 @@ def extracted_data(email:, has_next_page: false) end end - context 'when membership with lower access level exists in parent group' do + context 'when membership has lower access level than membership in parent group' do it 'does not create new membership' do data = member_data.merge(access_level: Gitlab::Access::GUEST) @@ -152,20 +152,96 @@ def extracted_data(email:, has_next_page: false) end context 'when importing to group' do - let(:portable) { create(:group) } + let_it_be(:portable) { create(:group) } + let(:portable_with_parent) { create(:group, parent: parent) } let(:entity) { create(:bulk_import_entity, :group_entity, group: portable, bulk_import: bulk_import) } let(:entity_with_parent) { create(:bulk_import_entity, :group_entity, group: portable_with_parent, bulk_import: bulk_import) } include_examples 'members import' + + context 'when user is a member of group through group sharing' do + before_all do + group = create(:group) + group.add_developer(member_user1) + create(:group_group_link, shared_group: portable, shared_with_group: group) + end + + it 'does not create new membership' do + expect { pipeline.load(context, member_data) }.not_to change(Member, :count) + end + + context 'when membership is a higher access level' do + it 'creates new direct membership' do + data = member_data.merge(access_level: Gitlab::Access::MAINTAINER) + + expect { pipeline.load(context, data) }.to change(portable.members, :count).by(1) + + member = portable.members.find_by_user_id(member_user1.id) + + expect(member.access_level).to eq(Gitlab::Access::MAINTAINER) + end + end + end end context 'when importing to project' do - let(:portable) { create(:project) } + let_it_be(:portable) { create(:project) } + let(:portable_with_parent) { create(:project, namespace: parent) } let(:entity) { create(:bulk_import_entity, :project_entity, project: portable, bulk_import: bulk_import) } let(:entity_with_parent) { create(:bulk_import_entity, :project_entity, project: portable_with_parent, bulk_import: bulk_import) } include_examples 'members import' + + context 'when project is shared with a group, and user is a direct member of the group' do + before_all do + group = create(:group) + group.add_developer(member_user1) + create(:project_group_link, project: portable, group: group) + end + + it 'does not create new membership' do + expect { pipeline.load(context, member_data) }.not_to change(Member, :count) + end + + context 'when membership is a higher access level' do + it 'creates new direct membership' do + data = member_data.merge(access_level: Gitlab::Access::MAINTAINER) + + expect { pipeline.load(context, data) }.to change(portable.members, :count).by(1) + + member = portable.members.find_by_user_id(member_user1.id) + + expect(member.access_level).to eq(Gitlab::Access::MAINTAINER) + end + end + end + + context 'when parent group is shared with other group, and user is a member of other group' do + let(:tracker) { create(:bulk_import_tracker, entity: entity_with_parent) } + + before do + group = create(:group) + group.add_developer(member_user1) + create(:group_group_link, shared_group: parent, shared_with_group: group) + end + + it 'does not create new membership' do + expect { pipeline.load(context, member_data) }.not_to change(Member, :count) + end + + context 'when membership is a higher access level' do + it 'creates new direct membership' do + data = member_data.merge(access_level: Gitlab::Access::MAINTAINER) + + expect { pipeline.load(context, data) }.to change(portable_with_parent.members, :count).by(1) + + member = portable_with_parent.members.find_by_user_id(member_user1.id) + + expect(member.access_level).to eq(Gitlab::Access::MAINTAINER) + end + end + end end end -- GitLab From 247a9548ac382fc9993a68a80a8885faf6222211 Mon Sep 17 00:00:00 2001 From: Luke Duncalfe Date: Wed, 3 Apr 2024 15:43:35 +1300 Subject: [PATCH 2/3] Add reviewer feedback --- doc/user/group/import/index.md | 34 ++++++++++++++++++---------------- 1 file changed, 18 insertions(+), 16 deletions(-) diff --git a/doc/user/group/import/index.md b/doc/user/group/import/index.md index fc75ff2a72f02b..d99a244b8e09b3 100644 --- a/doc/user/group/import/index.md +++ b/doc/user/group/import/index.md @@ -161,6 +161,20 @@ After migration: If you used a private network on your source instance to hide content from the general public, make sure to have a similar setup on the destination instance, or to import into a private group. +## Memberships + +> - Importing of shared and inherited shared members was [introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/129017) in GitLab 16.3. +> - [Before GitLab 16.11](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/148220), all shared and inherited shared members were added as direct members even when they were already shared or inherited shared members of the imported group or project. + +Group and project members are imported if the [user account prerequisites](#user-accounts) are followed. + +All [direct and indirect](../../../user/project/members/index.md#membership-types) members are imported. + +Indirect members are imported as [direct members](../../project/members/index.md#membership-types) if: + +- They are not already an indirect member. +- They are an indirect member, but have a lower [permission](../../../user/permissions.md). + ## Prerequisites > - Requirement for Maintainer role instead of Developer role introduced in GitLab 16.0 and backported to GitLab 15.11.1 and GitLab 15.10.5. @@ -349,7 +363,7 @@ Group items that are migrated to the destination GitLab instance include: | Group labels 2 | [GitLab 13.9](https://gitlab.com/gitlab-org/gitlab/-/issues/292429) | | Iterations | [GitLab 13.10](https://gitlab.com/gitlab-org/gitlab/-/issues/292428) | | Iteration cadences | [GitLab 15.4](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/96570) | -| Members 3 4 | [GitLab 13.9](https://gitlab.com/gitlab-org/gitlab/-/issues/299415) | +| Members 3 | [GitLab 13.9](https://gitlab.com/gitlab-org/gitlab/-/issues/299415) | | Group milestones | [GitLab 13.10](https://gitlab.com/gitlab-org/gitlab/-/issues/292427) | | Namespace settings | [GitLab 14.10](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/85128) | | Release milestones | [GitLab 15.0](https://gitlab.com/gitlab-org/gitlab/-/issues/339422) | @@ -362,13 +376,7 @@ Group items that are migrated to the destination GitLab instance include: metadata [introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/63551) in GitLab 14.0. 1. Group Labels cannot retain any associated Label Priorities during import. These labels will need to be re-prioritized manually once the relevant Project is migrated to the destination instance. -1. Group members are associated with the imported group if the user: - - Already exists in the destination GitLab instance. - - Has a public email in the source GitLab instance that matches a confirmed email in the destination GitLab instance. -1. All [direct and indirect](../../../user/project/members/index.md#membership-types) group members are imported. - Importing of shared and inherited shared members [introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/129017) - in GitLab 16.3. Group members are imported as [direct members](../../project/members/index.md#membership-types), unless - they are already an indirect member of the imported group with the same or higher [permission](../../../user/permissions.md). +1. See [Memberships](#memberships). ### Excluded items @@ -429,7 +437,7 @@ Project items that are migrated to the destination GitLab instance include: | Issue boards | [GitLab 14.4](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/71661) | | Labels | [GitLab 14.4](https://gitlab.com/gitlab-org/gitlab/-/issues/339419) | | LFS Objects | [GitLab 14.8](https://gitlab.com/gitlab-org/gitlab/-/issues/339405) | -| Members 2 3 | [GitLab 14.8](https://gitlab.com/gitlab-org/gitlab/-/issues/341886) | +| Members 2 | [GitLab 14.8](https://gitlab.com/gitlab-org/gitlab/-/issues/341886) | | Merge requests | [GitLab 14.5](https://gitlab.com/gitlab-org/gitlab/-/issues/339403) | | Push rules | [GitLab 14.6](https://gitlab.com/gitlab-org/gitlab/-/issues/339403) | | Milestones | [GitLab 14.5](https://gitlab.com/gitlab-org/gitlab/-/issues/339417) | @@ -449,13 +457,7 @@ Project items that are migrated to the destination GitLab instance include: 1. Imported branches respect the [default branch protection settings](../../project/protected_branches.md) of the destination group, which could cause an unprotected branch to be imported as protected. -1. Project members are associated with the imported project if the user: - - Already exists in the destination GitLab instance. - - Has a public email in the source GitLab instance that matches a confirmed email in the destination GitLab instance. -1. All [direct and indirect](../../../user/project/members/index.md#membership-types) project members are imported. -Importing of shared and inherited shared members [introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/129017) -in GitLab 16.3. Project members are imported as [direct members](../../project/members/index.md#membership-types), unless -they are already an indirect member of the imported project with the same or higher [permission](../../../user/permissions.md). +1. See [Memberships](#memberships). ### Issue-related items -- GitLab From 5a5ebfbe7874394928e1707359bfe3f373c31ac2 Mon Sep 17 00:00:00 2001 From: Luke Duncalfe Date: Mon, 8 Apr 2024 04:50:53 +0000 Subject: [PATCH 3/3] Apply 1 suggestion(s) to 1 file(s) --- doc/user/group/import/index.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/user/group/import/index.md b/doc/user/group/import/index.md index d99a244b8e09b3..49ae181b1864f0 100644 --- a/doc/user/group/import/index.md +++ b/doc/user/group/import/index.md @@ -164,7 +164,7 @@ make sure to have a similar setup on the destination instance, or to import into ## Memberships > - Importing of shared and inherited shared members was [introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/129017) in GitLab 16.3. -> - [Before GitLab 16.11](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/148220), all shared and inherited shared members were added as direct members even when they were already shared or inherited shared members of the imported group or project. +> - [Introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/148220) in GitLab 16.11, shared and inherited shared members are no longer imported as direct members if they are already shared or inherited shared members of the imported group or project. Group and project members are imported if the [user account prerequisites](#user-accounts) are followed. -- GitLab