From c25d94d68e0bc0ebde3ad050fc5a25bdf2729c26 Mon Sep 17 00:00:00 2001 From: ghinfey Date: Thu, 13 Oct 2022 10:02:22 +0100 Subject: [PATCH] Remove min_access_level check Remove min_access_level check in the finder available_subgroups_with_custom_project_templates Update specs for the same method Changelog: changed MR: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/100916 --- ee/app/helpers/ee/projects_helper.rb | 32 ++++++++---- ee/app/models/ee/user.rb | 2 +- .../users/available_group_templates.html.haml | 2 +- ee/spec/helpers/projects_helper_spec.rb | 52 +++++++++++++++++++ ee/spec/models/ee/user_spec.rb | 46 +++++++++++----- 5 files changed, 109 insertions(+), 25 deletions(-) diff --git a/ee/app/helpers/ee/projects_helper.rb b/ee/app/helpers/ee/projects_helper.rb index 5503461c533e48..2e75f0a71e381e 100644 --- a/ee/app/helpers/ee/projects_helper.rb +++ b/ee/app/helpers/ee/projects_helper.rb @@ -14,18 +14,18 @@ def sidebar_operations_paths override :project_permissions_settings def project_permissions_settings(project) super.merge({ - requirementsAccessLevel: project.requirements_access_level, - cveIdRequestEnabled: (project.public? && project.project_setting.cve_id_request_enabled?) - }) + requirementsAccessLevel: project.requirements_access_level, + cveIdRequestEnabled: (project.public? && project.project_setting.cve_id_request_enabled?) + }) end override :project_permissions_panel_data def project_permissions_panel_data(project) super.merge({ - requirementsAvailable: project.feature_available?(:requirements), - requestCveAvailable: ::Gitlab.com?, - cveIdRequestHelpPath: help_page_path('user/application_security/cve_id_request') - }) + requirementsAvailable: project.feature_available?(:requirements), + requestCveAvailable: ::Gitlab.com?, + cveIdRequestHelpPath: help_page_path('user/application_security/cve_id_request') + }) end override :default_url_to_repo @@ -162,9 +162,15 @@ def membership_locked? end def group_project_templates_count(group_id) - allowed_subgroups = current_user.available_subgroups_with_custom_project_templates(group_id) + projects_not_aimed_for_deletions_for(group_id).count do |project| + can?(current_user, :download_code, project) + end + end - ::Project.in_namespace(allowed_subgroups).not_aimed_for_deletion.count + def group_project_templates_select(group_id) + projects_not_aimed_for_deletions_for(group_id).select do |project| + can?(current_user, :download_code, project) + end end def project_security_dashboard_config(project) @@ -309,5 +315,13 @@ def security_dashboard_pipeline_data(project) } } end + + def allowed_subgroups(group_id) + current_user.available_subgroups_with_custom_project_templates(group_id) + end + + def projects_not_aimed_for_deletions_for(group_id) + ::Project.in_namespace(allowed_subgroups(group_id)).not_aimed_for_deletion + end end end diff --git a/ee/app/models/ee/user.rb b/ee/app/models/ee/user.rb index f2c2f2f91a177a..2c1ab470c53b13 100644 --- a/ee/app/models/ee/user.rb +++ b/ee/app/models/ee/user.rb @@ -227,7 +227,7 @@ def available_custom_project_templates(search: nil, subgroup_id: nil, project_id def available_subgroups_with_custom_project_templates(group_id = nil) found_groups = GroupsWithTemplatesFinder.new(group_id).execute - GroupsFinder.new(self, min_access_level: ::Gitlab::Access::DEVELOPER) + GroupsFinder.new(self) .execute .where(id: found_groups.select(:custom_project_templates_group_id)) .preload(:projects) diff --git a/ee/app/views/users/available_group_templates.html.haml b/ee/app/views/users/available_group_templates.html.haml index 8f5dd4e2f2d9e0..c70b6d472a3389 100644 --- a/ee/app/views/users/available_group_templates.html.haml +++ b/ee/app/views/users/available_group_templates.html.haml @@ -1,7 +1,7 @@ .custom-project-templates - if @groups_with_project_templates.present? - @groups_with_project_templates.each do |group| - - projects = group.projects.not_aimed_for_deletion + - projects = group_project_templates_select(group.id) .template-group-options.js-template-group-options{ class: ('expanded border-top-0' if @groups_with_project_templates.first == group) } .template-header.d-flex.align-items-center .template-subgroup.d-flex.flex-fill.align-items-center diff --git a/ee/spec/helpers/projects_helper_spec.rb b/ee/spec/helpers/projects_helper_spec.rb index c9d01dc5f4bbff..a31df80ad25f42 100644 --- a/ee/spec/helpers/projects_helper_spec.rb +++ b/ee/spec/helpers/projects_helper_spec.rb @@ -133,6 +133,7 @@ before do allow(helper).to receive(:current_user).and_return(user) + allow(helper).to receive(:can?).and_return(true) end specify do @@ -148,6 +149,57 @@ expect(helper.group_project_templates_count(parent_group.id)).to eq 0 end end + + context 'when project is not visible to user' do + before do + allow(helper).to receive(:can?).with(user, :download_code, template_project).and_return(false) + end + + specify do + expect(helper.group_project_templates_count(parent_group.id)).to eq 0 + end + end + end + + describe '#group_project_templates_select' do + let_it_be(:user) { create(:user) } + let_it_be(:parent_group) { create(:group, name: 'parent-group') } + let_it_be(:template_group) { create(:group, parent: parent_group, name: 'template-group') } + let_it_be(:template_project) { create(:project, group: template_group, name: 'template-project') } + + before_all do + parent_group.update!(custom_project_templates_group_id: template_group.id) + parent_group.add_owner(user) + end + + before do + allow(helper).to receive(:current_user).and_return(user) + allow(helper).to receive(:can?).and_return(true) + end + + specify do + expect(helper.group_project_templates_select(parent_group)).to match_array([template_project]) + end + + context 'when template project is pending deletion' do + before do + template_project.update!(marked_for_deletion_at: Date.current) + end + + specify do + expect(helper.group_project_templates_select(parent_group)).to be_empty + end + end + + context 'when project is not visible to user' do + before do + allow(helper).to receive(:can?).and_return(false) + end + + specify do + expect(helper.group_project_templates_select(parent_group)).to be_empty + end + end end describe '#project_security_dashboard_config' do diff --git a/ee/spec/models/ee/user_spec.rb b/ee/spec/models/ee/user_spec.rb index de8eb4548966f0..a4e485c58eb588 100644 --- a/ee/spec/models/ee/user_spec.rb +++ b/ee/spec/models/ee/user_spec.rb @@ -473,30 +473,48 @@ end context 'with Groups with custom project templates' do - let!(:group_1) { create(:group, name: 'group-1') } - let!(:group_2) { create(:group, name: 'group-2') } - let!(:group_3) { create(:group, name: 'group-3') } + let_it_be(:group_1) { create(:group, name: 'group-1') } + let_it_be(:group_2) { create(:group, :private, name: 'group-2') } + let_it_be(:group_3) { create(:group, name: 'group-3') } - let!(:subgroup_1) { create(:group, parent: group_1, name: 'subgroup-1') } - let!(:subgroup_2) { create(:group, parent: group_2, name: 'subgroup-2') } - let!(:subgroup_3) { create(:group, parent: group_3, name: 'subgroup-3') } + let_it_be(:subgroup_1) { create(:group, parent: group_1, name: 'subgroup-1') } + let_it_be(:subgroup_2) { create(:group, :private, parent: group_2, name: 'subgroup-2') } + let_it_be(:subgroup_3) { create(:group, parent: group_3, name: 'subgroup-3') } - before do + let_it_be(:public_project) { create(:project, namespace: subgroup_1) } + let_it_be(:private_project) { create(:project, :private, namespace: subgroup_2) } + + before_all do group_1.update!(custom_project_templates_group_id: subgroup_1.id) group_2.update!(custom_project_templates_group_id: subgroup_2.id) group_3.update!(custom_project_templates_group_id: subgroup_3.id) + end - create(:project, namespace: subgroup_1) - create(:project, namespace: subgroup_2) + context 'when a user is not a member of the groups' do + subject(:available_subgroups) { user.available_subgroups_with_custom_project_templates } + + it 'only templates in publicly visible groups with projects are available' do + expect(available_subgroups).to match_array([subgroup_1]) + end end - context 'when the access level of the user is below the required one' do - before do - group_1.add_reporter(user) + context 'when a user is a member of the groups' do + subject(:available_subgroups) { user.available_subgroups_with_custom_project_templates } + + where(:access_level) do + [:guest, :reporter, :developer, :maintainer, :owner] end - it 'returns an empty collection' do - expect(user.available_subgroups_with_custom_project_templates).to be_empty + with_them do + before do + group_1.add_member(user, access_level) + group_2.add_member(user, access_level) + group_3.add_member(user, access_level) + end + + it 'the templates in groups with projects are available' do + expect(available_subgroups).to match_array([subgroup_1, subgroup_2]) + end end end -- GitLab