From c44474150c8a82e62ed1e0ed5758b1f38bbf7c41 Mon Sep 17 00:00:00 2001
From: Robert Speicher <rspeicher@gmail.com>
Date: Wed, 16 Nov 2016 11:51:47 +0200
Subject: 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.
---
 app/finders/labels_finder.rb             | 47 +++++++++++++++-----------------
 changelogs/unreleased/rs-issue-24527.yml |  4 +++
 spec/finders/labels_finder_spec.rb       | 15 ++++++++++
 3 files changed, 41 insertions(+), 25 deletions(-)
 create mode 100644 changelogs/unreleased/rs-issue-24527.yml

diff --git a/app/finders/labels_finder.rb b/app/finders/labels_finder.rb
index 865f093f04a..fa0e2a5e3d8 100644
--- a/app/finders/labels_finder.rb
+++ b/app/finders/labels_finder.rb
@@ -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
-      label_ids << project.group.labels if project.group.present?
-      label_ids << project.labels
+    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
diff --git a/changelogs/unreleased/rs-issue-24527.yml b/changelogs/unreleased/rs-issue-24527.yml
new file mode 100644
index 00000000000..a7b6358e60e
--- /dev/null
+++ b/changelogs/unreleased/rs-issue-24527.yml
@@ -0,0 +1,4 @@
+---
+title: Limit labels returned for a specific project as an administrator
+merge_request: 7496
+author: 
diff --git a/spec/finders/labels_finder_spec.rb b/spec/finders/labels_finder_spec.rb
index 10cfb66ec1c..9085cc8debf 100644
--- a/spec/finders/labels_finder_spec.rb
+++ b/spec/finders/labels_finder_spec.rb
@@ -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
-- 
cgit v1.2.1