service/hook: Improve validation of input

Gitaly should return the same error for all RPCs where the
Repository input parameter is missing.
The test coverage extended to cover changed code.
The error verification is done not only for the code, but for
the message as well. It gives us confidence that a proper path
is tested.

10 files changed, 32 insertions, 18 deletions

diff --git a/internal/gitaly/service/hook/pack_objects.go b/internal/gitaly/service/hook/pack_objects.go
index 80a3f1ef2..259b16923 100644
--- a/internal/gitaly/service/hook/pack_objects.go
+++ b/internal/gitaly/service/hook/pack_objects.go
@@ -18,6 +18,7 @@ import (
 	"github.com/prometheus/client_golang/prometheus"
 	"github.com/prometheus/client_golang/prometheus/promauto"
 	"github.com/sirupsen/logrus"
+	gitalyerrors "gitlab.com/gitlab-org/gitaly/v15/internal/errors"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/pktline"
 	gitalyhook "gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/hook"
@@ -370,7 +371,7 @@ func bufferStdin(r io.Reader, h hash.Hash) (_ io.ReadCloser, err error) {
 
 func (s *server) PackObjectsHookWithSidechannel(ctx context.Context, req *gitalypb.PackObjectsHookWithSidechannelRequest) (*gitalypb.PackObjectsHookWithSidechannelResponse, error) {
 	if req.GetRepository() == nil {
-		return nil, helper.ErrInvalidArgument(errors.New("repository is empty"))
+		return nil, helper.ErrInvalidArgument(gitalyerrors.ErrEmptyRepository)
 	}
 
 	args, err := parsePackObjectsArgs(req.Args)
diff --git a/internal/gitaly/service/hook/pack_objects_test.go b/internal/gitaly/service/hook/pack_objects_test.go
index 21ce70c5b..da2ff8289 100644
--- a/internal/gitaly/service/hook/pack_objects_test.go
+++ b/internal/gitaly/service/hook/pack_objects_test.go
@@ -31,6 +31,7 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper/testserver"
 	"gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb"
 	"google.golang.org/grpc/codes"
+	"google.golang.org/grpc/status"
 )
 
 func runTestsWithRuntimeDir(t *testing.T, testFunc func(*testing.T, context.Context, string)) func(*testing.T, context.Context) {
@@ -502,20 +503,24 @@ func TestServer_PackObjectsHookWithSidechannel_invalidArgument(t *testing.T) {
 	})
 
 	testCases := []struct {
-		desc string
-		req  *gitalypb.PackObjectsHookWithSidechannelRequest
+		desc        string
+		req         *gitalypb.PackObjectsHookWithSidechannelRequest
+		expectedErr error
 	}{
 		{
-			desc: "empty",
-			req:  &gitalypb.PackObjectsHookWithSidechannelRequest{},
+			desc:        "empty",
+			req:         &gitalypb.PackObjectsHookWithSidechannelRequest{},
+			expectedErr: status.Error(codes.InvalidArgument, "empty Repository"),
 		},
 		{
-			desc: "repo, no args",
-			req:  &gitalypb.PackObjectsHookWithSidechannelRequest{Repository: repo},
+			desc:        "repo, no args",
+			req:         &gitalypb.PackObjectsHookWithSidechannelRequest{Repository: repo},
+			expectedErr: status.Error(codes.InvalidArgument, "invalid pack-objects command: []: missing pack-objects"),
 		},
 		{
-			desc: "repo, bad args",
-			req:  &gitalypb.PackObjectsHookWithSidechannelRequest{Repository: repo, Args: []string{"rm", "-rf"}},
+			desc:        "repo, bad args",
+			req:         &gitalypb.PackObjectsHookWithSidechannelRequest{Repository: repo, Args: []string{"rm", "-rf"}},
+			expectedErr: status.Error(codes.InvalidArgument, "invalid pack-objects command: [rm -rf]: missing pack-objects"),
 		},
 	}
 
@@ -525,7 +530,7 @@ func TestServer_PackObjectsHookWithSidechannel_invalidArgument(t *testing.T) {
 			defer conn.Close()
 
 			_, err := client.PackObjectsHookWithSidechannel(ctx, tc.req)
-			testhelper.RequireGrpcCode(t, err, codes.InvalidArgument)
+			testhelper.RequireGrpcError(t, tc.expectedErr, err)
 		})
 	}
 }
diff --git a/internal/gitaly/service/hook/post_receive.go b/internal/gitaly/service/hook/post_receive.go
index 88e2d15a0..713c65849 100644
--- a/internal/gitaly/service/hook/post_receive.go
+++ b/internal/gitaly/service/hook/post_receive.go
@@ -6,6 +6,7 @@ import (
 	"os/exec"
 	"sync"
 
+	gitalyerrors "gitlab.com/gitlab-org/gitaly/v15/internal/errors"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/helper"
 	"gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb"
 	"gitlab.com/gitlab-org/gitaly/v15/streamio"
@@ -67,7 +68,7 @@ func (s *server) PostReceiveHook(stream gitalypb.HookService_PostReceiveHookServ
 
 func validatePostReceiveHookRequest(in *gitalypb.PostReceiveHookRequest) error {
 	if in.GetRepository() == nil {
-		return errors.New("repository is empty")
+		return gitalyerrors.ErrEmptyRepository
 	}
 
 	return nil
diff --git a/internal/gitaly/service/hook/post_receive_test.go b/internal/gitaly/service/hook/post_receive_test.go
index 3e1c4ca8c..58bcdc839 100644
--- a/internal/gitaly/service/hook/post_receive_test.go
+++ b/internal/gitaly/service/hook/post_receive_test.go
@@ -23,6 +23,7 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb"
 	"gitlab.com/gitlab-org/gitaly/v15/streamio"
 	"google.golang.org/grpc/codes"
+	"google.golang.org/grpc/status"
 )
 
 func TestPostReceiveInvalidArgument(t *testing.T) {
@@ -34,7 +35,7 @@ func TestPostReceiveInvalidArgument(t *testing.T) {
 	require.NoError(t, stream.Send(&gitalypb.PostReceiveHookRequest{}), "empty repository should result in an error")
 	_, err = stream.Recv()
 
-	testhelper.RequireGrpcCode(t, err, codes.InvalidArgument)
+	testhelper.RequireGrpcError(t, status.Error(codes.InvalidArgument, "empty Repository"), err)
 }
 
 func TestHooksMissingStdin(t *testing.T) {
diff --git a/internal/gitaly/service/hook/pre_receive.go b/internal/gitaly/service/hook/pre_receive.go
index 442af0e26..e30741737 100644
--- a/internal/gitaly/service/hook/pre_receive.go
+++ b/internal/gitaly/service/hook/pre_receive.go
@@ -6,6 +6,7 @@ import (
 	"os/exec"
 	"sync"
 
+	gitalyerrors "gitlab.com/gitlab-org/gitaly/v15/internal/errors"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/helper"
 	"gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb"
 	"gitlab.com/gitlab-org/gitaly/v15/streamio"
@@ -57,7 +58,7 @@ func (s *server) PreReceiveHook(stream gitalypb.HookService_PreReceiveHookServer
 
 func validatePreReceiveHookRequest(in *gitalypb.PreReceiveHookRequest) error {
 	if in.GetRepository() == nil {
-		return errors.New("repository is empty")
+		return gitalyerrors.ErrEmptyRepository
 	}
 
 	return nil
diff --git a/internal/gitaly/service/hook/pre_receive_test.go b/internal/gitaly/service/hook/pre_receive_test.go
index 791d51eaf..b1f64d6a7 100644
--- a/internal/gitaly/service/hook/pre_receive_test.go
+++ b/internal/gitaly/service/hook/pre_receive_test.go
@@ -27,6 +27,7 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb"
 	"gitlab.com/gitlab-org/gitaly/v15/streamio"
 	"google.golang.org/grpc/codes"
+	"google.golang.org/grpc/status"
 )
 
 func TestPreReceiveInvalidArgument(t *testing.T) {
@@ -38,7 +39,7 @@ func TestPreReceiveInvalidArgument(t *testing.T) {
 	require.NoError(t, stream.Send(&gitalypb.PreReceiveHookRequest{}))
 	_, err = stream.Recv()
 
-	testhelper.RequireGrpcCode(t, err, codes.InvalidArgument)
+	testhelper.RequireGrpcError(t, status.Error(codes.InvalidArgument, "empty Repository"), err)
 }
 
 func sendPreReceiveHookRequest(t *testing.T, stream gitalypb.HookService_PreReceiveHookClient, stdin io.Reader) ([]byte, []byte, int32, error) {
diff --git a/internal/gitaly/service/hook/reference_transaction.go b/internal/gitaly/service/hook/reference_transaction.go
index e01841587..8078b5541 100644
--- a/internal/gitaly/service/hook/reference_transaction.go
+++ b/internal/gitaly/service/hook/reference_transaction.go
@@ -3,6 +3,7 @@ package hook
 import (
 	"errors"
 
+	gitalyerrors "gitlab.com/gitlab-org/gitaly/v15/internal/errors"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/hook"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/transaction"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/helper"
@@ -14,7 +15,7 @@ import (
 
 func validateReferenceTransactionHookRequest(in *gitalypb.ReferenceTransactionHookRequest) error {
 	if in.GetRepository() == nil {
-		return errors.New("repository is empty")
+		return gitalyerrors.ErrEmptyRepository
 	}
 
 	return nil
diff --git a/internal/gitaly/service/hook/reference_transaction_test.go b/internal/gitaly/service/hook/reference_transaction_test.go
index ca038bd21..d75a51429 100644
--- a/internal/gitaly/service/hook/reference_transaction_test.go
+++ b/internal/gitaly/service/hook/reference_transaction_test.go
@@ -22,6 +22,7 @@ import (
 	"google.golang.org/grpc"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/credentials/insecure"
+	"google.golang.org/grpc/status"
 )
 
 type testTransactionServer struct {
@@ -49,7 +50,7 @@ func TestReferenceTransactionHookInvalidArgument(t *testing.T) {
 	require.NoError(t, stream.Send(&gitalypb.ReferenceTransactionHookRequest{}))
 	_, err = stream.Recv()
 
-	testhelper.RequireGrpcCode(t, err, codes.InvalidArgument)
+	testhelper.RequireGrpcError(t, status.Error(codes.InvalidArgument, "empty Repository"), err)
 }
 
 func TestReferenceTransactionHook(t *testing.T) {
diff --git a/internal/gitaly/service/hook/update.go b/internal/gitaly/service/hook/update.go
index 81f0a6a46..dcbd032b7 100644
--- a/internal/gitaly/service/hook/update.go
+++ b/internal/gitaly/service/hook/update.go
@@ -5,6 +5,7 @@ import (
 	"os/exec"
 	"sync"
 
+	gitalyerrors "gitlab.com/gitlab-org/gitaly/v15/internal/errors"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/helper"
 	"gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb"
 	"gitlab.com/gitlab-org/gitaly/v15/streamio"
@@ -12,7 +13,7 @@ import (
 
 func validateUpdateHookRequest(in *gitalypb.UpdateHookRequest) error {
 	if in.GetRepository() == nil {
-		return errors.New("repository is empty")
+		return gitalyerrors.ErrEmptyRepository
 	}
 
 	return nil
diff --git a/internal/gitaly/service/hook/update_test.go b/internal/gitaly/service/hook/update_test.go
index b5e5b2994..5854366ce 100644
--- a/internal/gitaly/service/hook/update_test.go
+++ b/internal/gitaly/service/hook/update_test.go
@@ -19,6 +19,7 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper/testcfg"
 	"gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb"
 	"google.golang.org/grpc/codes"
+	"google.golang.org/grpc/status"
 )
 
 func TestUpdateInvalidArgument(t *testing.T) {
@@ -32,7 +33,7 @@ func TestUpdateInvalidArgument(t *testing.T) {
 	require.NoError(t, err)
 	_, err = stream.Recv()
 
-	testhelper.RequireGrpcCode(t, err, codes.InvalidArgument)
+	testhelper.RequireGrpcError(t, status.Error(codes.InvalidArgument, "empty Repository"), err)
 }
 
 func TestUpdate_CustomHooks(t *testing.T) {