Merge branch 'security-2914-labels-visible-despite-no-access-to-issues-repositories-12-3' into '12-3-stable'

Labels visible despite no access to issues & repositories

See merge request gitlab/gitlabhq!3430

6 files changed, 101 insertions, 8 deletions

diff --git a/app/finders/labels_finder.rb b/app/finders/labels_finder.rb
index e523942ea4c..027cdc4fc78 100644
--- a/app/finders/labels_finder.rb
+++ b/app/finders/labels_finder.rb
@@ -51,7 +51,7 @@ class LabelsFinder < UnionFinder
       end
 
       label_ids << Label.where(group_id: projects.group_ids)
-      label_ids << Label.where(project_id: projects.select(:id)) unless only_group_labels?
+      label_ids << Label.where(project_id: ids_user_can_read_labels(projects)) unless only_group_labels?
     end
 
     label_ids
@@ -188,4 +188,10 @@ class LabelsFinder < UnionFinder
       groups.select { |group| authorized_to_read_labels?(group) }
     end
   end
+
+  # rubocop: disable CodeReuse/ActiveRecord
+  def ids_user_can_read_labels(projects)
+    Project.where(id: projects.select(:id)).ids_with_issuables_available_for(current_user)
+  end
+  # rubocop: enable CodeReuse/ActiveRecord
 end
diff --git a/app/models/project.rb b/app/models/project.rb
index 817b7a05d65..e4341bcee6c 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -591,11 +591,11 @@ class Project < ApplicationRecord
       joins(:namespace).where(namespaces: { type: 'Group' }).select(:namespace_id)
     end
 
-    # Returns ids of projects with milestones available for given user
+    # Returns ids of projects with issuables available for given user
     #
-    # Used on queries to find milestones which user can see
-    # For example: Milestone.where(project_id: ids_with_milestone_available_for(user))
-    def ids_with_milestone_available_for(user)
+    # Used on queries to find milestones or labels which user can see
+    # For example: Milestone.where(project_id: ids_with_issuables_available_for(user))
+    def ids_with_issuables_available_for(user)
       with_issues_enabled = with_issues_available_for_user(user).select(:id)
       with_merge_requests_enabled = with_merge_requests_available_for_user(user).select(:id)
 
diff --git a/changelogs/unreleased/security-2914-labels-visible-despite-no-access-to-issues-repositories.yml b/changelogs/unreleased/security-2914-labels-visible-despite-no-access-to-issues-repositories.yml
new file mode 100644
index 00000000000..59af202a3bd
--- /dev/null
+++ b/changelogs/unreleased/security-2914-labels-visible-despite-no-access-to-issues-repositories.yml
@@ -0,0 +1,5 @@
+---
+title: Do not display project labels that are not visible for user accessing group labels
+merge_request:
+author:
+type: security
diff --git a/lib/gitlab/search_results.rb b/lib/gitlab/search_results.rb
index 93e172299b9..bca9b385211 100644
--- a/lib/gitlab/search_results.rb
+++ b/lib/gitlab/search_results.rb
@@ -163,7 +163,7 @@ module Gitlab
       return Milestone.none if project_ids.nil?
 
       authorized_project_ids_relation =
-        Project.where(id: project_ids).ids_with_milestone_available_for(current_user)
+        Project.where(id: project_ids).ids_with_issuables_available_for(current_user)
 
       milestones.where(project_id: authorized_project_ids_relation)
     end
diff --git a/spec/finders/labels_finder_spec.rb b/spec/finders/labels_finder_spec.rb
index ba41ded112a..024bfe4d97b 100644
--- a/spec/finders/labels_finder_spec.rb
+++ b/spec/finders/labels_finder_spec.rb
@@ -127,6 +127,88 @@ describe LabelsFinder do
         end
       end
     end
+    context 'when including labels from group projects with limited visibility' do
+      let(:finder)                     { described_class.new(user, group_id: group_4.id) }
+      let(:group_4)                    { create(:group) }
+      let(:limited_visibility_project) { create(:project, :public, group: group_4) }
+      let(:visible_project)            { create(:project, :public, group: group_4) }
+      let!(:group_label_1)             { create(:group_label, group: group_4) }
+      let!(:limited_visibility_label)  { create(:label, project: limited_visibility_project) }
+      let!(:visible_label)             { create(:label, project: visible_project) }
+
+      shared_examples 'with full visibility' do
+        it 'returns all projects labels' do
+          expect(finder.execute).to eq [group_label_1, limited_visibility_label, visible_label]
+        end
+      end
+
+      shared_examples 'with limited visibility' do
+        it 'returns only authorized projects labels' do
+          expect(finder.execute).to eq [group_label_1, visible_label]
+        end
+      end
+
+      context 'when merge requests and issues are not visible for non members' do
+        before do
+          limited_visibility_project.project_feature.update!(
+            merge_requests_access_level: ProjectFeature::PRIVATE,
+            issues_access_level: ProjectFeature::PRIVATE
+          )
+        end
+
+        context 'when user is not a group member' do
+          it_behaves_like 'with limited visibility'
+        end
+
+        context 'when user is a group member' do
+          before do
+            group_4.add_developer(user)
+          end
+
+          it_behaves_like 'with full visibility'
+        end
+      end
+
+      context 'when merge requests are not visible for non members' do
+        before do
+          limited_visibility_project.project_feature.update!(
+            merge_requests_access_level: ProjectFeature::PRIVATE
+          )
+        end
+
+        context 'when user is not a group member' do
+          it_behaves_like 'with full visibility'
+        end
+
+        context 'when user is a group member' do
+          before do
+            group_4.add_developer(user)
+          end
+
+          it_behaves_like 'with full visibility'
+        end
+      end
+
+      context 'when issues are not visible for non members' do
+        before do
+          limited_visibility_project.project_feature.update!(
+            issues_access_level: ProjectFeature::PRIVATE
+          )
+        end
+
+        context 'when user is not a group member' do
+          it_behaves_like 'with full visibility'
+        end
+
+        context 'when user is a group member' do
+          before do
+            group_4.add_developer(user)
+          end
+
+          it_behaves_like 'with full visibility'
+        end
+      end
+    end
 
     context 'filtering by project_id' do
       context 'when include_ancestor_groups is true' do
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index caad288dfa7..92648cddb24 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -3341,7 +3341,7 @@ describe Project do
     end
   end
 
-  describe '.ids_with_milestone_available_for' do
+  describe '.ids_with_issuables_available_for' do
     let!(:user) { create(:user) }
 
     it 'returns project ids with milestones available for user' do
@@ -3351,7 +3351,7 @@ describe Project do
       project_4 = create(:project, :public)
       project_4.project_feature.update(issues_access_level: ProjectFeature::PRIVATE, merge_requests_access_level: ProjectFeature::PRIVATE )
 
-      project_ids = described_class.ids_with_milestone_available_for(user).pluck(:id)
+      project_ids = described_class.ids_with_issuables_available_for(user).pluck(:id)
 
       expect(project_ids).to include(project_2.id, project_3.id)
       expect(project_ids).not_to include(project_1.id, project_4.id)