Merge branch '4257-usermergebranch-does-not-verify-it-got-a-user-name-and-email' into 'master'

usermergebranch: add validation for user.{email, name}

Closes #4257

See merge request gitlab-org/gitaly!4800

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()