Merge branch...

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

Labels visible despite no access to issues & repositories

See merge request gitlab/gitlabhq!3431

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

app/models/project.rb

@@ -581,11 +581,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)

changelogs/unreleased/security-2914-labels-visible-despite-no-access-to-issues-repositories.yml

+---
+title: Do not display project labels that are not visible for user accessing group labels
+merge_request:
+author:
+type: security

lib/gitlab/search_results.rb

@@ -162,7 +162,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

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

spec/models/project_spec.rb

@@ -3303,7 +3303,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
@@ -3313,7 +3313,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)