Merge branch 'sh-fix-merge-internal-allowed-failure' into 'master'

Fix internal API errors not being passed back to UserMergeBranch

Closes #3404

See merge request gitlab-org/gitaly!2987

6 files changed, 42 insertions, 15 deletions

diff --git a/changelogs/unreleased/sh-fix-merge-internal-allowed-failure.yml b/changelogs/unreleased/sh-fix-merge-internal-allowed-failure.yml
new file mode 100644
index 000000000..c45333a0c
--- /dev/null
+++ b/changelogs/unreleased/sh-fix-merge-internal-allowed-failure.yml
@@ -0,0 +1,5 @@
+---
+title: Fix internal API errors not being passed back to UserMergeBranch
+merge_request: 2987
+author:
+type: fixed
diff --git a/internal/gitaly/hook/prereceive.go b/internal/gitaly/hook/prereceive.go
index 35e47c1a5..8e5ce6f9c 100644
--- a/internal/gitaly/hook/prereceive.go
+++ b/internal/gitaly/hook/prereceive.go
@@ -15,6 +15,16 @@ import (
 	"gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
 )
 
+// NotAllowedError is needed to report internal API errors that
+// are made by the pre-receive hook.
+type NotAllowedError struct {
+	Message string
+}
+
+func (e NotAllowedError) Error() string {
+	return e.Message
+}
+
 func getRelativeObjectDirs(repoPath, gitObjectDir, gitAlternateObjectDirs string) (string, []string, error) {
 	repoPathReal, err := filepath.EvalSymlinks(repoPath)
 	if err != nil {
@@ -112,11 +122,10 @@ func (m *GitLabHookManager) preReceiveHook(ctx context.Context, payload git.Hook
 
 	allowed, message, err := m.gitlabAPI.Allowed(ctx, params)
 	if err != nil {
-		return fmt.Errorf("GitLab: %v", err)
+		return NotAllowedError{Message: fmt.Sprintf("GitLab: %v", err)}
 	}
-
 	if !allowed {
-		return errors.New(message)
+		return NotAllowedError{Message: message}
 	}
 
 	executor, err := m.newCustomHooksExecutor(repo, "pre-receive")
diff --git a/internal/gitaly/hook/prereceive_test.go b/internal/gitaly/hook/prereceive_test.go
index 92a47f228..d8419511f 100644
--- a/internal/gitaly/hook/prereceive_test.go
+++ b/internal/gitaly/hook/prereceive_test.go
@@ -247,7 +247,7 @@ func TestPrereceive_gitlab(t *testing.T) {
 				return false, "you shall not pass", nil
 			},
 			expectHookCall: false,
-			expectedErr:    errors.New("you shall not pass"),
+			expectedErr:    NotAllowedError{Message: "you shall not pass"},
 		},
 		{
 			desc:    "allowed returns error",
@@ -257,7 +257,7 @@ func TestPrereceive_gitlab(t *testing.T) {
 				return false, "", errors.New("oops")
 			},
 			expectHookCall: false,
-			expectedErr:    errors.New("GitLab: oops"),
+			expectedErr:    NotAllowedError{Message: "GitLab: oops"},
 		},
 		{
 			desc:    "prereceive rejects",
diff --git a/internal/gitaly/service/operations/merge.go b/internal/gitaly/service/operations/merge.go
index 6dd16f82a..25e8dc533 100644
--- a/internal/gitaly/service/operations/merge.go
+++ b/internal/gitaly/service/operations/merge.go
@@ -89,13 +89,6 @@ func validateMergeBranchRequest(request *gitalypb.UserMergeBranchRequest) error
 	return nil
 }
 
-func hookErrorFromStdoutAndStderr(sout string, serr string) string {
-	if len(strings.TrimSpace(serr)) > 0 {
-		return serr
-	}
-	return sout
-}
-
 func (s *Server) userMergeBranch(stream gitalypb.OperationService_UserMergeBranchServer) error {
 	ctx := stream.Context()
 
diff --git a/internal/gitaly/service/operations/update_with_hooks.go b/internal/gitaly/service/operations/update_with_hooks.go
index 55021714b..2d023a1ae 100644
--- a/internal/gitaly/service/operations/update_with_hooks.go
+++ b/internal/gitaly/service/operations/update_with_hooks.go
@@ -3,6 +3,7 @@ package operations
 import (
 	"bytes"
 	"context"
+	"errors"
 	"fmt"
 	"strings"
 
@@ -30,6 +31,18 @@ func (e updateRefError) Error() string {
 	return fmt.Sprintf("Could not update %s. Please refresh and try again.", e.reference)
 }
 
+func hookErrorMessage(sout string, serr string, err error) string {
+	if err != nil && errors.As(err, &hook.NotAllowedError{}) {
+		return err.Error()
+	}
+
+	if len(strings.TrimSpace(serr)) > 0 {
+		return serr
+	}
+
+	return sout
+}
+
 func (s *Server) updateReferenceWithHooks(ctx context.Context, repo *gitalypb.Repository, user *gitalypb.User, reference, newrev, oldrev string) error {
 	transaction, praefect, err := metadata.TransactionMetadataFromContext(ctx)
 	if err != nil {
@@ -63,11 +76,11 @@ func (s *Server) updateReferenceWithHooks(ctx context.Context, repo *gitalypb.Re
 	var stdout, stderr bytes.Buffer
 
 	if err := s.hookManager.PreReceiveHook(ctx, repo, nil, env, strings.NewReader(changes), &stdout, &stderr); err != nil {
-		msg := hookErrorFromStdoutAndStderr(stdout.String(), stderr.String())
+		msg := hookErrorMessage(stdout.String(), stderr.String(), err)
 		return preReceiveError{message: msg}
 	}
 	if err := s.hookManager.UpdateHook(ctx, repo, reference, oldrev, newrev, env, &stdout, &stderr); err != nil {
-		msg := hookErrorFromStdoutAndStderr(stdout.String(), stderr.String())
+		msg := hookErrorMessage(stdout.String(), stderr.String(), err)
 		return preReceiveError{message: msg}
 	}
 
@@ -89,7 +102,7 @@ func (s *Server) updateReferenceWithHooks(ctx context.Context, repo *gitalypb.Re
 	}
 
 	if err := s.hookManager.PostReceiveHook(ctx, repo, nil, env, strings.NewReader(changes), &stdout, &stderr); err != nil {
-		msg := hookErrorFromStdoutAndStderr(stdout.String(), stderr.String())
+		msg := hookErrorMessage(stdout.String(), stderr.String(), err)
 		return preReceiveError{message: msg}
 	}
 
diff --git a/internal/gitaly/service/operations/update_with_hooks_test.go b/internal/gitaly/service/operations/update_with_hooks_test.go
index 39d532372..3c3813f43 100644
--- a/internal/gitaly/service/operations/update_with_hooks_test.go
+++ b/internal/gitaly/service/operations/update_with_hooks_test.go
@@ -196,6 +196,13 @@ func TestUpdateReferenceWithHooks(t *testing.T) {
 			expectedErr: "prereceive failure",
 		},
 		{
+			desc: "prereceive error from GitLab API response",
+			preReceive: func(t *testing.T, ctx context.Context, repo *gitalypb.Repository, pushOptions, env []string, stdin io.Reader, stdout, stderr io.Writer) error {
+				return hook.NotAllowedError{Message: "GitLab: file is locked"}
+			},
+			expectedErr: "GitLab: file is locked",
+		},
+		{
 			desc: "update error",
 			preReceive: func(t *testing.T, ctx context.Context, repo *gitalypb.Repository, pushOptions, env []string, stdin io.Reader, stdout, stderr io.Writer) error {
 				return nil