Merge branch '24813-project-members-with-developer-access-can-no-longer-create-tags' into 'master'

Create tag after running pre-hooks and pass updated SHA to post-hooks

Closes #24813

See merge request !7700

4 files changed, 34 insertions, 30 deletions

diff --git a/app/models/repository.rb b/app/models/repository.rb
index bf136ccdb6c..e2e7d08abac 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -196,18 +196,12 @@ class Repository
 
     options = { message: message, tagger: user_to_committer(user) } if message
 
-    rugged.tags.create(tag_name, target, options)
-    tag = find_tag(tag_name)
-
-    GitHooksService.new.execute(user, path_to_repo, oldrev, tag.target, ref) do
-      # we already created a tag, because we need tag SHA to pass correct
-      # values to hooks
+    GitHooksService.new.execute(user, path_to_repo, oldrev, target, ref) do |service|
+      raw_tag = rugged.tags.create(tag_name, target, options)
+      service.newrev = raw_tag.target_id
     end
 
-    tag
-  rescue GitHooksService::PreReceiveError
-    rugged.tags.delete(tag_name)
-    raise
+    find_tag(tag_name)
   end
 
   def rm_branch(user, branch_name)
diff --git a/app/services/git_hooks_service.rb b/app/services/git_hooks_service.rb
index 172bd85dade..6cd3908d43a 100644
--- a/app/services/git_hooks_service.rb
+++ b/app/services/git_hooks_service.rb
@@ -1,6 +1,8 @@
 class GitHooksService
   PreReceiveError = Class.new(StandardError)
 
+  attr_accessor :oldrev, :newrev, :ref
+
   def execute(user, repo_path, oldrev, newrev, ref)
     @repo_path  = repo_path
     @user       = Gitlab::GlId.gl_id(user)
@@ -16,7 +18,7 @@ class GitHooksService
       end
     end
 
-    yield
+    yield self
 
     run_hook('post-receive')
   end
@@ -25,6 +27,6 @@ class GitHooksService
 
   def run_hook(name)
     hook = Gitlab::Git::Hook.new(name, @repo_path)
-    hook.trigger(@user, @oldrev, @newrev, @ref)
+    hook.trigger(@user, oldrev, newrev, ref)
   end
 end
diff --git a/changelogs/unreleased/24813-project-members-with-developer-access-can-no-longer-create-tags.yml b/changelogs/unreleased/24813-project-members-with-developer-access-can-no-longer-create-tags.yml
new file mode 100644
index 00000000000..9254db40742
--- /dev/null
+++ b/changelogs/unreleased/24813-project-members-with-developer-access-can-no-longer-create-tags.yml
@@ -0,0 +1,4 @@
+---
+title: Pass tag SHA to post-receive hook when tag is created via UI
+merge_request: 7700
+author:
diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb
index 04afb8ebc98..b797d19161d 100644
--- a/spec/models/repository_spec.rb
+++ b/spec/models/repository_spec.rb
@@ -1303,32 +1303,36 @@ describe Repository, models: true do
         repository.add_tag(user, '8.5', 'master', 'foo')
       end
 
-      it 'does not create a tag when a pre-hook fails' do
-        allow_any_instance_of(Gitlab::Git::Hook).to receive(:trigger).and_return([false, ''])
-
-        expect do
-          repository.add_tag(user, '8.5', 'master', 'foo')
-        end.to raise_error(GitHooksService::PreReceiveError)
+      it 'returns a Gitlab::Git::Tag object' do
+        tag = repository.add_tag(user, '8.5', 'master', 'foo')
 
-        repository.expire_tags_cache
-        expect(repository.find_tag('8.5')).to be_nil
+        expect(tag).to be_a(Gitlab::Git::Tag)
       end
 
-      it 'passes tag SHA to hooks' do
-        spy = GitHooksService.new
-        allow(GitHooksService).to receive(:new).and_return(spy)
-        allow(spy).to receive(:execute).and_call_original
+      it 'passes commit SHA to pre-receive and update hooks,\
+        and tag SHA to post-receive hook' do
+        pre_receive_hook = Gitlab::Git::Hook.new('pre-receive', repository.path_to_repo)
+        update_hook = Gitlab::Git::Hook.new('update', repository.path_to_repo)
+        post_receive_hook = Gitlab::Git::Hook.new('post-receive', repository.path_to_repo)
 
-        tag = repository.add_tag(user, '8.5', 'master', 'foo')
+        allow(Gitlab::Git::Hook).to receive(:new).
+          and_return(pre_receive_hook, update_hook, post_receive_hook)
 
-        expect(spy).to have_received(:execute).
-          with(anything, anything, anything, tag.target, anything)
-      end
+        allow(pre_receive_hook).to receive(:trigger).and_call_original
+        allow(update_hook).to receive(:trigger).and_call_original
+        allow(post_receive_hook).to receive(:trigger).and_call_original
 
-      it 'returns a Gitlab::Git::Tag object' do
         tag = repository.add_tag(user, '8.5', 'master', 'foo')
 
-        expect(tag).to be_a(Gitlab::Git::Tag)
+        commit_sha = repository.commit('master').id
+        tag_sha = tag.target
+
+        expect(pre_receive_hook).to have_received(:trigger).
+          with(anything, anything, commit_sha, anything)
+        expect(update_hook).to have_received(:trigger).
+          with(anything, anything, commit_sha, anything)
+        expect(post_receive_hook).to have_received(:trigger).
+          with(anything, anything, tag_sha, anything)
       end
     end