Protected Tags prevents all updates instead of just force pushes.

This only changes behaviour for masters, as developers are already prevented from updating/deleting tags without the Protected Tags feature

3 files changed, 49 insertions, 52 deletions

diff --git a/app/views/projects/protected_tags/_index.html.haml b/app/views/projects/protected_tags/_index.html.haml
index 591d64ae7de..0bfb1ad191d 100644
--- a/app/views/projects/protected_tags/_index.html.haml
+++ b/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
diff --git a/lib/gitlab/checks/change_access.rb b/lib/gitlab/checks/change_access.rb
index 07fd4024346..d0bbd713710 100644
--- a/lib/gitlab/checks/change_access.rb
+++ b/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
 
diff --git a/spec/lib/gitlab/checks/change_access_spec.rb b/spec/lib/gitlab/checks/change_access_spec.rb
index 8525422908b..afc29baa7e6 100644
--- a/spec/lib/gitlab/checks/change_access_spec.rb
+++ b/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,38 +51,46 @@ describe Gitlab::Checks::ChangeAccess, lib: true do
       context 'with protected tag' do
         let!(:protected_tag) { create(:protected_tag, project: project, name: 'v*') }
 
-        context 'deletion' do
-          let(:changes) do
-            {
-              oldrev: 'be93687618e4b132087f430a4d8fc3a609c9b77c',
-              newrev: '0000000000000000000000000000000000000000',
-              ref: 'refs/tags/v1.0.0'
-            }
-          end
+        context 'as master' do
+          before { project.add_master(user) }
 
-          it 'is prevented' do
-            expect(subject.status).to be(false)
-            expect(subject.message).to include('delete protected tags')
+          context 'deletion' do
+            let(:oldrev) { 'be93687618e4b132087f430a4d8fc3a609c9b77c' }
+            let(:newrev) { '0000000000000000000000000000000000000000' }
+
+            it 'is prevented' do
+              expect(subject.status).to be(false)
+              expect(subject.message).to include('cannot be deleted')
+            end
           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' }
 
-          expect(subject.status).to be(false)
-          expect(subject.message).to include('force push protected tags')
+            it 'is prevented' do
+              expect(subject.status).to be(false)
+              expect(subject.message).to include('cannot be updated')
+            end
+          end
         end
 
-        it 'prevents creation below access level' do
-          expect(subject.status).to be(false)
-          expect(subject.message).to include('allowed to')
-        end
+        context 'creation' do
+          let(:oldrev) { '0000000000000000000000000000000000000000' }
+          let(:newrev) { '54fcc214b94e78d7a41a9a8fe6d87a5e59500e51' }
+          let(:ref) { 'refs/tags/v9.1.0' }
 
-        context 'when user has access' do
-          let!(:protected_tag) { create(:protected_tag, :developers_can_push, project: project, name: 'v*') }
+          it 'prevents creation below access level' do
+            expect(subject.status).to be(false)
+            expect(subject.message).to include('allowed to create this tag as it is protected')
+          end
+
+          context 'when user has access' do
+            let!(:protected_tag) { create(:protected_tag, :developers_can_push, project: project, name: 'v*') }
 
-          it 'allows tag creation' do
-            expect(subject.status).to be(true)
+            it 'allows tag creation' do
+              expect(subject.status).to be(true)
+            end
           end
         end
       end
@@ -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)