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 f85fd2b01..fef90a408 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"
@@ -144,7 +145,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 bf27bc347..0beb1616f 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 70d55cc1a..37237c29e 100644
--- a/internal/gitaly/service/ssh/upload_archive.go
+++ b/internal/gitaly/service/ssh/upload_archive.go
@@ -7,6 +7,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"
@@ -99,6 +100,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 errors.New("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..d46b814a2 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
+		expectedErr error
 	}{
 		{
 			Desc: "Repository.RelativePath is empty",
 			Req:  &gitalypb.SSHUploadArchiveRequest{Repository: &gitalypb.Repository{StorageName: cfg.Storages[0].Name, RelativePath: ""}},
-			Code: codes.InvalidArgument,
+			expectedErr: 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,
+			expectedErr: 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 {
+			expectedErr: 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.expectedErr, err)
 		})
 	}
 }
diff --git a/internal/gitaly/service/ssh/upload_pack.go b/internal/gitaly/service/ssh/upload_pack.go
index a0719a568..d2fbd1d87 100644
--- a/internal/gitaly/service/ssh/upload_pack.go
+++ b/internal/gitaly/service/ssh/upload_pack.go
@@ -11,6 +11,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"
@@ -182,6 +183,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 errors.New("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")
 			}(),
 		},
 		{