From 9eee915f644bffb125ee4fe8ff9e1d8b6256c862 Mon Sep 17 00:00:00 2001 From: Alex Buijs Date: Mon, 19 Feb 2024 15:33:35 +0100 Subject: [PATCH 1/2] Add Remove Group custom ability This adds "Remove Group" as a custom role ability, so that it can be added onto any base role. Changelog: added EE: true --- app/controllers/groups_controller.rb | 4 +- app/policies/group_policy.rb | 4 + app/views/groups/edit.html.haml | 103 +++++++++--------- ...143045_add_remove_group_to_member_roles.rb | 15 +++ db/schema_migrations/20240219143045 | 1 + db/structure.sql | 3 +- doc/api/graphql/reference/index.md | 1 + doc/user/custom_roles/abilities.md | 1 + .../groups/settings/permissions/index.js | 3 + ee/app/controllers/ee/groups_controller.rb | 4 +- ee/app/policies/ee/group_policy.rb | 13 +++ .../groups/mark_for_deletion_service.rb | 2 +- ee/app/services/groups/restore_service.rb | 2 +- ee/config/custom_abilities/remove_group.yml | 11 ++ ee/lib/ee/api/groups.rb | 2 +- .../ee/sidebars/groups/menus/settings_menu.rb | 22 ++-- .../groups/menus/settings_menu_spec.rb | 22 ++++ ee/spec/models/ee/user_spec.rb | 1 + ee/spec/policies/group_policy_spec.rb | 18 +++ ee/spec/requests/api/member_roles_spec.rb | 2 + .../custom_roles/remove_group/request_spec.rb | 78 +++++++++++++ ...es_prompt_settings_link_shared_examples.rb | 2 + ee/spec/views/groups/edit.html.haml_spec.rb | 18 +++ lib/api/groups.rb | 2 +- .../policies/group_policy_shared_context.rb | 2 + 25 files changed, 265 insertions(+), 71 deletions(-) create mode 100644 db/migrate/20240219143045_add_remove_group_to_member_roles.rb create mode 100644 db/schema_migrations/20240219143045 create mode 100644 ee/config/custom_abilities/remove_group.yml create mode 100644 ee/spec/requests/custom_roles/remove_group/request_spec.rb diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb index 2ca7c7e368a3ed..a049c7de974be7 100644 --- a/app/controllers/groups_controller.rb +++ b/app/controllers/groups_controller.rb @@ -21,7 +21,9 @@ class GroupsController < Groups::ApplicationController before_action :group, except: [:index, :new, :create] # Authorize - before_action :authorize_admin_group!, only: [:edit, :update, :destroy, :projects, :transfer, :export, :download_export] + before_action :authorize_admin_group!, only: [:update, :projects, :transfer, :export, :download_export] + before_action :authorize_view_edit_page!, only: :edit + before_action :authorize_remove_group!, only: :destroy before_action :authorize_create_group!, only: [:new] before_action :load_recaptcha, only: [:new], if: -> { captcha_required? } diff --git a/app/policies/group_policy.rb b/app/policies/group_policy.rb index adbc399866b74f..8c2664d638a3e8 100644 --- a/app/policies/group_policy.rb +++ b/app/policies/group_policy.rb @@ -250,6 +250,8 @@ class GroupPolicy < Namespaces::GroupProjectNamespaceSharedPolicy enable :read_billing enable :edit_billing + + enable :remove_group end rule { can?(:read_nested_project_resources) }.policy do @@ -372,6 +374,8 @@ class GroupPolicy < Namespaces::GroupProjectNamespaceSharedPolicy rule { maintainer & ~raise_admin_package_to_owner_enabled }.enable :admin_package rule { owner & raise_admin_package_to_owner_enabled }.enable :admin_package + rule { can?(:remove_group) }.enable :view_edit_page + def access_level(for_any_session: false) return GroupMember::NO_ACCESS if @user.nil? return GroupMember::NO_ACCESS unless user_is_user? diff --git a/app/views/groups/edit.html.haml b/app/views/groups/edit.html.haml index 5c09fdcd021cc5..1c1388013afe50 100644 --- a/app/views/groups/edit.html.haml +++ b/app/views/groups/edit.html.haml @@ -5,57 +5,62 @@ = render 'shared/namespaces/cascading_settings/lock_popovers' -%section.settings.gs-general.no-animate.expanded#js-general-settings - .settings-header - %h4.settings-title.js-settings-toggle.js-settings-toggle-trigger-only{ role: 'button' } - = _('Naming, visibility') - = render Pajamas::ButtonComponent.new(button_options: { class: 'js-settings-toggle' }) do - = _('Collapse') - %p.gl-text-secondary - = _('Update your group name, description, avatar, and visibility.') - = link_to _('Learn more about groups.'), help_page_path('user/group/index') - .settings-content - = render 'groups/settings/general' +- if can?(current_user, :admin_group, @group) + %section.settings.gs-general.no-animate.expanded#js-general-settings + .settings-header + %h4.settings-title.js-settings-toggle.js-settings-toggle-trigger-only{ role: 'button' } + = _('Naming, visibility') + = render Pajamas::ButtonComponent.new(button_options: { class: 'js-settings-toggle' }) do + = _('Collapse') + %p.gl-text-secondary + = _('Update your group name, description, avatar, and visibility.') + = link_to _('Learn more about groups.'), help_page_path('user/group/index') + .settings-content + = render 'groups/settings/general' -%section.settings.gs-permissions.no-animate#js-permissions-settings{ class: ('expanded' if expanded), data: { testid: 'permissions-settings' } } - .settings-header - %h4.settings-title.js-settings-toggle.js-settings-toggle-trigger-only{ role: 'button' } - = _('Permissions and group features') - = render Pajamas::ButtonComponent.new(button_options: { class: 'js-settings-toggle' }) do - = expanded ? _('Collapse') : _('Expand') - %p.gl-text-secondary - = _('Configure advanced permissions, Large File Storage, two-factor authentication, and customer relations settings.') - .settings-content - = render 'groups/settings/permissions' + %section.settings.gs-permissions.no-animate#js-permissions-settings{ class: ('expanded' if expanded), data: { testid: 'permissions-settings' } } + .settings-header + %h4.settings-title.js-settings-toggle.js-settings-toggle-trigger-only{ role: 'button' } + = _('Permissions and group features') + = render Pajamas::ButtonComponent.new(button_options: { class: 'js-settings-toggle' }) do + = expanded ? _('Collapse') : _('Expand') + %p.gl-text-secondary + = _('Configure advanced permissions, Large File Storage, two-factor authentication, and customer relations settings.') + .settings-content + = render 'groups/settings/permissions' -= render_if_exists 'groups/settings/merge_requests/merge_requests', expanded: expanded, group: @group -= render_if_exists 'groups/settings/merge_requests/merge_request_approval_settings', expanded: expanded, group: @group, user: current_user -= render_if_exists 'groups/analytics', expanded: expanded + = render_if_exists 'groups/settings/merge_requests/merge_requests', expanded: expanded, group: @group + = render_if_exists 'groups/settings/merge_requests/merge_request_approval_settings', expanded: expanded, group: @group, user: current_user + = render_if_exists 'groups/analytics', expanded: expanded -%section.settings.no-animate#js-badge-settings{ class: ('expanded' if expanded) } - .settings-header - %h4.settings-title.js-settings-toggle.js-settings-toggle-trigger-only{ role: 'button' } - = s_('GroupSettings|Badges') - = render Pajamas::ButtonComponent.new(button_options: { class: 'js-settings-toggle' }) do - = expanded ? _('Collapse') : _('Expand') - %p.gl-text-secondary - = s_('GroupSettings|Customize this group\'s badges.') - = link_to s_('GroupSettings|What are badges?'), help_page_path('user/project/badges') - .settings-content - = render 'shared/badges/badge_settings' + %section.settings.no-animate#js-badge-settings{ class: ('expanded' if expanded) } + .settings-header + %h4.settings-title.js-settings-toggle.js-settings-toggle-trigger-only{ role: 'button' } + = s_('GroupSettings|Badges') + = render Pajamas::ButtonComponent.new(button_options: { class: 'js-settings-toggle' }) do + = expanded ? _('Collapse') : _('Expand') + %p.gl-text-secondary + = s_('GroupSettings|Customize this group\'s badges.') + = link_to s_('GroupSettings|What are badges?'), help_page_path('user/project/badges') + .settings-content + = render 'shared/badges/badge_settings' -= render_if_exists 'groups/compliance_frameworks', expanded: expanded -= render_if_exists 'groups/custom_project_templates_setting' -= render_if_exists 'groups/templates_setting', expanded: expanded -= render_if_exists 'shared/groups/max_pages_size_setting' + = render_if_exists 'groups/compliance_frameworks', expanded: expanded + = render_if_exists 'groups/custom_project_templates_setting' + = render_if_exists 'groups/templates_setting', expanded: expanded + = render_if_exists 'shared/groups/max_pages_size_setting' -%section.settings.gs-advanced.no-animate#js-advanced-settings{ class: ('expanded' if expanded), data: { testid: 'advanced-settings-content' } } - .settings-header - %h4.settings-title.js-settings-toggle.js-settings-toggle-trigger-only{ role: 'button' } - = _('Advanced') - = render Pajamas::ButtonComponent.new(button_options: { class: 'js-settings-toggle' }) do - = expanded ? _('Collapse') : _('Expand') - %p.gl-text-secondary - = _('Perform advanced options such as changing path, transferring, exporting, or removing the group.') - .settings-content - = render 'groups/settings/advanced' + %section.settings.gs-advanced.no-animate#js-advanced-settings{ class: ('expanded' if expanded), data: { testid: 'advanced-settings-content' } } + .settings-header + %h4.settings-title.js-settings-toggle.js-settings-toggle-trigger-only{ role: 'button' } + = _('Advanced') + = render Pajamas::ButtonComponent.new(button_options: { class: 'js-settings-toggle' }) do + = expanded ? _('Collapse') : _('Expand') + %p.gl-text-secondary + = _('Perform advanced options such as changing path, transferring, exporting, or removing the group.') + .settings-content + = render 'groups/settings/advanced' +- elsif can?(current_user, :remove_group, @group) + = render 'groups/settings/remove', group: @group, remove_form_id: 'js-remove-group-form' + = render_if_exists 'groups/settings/restore', group: @group + = render_if_exists 'groups/settings/immediately_remove', group: @group, remove_form_id: 'js-remove-group-form' diff --git a/db/migrate/20240219143045_add_remove_group_to_member_roles.rb b/db/migrate/20240219143045_add_remove_group_to_member_roles.rb new file mode 100644 index 00000000000000..22047afd163b96 --- /dev/null +++ b/db/migrate/20240219143045_add_remove_group_to_member_roles.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +class AddRemoveGroupToMemberRoles < Gitlab::Database::Migration[2.2] + milestone '16.10' + + enable_lock_retries! + + def up + add_column :member_roles, :remove_group, :boolean, default: false, null: false + end + + def down + remove_column :member_roles, :remove_group + end +end diff --git a/db/schema_migrations/20240219143045 b/db/schema_migrations/20240219143045 new file mode 100644 index 00000000000000..5f98c2db602700 --- /dev/null +++ b/db/schema_migrations/20240219143045 @@ -0,0 +1 @@ +12e8704b5f9d37baadb7e125d08096c7a1df93f667d64ff1f9496e59e046d0a4 \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index 241efc083256c3..a97440285cf1ff 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -4106,7 +4106,6 @@ CREATE TABLE application_settings ( lock_toggle_security_policy_custom_ci boolean DEFAULT false NOT NULL, toggle_security_policies_policy_scope boolean DEFAULT false NOT NULL, lock_toggle_security_policies_policy_scope boolean DEFAULT false NOT NULL, - include_optional_metrics_in_service_ping boolean DEFAULT true NOT NULL, rate_limits jsonb DEFAULT '{}'::jsonb NOT NULL, elasticsearch_max_code_indexing_concurrency integer DEFAULT 30 NOT NULL, enable_member_promotion_management boolean DEFAULT false NOT NULL, @@ -4120,6 +4119,7 @@ CREATE TABLE application_settings ( lock_duo_features_enabled boolean DEFAULT false NOT NULL, asciidoc_max_includes smallint DEFAULT 32 NOT NULL, clickhouse jsonb DEFAULT '{}'::jsonb NOT NULL, + include_optional_metrics_in_service_ping boolean DEFAULT true NOT NULL, CONSTRAINT app_settings_container_reg_cleanup_tags_max_list_size_positive CHECK ((container_registry_cleanup_tags_service_max_list_size >= 0)), CONSTRAINT app_settings_container_registry_pre_import_tags_rate_positive CHECK ((container_registry_pre_import_tags_rate >= (0)::numeric)), CONSTRAINT app_settings_dep_proxy_ttl_policies_worker_capacity_positive CHECK ((dependency_proxy_ttl_group_policy_worker_capacity >= 0)), @@ -10670,6 +10670,7 @@ CREATE TABLE member_roles ( remove_project boolean DEFAULT false NOT NULL, admin_terraform_state boolean DEFAULT false NOT NULL, admin_cicd_variables boolean DEFAULT false NOT NULL, + remove_group boolean DEFAULT false NOT NULL, occupies_seat boolean DEFAULT false NOT NULL, CONSTRAINT check_4364846f58 CHECK ((char_length(description) <= 255)), CONSTRAINT check_9907916995 CHECK ((char_length(name) <= 255)) diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md index df2e53bda31b0b..7c9e59d0398234 100644 --- a/doc/api/graphql/reference/index.md +++ b/doc/api/graphql/reference/index.md @@ -32100,6 +32100,7 @@ Member role permission. | `READ_CODE` | Allows read-only access to the source code. | | `READ_DEPENDENCY` | Allows read-only access to the dependencies and licenses. | | `READ_VULNERABILITY` | Read vulnerability reports and security dashboards. | +| `REMOVE_GROUP` | Ability to delete a group. This ability canndoes not allow deleting top level groups. Review the Retention period settings to prevent accidental deletion. | | `REMOVE_PROJECT` | Allows deletion of projects. | ### `MemberRolesOrderBy` diff --git a/doc/user/custom_roles/abilities.md b/doc/user/custom_roles/abilities.md index 013ca1252fe357..0779affca2089c 100644 --- a/doc/user/custom_roles/abilities.md +++ b/doc/user/custom_roles/abilities.md @@ -41,6 +41,7 @@ These requirements are documented in the `Required permission` column in the fol | Name | Required permission | Description | Introduced in | Feature flag | Enabled in | |:-----|:------------|:------------------|:---------|:--------------|:---------| | [`archive_project`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/134998) | | Allows archiving of projects. | GitLab [16.6](https://gitlab.com/gitlab-org/gitlab/-/issues/425957) | `archive_project` | GitLab [16.7](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/139260) | +| [`remove_group`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/145166) | | Ability to delete a group. This ability canndoes not allow deleting top level groups. Review the Retention period settings to prevent accidental deletion. | GitLab [16.10](https://gitlab.com/gitlab-org/gitlab/-/issues/425962) | | | | [`remove_project`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/139696) | | Allows deletion of projects. | GitLab [16.8](https://gitlab.com/gitlab-org/gitlab/-/issues/425959) | | | ## Infrastructure as code diff --git a/ee/app/assets/javascripts/groups/settings/permissions/index.js b/ee/app/assets/javascripts/groups/settings/permissions/index.js index 9a1539fe4c246f..21f2c723d123c8 100644 --- a/ee/app/assets/javascripts/groups/settings/permissions/index.js +++ b/ee/app/assets/javascripts/groups/settings/permissions/index.js @@ -83,6 +83,9 @@ const onGroupPermissionsFormSubmit = (event) => { export const initGroupPermissionsFormSubmit = () => { const groupPermissionsForm = document.querySelector('.js-general-permissions-form'); + if (!groupPermissionsForm) { + return; + } const confirmModalWrapper = document.createElement('div'); confirmModalWrapper.className = confirmModalWrapperClassName; diff --git a/ee/app/controllers/ee/groups_controller.rb b/ee/app/controllers/ee/groups_controller.rb index c94d27a9c0af54..70723df85ebfd2 100644 --- a/ee/app/controllers/ee/groups_controller.rb +++ b/ee/app/controllers/ee/groups_controller.rb @@ -12,9 +12,7 @@ module GroupsController include GeoInstrumentation include GitlabSubscriptions::SeatCountAlert - alias_method :ee_authorize_admin_group!, :authorize_admin_group! - - before_action :ee_authorize_admin_group!, only: [:restore] + before_action :authorize_remove_group!, only: [:destroy, :restore] before_action :check_subscription!, only: [:destroy] before_action do diff --git a/ee/app/policies/ee/group_policy.rb b/ee/app/policies/ee/group_policy.rb index 010ec03a4d4e73..a917cf579186fa 100644 --- a/ee/app/policies/ee/group_policy.rb +++ b/ee/app/policies/ee/group_policy.rb @@ -252,6 +252,15 @@ module GroupPolicy ).has_ability? end + desc "Custom role on group that enables removing groups" + condition(:role_enables_remove_group) do + ::Auth::MemberRoleAbilityLoader.new( + user: @user, + resource: @subject, + ability: :remove_group + ).has_ability? + end + rule { owner & unique_project_download_limit_enabled }.policy do enable :ban_group_member end @@ -561,6 +570,10 @@ module GroupPolicy enable :admin_cicd_variables end + rule { custom_roles_allowed & role_enables_remove_group & has_parent }.policy do + enable :remove_group + end + rule { can?(:read_group_security_dashboard) }.policy do enable :create_vulnerability_export enable :read_security_resource diff --git a/ee/app/services/groups/mark_for_deletion_service.rb b/ee/app/services/groups/mark_for_deletion_service.rb index e55d86d31f94b0..e311d02c820a03 100644 --- a/ee/app/services/groups/mark_for_deletion_service.rb +++ b/ee/app/services/groups/mark_for_deletion_service.rb @@ -3,7 +3,7 @@ module Groups class MarkForDeletionService < Groups::BaseService def execute - return error(_('You are not authorized to perform this action')) unless can?(current_user, :admin_group, group) + return error(_('You are not authorized to perform this action')) unless can?(current_user, :remove_group, group) return error(_('Group has been already marked for deletion')) if group.marked_for_deletion? result = create_deletion_schedule diff --git a/ee/app/services/groups/restore_service.rb b/ee/app/services/groups/restore_service.rb index 19592a6b455e48..7676f56d68ea3d 100644 --- a/ee/app/services/groups/restore_service.rb +++ b/ee/app/services/groups/restore_service.rb @@ -3,7 +3,7 @@ module Groups class RestoreService < Groups::BaseService def execute - return error(_('You are not authorized to perform this action')) unless can?(current_user, :admin_group, group) + return error(_('You are not authorized to perform this action')) unless can?(current_user, :remove_group, group) return error(_('Group has not been marked for deletion')) unless group.marked_for_deletion? result = remove_deletion_schedule diff --git a/ee/config/custom_abilities/remove_group.yml b/ee/config/custom_abilities/remove_group.yml new file mode 100644 index 00000000000000..9ec3b25a595a08 --- /dev/null +++ b/ee/config/custom_abilities/remove_group.yml @@ -0,0 +1,11 @@ +--- +name: remove_group +description: Ability to delete a group. This ability canndoes not allow deleting top level groups. Review the Retention period settings to prevent accidental deletion. +introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/425962 +introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/145166 +feature_category: groups_and_projects +milestone: "16.10" +group_ability: true +project_ability: false +requirements: [] +available_from_access_level: 50 diff --git a/ee/lib/ee/api/groups.rb b/ee/lib/ee/api/groups.rb index 57b6b2380d56b2..bfbd8388a2b49a 100644 --- a/ee/lib/ee/api/groups.rb +++ b/ee/lib/ee/api/groups.rb @@ -219,7 +219,7 @@ def delete_group(group) desc 'Restore a group.' post ':id/restore', feature_category: :groups_and_projects do - authorize! :admin_group, user_group + authorize! :remove_group, user_group break not_found! unless user_group.licensed_feature_available?(:adjourned_deletion_for_projects_and_groups) result = ::Groups::RestoreService.new(user_group, current_user).execute diff --git a/ee/lib/ee/sidebars/groups/menus/settings_menu.rb b/ee/lib/ee/sidebars/groups/menus/settings_menu.rb index 096b238c32b728..7021ce16fc6cbf 100644 --- a/ee/lib/ee/sidebars/groups/menus/settings_menu.rb +++ b/ee/lib/ee/sidebars/groups/menus/settings_menu.rb @@ -21,24 +21,20 @@ def configure_menu_items add_item(billing_menu_item) add_item(reporting_menu_item) else - if can?(context.current_user, :read_resource_access_tokens, context.group) - # Managing group acccess tokens is a custom ability independent of the access level. - add_item(access_tokens_menu_item) - end - - # Push Rules are the only group setting that can also be edited by maintainers. - # 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) + add_menu_item_for_ability(general_menu_item, :remove_group) + add_menu_item_for_ability(access_tokens_menu_item, :read_resource_access_tokens) + add_menu_item_for_ability(repository_menu_item, :change_push_rules) + add_menu_item_for_ability(ci_cd_menu_item, :admin_cicd_variables) + add_menu_item_for_ability(billing_menu_item, :read_billing) end end private + def add_menu_item_for_ability(menu_item, ability) + add_item(menu_item) if can?(context.current_user, ability, context.group) + end + def roles_and_permissions_menu_item return ::Sidebars::NilMenuItem.new(item_id: :roles_and_permissions) unless custom_roles_enabled? 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 9f543d941dfa75..26beea708ba689 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 @@ -316,5 +316,27 @@ end end end + + context 'when the user is not an owner but has `remove_group` 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, :remove_group, group).and_return(true) + end + + describe 'General menu item' do + let(:item_id) { :general } + + 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/models/ee/user_spec.rb b/ee/spec/models/ee/user_spec.rb index 242c2277c670bc..b92844e5ef0671 100644 --- a/ee/spec/models/ee/user_spec.rb +++ b/ee/spec/models/ee/user_spec.rb @@ -1272,6 +1272,7 @@ OR manage_project_access_tokens = true OR read_dependency = true OR read_vulnerability = true + OR remove_group = true OR remove_project = true\)\)\)\)'.squish # allow_cross_joins_across_databases end diff --git a/ee/spec/policies/group_policy_spec.rb b/ee/spec/policies/group_policy_spec.rb index a13ed1bfc2b2a8..cf280393cfa42c 100644 --- a/ee/spec/policies/group_policy_spec.rb +++ b/ee/spec/policies/group_policy_spec.rb @@ -3284,6 +3284,24 @@ def create_member_role(member, abilities = member_role_abilities) it_behaves_like 'custom roles abilities' end + + context 'for a custom role with the `remove_group` ability' do + let(:member_role_abilities) { { remove_group: true } } + let(:allowed_abilities) { [:remove_group, :view_edit_page] } + + it_behaves_like 'custom roles abilities' + + context 'when the group is a top level group' do + before do + create_member_role(parent_group_member_guest) + stub_licensed_features(custom_roles: true) + end + + subject { described_class.new(current_user, parent_group) } + + it { is_expected.to be_disallowed(*allowed_abilities) } + end + end end context 'for :read_limit_alert' do diff --git a/ee/spec/requests/api/member_roles_spec.rb b/ee/spec/requests/api/member_roles_spec.rb index b93047ff302dd7..b556783444a455 100644 --- a/ee/spec/requests/api/member_roles_spec.rb +++ b/ee/spec/requests/api/member_roles_spec.rb @@ -109,6 +109,7 @@ "manage_group_access_tokens" => false, "manage_project_access_tokens" => false, "archive_project" => false, + "remove_group" => false, "remove_project" => false, "admin_cicd_variables" => false, "group_id" => group_id @@ -128,6 +129,7 @@ "manage_group_access_tokens" => false, "manage_project_access_tokens" => false, "archive_project" => false, + "remove_group" => false, "remove_project" => false, "admin_cicd_variables" => false, "group_id" => group_id diff --git a/ee/spec/requests/custom_roles/remove_group/request_spec.rb b/ee/spec/requests/custom_roles/remove_group/request_spec.rb new file mode 100644 index 00000000000000..f42e9878e2fa34 --- /dev/null +++ b/ee/spec/requests/custom_roles/remove_group/request_spec.rb @@ -0,0 +1,78 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe 'User with the `remove_group` custom ability', feature_category: :groups_and_projects do + let_it_be(:user) { create(:user) } + let_it_be(:group) { create(:group) } + let_it_be(:role) { create(:member_role, :guest, namespace: group, remove_group: true) } + let_it_be(:member) { create(:group_member, :guest, member_role: role, user: user, group: group) } + let_it_be(:subgroup) { create(:group, parent: group) } + + before do + stub_licensed_features(custom_roles: true, adjourned_deletion_for_projects_and_groups: true) + end + + describe GroupsController do + before do + sign_in(user) + end + + describe '#edit' do + it 'user can view the edit page of the subgroup' do + get edit_group_path(subgroup) + + expect(response).to have_gitlab_http_status(:ok) + expect(response.body).to include(_('Remove group')) + end + end + + describe '#destroy' do + it 'user can delete the subgroup' do + delete group_path(subgroup) + + expect(response).to have_gitlab_http_status(:found) + expect(subgroup.reload).to be_marked_for_deletion + end + end + + describe '#restore' do + before do + create(:group_deletion_schedule, group: subgroup) + end + + it 'user can restore the subgroup' do + post group_restore_path(subgroup) + + expect(response).to have_gitlab_http_status(:found) + expect(subgroup.reload).not_to be_marked_for_deletion + end + end + end + + describe API::Groups do + include ApiHelpers + + describe '#delete' do + it 'user can delete the subgroup' do + delete api("/groups/#{subgroup.id}", user) + + expect(response).to have_gitlab_http_status(:accepted) + expect(subgroup.reload).to be_marked_for_deletion + end + end + + describe '#restore' do + before do + create(:group_deletion_schedule, group: subgroup) + end + + it 'user can restore the subgroup' do + post api("/groups/#{subgroup.id}/restore", user) + + expect(response).to have_gitlab_http_status(:created) + expect(subgroup.reload).not_to be_marked_for_deletion + end + end + end +end diff --git a/ee/spec/support/shared_examples/views/registration_features_prompt_settings_link_shared_examples.rb b/ee/spec/support/shared_examples/views/registration_features_prompt_settings_link_shared_examples.rb index 230ff310bf6851..448ba6435dffb6 100644 --- a/ee/spec/support/shared_examples/views/registration_features_prompt_settings_link_shared_examples.rb +++ b/ee/spec/support/shared_examples/views/registration_features_prompt_settings_link_shared_examples.rb @@ -7,6 +7,7 @@ context 'as regular user' do before do allow(view).to receive(:current_user) { user } + allow(view).to receive(:can?).with(user, :admin_group, anything).and_return(false) end it 'does not render settings link', :aggregate_failures do @@ -17,6 +18,7 @@ context 'as admin' do before do allow(view).to receive(:current_user) { admin } + allow(view).to receive(:can?).with(admin, :admin_group, anything).and_return(true) end it 'renders settings link', :aggregate_failures do diff --git a/ee/spec/views/groups/edit.html.haml_spec.rb b/ee/spec/views/groups/edit.html.haml_spec.rb index 4b77219e83c732..d74f5cb80df4ac 100644 --- a/ee/spec/views/groups/edit.html.haml_spec.rb +++ b/ee/spec/views/groups/edit.html.haml_spec.rb @@ -170,4 +170,22 @@ it_behaves_like 'does not render allowed_email_domain setting' end end + + context 'when rendering for a user that is not an owner', feature_category: :permissions do + before do + group.add_guest(user) + end + + subject { render } + + it { is_expected.not_to have_text(_('Remove group')) } + + context 'when the user can remove groups' do + before do + allow(view).to receive(:can?).with(user, :remove_group, group).and_return(true) + end + + it { is_expected.to have_text(_('Remove group')) } + end + end end diff --git a/lib/api/groups.rb b/lib/api/groups.rb index 7b755a76f29c3a..0132ab9c2829f4 100644 --- a/lib/api/groups.rb +++ b/lib/api/groups.rb @@ -291,7 +291,7 @@ def check_subscription!(group) end delete ":id", feature_category: :groups_and_projects, urgency: :low do group = find_group!(params[:id]) - authorize! :admin_group, group + authorize! :remove_group, group check_subscription! group delete_group(group) diff --git a/spec/support/shared_contexts/policies/group_policy_shared_context.rb b/spec/support/shared_contexts/policies/group_policy_shared_context.rb index 509bc413b8d4c1..565de038f51c24 100644 --- a/spec/support/shared_contexts/policies/group_policy_shared_context.rb +++ b/spec/support/shared_contexts/policies/group_policy_shared_context.rb @@ -80,6 +80,8 @@ destroy_issue admin_member_access_request update_git_access_protocol + remove_group + view_edit_page ] end -- GitLab From 8eaac943cbbb2f0c2f66747fc650b7b844325370 Mon Sep 17 00:00:00 2001 From: Alex Buijs Date: Fri, 23 Feb 2024 11:55:19 +0100 Subject: [PATCH 2/2] Fix typo in yaml file and docs --- doc/api/graphql/reference/index.md | 2 +- doc/user/custom_roles/abilities.md | 2 +- ee/config/custom_abilities/remove_group.yml | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md index 7c9e59d0398234..67967940b64e35 100644 --- a/doc/api/graphql/reference/index.md +++ b/doc/api/graphql/reference/index.md @@ -32100,7 +32100,7 @@ Member role permission. | `READ_CODE` | Allows read-only access to the source code. | | `READ_DEPENDENCY` | Allows read-only access to the dependencies and licenses. | | `READ_VULNERABILITY` | Read vulnerability reports and security dashboards. | -| `REMOVE_GROUP` | Ability to delete a group. This ability canndoes not allow deleting top level groups. Review the Retention period settings to prevent accidental deletion. | +| `REMOVE_GROUP` | Ability to delete or restore a group. This ability does not allow deleting top level groups. Review the Retention period settings to prevent accidental deletion. | | `REMOVE_PROJECT` | Allows deletion of projects. | ### `MemberRolesOrderBy` diff --git a/doc/user/custom_roles/abilities.md b/doc/user/custom_roles/abilities.md index 0779affca2089c..38698cefaf55af 100644 --- a/doc/user/custom_roles/abilities.md +++ b/doc/user/custom_roles/abilities.md @@ -41,7 +41,7 @@ These requirements are documented in the `Required permission` column in the fol | Name | Required permission | Description | Introduced in | Feature flag | Enabled in | |:-----|:------------|:------------------|:---------|:--------------|:---------| | [`archive_project`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/134998) | | Allows archiving of projects. | GitLab [16.6](https://gitlab.com/gitlab-org/gitlab/-/issues/425957) | `archive_project` | GitLab [16.7](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/139260) | -| [`remove_group`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/145166) | | Ability to delete a group. This ability canndoes not allow deleting top level groups. Review the Retention period settings to prevent accidental deletion. | GitLab [16.10](https://gitlab.com/gitlab-org/gitlab/-/issues/425962) | | | +| [`remove_group`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/145166) | | Ability to delete or restore a group. This ability does not allow deleting top level groups. Review the Retention period settings to prevent accidental deletion. | GitLab [16.10](https://gitlab.com/gitlab-org/gitlab/-/issues/425962) | | | | [`remove_project`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/139696) | | Allows deletion of projects. | GitLab [16.8](https://gitlab.com/gitlab-org/gitlab/-/issues/425959) | | | ## Infrastructure as code diff --git a/ee/config/custom_abilities/remove_group.yml b/ee/config/custom_abilities/remove_group.yml index 9ec3b25a595a08..e3f2c33b13232b 100644 --- a/ee/config/custom_abilities/remove_group.yml +++ b/ee/config/custom_abilities/remove_group.yml @@ -1,6 +1,6 @@ --- name: remove_group -description: Ability to delete a group. This ability canndoes not allow deleting top level groups. Review the Retention period settings to prevent accidental deletion. +description: Ability to delete or restore a group. This ability does not allow deleting top level groups. Review the Retention period settings to prevent accidental deletion. introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/425962 introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/145166 feature_category: groups_and_projects -- GitLab