diff --git a/app/models/member.rb b/app/models/member.rb index 9347aca00814f3c1280a01d3291e291be924cd4f..3eaddbf9e2a27a6d5a489339f251d2961c0e8d25 100644 --- a/app/models/member.rb +++ b/app/models/member.rb @@ -190,6 +190,7 @@ class Member < ApplicationRecord scope :with_source_id, ->(source_id) { where(source_id: source_id) } scope :including_source, -> { includes(:source) } + scope :including_user, -> { includes(:user) } scope :distinct_on_user_with_max_access_level, ->(for_object) do valid_objects = %w[Project Namespace] diff --git a/ee/app/services/namespaces/export/base_service.rb b/ee/app/services/namespaces/export/base_service.rb index ee5ae20088ae78eab4488a8b8b11be6c97ded068..0cb8e9db00b2a254ff5dbc7c9158ee16e2b75372 100644 --- a/ee/app/services/namespaces/export/base_service.rb +++ b/ee/app/services/namespaces/export/base_service.rb @@ -4,8 +4,7 @@ module Namespaces module Export class BaseService < ::BaseContainerService def execute - return ServiceResponse.error(message: 'Not available') unless current_user.can?(:export_group_memberships, - container) + return service_not_available unless current_user.can?(:export_group_memberships, container) ServiceResponse.success(payload: csv_builder.render) end @@ -22,6 +21,10 @@ def member_source(member) 'Descendant member' end + + def service_not_available + ServiceResponse.error(message: 'Not available') + end end end end diff --git a/ee/app/services/namespaces/export/detailed_data_service.rb b/ee/app/services/namespaces/export/detailed_data_service.rb index ff7272d1e2ca63364cc1aa94d6445495c93a68ab..91033f5dbcb08321f0eb3a90b599f3d1f7f4eb7e 100644 --- a/ee/app/services/namespaces/export/detailed_data_service.rb +++ b/ee/app/services/namespaces/export/detailed_data_service.rb @@ -1,9 +1,34 @@ # frozen_string_literal: true -# TODO: this service is being implemented in https://gitlab.com/gitlab-org/gitlab/-/merge_requests/157296 module Namespaces module Export class DetailedDataService < BaseService + def execute + return service_not_available unless Feature.enabled?(:members_permissions_detailed_export, container) + + super + end + + private + + def data + GroupMembershipCollector.new(container, current_user).execute + end + + def header_to_value_hash + { + 'Name' => ->(member) { member.name }, + 'Username' => ->(member) { member.username }, + 'Email' => ->(member) { member.email }, + 'Path' => ->(member) { member.group_path }, + 'Role' => ->(member) { member.role }, + 'Membership type' => ->(member) { member.membership_type }, + 'Membership source' => ->(member) { member.membership_source }, + 'Access granted' => ->(member) { member.access_granted }, + 'Access expired' => ->(member) { member.access_expired }, + 'Last activity' => ->(member) { member.last_activity } + } + end end end end diff --git a/ee/app/services/namespaces/export/group_members_type_combinator.rb b/ee/app/services/namespaces/export/group_members_type_combinator.rb new file mode 100644 index 0000000000000000000000000000000000000000..fd84de2659607b379f302ae739794cc1a9822674 --- /dev/null +++ b/ee/app/services/namespaces/export/group_members_type_combinator.rb @@ -0,0 +1,60 @@ +# frozen_string_literal: true + +module Namespaces + module Export + class GroupMembersTypeCombinator + attr_reader :group + + def initialize(group) + @group = group + end + + def execute(group_members, inherited_members) + # first we select only the members who are really direct + # in group_members we will have only shared members remained + direct_members = group_members.extract! { |member| direct_member?(member) } + direct_user_ids = direct_members.map(&:user_id) + + indirect_members = [] + overridden_shared_members = [] + + inherited_members.each do |inherited_member| + shared_member = shared_membership_for(inherited_member, group_members) + type = membership_type(inherited_member, shared_member, direct_user_ids, group_members) + + next if type == :direct + + overridden_shared_members << shared_member if type == :shared + indirect_members << inherited_member + end + + shared_members = group_members - overridden_shared_members + + # we return combination of direct, inherited and shared members + direct_members + indirect_members + shared_members + end + + private + + def shared_membership_for(member, group_members) + group_members.find { |m| m.user_id == member.user_id } + end + + def membership_type(member, shared_member, direct_user_ids, group_members) + return :direct if direct_user_ids.include?(member.user_id) + + return :indirect if group_members.blank? # we can skip if we don't have any further group members + + return :indirect unless shared_member + + return :direct if member.access_level < shared_member.access_level + + :shared + end + + def direct_member?(member) + member.source_id == group.id + end + end + end +end diff --git a/ee/app/services/namespaces/export/group_membership_collector.rb b/ee/app/services/namespaces/export/group_membership_collector.rb new file mode 100644 index 0000000000000000000000000000000000000000..8f76c5c0fdaf535ce3b9dc56bfbb010dd2cdd3ee --- /dev/null +++ b/ee/app/services/namespaces/export/group_membership_collector.rb @@ -0,0 +1,86 @@ +# frozen_string_literal: true + +module Namespaces + module Export + class GroupMembershipCollector + attr_reader :result, :target_group_ancestor_ids, :members, :target_group, :current_user + + def initialize(target_group, current_user) + @result = [] + @members = {} + + @current_user = current_user + @target_group = target_group + @target_group_ancestor_ids = target_group.ancestor_ids + end + + def execute + cursor = { current_id: target_group.id, depth: [target_group.id] } + iterator = Gitlab::Database::NamespaceEachBatch.new(namespace_class: Group, cursor: cursor) + + iterator.each_batch(of: 100) do |ids| + groups = Group.id_in(ids).in_order_of(:id, ids) + + groups.each do |group| + process_group(group) + end + end + + order + end + + private + + def order + result.sort_by { |member| [member.group_id, member.membership_type, member.username] } + end + + def process_group(group) + group_memberships = memberships_for_group(group) + group_parent = group.parent_id unless target_group == group + + update_parent_groups(group_parent) unless group == target_group + + all_group_members = if target_group == group + group_memberships + else + GroupMembersTypeCombinator.new(group) + .execute(group_memberships.to_a, members[group_parent]) + end + + result.concat(transform_data(all_group_members, group)) + + target_group_ancestor_ids << group.id + members[group.id] = all_group_members + end + + def memberships_for_group(group) + # for all groups we retrieve direct and shared members, inherited will be calculated from the ancestors + relations = [:direct, :shared_from_groups] + + # for root group we have to retrieve also inherited members as there is no ancestor to calculate them from + relations << :inherited if group == target_group + + GroupMembersFinder.new(group, current_user).execute(include_relations: relations) + .including_source.including_user + end + + def update_parent_groups(group_parent) + return if target_group_ancestor_ids.empty? + return if group_parent == target_group_ancestor_ids.last + + parent_index = target_group_ancestor_ids.find_index(group_parent) + count_to_remove = target_group_ancestor_ids.size - parent_index - 1 + target_group_ancestor_ids.pop(count_to_remove) + end + + def transform_data(memberships, group) + return [] unless memberships + + memberships.map do |member| + ::Namespaces::Export::Member.new(member, group, target_group_ancestor_ids) + end + end + end + end +end diff --git a/ee/app/services/namespaces/export/member.rb b/ee/app/services/namespaces/export/member.rb new file mode 100644 index 0000000000000000000000000000000000000000..ffafd10c4ed22c143edf1a6af81c1341025bd6d1 --- /dev/null +++ b/ee/app/services/namespaces/export/member.rb @@ -0,0 +1,56 @@ +# frozen_string_literal: true + +module Namespaces + module Export + class Member + include ::ActiveModel::Attributes + include ::ActiveModel::AttributeAssignment + + attribute :id, :integer + attribute :name, :string + attribute :username, :string + attribute :email, :string + attribute :group_id, :integer + attribute :group_path, :string + attribute :role, :string + attribute :membership_type, :string + attribute :membership_source, :string + attribute :access_granted, :string + attribute :access_expired, :string + attribute :access_level, :integer + attribute :last_activity, :string + + def initialize(member, group, parent_groups) + super() + + map_attributes(member, group, parent_groups) + end + + def map_attributes(member, group, parent_groups) + membership_type = if member.source == group + 'direct' + elsif parent_groups.include?(member.source_id) + 'inherited' + else + 'shared' + end + + assign_attributes( + id: member.id, + name: member.user.name, + username: member.user.username, + email: member.user.email, + group_id: group.id, + group_path: group.full_path, + access_level: member.access_level, + role: member.present.access_level_for_export, + membership_type: membership_type, + membership_source: member.source.full_path, + access_granted: member.created_at.to_fs(:csv), + access_expired: member.expires_at, + last_activity: member.last_activity_on + ) + end + end + end +end diff --git a/ee/config/feature_flags/beta/members_permissions_detailed_export.yml b/ee/config/feature_flags/beta/members_permissions_detailed_export.yml new file mode 100644 index 0000000000000000000000000000000000000000..5728d72fcb5800964bc6fec53ade83306fc4e1b7 --- /dev/null +++ b/ee/config/feature_flags/beta/members_permissions_detailed_export.yml @@ -0,0 +1,9 @@ +--- +name: members_permissions_detailed_export +feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/460477 +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/157296 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/467359 +milestone: '17.4' +group: group::authorization +type: beta +default_enabled: false diff --git a/ee/spec/services/namespaces/export/detailed_data_service_spec.rb b/ee/spec/services/namespaces/export/detailed_data_service_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..1af165d28d731d13cb6322f6c1dc3b6ffd131fb7 --- /dev/null +++ b/ee/spec/services/namespaces/export/detailed_data_service_spec.rb @@ -0,0 +1,189 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Namespaces::Export::DetailedDataService, feature_category: :system_access do + include_context 'with group members shared context' + + let(:current_user) { users[0] } + let(:requested_group) { group } + + subject(:export) { described_class.new(container: requested_group, current_user: current_user).execute } + + shared_examples 'not available' do + it 'returns a failed response' do + response = export + + expect(response).not_to be_success + expect(response.message).to eq('Not available') + end + end + + describe '#execute' do + context 'when unlicensed' do + before do + stub_licensed_features(export_user_permissions: false) + end + + it_behaves_like 'not available' + end + + context 'when licensed' do + before do + stub_licensed_features(export_user_permissions: true) + end + + context 'when current_user is a group maintainer' do + let(:current_user) { users[1] } + + it_behaves_like 'not available' + end + + context 'when current user is a group owner' do + shared_examples 'exporting correct data' do + it 'is successful' do + expect(export).to be_success + end + + it 'returns correct data' do + headers = ['Name', 'Username', 'Email', 'Path', 'Role', 'Membership type', 'Membership source', + 'Access granted', 'Access expired', 'Last activity'] + + expect(data).to match_array([headers] + expected_result) + end + + it 'avoids N+1 queries' do + count = ActiveRecord::QueryRecorder.new { export } + + create(:group_member, :owner, group: requested_group, user: create(:user)) + + expect { described_class.new(container: requested_group, current_user: current_user).execute } + .not_to exceed_query_limit(count).with_threshold(1) + end + end + + let(:data) { CSV.parse(export.payload) } + let(:group_members) do + [ + user_data(0) + [group.full_path, 'Owner', 'direct', group.full_path] + member_data(group_owner_1), + user_data(1) + [group.full_path, 'Maintainer', 'direct', group.full_path] + member_data(group_maintainer_2), + user_data(2) + [group.full_path, 'Developer', 'direct', group.full_path] + member_data(group_developer_3) + ] + end + + let(:sub_group_1_members) do + [ + user_data(1) + [sub_group_1.full_path, 'Owner', 'direct', + sub_group_1.full_path] + member_data(sub_group_1_owner_2), + user_data(0) + [sub_group_1.full_path, 'Owner', 'inherited', group.full_path] + member_data(group_owner_1), + user_data(2) + [sub_group_1.full_path, 'Developer', 'inherited', + group.full_path] + member_data(group_developer_3), + user_data(4) + [sub_group_1.full_path, 'Developer', 'shared', + shared_group.full_path] + member_data(shared_maintainer_5), + user_data(5) + [sub_group_1.full_path, 'Developer', 'shared', + shared_group.full_path] + member_data(shared_maintainer_6) + ] + end + + let(:sub_group_2_members) do + [ + user_data(0) + [sub_group_2.full_path, 'Owner', 'inherited', group.full_path] + member_data(group_owner_1), + user_data(1) + [sub_group_2.full_path, 'Maintainer', 'inherited', + group.full_path] + member_data(group_maintainer_2), + user_data(2) + [sub_group_2.full_path, 'Developer', 'inherited', + group.full_path] + member_data(group_developer_3) + ] + end + + let(:sub_sub_group_1_members) do + [ + user_data(3) + [sub_sub_group_1.full_path, 'Owner', 'direct', + sub_sub_group_1.full_path] + member_data(sub_sub_group_owner_4), + user_data(4) + [sub_sub_group_1.full_path, 'Owner', 'direct', + sub_sub_group_1.full_path] + member_data(sub_sub_group_owner_5), + user_data(0) + [sub_sub_group_1.full_path, 'Owner', 'inherited', + group.full_path] + member_data(group_owner_1), + user_data(1) + [sub_sub_group_1.full_path, 'Owner', 'inherited', + sub_group_1.full_path] + member_data(sub_group_1_owner_2), + user_data(2) + [sub_sub_group_1.full_path, 'Developer', 'inherited', + group.full_path] + member_data(group_developer_3), + user_data(5) + [sub_sub_group_1.full_path, 'Developer', 'shared', + shared_group.full_path] + member_data(shared_maintainer_6) + ] + end + + let(:sub_sub_sub_group_1_members) do + [ + user_data(0) + [sub_sub_sub_group_1.full_path, 'Owner', 'inherited', + group.full_path] + member_data(group_owner_1), + user_data(1) + [sub_sub_sub_group_1.full_path, 'Owner', 'inherited', + sub_group_1.full_path] + member_data(sub_group_1_owner_2), + user_data(2) + [sub_sub_sub_group_1.full_path, 'Developer', 'inherited', + group.full_path] + member_data(group_developer_3), + user_data(3) + [sub_sub_sub_group_1.full_path, 'Owner', 'inherited', + sub_sub_group_1.full_path] + member_data(sub_sub_group_owner_4), + user_data(4) + [sub_sub_sub_group_1.full_path, 'Owner', 'inherited', + sub_sub_group_1.full_path] + member_data(sub_sub_group_owner_5), + user_data(5) + [sub_sub_sub_group_1.full_path, 'Developer', 'shared', + shared_group.full_path] + member_data(shared_maintainer_6) + ] + end + + def user_data(user_id) + user = users[user_id] + + [user.name, user.username, user.email] + end + + def member_data(member) + [member.created_at.to_fs(:csv), nil, member.reload.last_activity_on.to_fs(:csv)] + end + + context 'when members_permissions_detailed_export feature flag is disabled' do + before do + stub_feature_flags(members_permissions_detailed_export: false) + end + + it_behaves_like 'not available' + end + + context 'when members_permissions_detailed_export feature flag is enabled' do + before do + stub_feature_flags(members_permissions_detailed_export: true) + end + + context 'for group' do + let(:requested_group) { group } + let(:expected_result) do + group_members + sub_group_1_members + sub_group_2_members + + sub_sub_group_1_members + sub_sub_sub_group_1_members + end + + it_behaves_like 'exporting correct data' + end + + context 'for subgroup' do + let(:requested_group) { sub_group_1 } + let(:expected_result) { sub_group_1_members + sub_sub_group_1_members + sub_sub_sub_group_1_members } + + it_behaves_like 'exporting correct data' + end + + context 'for sub_sub_group' do + let(:requested_group) { sub_sub_group_1 } + let(:expected_result) { sub_sub_group_1_members + sub_sub_sub_group_1_members } + + it_behaves_like 'exporting correct data' + end + + context 'for sub_sub_sub_group_1' do + let(:requested_group) { sub_sub_sub_group_1 } + let(:expected_result) { sub_sub_sub_group_1_members } + + it_behaves_like 'exporting correct data' + end + end + end + end + end +end diff --git a/ee/spec/services/namespaces/export/group_members_type_combinator_spec.rb b/ee/spec/services/namespaces/export/group_members_type_combinator_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..608999f9a15f6bb9f0a1567c512eb842cec9851d --- /dev/null +++ b/ee/spec/services/namespaces/export/group_members_type_combinator_spec.rb @@ -0,0 +1,25 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Namespaces::Export::GroupMembersTypeCombinator, feature_category: :system_access do + include_context 'with group members shared context' + + let(:requested_group) { sub_sub_sub_group_1 } + let(:group_members) do + [shared_maintainer_5, shared_maintainer_6] + end + + let(:inherited_members) do + [group_owner_1, sub_group_1_owner_2, group_developer_3, sub_sub_group_owner_4, sub_sub_group_owner_5] + end + + subject(:process_group) { described_class.new(requested_group).execute(group_members, inherited_members) } + + it 'returns the members with the higher access level' do + expected_result = [group_owner_1, sub_group_1_owner_2, group_developer_3, sub_sub_group_owner_4, + sub_sub_group_owner_5, shared_maintainer_6] + + expect(process_group).to match_array(expected_result) + end +end diff --git a/ee/spec/services/namespaces/export/group_membership_collector_spec.rb b/ee/spec/services/namespaces/export/group_membership_collector_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..7000f88a82009eb4b24b4f6d2676b7c4ef30a367 --- /dev/null +++ b/ee/spec/services/namespaces/export/group_membership_collector_spec.rb @@ -0,0 +1,67 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Namespaces::Export::GroupMembershipCollector, feature_category: :system_access do + include_context 'with group members shared context' + + let(:current_user) { users[0] } + let(:requested_group) { group } + + subject(:prepare_data) { described_class.new(requested_group, current_user).execute } + + describe '#execute' do + let(:member_klass) { Namespaces::Export::Member } + let(:group_member) { member_klass.new(group: group, membership_type: 'direct', username: 'Anna') } + + def init_final_member(group, db_member) + Namespaces::Export::Member.new( + group: group, membership_type: 'direct', username: db_member.user.username + ) + end + + def mock_root_group(members) + finder = instance_double(GroupMembersFinder) + members_relation = class_double(GroupMember) + + expect(GroupMembersFinder).to receive(:new).with(group, current_user).and_return(finder) + expect(finder).to receive(:execute).with(include_relations: [:direct, :shared_from_groups, + :inherited]).and_return(members_relation) + + expect(members_relation).to receive(:including_source).and_return(members_relation) + expect(members_relation).to receive(:including_user).and_return(members) + end + + def mock_group(group, members, _parent_groups) + finder = instance_double(GroupMembersFinder) + members_relation = class_double(GroupMember) + + expect(GroupMembersFinder).to receive(:new).with(group, current_user).and_return(finder) + expect(finder).to receive(:execute).with(include_relations: [:direct, :shared_from_groups]) + .and_return(members_relation) + + expect(members_relation).to receive(:including_source).and_return(members_relation) + expect(members_relation).to receive(:including_user).and_return(members) + + combinator = instance_double(Namespaces::Export::GroupMembersTypeCombinator) + + expect(Namespaces::Export::GroupMembersTypeCombinator).to receive(:new).with(group).and_return(combinator) + expect(combinator).to receive(:execute).and_return(members) + end + + it 'returns correct data' do + mock_root_group([group_owner_1]) + + mock_group(sub_group_1, [sub_group_1_owner_2], [group.id]) + mock_group(sub_sub_group_1, [sub_sub_group_owner_4], [group.id, sub_group_1.id]) + mock_group(sub_sub_sub_group_1, [shared_maintainer_5], [group.id, sub_group_1.id, sub_sub_group_1.id]) + mock_group(sub_group_2, [group_owner_1], [group.id]) + + result = prepare_data + + expect(result.count).to eq(5) + expect(result.map(&:id)).to eq([group_owner_1.id, + sub_group_1_owner_2.id, group_owner_1.id, sub_sub_group_owner_4.id, shared_maintainer_5.id]) + end + end +end diff --git a/ee/spec/services/namespaces/export/member_spec.rb b/ee/spec/services/namespaces/export/member_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..4685dca3c74830210ef199000fdf757016b49ceb --- /dev/null +++ b/ee/spec/services/namespaces/export/member_spec.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Namespaces::Export::Member, feature_category: :system_access do + let_it_be(:group) { create(:group) } + let_it_be(:parent_groups) { [] } + let_it_be(:group_member) { create(:group_member, :developer, group: group) } + + describe 'initialization' do + it 'creates a new instance correctly' do + member = described_class.new(group_member, group, parent_groups) + + aggregate_failures do + expect(member.name).to eq(group_member.user.name) + expect(member.username).to eq(group_member.user.username) + expect(member.role).to eq('Developer') + expect(member.membership_type).to eq('direct') + expect { member.unknown }.to raise_error(NoMethodError) + end + end + end +end diff --git a/spec/support/shared_contexts/services/namespaces/group_members_shared_context.rb b/spec/support/shared_contexts/services/namespaces/group_members_shared_context.rb new file mode 100644 index 0000000000000000000000000000000000000000..8f075779ae0727e440213f89aeadb4c15a74d40e --- /dev/null +++ b/spec/support/shared_contexts/services/namespaces/group_members_shared_context.rb @@ -0,0 +1,25 @@ +# frozen_string_literal: true + +RSpec.shared_context 'with group members shared context' do + let_it_be(:group) { create(:group) } + let_it_be(:sub_group_1) { create(:group, parent: group) } + let_it_be(:sub_group_2) { create(:group, parent: group) } + let_it_be(:sub_sub_group_1) { create(:group, parent: sub_group_1) } + let_it_be(:sub_sub_sub_group_1) { create(:group, parent: sub_sub_group_1) } + let_it_be(:shared_group) { create(:group) } + + let_it_be(:link) do + create(:group_group_link, shared_group: sub_group_1, shared_with_group: shared_group) + end + + let_it_be(:users) { create_list(:user, 6) } + + let_it_be(:group_owner_1) { create(:group_member, :owner, group: group, user: users[0]) } + let_it_be(:group_maintainer_2) { create(:group_member, :maintainer, group: group, user: users[1]) } + let_it_be(:sub_group_1_owner_2) { create(:group_member, :owner, group: sub_group_1, user: users[1]) } + let_it_be(:group_developer_3) { create(:group_member, :developer, group: group, user: users[2]) } + let_it_be(:sub_sub_group_owner_4) { create(:group_member, :owner, group: sub_sub_group_1, user: users[3]) } + let_it_be(:sub_sub_group_owner_5) { create(:group_member, :owner, group: sub_sub_group_1, user: users[4]) } + let_it_be(:shared_maintainer_5) { create(:group_member, :maintainer, group: shared_group, user: users[4]) } + let_it_be(:shared_maintainer_6) { create(:group_member, :maintainer, group: shared_group, user: users[5]) } +end