Merge branch 'fix/tag-api-delete-auth' into 'master'

Add auth check to delete tag endpoint

See merge request https://gitlab.com/gitlab-org/gitlab/-/merge_requests/166606



Merged-by: Joe Woodward <jwoodward@gitlab.com>
Approved-by: Alex Buijs <abuijs@gitlab.com>
Approved-by: Vasilii Iakliushin <viakliushin@gitlab.com>
Reviewed-by: Vasilii Iakliushin <viakliushin@gitlab.com>
Reviewed-by: Alex Buijs <abuijs@gitlab.com>
Co-authored-by: Joe Woodward <j@joewoodward.me>

app/policies/gitlab/git/tag_policy.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Git
+    class TagPolicy < BasePolicy
+      delegate { project }
+
+      condition(:protected_tag, scope: :subject) do
+        ProtectedTag.protected?(project, @subject.name)
+      end
+
+      rule { can?(:admin_tag) & (~protected_tag | can?(:maintainer_access)) }.enable :delete_tag
+
+      def project
+        @subject.repository.container
+      end
+    end
+  end
+end

app/services/tags/destroy_service.rb

@@ -2,21 +2,17 @@
 
 module Tags
   class DestroyService < BaseService
-    def execute(tag_name)
+    def execute(tag_name, skip_find: false)
       repository = project.repository
-      tag = repository.find_tag(tag_name)
 
-      unless tag
-        return error('No such tag', 404)
-      end
+      # If we've found the tag upstream we don't need to refind it so we can
+      # pass skip_find: true
+      return error('No such tag', 404) unless skip_find || tag_exists?(tag_name)
 
       if repository.rm_tag(current_user, tag_name)
-        ##
-        # When a tag in a repository is destroyed,
-        # release assets will be destroyed too.
-        Releases::DestroyService
-          .new(project, current_user, tag: tag_name)
-          .execute
+        # When a tag in a repository is destroyed, release assets will be
+        # destroyed too.
+        destroy_releases(tag_name)
 
         unlock_artifacts(tag_name)
 
@@ -38,6 +34,14 @@ def success(message)
 
     private
 
+    def tag_exists?(tag_name)
+      repository.find_tag(tag_name)
+    end
+
+    def destroy_releases(tag_name)
+      Releases::DestroyService.new(project, current_user, tag: tag_name).execute
+    end
+
     def unlock_artifacts(tag_name)
       Ci::RefDeleteUnlockArtifactsWorker.perform_async(project.id, current_user.id, "#{::Gitlab::Git::TAG_REF_PREFIX}#{tag_name}")
     end

lib/api/tags.rb

@@ -119,6 +119,7 @@ def find_releases(tags)
       desc 'Delete a repository tag' do
         success code: 204
         failure [
+          { code: 400, message: 'Bad request' },
           { code: 403, message: 'Unauthenticated' },
           { code: 404, message: 'Not found' },
           { code: 412, message: 'Precondition failed' }
@@ -129,16 +130,14 @@ def find_releases(tags)
         requires :tag_name, type: String, desc: 'The name of the tag'
       end
       delete ':id/repository/tags/:tag_name', requirements: TAG_ENDPOINT_REQUIREMENTS, feature_category: :source_code_management do
-        authorize_admin_tag
-
         tag = user_project.repository.find_tag(params[:tag_name])
         not_found!('Tag') unless tag
+        authorize!(:delete_tag, tag)
 
         commit = user_project.repository.commit(tag.dereferenced_target)
 
         destroy_conditionally!(commit, last_updated: commit.authored_date) do
-          result = ::Tags::DestroyService.new(user_project, current_user)
-                    .execute(params[:tag_name])
+          result = ::Tags::DestroyService.new(user_project, current_user).execute(params[:tag_name], skip_find: true)
 
           if result[:status] != :success
             render_api_error!(result[:message], result[:return_code])

spec/policies/gitlab/git/tag_policy_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::Git::TagPolicy, feature_category: :source_code_management do
+  let_it_be(:project) { create(:project, :public, :repository) }
+  let_it_be(:guest) { create(:user, guest_of: project) }
+  let_it_be(:developer) { create(:user, developer_of: project) }
+  let_it_be(:maintainer) { create(:user, maintainer_of: project) }
+  let_it_be(:owner) { create(:user, owner_of: project) }
+  let_it_be(:tag) { project.repository.tags.first }
+
+  subject { described_class.new(user, tag) }
+
+  context 'when user is a project guest' do
+    let(:user) { guest }
+
+    it { is_expected.to be_disallowed(:delete_tag) }
+  end
+
+  context 'when user is a project developer' do
+    let(:user) { developer }
+
+    it { is_expected.to be_allowed(:delete_tag) }
+
+    context 'when the tag is protected' do
+      let_it_be(:protected_tag) { create(:protected_tag, project: project, name: tag.name) }
+
+      it { is_expected.to be_disallowed(:delete_tag) }
+    end
+  end
+
+  context 'when user is a project maintainer' do
+    let(:user) { maintainer }
+
+    it { is_expected.to be_allowed(:delete_tag) }
+
+    context 'when the tag is protected' do
+      let_it_be(:protected_tag) { create(:protected_tag, project: project, name: tag.name) }
+
+      it { is_expected.to be_allowed(:delete_tag) }
+    end
+  end
+
+  context 'when user is a project owner' do
+    let(:user) { owner }
+
+    it { is_expected.to be_allowed(:delete_tag) }
+
+    context 'when the tag is protected' do
+      let_it_be(:protected_tag) { create(:protected_tag, project: project, name: tag.name) }
+
+      it { is_expected.to be_allowed(:delete_tag) }
+    end
+  end
+end

spec/requests/api/tags_spec.rb

@@ -537,8 +537,16 @@
       end
     end
 
-    context 'when authenticated', 'as a maintainer' do
-      let(:current_user) { user }
+    context 'when authenticated as a guest' do
+      let(:current_user) { create(:user, guest_of: project) }
+
+      it_behaves_like '403 response' do
+        let(:request) { delete api(route, current_user) }
+      end
+    end
+
+    context 'when authenticated as a developer' do
+      let(:current_user) { create(:user, developer_of: project) }
 
       it_behaves_like 'repository delete tag'
 
@@ -547,6 +555,42 @@
 
         it_behaves_like 'repository delete tag'
       end
+
+      context 'when the tag is protected' do
+        before do
+          create(:protected_tag, project: project, name: tag_name)
+        end
+
+        it_behaves_like '403 response' do
+          let(:request) { delete api(route, current_user) }
+        end
+      end
+    end
+
+    context 'when authenticated as a maintainer' do
+      let(:current_user) { create(:user, maintainer_of: project) }
+
+      it_behaves_like 'repository delete tag'
+
+      context 'when the tag is protected' do
+        before do
+          create(:protected_tag, project: project, name: tag_name)
+        end
+
+        it_behaves_like 'repository delete tag'
+      end
+    end
+
+    context 'when authenticated as an owner' do
+      let(:current_user) { create(:user, owner_of: project) }
+
+      context 'when the tag is protected' do
+        before do
+          create(:protected_tag, project: project, name: tag_name)
+        end
+
+        it_behaves_like 'repository delete tag'
+      end
     end
   end
 

spec/services/tags/destroy_service_spec.rb

@@ -7,25 +7,46 @@
   let(:repository) { project.repository }
   let(:user) { create(:user) }
   let(:service) { described_class.new(project, user) }
+  let(:skip_find) { false }
 
-  describe '#execute' do
-    subject { service.execute(tag_name) }
+  describe '#execute(tag_name, skip_find: false)' do
+    subject(:execute) { service.execute(tag_name, skip_find: skip_find) }
 
     before do
       allow(Ci::RefDeleteUnlockArtifactsWorker).to receive(:perform_async)
     end
 
-    it 'removes the tag' do
-      expect(repository).to receive(:before_remove_tag)
-      expect(service).to receive(:success)
+    context 'with tag named v1.1.0' do
+      let(:tag_name) { 'v1.1.0' }
 
-      service.execute('v1.1.0')
-    end
+      it 'removes the tag' do
+        allow(repository).to receive(:before_remove_tag)
+        allow(service).to receive(:success)
+
+        execute
+
+        expect(repository).to have_received(:before_remove_tag)
+        expect(service).to have_received(:success)
+      end
+
+      context 'when skip_find is true' do
+        let(:skip_find) { true }
 
-    it 'calls the RefDeleteUnlockArtifactsWorker' do
-      expect(Ci::RefDeleteUnlockArtifactsWorker).to receive(:perform_async).with(project.id, user.id, 'refs/tags/v1.1.0')
+        before do
+          allow(repository).to receive(:find_tag)
+          execute
+        end
 
-      service.execute('v1.1.0')
+        it 'does not verify the tag exists in the repository' do
+          expect(repository).not_to have_received(:find_tag)
+        end
+      end
+
+      it 'calls the RefDeleteUnlockArtifactsWorker' do
+        expect(Ci::RefDeleteUnlockArtifactsWorker).to receive(:perform_async).with(project.id, user.id, "refs/tags/#{tag_name}")
+
+        execute
+      end
     end
 
     context 'when there is an associated release on the tag' do