From 3217cc5e819db28aecc7a02d2041e37c4cc3b88d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jarka=20Ko=C5=A1anov=C3=A1?= Date: Tue, 7 May 2024 09:14:17 +0200 Subject: [PATCH 1/6] Add managing protected branches as custom permission Changelog: added EE: true --- .../settings/components/access_dropdown.vue | 100 +++++++----- .../settings/repository_controller.rb | 2 + app/policies/protected_branch_policy.rb | 1 - .../json_schemas/member_role_permissions.json | 3 + .../settings/repository/show.html.haml | 2 + doc/api/graphql/reference/index.md | 1 + doc/user/custom_roles/abilities.md | 1 + .../projects/protected_branches_controller.rb | 4 + .../settings/repository_controller.rb | 3 +- ee/app/policies/ee/project_policy.rb | 8 + .../admin_protected_branch.yml | 11 ++ .../sidebars/projects/menus/settings_menu.rb | 3 +- .../protected_branches_controller_spec.rb | 56 +++++++ .../settings/repository_controller_spec.rb | 34 ++++ .../projects/protected_branches_spec.rb | 24 +++ .../settings/ee/protected_branches_spec.rb | 35 ++++ .../add_approvers_spec.js | 1 + .../create_protected_environment_spec.js | 1 + .../edit_protected_environments_list_spec.js | 5 +- .../protected_environment_edit_spec.js | 1 + .../projects/menus/settings_menu_spec.rb | 16 ++ ee/spec/policies/project_policy_spec.rb | 10 ++ .../admin_protected_branch/request_spec.rb | 112 +++++++++++++ .../protected_branches_controller_spec.rb | 95 +++++++++++ .../repository_settings_shared_contexts.rb | 17 ++ lib/api/helpers/protected_branches_helpers.rb | 12 ++ lib/api/protected_branches.rb | 10 +- spec/features/protected_branches_spec.rb | 151 +----------------- .../components/access_dropdown_spec.js | 4 + .../protected_branch_create_spec.js | 1 + .../protected_branch_edit_spec.js | 1 + .../protected_branches_shared_examples.rb | 100 ++++++++++++ 32 files changed, 629 insertions(+), 196 deletions(-) create mode 100644 ee/config/custom_abilities/admin_protected_branch.yml create mode 100644 ee/spec/features/projects/protected_branches_spec.rb create mode 100644 ee/spec/features/projects/settings/ee/protected_branches_spec.rb create mode 100644 ee/spec/requests/custom_roles/admin_protected_branch/request_spec.rb create mode 100644 ee/spec/requests/ee/projects/protected_branches_controller_spec.rb create mode 100644 ee/spec/support/shared_contexts/repository_settings_shared_contexts.rb create mode 100644 spec/support/shared_examples/projects/protected_branches_shared_examples.rb diff --git a/app/assets/javascripts/projects/settings/components/access_dropdown.vue b/app/assets/javascripts/projects/settings/components/access_dropdown.vue index 135e926cedf666..e86ba8539e09ce 100644 --- a/app/assets/javascripts/projects/settings/components/access_dropdown.vue +++ b/app/assets/javascripts/projects/settings/components/access_dropdown.vue @@ -231,24 +231,46 @@ export default { this.loading = true; if (this.hasLicense) { - Promise.all([ - getDeployKeys(this.query), - getUsers(this.query), - this.groups.length - ? Promise.resolve({ data: this.groups }) - : getGroups({ withProjectAccess: this.groupsWithProjectAccess }), - ]) - .then(([deployKeysResponse, usersResponse, groupsResponse]) => { - this.consolidateData(deployKeysResponse.data, usersResponse.data, groupsResponse.data); - this.setSelected({ initial }); - }) - .catch(() => - createAlert({ message: __('Failed to load groups, users and deploy keys.') }), - ) - .finally(() => { - this.initialLoading = false; - this.loading = false; - }); + if (gon.abilities.adminProject) { + Promise.all([ + getDeployKeys(this.query), + getUsers(this.query), + this.groups.length + ? Promise.resolve({ data: this.groups }) + : getGroups({ withProjectAccess: this.groupsWithProjectAccess }), + ]) + .then(([deployKeysResponse, usersResponse, groupsResponse]) => { + this.consolidateData( + deployKeysResponse.data, + usersResponse.data, + groupsResponse.data, + ); + this.setSelected({ initial }); + }) + .catch(() => + createAlert({ message: __('Failed to load groups, users and deploy keys.') }), + ) + .finally(() => { + this.initialLoading = false; + this.loading = false; + }); + } else if (gon.abilities.adminProtectedBranch) { + Promise.all([ + getUsers(this.query), + this.groups.length + ? Promise.resolve({ data: this.groups }) + : getGroups({ withProjectAccess: this.groupsWithProjectAccess }), + ]) + .then(([usersResponse, groupsResponse]) => { + this.consolidateData(null, usersResponse.data, groupsResponse.data); + this.setSelected({ initial }); + }) + .catch(() => createAlert({ message: __('Failed to load groups and users.') })) + .finally(() => { + this.initialLoading = false; + this.loading = false; + }); + } } else { getDeployKeys(this.query) .then((deployKeysResponse) => { @@ -284,27 +306,31 @@ export default { } } - this.deployKeys = deployKeysResponse.map((response) => { - const { - id, - fingerprint, - fingerprint_sha256: fingerprintSha256, - title, - owner: { avatar_url, name, username }, - } = response; + if (gon.abilities.adminProject) { + this.deployKeys = deployKeysResponse.map((response) => { + const { + id, + fingerprint, + fingerprint_sha256: fingerprintSha256, + title, + owner: { avatar_url, name, username }, + } = response; - const availableFingerprint = fingerprintSha256 || fingerprint; - const shortFingerprint = `(${availableFingerprint.substring(0, 14)}...)`; + const availableFingerprint = fingerprintSha256 || fingerprint; + const shortFingerprint = `(${availableFingerprint.substring(0, 14)}...)`; - return { - id, - title: title.concat(' ', shortFingerprint), - avatar_url, - fullname: name, - username, - type: LEVEL_TYPES.DEPLOY_KEY, - }; - }); + return { + id, + title: title.concat(' ', shortFingerprint), + avatar_url, + fullname: name, + username, + type: LEVEL_TYPES.DEPLOY_KEY, + }; + }); + } else { + this.deployKeys = []; + } }, setSelected({ initial } = {}) { if (initial) { diff --git a/app/controllers/projects/settings/repository_controller.rb b/app/controllers/projects/settings/repository_controller.rb index a06858cf9c1d5d..5ae0d5a81a6374 100644 --- a/app/controllers/projects/settings/repository_controller.rb +++ b/app/controllers/projects/settings/repository_controller.rb @@ -9,6 +9,8 @@ class RepositoryController < Projects::ApplicationController before_action do push_frontend_feature_flag(:edit_branch_rules, @project) + push_frontend_ability(ability: :admin_project, resource: project, user: current_user) + push_frontend_ability(ability: :admin_protected_branch, resource: project, user: current_user) end feature_category :source_code_management, [:show, :cleanup, :update] diff --git a/app/policies/protected_branch_policy.rb b/app/policies/protected_branch_policy.rb index 2be96ea7f24d46..aeab29c5b6cb85 100644 --- a/app/policies/protected_branch_policy.rb +++ b/app/policies/protected_branch_policy.rb @@ -5,7 +5,6 @@ class ProtectedBranchPolicy < BasePolicy rule { can?(:admin_project) }.policy do enable :read_protected_branch - enable :create_protected_branch enable :update_protected_branch enable :destroy_protected_branch end diff --git a/app/validators/json_schemas/member_role_permissions.json b/app/validators/json_schemas/member_role_permissions.json index 86abb3ce9ceac8..2504dfc5df2599 100644 --- a/app/validators/json_schemas/member_role_permissions.json +++ b/app/validators/json_schemas/member_role_permissions.json @@ -19,6 +19,9 @@ "admin_merge_request": { "type": "boolean" }, + "admin_protected_branch": { + "type": "boolean" + }, "admin_push_rules": { "type": "boolean" }, diff --git a/app/views/projects/settings/repository/show.html.haml b/app/views/projects/settings/repository/show.html.haml index cf950de75db945..bfefc1ecda5e7e 100644 --- a/app/views/projects/settings/repository/show.html.haml +++ b/app/views/projects/settings/repository/show.html.haml @@ -5,6 +5,8 @@ - if can?(current_user, :admin_project, @project) = render "projects/branch_defaults/show" + +- if can?(current_user, :admin_protected_branch, @project) = render "projects/branch_rules/show" - if can?(current_user, :admin_push_rules, @project) diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md index 3195920dd07500..30d86852ed87ea 100644 --- a/doc/api/graphql/reference/index.md +++ b/doc/api/graphql/reference/index.md @@ -36691,6 +36691,7 @@ Member role permission. | `ADMIN_GROUP_MEMBER` | Add or remove users in a group, and assign roles to users. When assigning a role, users with this custom permission must select a role that has the same or fewer permissions as the default role used as the base for their custom role. | | `ADMIN_INTEGRATIONS` | Create, read, update, and delete integrations with external applications. | | `ADMIN_MERGE_REQUEST` | Allows approval of merge requests. | +| `ADMIN_PROTECTED_BRANCH` | Create, read, update, and delete protected branches for a project. | | `ADMIN_PUSH_RULES` | Configure push rules for repositories at the group or project level. | | `ADMIN_RUNNERS` | Create, view, edit, and delete group or project Runners. Includes configuring Runner settings. | | `ADMIN_TERRAFORM_STATE` | Execute terraform commands, lock/unlock terraform state files, and remove file versions. | diff --git a/doc/user/custom_roles/abilities.md b/doc/user/custom_roles/abilities.md index 21157a17d1ddc5..a9ffbd4252bc41 100644 --- a/doc/user/custom_roles/abilities.md +++ b/doc/user/custom_roles/abilities.md @@ -86,6 +86,7 @@ These requirements are documented in the `Required permission` column in the fol | Name | Required permission | Description | Introduced in | Feature flag | Enabled in | |:-----|:------------|:------------------|:---------|:--------------|:---------| | [`admin_merge_request`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/128302) | | Allows approval of merge requests. | GitLab [16.4](https://gitlab.com/gitlab-org/gitlab/-/issues/412708) | | | +| [`admin_protected_branch`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/162208) | | Create, read, update, and delete protected branches for a project. | GitLab [17.4](https://gitlab.com/gitlab-org/gitlab/-/issues/448823) | | | | [`admin_push_rules`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/147872) | | Configure push rules for repositories at the group or project level. | GitLab [16.11](https://gitlab.com/gitlab-org/gitlab/-/issues/421786) | `custom_ability_admin_push_rules` | | | [`read_code`](https://gitlab.com/gitlab-org/gitlab/-/issues/376180) | | Allows read-only access to the source code in the user interface. Does not allow users to edit or download repository archives, clone or pull repositories, view source code in an IDE, or view merge requests for private projects. You can download individual files because read-only access inherently grants the ability to make a local copy of the file. | GitLab [15.7](https://gitlab.com/gitlab-org/gitlab/-/issues/20277) | `customizable_roles` | GitLab [15.9](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/110810) | diff --git a/ee/app/controllers/ee/projects/protected_branches_controller.rb b/ee/app/controllers/ee/projects/protected_branches_controller.rb index f1964210371557..6cdcf49fcec86a 100644 --- a/ee/app/controllers/ee/projects/protected_branches_controller.rb +++ b/ee/app/controllers/ee/projects/protected_branches_controller.rb @@ -15,6 +15,10 @@ def protected_ref_params(*attrs) params_hash end + + def authorize_admin_protected_refs! + authorize_admin_protected_branch! + end end end end diff --git a/ee/app/controllers/ee/projects/settings/repository_controller.rb b/ee/app/controllers/ee/projects/settings/repository_controller.rb index 86a7fcefffa0c3..06903678837160 100644 --- a/ee/app/controllers/ee/projects/settings/repository_controller.rb +++ b/ee/app/controllers/ee/projects/settings/repository_controller.rb @@ -95,7 +95,8 @@ def allow_protected_branches_for_group?(group) def authorize_view_repository_settings! return if can?(current_user, :admin_push_rules, project) || - can?(current_user, :manage_deploy_tokens, project) + can?(current_user, :manage_deploy_tokens, project) || + can?(current_user, :admin_protected_branch, project) authorize_admin_project! end diff --git a/ee/app/policies/ee/project_policy.rb b/ee/app/policies/ee/project_policy.rb index a4de461735be25..14c10fec5d9322 100644 --- a/ee/app/policies/ee/project_policy.rb +++ b/ee/app/policies/ee/project_policy.rb @@ -768,6 +768,14 @@ module ProjectPolicy enable :destroy_deploy_token end + rule { custom_role_enables_admin_protected_branch }.policy do + enable :read_protected_branch + enable :create_protected_branch + enable :update_protected_branch + enable :destroy_protected_branch + enable :admin_protected_branch + end + rule { can?(:create_issue) & okrs_enabled }.policy do enable :create_objective enable :create_key_result diff --git a/ee/config/custom_abilities/admin_protected_branch.yml b/ee/config/custom_abilities/admin_protected_branch.yml new file mode 100644 index 00000000000000..0274ff48f684a2 --- /dev/null +++ b/ee/config/custom_abilities/admin_protected_branch.yml @@ -0,0 +1,11 @@ +--- +name: admin_protected_branch +description: Create, read, update, and delete protected branches for a project. +introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/448823 +introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/162208 +feature_category: source_code_management +milestone: '17.4' +group_ability: false +project_ability: true +requirements: [] +available_from_access_level: 40 diff --git a/ee/lib/ee/sidebars/projects/menus/settings_menu.rb b/ee/lib/ee/sidebars/projects/menus/settings_menu.rb index 02adfce3bbec13..e5675363aa8b7a 100644 --- a/ee/lib/ee/sidebars/projects/menus/settings_menu.rb +++ b/ee/lib/ee/sidebars/projects/menus/settings_menu.rb @@ -16,7 +16,8 @@ module SettingsMenu ], repository_menu_item: [ :admin_push_rules, - :manage_deploy_tokens + :manage_deploy_tokens, + :admin_protected_branch ], merge_requests_menu_item: [ :manage_merge_request_settings diff --git a/ee/spec/controllers/ee/projects/protected_branches_controller_spec.rb b/ee/spec/controllers/ee/projects/protected_branches_controller_spec.rb index b8ab963723650c..6be04f91095ed1 100644 --- a/ee/spec/controllers/ee/projects/protected_branches_controller_spec.rb +++ b/ee/spec/controllers/ee/projects/protected_branches_controller_spec.rb @@ -11,6 +11,62 @@ project.add_maintainer(user) end + context 'when using custom roles' do + let_it_be(:group) { create(:group) } + let_it_be(:project) { create(:project, :repository, group: group) } + let(:base_params) { project_params.merge(id: protected_branch.id) } + + let_it_be(:protected_branch) { create(:protected_branch, project: project) } + let_it_be(:another_user) { create(:user) } + + let(:maintainer_access_level) { [{ access_level: Gitlab::Access::MAINTAINER }] } + let(:access_level_params) do + { merge_access_levels_attributes: maintainer_access_level, + push_access_levels_attributes: maintainer_access_level } + end + + let(:create_params) do + attributes_for(:protected_branch).merge(access_level_params) + end + + let(:update_params) { { name: 'new_name' } } + + before do + sign_in(another_user) + end + + context 'when a user has custom roles with `admin_protected_branch` assigned' do + let_it_be(:role) { create(:member_role, :guest, namespace: group, admin_protected_branch: true) } + let_it_be(:membership) { create(:group_member, :guest, member_role: role, user: another_user, group: group) } + + context 'when custom_roles feature is available' do + before do + stub_licensed_features(custom_roles: true) + end + + describe "POST #create" do + subject(:create_request) { post(:create, params: project_params.merge(protected_branch: create_params)) } + + it 'creates a protected branch' do + expect { create_request }.to change { ProtectedBranch.count }.by(1) + + expect(response).to have_gitlab_http_status(:found) + end + end + + describe "PUT #update" do + subject(:update_request) { put(:update, params: base_params.merge(protected_branch: update_params)) } + + it 'creates a protected branch' do + expect { update_request }.to change { protected_branch.reload.name } + + expect(response).to have_gitlab_http_status(:ok) + end + end + end + end + end + describe "POST #create" do shared_examples "protected branch with code owner approvals feature" do |boolean| it "sets code owner approvals to #{boolean} when protecting the branch" do diff --git a/ee/spec/controllers/projects/settings/repository_controller_spec.rb b/ee/spec/controllers/projects/settings/repository_controller_spec.rb index 17fa491eba5693..358cf1bfedf51e 100644 --- a/ee/spec/controllers/projects/settings/repository_controller_spec.rb +++ b/ee/spec/controllers/projects/settings/repository_controller_spec.rb @@ -198,5 +198,39 @@ .to contain_exactly(protected_branch_from_deletion) end end + + context 'when accessing through custom ability' do + let_it_be(:another_user) { create(:user) } + let_it_be(:role) { create(:member_role, :guest, namespace: group, admin_protected_branch: true) } + let_it_be(:membership) { create(:group_member, :guest, member_role: role, user: another_user, group: group) } + + before do + sign_in(another_user) + end + + context 'with custom_roles feature enabled' do + before do + stub_licensed_features(custom_roles: true) + end + + it 'allows access' do + get :show, params: { namespace_id: group, project_id: project } + + expect(response).to have_gitlab_http_status(:ok) + end + end + + context 'with custom_roles feature disabled' do + before do + stub_licensed_features(custom_roles: false) + end + + it 'does not allow access' do + get :show, params: { namespace_id: group, project_id: project } + + expect(response).to have_gitlab_http_status(:not_found) + end + end + end end end diff --git a/ee/spec/features/projects/protected_branches_spec.rb b/ee/spec/features/projects/protected_branches_spec.rb new file mode 100644 index 00000000000000..60b4673aa68b34 --- /dev/null +++ b/ee/spec/features/projects/protected_branches_spec.rb @@ -0,0 +1,24 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe 'Protected Branches', :js, feature_category: :source_code_management do + include ProtectedBranchHelpers + + context 'when a guest has custom roles with `admin_protected_branch` assigned' do + let_it_be(:user) { create(:user) } + let_it_be(:admin) { create(:admin) } + let_it_be(:group) { create(:group) } + let_it_be(:project) { create(:project, :repository, group: group) } + let_it_be(:role) { create(:member_role, :guest, namespace: group, admin_protected_branch: true) } + let_it_be(:membership) { create(:group_member, :guest, member_role: role, user: user, group: group) } + + before do + stub_licensed_features(custom_roles: true) + + sign_in(user) + end + + it_behaves_like 'setting project protected branches' + end +end diff --git a/ee/spec/features/projects/settings/ee/protected_branches_spec.rb b/ee/spec/features/projects/settings/ee/protected_branches_spec.rb new file mode 100644 index 00000000000000..ac93b7ef7c9763 --- /dev/null +++ b/ee/spec/features/projects/settings/ee/protected_branches_spec.rb @@ -0,0 +1,35 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe 'Projects > Settings > Repository settings using custom role', :js, feature_category: :source_code_management do + include ProtectedBranchHelpers + + let_it_be(:group) { create(:group) } + let_it_be(:project) { create(:project, :repository, group: group) } + + let_it_be(:current_user) { create(:user) } + let_it_be(:admin) { create(:admin) } + let_it_be(:role) { create(:member_role, :guest, namespace: group, admin_protected_branch: true) } + let_it_be(:membership) { create(:group_member, :guest, member_role: role, user: current_user, group: group) } + + context 'when user is a guest with custom roles that enables handling protected branches' do + before do + stub_licensed_features(custom_roles: true) + + sign_in(current_user) + end + + it_behaves_like 'setting project protected branches' + + it 'does not show sections not allowed by the custom role', :aggregate_failures do + expect(page).not_to have_content('Branch defaults') + expect(page).not_to have_content('Push rules') + expect(page).not_to have_content('Mirroring repositories') + expect(page).not_to have_content('Protected tags') + expect(page).not_to have_content('Deploy tokens') + expect(page).not_to have_content('Deploy keys') + expect(page).not_to have_content('Repository maintenance') + end + end +end diff --git a/ee/spec/frontend/protected_environments/add_approvers_spec.js b/ee/spec/frontend/protected_environments/add_approvers_spec.js index 2d3459d705c1df..89fe5a36b3b5d3 100644 --- a/ee/spec/frontend/protected_environments/add_approvers_spec.js +++ b/ee/spec/frontend/protected_environments/add_approvers_spec.js @@ -74,6 +74,7 @@ describe('ee/protected_environments/add_approvers.vue', () => { deploy_access_levels: { roles: [], }, + abilities: { adminProject: true }, }; mockAxios = new MockAdapter(axios); }); diff --git a/ee/spec/frontend/protected_environments/create_protected_environment_spec.js b/ee/spec/frontend/protected_environments/create_protected_environment_spec.js index 5fb170ba1c10a2..dd43acdaeb3a9b 100644 --- a/ee/spec/frontend/protected_environments/create_protected_environment_spec.js +++ b/ee/spec/frontend/protected_environments/create_protected_environment_spec.js @@ -57,6 +57,7 @@ describe('ee/protected_environments/create_protected_environment.vue', () => { deploy_access_levels: { roles: [], }, + abilities: { adminProject: true }, }; mockAxios = new MockAdapter(axios); }); diff --git a/ee/spec/frontend/protected_environments/edit_protected_environments_list_spec.js b/ee/spec/frontend/protected_environments/edit_protected_environments_list_spec.js index 125204fce5de8d..54e722544376f8 100644 --- a/ee/spec/frontend/protected_environments/edit_protected_environments_list_spec.js +++ b/ee/spec/frontend/protected_environments/edit_protected_environments_list_spec.js @@ -135,7 +135,10 @@ describe('ee/protected_environments/edit_protected_environments_list.vue', () => describe('on the project level', () => { beforeEach(() => { mock = new MockAdapter(axios); - window.gon = { api_version: 'v4' }; + window.gon = { + api_version: 'v4', + abilities: { adminProject: true }, + }; mock .onGet('/api/v4/projects/8/protected_environments/') .reply(HTTP_STATUS_OK, DEFAULT_ENVIRONMENTS); diff --git a/ee/spec/frontend/protected_environments/protected_environment_edit_spec.js b/ee/spec/frontend/protected_environments/protected_environment_edit_spec.js index 95c56b9d1c87e9..28a3a16991d0ad 100644 --- a/ee/spec/frontend/protected_environments/protected_environment_edit_spec.js +++ b/ee/spec/frontend/protected_environments/protected_environment_edit_spec.js @@ -27,6 +27,7 @@ describe('Protected Environment Edit', () => { deploy_access_levels: { roles: [], }, + abilities: { adminProject: true }, }; mockAxios = new MockAdapter(axios); }); 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 06835533292d2c..4c0ead211ec669 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 @@ -94,6 +94,22 @@ end end end + + describe 'Repository' do + let(:item_id) { :repository } + + describe 'when the user is not an admin but has `admin_protected_branch` custom ability' do + 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, :admin_protected_branch, project).and_return(true) + end + + it 'includes repository menu item' do + expect(subject.title).to eql('Repository') + end + end + end end describe 'Custom Roles' do diff --git a/ee/spec/policies/project_policy_spec.rb b/ee/spec/policies/project_policy_spec.rb index e9a34196faad7d..15640f2dabb0d6 100644 --- a/ee/spec/policies/project_policy_spec.rb +++ b/ee/spec/policies/project_policy_spec.rb @@ -3017,6 +3017,16 @@ def create_member_role(member, abilities = member_role_abilities) it_behaves_like 'custom roles abilities' end + + context 'for a member role with `admin_protected_branch` true' do + let(:member_role_abilities) { { admin_protected_branch: true } } + let(:allowed_abilities) do + [:admin_protected_branch, :read_protected_branch, :create_protected_branch, + :update_protected_branch, :destroy_protected_branch] + end + + it_behaves_like 'custom roles abilities' + end end describe 'permissions for suggested reviewers bot', :saas do diff --git a/ee/spec/requests/custom_roles/admin_protected_branch/request_spec.rb b/ee/spec/requests/custom_roles/admin_protected_branch/request_spec.rb new file mode 100644 index 00000000000000..f5a30ba7836d0b --- /dev/null +++ b/ee/spec/requests/custom_roles/admin_protected_branch/request_spec.rb @@ -0,0 +1,112 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe 'User with admin_protected_branch custom role', feature_category: :source_code_management do + include ApiHelpers + + let_it_be(:group) { create(:group) } + let_it_be(:project) { create(:project, group: group) } + let_it_be(:protected_branch) { create(:protected_branch, project: project) } + + let_it_be(:current_user) { create(:user) } + let_it_be(:role) { create(:member_role, :guest, namespace: group, admin_protected_branch: true) } + let_it_be(:membership) { create(:group_member, :guest, member_role: role, user: current_user, group: group) } + + before do + stub_licensed_features(custom_roles: true) + + sign_in(current_user) + end + + describe Projects::ProtectedBranchesController do + describe 'POST #create' do + include_context 'with correct create params' + + it 'user can create protected branch via a custom role' do + post project_protected_branches_path(project, params: { protected_branch: create_params }) + + expect(response).to have_gitlab_http_status(:found) + end + end + + describe 'PUT #update' do + include_context 'with correct update params' + + it 'user can update the protected branch via a custom role' do + put project_protected_branch_path(project, id: protected_branch, params: { protected_branch: update_params }) + + expect(response).to have_gitlab_http_status(:ok) + end + end + + describe 'DELETE #destroy' do + it 'user can destroy the protected branch via a custom role' do + delete project_protected_branch_path(project, id: protected_branch) + + expect(response).to have_gitlab_http_status(:found) + end + end + end + + describe Projects::Settings::RepositoryController do + describe 'GET show' do + it 'user can see project repository settings page via a custom role' do + get namespace_project_settings_repository_path(namespace_id: group, project_id: project) + + expect(response).to have_gitlab_http_status(:ok) + end + end + end + + describe API::ProtectedBranches do + describe "GET /projects/:id/protected_branches" do + it 'user can see a protected branches list via a custom role' do + get api("/projects/#{project.id}/protected_branches", current_user) + + expect(response).to have_gitlab_http_status(:ok) + end + end + + describe "GET /projects/:id/protected_branches/:branch" do + it 'user can see a protected branch detail via a custom role' do + get api("/projects/#{project.id}/protected_branches/#{protected_branch.name}", current_user) + + expect(response).to have_gitlab_http_status(:ok) + end + end + + describe "PATCH /projects/:id/protected_branches/:branch" do + let(:params) do + { access_level_param: + [ + { + access_level: Gitlab::Access::MAINTAINER + } + ] } + end + + it 'user can update a protected branch via a custom role' do + patch api("/projects/#{project.id}/protected_branches/#{protected_branch.name}", current_user), params: params + + expect(response).to have_gitlab_http_status(:ok) + end + end + + describe 'POST /projects/:id/protected_branches' do + it 'user can create a protected branch via a custom role' do + post api("/projects/#{project.id}/protected_branches", current_user), params: { name: 'branch_name' } + + expect(response).to have_gitlab_http_status(:created) + end + end + + describe 'DELETE /projects/:id/protected_branches/:name' do + it 'user can create a protected branch via a custom role' do + delete api("/projects/#{project.id}/protected_branches/#{protected_branch.name}", current_user) + + expect(response).to have_gitlab_http_status(:no_content) + end + end + end +end diff --git a/ee/spec/requests/ee/projects/protected_branches_controller_spec.rb b/ee/spec/requests/ee/projects/protected_branches_controller_spec.rb new file mode 100644 index 00000000000000..e231484b8d8541 --- /dev/null +++ b/ee/spec/requests/ee/projects/protected_branches_controller_spec.rb @@ -0,0 +1,95 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Projects::ProtectedBranchesController, feature_category: :source_code_management do + let_it_be(:group) { create(:group) } + let_it_be(:project) { create(:project, :repository, group: group) } + let_it_be(:protected_branch) { create(:protected_branch, project: project) } + + let_it_be(:user) { create(:user) } + let_it_be(:role) { create(:member_role, :guest, namespace: group, admin_protected_branch: true) } + let_it_be(:membership) { create(:group_member, :guest, member_role: role, user: user, group: group) } + + let(:create_request) do + post project_protected_branches_path(project, params: { protected_branch: create_params }) + end + + let(:update_request) do + put project_protected_branch_path(project, id: protected_branch, params: { protected_branch: update_params }) + end + + subject(:delete_request) { delete project_protected_branch_path(project, id: protected_branch) } + + before do + sign_in(user) + end + + context 'with custom_roles feature enabled' do + before do + stub_licensed_features(custom_roles: true) + end + + describe "POST #create" do + include_context 'with correct create params' + + it 'creates a protected branch' do + expect { create_request }.to change { ProtectedBranch.count }.by(1) + + expect(response).to have_gitlab_http_status(:found) + end + end + + describe "PUT #update" do + include_context 'with correct update params' + + it 'updates the protected branch' do + expect { update_request }.to change { protected_branch.reload.name }.to('new name') + + expect(response).to have_gitlab_http_status(:ok) + end + end + + describe "DELETE #destroy" do + it 'destroys the protected branch' do + expect { delete_request }.to change { ProtectedBranch.count }.by(-1) + + expect(response).to have_gitlab_http_status(:found) + end + end + end + + context 'with custom_roles feature disabled' do + before do + stub_licensed_features(custom_roles: false) + end + + describe "POST #create" do + include_context 'with correct create params' + + it 'does not create a protected branch' do + expect { create_request }.not_to change { ProtectedBranch.count } + + expect(response).to have_gitlab_http_status(:not_found) + end + end + + describe "PUT #update" do + include_context 'with correct update params' + + it 'does not update the protected branch' do + expect { update_request }.not_to change { protected_branch.reload.name } + + expect(response).to have_gitlab_http_status(:not_found) + end + end + + describe "DELETE #destroy" do + it 'does not destroy the protected branch' do + expect { delete_request }.not_to change { ProtectedBranch.count } + + expect(response).to have_gitlab_http_status(:not_found) + end + end + end +end diff --git a/ee/spec/support/shared_contexts/repository_settings_shared_contexts.rb b/ee/spec/support/shared_contexts/repository_settings_shared_contexts.rb new file mode 100644 index 00000000000000..f43bab74705ff1 --- /dev/null +++ b/ee/spec/support/shared_contexts/repository_settings_shared_contexts.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +RSpec.shared_context 'with correct create params' do + let(:maintainer_access_level) { [{ access_level: Gitlab::Access::MAINTAINER }] } + let(:access_level_params) do + { + merge_access_levels_attributes: maintainer_access_level, + push_access_levels_attributes: maintainer_access_level + } + end + + let(:create_params) { attributes_for(:protected_branch).merge(access_level_params) } +end + +RSpec.shared_context 'with correct update params' do + let(:update_params) { { id: protected_branch, name: 'new name' } } +end diff --git a/lib/api/helpers/protected_branches_helpers.rb b/lib/api/helpers/protected_branches_helpers.rb index 4a968ad1d6064a..c53c398a84c4bf 100644 --- a/lib/api/helpers/protected_branches_helpers.rb +++ b/lib/api/helpers/protected_branches_helpers.rb @@ -6,6 +6,18 @@ module ProtectedBranchesHelpers extend ActiveSupport::Concern extend Grape::API::Helpers + def authorize_create_protected_branch! + authorize!(:create_protected_branch, user_project) + end + + def authorize_update_protected_branch!(protected_branch) + authorize!(:update_protected_branch, protected_branch) + end + + def authorize_destroy_protected_branch!(protected_branch) + authorize!(:read_protected_branch, protected_branch) + end + params :optional_params_ee do end end diff --git a/lib/api/protected_branches.rb b/lib/api/protected_branches.rb index 60ce5ddc4f70f5..bfe496ca1c4122 100644 --- a/lib/api/protected_branches.rb +++ b/lib/api/protected_branches.rb @@ -88,7 +88,7 @@ class ProtectedBranches < ::API::Base end # rubocop: disable CodeReuse/ActiveRecord post ':id/protected_branches' do - authorize_admin_project + authorize_create_protected_branch! protected_branch = user_project.protected_branches.find_by(name: params[:name]) @@ -127,10 +127,10 @@ class ProtectedBranches < ::API::Base end # rubocop: disable CodeReuse/ActiveRecord patch ':id/protected_branches/:name', requirements: BRANCH_ENDPOINT_REQUIREMENTS do - authorize_admin_project - protected_branch = user_project.protected_branches.find_by!(name: params[:name]) + authorize_update_protected_branch!(protected_branch) + declared_params = declared_params(include_missing: false) api_service = ::ProtectedBranches::ApiService.new(user_project, current_user, declared_params) protected_branch = api_service.update(protected_branch) @@ -156,10 +156,10 @@ class ProtectedBranches < ::API::Base end # rubocop: disable CodeReuse/ActiveRecord delete ':id/protected_branches/:name', requirements: BRANCH_ENDPOINT_REQUIREMENTS, urgency: :low do - authorize_admin_project - protected_branch = user_project.protected_branches.find_by!(name: params[:name]) + authorize_destroy_protected_branch!(protected_branch) + destroy_conditionally!(protected_branch) do destroy_service = ::ProtectedBranches::DestroyService.new(user_project, current_user) destroy_service.execute(protected_branch) diff --git a/spec/features/protected_branches_spec.rb b/spec/features/protected_branches_spec.rb index 517bdada675f0e..6467a7c330d655 100644 --- a/spec/features/protected_branches_spec.rb +++ b/spec/features/protected_branches_spec.rb @@ -38,48 +38,7 @@ sign_in(user) end - it 'allows to create a protected branch with name containing HTML tags' do - visit project_protected_branches_path(project) - - show_add_form - set_defaults - set_protected_branch_name('foobar<\b>') - click_on "Protect" - - within(".protected-branches-list") { expect(page).to have_content('foobar<\b>') } - expect(ProtectedBranch.count).to eq(1) - expect(ProtectedBranch.last.name).to eq('foobar<\b>') - end - - describe 'Delete protected branch' do - before do - create(:protected_branch, project: project, name: 'fix') - expect(ProtectedBranch.count).to eq(1) - end - - it 'removes branch after modal confirmation' do - visit project_branches_path(project) - - find('input[data-testid="branch-search"]').set('fix') - find('input[data-testid="branch-search"]').native.send_keys(:enter) - - expect(page).to have_content('fix') - expect(find('.all-branches')).to have_selector('li', count: 1) - - within_testid('branch-more-actions') do - find('button').click - end - - wait_for_requests - expect(page).to have_button('Delete protected branch', disabled: false) - - find_by_testid('delete-branch-button').click - fill_in 'delete_branch_input', with: 'fix' - click_button 'Yes, delete protected branch' - - expect(page).to have_content('No branches to show') - end - end + it_behaves_like 'setting project protected branches' end context 'logged in as admin' do @@ -88,114 +47,6 @@ enable_admin_mode!(admin) end - describe "explicit protected branches" do - it "allows creating explicit protected branches" do - visit project_protected_branches_path(project) - - show_add_form - set_defaults - set_protected_branch_name('some->branch') - click_on "Protect" - - within(".protected-branches-list") { expect(page).to have_content('some->branch') } - expect(ProtectedBranch.count).to eq(1) - expect(ProtectedBranch.last.name).to eq('some->branch') - end - - it "shows success alert once protected branch is created" do - visit project_protected_branches_path(project) - - show_add_form - set_defaults - set_protected_branch_name('some->branch') - click_on "Protect" - wait_for_requests - expect(page).to have_content(s_('ProtectedBranch|View protected branches as branch rules')) - end - - it "displays the last commit on the matching branch if it exists" do - commit = create(:commit, project: project) - project.repository.add_branch(admin, 'some-branch', commit.id) - - visit project_protected_branches_path(project) - - show_add_form - set_defaults - set_protected_branch_name('some-branch') - click_on "Protect" - - within(".protected-branches-list") do - expect(page).not_to have_content("matching") - expect(page).not_to have_content("was deleted") - end - end - - it "displays an error message if the named branch does not exist" do - visit project_protected_branches_path(project) - - show_add_form - set_defaults - set_protected_branch_name('some-branch') - click_on "Protect" - - within(".protected-branches-list") { expect(page).to have_content('Branch does not exist') } - end - end - - describe "wildcard protected branches" do - it "allows creating protected branches with a wildcard" do - visit project_protected_branches_path(project) - - show_add_form - set_defaults - set_protected_branch_name('*-stable') - click_on "Protect" - - within(".protected-branches-list") { expect(page).to have_content('*-stable') } - expect(ProtectedBranch.count).to eq(1) - expect(ProtectedBranch.last.name).to eq('*-stable') - end - - it "displays the number of matching branches", - quarantine: 'https://gitlab.com/gitlab-org/quality/engineering-productivity/flaky-tests/-/issues/3459' do - project.repository.add_branch(admin, 'production-stable', 'master') - project.repository.add_branch(admin, 'staging-stable', 'master') - - visit project_protected_branches_path(project) - - show_add_form - set_defaults - set_protected_branch_name('*-stable') - click_on "Protect" - - within(".protected-branches-list") do - expect(page).to have_content("2 matching branches") - end - end - - it "displays all the branches matching the wildcard" do - project.repository.add_branch(admin, 'production-stable', 'master') - project.repository.add_branch(admin, 'staging-stable', 'master') - project.repository.add_branch(admin, 'development', 'master') - - visit project_protected_branches_path(project) - - show_add_form - set_protected_branch_name('*-stable') - set_defaults - click_on "Protect" - - visit project_protected_branches_path(project) - click_on "2 matching branches" - - within(".protected-branches-list") do - expect(page).to have_content("production-stable") - expect(page).to have_content("staging-stable") - expect(page).not_to have_content("development") - end - end - end - describe "access control" do before do stub_licensed_features(protected_refs_for_users: false) diff --git a/spec/frontend/projects/settings/components/access_dropdown_spec.js b/spec/frontend/projects/settings/components/access_dropdown_spec.js index bbc87462777be8..5cff92d63a7359 100644 --- a/spec/frontend/projects/settings/components/access_dropdown_spec.js +++ b/spec/frontend/projects/settings/components/access_dropdown_spec.js @@ -50,6 +50,10 @@ jest.mock('~/projects/settings/api/access_dropdown_api', () => ({ })); describe('Access Level Dropdown', () => { + beforeEach(() => { + window.gon = { abilities: { adminProject: true } }; + }); + let wrapper; const defaultToggleClass = '!gl-text-gray-500'; const mockAccessLevelsData = [ diff --git a/spec/frontend/protected_branches/protected_branch_create_spec.js b/spec/frontend/protected_branches/protected_branch_create_spec.js index e2a0f02e0cf775..a7e0dcc6597f02 100644 --- a/spec/frontend/protected_branches/protected_branch_create_spec.js +++ b/spec/frontend/protected_branches/protected_branch_create_spec.js @@ -17,6 +17,7 @@ describe('ProtectedBranchCreate', () => { window.gon = { merge_access_levels: { roles: [] }, push_access_levels: { roles: [] }, + abilities: { adminProject: true }, }; }); diff --git a/spec/frontend/protected_branches/protected_branch_edit_spec.js b/spec/frontend/protected_branches/protected_branch_edit_spec.js index 463354ab7e1eb3..edcc91e07f2bc1 100644 --- a/spec/frontend/protected_branches/protected_branch_edit_spec.js +++ b/spec/frontend/protected_branches/protected_branch_edit_spec.js @@ -143,6 +143,7 @@ describe('ProtectedBranchEdit', () => { current_project_id: 1, merge_access_levels: { roles: accessLevels }, push_access_levels: { roles: accessLevels }, + abilities: { adminProject: true }, }; jest.spyOn(ProtectedBranchEdit.prototype, 'initToggles').mockImplementation(); diff --git a/spec/support/shared_examples/projects/protected_branches_shared_examples.rb b/spec/support/shared_examples/projects/protected_branches_shared_examples.rb new file mode 100644 index 00000000000000..2217aef66a082f --- /dev/null +++ b/spec/support/shared_examples/projects/protected_branches_shared_examples.rb @@ -0,0 +1,100 @@ +# frozen_string_literal: true + +RSpec.shared_examples 'setting project protected branches' do + describe "explicit protected branches" do + it "allows creating explicit protected branches" do + visit project_protected_branches_path(project) + + show_add_form + set_defaults + set_protected_branch_name('some->branch') + click_on "Protect" + + within(".protected-branches-list") { expect(page).to have_content('some->branch') } + expect(ProtectedBranch.count).to eq(1) + expect(ProtectedBranch.last.name).to eq('some->branch') + end + + it "displays the last commit on the matching branch if it exists" do + commit = create(:commit, project: project) + project.repository.add_branch(admin, 'some-branch', commit.id) + + visit project_protected_branches_path(project) + + show_add_form + set_defaults + set_protected_branch_name('some-branch') + click_on "Protect" + + within(".protected-branches-list") do + expect(page).not_to have_content("matching") + expect(page).not_to have_content("was deleted") + end + end + + it "displays an error message if the named branch does not exist" do + visit project_protected_branches_path(project) + + show_add_form + set_defaults + set_protected_branch_name('some-unexisting-branch') + click_on "Protect" + + within(".protected-branches-list") { expect(page).to have_content('Branch does not exist') } + end + end + + describe "wildcard protected branches" do + it "allows creating protected branches with a wildcard" do + visit project_protected_branches_path(project) + + show_add_form + set_defaults + set_protected_branch_name('*-stable') + click_on "Protect" + + within(".protected-branches-list") { expect(page).to have_content('*-stable') } + expect(ProtectedBranch.count).to eq(1) + expect(ProtectedBranch.last.name).to eq('*-stable') + end + + it "displays the number of matching branches", + quarantine: 'https://gitlab.com/gitlab-org/quality/engineering-productivity/flaky-tests/-/issues/3459' do + project.repository.add_branch(admin, 'production-stable', 'master') + project.repository.add_branch(admin, 'staging-stable', 'master') + + visit project_protected_branches_path(project) + + show_add_form + set_defaults + set_protected_branch_name('*-stable') + click_on "Protect" + + within(".protected-branches-list") do + expect(page).to have_content("2 matching branches") + end + end + + it "displays all the branches matching the wildcard" do + project.repository.add_branch(admin, 'production-stable', 'master') + project.repository.add_branch(admin, 'staging-stable', 'master') + project.repository.add_branch(admin, 'development', 'master') + + visit project_protected_branches_path(project) + + show_add_form + set_protected_branch_name('*-stable') + set_defaults + click_on "Protect" + + visit project_protected_branches_path(project) + click_on "2 matching branches" + + within(".protected-branches-list") do + expect(page).to have_content("production-stable") + expect(page).to have_content("staging-stable") + expect(page).not_to have_content("development") + end + end + end +end -- GitLab From 31e8f23dd95dd98abd93dd86a5e2594422145b33 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jarka=20Ko=C5=A1anov=C3=A1?= Date: Thu, 15 Aug 2024 11:06:01 +0200 Subject: [PATCH 2/6] Remove access to branch rules for manage prot branches custom ability --- .../protected_branch_create.js | 16 +++++++++++++++- app/policies/projects/branch_rule_policy.rb | 10 ++++++---- app/policies/protected_branch_policy.rb | 1 + .../projects/settings/repository/show.html.haml | 2 -- locale/gitlab.pot | 3 +++ .../settings/components/access_dropdown_spec.js | 13 +++++++++++++ 6 files changed, 38 insertions(+), 7 deletions(-) diff --git a/app/assets/javascripts/protected_branches/protected_branch_create.js b/app/assets/javascripts/protected_branches/protected_branch_create.js index 612f801300e5f1..82398b021c1863 100644 --- a/app/assets/javascripts/protected_branches/protected_branch_create.js +++ b/app/assets/javascripts/protected_branches/protected_branch_create.js @@ -143,13 +143,27 @@ export default class ProtectedBranchCreate { }); } + createLimitedSuccessAlert() { + this.alert = createAlert({ + variant: VARIANT_SUCCESS, + containerSelector: '.js-alert-protected-branch-created-container', + // title: this.successMessageTitle(), + message: s__('ProtectedBranch|Protected branch was sucessfully created'), + }); + } + showSuccessAlertIfNeeded() { if (!this.hasProtectedBranchSuccessAlert()) { return; } this.expandAndScroll(PROTECTED_BRANCHES_ANCHOR); - this.createSuccessAlert(); + if (gon.abilities.adminProject) { + this.createSuccessAlert(); + } else { + this.createLimitedSuccessAlert(); + } + localStorage.removeItem(IS_PROTECTED_BRANCH_CREATED); } diff --git a/app/policies/projects/branch_rule_policy.rb b/app/policies/projects/branch_rule_policy.rb index d1a6b66eebc1a6..0094da6699256e 100644 --- a/app/policies/projects/branch_rule_policy.rb +++ b/app/policies/projects/branch_rule_policy.rb @@ -2,10 +2,12 @@ module Projects class BranchRulePolicy < ::ProtectedBranchPolicy - rule { can?(:read_protected_branch) }.enable :read_branch_rule - rule { can?(:create_protected_branch) }.enable :create_branch_rule - rule { can?(:update_protected_branch) }.enable :update_branch_rule - rule { can?(:destroy_protected_branch) }.enable :destroy_branch_rule + rule { can?(:admin_project) }.policy do + enable :read_branch_rule + enable :create_branch_rule + enable :update_branch_rule + enable :destroy_branch_rule + end end end diff --git a/app/policies/protected_branch_policy.rb b/app/policies/protected_branch_policy.rb index aeab29c5b6cb85..2be96ea7f24d46 100644 --- a/app/policies/protected_branch_policy.rb +++ b/app/policies/protected_branch_policy.rb @@ -5,6 +5,7 @@ class ProtectedBranchPolicy < BasePolicy rule { can?(:admin_project) }.policy do enable :read_protected_branch + enable :create_protected_branch enable :update_protected_branch enable :destroy_protected_branch end diff --git a/app/views/projects/settings/repository/show.html.haml b/app/views/projects/settings/repository/show.html.haml index bfefc1ecda5e7e..cf950de75db945 100644 --- a/app/views/projects/settings/repository/show.html.haml +++ b/app/views/projects/settings/repository/show.html.haml @@ -5,8 +5,6 @@ - if can?(current_user, :admin_project, @project) = render "projects/branch_defaults/show" - -- if can?(current_user, :admin_protected_branch, @project) = render "projects/branch_rules/show" - if can?(current_user, :admin_push_rules, @project) diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 67e5a792ea2644..e32fb4fc701c50 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -43647,6 +43647,9 @@ msgstr "" msgid "ProtectedBranch|Protect a branch" msgstr "" +msgid "ProtectedBranch|Protected branch was sucessfully created" +msgstr "" + msgid "ProtectedBranch|Protected branches" msgstr "" diff --git a/spec/frontend/projects/settings/components/access_dropdown_spec.js b/spec/frontend/projects/settings/components/access_dropdown_spec.js index 5cff92d63a7359..ca6198583b2fc1 100644 --- a/spec/frontend/projects/settings/components/access_dropdown_spec.js +++ b/spec/frontend/projects/settings/components/access_dropdown_spec.js @@ -125,6 +125,19 @@ describe('Access Level Dropdown', () => { }); }); + describe('withProtectedBranchesAccess', () => { + beforeEach(() => { + window.gon = { abilities: { adminProject: false, adminProtectedBranch: true } }; + }); + + it('should make an api call for users && groups when user has a license', () => { + createComponent({ groupsWithProjectAccess: true }); + expect(getUsers).toHaveBeenCalled(); + expect(getGroups).toHaveBeenCalledWith({ withProjectAccess: true }); + expect(getDeployKeys).not.toHaveBeenCalled(); + }); + }); + it('should make an api call for deployKeys but not for users or groups when user does not have a license', () => { createComponent({ hasLicense: false }); expect(getUsers).not.toHaveBeenCalled(); -- GitLab From 06d50d92703865735e52a1a6000d4e87cff8c4bd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jarka=20Ko=C5=A1anov=C3=A1?= Date: Fri, 16 Aug 2024 16:27:19 +0200 Subject: [PATCH 3/6] Push frontend abilities from controllers using access_dropdown --- .../projects/settings/components/access_dropdown.vue | 4 ++-- app/controllers/groups/settings/repository_controller.rb | 4 ++++ app/controllers/projects/settings/ci_cd_controller.rb | 2 ++ 3 files changed, 8 insertions(+), 2 deletions(-) diff --git a/app/assets/javascripts/projects/settings/components/access_dropdown.vue b/app/assets/javascripts/projects/settings/components/access_dropdown.vue index e86ba8539e09ce..35e3eb68d9d6de 100644 --- a/app/assets/javascripts/projects/settings/components/access_dropdown.vue +++ b/app/assets/javascripts/projects/settings/components/access_dropdown.vue @@ -231,7 +231,7 @@ export default { this.loading = true; if (this.hasLicense) { - if (gon.abilities.adminProject) { + if (gon.abilities.adminProject || gon.abilities.adminGroup) { Promise.all([ getDeployKeys(this.query), getUsers(this.query), @@ -306,7 +306,7 @@ export default { } } - if (gon.abilities.adminProject) { + if (gon.abilities.adminProject || gon.abilities.adminGroup) { this.deployKeys = deployKeysResponse.map((response) => { const { id, diff --git a/app/controllers/groups/settings/repository_controller.rb b/app/controllers/groups/settings/repository_controller.rb index ecd5d814fb613d..6729ea1a01409b 100644 --- a/app/controllers/groups/settings/repository_controller.rb +++ b/app/controllers/groups/settings/repository_controller.rb @@ -9,6 +9,10 @@ class RepositoryController < Groups::ApplicationController before_action :authorize_access!, only: :show before_action :define_deploy_token_variables, if: -> { can?(current_user, :create_deploy_token, @group) } + before_action do + push_frontend_ability(ability: :admin_group, resource: @group, user: current_user) + end + feature_category :continuous_delivery urgency :low diff --git a/app/controllers/projects/settings/ci_cd_controller.rb b/app/controllers/projects/settings/ci_cd_controller.rb index 21a8da551c0398..07bf98dc07e559 100644 --- a/app/controllers/projects/settings/ci_cd_controller.rb +++ b/app/controllers/projects/settings/ci_cd_controller.rb @@ -17,6 +17,8 @@ class CiCdController < Projects::ApplicationController push_frontend_feature_flag(:ci_variables_pages, current_user) push_frontend_feature_flag(:allow_push_repository_for_job_token, @project) push_frontend_feature_flag(:ci_hidden_variables, @project.root_ancestor) + + push_frontend_ability(ability: :admin_project, resource: project, user: current_user) end helper_method :highlight_badge -- GitLab From c7ec62caf1ffd0315f876b6fb4d3f2245e0915b1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jarka=20Ko=C5=A1anov=C3=A1?= Date: Wed, 21 Aug 2024 08:59:27 +0200 Subject: [PATCH 4/6] Small improvements, adding tests, fixing typos --- .../settings/components/access_dropdown.vue | 27 +++++++++---------- .../protected_branch_create.js | 3 +-- .../projects/settings/ci_cd_controller.rb | 2 +- .../settings/repository_controller.rb | 4 +-- .../projects/protected_branches_spec.rb | 2 ++ .../settings/ee/protected_branches_spec.rb | 2 ++ spec/features/protected_branches_spec.rb | 2 ++ .../protected_branches_shared_examples.rb | 11 ++++++++ 8 files changed, 33 insertions(+), 20 deletions(-) diff --git a/app/assets/javascripts/projects/settings/components/access_dropdown.vue b/app/assets/javascripts/projects/settings/components/access_dropdown.vue index 35e3eb68d9d6de..de4a58b5bff195 100644 --- a/app/assets/javascripts/projects/settings/components/access_dropdown.vue +++ b/app/assets/javascripts/projects/settings/components/access_dropdown.vue @@ -226,19 +226,21 @@ export default { focusInput() { this.$refs.search?.focusInput(); }, + canAdminContainer() { + return gon.abilities.adminProject || gon.abilities.adminGroup; + }, + getGroups() { + return this.groups.length + ? Promise.resolve({ data: this.groups }) + : getGroups({ withProjectAccess: this.groupsWithProjectAccess }); + }, getData({ initial = false } = {}) { this.initialLoading = initial; this.loading = true; if (this.hasLicense) { - if (gon.abilities.adminProject || gon.abilities.adminGroup) { - Promise.all([ - getDeployKeys(this.query), - getUsers(this.query), - this.groups.length - ? Promise.resolve({ data: this.groups }) - : getGroups({ withProjectAccess: this.groupsWithProjectAccess }), - ]) + if (this.canAdminContainer()) { + Promise.all([getDeployKeys(this.query), getUsers(this.query), this.getGroups()]) .then(([deployKeysResponse, usersResponse, groupsResponse]) => { this.consolidateData( deployKeysResponse.data, @@ -255,12 +257,7 @@ export default { this.loading = false; }); } else if (gon.abilities.adminProtectedBranch) { - Promise.all([ - getUsers(this.query), - this.groups.length - ? Promise.resolve({ data: this.groups }) - : getGroups({ withProjectAccess: this.groupsWithProjectAccess }), - ]) + Promise.all([getUsers(this.query), this.getGroups()]) .then(([usersResponse, groupsResponse]) => { this.consolidateData(null, usersResponse.data, groupsResponse.data); this.setSelected({ initial }); @@ -306,7 +303,7 @@ export default { } } - if (gon.abilities.adminProject || gon.abilities.adminGroup) { + if (this.canAdminContainer()) { this.deployKeys = deployKeysResponse.map((response) => { const { id, diff --git a/app/assets/javascripts/protected_branches/protected_branch_create.js b/app/assets/javascripts/protected_branches/protected_branch_create.js index 82398b021c1863..f74256425826e2 100644 --- a/app/assets/javascripts/protected_branches/protected_branch_create.js +++ b/app/assets/javascripts/protected_branches/protected_branch_create.js @@ -147,7 +147,6 @@ export default class ProtectedBranchCreate { this.alert = createAlert({ variant: VARIANT_SUCCESS, containerSelector: '.js-alert-protected-branch-created-container', - // title: this.successMessageTitle(), message: s__('ProtectedBranch|Protected branch was sucessfully created'), }); } @@ -158,7 +157,7 @@ export default class ProtectedBranchCreate { } this.expandAndScroll(PROTECTED_BRANCHES_ANCHOR); - if (gon.abilities.adminProject) { + if (gon.abilities.adminProject || gon.abilities.adminGroup) { this.createSuccessAlert(); } else { this.createLimitedSuccessAlert(); diff --git a/app/controllers/projects/settings/ci_cd_controller.rb b/app/controllers/projects/settings/ci_cd_controller.rb index 07bf98dc07e559..3157cfa89349bb 100644 --- a/app/controllers/projects/settings/ci_cd_controller.rb +++ b/app/controllers/projects/settings/ci_cd_controller.rb @@ -18,7 +18,7 @@ class CiCdController < Projects::ApplicationController push_frontend_feature_flag(:allow_push_repository_for_job_token, @project) push_frontend_feature_flag(:ci_hidden_variables, @project.root_ancestor) - push_frontend_ability(ability: :admin_project, resource: project, user: current_user) + push_frontend_ability(ability: :admin_project, resource: @project, user: current_user) end helper_method :highlight_badge diff --git a/app/controllers/projects/settings/repository_controller.rb b/app/controllers/projects/settings/repository_controller.rb index 5ae0d5a81a6374..ac57e1e59fa86d 100644 --- a/app/controllers/projects/settings/repository_controller.rb +++ b/app/controllers/projects/settings/repository_controller.rb @@ -9,8 +9,8 @@ class RepositoryController < Projects::ApplicationController before_action do push_frontend_feature_flag(:edit_branch_rules, @project) - push_frontend_ability(ability: :admin_project, resource: project, user: current_user) - push_frontend_ability(ability: :admin_protected_branch, resource: project, user: current_user) + push_frontend_ability(ability: :admin_project, resource: @project, user: current_user) + push_frontend_ability(ability: :admin_protected_branch, resource: @project, user: current_user) end feature_category :source_code_management, [:show, :cleanup, :update] diff --git a/ee/spec/features/projects/protected_branches_spec.rb b/ee/spec/features/projects/protected_branches_spec.rb index 60b4673aa68b34..997157c61ae7b9 100644 --- a/ee/spec/features/projects/protected_branches_spec.rb +++ b/ee/spec/features/projects/protected_branches_spec.rb @@ -13,6 +13,8 @@ let_it_be(:role) { create(:member_role, :guest, namespace: group, admin_protected_branch: true) } let_it_be(:membership) { create(:group_member, :guest, member_role: role, user: user, group: group) } + let(:success_message) { s_('ProtectedBranch|Protected branch was sucessfully created') } + before do stub_licensed_features(custom_roles: true) diff --git a/ee/spec/features/projects/settings/ee/protected_branches_spec.rb b/ee/spec/features/projects/settings/ee/protected_branches_spec.rb index ac93b7ef7c9763..5f365b6ae36db7 100644 --- a/ee/spec/features/projects/settings/ee/protected_branches_spec.rb +++ b/ee/spec/features/projects/settings/ee/protected_branches_spec.rb @@ -13,6 +13,8 @@ let_it_be(:role) { create(:member_role, :guest, namespace: group, admin_protected_branch: true) } let_it_be(:membership) { create(:group_member, :guest, member_role: role, user: current_user, group: group) } + let(:success_message) { s_('ProtectedBranch|Protected branch was sucessfully created') } + context 'when user is a guest with custom roles that enables handling protected branches' do before do stub_licensed_features(custom_roles: true) diff --git a/spec/features/protected_branches_spec.rb b/spec/features/protected_branches_spec.rb index 6467a7c330d655..b90277d2e0cab3 100644 --- a/spec/features/protected_branches_spec.rb +++ b/spec/features/protected_branches_spec.rb @@ -33,6 +33,8 @@ end context 'logged in as maintainer' do + let(:success_message) { s_('ProtectedBranch|View protected branches as branch rules') } + before do project.add_maintainer(user) sign_in(user) diff --git a/spec/support/shared_examples/projects/protected_branches_shared_examples.rb b/spec/support/shared_examples/projects/protected_branches_shared_examples.rb index 2217aef66a082f..e6ed34026d2721 100644 --- a/spec/support/shared_examples/projects/protected_branches_shared_examples.rb +++ b/spec/support/shared_examples/projects/protected_branches_shared_examples.rb @@ -15,6 +15,17 @@ expect(ProtectedBranch.last.name).to eq('some->branch') end + it "shows success alert once protected branch is created" do + visit project_protected_branches_path(project) + + show_add_form + set_defaults + set_protected_branch_name('some->branch') + click_on "Protect" + wait_for_requests + expect(page).to have_content(success_message) + end + it "displays the last commit on the matching branch if it exists" do commit = create(:commit, project: project) project.repository.add_branch(admin, 'some-branch', commit.id) -- GitLab From c18b6a931a682885e6adca61693ff00876cccbcb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jarka=20Ko=C5=A1anov=C3=A1?= Date: Wed, 21 Aug 2024 21:05:44 +0200 Subject: [PATCH 5/6] Use glAbilitiesMixin instead of gon --- .../settings/components/access_dropdown.vue | 14 ++++++++------ .../settings/components/access_dropdown_spec.js | 17 ++++++++++++----- 2 files changed, 20 insertions(+), 11 deletions(-) diff --git a/app/assets/javascripts/projects/settings/components/access_dropdown.vue b/app/assets/javascripts/projects/settings/components/access_dropdown.vue index de4a58b5bff195..0e75054a50d667 100644 --- a/app/assets/javascripts/projects/settings/components/access_dropdown.vue +++ b/app/assets/javascripts/projects/settings/components/access_dropdown.vue @@ -9,6 +9,7 @@ import { GlSprintf, } from '@gitlab/ui'; import { debounce, intersectionWith, groupBy, differenceBy, intersectionBy } from 'lodash'; +import glAbilitiesMixin from '~/vue_shared/mixins/gl_abilities_mixin'; import { createAlert } from '~/alert'; import { __, s__, n__ } from '~/locale'; import { getUsers, getGroups, getDeployKeys } from '../api/access_dropdown_api'; @@ -35,6 +36,7 @@ export default { GlAvatar, GlSprintf, }, + mixins: [glAbilitiesMixin()], props: { accessLevelsData: { type: Array, @@ -187,6 +189,9 @@ export default { ...this.getDataForSave(LEVEL_TYPES.DEPLOY_KEY, 'deploy_key_id'), ]; }, + canAdminContainer() { + return this.glAbilities.adminProject || this.glAbilities.adminGroup; + }, }, watch: { query: debounce(function debouncedSearch() { @@ -226,9 +231,6 @@ export default { focusInput() { this.$refs.search?.focusInput(); }, - canAdminContainer() { - return gon.abilities.adminProject || gon.abilities.adminGroup; - }, getGroups() { return this.groups.length ? Promise.resolve({ data: this.groups }) @@ -239,7 +241,7 @@ export default { this.loading = true; if (this.hasLicense) { - if (this.canAdminContainer()) { + if (this.canAdminContainer) { Promise.all([getDeployKeys(this.query), getUsers(this.query), this.getGroups()]) .then(([deployKeysResponse, usersResponse, groupsResponse]) => { this.consolidateData( @@ -256,7 +258,7 @@ export default { this.initialLoading = false; this.loading = false; }); - } else if (gon.abilities.adminProtectedBranch) { + } else if (this.glAbilities.adminProtectedBranch) { Promise.all([getUsers(this.query), this.getGroups()]) .then(([usersResponse, groupsResponse]) => { this.consolidateData(null, usersResponse.data, groupsResponse.data); @@ -303,7 +305,7 @@ export default { } } - if (this.canAdminContainer()) { + if (this.canAdminContainer) { this.deployKeys = deployKeysResponse.map((response) => { const { id, diff --git a/spec/frontend/projects/settings/components/access_dropdown_spec.js b/spec/frontend/projects/settings/components/access_dropdown_spec.js index ca6198583b2fc1..266593907e9850 100644 --- a/spec/frontend/projects/settings/components/access_dropdown_spec.js +++ b/spec/frontend/projects/settings/components/access_dropdown_spec.js @@ -75,9 +75,14 @@ describe('Access Level Dropdown', () => { }, ]; + const abilities = { + adminProject: true, + adminProtectedBranch: false, + }; const createComponent = ({ accessLevelsData = mockAccessLevelsData, accessLevel = ACCESS_LEVELS.PUSH, + glAbilities = abilities, stubs = {}, ...optionalProps } = {}) => { @@ -87,6 +92,9 @@ describe('Access Level Dropdown', () => { accessLevel, ...optionalProps, }, + provide: { + glAbilities, + }, stubs: { GlSprintf, GlDropdown, @@ -126,12 +134,11 @@ describe('Access Level Dropdown', () => { }); describe('withProtectedBranchesAccess', () => { - beforeEach(() => { - window.gon = { abilities: { adminProject: false, adminProtectedBranch: true } }; - }); - it('should make an api call for users && groups when user has a license', () => { - createComponent({ groupsWithProjectAccess: true }); + createComponent({ + groupsWithProjectAccess: true, + glAbilities: { adminProject: false, adminProtectedBranch: true }, + }); expect(getUsers).toHaveBeenCalled(); expect(getGroups).toHaveBeenCalledWith({ withProjectAccess: true }); expect(getDeployKeys).not.toHaveBeenCalled(); -- GitLab From 327e41fb2deaed5fc9699fe752b49998676ed6c7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jarka=20Ko=C5=A1anov=C3=A1?= Date: Wed, 28 Aug 2024 17:45:12 +0200 Subject: [PATCH 6/6] Improve specs for settings menu when custom ab taking effect --- .../projects/menus/settings_menu_spec.rb | 17 +---------------- 1 file changed, 1 insertion(+), 16 deletions(-) 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 4c0ead211ec669..1c10ec7d7c742c 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 @@ -94,22 +94,6 @@ end end end - - describe 'Repository' do - let(:item_id) { :repository } - - describe 'when the user is not an admin but has `admin_protected_branch` custom ability' do - 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, :admin_protected_branch, project).and_return(true) - end - - it 'includes repository menu item' do - expect(subject.title).to eql('Repository') - end - end - end end describe 'Custom Roles' do @@ -130,6 +114,7 @@ where(:ability, :menu_item) do :admin_cicd_variables | 'CI/CD' :admin_push_rules | 'Repository' + :admin_protected_branch | 'Repository' :admin_runners | 'CI/CD' :manage_deploy_tokens | 'Repository' :manage_merge_request_settings | 'Merge requests' -- GitLab