Merge branch '52954-allow-developers-to-delete-tags' into 'master'

Allow developers to delete tags

Closes #52954

See merge request gitlab-org/gitlab-ce!29668

20 files changed, 44 insertions, 33 deletions

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
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
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
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
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