Merge branch 'security-ag-hide-private-members-in-project-member-autocomplete-12-4' into '12-4-stable'

Hide private members in project member autocomplete

See merge request gitlab/gitlabhq!3503

6 files changed, 186 insertions, 18 deletions

diff --git a/app/models/member.rb b/app/models/member.rb
index e2d26773d45..2654453cf3f 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 7080f388e53..1cd81fe37c7 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 00000000000..5992e93bda2
--- /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 9340d239677..79fb2ea50a0 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 34765ae3951..fc8fe1ac4f6 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
   let_it_be(:issue) { create(:issue, project: project) }
   let_it_be(: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 4def83513a4..239d28557ee 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