Commit c4447415 authored by Robert Speicher's avatar Robert Speicher

Limit labels returned for a specific project as an administrator

Prior, an administrator viewing a project's Labels page would see _all_
labels from every project they had access to, rather than only the
labels of that specific project (if any).

This was not an information disclosure, as admins have access to
everything, but it was a performance issue.
parent f27f9803
......@@ -6,7 +6,7 @@ class LabelsFinder < UnionFinder
def execute(skip_authorization: false)
@skip_authorization = skip_authorization
items = find_union(label_ids, Label)
items = find_union(label_ids, Label) || Label.none
items = with_title(items)
sort(items)
end
......@@ -18,9 +18,11 @@ class LabelsFinder < UnionFinder
def label_ids
label_ids = []
if project?
if project
label_ids << project.group.labels if project.group.present?
label_ids << project.labels
end
else
label_ids << Label.where(group_id: projects.group_ids)
label_ids << Label.where(project_id: projects.select(:id))
......@@ -40,16 +42,16 @@ class LabelsFinder < UnionFinder
items.where(title: title)
end
def group_id
params[:group_id].presence
def group?
params[:group_id].present?
end
def project_id
params[:project_id].presence
def project?
params[:project_id].present?
end
def projects_ids
params[:project_ids]
def projects?
params[:project_ids].present?
end
def title
......@@ -59,8 +61,9 @@ class LabelsFinder < UnionFinder
def project
return @project if defined?(@project)
if project_id
@project = find_project
if project?
@project = Project.find(params[:project_id])
@project = nil unless authorized_to_read_labels?(@project)
else
@project = nil
end
......@@ -68,26 +71,20 @@ class LabelsFinder < UnionFinder
@project
end
def find_project
if skip_authorization
Project.find_by(id: project_id)
else
available_projects.find_by(id: project_id)
end
end
def projects
return @projects if defined?(@projects)
@projects = skip_authorization ? Project.all : available_projects
@projects = @projects.in_namespace(group_id) if group_id
@projects = @projects.where(id: projects_ids) if projects_ids
@projects = skip_authorization ? Project.all : ProjectsFinder.new.execute(current_user)
@projects = @projects.in_namespace(params[:group_id]) if group?
@projects = @projects.where(id: params[:project_ids]) if projects?
@projects = @projects.reorder(nil)
@projects
end
def available_projects
@available_projects ||= ProjectsFinder.new.execute(current_user)
def authorized_to_read_labels?(project)
return true if skip_authorization
Ability.allowed?(current_user, :read_label, project)
end
end
---
title: Limit labels returned for a specific project as an administrator
merge_request: 7496
author:
......@@ -64,6 +64,21 @@ describe LabelsFinder do
expect(finder.execute).to eq [group_label_2, project_label_1, group_label_1]
end
context 'as an administrator' do
it 'does not return labels from another project' do
# Purposefully creating a project with _nothing_ associated to it
isolated_project = create(:empty_project)
admin = create(:admin)
# project_3 has a label associated to it, which we don't want coming
# back when we ask for the isolated project's labels
project_3.team << [admin, :reporter]
finder = described_class.new(admin, project_id: isolated_project.id)
expect(finder.execute).to be_empty
end
end
end
context 'filtering by title' do
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment