diff options
author | Eugenia Grieff <egrieff@gitlab.com> | 2019-09-23 14:29:52 +0100 |
---|---|---|
committer | Eugenia Grieff <egrieff@gitlab.com> | 2019-10-22 17:23:13 +0100 |
commit | 457b355ad1f0584fc3a6106984650bb37a0ce16b (patch) | |
tree | b746c80f40ed5d29d4c208e998880bb470a2c16c | |
parent | 635e1578219d95ee683cd2901fa5d0f6965e7033 (diff) | |
download | gitlab-ce-457b355ad1f0584fc3a6106984650bb37a0ce16b.tar.gz |
Backport for CE MR
https://dev.gitlab.org/gitlab/gitlabhq/merge_requests/3409
-rw-r--r-- | app/finders/labels_finder.rb | 8 | ||||
-rw-r--r-- | app/models/project.rb | 8 | ||||
-rw-r--r-- | changelogs/unreleased/security-2914-labels-visible-despite-no-access-to-issues-repositories.yml | 5 | ||||
-rw-r--r-- | lib/gitlab/search_results.rb | 2 | ||||
-rw-r--r-- | spec/finders/labels_finder_spec.rb | 82 | ||||
-rw-r--r-- | spec/models/project_spec.rb | 4 |
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 a1bd5edaba9..645c439fb20 100644 --- a/app/models/project.rb +++ b/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) 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 ce4c1611687..cf4e85363f6 100644 --- a/lib/gitlab/search_results.rb +++ b/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 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 de5fe9ee8a8..face16ebf41 100644 --- a/spec/models/project_spec.rb +++ b/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) |