service/ssh: 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.

6 files changed, 28 insertions, 13 deletions

diff --git a/internal/gitaly/service/ssh/receive_pack.go b/internal/gitaly/service/ssh/receive_pack.go
index 1dbddee99..207c80f82 100644
--- a/internal/gitaly/service/ssh/receive_pack.go
+++ b/internal/gitaly/service/ssh/receive_pack.go
@@ -10,6 +10,7 @@ import (
 	"github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus/ctxlogrus"
 	log "github.com/sirupsen/logrus"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/command"
+	gitalyerrors "gitlab.com/gitlab-org/gitaly/v15/internal/errors"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/transaction"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/helper"
@@ -146,7 +147,7 @@ func validateFirstReceivePackRequest(req *gitalypb.SSHReceivePackRequest) error
 		return errors.New("non-empty data in first request")
 	}
 	if req.Repository == nil {
-		return errors.New("repository is empty")
+		return gitalyerrors.ErrEmptyRepository
 	}
 
 	return nil
diff --git a/internal/gitaly/service/ssh/receive_pack_test.go b/internal/gitaly/service/ssh/receive_pack_test.go
index 87c5098fe..9a777dd64 100644
--- a/internal/gitaly/service/ssh/receive_pack_test.go
+++ b/internal/gitaly/service/ssh/receive_pack_test.go
@@ -81,7 +81,7 @@ func TestReceivePack_validation(t *testing.T) {
 					return helper.ErrInvalidArgumentf("repo scoped: empty Repository")
 				}
 
-				return helper.ErrInvalidArgumentf("repository is empty")
+				return helper.ErrInvalidArgumentf("empty Repository")
 			}(),
 		},
 		{
diff --git a/internal/gitaly/service/ssh/upload_archive.go b/internal/gitaly/service/ssh/upload_archive.go
index 3dc362a91..c99d09f23 100644
--- a/internal/gitaly/service/ssh/upload_archive.go
+++ b/internal/gitaly/service/ssh/upload_archive.go
@@ -6,6 +6,7 @@ import (
 	"sync"
 
 	"gitlab.com/gitlab-org/gitaly/v15/internal/command"
+	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"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/helper"
@@ -98,6 +99,9 @@ func (s *server) sshUploadArchive(stream gitalypb.SSHService_SSHUploadArchiveSer
 }
 
 func validateFirstUploadArchiveRequest(req *gitalypb.SSHUploadArchiveRequest) error {
+	if req.GetRepository() == nil {
+		return gitalyerrors.ErrEmptyRepository
+	}
 	if req.Stdin != nil {
 		return fmt.Errorf("non-empty stdin in first request")
 	}
diff --git a/internal/gitaly/service/ssh/upload_archive_test.go b/internal/gitaly/service/ssh/upload_archive_test.go
index 0f41c35ca..22b47ed14 100644
--- a/internal/gitaly/service/ssh/upload_archive_test.go
+++ b/internal/gitaly/service/ssh/upload_archive_test.go
@@ -15,6 +15,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"
 	"google.golang.org/protobuf/encoding/protojson"
 )
 
@@ -66,29 +67,34 @@ func TestFailedUploadArchiveRequestDueToValidationError(t *testing.T) {
 	client := newSSHClient(t, serverSocketPath)
 
 	tests := []struct {
-		Desc string
-		Req  *gitalypb.SSHUploadArchiveRequest
-		Code codes.Code
+		Desc   string
+		Req    *gitalypb.SSHUploadArchiveRequest
+		expErr error
 	}{
 		{
 			Desc: "Repository.RelativePath is empty",
 			Req:  &gitalypb.SSHUploadArchiveRequest{Repository: &gitalypb.Repository{StorageName: cfg.Storages[0].Name, RelativePath: ""}},
-			Code: codes.InvalidArgument,
+			expErr: status.Error(codes.InvalidArgument, testhelper.GitalyOrPraefect(
+				"GetPath: relative path missing",
+				"repo scoped: invalid Repository",
+			)),
 		},
 		{
 			Desc: "Repository is nil",
 			Req:  &gitalypb.SSHUploadArchiveRequest{Repository: nil},
-			Code: codes.InvalidArgument,
+			expErr: status.Error(codes.InvalidArgument, testhelper.GitalyOrPraefect(
+				"empty Repository",
+				"repo scoped: empty Repository",
+			)),
 		},
 		{
 			Desc: "Data exists on first request",
 			Req:  &gitalypb.SSHUploadArchiveRequest{Repository: &gitalypb.Repository{StorageName: cfg.Storages[0].Name, RelativePath: "path/to/repo"}, Stdin: []byte("Fail")},
-			Code: func() codes.Code {
+			expErr: func() error {
 				if testhelper.IsPraefectEnabled() {
-					return codes.NotFound
+					return status.Error(codes.NotFound, `accessor call: route repository accessor: consistent storages: repository "default"/"path/to/repo" not found`)
 				}
-
-				return codes.InvalidArgument
+				return status.Error(codes.InvalidArgument, "non-empty stdin in first request")
 			}(),
 		},
 	}
@@ -107,7 +113,7 @@ func TestFailedUploadArchiveRequestDueToValidationError(t *testing.T) {
 			require.NoError(t, stream.CloseSend())
 
 			err = testUploadArchiveFailedResponse(t, stream)
-			testhelper.RequireGrpcCode(t, err, test.Code)
+			testhelper.RequireGrpcError(t, test.expErr, err)
 		})
 	}
 }
diff --git a/internal/gitaly/service/ssh/upload_pack.go b/internal/gitaly/service/ssh/upload_pack.go
index e468c834c..632ea17e8 100644
--- a/internal/gitaly/service/ssh/upload_pack.go
+++ b/internal/gitaly/service/ssh/upload_pack.go
@@ -10,6 +10,7 @@ import (
 	"github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus/ctxlogrus"
 	log "github.com/sirupsen/logrus"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/command"
+	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"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/stats"
@@ -181,6 +182,9 @@ func (s *server) sshUploadPack(rpcContext context.Context, req sshUploadPackRequ
 }
 
 func validateFirstUploadPackRequest(req *gitalypb.SSHUploadPackRequest) error {
+	if req.GetRepository() == nil {
+		return gitalyerrors.ErrEmptyRepository
+	}
 	if req.Stdin != nil {
 		return fmt.Errorf("non-empty stdin in first request")
 	}
diff --git a/internal/gitaly/service/ssh/upload_pack_test.go b/internal/gitaly/service/ssh/upload_pack_test.go
index 9e4bd4833..59bbe7e2d 100644
--- a/internal/gitaly/service/ssh/upload_pack_test.go
+++ b/internal/gitaly/service/ssh/upload_pack_test.go
@@ -457,7 +457,7 @@ func TestUploadPack_validation(t *testing.T) {
 				if testhelper.IsPraefectEnabled() {
 					return helper.ErrInvalidArgumentf("repo scoped: empty Repository")
 				}
-				return helper.ErrInvalidArgumentf("GetStorageByName: no such storage: \"\"")
+				return helper.ErrInvalidArgumentf("empty Repository")
 			}(),
 		},
 		{