summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorEugenia Grieff <egrieff@gitlab.com>2019-10-22 16:57:55 +0100
committerEugenia Grieff <egrieff@gitlab.com>2019-10-22 16:57:55 +0100
commitb58dd075d32e852e6c7ab306c84945cb5d73c06a (patch)
treed4b494bc2bda9edf65ffcbde56ed3f949d33e0b0
parent1425a56c75beecaa289ad59587d636f8f469509e (diff)
downloadgitlab-ce-b58dd075d32e852e6c7ab306c84945cb5d73c06a.tar.gz
Fix labels finder to filter issuables
Use project scopes to filter project labels that are visible for user
-rw-r--r--app/finders/labels_finder.rb8
-rw-r--r--app/models/project.rb8
-rw-r--r--changelogs/unreleased/security-2914-labels-visible-despite-no-access-to-issues-repositories-12-.yml5
-rw-r--r--lib/gitlab/search_results.rb2
-rw-r--r--spec/finders/labels_finder_spec.rb83
-rw-r--r--spec/models/project_spec.rb4
6 files changed, 102 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 3525f37f8d5..1aecf3ef6ab 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -609,11 +609,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-12-.yml b/changelogs/unreleased/security-2914-labels-visible-despite-no-access-to-issues-repositories-12-.yml
new file mode 100644
index 00000000000..59af202a3bd
--- /dev/null
+++ b/changelogs/unreleased/security-2914-labels-visible-despite-no-access-to-issues-repositories-12-.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 782ac534a7b..d74e64116ca 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 2681f098fec..7590c399cf9 100644
--- a/spec/finders/labels_finder_spec.rb
+++ b/spec/finders/labels_finder_spec.rb
@@ -128,6 +128,89 @@ describe LabelsFinder do
expect(finder.execute).to eq [private_subgroup_label_1]
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
end
context 'filtering by project_id' do
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index 9f3313e67b5..1467129d8e6 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -3416,7 +3416,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
@@ -3426,7 +3426,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)