Merge branch 'yorick/environment-names-finder-security' into 'master'

Fix EnvironmentNamesFinder for private projects

See merge request gitlab-org/gitlab!45779

app/finders/environment_names_finder.rb

@@ -31,8 +31,18 @@ class EnvironmentNamesFinder
   end
 
   def namespace_environments
-    projects =
-      project_or_group.all_projects.public_or_visible_to_user(current_user)
+    # We assume reporter access is needed for the :read_environment permission
+    # here. This expection is also present in
+    # IssuableFinder::Params#min_access_level, which is used for filtering out
+    # merge requests that don't have the right permissions.
+    #
+    # We use this approach so we don't need to load every project into memory
+    # just to verify if we can see their environments. Doing so would not be
+    # efficient, and possibly mess up pagination if certain projects are not
+    # meant to be visible.
+    projects = project_or_group
+      .all_projects
+      .public_or_visible_to_user(current_user, Gitlab::Access::REPORTER)
 
     Environment.for_project(projects)
   end

spec/finders/environment_names_finder_spec.rb

@@ -5,74 +5,178 @@ require 'spec_helper'
 RSpec.describe EnvironmentNamesFinder do
   describe '#execute' do
     let!(:group) { create(:group) }
-    let!(:project1) { create(:project, :public, namespace: group) }
-    let!(:project2) { create(:project, :private, namespace: group) }
+    let!(:public_project) { create(:project, :public, namespace: group) }
+    let!(:private_project) { create(:project, :private, namespace: group) }
     let!(:user) { create(:user) }
 
     before do
-      create(:environment, name: 'gstg', project: project1)
-      create(:environment, name: 'gprd', project: project1)
-      create(:environment, name: 'gprd', project: project2)
-      create(:environment, name: 'gcny', project: project2)
+      create(:environment, name: 'gstg', project: public_project)
+      create(:environment, name: 'gprd', project: public_project)
+      create(:environment, name: 'gprd', project: private_project)
+      create(:environment, name: 'gcny', project: private_project)
     end
 
-    context 'using a group and a group member' do
-      it 'returns environment names for all projects' do
-        group.add_developer(user)
+    context 'using a group' do
+      context 'with a group developer' do
+        it 'returns environment names for all projects' do
+          group.add_developer(user)
 
-        names = described_class.new(group, user).execute
+          names = described_class.new(group, user).execute
 
-        expect(names).to eq(%w[gcny gprd gstg])
+          expect(names).to eq(%w[gcny gprd gstg])
+        end
       end
-    end
 
-    context 'using a group and a guest' do
-      it 'returns environment names for all public projects' do
-        names = described_class.new(group, user).execute
+      context 'with a group reporter' do
+        it 'returns environment names for all projects' do
+          group.add_reporter(user)
+
+          names = described_class.new(group, user).execute
 
-        expect(names).to eq(%w[gprd gstg])
+          expect(names).to eq(%w[gcny gprd gstg])
+        end
       end
-    end
 
-    context 'using a public project and a project member' do
-      it 'returns all the unique environment names' do
-        project1.team.add_developer(user)
+      context 'with a public project reporter' do
+        it 'returns environment names for all public projects' do
+          public_project.add_reporter(user)
 
-        names = described_class.new(project1, user).execute
+          names = described_class.new(group, user).execute
 
-        expect(names).to eq(%w[gprd gstg])
+          expect(names).to eq(%w[gprd gstg])
+        end
       end
-    end
 
-    context 'using a public project and a guest' do
-      it 'returns all the unique environment names' do
-        names = described_class.new(project1, user).execute
+      context 'with a private project reporter' do
+        it 'returns environment names for all public projects' do
+          private_project.add_reporter(user)
+
+          names = described_class.new(group, user).execute
 
-        expect(names).to eq(%w[gprd gstg])
+          expect(names).to eq(%w[gcny gprd gstg])
+        end
       end
-    end
 
-    context 'using a private project and a guest' do
-      it 'returns all the unique environment names' do
-        names = described_class.new(project2, user).execute
+      context 'with a group guest' do
+        it 'returns environment names for all public projects' do
+          group.add_guest(user)
+
+          names = described_class.new(group, user).execute
+
+          expect(names).to eq(%w[gprd gstg])
+        end
+      end
+
+      context 'with a non-member' do
+        it 'returns environment names for all public projects' do
+          names = described_class.new(group, user).execute
+
+          expect(names).to eq(%w[gprd gstg])
+        end
+      end
+
+      context 'without a user' do
+        it 'returns environment names for all public projects' do
+          names = described_class.new(group).execute
 
-        expect(names).to be_empty
+          expect(names).to eq(%w[gprd gstg])
+        end
       end
     end
 
-    context 'using a public project without a user' do
-      it 'returns all the unique environment names' do
-        names = described_class.new(project1).execute
+    context 'using a public project' do
+      context 'with a project developer' do
+        it 'returns all the unique environment names' do
+          public_project.add_developer(user)
+
+          names = described_class.new(public_project, user).execute
+
+          expect(names).to eq(%w[gprd gstg])
+        end
+      end
+
+      context 'with a project reporter' do
+        it 'returns all the unique environment names' do
+          public_project.add_reporter(user)
+
+          names = described_class.new(public_project, user).execute
+
+          expect(names).to eq(%w[gprd gstg])
+        end
+      end
+
+      context 'with a project guest' do
+        it 'returns all the unique environment names' do
+          public_project.add_guest(user)
+
+          names = described_class.new(public_project, user).execute
 
-        expect(names).to eq(%w[gprd gstg])
+          expect(names).to eq(%w[gprd gstg])
+        end
+      end
+
+      context 'with a non-member' do
+        it 'returns all the unique environment names' do
+          names = described_class.new(public_project, user).execute
+
+          expect(names).to eq(%w[gprd gstg])
+        end
+      end
+
+      context 'without a user' do
+        it 'returns all the unique environment names' do
+          names = described_class.new(public_project).execute
+
+          expect(names).to eq(%w[gprd gstg])
+        end
       end
     end
 
-    context 'using a private project without a user' do
-      it 'does not return any environment names' do
-        names = described_class.new(project2).execute
+    context 'using a private project' do
+      context 'with a project developer' do
+        it 'returns all the unique environment names' do
+          private_project.add_developer(user)
+
+          names = described_class.new(private_project, user).execute
+
+          expect(names).to eq(%w[gcny gprd])
+        end
+      end
+
+      context 'with a project reporter' do
+        it 'returns all the unique environment names' do
+          private_project.add_reporter(user)
+
+          names = described_class.new(private_project, user).execute
+
+          expect(names).to eq(%w[gcny gprd])
+        end
+      end
+
+      context 'with a project guest' do
+        it 'does not return any environment names' do
+          private_project.add_guest(user)
+
+          names = described_class.new(private_project, user).execute
+
+          expect(names).to be_empty
+        end
+      end
+
+      context 'with a non-member' do
+        it 'does not return any environment names' do
+          names = described_class.new(private_project, user).execute
+
+          expect(names).to be_empty
+        end
+      end
+
+      context 'without a user' do
+        it 'does not return any environment names' do
+          names = described_class.new(private_project).execute
 
-        expect(names).to eq([])
+          expect(names).to be_empty
+        end
       end
     end
   end