diff --git a/app/models/member.rb b/app/models/member.rb index d3c502410affd6627fcd9175618da0d6f74cac6c..d9517c6c1ad57630d2b831ccff01a0e399925140 100644 --- a/app/models/member.rb +++ b/app/models/member.rb @@ -293,6 +293,9 @@ class Member < ApplicationRecord end attribute :notification_level, default: -> { NotificationSetting.levels[:global] } + # Only false when the current user is a member of the shared group or project but not of the invited private group + # so the current user can't see the source of the membership. + attribute :is_source_accessible_to_current_user, default: true class << self def search(query) diff --git a/app/models/members/members/invited_private_group_accessibility_assigner.rb b/app/models/members/members/invited_private_group_accessibility_assigner.rb new file mode 100644 index 0000000000000000000000000000000000000000..cd92ed797725ee78b3e2c8772e9c216c15da91bd --- /dev/null +++ b/app/models/members/members/invited_private_group_accessibility_assigner.rb @@ -0,0 +1,68 @@ +# frozen_string_literal: true + +module Members + # We allow the current user to see the invited private group when the current user is a member of the shared group to + # allow better collaboration between the two groups even though the current user is not a member of the invited group. + # We don't allow the current user to see the source of membership i.e. the group name, path, and other group info as + # it's sensitive information if the current user is not an owner of the group or at least maintainer of the project. + # This class deals with setting `is_source_accessible_to_current_user` which is used to hide or show the source of + # memberships as per the above cases. + class InvitedPrivateGroupAccessibilityAssigner + include Gitlab::Allowable + include Gitlab::Utils::StrongMemoize + + def initialize(members, source:, current_user:) + if !members.is_a?(Array) && !members.is_a?(MembersPresenter) + raise ArgumentError, "members should be an instance of Array or MembersPresenter" + end + + @members = members + @source = source + @current_user = current_user + end + + def execute + # We don't need to calculate the access level of the current user in the invited groups if: + # + # 1. The current user can admin members then the user should be able to see the source of all memberships + # to enable management of group/project memberships. + # 2. There are no members invited from a private group. + return if can_admin_members? || private_invited_group_members.nil? + + private_invited_group_members.each do |member| + member.is_source_accessible_to_current_user = authorized_group_ids.include?(member.source_id) + end + end + + private + + attr_reader :members, :source, :current_user + + def authorized_group_ids + return [] if current_user.nil? + + private_invited_group_ids = private_invited_group_members.map(&:source_id).uniq + current_user.authorized_groups.id_in(private_invited_group_ids).map(&:id) + end + strong_memoize_attr(:authorized_group_ids) + + def private_invited_group_members + members.select do |member| + # The user can see those members where: + # - The source is public. + # - The member is direct or inherited. ProjectMember type is always direct. + member.is_a?(GroupMember) && + member.source.visibility_level != Gitlab::VisibilityLevel::PUBLIC && + member.source_id != source.id && # Exclude direct member + member.source.traversal_ids.exclude?(source.id) # Exclude inherited member + end + end + strong_memoize_attr(:private_invited_group_members) + + def can_admin_members? + return can?(current_user, :admin_project_member, source) if source.is_a?(Project) + + can?(current_user, :admin_group_member, source) + end + end +end diff --git a/app/models/members/members/members_with_parents.rb b/app/models/members/members/members_with_parents.rb index 61ce99e1f3e56aed5ce38f5f92aa8cf6662967b5..3598a146e8df9e3a07f4908fce2a8f223e1712cf 100644 --- a/app/models/members/members/members_with_parents.rb +++ b/app/models/members/members/members_with_parents.rb @@ -75,7 +75,7 @@ def members_from_self_and_ancestor_group_shares # Instead of members.access_level, we need to maximize that access_level at # the respective group_group_links.group_access. - member_columns = GroupMember.attribute_names.map do |column_name| + member_columns = GroupMember.column_names.map do |column_name| if column_name == 'access_level' smallest_value_arel([cte_alias[:group_access], group_member_table[:access_level]], 'access_level') else diff --git a/app/models/preloaders/user_max_access_level_in_groups_preloader.rb b/app/models/preloaders/user_max_access_level_in_groups_preloader.rb index aaa54e0228b7755962117361f892835b6d761883..4615b22cfbab82ad6c1a9b59385ed7dc096769b9 100644 --- a/app/models/preloaders/user_max_access_level_in_groups_preloader.rb +++ b/app/models/preloaders/user_max_access_level_in_groups_preloader.rb @@ -48,7 +48,7 @@ def alter_direct_memberships_to_make_it_act_like_memberships_in_shared_groups group_group_link_table = GroupGroupLink.arel_table group_member_table = GroupMember.arel_table - altered_columns = GroupMember.attribute_names.map do |column_name| + altered_columns = GroupMember.column_names.map do |column_name| case column_name when 'access_level' # Consider the limiting effect of group share's access level diff --git a/app/serializers/member_entity.rb b/app/serializers/member_entity.rb index f259cba65a6ecf6912eb64cb20da13da90a17cfc..450937d9cb6efcf185d2262801b1b01cdd2baf19 100644 --- a/app/serializers/member_entity.rb +++ b/app/serializers/member_entity.rb @@ -11,7 +11,8 @@ class MemberEntity < Grape::Entity end expose :requested_at - expose :created_by, if: -> (member) { member.created_by.present? } do |member| + expose :created_by, + if: -> (member) { member.created_by.present? && member.is_source_accessible_to_current_user } do |member| UserEntity.represent(member.created_by, only: [:name, :web_url]) end @@ -38,10 +39,14 @@ class MemberEntity < Grape::Entity expose :custom_permissions - expose :source do |member| + expose :source, if: -> (member) { member.is_source_accessible_to_current_user } do |member| GroupEntity.represent(member.source, only: [:id, :full_name, :web_url]) end + expose :is_shared_with_group_private do |member| + !member.is_source_accessible_to_current_user + end + expose :type expose :valid_level_roles, as: :valid_roles diff --git a/app/serializers/member_serializer.rb b/app/serializers/member_serializer.rb index ad258b0ef1e9721e635ab85079dd0c01b815f871..aa79a6ebb8018723c16151dc82ddfba3f8caa7d6 100644 --- a/app/serializers/member_serializer.rb +++ b/app/serializers/member_serializer.rb @@ -5,6 +5,8 @@ class MemberSerializer < BaseSerializer def represent(members, opts = {}) LastGroupOwnerAssigner.new(opts[:group], members).execute unless opts[:source].is_a?(Project) + Members::InvitedPrivateGroupAccessibilityAssigner + .new(members, source: opts[:source], current_user: opts[:current_user]).execute super(members, opts) end diff --git a/spec/finders/projects/members/effective_access_level_per_user_finder_spec.rb b/spec/finders/projects/members/effective_access_level_per_user_finder_spec.rb index 3872938d20e8d015a1a51d0c1c786ad568cb9c60..91452fe0d8862905c49651d27830c3d4bd230d26 100644 --- a/spec/finders/projects/members/effective_access_level_per_user_finder_spec.rb +++ b/spec/finders/projects/members/effective_access_level_per_user_finder_spec.rb @@ -26,13 +26,13 @@ end it 'includes the highest access level from all avenues of memberships for the specific user alone' do - expect(subject).to eq( - [{ + expect(subject.first).to match(hash_including( + { 'user_id' => user.id, 'access_level' => Gitlab::Access::MAINTAINER, # From project_group_link 'id' => nil - }] - ) + } + )) end end end diff --git a/spec/helpers/projects/project_members_helper_spec.rb b/spec/helpers/projects/project_members_helper_spec.rb index 2cc87e8aeb99df5e426211f56aa56c119a63307a..0a85b28c91911bf85d6326bcb1f66d4a4d5f2e5a 100644 --- a/spec/helpers/projects/project_members_helper_spec.rb +++ b/spec/helpers/projects/project_members_helper_spec.rb @@ -113,6 +113,9 @@ let_it_be(:top_group) { create(:group) } let_it_be(:sub_group) { create(:group, parent: top_group) } let_it_be(:project) { create(:project, group: sub_group) } + let_it_be(:members) { create_list(:project_member, 2, project: project) } + let_it_be(:invited) { create_list(:project_member, 2, :invited, project: project) } + let_it_be(:access_requests) { create_list(:project_member, 2, :access_request, project: project) } let_it_be(:group_link_1) { create(:group_group_link, shared_group: top_group, shared_with_group: shared_with_group_1, group_access: Gitlab::Access::GUEST) } let_it_be(:group_link_2) { create(:group_group_link, shared_group: top_group, shared_with_group: shared_with_group_4, group_access: Gitlab::Access::GUEST) } let_it_be(:group_link_3) { create(:group_group_link, shared_group: top_group, shared_with_group: shared_with_group_5, group_access: Gitlab::Access::DEVELOPER) } diff --git a/spec/models/members/members/invited_private_group_accessibility_assigner_spec.rb b/spec/models/members/members/invited_private_group_accessibility_assigner_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..1f720b779cf44e6ba3dee6da447b02882eab4663 --- /dev/null +++ b/spec/models/members/members/invited_private_group_accessibility_assigner_spec.rb @@ -0,0 +1,180 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Members::InvitedPrivateGroupAccessibilityAssigner, feature_category: :groups_and_projects do + using RSpec::Parameterized::TableSyntax + + let_it_be(:user) { create(:user) } + + describe '#initialize' do + it 'raises an error when initializing using ActiveRecord::Relation' do + members = Member.all + expect { described_class.new(members, source: nil, current_user: user) }.to raise_error(ArgumentError) + end + + it 'does not raise an error when initializing using Array or MembersPresenter' do + expect { described_class.new([], source: nil, current_user: user) }.not_to raise_error + expect { described_class.new(MembersPresenter.new(nil), source: nil, current_user: user) }.not_to raise_error + end + end + + describe '#execute' do + shared_examples 'assigns is_source_accessible_to_current_user' do + let(:current_user) { user } + let_it_be(:member_user) { create(:user) } + + subject(:assigner) { described_class.new(members, source: source, current_user: current_user) } + + context 'for direct members' do + let_it_be(:source) { create(source_type) } # rubocop:disable Rails/SaveBang -- Using factory, not ActiveRecord + let_it_be(:direct_member) { source.add_developer(member_user) } + let(:members) { [direct_member] } + + it 'sets is_source_accessible_to_current_user to true for all members' do + assigner.execute + + expect(members.map(&:is_source_accessible_to_current_user)).to all(be_truthy) + end + end + + context 'for inherited members' do + let_it_be(:parent) { create(:group) } + let_it_be(:source) { create(source_type, parent_key => parent) } + let_it_be(:inherited_member) { parent.add_developer(member_user) } + let(:members) { [inherited_member] } + + it 'sets is_source_accessible_to_current_user to true for all members' do + assigner.execute + + expect(members.first.is_source_accessible_to_current_user).to eq(true) + end + end + + context 'for shared source members' do + let(:shared_source) { create(source_type, shared_source_visibility) } + let(:invited_group) { create(:group, invited_group_visibility) } + let(:source) { shared_source } + let(:invited_member) { invited_group.add_developer(member_user) } + let(:members) { [invited_member] } + let!(:link) { create_link(source, invited_group) } + + shared_examples 'sets correct is_source_accessible_to_current_user for invited members' do + with_them do + specify do + assigner.execute + + expect(members.first.is_source_accessible_to_current_user).to eq(can_see_invited_members_source?) + end + + context 'with multiple members belonging to different sources' do + it 'avoid N+1 queries' do + assigner # Initialize objects in let blocks + recorder = ActiveRecord::QueryRecorder.new { assigner.execute } + + members = create_list(:group, 3, invited_group_visibility).map do |invited_group| + create_link(shared_source, invited_group) + create(:group_member, group: invited_group) + end + + assigner = described_class.new(members, source: shared_source, current_user: current_user) + expect { assigner.execute }.not_to exceed_query_limit(recorder) + end + end + end + end + + context 'when current user is unauthenticated' do + let(:current_user) { nil } + + where(:shared_source_visibility, :invited_group_visibility, :can_see_invited_members_source?) do + :public | :public | true + :public | :private | false + end + + include_examples 'sets correct is_source_accessible_to_current_user for invited members' + end + + context 'when current user non-member of shared source' do + where(:shared_source_visibility, :invited_group_visibility, :can_see_invited_members_source?) do + :public | :public | true + :public | :private | false + end + + include_examples 'sets correct is_source_accessible_to_current_user for invited members' + end + + context 'when current user a member of shared source but not of invited group' do + before do + shared_source.add_developer(current_user) + end + + where(:shared_source_visibility, :invited_group_visibility, :can_see_invited_members_source?) do + :public | :public | true + :public | :private | false + :private | :public | true + :private | :private | false + end + + include_examples 'sets correct is_source_accessible_to_current_user for invited members' + end + + context 'when current user a direct member of shared group and of invited group through sharing' do + before do + group = create(:group, :private) + group.add_developer(current_user) + create(:group_group_link, shared_group: invited_group, shared_with_group: group) + end + + where(:shared_source_visibility, :invited_group_visibility, :can_see_invited_members_source?) do + :public | :public | true + :public | :private | true + :private | :public | true + :private | :private | true + end + + include_examples 'sets correct is_source_accessible_to_current_user for invited members' + end + + context 'when current user can manage member of shared group not invited group members' do + before do + shared_source.add_member(current_user, admin_member_access) + end + + where(:shared_source_visibility, :invited_group_visibility, :can_see_invited_members_source?) do + :public | :public | true + :public | :private | true + :private | :public | true + :private | :private | true + end + + include_examples 'sets correct is_source_accessible_to_current_user for invited members' + end + end + end + + context 'for project members' do + let_it_be(:source_type) { 'project' } + let_it_be(:admin_member_access) { Gitlab::Access::MAINTAINER } + let_it_be(:parent_key) { :group } + + it_behaves_like 'assigns is_source_accessible_to_current_user' + + def create_link(shared, invited) + create(:project_group_link, project: shared, group: invited) + end + end + + context 'for group members' do + let_it_be(:source_type) { 'group' } + let_it_be(:admin_member_access) { Gitlab::Access::OWNER } + let_it_be(:parent_key) { :parent } + + it_behaves_like 'assigns is_source_accessible_to_current_user' + + def create_link(shared, invited) + create(:group_group_link, shared_group: shared, shared_with_group: invited) + end + end + end +end diff --git a/spec/serializers/member_entity_spec.rb b/spec/serializers/member_entity_spec.rb index 350355eb72d06606ae9e86440e6709efb77c0ce6..2f4a803d61bd79a007180a964ebca655476668e3 100644 --- a/spec/serializers/member_entity_spec.rb +++ b/spec/serializers/member_entity_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe MemberEntity do +RSpec.describe MemberEntity, feature_category: :groups_and_projects do let_it_be(:current_user) { create(:user) } let(:entity) { described_class.new(member, { current_user: current_user, group: group, source: source }) } @@ -24,6 +24,28 @@ expect(entity_hash[:can_remove]).to be(true) end + + context 'when is_source_accessible_to_current_user is true' do + before do + allow(member).to receive(:is_source_accessible_to_current_user).and_return(true) + end + + it 'exposes source and created_by' do + expect(entity_hash[:source]).to be_present + expect(entity_hash[:created_by]).to be_present + end + end + + context 'when is_source_accessible_to_current_user is false' do + before do + allow(member).to receive(:is_source_accessible_to_current_user).and_return(false) + end + + it 'does not exposes source and created_by' do + expect(entity_hash[:source]).to be_nil + expect(entity_hash[:created_by]).to be_nil + end + end end shared_examples 'invite' do @@ -72,12 +94,20 @@ context 'group member' do let(:group) { create(:group) } let(:source) { group } - let(:member) { GroupMemberPresenter.new(create(:group_member, group: group), current_user: current_user) } + let(:member) do + GroupMemberPresenter.new( + create(:group_member, group: group, created_by: current_user), current_user: current_user + ) + end it_behaves_like 'member.json' context 'invite' do - let(:member) { GroupMemberPresenter.new(create(:group_member, :invited, group: group), current_user: current_user) } + let(:member) do + GroupMemberPresenter.new( + create(:group_member, :invited, group: group, created_by: current_user), current_user: current_user + ) + end it_behaves_like 'member.json' it_behaves_like 'invite' @@ -125,12 +155,20 @@ let(:project) { create(:project) } let(:group) { project.group } let(:source) { project } - let(:member) { ProjectMemberPresenter.new(create(:project_member, project: project), current_user: current_user) } + let(:member) do + ProjectMemberPresenter.new( + create(:project_member, project: project, created_by: current_user), current_user: current_user + ) + end it_behaves_like 'member.json' context 'invite' do - let(:member) { ProjectMemberPresenter.new(create(:project_member, :invited, project: project), current_user: current_user) } + let(:member) do + ProjectMemberPresenter.new( + create(:project_member, :invited, project: project, created_by: current_user), current_user: current_user + ) + end it_behaves_like 'member.json' it_behaves_like 'invite' diff --git a/spec/serializers/member_serializer_spec.rb b/spec/serializers/member_serializer_spec.rb index c35ecf5f63606ddc9af5ae9e6495d929c49db30e..ee198733ef259375f3ad4f157858caf1c85d087d 100644 --- a/spec/serializers/member_serializer_spec.rb +++ b/spec/serializers/member_serializer_spec.rb @@ -2,8 +2,9 @@ require 'spec_helper' -RSpec.describe MemberSerializer do +RSpec.describe MemberSerializer, feature_category: :groups_and_projects do include MembersPresentation + using RSpec::Parameterized::TableSyntax let_it_be(:current_user) { create(:user) } @@ -15,6 +16,80 @@ it { is_expected.to match_schema('members') } end + shared_examples 'shared source members' do + let_it_be(:member_user) { create(:user) } + let(:shared_source) { create(source_type, shared_source_visibility) } + let(:invited_group) { create(:group, invited_group_visibility) } + let(:source) { shared_source } + let(:group) { source.is_a?(Project) ? source.group : source } + let(:invited_member) { invited_group.add_developer(member_user) } + let(:members) { present_members([invited_member]) } + + shared_examples 'exposes source correctly' do + with_them do + before do + create_link(source, invited_group) + end + + specify do + representation + + expect(invited_member.is_source_accessible_to_current_user).to eq(can_see_invited_members_source?) + end + end + end + + context 'when current user is unauthenticated' do + let(:current_user) { nil } + + where(:shared_source_visibility, :invited_group_visibility, :can_see_invited_members_source?) do + :public | :public | true + :public | :private | false + end + + include_examples 'exposes source correctly' + end + + context 'when current user non-member of shared source' do + where(:shared_source_visibility, :invited_group_visibility, :can_see_invited_members_source?) do + :public | :public | true + :public | :private | false + end + + include_examples 'exposes source correctly' + end + + context 'when current user a member of shared source but not of invited group' do + before do + shared_source.add_developer(current_user) + end + + where(:shared_source_visibility, :invited_group_visibility, :can_see_invited_members_source?) do + :public | :public | true + :public | :private | false + :private | :public | true + :private | :private | false + end + + include_examples 'exposes source correctly' + end + + context 'when current user can manage member of shared group but not invited group members' do + before do + shared_source.add_member(current_user, admin_member_access) + end + + where(:shared_source_visibility, :invited_group_visibility, :can_see_invited_members_source?) do + :public | :public | true + :public | :private | true + :private | :public | true + :private | :private | true + end + + include_examples 'exposes source correctly' + end + end + context 'group member' do let_it_be(:group) { create(:group) } let_it_be(:members) { present_members(create_list(:group_member, 1, group: group)) } @@ -29,6 +104,15 @@ expect { representation }.to change(group_member, :last_owner) .from(nil).to(true) end + + it_behaves_like 'shared source members' do + let_it_be(:source_type) { :group } + let_it_be(:admin_member_access) { Gitlab::Access::OWNER } + + def create_link(shared, invited) + create(:group_group_link, shared_group: shared, shared_with_group: invited) + end + end end context 'project member' do @@ -45,5 +129,14 @@ representation end + + it_behaves_like 'shared source members' do + let_it_be(:source_type) { :project } + let_it_be(:admin_member_access) { Gitlab::Access::MAINTAINER } + + def create_link(shared, invited) + create(:project_group_link, project: shared, group: invited) + end + end end end