Protected Tags prevents all updates instead of just force pushes.

app/views/projects/protected_tags/_index.html.haml

@@ -8,8 +8,8 @@
     %p.prepend-top-20
       By default, Protected tags are designed to:
       %ul
-        %li Prevent tag pushes from everybody except Masters
-        %li Prevent <strong>anyone</strong> from force pushing to the tag
+        %li Prevent tag creation by everybody except Masters
+        %li Prevent <strong>anyone</strong> from updating the tag
         %li Prevent <strong>anyone</strong> from deleting the tag
   .col-lg-9
     - if can? current_user, :admin_project, @project

lib/gitlab/checks/change_access.rb

@@ -37,7 +37,7 @@ module Gitlab
 
         if forced_push?
           return "You are not allowed to force push code to a protected branch on this project."
-        elsif blank_ref?
+        elsif deletion?
           return "You are not allowed to delete protected branches from this project."
         end
 
@@ -62,7 +62,7 @@ module Gitlab
         return unless @tag_name
 
         if tag_exists? && user_access.cannot_do_action?(:admin_project)
-          "You are not allowed to change existing tags on this project."
+          return "You are not allowed to change existing tags on this project."
         end
 
         protected_tag_checks
@@ -71,11 +71,11 @@ module Gitlab
       def protected_tag_checks
         return unless tag_protected?
 
-        if forced_push? #TODO: Verify if this should prevent all updates, and mention in UI and documentation
+        if update?
           return "Protected tags cannot be updated."
         end
 
-        if Gitlab::Git.blank_ref?(@newrev)
+        if deletion?
           return "Protected tags cannot be deleted."
         end
 
@@ -106,7 +106,11 @@ module Gitlab
         Gitlab::Checks::ForcePush.force_push?(@project, @oldrev, @newrev, env: @env)
       end
 
-      def blank_ref?
+      def update?
+        !Gitlab::Git.blank_ref?(@oldrev) && !deletion?
+      end
+
+      def deletion?
         Gitlab::Git.blank_ref?(@newrev)
       end
 

spec/lib/gitlab/checks/change_access_spec.rb

@@ -5,13 +5,10 @@ describe Gitlab::Checks::ChangeAccess, lib: true do
     let(:user) { create(:user) }
     let(:project) { create(:project, :repository) }
     let(:user_access) { Gitlab::UserAccess.new(user, project: project) }
-    let(:changes) do
-      {
-        oldrev: 'be93687618e4b132087f430a4d8fc3a609c9b77c',
-        newrev: '54fcc214b94e78d7a41a9a8fe6d87a5e59500e51',
-        ref: 'refs/heads/master'
-      }
-    end
+    let(:oldrev) { 'be93687618e4b132087f430a4d8fc3a609c9b77c' }
+    let(:newrev) { '54fcc214b94e78d7a41a9a8fe6d87a5e59500e51' }
+    let(:ref) { 'refs/heads/master' }
+    let(:changes) { { oldrev: oldrev, newrev: newrev, ref: ref } }
     let(:protocol) { 'ssh' }
 
     subject do
@@ -41,13 +38,7 @@ describe Gitlab::Checks::ChangeAccess, lib: true do
     end
 
     context 'tags check' do
-      let(:changes) do
-        {
-          oldrev: 'be93687618e4b132087f430a4d8fc3a609c9b77c',
-          newrev: '54fcc214b94e78d7a41a9a8fe6d87a5e59500e51',
-          ref: 'refs/tags/v1.0.0'
-        }
-      end
+      let(:ref) { 'refs/tags/v1.0.0' }
 
       it 'returns an error if the user is not allowed to update tags' do
         allow(user_access).to receive(:can_do_action?).with(:push_code).and_return(true)
@@ -60,31 +51,38 @@ describe Gitlab::Checks::ChangeAccess, lib: true do
       context 'with protected tag' do
         let!(:protected_tag) { create(:protected_tag, project: project, name: 'v*') }
 
+        context 'as master' do
+          before { project.add_master(user) }
+
           context 'deletion' do
-          let(:changes) do
-            {
-              oldrev: 'be93687618e4b132087f430a4d8fc3a609c9b77c',
-              newrev: '0000000000000000000000000000000000000000',
-              ref: 'refs/tags/v1.0.0'
-            }
-          end
+            let(:oldrev) { 'be93687618e4b132087f430a4d8fc3a609c9b77c' }
+            let(:newrev) { '0000000000000000000000000000000000000000' }
 
             it 'is prevented' do
               expect(subject.status).to be(false)
-            expect(subject.message).to include('delete protected tags')
+              expect(subject.message).to include('cannot be deleted')
             end
           end
 
-        it 'prevents force push' do
-          expect(Gitlab::Checks::ForcePush).to receive(:force_push?).and_return(true)
+          context 'update' do
+            let(:oldrev) { 'be93687618e4b132087f430a4d8fc3a609c9b77c' }
+            let(:newrev) { '54fcc214b94e78d7a41a9a8fe6d87a5e59500e51' }
 
+            it 'is prevented' do
               expect(subject.status).to be(false)
-          expect(subject.message).to include('force push protected tags')
+              expect(subject.message).to include('cannot be updated')
             end
+          end
+        end
+
+        context 'creation' do
+          let(:oldrev) { '0000000000000000000000000000000000000000' }
+          let(:newrev) { '54fcc214b94e78d7a41a9a8fe6d87a5e59500e51' }
+          let(:ref) { 'refs/tags/v9.1.0' }
 
           it 'prevents creation below access level' do
             expect(subject.status).to be(false)
-          expect(subject.message).to include('allowed to')
+            expect(subject.message).to include('allowed to create this tag as it is protected')
           end
 
           context 'when user has access' do
@@ -96,6 +94,7 @@ describe Gitlab::Checks::ChangeAccess, lib: true do
           end
         end
       end
+    end
 
     context 'protected branches check' do
       before do
@@ -126,13 +125,7 @@ describe Gitlab::Checks::ChangeAccess, lib: true do
       end
 
       context 'branch deletion' do
-        let(:changes) do
-          {
-            oldrev: 'be93687618e4b132087f430a4d8fc3a609c9b77c',
-            newrev: '0000000000000000000000000000000000000000',
-            ref: 'refs/heads/master'
-          }
-        end
+        let(:newrev) { '0000000000000000000000000000000000000000' }
 
         it 'returns an error if the user is not allowed to delete protected branches' do
           expect(subject.status).to be(false)