diff --git a/app/models/member.rb b/app/models/member.rb index e2d26773d450e8d6757255ab6877675b6c20cccf..2654453cf3fdb240234c6c2f00265bd823747e45 100644 --- a/app/models/member.rb +++ b/app/models/member.rb @@ -8,6 +8,7 @@ class Member < ApplicationRecord include Gitlab::Access include Presentable include Gitlab::Utils::StrongMemoize + include FromUnion attr_accessor :raw_invite_token diff --git a/app/services/projects/participants_service.rb b/app/services/projects/participants_service.rb index 7080f388e53e9352d018698066969bf18ad39f04..1cd81fe37c7b6859596d3efd2e49141f034fa564 100644 --- a/app/services/projects/participants_service.rb +++ b/app/services/projects/participants_service.rb @@ -7,16 +7,69 @@ module Projects def execute(noteable) @noteable = noteable - participants = noteable_owner + participants_in_noteable + all_members + groups + project_members + participants = + noteable_owner + + participants_in_noteable + + all_members + + groups + + project_members + participants.uniq end def project_members - @project_members ||= sorted(project.team.members) + @project_members ||= sorted(get_project_members) + end + + def get_project_members + members = Member.from_union([project_members_through_ancestral_groups, + project_members_through_invited_groups, + individual_project_members]) + + User.id_in(members.select(:user_id)) end def all_members [{ username: "all", name: "All Project and Group Members", count: project_members.count }] end + + private + + def project_members_through_invited_groups + groups_with_ancestors_ids = Gitlab::ObjectHierarchy + .new(visible_groups) + .base_and_ancestors + .pluck_primary_key + + GroupMember + .active_without_invites_and_requests + .with_source_id(groups_with_ancestors_ids) + end + + def visible_groups + visible_groups = project.invited_groups + + unless project_owner? + visible_groups = visible_groups.public_or_visible_to_user(current_user) + end + + visible_groups + end + + def project_members_through_ancestral_groups + project.group.present? ? project.group.members_with_parents : Member.none + end + + def individual_project_members + project.project_members + end + + def project_owner? + if project.group.present? + project.group.owners.include?(current_user) + else + project.namespace.owner == current_user + end + end end end diff --git a/changelogs/unreleased/security-hide-private-members-in-project-member-autocomplete.yml b/changelogs/unreleased/security-hide-private-members-in-project-member-autocomplete.yml new file mode 100644 index 0000000000000000000000000000000000000000..5992e93bda2343cdef46238246f0cb2da39c4e79 --- /dev/null +++ b/changelogs/unreleased/security-hide-private-members-in-project-member-autocomplete.yml @@ -0,0 +1,3 @@ +--- +title: "Don't leak private members in project member autocomplete suggestions" +type: security diff --git a/doc/user/project/members/share_project_with_groups.md b/doc/user/project/members/share_project_with_groups.md index 9340d23967794d9caaf1d6c128ba799788cea184..79fb2ea50a043bce803d52c911b6f73be2824e35 100644 --- a/doc/user/project/members/share_project_with_groups.md +++ b/doc/user/project/members/share_project_with_groups.md @@ -44,6 +44,10 @@ Admins are able to share projects with any group in the system. In the example above, the maximum access level of 'Developer' for members from 'Engineering' means that users with higher access levels in 'Engineering' ('Maintainer' or 'Owner') will only have 'Developer' access to 'Project Acme'. +## Sharing public project with private group + +When sharing a public project with a private group, owners and maintainers of the project will see the name of the group in the `members` page. Owners will also have the possibility to see members of the private group they don't have access to when mentioning them in the issue or merge request. + ## Share project with group lock It is possible to prevent projects in a group from [sharing diff --git a/spec/controllers/projects/autocomplete_sources_controller_spec.rb b/spec/controllers/projects/autocomplete_sources_controller_spec.rb index a9a058e7e17310bcd2552275561e1e5c30f9aa3e..f9c8e91d81696e98b1228a0fa4f8fb33a8a3e399 100644 --- a/spec/controllers/projects/autocomplete_sources_controller_spec.rb +++ b/spec/controllers/projects/autocomplete_sources_controller_spec.rb @@ -8,6 +8,10 @@ describe Projects::AutocompleteSourcesController do set(:issue) { create(:issue, project: project) } set(:user) { create(:user) } + def members_by_username(username) + json_response.find { |member| member['username'] == username } + end + describe 'GET members' do before do group.add_owner(user) @@ -17,22 +21,21 @@ describe Projects::AutocompleteSourcesController do it 'returns an array of member object' do get :members, format: :json, params: { namespace_id: group.path, project_id: project.path, type: issue.class.name, type_id: issue.id } - all = json_response.find {|member| member["username"] == 'all'} - the_group = json_response.find {|member| member["username"] == group.full_path} - the_user = json_response.find {|member| member["username"] == user.username} - - expect(all.symbolize_keys).to include(username: 'all', - name: 'All Project and Group Members', - count: 1) - - expect(the_group.symbolize_keys).to include(type: group.class.name, - name: group.full_name, - avatar_url: group.avatar_url, - count: 1) - - expect(the_user.symbolize_keys).to include(type: user.class.name, - name: user.name, - avatar_url: user.avatar_url) + expect(members_by_username('all').symbolize_keys).to include( + username: 'all', + name: 'All Project and Group Members', + count: 1) + + expect(members_by_username(group.full_path).symbolize_keys).to include( + type: group.class.name, + name: group.full_name, + avatar_url: group.avatar_url, + count: 1) + + expect(members_by_username(user.username).symbolize_keys).to include( + type: user.class.name, + name: user.name, + avatar_url: user.avatar_url) end end diff --git a/spec/services/projects/participants_service_spec.rb b/spec/services/projects/participants_service_spec.rb index 4def83513a478ae7bce78107c0ff60f9da5ed0e0..239d28557eeff33577a6de2b0ca7fed584afbd99 100644 --- a/spec/services/projects/participants_service_spec.rb +++ b/spec/services/projects/participants_service_spec.rb @@ -57,4 +57,108 @@ describe Projects::ParticipantsService do end end end + + describe '#project_members' do + subject(:usernames) { service.project_members.map { |member| member[:username] } } + + context 'when there is a project in group namespace' do + set(:public_group) { create(:group, :public) } + set(:public_project) { create(:project, :public, namespace: public_group)} + + set(:public_group_owner) { create(:user) } + + let(:service) { described_class.new(public_project, create(:user)) } + + before do + public_group.add_owner(public_group_owner) + end + + it 'returns members of a group' do + expect(usernames).to include(public_group_owner.username) + end + end + + context 'when there is a private group and a public project' do + set(:public_group) { create(:group, :public) } + set(:private_group) { create(:group, :private, :nested) } + set(:public_project) { create(:project, :public, namespace: public_group)} + + set(:project_issue) { create(:issue, project: public_project)} + + set(:public_group_owner) { create(:user) } + set(:private_group_member) { create(:user) } + set(:public_project_maintainer) { create(:user) } + set(:private_group_owner) { create(:user) } + + set(:group_ancestor_owner) { create(:user) } + + before(:context) do + public_group.add_owner public_group_owner + private_group.add_developer private_group_member + public_project.add_maintainer public_project_maintainer + + private_group.add_owner private_group_owner + private_group.parent.add_owner group_ancestor_owner + end + + context 'when the private group is invited to the public project' do + before(:context) do + create(:project_group_link, group: private_group, project: public_project) + end + + context 'when a user who is outside the public project and the private group is signed in' do + let(:service) { described_class.new(public_project, create(:user)) } + + it 'does not return the private group' do + expect(usernames).not_to include(private_group.name) + end + + it 'does not return private group members' do + expect(usernames).not_to include(private_group_member.username) + end + + it 'returns the project maintainer' do + expect(usernames).to include(public_project_maintainer.username) + end + + it 'returns project members from an invited public group' do + invited_public_group = create(:group, :public) + invited_public_group.add_owner create(:user) + + create(:project_group_link, group: invited_public_group, project: public_project) + + expect(usernames).to include(invited_public_group.users.first.username) + end + + it 'does not return ancestors of the private group' do + expect(usernames).not_to include(group_ancestor_owner.username) + end + end + + context 'when private group owner is signed in' do + let(:service) { described_class.new(public_project, private_group_owner) } + + it 'returns private group members' do + expect(usernames).to include(private_group_member.username) + end + + it 'returns ancestors of the the private group' do + expect(usernames).to include(group_ancestor_owner.username) + end + end + + context 'when the namespace owner of the public project is signed in' do + let(:service) { described_class.new(public_project, public_group_owner) } + + it 'returns private group members' do + expect(usernames).to include(private_group_member.username) + end + + it 'does not return members of the ancestral groups of the private group' do + expect(usernames).to include(group_ancestor_owner.username) + end + end + end + end + end end