Limit access to templates with restricted features

Context:
The custom project templates feature uses project export to copy the
template data to the new project.

Problem:
Public project templates, with restricted features access, like issues
restricted to the project members only, were having the protected data
leaked to the new project since the feature access level wasn't being
validated on the `User#available_custom_project_templates`.

Solution:
Validate the user access to all the exportable features before listing
the project as an available custom project template to the user.

doc/user/admin_area/custom_project_templates.md

@@ -15,7 +15,8 @@ templates are sourced.
 Every project directly under the group namespace will be
 available to the user if they have access to them. For example:
 
-- Public project in the group will be available to every logged in user.
+- Public projects, in the group will be available to every signed-in user, if all enabled [project features](../project/settings/index.md#sharing-and-permissions)
+  are set to **Everyone With Access**.
 - Private projects will be available only if the user is a member of the project.
 
 Repository and database information that are copied over to each new project are

doc/user/group/custom_project_templates.md

@@ -61,9 +61,8 @@ GitLab administrators can
 [set project templates for an entire GitLab instance](../admin_area/custom_project_templates.md).
 
 Within this section, you can configure the group where all the custom project
-templates are sourced. Every project directly under the group namespace will be
-available to the user if they have access to them. For example, every public
-project in the group will be available to every logged in user.
+templates are sourced. Every project _template_ directly under the group namespace is
+available to every signed-in user, if all enabled [project features](../project/settings/index.md#sharing-and-permissions) are set to **Everyone With Access**.
 
 However, private projects will be available only if the user is a member of the project.
 

ee/app/finders/custom_project_templates_finder.rb

+# frozen_string_literal: true
+
+# To avoid leaking protected project features in templates, users will only have
+# access to use project as templates if they have access to all the enabled
+# project features
+class CustomProjectTemplatesFinder < ::ProjectsFinder
+  def initialize(current_user:, search: nil, subgroup_id: nil, project_id: nil)
+    @current_user = current_user
+    @search = search
+    @subgroup_id = subgroup_id
+    @project_id = project_id
+  end
+
+  def execute
+    scope = super
+
+    ::ProjectFeature::FEATURES.reduce(scope) do |scope, feature|
+      scope.with_feature_access_level(feature, ::ProjectFeature::DISABLED)
+        .or(scope.with_feature_available_for_user(feature, current_user))
+    end
+  end
+
+  # Override the `::ProjectsFinder#params` to leverage of the scope build there.
+  def params
+    @params ||=
+      if project_id
+        {}
+      else
+        { search: search, sort: 'name_asc' }
+      end
+  end
+
+  # Override the `::ProjectsFinder#project_ids_relation` to leverage of the scope build there.
+  def project_ids_relation
+    @project_ids_relation ||=
+      if project_id
+        templates.id_in(project_id)
+      else
+        templates
+      end
+  end
+
+  private
+
+  attr_reader :search, :subgroup_id, :project_id
+
+  def templates
+    @templates ||= ::Gitlab::CurrentSettings
+      .available_custom_project_templates(subgroup_id)
+  end
+end

ee/app/models/ee/user.rb

@@ -176,20 +176,9 @@ module EE
     end
 
     def available_custom_project_templates(search: nil, subgroup_id: nil, project_id: nil)
-      templates = ::Gitlab::CurrentSettings.available_custom_project_templates(subgroup_id)
-
-      params = {}
-
-      if project_id
-        templates = templates.where(id: project_id)
-      else
-        params = { search: search, sort: 'name_asc' }
-      end
-
-      ::ProjectsFinder.new(current_user: self,
-                           project_ids_relation: templates,
-                           params: params)
-                      .execute
+      CustomProjectTemplatesFinder
+        .new(current_user: self, search: search, subgroup_id: subgroup_id, project_id: project_id)
+        .execute
     end
 
     def available_subgroups_with_custom_project_templates(group_id = nil)

ee/changelogs/unreleased/security-project-user-custom-templates-from-leaking-features.yml

+---
+title: Do not leak templates with protected features
+merge_request:
+author:
+type: security

ee/spec/controllers/projects_controller_spec.rb

@@ -103,7 +103,7 @@ RSpec.describe ProjectsController do
 
     context 'custom project templates' do
       let(:group) { create(:group) }
-      let(:project_template) { create(:project, :repository, :public, namespace: group) }
+      let(:project_template) { create(:project, :repository, :public, :metrics_dashboard_enabled, namespace: group) }
       let(:templates_params) do
         {
           path: 'foo',

ee/spec/features/projects/custom_projects_template_spec.rb

@@ -6,7 +6,7 @@ RSpec.describe 'Project' do
   describe 'Custom instance-level projects templates' do
     let(:user) { create(:user) }
     let(:group) { create(:group) }
-    let!(:projects) { create_list(:project, 3, :public, namespace: group) }
+    let!(:projects) { create_list(:project, 3, :public, :metrics_dashboard_enabled, namespace: group) }
 
     before do
       stub_ee_application_setting(custom_project_templates_group_id: group.id)

ee/spec/finders/custom_project_templates_finder_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe CustomProjectTemplatesFinder do
+  let_it_be(:user) { create(:user) }
+  let(:base_params) { { current_user: user } }
+  let(:params) { {} }
+
+  subject { described_class.new(base_params.merge(params)) }
+
+  it 'returns an empty relation if group is not set' do
+    expect(subject.execute).to be_empty
+  end
+
+  context 'when group with custom project templates is set' do
+    let_it_be(:group) { create(:group) }
+
+    before do
+      stub_ee_application_setting(custom_project_templates_group_id: group.id)
+    end
+
+    it 'returns an empty relation if group has no available project templates' do
+      expect(group.projects).to be_empty
+      expect(subject.execute).to be_empty
+    end
+
+    context 'when the group has projects' do
+      using RSpec::Parameterized::TableSyntax
+
+      let_it_be_with_reload(:private_project) { create :project, :metrics_dashboard_enabled, :private, namespace: group, name: 'private' }
+      let_it_be_with_reload(:internal_project) { create :project, :metrics_dashboard_enabled, :internal, namespace: group, name: 'internal' }
+      let_it_be_with_reload(:public_project) { create :project, :metrics_dashboard_enabled, :public, namespace: group, name: 'public' }
+
+      where(:issues_access_level, :minimal_user_access, :available_templates) do
+        :disabled | :no_access | %w[public]
+        :disabled | :guest     | %w[public internal private]
+        :private  | :guest     | %w[public internal private]
+        :private  | :no_access | %w[]
+        :enabled  | :guest     | %w[public internal private]
+        :enabled  | :no_access | %w[public]
+      end
+
+      with_them do
+        context "when templates have the issues feature #{params[:issues_access_level]}" do
+          before do
+            private_project.project_feature.update!(issues_access_level: ProjectFeature::STRING_OPTIONS[issues_access_level])
+            internal_project.project_feature.update!(issues_access_level: ProjectFeature::STRING_OPTIONS[issues_access_level])
+            public_project.project_feature.update!(issues_access_level: ProjectFeature::STRING_OPTIONS[issues_access_level])
+          end
+
+          it "returns #{params[:available_templates].join(', ')} projects to users with #{params[:minimal_user_access]} to the project" do
+            unless minimal_user_access == :no_access
+              public_project.add_user(user, minimal_user_access)
+              internal_project.add_user(user, minimal_user_access)
+              private_project.add_user(user, minimal_user_access)
+            end
+
+            expect(subject.execute.pluck(:name)).to match_array(available_templates)
+          end
+
+          it "returns #{params[:available_templates].join(', ')} projects to users with #{params[:minimal_user_access]} to the group" do
+            unless minimal_user_access == :no_access
+              group.add_user(user, minimal_user_access)
+            end
+
+            expect(subject.execute.pluck(:name)).to match_array(available_templates)
+          end
+        end
+      end
+
+      context 'filtering the results' do
+        let_it_be(:other_public_project) { create :project, :metrics_dashboard_enabled, :public, namespace: group, name: 'other public' }
+
+        it 'allows to search available project templates by name' do
+          params[:search] = 'publi'
+
+          expect(subject.execute).to contain_exactly(public_project, other_public_project)
+        end
+
+        it 'filters by single project ID' do
+          params[:project_id] = public_project.id
+
+          expect(subject.execute).to contain_exactly(public_project)
+        end
+
+        it 'filters by list of project IDs' do
+          params[:project_id] = [public_project.id, other_public_project.id]
+
+          expect(subject.execute).to contain_exactly(public_project, other_public_project)
+        end
+
+        it 'does not return inaccessible projects' do
+          params[:project_id] = private_project.id
+
+          expect(subject.execute).to be_empty
+        end
+      end
+    end
+  end
+end

ee/spec/models/user_spec.rb

@@ -316,8 +316,8 @@ RSpec.describe User do
       context 'when group has custom project templates' do
         let!(:private_project) { create :project, :private, namespace: group, name: 'private_project' }
         let!(:internal_project) { create :project, :internal, namespace: group, name: 'internal_project' }
-        let!(:public_project) { create :project, :public, namespace: group, name: 'public_project' }
-        let!(:public_project_two) { create :project, :public, namespace: group, name: 'public_project_second' }
+        let!(:public_project) { create :project, :metrics_dashboard_enabled, :public, namespace: group, name: 'public_project' }
+        let!(:public_project_two) { create :project, :metrics_dashboard_enabled, :public, namespace: group, name: 'public_project_second' }
 
         it 'returns public projects' do
           expect(user.available_custom_project_templates).to include public_project
@@ -341,8 +341,22 @@ RSpec.describe User do
           end
         end
 
-        it 'returns internal projects' do
-          expect(user.available_custom_project_templates).to include internal_project
+        context 'returns internal projects if user' do
+          it 'is a member of the project' do
+            expect(user.available_custom_project_templates).not_to include internal_project
+
+            internal_project.add_developer(user)
+
+            expect(user.available_custom_project_templates).to include internal_project
+          end
+
+          it 'is a member of the group' do
+            expect(user.available_custom_project_templates).not_to include internal_project
+
+            group.add_developer(user)
+
+            expect(user.available_custom_project_templates).to include internal_project
+          end
         end
 
         it 'allows to search available project templates by name' do
@@ -370,6 +384,22 @@ RSpec.describe User do
           expect(projects.count).to eq 0
         end
       end
+
+      it 'returns project with disabled features' do
+        public_project = create(:project, :public, :metrics_dashboard_enabled, namespace: group)
+        disabled_issues_project = create(:project, :public, :metrics_dashboard_enabled, :issues_disabled, namespace: group)
+
+        expect(user.available_custom_project_templates).to include public_project
+        expect(user.available_custom_project_templates).to include disabled_issues_project
+      end
+
+      it 'does not return project with private issues' do
+        accessible_project = create(:project, :public, :metrics_dashboard_enabled, namespace: group)
+        restricted_features_project = create(:project, :public, :metrics_dashboard_enabled, :issues_private, namespace: group)
+
+        expect(user.available_custom_project_templates).to include accessible_project
+        expect(user.available_custom_project_templates).not_to include restricted_features_project
+      end
     end
   end
 

ee/spec/services/projects/create_from_template_service_spec.rb

@@ -31,7 +31,7 @@ RSpec.describe Projects::CreateFromTemplateService do
   let(:subgroup_1_2_1) { create(:group, parent: subgroup_1_2) }
   let(:subgroup_2) { create(:group, parent: group) }
   let(:subgroup_2_1) { create(:group, parent: subgroup_2) }
-  let(:project_template) { create(:project, :public, namespace: subgroup_1_2) }
+  let(:project_template) { create(:project, :public, :metrics_dashboard_enabled, namespace: subgroup_1_2) }
   let(:template_name) { project_template.name }
   let(:namespace_id) { nil }
   let(:group_with_project_templates_id) { nil }

spec/factories/projects.rb

@@ -29,6 +29,7 @@ FactoryBot.define do
       pages_access_level do
         visibility_level == Gitlab::VisibilityLevel::PUBLIC ? ProjectFeature::ENABLED : ProjectFeature::PRIVATE
       end
+      metrics_dashboard_access_level { ProjectFeature::PRIVATE }
 
       # we can't assign the delegated `#ci_cd_settings` attributes directly, as the
       # `#ci_cd_settings` relation needs to be created first
@@ -53,7 +54,8 @@ FactoryBot.define do
         forking_access_level: evaluator.forking_access_level,
         merge_requests_access_level: merge_requests_access_level,
         repository_access_level: evaluator.repository_access_level,
-        pages_access_level: evaluator.pages_access_level
+        pages_access_level: evaluator.pages_access_level,
+        metrics_dashboard_access_level: evaluator.metrics_dashboard_access_level
       }
 
       project.build_project_feature(hash)
@@ -309,6 +311,9 @@ FactoryBot.define do
     trait(:pages_enabled)           { pages_access_level { ProjectFeature::ENABLED } }
     trait(:pages_disabled)          { pages_access_level { ProjectFeature::DISABLED } }
     trait(:pages_private)           { pages_access_level { ProjectFeature::PRIVATE } }
+    trait(:metrics_dashboard_enabled) { metrics_dashboard_access_level { ProjectFeature::ENABLED } }
+    trait(:metrics_dashboard_disabled) { metrics_dashboard_access_level { ProjectFeature::DISABLED } }
+    trait(:metrics_dashboard_private) { metrics_dashboard_access_level { ProjectFeature::PRIVATE } }
 
     trait :auto_devops do
       association :auto_devops, factory: :project_auto_devops

spec/requests/projects/metrics_dashboard_spec.rb

@@ -39,7 +39,9 @@ RSpec.describe 'Projects::MetricsDashboardController' do
 
     context 'with anonymous user and public dashboard visibility' do
       let(:anonymous_user) { create(:user) }
-      let(:project) { create(:project, :public) }
+      let(:project) do
+        create(:project, :public, :metrics_dashboard_enabled)
+      end
 
       before do
         project.update!(metrics_dashboard_access_level: 'enabled')