operations: Validate user.{email, name} in UserMergeBranch

While 'UserMergeBranch' verifies that it got a user as part of the first
request, it doesn't verify that the user actually has a name and email
set. This leads to an Internal error at a later point when trying to
create the merge commit because you cannot create merge commits with
either of them being empty.

This commit improves our validation to check these parameters and return
an InvalidArgument error if unset. We add a new test
'TestUserMergeBranch_failure' to test the validation logic in
'UserMergeBranch'.

Signed-off-by: Karthik Nayak <knayak@gitlab.com>

2 files changed, 110 insertions, 0 deletions

diff --git a/internal/gitaly/service/operations/merge.go b/internal/gitaly/service/operations/merge.go
index 8237ec034..d54b7b8d4 100644
--- a/internal/gitaly/service/operations/merge.go
+++ b/internal/gitaly/service/operations/merge.go
@@ -21,6 +21,14 @@ func validateMergeBranchRequest(request *gitalypb.UserMergeBranchRequest) error
 		return fmt.Errorf("empty user")
 	}
 
+	if len(request.User.Email) == 0 {
+		return fmt.Errorf("empty user email")
+	}
+
+	if len(request.User.Name) == 0 {
+		return fmt.Errorf("empty user name")
+	}
+
 	if len(request.Branch) == 0 {
 		return fmt.Errorf("empty branch name")
 	}
diff --git a/internal/gitaly/service/operations/merge_test.go b/internal/gitaly/service/operations/merge_test.go
index 41c5038c8..22dbc32f8 100644
--- a/internal/gitaly/service/operations/merge_test.go
+++ b/internal/gitaly/service/operations/merge_test.go
@@ -120,6 +120,108 @@ func TestUserMergeBranch_successful(t *testing.T) {
 	}
 }
 
+func TestUserMergeBranch_failure(t *testing.T) {
+	t.Parallel()
+
+	ctx := testhelper.Context(t)
+
+	ctx, cfg, client := setupOperationsServiceWithoutRepo(t, ctx)
+	repoProto, _ := gittest.CreateRepository(ctx, t, cfg)
+
+	_ = localrepo.NewTestRepo(t, cfg, repoProto)
+
+	testCases := []struct {
+		user        *gitalypb.User
+		repo        *gitalypb.Repository
+		desc        string
+		commitID    string
+		branch      []byte
+		message     []byte
+		expectedErr error
+	}{
+		{
+			desc:        "empty user",
+			repo:        repoProto,
+			branch:      []byte(mergeBranchName),
+			commitID:    commitToMerge,
+			message:     []byte("sample-message"),
+			expectedErr: helper.ErrInvalidArgumentf("empty user"),
+		},
+		{
+			desc: "empty user name",
+			user: &gitalypb.User{
+				GlId:       gittest.TestUser.GlId,
+				GlUsername: gittest.TestUser.GlUsername,
+				Email:      gittest.TestUser.Email,
+				Timezone:   gittest.TestUser.Timezone,
+			},
+			repo:        repoProto,
+			branch:      []byte(mergeBranchName),
+			commitID:    commitToMerge,
+			message:     []byte("sample-message"),
+			expectedErr: helper.ErrInvalidArgumentf("empty user name"),
+		},
+		{
+			desc: "empty user email",
+			user: &gitalypb.User{
+				GlId:       gittest.TestUser.GlId,
+				Name:       gittest.TestUser.Name,
+				GlUsername: gittest.TestUser.GlUsername,
+				Timezone:   gittest.TestUser.Timezone,
+			},
+			repo:        repoProto,
+			branch:      []byte(mergeBranchName),
+			commitID:    commitToMerge,
+			message:     []byte("sample-message"),
+			expectedErr: helper.ErrInvalidArgumentf("empty user email"),
+		},
+		{
+			desc:        "empty commit",
+			repo:        repoProto,
+			user:        gittest.TestUser,
+			branch:      []byte(mergeBranchName),
+			message:     []byte("sample-message"),
+			expectedErr: helper.ErrInvalidArgumentf("empty commit ID"),
+		},
+		{
+			desc:        "empty branch",
+			repo:        repoProto,
+			user:        gittest.TestUser,
+			commitID:    commitToMerge,
+			message:     []byte("sample-message"),
+			expectedErr: helper.ErrInvalidArgumentf("empty branch name"),
+		},
+		{
+			desc:        "empty message",
+			repo:        repoProto,
+			user:        gittest.TestUser,
+			branch:      []byte(mergeBranchName),
+			commitID:    commitToMerge,
+			expectedErr: helper.ErrInvalidArgumentf("empty message"),
+		},
+	}
+
+	for _, testCase := range testCases {
+		t.Run(testCase.desc, func(t *testing.T) {
+			request := &gitalypb.UserMergeBranchRequest{
+				Repository: testCase.repo,
+				User:       testCase.user,
+				Branch:     testCase.branch,
+				CommitId:   testCase.commitID,
+				Message:    testCase.message,
+			}
+
+			mergeBidi, err := client.UserMergeBranch(ctx)
+			require.NoError(t, err)
+
+			require.NoError(t, mergeBidi.Send(request), "apply merge")
+			_, err = mergeBidi.Recv()
+
+			testhelper.RequireGrpcError(t, testCase.expectedErr, err)
+		})
+	}
+}
+
 func TestUserMergeBranch_quarantine(t *testing.T) {
 	t.Parallel()