Merge branch 'skip-deny-delete-tag-push-rule-web' into 'master'

Skip "deny_delete_tag" push rule when deleting from web UI

Closes #1889

See merge request !1406

lib/gitlab/checks/change_access.rb

@@ -3,16 +3,20 @@ module Gitlab
     class ChangeAccess
       include PathLocksHelper
 
-      attr_reader :user_access, :project, :skip_authorization
+      # protocol is currently used only in EE
+      attr_reader :user_access, :project, :skip_authorization, :protocol
 
       def initialize(
-        change, user_access:, project:, env: {}, skip_authorization: false)
+        change, user_access:, project:, env: {}, skip_authorization: false,
+        protocol:
+      )
         @oldrev, @newrev, @ref = change.values_at(:oldrev, :newrev, :ref)
         @branch_name = Gitlab::Git.branch_name(@ref)
         @user_access = user_access
         @project = project
         @env = env
         @skip_authorization = skip_authorization
+        @protocol = protocol
       end
 
       def exec
@@ -92,8 +96,8 @@ module Gitlab
 
         # Prevent tag removal
         if Gitlab::Git.tag_name(@ref)
-          if push_rule.try(:deny_delete_tag) && protected_tag?(Gitlab::Git.tag_name(@ref)) && Gitlab::Git.blank_ref?(@newrev)
-            return  "You can not delete a tag"
+          if tag_deletion_denied_by_push_rule?(push_rule)
+            return 'You cannot delete a tag'
           end
         else
           commit_validation = push_rule.try(:commit_validation?)
@@ -116,6 +120,13 @@ module Gitlab
         nil
       end
 
+      def tag_deletion_denied_by_push_rule?(push_rule)
+        push_rule.try(:deny_delete_tag) &&
+          protocol != 'web' &&
+          Gitlab::Git.blank_ref?(@newrev) &&
+          protected_tag?(Gitlab::Git.tag_name(@ref))
+      end
+
       # If commit does not pass push rule validation the whole push should be rejected.
       # This method should return nil if no error found or status object if there are some errors.
       # In case of errors - all other checks will be canceled and push will be rejected.

lib/gitlab/git_access.rb

@@ -192,7 +192,9 @@ module Gitlab
         user_access: user_access,
         project: project,
         env: @env,
-        skip_authorization: deploy_key?).exec
+        skip_authorization: deploy_key?,
+        protocol: protocol
+      ).exec
     end
 
     def deploy_key

spec/lib/gitlab/checks/change_access_spec.rb

@@ -12,8 +12,16 @@ describe Gitlab::Checks::ChangeAccess, lib: true do
         ref: 'refs/heads/master'
       }
     end
-
-    subject { described_class.new(changes, project: project, user_access: user_access).exec }
+    let(:protocol) { 'ssh' }
+
+    subject do
+      described_class.new(
+        changes,
+        project: project,
+        user_access: user_access,
+        protocol: protocol
+      ).exec
+    end
 
     before { allow(user_access).to receive(:can_do_action?).with(:push_code).and_return(true) }
 
@@ -116,7 +124,15 @@ describe Gitlab::Checks::ChangeAccess, lib: true do
 
         it 'returns an error if the rule denies tag deletion' do
           expect(subject.status).to be(false)
-          expect(subject.message).to eq('You can not delete a tag')
+          expect(subject.message).to eq('You cannot delete a tag')
+        end
+
+        context 'when tag is deleted in web UI' do
+          let(:protocol) { 'web' }
+
+          it 'ignores the push rule' do
+            expect(subject.status).to be(true)
+          end
         end
       end
 
@@ -179,7 +195,14 @@ describe Gitlab::Checks::ChangeAccess, lib: true do
 
         context 'blacklisted files check' do
           let(:push_rule) { create(:push_rule, prevent_secrets: true) }
-          let(:checker) { described_class.new(changes, project: project, user_access: user_access) }
+          let(:checker) do
+            described_class.new(
+              changes,
+              project: project,
+              user_access: user_access,
+              protocol: protocol
+            )
+          end
 
           it "returns status true if there is no blacklisted files" do
             new_rev = nil