From 2dcf5ee628b65203414bd9b7c3bc5cb6e34a861d Mon Sep 17 00:00:00 2001 From: Alex Buijs Date: Wed, 7 Feb 2024 12:43:22 +0100 Subject: [PATCH] Allow managing group CI/CD variables For users with the admin_cicd_variables custom role ability. EE: true --- .../group_settings/mount_shared_runners.js | 2 + .../groups/settings/ci_cd_controller.rb | 3 +- .../groups/variables_controller.rb | 7 +- app/graphql/types/group_type.rb | 4 +- app/policies/group_policy.rb | 1 + .../groups/settings/ci_cd/show.html.haml | 64 ++++++------ .../ee/sidebars/groups/menus/settings_menu.rb | 3 + .../groups/menus/settings_menu_spec.rb | 22 +++++ .../groups_request_spec.rb | 98 +++++++++++++++++++ 9 files changed, 165 insertions(+), 39 deletions(-) create mode 100644 ee/spec/requests/custom_roles/admin_cicd_variables/groups_request_spec.rb diff --git a/app/assets/javascripts/group_settings/mount_shared_runners.js b/app/assets/javascripts/group_settings/mount_shared_runners.js index 334192a6f87f49..480b712d9326fd 100644 --- a/app/assets/javascripts/group_settings/mount_shared_runners.js +++ b/app/assets/javascripts/group_settings/mount_shared_runners.js @@ -5,6 +5,8 @@ import UpdateSharedRunnersForm from './components/shared_runners_form.vue'; export default (containerId = 'update-shared-runners-form') => { const containerEl = document.getElementById(containerId); + if (!containerEl) return null; + const { groupId, groupName, diff --git a/app/controllers/groups/settings/ci_cd_controller.rb b/app/controllers/groups/settings/ci_cd_controller.rb index 371db7b30b6845..0340fc15f911e1 100644 --- a/app/controllers/groups/settings/ci_cd_controller.rb +++ b/app/controllers/groups/settings/ci_cd_controller.rb @@ -5,7 +5,8 @@ module Settings class CiCdController < Groups::ApplicationController layout 'group_settings' skip_cross_project_access_check :show - before_action :authorize_admin_group! + before_action :authorize_admin_group!, except: :show + before_action :authorize_admin_cicd_variables!, only: :show before_action :authorize_update_max_artifacts_size!, only: [:update] before_action :define_variables, only: [:show] before_action :push_licensed_features, only: [:show] diff --git a/app/controllers/groups/variables_controller.rb b/app/controllers/groups/variables_controller.rb index d27d70dc85704a..d857854d977031 100644 --- a/app/controllers/groups/variables_controller.rb +++ b/app/controllers/groups/variables_controller.rb @@ -2,7 +2,8 @@ module Groups class VariablesController < Groups::ApplicationController - before_action :authorize_admin_group! + before_action :authorize_admin_group!, except: :update + before_action :authorize_admin_cicd_variables!, only: :update skip_cross_project_access_check :show, :update @@ -52,10 +53,6 @@ def group_variables_params def variable_params_attributes %i[id variable_type key description secret_value protected masked raw _destroy] end - - def authorize_admin_build! - return render_404 unless can?(current_user, :admin_build, group) - end end end diff --git a/app/graphql/types/group_type.rb b/app/graphql/types/group_type.rb index b2d05cef8fa52c..9a2730eb74878f 100644 --- a/app/graphql/types/group_type.rb +++ b/app/graphql/types/group_type.rb @@ -96,7 +96,7 @@ class GroupType < NamespaceType Types::Ci::GroupEnvironmentScopeType.connection_type, description: 'Environment scopes of the group.', null: true, - authorize: :admin_group, + authorize: :admin_cicd_variables, resolver: Resolvers::GroupEnvironmentScopesResolver field :milestones, @@ -241,7 +241,7 @@ class GroupType < NamespaceType Types::Ci::GroupVariableType.connection_type, null: true, description: "List of the group's CI/CD variables.", - authorize: :admin_group, + authorize: :admin_cicd_variables, resolver: Resolvers::Ci::VariablesResolver field :runners, Types::Ci::RunnerType.connection_type, diff --git a/app/policies/group_policy.rb b/app/policies/group_policy.rb index 1c9aaa61be42d8..adbc399866b74f 100644 --- a/app/policies/group_policy.rb +++ b/app/policies/group_policy.rb @@ -246,6 +246,7 @@ class GroupPolicy < Namespaces::GroupProjectNamespaceSharedPolicy enable :update_runners_registration_token enable :owner_access enable :update_git_access_protocol + enable :admin_cicd_variables enable :read_billing enable :edit_billing diff --git a/app/views/groups/settings/ci_cd/show.html.haml b/app/views/groups/settings/ci_cd/show.html.haml index f9ade00a300bb5..c03747400348f3 100644 --- a/app/views/groups/settings/ci_cd/show.html.haml +++ b/app/views/groups/settings/ci_cd/show.html.haml @@ -19,38 +19,40 @@ .settings-content = render 'groups/settings/ci_cd/form', group: @group -%section.settings#ci-variables.no-animate{ class: ('expanded' if expanded) } - .settings-header - = render 'ci/variables/header', expanded: expanded - .settings-content - = render 'ci/variables/index', save_endpoint: group_variables_path +- if can?(current_user, :admin_cicd_variables, @group) + %section.settings#ci-variables.no-animate{ class: ('expanded' if expanded) } + .settings-header + = render 'ci/variables/header', expanded: expanded + .settings-content + = render 'ci/variables/index', save_endpoint: group_variables_path -%section.settings#runners-settings.no-animate{ class: ('expanded' if expanded) } - .settings-header - %h4.settings-title.js-settings-toggle.js-settings-toggle-trigger-only - = _('Runners') - = render Pajamas::ButtonComponent.new(button_options: { class: 'js-settings-toggle' }) do - = expanded ? _('Collapse') : _('Expand') - %p.gl-text-secondary - = _("Runners are processes that pick up and execute CI/CD jobs for GitLab.") - = link_to s_('What is GitLab Runner?'), 'https://docs.gitlab.com/runner/', target: '_blank', rel: 'noopener noreferrer' - .settings-content - = render 'groups/runners/settings' +- if can?(current_user, :admin_group, @group) + %section.settings#runners-settings.no-animate{ class: ('expanded' if expanded) } + .settings-header + %h4.settings-title.js-settings-toggle.js-settings-toggle-trigger-only + = _('Runners') + = render Pajamas::ButtonComponent.new(button_options: { class: 'js-settings-toggle' }) do + = expanded ? _('Collapse') : _('Expand') + %p.gl-text-secondary + = _("Runners are processes that pick up and execute CI/CD jobs for GitLab.") + = link_to s_('What is GitLab Runner?'), 'https://docs.gitlab.com/runner/', target: '_blank', rel: 'noopener noreferrer' + .settings-content + = render 'groups/runners/settings' -%section.settings#auto-devops-settings.no-animate{ class: ('expanded' if expanded) } - .settings-header - %h4.settings-title.js-settings-toggle.js-settings-toggle-trigger-only - = _('Auto DevOps') - = render Pajamas::ButtonComponent.new(button_options: { class: 'js-settings-toggle' }) do - = expanded ? _('Collapse') : _('Expand') - %p.gl-text-secondary - - auto_devops_url = help_page_path('topics/autodevops/index') - - quickstart_url = help_page_path('topics/autodevops/cloud_deployments/auto_devops_with_gke') - - auto_devops_start = ''.html_safe % { url: auto_devops_url } - - quickstart_start = ''.html_safe % { url: quickstart_url } - = html_escape(s_('AutoDevOps|%{auto_devops_start}Automate building, testing, and deploying%{auto_devops_end} your applications based on your continuous integration and delivery configuration. %{quickstart_start}How do I get started?%{quickstart_end}')) % { auto_devops_start: auto_devops_start, auto_devops_end: ''.html_safe, quickstart_start: quickstart_start, quickstart_end: ''.html_safe } + %section.settings#auto-devops-settings.no-animate{ class: ('expanded' if expanded) } + .settings-header + %h4.settings-title.js-settings-toggle.js-settings-toggle-trigger-only + = _('Auto DevOps') + = render Pajamas::ButtonComponent.new(button_options: { class: 'js-settings-toggle' }) do + = expanded ? _('Collapse') : _('Expand') + %p.gl-text-secondary + - auto_devops_url = help_page_path('topics/autodevops/index') + - quickstart_url = help_page_path('topics/autodevops/cloud_deployments/auto_devops_with_gke') + - auto_devops_start = ''.html_safe % { url: auto_devops_url } + - quickstart_start = ''.html_safe % { url: quickstart_url } + = html_escape(s_('AutoDevOps|%{auto_devops_start}Automate building, testing, and deploying%{auto_devops_end} your applications based on your continuous integration and delivery configuration. %{quickstart_start}How do I get started?%{quickstart_end}')) % { auto_devops_start: auto_devops_start, auto_devops_end: ''.html_safe, quickstart_start: quickstart_start, quickstart_end: ''.html_safe } - .settings-content - = render 'groups/settings/ci_cd/auto_devops_form', group: @group + .settings-content + = render 'groups/settings/ci_cd/auto_devops_form', group: @group -= render_if_exists 'groups/settings/ci_cd/protected_environments', expanded: expanded + = render_if_exists 'groups/settings/ci_cd/protected_environments', expanded: expanded diff --git a/ee/lib/ee/sidebars/groups/menus/settings_menu.rb b/ee/lib/ee/sidebars/groups/menus/settings_menu.rb index 7b2bae35815af2..096b238c32b728 100644 --- a/ee/lib/ee/sidebars/groups/menus/settings_menu.rb +++ b/ee/lib/ee/sidebars/groups/menus/settings_menu.rb @@ -30,6 +30,9 @@ def configure_menu_items # They only get the Repository settings which only show the Push Rules section for maintainers. add_item(repository_menu_item) if can?(context.current_user, :change_push_rules, context.group) + # Managing CI/CD settings is a custom ability independent of the access level. + add_item(ci_cd_menu_item) if can?(context.current_user, :admin_cicd_variables, context.group) + add_item(billing_menu_item) if can?(context.current_user, :read_billing, context.group) end end diff --git a/ee/spec/lib/ee/sidebars/groups/menus/settings_menu_spec.rb b/ee/spec/lib/ee/sidebars/groups/menus/settings_menu_spec.rb index 82a25f8f657555..9f543d941dfa75 100644 --- a/ee/spec/lib/ee/sidebars/groups/menus/settings_menu_spec.rb +++ b/ee/spec/lib/ee/sidebars/groups/menus/settings_menu_spec.rb @@ -294,5 +294,27 @@ end end end + + context 'when the user is not an owner but has `admin_cicd_variables` custom ability', feature_category: :permissions do + let_it_be(:user) { create(:user) } + + subject { menu.renderable_items.find { |e| e.item_id == item_id } } + + before do + allow(Ability).to receive(:allowed?).and_call_original + allow(Ability).to receive(:allowed?).with(user, :admin_group, group).and_return(false) + allow(Ability).to receive(:allowed?).with(user, :admin_cicd_variables, group).and_return(true) + end + + describe 'CI/CD menu item' do + let(:item_id) { :ci_cd } + + it { is_expected.to be_present } + + it 'does not show any other menu items' do + expect(menu.renderable_items.length).to equal(1) + end + end + end end end diff --git a/ee/spec/requests/custom_roles/admin_cicd_variables/groups_request_spec.rb b/ee/spec/requests/custom_roles/admin_cicd_variables/groups_request_spec.rb new file mode 100644 index 00000000000000..ffb4cf31a8c38c --- /dev/null +++ b/ee/spec/requests/custom_roles/admin_cicd_variables/groups_request_spec.rb @@ -0,0 +1,98 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe 'User with admin_cicd_variables custom role', feature_category: :secrets_management do + let_it_be(:user) { create(:user) } + let_it_be(:group) { create(:group) } + let_it_be(:role) { create(:member_role, :guest, namespace: group, admin_cicd_variables: true) } + let_it_be(:member) { create(:group_member, :guest, member_role: role, user: user, group: group) } + + before do + stub_licensed_features(custom_roles: true) + + sign_in(user) + end + + describe Groups::Settings::CiCdController do + describe '#show' do + it 'user can view CI/CD settings page' do + get group_settings_ci_cd_path(group) + + expect(response).to have_gitlab_http_status(:ok) + expect(response.body).to include('CI/CD Settings') + end + end + end + + describe 'Querying CI Variables and environment scopes' do + include GraphqlHelpers + + let_it_be(:variable) do + create(:ci_group_variable, key: 'new_key', value: 'dummy_value', group: group, environment_scope: 'test_scope') + end + + let(:query) do + %( + query { + group(fullPath: "#{group.full_path}") { + ciVariables { + nodes { + key + value + } + } + environmentScopes { + nodes { + name + } + } + } + } + ) + end + + it 'returns variables and scopes for the group' do + result = GitlabSchema.execute(query, context: { current_user: user }).as_json + + group_data = result.dig('data', 'group') + + expect(group_data.dig('ciVariables', 'nodes').first).to eq('key' => variable.key, 'value' => variable.value) + expect(group_data.dig('environmentScopes', 'nodes').first).to eq('name' => variable.environment_scope) + end + end + + describe Groups::VariablesController do + describe '#update' do + it 'user can create CI/CD variables' do + params = { variables_attributes: [{ key: 'new_key', secret_value: 'dummy_value' }] } + put group_variables_path(group, params: params, format: :json) + + expect(response).to have_gitlab_http_status(:ok) + expect(Gitlab::Json.parse(response.body)['variables'][0]) + .to include('key' => 'new_key', 'value' => 'dummy_value') + end + + it 'user can update CI/CD variables' do + var = create(:ci_group_variable, group: group) + + params = { variables_attributes: [{ id: var.id, key: 'new_key', secret_value: 'dummy_value' }] } + put group_variables_path(group, params: params, format: :json) + + expect(response).to have_gitlab_http_status(:ok) + expect(Gitlab::Json.parse(response.body)['variables'][0]) + .to include('key' => 'new_key', 'value' => 'dummy_value') + end + + it 'user can destroy CI/CD variables' do + var = create(:ci_group_variable, group: group) + + params = { variables_attributes: [{ id: var.id, _destroy: 'true' }] } + put group_variables_path(group, params: params, format: :json) + + expect(response).to have_gitlab_http_status(:ok) + expect { var.reload }.to raise_error(ActiveRecord::RecordNotFound) + end + end + end +end -- GitLab