From 9b8cdda22b6789a8b341f01e59788863d2ae59b2 Mon Sep 17 00:00:00 2001 From: Alex Buijs Date: Wed, 13 Dec 2023 18:26:58 +0100 Subject: [PATCH 1/3] Add Delete Project custom permission This commit adds "Delete Project" as a customizable permission, so that it can be added onto any base role. Changelog: added EE: true --- .../projects/shared/permissions/index.js | 3 + .../pages/shared/mount_badge_settings.js | 2 + app/views/projects/edit.html.haml | 7 ++- ...0159_add_delete_project_to_member_roles.rb | 10 +++ db/schema_migrations/20231213170159 | 1 + db/structure.sql | 1 + doc/api/graphql/reference/index.md | 3 + doc/api/member_roles.md | 8 ++- doc/user/custom_roles.md | 4 +- ee/app/controllers/ee/projects_controller.rb | 2 +- .../graphql/mutations/member_roles/create.rb | 4 ++ .../types/member_roles/member_role_type.rb | 6 ++ ee/app/models/members/member_role.rb | 4 +- ee/app/policies/ee/project_policy.rb | 13 ++++ ee/config/custom_abilities/delete_project.yml | 10 +++ .../sidebars/projects/menus/settings_menu.rb | 3 +- .../controllers/projects_controller_spec.rb | 20 +++++- .../delete_project_custom_permission_spec.rb | 61 +++++++++++++++++++ .../lib/ee/api/entities/member_role_spec.rb | 1 + .../projects/menus/settings_menu_spec.rb | 34 +++++++++-- ee/spec/models/ee/user_spec.rb | 1 + ee/spec/policies/project_policy_spec.rb | 7 +++ ee/spec/requests/api/member_roles_spec.rb | 2 + ee/spec/views/projects/edit.html.haml_spec.rb | 13 +++- 24 files changed, 202 insertions(+), 18 deletions(-) create mode 100644 db/migrate/20231213170159_add_delete_project_to_member_roles.rb create mode 100644 db/schema_migrations/20231213170159 create mode 100644 ee/config/custom_abilities/delete_project.yml create mode 100644 ee/spec/features/projects/custom_roles/delete_project_custom_permission_spec.rb diff --git a/app/assets/javascripts/pages/projects/shared/permissions/index.js b/app/assets/javascripts/pages/projects/shared/permissions/index.js index 4b4589dc46c0e7..78dd456aad93b9 100644 --- a/app/assets/javascripts/pages/projects/shared/permissions/index.js +++ b/app/assets/javascripts/pages/projects/shared/permissions/index.js @@ -14,6 +14,9 @@ export default function initProjectPermissionsSettings() { const mountPoint = document.querySelector('.js-project-permissions-form'); const componentPropsEl = document.querySelector('.js-project-permissions-form-data'); + + if (!mountPoint) return null; + const componentProps = JSON.parse(componentPropsEl.innerHTML); const { diff --git a/app/assets/javascripts/pages/shared/mount_badge_settings.js b/app/assets/javascripts/pages/shared/mount_badge_settings.js index aeb9f2fb8d371e..9c92d4f8f1e709 100644 --- a/app/assets/javascripts/pages/shared/mount_badge_settings.js +++ b/app/assets/javascripts/pages/shared/mount_badge_settings.js @@ -5,6 +5,8 @@ import store from '~/badges/store'; export default (kind) => { const badgeSettingsElement = document.getElementById('badge-settings'); + if (!badgeSettingsElement) return null; + store.dispatch('loadBadges', { kind, apiEndpointUrl: badgeSettingsElement.dataset.apiEndpointUrl, diff --git a/app/views/projects/edit.html.haml b/app/views/projects/edit.html.haml index 9269369c83ebbc..90837a1a2918f5 100644 --- a/app/views/projects/edit.html.haml +++ b/app/views/projects/edit.html.haml @@ -118,8 +118,11 @@ = render 'remove_fork', project: @project = render 'remove', project: @project -- elsif can?(current_user, :archive_project, @project) - = render_if_exists 'projects/settings/archive' +- else + - if can?(current_user, :archive_project, @project) + = render_if_exists 'projects/settings/archive' + - if can?(current_user, :remove_project, @project) + = render 'remove', project: @project .save-project-loader.hide .center diff --git a/db/migrate/20231213170159_add_delete_project_to_member_roles.rb b/db/migrate/20231213170159_add_delete_project_to_member_roles.rb new file mode 100644 index 00000000000000..bb9fee0f1be7e1 --- /dev/null +++ b/db/migrate/20231213170159_add_delete_project_to_member_roles.rb @@ -0,0 +1,10 @@ +# frozen_string_literal: true + +class AddDeleteProjectToMemberRoles < Gitlab::Database::Migration[2.2] + milestone '16.8' + enable_lock_retries! + + def change + add_column :member_roles, :delete_project, :boolean, default: false, null: false + end +end diff --git a/db/schema_migrations/20231213170159 b/db/schema_migrations/20231213170159 new file mode 100644 index 00000000000000..ff5e6e29a3c797 --- /dev/null +++ b/db/schema_migrations/20231213170159 @@ -0,0 +1 @@ +f73fde4e3e54fa88d8dba9ec3a98b7dfb8332aaf7a76de73baf899292ed751b1 \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index 85854dc30178ec..1aa547766c2390 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -18657,6 +18657,7 @@ CREATE TABLE member_roles ( admin_group_member boolean DEFAULT false NOT NULL, manage_project_access_tokens boolean DEFAULT false NOT NULL, archive_project boolean DEFAULT false NOT NULL, + delete_project 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 fbba2f2474b097..13f1dc09e241f9 100644 --- a/doc/api/graphql/reference/index.md +++ b/doc/api/graphql/reference/index.md @@ -5352,6 +5352,7 @@ Input type: `MemberRoleCreateInput` | `archiveProject` | [`Boolean`](#boolean) | Permission to archive projects. | | `baseAccessLevel` | [`MemberAccessLevel!`](#memberaccesslevel) | Base access level for the custom role. | | `clientMutationId` | [`String`](#string) | A unique identifier for the client performing the mutation. | +| `deleteProject` | [`Boolean`](#boolean) | Permission to delete projects. | | `description` | [`String`](#string) | Description of the member role. | | `groupPath` | [`ID!`](#id) | Group the member role to mutate is in. | | `manageProjectAccessTokens` | [`Boolean`](#boolean) | Permission to admin project access tokens. | @@ -21284,6 +21285,7 @@ Represents a member role. | `adminVulnerability` **{warning-solid}** | [`Boolean`](#boolean) | **Introduced** in 16.5. This feature is an Experiment. It can be changed or removed at any time. Permission to admin vulnerability. | | `archiveProject` **{warning-solid}** | [`Boolean`](#boolean) | **Introduced** in 16.6. This feature is an Experiment. It can be changed or removed at any time. Permission to archive projects. | | `baseAccessLevel` **{warning-solid}** | [`AccessLevel!`](#accesslevel) | **Introduced** in 16.5. This feature is an Experiment. It can be changed or removed at any time. Base access level for the custom role. | +| `deleteProject` **{warning-solid}** | [`Boolean`](#boolean) | **Introduced** in 16.8. This feature is an Experiment. It can be changed or removed at any time. Permission to delete projects. | | `description` | [`String`](#string) | Description of the member role. | | `enabledPermissions` **{warning-solid}** | [`[MemberRolePermission!]`](#memberrolepermission) | **Introduced** in 16.5. This feature is an Experiment. It can be changed or removed at any time. Array of all permissions enabled for the custom role. | | `id` | [`MemberRoleID!`](#memberroleid) | ID of the member role. | @@ -30606,6 +30608,7 @@ Member role permission. | `ADMIN_MERGE_REQUEST` | Allows to approve merge requests. | | `ADMIN_VULNERABILITY` | Allows admin access to the vulnerability reports. | | `ARCHIVE_PROJECT` | Allows to archive projects. | +| `DELETE_PROJECT` | Allows to delete projects. | | `MANAGE_PROJECT_ACCESS_TOKENS` | Allows manage access to the project access tokens. | | `READ_CODE` | Allows read-only access to the source code. | | `READ_DEPENDENCY` | Allows read-only access to the dependencies. | diff --git a/doc/api/member_roles.md b/doc/api/member_roles.md index 24ac70990047d5..2ce8ed4f852db5 100644 --- a/doc/api/member_roles.md +++ b/doc/api/member_roles.md @@ -17,6 +17,7 @@ info: To determine the technical writer assigned to the Stage/Group associated w > - [Admin group members introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/131914) in GitLab 16.5 [with a flag](../administration/feature_flags.md) named `admin_group_member`. Disabled by default. The feature flag has been removed in GitLab 16.6. > - [Manage project access tokens introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/132342) in GitLab 16.5 in [with a flag](../administration/feature_flags.md) named `manage_project_access_tokens`. Disabled by default. > - [Archive project introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/134998) in GitLab 16.7. +> - [Delete project introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/139696) in GitLab 16.8. FLAG: On self-managed GitLab, by default these features are not available. To make them available, an administrator can [enable the feature flags](../administration/feature_flags.md) named `admin_group_member` and `manage_project_access_tokens`. @@ -51,6 +52,7 @@ If successful, returns [`200`](rest/index.md#status-codes) and the following res | `[].admin_group_member` | boolean | Permission to admin members of a group. | | `[].manage_project_access_tokens` | boolean | Permission to manage project access tokens. | | `[].archive_project` | boolean | Permission to archive projects. | +| `[].delete_project` | boolean | Permission to delete projects. | Example request: @@ -74,7 +76,8 @@ Example response: "read_dependency": false, "read_vulnerability": false, "manage_project_access_tokens": false, - "archive_project": false + "archive_project": false, + "delete_project": false }, { "id": 3, @@ -88,7 +91,8 @@ Example response: "read_dependency": true, "read_vulnerability": true, "manage_project_access_tokens": false, - "archive_project": false + "archive_project": false, + "delete_project": false } ] ``` diff --git a/doc/user/custom_roles.md b/doc/user/custom_roles.md index 1f3628efa39105..b1c26ca364c1cf 100644 --- a/doc/user/custom_roles.md +++ b/doc/user/custom_roles.md @@ -17,6 +17,7 @@ info: To determine the technical writer assigned to the Stage/Group associated w > - Ability to manage project access tokens [introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/421778) in GitLab 16.5 [with a flag](../administration/feature_flags.md) named `manage_project_access_tokens`. > - Ability to archive projects [introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/425957) in GitLab 16.7. > - Ability to use the UI to add a user to your group with a custom role, change a user's custom role, or remove a custom role from a group member [introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/393239) in GitLab 16.7. +> - Ability to delete projects [introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/425959) in GitLab 16.8. Custom roles allow group Owners or instance administrators to create roles specific to the needs of their organization. @@ -114,7 +115,8 @@ These requirements are documented in the `Required permission` column in the fol | `admin_merge_request` | GitLab 16.4 and later | Not applicable | View and approve [merge requests](project/merge_requests/index.md), revoke merge request approval, and view the associated merge request code.
Does not allow users to view or change merge request approval rules. | | `manage_project_access_tokens` | GitLab 16.5 and later | Not applicable | Create, delete, and list [project access tokens](project/settings/project_access_tokens.md). | | `admin_group_member` | GitLab 16.5 and later | Not applicable | Add or remove [group members](group/manage.md). | -| `archive_project` | GitLab 16.6 and later | Not applicable | Archive and unarchive [projects](project/settings/migrate_projects.md#archive-a-project). | +| `archive_project` | GitLab 16.7 and later | Not applicable | Archive and unarchive [projects](project/settings/migrate_projects.md#archive-a-project). | +| `delete_project` | GitLab 16.8 and later | Not applicable | Delete [projects](project/working_with_projects.md#delete-a-project). | ## Billing and seat usage diff --git a/ee/app/controllers/ee/projects_controller.rb b/ee/app/controllers/ee/projects_controller.rb index ae9f40931233be..bd849060c0250b 100644 --- a/ee/app/controllers/ee/projects_controller.rb +++ b/ee/app/controllers/ee/projects_controller.rb @@ -222,7 +222,7 @@ def log_unarchive_audit_event end def authorize_admin_project_or_custom_permissions! - can?(current_user, :archive_project, project) || super + can?(current_user, :archive_project, project) || can?(current_user, :remove_project, project) || super end end end diff --git a/ee/app/graphql/mutations/member_roles/create.rb b/ee/app/graphql/mutations/member_roles/create.rb index 455ae3faac8eb0..947a4ee7947225 100644 --- a/ee/app/graphql/mutations/member_roles/create.rb +++ b/ee/app/graphql/mutations/member_roles/create.rb @@ -27,6 +27,10 @@ class Create < Base ::Types::MemberAccessLevelEnum, required: true, description: 'Base access level for the custom role.' + argument :delete_project, + GraphQL::Types::Boolean, + required: false, + description: 'Permission to delete projects.' argument :group_path, GraphQL::Types::ID, required: true, description: 'Group the member role to mutate is in.' diff --git a/ee/app/graphql/types/member_roles/member_role_type.rb b/ee/app/graphql/types/member_roles/member_role_type.rb index 1128c0b6b8f65c..d3b924a03ee045 100644 --- a/ee/app/graphql/types/member_roles/member_role_type.rb +++ b/ee/app/graphql/types/member_roles/member_role_type.rb @@ -47,6 +47,12 @@ class MemberRoleType < BaseObject alpha: { milestone: '16.6' }, description: 'Permission to archive projects.' + field :delete_project, + GraphQL::Types::Boolean, + null: true, + alpha: { milestone: '16.8' }, + description: 'Permission to delete projects.' + field :manage_project_access_tokens, GraphQL::Types::Boolean, null: true, diff --git a/ee/app/models/members/member_role.rb b/ee/app/models/members/member_role.rb index 2d1726b12f90c8..8d3cdee5fccd34 100644 --- a/ee/app/models/members/member_role.rb +++ b/ee/app/models/members/member_role.rb @@ -119,8 +119,8 @@ def validate_requirements requirement = params[:requirement] next unless self[permission] # skipping permissions not set for the object - next unless requirement # skipping permissions that have no requirement - next if self[requirement] # the requierement is met + next unless requirement.present? # skipping permissions that have no requirement + next if self[requirement] # the requirement is met errors.add(permission, format(s_("MemberRole|%{requirement} has to be enabled in order to enable %{permission}."), diff --git a/ee/app/policies/ee/project_policy.rb b/ee/app/policies/ee/project_policy.rb index 4a92bb63f54555..a455977529694c 100644 --- a/ee/app/policies/ee/project_policy.rb +++ b/ee/app/policies/ee/project_policy.rb @@ -655,6 +655,19 @@ module ProjectPolicy enable :archive_project end + desc "Custom role on project that enables deleting projects" + condition(:custom_role_enables_delete_projects) do + ::Auth::MemberRoleAbilityLoader.new( + user: @user, + resource: @subject, + ability: :delete_project + ).has_ability? + end + + rule { custom_roles_allowed & custom_role_enables_delete_projects }.policy do + enable :remove_project + end + rule { needs_new_sso_session }.policy do prevent :read_project end diff --git a/ee/config/custom_abilities/delete_project.yml b/ee/config/custom_abilities/delete_project.yml new file mode 100644 index 00000000000000..5af9e197f9c322 --- /dev/null +++ b/ee/config/custom_abilities/delete_project.yml @@ -0,0 +1,10 @@ +--- +name: delete_project +description: Allows to delete projects. +introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/425959 +introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/139696 +feature_category: groups_and_projects +milestone: '16.8' +group_ability: false +project_ability: true +requirement: '' diff --git a/ee/lib/ee/sidebars/projects/menus/settings_menu.rb b/ee/lib/ee/sidebars/projects/menus/settings_menu.rb index 56cc28e7d1faaf..781077ddc15cf1 100644 --- a/ee/lib/ee/sidebars/projects/menus/settings_menu.rb +++ b/ee/lib/ee/sidebars/projects/menus/settings_menu.rb @@ -49,7 +49,8 @@ def custom_roles_menu_items end def custom_roles_general_menu_item? - can?(context.current_user, :archive_project, context.project) + can?(context.current_user, :archive_project, context.project) || + can?(context.current_user, :remove_project, context.project) end def custom_roles_access_token_menu_item? diff --git a/ee/spec/controllers/projects_controller_spec.rb b/ee/spec/controllers/projects_controller_spec.rb index 463380dcfa5b8c..b3508e044e5fd9 100644 --- a/ee/spec/controllers/projects_controller_spec.rb +++ b/ee/spec/controllers/projects_controller_spec.rb @@ -169,7 +169,7 @@ end end - describe 'GET edit', feature_category: :groups_and_projects do + describe 'GET edit', feature_category: :permissions do subject(:request) { get :edit, params: { namespace_id: project.namespace.path, id: project.path } } it 'does not allow an auditor user to access the page' do @@ -197,6 +197,24 @@ expect(response).to render_template(:edit) end end + + context 'when the user can remove projects' do + let_it_be(:guest) { create(:user) } + + before do + project.add_guest(guest) + allow(controller).to receive(:can?).and_call_original + allow(controller).to receive(:can?).with(guest, :remove_project, anything).and_return(true) + + sign_in(guest) + request + end + + it 'allows the user to access the page' do + expect(response).to have_gitlab_http_status(:ok) + expect(response).to render_template(:edit) + end + end end describe 'POST create', feature_category: :groups_and_projects do diff --git a/ee/spec/features/projects/custom_roles/delete_project_custom_permission_spec.rb b/ee/spec/features/projects/custom_roles/delete_project_custom_permission_spec.rb new file mode 100644 index 00000000000000..432f881220f638 --- /dev/null +++ b/ee/spec/features/projects/custom_roles/delete_project_custom_permission_spec.rb @@ -0,0 +1,61 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe 'Delete Project Custom Permission', feature_category: :permissions do + let_it_be(:project) { create(:project, :in_group) } + let_it_be(:user) { create(:user) } + let_it_be(:member_role) { create(:member_role, :guest, namespace: project.root_namespace, delete_project: true) } + + before do + stub_licensed_features(custom_roles: true) + create(:project_member, :guest, member_role: member_role, user: user, project: project) + sign_in(user) + end + + it 'allows a guest user with the custom `delete_project` ability to delete it', :js, :aggregate_failures do + visit project_path(project) + + within_testid('super-sidebar') do + click_button('Settings') + click_link('General') + end + + click_button(_('Delete project')) + fill_in 'confirm_name_input', with: project.path_with_namespace + click_button(_('Yes, delete project')) + + expect(page).to have_content("Project '#{project.full_name}' is in the process of being deleted.") + expect(project.reload).to be_pending_delete + end + + shared_examples 'does not allow the user to delete the project' do + it 'does not show the `Settings` sidebar item', :js do + visit project_path(project) + + within_testid('super-sidebar') do + expect(page).not_to have_button('Settings') + end + end + + it 'does not allow access to the edit page' do + visit edit_project_path(project) + + expect(page).to have_gitlab_http_status(:not_found) + end + end + + context 'when the `custom_roles` licensed feature is not available' do + before do + stub_licensed_features(custom_roles: false) + end + + it_behaves_like 'does not allow the user to delete the project' + end + + context 'when the user does not have the custom role' do + let_it_be(:member_role) { nil } + + it_behaves_like 'does not allow the user to delete the project' + end +end diff --git a/ee/spec/lib/ee/api/entities/member_role_spec.rb b/ee/spec/lib/ee/api/entities/member_role_spec.rb index 83ee63da11f01a..b0ff59628c8e8a 100644 --- a/ee/spec/lib/ee/api/entities/member_role_spec.rb +++ b/ee/spec/lib/ee/api/entities/member_role_spec.rb @@ -22,6 +22,7 @@ expect(subject[:admin_vulnerability]).to eq member_role.admin_vulnerability expect(subject[:manage_project_access_tokens]).to eq member_role.manage_project_access_tokens expect(subject[:archive_project]).to eq member_role.archive_project + expect(subject[:delete_project]).to eq member_role.delete_project expect(subject[:group_id]).to eq(member_role.namespace.id) end end diff --git a/ee/spec/lib/ee/sidebars/projects/menus/settings_menu_spec.rb b/ee/spec/lib/ee/sidebars/projects/menus/settings_menu_spec.rb index e439bc7ea8290f..a21d5ae76f1384 100644 --- a/ee/spec/lib/ee/sidebars/projects/menus/settings_menu_spec.rb +++ b/ee/spec/lib/ee/sidebars/projects/menus/settings_menu_spec.rb @@ -41,15 +41,39 @@ describe 'General' do let(:item_id) { :general } - describe 'when the user is not an admin but has archive_project custom permission' do + describe 'when the user is not an admin' do + let_it_be(:user) { create(:user) } + + before_all do + project.add_guest(user) + end + before do allow(Ability).to receive(:allowed?).and_call_original - allow(Ability).to receive(:allowed?).with(user, :admin_project, project).and_return(false) - allow(Ability).to receive(:allowed?).with(user, :archive_project, project).and_return(true) end - it 'includes general menu item' do - expect(subject.title).to eql('General') + it 'does not include the general menu item' do + expect(subject).to be_nil + end + + context 'when the user has the `archive_project` ability' do + before do + allow(Ability).to receive(:allowed?).with(user, :archive_project, project).and_return(true) + end + + it 'includes the general menu item' do + expect(subject.title).to eql('General') + end + end + + context 'when the user has the `remove_project` ability' do + before do + allow(Ability).to receive(:allowed?).with(user, :remove_project, project).and_return(true) + end + + it 'includes the general menu item' do + expect(subject.title).to eql('General') + end end end end diff --git a/ee/spec/models/ee/user_spec.rb b/ee/spec/models/ee/user_spec.rb index 54e8c85d713fcb..402d0e078dbc26 100644 --- a/ee/spec/models/ee/user_spec.rb +++ b/ee/spec/models/ee/user_spec.rb @@ -1224,6 +1224,7 @@ OR admin_merge_request = true OR admin_vulnerability = true OR archive_project = true + OR delete_project = true OR manage_project_access_tokens = true OR read_dependency = true OR read_vulnerability = true\)\)\)\)'.squish # allow_cross_joins_across_databases diff --git a/ee/spec/policies/project_policy_spec.rb b/ee/spec/policies/project_policy_spec.rb index 73f94bddd14685..ab083129345101 100644 --- a/ee/spec/policies/project_policy_spec.rb +++ b/ee/spec/policies/project_policy_spec.rb @@ -2784,6 +2784,13 @@ def create_member_role(member, abilities = member_role_abilities) it_behaves_like 'custom roles abilities' end + + context 'for a member role with `delete_project` true' do + let(:member_role_abilities) { { delete_project: true } } + let(:allowed_abilities) { [:remove_project] } + + it_behaves_like 'custom roles abilities' + end end describe 'permissions for suggested reviewers bot', :saas do diff --git a/ee/spec/requests/api/member_roles_spec.rb b/ee/spec/requests/api/member_roles_spec.rb index c4b653fa73218b..e3d64d0a944fc0 100644 --- a/ee/spec/requests/api/member_roles_spec.rb +++ b/ee/spec/requests/api/member_roles_spec.rb @@ -107,6 +107,7 @@ "admin_vulnerability" => false, "manage_project_access_tokens" => false, "archive_project" => false, + "delete_project" => false, "group_id" => group_id }, { @@ -122,6 +123,7 @@ "admin_vulnerability" => false, "manage_project_access_tokens" => false, "archive_project" => false, + "delete_project" => false, "group_id" => group_id } ] diff --git a/ee/spec/views/projects/edit.html.haml_spec.rb b/ee/spec/views/projects/edit.html.haml_spec.rb index 1ff41577815c54..14df5aa749c106 100644 --- a/ee/spec/views/projects/edit.html.haml_spec.rb +++ b/ee/spec/views/projects/edit.html.haml_spec.rb @@ -42,23 +42,30 @@ context 'when rendering for a user that is not an owner' do let_it_be(:user) { create(:user) } + let(:can_archive_projects) { false } + let(:can_remove_projects) { false } + before do allow(view).to receive(:can?).with(user, :archive_project, project).and_return(can_archive_projects) + allow(view).to receive(:can?).with(user, :remove_project, project).and_return(can_remove_projects) render end subject { rendered } + it { is_expected.not_to have_link(_('Archive project')) } + it { is_expected.not_to have_text(_('Delete this project')) } + context 'when the user can archive projects' do let(:can_archive_projects) { true } it { is_expected.to have_link(_('Archive project')) } end - context 'when the user cannot archive projects' do - let(:can_archive_projects) { false } + context 'when the user can remove projects' do + let(:can_remove_projects) { true } - it { is_expected.not_to have_link(_('Archive project')) } + it { is_expected.to have_text(_('Delete this project')) } end end end -- GitLab From 0d2b15b6a882431c93d9e21d5c5eb91053a092a6 Mon Sep 17 00:00:00 2001 From: Alex Buijs Date: Fri, 15 Dec 2023 14:55:34 +0100 Subject: [PATCH 2/3] Implement backend maintainer feedback --- app/controllers/projects_controller.rb | 7 +- app/policies/project_policy.rb | 1 + ...0159_add_delete_project_to_member_roles.rb | 10 --- ...0159_add_remove_project_to_member_roles.rb | 10 +++ db/structure.sql | 2 +- doc/api/graphql/reference/index.md | 6 +- doc/api/member_roles.md | 6 +- doc/user/custom_roles.md | 2 +- ee/app/controllers/ee/projects_controller.rb | 4 - .../graphql/mutations/member_roles/create.rb | 8 +- .../types/member_roles/member_role_type.rb | 4 +- ee/app/policies/ee/project_policy.rb | 10 ++- ...{delete_project.yml => remove_project.yml} | 4 +- .../sidebars/projects/menus/settings_menu.rb | 3 +- .../controllers/projects_controller_spec.rb | 48 ----------- .../archive_project_custom_permission_spec.rb | 81 ------------------- .../delete_project_custom_permission_spec.rb | 61 -------------- .../lib/ee/api/entities/member_role_spec.rb | 4 +- .../projects/menus/settings_menu_spec.rb | 14 +--- ee/spec/models/ee/user_spec.rb | 4 +- ee/spec/policies/project_policy_spec.rb | 8 +- ee/spec/requests/api/member_roles_spec.rb | 4 +- .../archive_project/request_spec.rb | 46 +++++++++++ .../delete_project/request_spec.rb | 37 +++++++++ ee/spec/views/projects/edit.html.haml_spec.rb | 2 +- spec/policies/project_policy_spec.rb | 10 +-- 26 files changed, 137 insertions(+), 259 deletions(-) delete mode 100644 db/migrate/20231213170159_add_delete_project_to_member_roles.rb create mode 100644 db/migrate/20231213170159_add_remove_project_to_member_roles.rb rename ee/config/custom_abilities/{delete_project.yml => remove_project.yml} (81%) delete mode 100644 ee/spec/features/projects/custom_roles/archive_project_custom_permission_spec.rb delete mode 100644 ee/spec/features/projects/custom_roles/delete_project_custom_permission_spec.rb create mode 100644 ee/spec/requests/custom_roles/archive_project/request_spec.rb create mode 100644 ee/spec/requests/custom_roles/delete_project/request_spec.rb diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index 1152bdcf0585ec..b997b2e0b6b398 100644 --- a/app/controllers/projects_controller.rb +++ b/app/controllers/projects_controller.rb @@ -29,7 +29,7 @@ class ProjectsController < Projects::ApplicationController before_action :authorize_read_code!, only: [:refs] # Authorize - before_action :authorize_admin_project_or_custom_permissions!, only: :edit + before_action :authorize_view_edit_page!, only: :edit before_action :authorize_admin_project!, only: [:update, :housekeeping, :download_export, :export, :remove_export, :generate_new_export] before_action :authorize_archive_project!, only: [:archive, :unarchive] before_action :event_filter, only: [:show, :activity] @@ -612,11 +612,6 @@ def check_export_rate_limit! def render_edit render 'edit' end - - # Overridden in EE - def authorize_admin_project_or_custom_permissions! - authorize_admin_project! - end end ProjectsController.prepend_mod_with('ProjectsController') diff --git a/app/policies/project_policy.rb b/app/policies/project_policy.rb index 255538c538afc0..a26758974d6150 100644 --- a/app/policies/project_policy.rb +++ b/app/policies/project_policy.rb @@ -914,6 +914,7 @@ class ProjectPolicy < BasePolicy rule { can?(:admin_project) }.policy do enable :read_usage_quotas + enable :view_edit_page end rule { can?(:project_bot_access) }.policy do diff --git a/db/migrate/20231213170159_add_delete_project_to_member_roles.rb b/db/migrate/20231213170159_add_delete_project_to_member_roles.rb deleted file mode 100644 index bb9fee0f1be7e1..00000000000000 --- a/db/migrate/20231213170159_add_delete_project_to_member_roles.rb +++ /dev/null @@ -1,10 +0,0 @@ -# frozen_string_literal: true - -class AddDeleteProjectToMemberRoles < Gitlab::Database::Migration[2.2] - milestone '16.8' - enable_lock_retries! - - def change - add_column :member_roles, :delete_project, :boolean, default: false, null: false - end -end diff --git a/db/migrate/20231213170159_add_remove_project_to_member_roles.rb b/db/migrate/20231213170159_add_remove_project_to_member_roles.rb new file mode 100644 index 00000000000000..d13777c1df1f85 --- /dev/null +++ b/db/migrate/20231213170159_add_remove_project_to_member_roles.rb @@ -0,0 +1,10 @@ +# frozen_string_literal: true + +class AddRemoveProjectToMemberRoles < Gitlab::Database::Migration[2.2] + milestone '16.8' + enable_lock_retries! + + def change + add_column :member_roles, :remove_project, :boolean, default: false, null: false + end +end diff --git a/db/structure.sql b/db/structure.sql index 1aa547766c2390..a6a18a6d35db2d 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -18657,7 +18657,7 @@ CREATE TABLE member_roles ( admin_group_member boolean DEFAULT false NOT NULL, manage_project_access_tokens boolean DEFAULT false NOT NULL, archive_project boolean DEFAULT false NOT NULL, - delete_project boolean DEFAULT false NOT NULL, + remove_project 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 13f1dc09e241f9..ac98b00353303b 100644 --- a/doc/api/graphql/reference/index.md +++ b/doc/api/graphql/reference/index.md @@ -5352,7 +5352,6 @@ Input type: `MemberRoleCreateInput` | `archiveProject` | [`Boolean`](#boolean) | Permission to archive projects. | | `baseAccessLevel` | [`MemberAccessLevel!`](#memberaccesslevel) | Base access level for the custom role. | | `clientMutationId` | [`String`](#string) | A unique identifier for the client performing the mutation. | -| `deleteProject` | [`Boolean`](#boolean) | Permission to delete projects. | | `description` | [`String`](#string) | Description of the member role. | | `groupPath` | [`ID!`](#id) | Group the member role to mutate is in. | | `manageProjectAccessTokens` | [`Boolean`](#boolean) | Permission to admin project access tokens. | @@ -5361,6 +5360,7 @@ Input type: `MemberRoleCreateInput` | `readCode` | [`Boolean`](#boolean) | Permission to read code. | | `readDependency` | [`Boolean`](#boolean) | Permission to read dependency. | | `readVulnerability` | [`Boolean`](#boolean) | Permission to read vulnerability. | +| `removeProject` | [`Boolean`](#boolean) | Permission to remove projects. | #### Fields @@ -21285,7 +21285,6 @@ Represents a member role. | `adminVulnerability` **{warning-solid}** | [`Boolean`](#boolean) | **Introduced** in 16.5. This feature is an Experiment. It can be changed or removed at any time. Permission to admin vulnerability. | | `archiveProject` **{warning-solid}** | [`Boolean`](#boolean) | **Introduced** in 16.6. This feature is an Experiment. It can be changed or removed at any time. Permission to archive projects. | | `baseAccessLevel` **{warning-solid}** | [`AccessLevel!`](#accesslevel) | **Introduced** in 16.5. This feature is an Experiment. It can be changed or removed at any time. Base access level for the custom role. | -| `deleteProject` **{warning-solid}** | [`Boolean`](#boolean) | **Introduced** in 16.8. This feature is an Experiment. It can be changed or removed at any time. Permission to delete projects. | | `description` | [`String`](#string) | Description of the member role. | | `enabledPermissions` **{warning-solid}** | [`[MemberRolePermission!]`](#memberrolepermission) | **Introduced** in 16.5. This feature is an Experiment. It can be changed or removed at any time. Array of all permissions enabled for the custom role. | | `id` | [`MemberRoleID!`](#memberroleid) | ID of the member role. | @@ -21295,6 +21294,7 @@ Represents a member role. | `readCode` **{warning-solid}** | [`Boolean`](#boolean) | **Introduced** in 16.5. This feature is an Experiment. It can be changed or removed at any time. Permission to read code. | | `readDependency` **{warning-solid}** | [`Boolean`](#boolean) | **Introduced** in 16.5. This feature is an Experiment. It can be changed or removed at any time. Permission to read dependency. | | `readVulnerability` **{warning-solid}** | [`Boolean`](#boolean) | **Introduced** in 16.5. This feature is an Experiment. It can be changed or removed at any time. Permission to read vulnerability. | +| `removeProject` **{warning-solid}** | [`Boolean`](#boolean) | **Introduced** in 16.8. This feature is an Experiment. It can be changed or removed at any time. Permission to remove projects. | ### `MergeAccessLevel` @@ -30608,11 +30608,11 @@ Member role permission. | `ADMIN_MERGE_REQUEST` | Allows to approve merge requests. | | `ADMIN_VULNERABILITY` | Allows admin access to the vulnerability reports. | | `ARCHIVE_PROJECT` | Allows to archive projects. | -| `DELETE_PROJECT` | Allows to delete projects. | | `MANAGE_PROJECT_ACCESS_TOKENS` | Allows manage access to the project access tokens. | | `READ_CODE` | Allows read-only access to the source code. | | `READ_DEPENDENCY` | Allows read-only access to the dependencies. | | `READ_VULNERABILITY` | Allows read-only access to the vulnerability reports. | +| `REMOVE_PROJECT` | Allows to remove projects. | ### `MemberSort` diff --git a/doc/api/member_roles.md b/doc/api/member_roles.md index 2ce8ed4f852db5..a482195b94e39f 100644 --- a/doc/api/member_roles.md +++ b/doc/api/member_roles.md @@ -52,7 +52,7 @@ If successful, returns [`200`](rest/index.md#status-codes) and the following res | `[].admin_group_member` | boolean | Permission to admin members of a group. | | `[].manage_project_access_tokens` | boolean | Permission to manage project access tokens. | | `[].archive_project` | boolean | Permission to archive projects. | -| `[].delete_project` | boolean | Permission to delete projects. | +| `[].remove_project` | boolean | Permission to delete projects. | Example request: @@ -77,7 +77,7 @@ Example response: "read_vulnerability": false, "manage_project_access_tokens": false, "archive_project": false, - "delete_project": false + "remove_project": false }, { "id": 3, @@ -92,7 +92,7 @@ Example response: "read_vulnerability": true, "manage_project_access_tokens": false, "archive_project": false, - "delete_project": false + "remove_project": false } ] ``` diff --git a/doc/user/custom_roles.md b/doc/user/custom_roles.md index b1c26ca364c1cf..a4518143d5ccbe 100644 --- a/doc/user/custom_roles.md +++ b/doc/user/custom_roles.md @@ -116,7 +116,7 @@ These requirements are documented in the `Required permission` column in the fol | `manage_project_access_tokens` | GitLab 16.5 and later | Not applicable | Create, delete, and list [project access tokens](project/settings/project_access_tokens.md). | | `admin_group_member` | GitLab 16.5 and later | Not applicable | Add or remove [group members](group/manage.md). | | `archive_project` | GitLab 16.7 and later | Not applicable | Archive and unarchive [projects](project/settings/migrate_projects.md#archive-a-project). | -| `delete_project` | GitLab 16.8 and later | Not applicable | Delete [projects](project/working_with_projects.md#delete-a-project). | +| `remove_project` | GitLab 16.8 and later | Not applicable | Remove [projects](project/working_with_projects.md#delete-a-project). | ## Billing and seat usage diff --git a/ee/app/controllers/ee/projects_controller.rb b/ee/app/controllers/ee/projects_controller.rb index bd849060c0250b..a7d0d6f43951b1 100644 --- a/ee/app/controllers/ee/projects_controller.rb +++ b/ee/app/controllers/ee/projects_controller.rb @@ -220,9 +220,5 @@ def log_archive_audit_event def log_unarchive_audit_event log_audit_event(message: 'Project unarchived', event_type: 'project_unarchived') end - - def authorize_admin_project_or_custom_permissions! - can?(current_user, :archive_project, project) || can?(current_user, :remove_project, project) || super - end end end diff --git a/ee/app/graphql/mutations/member_roles/create.rb b/ee/app/graphql/mutations/member_roles/create.rb index 947a4ee7947225..2fe246a2a419c0 100644 --- a/ee/app/graphql/mutations/member_roles/create.rb +++ b/ee/app/graphql/mutations/member_roles/create.rb @@ -27,10 +27,6 @@ class Create < Base ::Types::MemberAccessLevelEnum, required: true, description: 'Base access level for the custom role.' - argument :delete_project, - GraphQL::Types::Boolean, - required: false, - description: 'Permission to delete projects.' argument :group_path, GraphQL::Types::ID, required: true, description: 'Group the member role to mutate is in.' @@ -54,6 +50,10 @@ class Create < Base GraphQL::Types::Boolean, required: false, description: 'Permission to read vulnerability.' + argument :remove_project, + GraphQL::Types::Boolean, + required: false, + description: 'Permission to remove projects.' def resolve(args) group = authorized_find!(group_path: args.delete(:group_path)) diff --git a/ee/app/graphql/types/member_roles/member_role_type.rb b/ee/app/graphql/types/member_roles/member_role_type.rb index d3b924a03ee045..34fa910e56a183 100644 --- a/ee/app/graphql/types/member_roles/member_role_type.rb +++ b/ee/app/graphql/types/member_roles/member_role_type.rb @@ -47,11 +47,11 @@ class MemberRoleType < BaseObject alpha: { milestone: '16.6' }, description: 'Permission to archive projects.' - field :delete_project, + field :remove_project, GraphQL::Types::Boolean, null: true, alpha: { milestone: '16.8' }, - description: 'Permission to delete projects.' + description: 'Permission to remove projects.' field :manage_project_access_tokens, GraphQL::Types::Boolean, diff --git a/ee/app/policies/ee/project_policy.rb b/ee/app/policies/ee/project_policy.rb index a455977529694c..3fd800092d8093 100644 --- a/ee/app/policies/ee/project_policy.rb +++ b/ee/app/policies/ee/project_policy.rb @@ -656,18 +656,22 @@ module ProjectPolicy end desc "Custom role on project that enables deleting projects" - condition(:custom_role_enables_delete_projects) do + condition(:custom_role_enables_remove_projects) do ::Auth::MemberRoleAbilityLoader.new( user: @user, resource: @subject, - ability: :delete_project + ability: :remove_project ).has_ability? end - rule { custom_roles_allowed & custom_role_enables_delete_projects }.policy do + rule { custom_roles_allowed & custom_role_enables_remove_projects }.policy do enable :remove_project end + rule { can?(:admin_project) | can?(:archive_project) | can?(:remove_project) }.policy do + enable :view_edit_page + end + rule { needs_new_sso_session }.policy do prevent :read_project end diff --git a/ee/config/custom_abilities/delete_project.yml b/ee/config/custom_abilities/remove_project.yml similarity index 81% rename from ee/config/custom_abilities/delete_project.yml rename to ee/config/custom_abilities/remove_project.yml index 5af9e197f9c322..571fcfcd0bee32 100644 --- a/ee/config/custom_abilities/delete_project.yml +++ b/ee/config/custom_abilities/remove_project.yml @@ -1,6 +1,6 @@ --- -name: delete_project -description: Allows to delete projects. +name: remove_project +description: Allows to remove projects. introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/425959 introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/139696 feature_category: groups_and_projects diff --git a/ee/lib/ee/sidebars/projects/menus/settings_menu.rb b/ee/lib/ee/sidebars/projects/menus/settings_menu.rb index 781077ddc15cf1..1e274561219dd1 100644 --- a/ee/lib/ee/sidebars/projects/menus/settings_menu.rb +++ b/ee/lib/ee/sidebars/projects/menus/settings_menu.rb @@ -49,8 +49,7 @@ def custom_roles_menu_items end def custom_roles_general_menu_item? - can?(context.current_user, :archive_project, context.project) || - can?(context.current_user, :remove_project, context.project) + can?(context.current_user, :view_edit_page, context.project) end def custom_roles_access_token_menu_item? diff --git a/ee/spec/controllers/projects_controller_spec.rb b/ee/spec/controllers/projects_controller_spec.rb index b3508e044e5fd9..0670db7c7dbdd0 100644 --- a/ee/spec/controllers/projects_controller_spec.rb +++ b/ee/spec/controllers/projects_controller_spec.rb @@ -169,54 +169,6 @@ end end - describe 'GET edit', feature_category: :permissions do - subject(:request) { get :edit, params: { namespace_id: project.namespace.path, id: project.path } } - - it 'does not allow an auditor user to access the page' do - sign_in(create(:user, :auditor)) - - request - - expect(response).to have_gitlab_http_status(:not_found) - end - - context 'when the user can archive projects' do - let_it_be(:guest) { create(:user) } - - before do - project.add_guest(guest) - allow(controller).to receive(:can?).and_call_original - allow(controller).to receive(:can?).with(guest, :archive_project, anything).and_return(true) - - sign_in(guest) - request - end - - it 'allows the user to access the page' do - expect(response).to have_gitlab_http_status(:ok) - expect(response).to render_template(:edit) - end - end - - context 'when the user can remove projects' do - let_it_be(:guest) { create(:user) } - - before do - project.add_guest(guest) - allow(controller).to receive(:can?).and_call_original - allow(controller).to receive(:can?).with(guest, :remove_project, anything).and_return(true) - - sign_in(guest) - request - end - - it 'allows the user to access the page' do - expect(response).to have_gitlab_http_status(:ok) - expect(response).to render_template(:edit) - end - end - end - describe 'POST create', feature_category: :groups_and_projects do let!(:params) do { diff --git a/ee/spec/features/projects/custom_roles/archive_project_custom_permission_spec.rb b/ee/spec/features/projects/custom_roles/archive_project_custom_permission_spec.rb deleted file mode 100644 index 7285adbf042e3f..00000000000000 --- a/ee/spec/features/projects/custom_roles/archive_project_custom_permission_spec.rb +++ /dev/null @@ -1,81 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe 'Archive Project Custom Permission', feature_category: :groups_and_projects do - let_it_be(:project) { create(:project, :in_group) } - let_it_be(:user) { create(:user) } - let_it_be(:custom_role) { create(:member_role, :guest, namespace: project.root_namespace, archive_project: true) } - - before do - stub_licensed_features(custom_roles: true) - create(:project_member, :guest, member_role: custom_role, user: user, project: project) - sign_in(user) - end - - context 'when the project is not archived', :js, :aggregate_failures do - it 'allows a guest user with custom `archive_project` permissions to archive it' do - visit project_path(project) - - within_testid('super-sidebar') do - click_button('Settings') - click_link('General') - end - - click_link('Archive project') - click_button('Archive project') - - expect(page).to have_current_path(project_path(project)) - expect(project.reload.archived?).to eq(true) - end - end - - context 'when the project is archived', :js, :aggregate_failures do - let_it_be(:project) { create(:project, :archived, namespace: project.namespace) } - - it 'allows a guest user with custom `archive_project` permissions to unarchive it' do - visit project_path(project) - - within_testid('super-sidebar') do - click_button('Settings') - click_link('General') - end - - click_link('Unarchive project') - click_button('Unarchive project') - - expect(page).to have_current_path(project_path(project)) - expect(project.reload).not_to be_archived - end - end - - shared_examples 'does not allow the user to archive the project' do - it 'does not show the `Settings` sidebar item', :js do - visit project_path(project) - - within_testid('super-sidebar') do - expect(page).not_to have_button('Settings') - end - end - - it 'does not allow access to the edit page' do - visit edit_project_path(project) - - expect(page).to have_gitlab_http_status(:not_found) - end - end - - context 'when the `custom_roles` licensed feature is not available' do - before do - stub_licensed_features(custom_roles: false) - end - - it_behaves_like 'does not allow the user to archive the project' - end - - context 'when the user does not have the custom `archive_project` permission' do - let_it_be(:custom_role) { create(:member_role, :guest, namespace: project.root_namespace, archive_project: false) } - - it_behaves_like 'does not allow the user to archive the project' - end -end diff --git a/ee/spec/features/projects/custom_roles/delete_project_custom_permission_spec.rb b/ee/spec/features/projects/custom_roles/delete_project_custom_permission_spec.rb deleted file mode 100644 index 432f881220f638..00000000000000 --- a/ee/spec/features/projects/custom_roles/delete_project_custom_permission_spec.rb +++ /dev/null @@ -1,61 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe 'Delete Project Custom Permission', feature_category: :permissions do - let_it_be(:project) { create(:project, :in_group) } - let_it_be(:user) { create(:user) } - let_it_be(:member_role) { create(:member_role, :guest, namespace: project.root_namespace, delete_project: true) } - - before do - stub_licensed_features(custom_roles: true) - create(:project_member, :guest, member_role: member_role, user: user, project: project) - sign_in(user) - end - - it 'allows a guest user with the custom `delete_project` ability to delete it', :js, :aggregate_failures do - visit project_path(project) - - within_testid('super-sidebar') do - click_button('Settings') - click_link('General') - end - - click_button(_('Delete project')) - fill_in 'confirm_name_input', with: project.path_with_namespace - click_button(_('Yes, delete project')) - - expect(page).to have_content("Project '#{project.full_name}' is in the process of being deleted.") - expect(project.reload).to be_pending_delete - end - - shared_examples 'does not allow the user to delete the project' do - it 'does not show the `Settings` sidebar item', :js do - visit project_path(project) - - within_testid('super-sidebar') do - expect(page).not_to have_button('Settings') - end - end - - it 'does not allow access to the edit page' do - visit edit_project_path(project) - - expect(page).to have_gitlab_http_status(:not_found) - end - end - - context 'when the `custom_roles` licensed feature is not available' do - before do - stub_licensed_features(custom_roles: false) - end - - it_behaves_like 'does not allow the user to delete the project' - end - - context 'when the user does not have the custom role' do - let_it_be(:member_role) { nil } - - it_behaves_like 'does not allow the user to delete the project' - end -end diff --git a/ee/spec/lib/ee/api/entities/member_role_spec.rb b/ee/spec/lib/ee/api/entities/member_role_spec.rb index b0ff59628c8e8a..6b1fe24b342650 100644 --- a/ee/spec/lib/ee/api/entities/member_role_spec.rb +++ b/ee/spec/lib/ee/api/entities/member_role_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe EE::API::Entities::MemberRole do +RSpec.describe EE::API::Entities::MemberRole, feature_category: :permissions do describe 'exposes expected fields' do let_it_be(:group) { create(:group) } let_it_be(:owner) { create(:group_member, :owner, source: group) } @@ -22,7 +22,7 @@ expect(subject[:admin_vulnerability]).to eq member_role.admin_vulnerability expect(subject[:manage_project_access_tokens]).to eq member_role.manage_project_access_tokens expect(subject[:archive_project]).to eq member_role.archive_project - expect(subject[:delete_project]).to eq member_role.delete_project + expect(subject[:remove_project]).to eq member_role.remove_project expect(subject[:group_id]).to eq(member_role.namespace.id) end end diff --git a/ee/spec/lib/ee/sidebars/projects/menus/settings_menu_spec.rb b/ee/spec/lib/ee/sidebars/projects/menus/settings_menu_spec.rb index a21d5ae76f1384..6c88d80391d20b 100644 --- a/ee/spec/lib/ee/sidebars/projects/menus/settings_menu_spec.rb +++ b/ee/spec/lib/ee/sidebars/projects/menus/settings_menu_spec.rb @@ -56,19 +56,9 @@ expect(subject).to be_nil end - context 'when the user has the `archive_project` ability' do + context 'when the user has the `view_edit_page` ability' do before do - allow(Ability).to receive(:allowed?).with(user, :archive_project, project).and_return(true) - end - - it 'includes the general menu item' do - expect(subject.title).to eql('General') - end - end - - context 'when the user has the `remove_project` ability' do - before do - allow(Ability).to receive(:allowed?).with(user, :remove_project, project).and_return(true) + allow(Ability).to receive(:allowed?).with(user, :view_edit_page, project).and_return(true) end it 'includes the general menu item' do diff --git a/ee/spec/models/ee/user_spec.rb b/ee/spec/models/ee/user_spec.rb index 402d0e078dbc26..ddea5fa7d65942 100644 --- a/ee/spec/models/ee/user_spec.rb +++ b/ee/spec/models/ee/user_spec.rb @@ -1224,10 +1224,10 @@ OR admin_merge_request = true OR admin_vulnerability = true OR archive_project = true - OR delete_project = true OR manage_project_access_tokens = true OR read_dependency = true - OR read_vulnerability = true\)\)\)\)'.squish # allow_cross_joins_across_databases + OR read_vulnerability = true + OR remove_project = true\)\)\)\)'.squish # allow_cross_joins_across_databases end before do diff --git a/ee/spec/policies/project_policy_spec.rb b/ee/spec/policies/project_policy_spec.rb index ab083129345101..36a7ce13b413a9 100644 --- a/ee/spec/policies/project_policy_spec.rb +++ b/ee/spec/policies/project_policy_spec.rb @@ -2780,14 +2780,14 @@ def create_member_role(member, abilities = member_role_abilities) context 'for a member role with archive_project true' do let(:member_role_abilities) { { archive_project: true } } - let(:allowed_abilities) { [:archive_project] } + let(:allowed_abilities) { [:archive_project, :view_edit_page] } it_behaves_like 'custom roles abilities' end - context 'for a member role with `delete_project` true' do - let(:member_role_abilities) { { delete_project: true } } - let(:allowed_abilities) { [:remove_project] } + context 'for a member role with `remove_project` true' do + let(:member_role_abilities) { { remove_project: true } } + let(:allowed_abilities) { [:remove_project, :view_edit_page] } it_behaves_like 'custom roles abilities' end diff --git a/ee/spec/requests/api/member_roles_spec.rb b/ee/spec/requests/api/member_roles_spec.rb index e3d64d0a944fc0..a1f52f1db2469e 100644 --- a/ee/spec/requests/api/member_roles_spec.rb +++ b/ee/spec/requests/api/member_roles_spec.rb @@ -107,7 +107,7 @@ "admin_vulnerability" => false, "manage_project_access_tokens" => false, "archive_project" => false, - "delete_project" => false, + "remove_project" => false, "group_id" => group_id }, { @@ -123,7 +123,7 @@ "admin_vulnerability" => false, "manage_project_access_tokens" => false, "archive_project" => false, - "delete_project" => false, + "remove_project" => false, "group_id" => group_id } ] diff --git a/ee/spec/requests/custom_roles/archive_project/request_spec.rb b/ee/spec/requests/custom_roles/archive_project/request_spec.rb new file mode 100644 index 00000000000000..5520b9159b9f37 --- /dev/null +++ b/ee/spec/requests/custom_roles/archive_project/request_spec.rb @@ -0,0 +1,46 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe 'User with archive_project custom role', feature_category: :permissions do + let_it_be(:user) { create(:user) } + let_it_be(:project) { create(:project, :in_group) } + + before do + stub_licensed_features(custom_roles: true) + + sign_in(user) + end + + describe ProjectsController do + let_it_be(:role) { create(:member_role, :guest, namespace: project.group, archive_project: true) } + let_it_be(:member) { create(:project_member, :guest, member_role: role, user: user, project: project) } + + describe "#edit" do + it 'user has access via a custom role' do + get edit_project_path(project) + + expect(response).to have_gitlab_http_status(:ok) + expect(response).to render_template(:edit) + end + end + + describe "#archive" do + it 'user has access via a custom role' do + post archive_project_path(project) + + expect(project.reload).to be_archived + expect(response).to have_gitlab_http_status(:redirect) + end + end + + describe "#unarchive" do + it 'user has access via a custom role' do + post unarchive_project_path(project) + + expect(project.reload).not_to be_archived + expect(response).to have_gitlab_http_status(:redirect) + end + end + end +end diff --git a/ee/spec/requests/custom_roles/delete_project/request_spec.rb b/ee/spec/requests/custom_roles/delete_project/request_spec.rb new file mode 100644 index 00000000000000..8b42e8fbadbf6e --- /dev/null +++ b/ee/spec/requests/custom_roles/delete_project/request_spec.rb @@ -0,0 +1,37 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe 'User with remove_project custom role', feature_category: :permissions do + let_it_be(:user) { create(:user) } + let_it_be(:project) { create(:project, :in_group) } + + before do + stub_licensed_features(custom_roles: true) + + sign_in(user) + end + + describe ProjectsController do + let_it_be(:role) { create(:member_role, :guest, namespace: project.group, remove_project: true) } + let_it_be(:member) { create(:project_member, :guest, member_role: role, user: user, project: project) } + + describe "#edit" do + it 'user has access via a custom role' do + get edit_project_path(project) + + expect(response).to have_gitlab_http_status(:ok) + expect(response).to render_template(:edit) + end + end + + describe "#destroy" do + it 'user has access via a custom role' do + delete project_path(project) + + expect(project.reload).to be_pending_delete + expect(response).to have_gitlab_http_status(:found) + end + end + end +end diff --git a/ee/spec/views/projects/edit.html.haml_spec.rb b/ee/spec/views/projects/edit.html.haml_spec.rb index 14df5aa749c106..752796896647dc 100644 --- a/ee/spec/views/projects/edit.html.haml_spec.rb +++ b/ee/spec/views/projects/edit.html.haml_spec.rb @@ -39,7 +39,7 @@ end end - context 'when rendering for a user that is not an owner' do + context 'when rendering for a user that is not an owner', feature_category: :permissions do let_it_be(:user) { create(:user) } let(:can_archive_projects) { false } diff --git a/spec/policies/project_policy_spec.rb b/spec/policies/project_policy_spec.rb index a9a4575d747cde..9f4bf4f6b369c8 100644 --- a/spec/policies/project_policy_spec.rb +++ b/spec/policies/project_policy_spec.rb @@ -472,12 +472,12 @@ def set_access_level(access_level) end end - context 'reading usage quotas' do + context 'reading usage quotas and viewing the edit page' do %w[maintainer owner].each do |role| context "with #{role}" do let(:current_user) { send(role) } - it { is_expected.to be_allowed(:read_usage_quotas) } + it { is_expected.to be_allowed(:read_usage_quotas, :view_edit_page) } end end @@ -485,7 +485,7 @@ def set_access_level(access_level) context "with #{role}" do let(:current_user) { send(role) } - it { is_expected.to be_disallowed(:read_usage_quotas) } + it { is_expected.to be_disallowed(:read_usage_quotas, :view_edit_page) } end end @@ -493,11 +493,11 @@ def set_access_level(access_level) let(:current_user) { admin } context 'when admin mode is enabled', :enable_admin_mode do - it { expect_allowed(:read_usage_quotas) } + it { expect_allowed(:read_usage_quotas, :view_edit_page) } end context 'when admin mode is disabled' do - it { expect_disallowed(:read_usage_quotas) } + it { expect_disallowed(:read_usage_quotas, :view_edit_page) } end end end -- GitLab From 893469e7c9ea7447f0b35c7c6110898f0220424c Mon Sep 17 00:00:00 2001 From: Alex Buijs Date: Mon, 18 Dec 2023 15:59:26 +0100 Subject: [PATCH 3/3] Replace `remove` with `delete` in documentation --- doc/api/graphql/reference/index.md | 6 +++--- doc/user/custom_roles.md | 4 ++-- ee/app/graphql/mutations/member_roles/create.rb | 2 +- ee/app/graphql/types/member_roles/member_role_type.rb | 2 +- ee/config/custom_abilities/remove_project.yml | 2 +- 5 files changed, 8 insertions(+), 8 deletions(-) diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md index ac98b00353303b..2ca522715fbba1 100644 --- a/doc/api/graphql/reference/index.md +++ b/doc/api/graphql/reference/index.md @@ -5360,7 +5360,7 @@ Input type: `MemberRoleCreateInput` | `readCode` | [`Boolean`](#boolean) | Permission to read code. | | `readDependency` | [`Boolean`](#boolean) | Permission to read dependency. | | `readVulnerability` | [`Boolean`](#boolean) | Permission to read vulnerability. | -| `removeProject` | [`Boolean`](#boolean) | Permission to remove projects. | +| `removeProject` | [`Boolean`](#boolean) | Permission to delete projects. | #### Fields @@ -21294,7 +21294,7 @@ Represents a member role. | `readCode` **{warning-solid}** | [`Boolean`](#boolean) | **Introduced** in 16.5. This feature is an Experiment. It can be changed or removed at any time. Permission to read code. | | `readDependency` **{warning-solid}** | [`Boolean`](#boolean) | **Introduced** in 16.5. This feature is an Experiment. It can be changed or removed at any time. Permission to read dependency. | | `readVulnerability` **{warning-solid}** | [`Boolean`](#boolean) | **Introduced** in 16.5. This feature is an Experiment. It can be changed or removed at any time. Permission to read vulnerability. | -| `removeProject` **{warning-solid}** | [`Boolean`](#boolean) | **Introduced** in 16.8. This feature is an Experiment. It can be changed or removed at any time. Permission to remove projects. | +| `removeProject` **{warning-solid}** | [`Boolean`](#boolean) | **Introduced** in 16.8. This feature is an Experiment. It can be changed or removed at any time. Permission to delete projects. | ### `MergeAccessLevel` @@ -30612,7 +30612,7 @@ Member role permission. | `READ_CODE` | Allows read-only access to the source code. | | `READ_DEPENDENCY` | Allows read-only access to the dependencies. | | `READ_VULNERABILITY` | Allows read-only access to the vulnerability reports. | -| `REMOVE_PROJECT` | Allows to remove projects. | +| `REMOVE_PROJECT` | Allows deletion of projects. | ### `MemberSort` diff --git a/doc/user/custom_roles.md b/doc/user/custom_roles.md index a4518143d5ccbe..ac04e7cca0f1c6 100644 --- a/doc/user/custom_roles.md +++ b/doc/user/custom_roles.md @@ -115,8 +115,8 @@ These requirements are documented in the `Required permission` column in the fol | `admin_merge_request` | GitLab 16.4 and later | Not applicable | View and approve [merge requests](project/merge_requests/index.md), revoke merge request approval, and view the associated merge request code.
Does not allow users to view or change merge request approval rules. | | `manage_project_access_tokens` | GitLab 16.5 and later | Not applicable | Create, delete, and list [project access tokens](project/settings/project_access_tokens.md). | | `admin_group_member` | GitLab 16.5 and later | Not applicable | Add or remove [group members](group/manage.md). | -| `archive_project` | GitLab 16.7 and later | Not applicable | Archive and unarchive [projects](project/settings/migrate_projects.md#archive-a-project). | -| `remove_project` | GitLab 16.8 and later | Not applicable | Remove [projects](project/working_with_projects.md#delete-a-project). | +| `archive_project` | GitLab 16.7 and later | Not applicable | [Archive and unarchive projects](project/settings/migrate_projects.md#archive-a-project). | +| `remove_project` | GitLab 16.8 and later | Not applicable | [Delete projects](project/working_with_projects.md#delete-a-project). | ## Billing and seat usage diff --git a/ee/app/graphql/mutations/member_roles/create.rb b/ee/app/graphql/mutations/member_roles/create.rb index 2fe246a2a419c0..8e2a298c10e5b6 100644 --- a/ee/app/graphql/mutations/member_roles/create.rb +++ b/ee/app/graphql/mutations/member_roles/create.rb @@ -53,7 +53,7 @@ class Create < Base argument :remove_project, GraphQL::Types::Boolean, required: false, - description: 'Permission to remove projects.' + description: 'Permission to delete projects.' def resolve(args) group = authorized_find!(group_path: args.delete(:group_path)) diff --git a/ee/app/graphql/types/member_roles/member_role_type.rb b/ee/app/graphql/types/member_roles/member_role_type.rb index 34fa910e56a183..58ea61772875e6 100644 --- a/ee/app/graphql/types/member_roles/member_role_type.rb +++ b/ee/app/graphql/types/member_roles/member_role_type.rb @@ -51,7 +51,7 @@ class MemberRoleType < BaseObject GraphQL::Types::Boolean, null: true, alpha: { milestone: '16.8' }, - description: 'Permission to remove projects.' + description: 'Permission to delete projects.' field :manage_project_access_tokens, GraphQL::Types::Boolean, diff --git a/ee/config/custom_abilities/remove_project.yml b/ee/config/custom_abilities/remove_project.yml index 571fcfcd0bee32..7ed3a5095a69af 100644 --- a/ee/config/custom_abilities/remove_project.yml +++ b/ee/config/custom_abilities/remove_project.yml @@ -1,6 +1,6 @@ --- name: remove_project -description: Allows to remove projects. +description: Allows deletion of projects. introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/425959 introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/139696 feature_category: groups_and_projects -- GitLab