From 53b17f030161ba2afade8fe3d41b849a7fa41a89 Mon Sep 17 00:00:00 2001 From: Manoj MJ Date: Wed, 19 Jun 2019 07:08:56 +0000 Subject: [PATCH] Add documentation and tests This commit adds - feature specs - to test the ability of a user with "developer" permission to delete tags in repositories. - documentation --- app/controllers/projects/tags_controller.rb | 3 +-- app/policies/project_policy.rb | 1 + app/views/projects/tags/_tag.html.haml | 8 +++----- app/views/projects/tags/index.html.haml | 2 +- app/views/projects/tags/show.html.haml | 4 ++-- .../unreleased/52954-allow-developers-to-delete-tags.yml | 5 +++++ doc/user/permissions.md | 2 +- lib/api/helpers.rb | 4 ++++ lib/api/tags.rb | 4 ++-- lib/gitlab/checks/tag_check.rb | 2 +- lib/gitlab/user_access.rb | 2 +- ..._creates_tag_spec.rb => developer_creates_tag_spec.rb} | 7 ++++--- ..._deletes_tag_spec.rb => developer_deletes_tag_spec.rb} | 7 ++++--- ..._updates_tag_spec.rb => developer_updates_tag_spec.rb} | 7 ++++--- ...er_views_tags_spec.rb => developer_views_tags_spec.rb} | 7 ++++--- spec/lib/gitlab/checks/tag_check_spec.rb | 5 ++--- spec/lib/gitlab/git_access_spec.rb | 2 +- spec/policies/project_policy_spec.rb | 2 +- spec/requests/api/tags_spec.rb | 2 +- .../policies/project_policy_shared_examples.rb | 1 + 20 files changed, 44 insertions(+), 33 deletions(-) create mode 100644 changelogs/unreleased/52954-allow-developers-to-delete-tags.yml rename spec/features/tags/{master_creates_tag_spec.rb => developer_creates_tag_spec.rb} (94%) rename spec/features/tags/{master_deletes_tag_spec.rb => developer_deletes_tag_spec.rb} (87%) rename spec/features/tags/{master_updates_tag_spec.rb => developer_updates_tag_spec.rb} (89%) rename spec/features/tags/{master_views_tags_spec.rb => developer_views_tags_spec.rb} (90%) diff --git a/app/controllers/projects/tags_controller.rb b/app/controllers/projects/tags_controller.rb index a17c050b696..7d9387b1d94 100644 --- a/app/controllers/projects/tags_controller.rb +++ b/app/controllers/projects/tags_controller.rb @@ -8,8 +8,7 @@ class Projects::TagsController < Projects::ApplicationController # Authorize before_action :require_non_empty_project before_action :authorize_download_code! - before_action :authorize_push_code!, only: [:new, :create] - before_action :authorize_admin_project!, only: [:destroy] + before_action :authorize_admin_tag!, only: [:new, :create, :destroy] # rubocop: disable CodeReuse/ActiveRecord def index diff --git a/app/policies/project_policy.rb b/app/policies/project_policy.rb index b3e29e775fc..08bfe5d14ee 100644 --- a/app/policies/project_policy.rb +++ b/app/policies/project_policy.rb @@ -297,6 +297,7 @@ class ProjectPolicy < BasePolicy end rule { (mirror_available & can?(:admin_project)) | admin }.enable :admin_remote_mirror + rule { can?(:push_code) }.enable :admin_tag rule { archived }.policy do prevent :push_code diff --git a/app/views/projects/tags/_tag.html.haml b/app/views/projects/tags/_tag.html.haml index 8bfface3f5a..b1432917f1d 100644 --- a/app/views/projects/tags/_tag.html.haml +++ b/app/views/projects/tags/_tag.html.haml @@ -26,10 +26,8 @@ .row-fixed-content.controls.flex-row = render 'projects/buttons/download', project: @project, ref: tag.name, pipeline: @tags_pipelines[tag.name] - - if can?(current_user, :push_code, @project) + - if can?(current_user, :admin_tag, @project) = link_to edit_project_tag_release_path(@project, tag.name), class: 'btn btn-edit has-tooltip', title: s_('TagsPage|Edit release notes'), data: { container: "body" } do = icon("pencil") - - - if can?(current_user, :admin_project, @project) - = link_to project_tag_path(@project, tag.name), class: "btn btn-remove remove-row has-tooltip prepend-left-10 #{protected_tag?(@project, tag) ? 'disabled' : ''}", title: s_('TagsPage|Delete tag'), method: :delete, data: { confirm: s_('TagsPage|Deleting the %{tag_name} tag cannot be undone. Are you sure?') % { tag_name: tag.name }, container: 'body' }, remote: true do - = icon("trash-o") + = link_to project_tag_path(@project, tag.name), class: "btn btn-remove remove-row has-tooltip prepend-left-10 #{protected_tag?(@project, tag) ? 'disabled' : ''}", title: s_('TagsPage|Delete tag'), method: :delete, data: { confirm: s_('TagsPage|Deleting the %{tag_name} tag cannot be undone. Are you sure?') % { tag_name: tag.name }, container: 'body' }, remote: true do + = icon("trash-o") diff --git a/app/views/projects/tags/index.html.haml b/app/views/projects/tags/index.html.haml index 2e78b0bff3e..1f0de1e2603 100644 --- a/app/views/projects/tags/index.html.haml +++ b/app/views/projects/tags/index.html.haml @@ -24,7 +24,7 @@ - tags_sort_options_hash.each do |value, title| %li = link_to title, filter_tags_path(sort: value), class: ("is-active" if @sort == value) - - if can?(current_user, :push_code, @project) + - if can?(current_user, :admin_tag, @project) = link_to new_project_tag_path(@project), class: 'btn btn-success new-tag-btn' do = s_('TagsPage|New tag') = link_to project_tags_path(@project, rss_url_options), title: _("Tags feed"), class: 'btn d-none d-sm-inline-block has-tooltip' do diff --git a/app/views/projects/tags/show.html.haml b/app/views/projects/tags/show.html.haml index 59232372150..02f6ef02843 100644 --- a/app/views/projects/tags/show.html.haml +++ b/app/views/projects/tags/show.html.haml @@ -19,7 +19,7 @@ = s_("TagsPage|Can't find HEAD commit for this tag") .nav-controls - - if can?(current_user, :push_code, @project) + - if can?(current_user, :admin_tag, @project) = link_to edit_project_tag_release_path(@project, @tag.name), class: 'btn btn-edit controls-item has-tooltip', title: s_('TagsPage|Edit release notes') do = icon("pencil") = link_to project_tree_path(@project, @tag.name), class: 'btn controls-item has-tooltip', title: s_('TagsPage|Browse files') do @@ -28,7 +28,7 @@ = icon('history') .btn-container.controls-item = render 'projects/buttons/download', project: @project, ref: @tag.name - - if can?(current_user, :push_code, @project) && can?(current_user, :admin_project, @project) + - if can?(current_user, :admin_tag, @project) .btn-container.controls-item-full = link_to project_tag_path(@project, @tag.name), class: "btn btn-remove remove-row has-tooltip #{protected_tag?(@project, @tag) ? 'disabled' : ''}", title: s_('TagsPage|Delete tag'), method: :delete, data: { confirm: s_('TagsPage|Deleting the %{tag_name} tag cannot be undone. Are you sure?') % { tag_name: @tag.name } } do %i.fa.fa-trash-o diff --git a/changelogs/unreleased/52954-allow-developers-to-delete-tags.yml b/changelogs/unreleased/52954-allow-developers-to-delete-tags.yml new file mode 100644 index 00000000000..38c65a67f2a --- /dev/null +++ b/changelogs/unreleased/52954-allow-developers-to-delete-tags.yml @@ -0,0 +1,5 @@ +--- +title: Allow developers to delete tags +merge_request: 29668 +author: +type: changed diff --git a/doc/user/permissions.md b/doc/user/permissions.md index 4b2dfdfc32e..7af3d4a0ac3 100644 --- a/doc/user/permissions.md +++ b/doc/user/permissions.md @@ -95,6 +95,7 @@ The following table depicts the various user permission levels in a project. | Dismiss vulnerability **[ULTIMATE]** | | | ✓ | ✓ | ✓ | | Apply code change suggestions | | | ✓ | ✓ | ✓ | | Create and edit wiki pages | | | ✓ | ✓ | ✓ | +| Rewrite/remove Git tags | | | ✓ | ✓ | ✓ | | Use environment terminals | | | | ✓ | ✓ | | Run Web IDE's Interactive Web Terminals **[ULTIMATE ONLY]** | | | | ✓ | ✓ | | Add new team members | | | | ✓ | ✓ | @@ -102,7 +103,6 @@ The following table depicts the various user permission levels in a project. | Push to protected branches | | | | ✓ | ✓ | | Turn on/off protected branch push for devs | | | | ✓ | ✓ | | Enable/disable tag protections | | | | ✓ | ✓ | -| Rewrite/remove Git tags | | | | ✓ | ✓ | | Edit project | | | | ✓ | ✓ | | Add deploy keys to project | | | | ✓ | ✓ | | Configure project hooks | | | | ✓ | ✓ | diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb index 00bcf6b055b..fd258e3edbc 100644 --- a/lib/api/helpers.rb +++ b/lib/api/helpers.rb @@ -235,6 +235,10 @@ module API authorize! :push_code, user_project end + def authorize_admin_tag + authorize! :admin_tag, user_project + end + def authorize_admin_project authorize! :admin_project, user_project end diff --git a/lib/api/tags.rb b/lib/api/tags.rb index f5359fd316c..796b1450602 100644 --- a/lib/api/tags.rb +++ b/lib/api/tags.rb @@ -55,7 +55,7 @@ module API optional :release_description, type: String, desc: 'Specifying release notes stored in the GitLab database (deprecated in GitLab 11.7)' end post ':id/repository/tags' do - authorize_push_project + authorize_admin_tag result = ::Tags::CreateService.new(user_project, current_user) .execute(params[:tag_name], params[:ref], params[:message]) @@ -87,7 +87,7 @@ module API requires :tag_name, type: String, desc: 'The name of the tag' end delete ':id/repository/tags/:tag_name', requirements: TAG_ENDPOINT_REQUIREMENTS do - authorize_push_project + authorize_admin_tag tag = user_project.repository.find_tag(params[:tag_name]) not_found!('Tag') unless tag diff --git a/lib/gitlab/checks/tag_check.rb b/lib/gitlab/checks/tag_check.rb index 2a75c8059bd..ced0612a7a3 100644 --- a/lib/gitlab/checks/tag_check.rb +++ b/lib/gitlab/checks/tag_check.rb @@ -19,7 +19,7 @@ module Gitlab return unless tag_name logger.log_timed(LOG_MESSAGES[:tag_checks]) do - if tag_exists? && user_access.cannot_do_action?(:admin_project) + if tag_exists? && user_access.cannot_do_action?(:admin_tag) raise GitAccess::UnauthorizedError, ERROR_MESSAGES[:change_existing_tags] end end diff --git a/lib/gitlab/user_access.rb b/lib/gitlab/user_access.rb index 9ef23cf849f..097b502316e 100644 --- a/lib/gitlab/user_access.rb +++ b/lib/gitlab/user_access.rb @@ -45,7 +45,7 @@ module Gitlab if protected?(ProtectedTag, project, ref) protected_tag_accessible_to?(ref, action: :create) else - user.can?(:push_code, project) + user.can?(:admin_tag, project) end end diff --git a/spec/features/tags/master_creates_tag_spec.rb b/spec/features/tags/developer_creates_tag_spec.rb similarity index 94% rename from spec/features/tags/master_creates_tag_spec.rb rename to spec/features/tags/developer_creates_tag_spec.rb index f80ddd050d7..b2ad7ed8f3f 100644 --- a/spec/features/tags/master_creates_tag_spec.rb +++ b/spec/features/tags/developer_creates_tag_spec.rb @@ -1,11 +1,12 @@ require 'spec_helper' -describe 'Maintainer creates tag' do +describe 'Developer creates tag' do let(:user) { create(:user) } - let(:project) { create(:project, :repository, namespace: user.namespace) } + let(:group) { create(:group) } + let(:project) { create(:project, :repository, namespace: group) } before do - project.add_maintainer(user) + project.add_developer(user) sign_in(user) end diff --git a/spec/features/tags/master_deletes_tag_spec.rb b/spec/features/tags/developer_deletes_tag_spec.rb similarity index 87% rename from spec/features/tags/master_deletes_tag_spec.rb rename to spec/features/tags/developer_deletes_tag_spec.rb index bdbbe645779..dc4c7a4fb0a 100644 --- a/spec/features/tags/master_deletes_tag_spec.rb +++ b/spec/features/tags/developer_deletes_tag_spec.rb @@ -1,11 +1,12 @@ require 'spec_helper' -describe 'Maintainer deletes tag' do +describe 'Developer deletes tag' do let(:user) { create(:user) } - let(:project) { create(:project, :repository, namespace: user.namespace) } + let(:group) { create(:group) } + let(:project) { create(:project, :repository, namespace: group) } before do - project.add_maintainer(user) + project.add_developer(user) sign_in(user) visit project_tags_path(project) end diff --git a/spec/features/tags/master_updates_tag_spec.rb b/spec/features/tags/developer_updates_tag_spec.rb similarity index 89% rename from spec/features/tags/master_updates_tag_spec.rb rename to spec/features/tags/developer_updates_tag_spec.rb index d8b5b3c4cc4..1e11fc9e5d5 100644 --- a/spec/features/tags/master_updates_tag_spec.rb +++ b/spec/features/tags/developer_updates_tag_spec.rb @@ -1,11 +1,12 @@ require 'spec_helper' -describe 'Maintainer updates tag' do +describe 'Developer updates tag' do let(:user) { create(:user) } - let(:project) { create(:project, :repository, namespace: user.namespace) } + let(:group) { create(:group) } + let(:project) { create(:project, :repository, namespace: group) } before do - project.add_maintainer(user) + project.add_developer(user) sign_in(user) visit project_tags_path(project) end diff --git a/spec/features/tags/master_views_tags_spec.rb b/spec/features/tags/developer_views_tags_spec.rb similarity index 90% rename from spec/features/tags/master_views_tags_spec.rb rename to spec/features/tags/developer_views_tags_spec.rb index 36cfeb5ed84..09e644c6b97 100644 --- a/spec/features/tags/master_views_tags_spec.rb +++ b/spec/features/tags/developer_views_tags_spec.rb @@ -1,7 +1,8 @@ require 'spec_helper' -describe 'Maintainer views tags' do +describe 'Developer views tags' do let(:user) { create(:user) } + let(:group) { create(:group) } before do project.add_maintainer(user) @@ -9,7 +10,7 @@ describe 'Maintainer views tags' do end context 'when project has no tags' do - let(:project) { create(:project_empty_repo) } + let(:project) { create(:project_empty_repo, namespace: group) } before do visit project_path(project) @@ -25,7 +26,7 @@ describe 'Maintainer views tags' do end context 'when project has tags' do - let(:project) { create(:project, :repository, namespace: user.namespace) } + let(:project) { create(:project, :repository, namespace: group) } let(:repository) { project.repository } before do diff --git a/spec/lib/gitlab/checks/tag_check_spec.rb b/spec/lib/gitlab/checks/tag_check_spec.rb index b1258270611..80e9eb504ad 100644 --- a/spec/lib/gitlab/checks/tag_check_spec.rb +++ b/spec/lib/gitlab/checks/tag_check_spec.rb @@ -8,9 +8,8 @@ describe Gitlab::Checks::TagCheck do describe '#validate!' do let(:ref) { 'refs/tags/v1.0.0' } - it 'raises an error' do - allow(user_access).to receive(:can_do_action?).with(:push_code).and_return(true) - expect(user_access).to receive(:can_do_action?).with(:admin_project).and_return(false) + it 'raises an error when user does not have access' do + allow(user_access).to receive(:can_do_action?).with(:admin_tag).and_return(false) expect { subject.validate! }.to raise_error(Gitlab::GitAccess::UnauthorizedError, 'You are not allowed to change existing tags on this project.') end diff --git a/spec/lib/gitlab/git_access_spec.rb b/spec/lib/gitlab/git_access_spec.rb index 634c370d211..b9c21b3a7bd 100644 --- a/spec/lib/gitlab/git_access_spec.rb +++ b/spec/lib/gitlab/git_access_spec.rb @@ -831,7 +831,7 @@ describe Gitlab::GitAccess do push_master: true, push_protected_branch: false, push_remove_protected_branch: false, - push_tag: false, + push_tag: true, push_new_tag: true, push_all: false, merge_into_protected_branch: false diff --git a/spec/policies/project_policy_spec.rb b/spec/policies/project_policy_spec.rb index 4b723a52b51..fd82150c12a 100644 --- a/spec/policies/project_policy_spec.rb +++ b/spec/policies/project_policy_spec.rb @@ -36,7 +36,7 @@ describe ProjectPolicy do let(:developer_permissions) do %i[ - admin_milestone admin_merge_request update_merge_request create_commit_status + admin_tag admin_milestone admin_merge_request update_merge_request create_commit_status update_commit_status create_build update_build create_pipeline update_pipeline create_merge_request_from create_wiki push_code resolve_note create_container_image update_container_image destroy_container_image diff --git a/spec/requests/api/tags_spec.rb b/spec/requests/api/tags_spec.rb index d898319e709..c4f4a2cb889 100644 --- a/spec/requests/api/tags_spec.rb +++ b/spec/requests/api/tags_spec.rb @@ -10,7 +10,7 @@ describe API::Tags do let(:current_user) { nil } before do - project.add_maintainer(user) + project.add_developer(user) end describe 'GET /projects/:id/repository/tags' do diff --git a/spec/support/shared_examples/policies/project_policy_shared_examples.rb b/spec/support/shared_examples/policies/project_policy_shared_examples.rb index 7a71e2ee370..13b7ade658b 100644 --- a/spec/support/shared_examples/policies/project_policy_shared_examples.rb +++ b/spec/support/shared_examples/policies/project_policy_shared_examples.rb @@ -17,6 +17,7 @@ RSpec.shared_examples 'archived project policies' do upload_file resolve_note award_emoji + admin_tag ] end