From d37eca37cef76297f1a622c4a6f354a716bf09c2 Mon Sep 17 00:00:00 2001 From: Karthik Nayak Date: Wed, 23 Nov 2022 17:11:24 +0100 Subject: [PATCH] operations: Use `ExpectedOldOid` in `UserCommitFiles` If the clients provide the `ExpectedOldOid` in the request of `UserCommitFiles`, we need to use this as the ref's current OID for updating the reference. This is directly fed to git-update-ref(1) where it'll exit with an error code if the current OID of the ref doesn't match the OID provided by us. This allows us to avoid any race conditions wherein the branch was updated concurrently by another process. --- .../gitaly/service/operations/commit_files.go | 26 ++++-- .../service/operations/commit_files_test.go | 89 ++++++++++++++----- 2 files changed, 86 insertions(+), 29 deletions(-) diff --git a/internal/gitaly/service/operations/commit_files.go b/internal/gitaly/service/operations/commit_files.go index d213c449c3..c92e7bd651 100644 --- a/internal/gitaly/service/operations/commit_files.go +++ b/internal/gitaly/service/operations/commit_files.go @@ -337,11 +337,27 @@ func (s *Server) userCommitFiles(ctx context.Context, header *gitalypb.UserCommi return fmt.Errorf("was repo created: %w", err) } - oldRevision := parentCommitOID - if targetBranchCommit == "" { - oldRevision = git.ObjectHashSHA1.ZeroOID - } else if header.Force { - oldRevision = targetBranchCommit + var oldRevision git.ObjectID + if expectedOldOID := header.GetExpectedOldOid(); expectedOldOID != "" { + oldRevision, err = git.ObjectHashSHA1.FromHex(expectedOldOID) + if err != nil { + return structerr.NewInvalidArgument("invalid expected old object ID: %w", err).WithMetadata("old_object_id", expectedOldOID) + } + + oldRevision, err = s.localrepo(header.GetRepository()).ResolveRevision( + ctx, git.Revision(fmt.Sprintf("%s^{object}", oldRevision)), + ) + if err != nil { + return structerr.NewInvalidArgument("cannot resolve expected old object ID: %w", err). + WithMetadata("old_object_id", expectedOldOID) + } + } else { + oldRevision = parentCommitOID + if targetBranchCommit == "" { + oldRevision = git.ObjectHashSHA1.ZeroOID + } else if header.Force { + oldRevision = targetBranchCommit + } } if err := s.updateReferenceWithHooks(ctx, header.GetRepository(), header.User, quarantineDir, targetBranchName, commitID, oldRevision); err != nil { diff --git a/internal/gitaly/service/operations/commit_files_test.go b/internal/gitaly/service/operations/commit_files_test.go index 16fe18a8d9..ccd85753ad 100644 --- a/internal/gitaly/service/operations/commit_files_test.go +++ b/internal/gitaly/service/operations/commit_files_test.go @@ -19,6 +19,7 @@ import ( "gitlab.com/gitlab-org/gitaly/v15/internal/helper" "gitlab.com/gitlab-org/gitaly/v15/internal/helper/text" "gitlab.com/gitlab-org/gitaly/v15/internal/metadata/featureflag" + "gitlab.com/gitlab-org/gitaly/v15/internal/structerr" "gitlab.com/gitlab-org/gitaly/v15/internal/testhelper" "gitlab.com/gitlab-org/gitaly/v15/internal/testhelper/testcfg" "gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb" @@ -912,6 +913,7 @@ func testUserCommitFiles(t *testing.T, ctx context.Context) { branch, []byte("commit message"), "", + "", ) setAuthorAndEmail(headerRequest, []byte("Author Name"), []byte("author.email@example.com")) @@ -1001,7 +1003,7 @@ func testUserCommitFilesStableCommitID(t *testing.T, ctx context.Context) { stream, err := client.UserCommitFiles(ctx) require.NoError(t, err) - headerRequest := headerRequest(repoProto, gittest.TestUser, "master", []byte("commit message"), "") + headerRequest := headerRequest(repoProto, gittest.TestUser, "master", []byte("commit message"), "", "") setAuthorAndEmail(headerRequest, []byte("Author Name"), []byte("author.email@example.com")) setTimestamp(headerRequest, time.Unix(12345, 0)) require.NoError(t, stream.Send(headerRequest)) @@ -1070,7 +1072,7 @@ func testUserCommitFilesQuarantine(t *testing.T, ctx context.Context) { stream, err := client.UserCommitFiles(ctx) require.NoError(t, err) - headerRequest := headerRequest(repoProto, gittest.TestUser, "master", []byte("commit message"), "") + headerRequest := headerRequest(repoProto, gittest.TestUser, "master", []byte("commit message"), "", "") setAuthorAndEmail(headerRequest, []byte("Author Name"), []byte("author.email@example.com")) setTimestamp(headerRequest, time.Unix(12345, 0)) require.NoError(t, stream.Send(headerRequest)) @@ -1125,9 +1127,11 @@ func testSuccessfulUserCommitFilesRequest(t *testing.T, ctx context.Context) { repoPath string branchName string startBranchName string + expectedOldOID string repoCreated bool branchCreated bool executeFilemode bool + expectedError error }{ { desc: "existing repo and branch", @@ -1137,6 +1141,37 @@ func testSuccessfulUserCommitFilesRequest(t *testing.T, ctx context.Context) { repoCreated: false, branchCreated: false, }, + { + desc: "existing repo and branch + expectedOldOID", + repo: repo, + repoPath: repoPath, + branchName: "wip", + expectedOldOID: text.ChompBytes(gittest.Exec(t, cfg, "-C", repoPath, "rev-parse", "refs/heads/wip")), + }, + { + desc: "existing repo and branch + invalid expectedOldOID", + repo: repo, + repoPath: repoPath, + branchName: "few-commits", + expectedOldOID: "foobar", + expectedError: structerr.NewInvalidArgument(`invalid expected old object ID: invalid object ID: "foobar"`), + }, + { + desc: "existing repo and branch + valid expectedOldOID but invalid object", + repo: repo, + repoPath: repoPath, + branchName: "few-commits", + expectedOldOID: gittest.DefaultObjectHash.ZeroOID.String(), + expectedError: structerr.NewInvalidArgument("cannot resolve expected old object ID: reference not found"), + }, + { + desc: "existing repo and branch + incorrect expectedOldOID", + repo: repo, + repoPath: repoPath, + branchName: "few-commits", + expectedOldOID: text.ChompBytes(gittest.Exec(t, cfg, "-C", repoPath, "rev-parse", "refs/heads/few-commits~1")), + expectedError: structerr.NewFailedPrecondition("Could not update refs/heads/few-commits. Please refresh and try again."), + }, { desc: "existing repo, new branch", repo: repo, @@ -1175,7 +1210,7 @@ func testSuccessfulUserCommitFilesRequest(t *testing.T, ctx context.Context) { for _, tc := range testCases { t.Run(tc.desc, func(t *testing.T) { - headerRequest := headerRequest(tc.repo, gittest.TestUser, tc.branchName, commitFilesMessage, tc.startBranchName) + headerRequest := headerRequest(tc.repo, gittest.TestUser, tc.branchName, commitFilesMessage, tc.startBranchName, tc.expectedOldOID) setAuthorAndEmail(headerRequest, authorName, authorEmail) actionsRequest1 := createFileHeaderRequest(filePath) @@ -1184,6 +1219,7 @@ func testSuccessfulUserCommitFilesRequest(t *testing.T, ctx context.Context) { actionsRequest4 := chmodFileHeaderRequest(filePath, tc.executeFilemode) stream, err := client.UserCommitFiles(ctx) + require.NoError(t, err) require.NoError(t, stream.Send(headerRequest)) require.NoError(t, stream.Send(actionsRequest1)) @@ -1192,6 +1228,10 @@ func testSuccessfulUserCommitFilesRequest(t *testing.T, ctx context.Context) { require.NoError(t, stream.Send(actionsRequest4)) resp, err := stream.CloseAndRecv() + if tc.expectedError != nil { + testhelper.RequireGrpcError(t, tc.expectedError, err) + return + } require.NoError(t, err) require.Equal(t, tc.repoCreated, resp.GetBranchUpdate().GetRepoCreated()) require.Equal(t, tc.branchCreated, resp.GetBranchUpdate().GetBranchCreated()) @@ -1249,7 +1289,7 @@ func testSuccessfulUserCommitFilesRequestMove(t *testing.T, ctx context.Context) }) origFileContent := gittest.Exec(t, cfg, "-C", testRepoPath, "show", branchName+":"+previousFilePath) - headerRequest := headerRequest(testRepo, gittest.TestUser, branchName, commitFilesMessage, "") + headerRequest := headerRequest(testRepo, gittest.TestUser, branchName, commitFilesMessage, "", "") setAuthorAndEmail(headerRequest, authorName, authorEmail) actionsRequest1 := moveFileHeaderRequest(previousFilePath, filePath, tc.infer) @@ -1308,7 +1348,7 @@ func testSuccessfulUserCommitFilesRequestForceCommit(t *testing.T, ctx context.C mergeBaseID := text.ChompBytes(mergeBaseOut) require.NotEqual(t, mergeBaseID, targetBranchCommit.Id, "expected %s not to be an ancestor of %s", targetBranchCommit.Id, startBranchCommit.Id) - headerRequest := headerRequest(repoProto, gittest.TestUser, targetBranchName, commitFilesMessage, "") + headerRequest := headerRequest(repoProto, gittest.TestUser, targetBranchName, commitFilesMessage, "", "") setAuthorAndEmail(headerRequest, authorName, authorEmail) setStartBranchName(headerRequest, startBranchName) setForce(headerRequest, true) @@ -1348,7 +1388,7 @@ func testSuccessfulUserCommitFilesRequestStartSha(t *testing.T, ctx context.Cont startCommit, err := repo.ReadCommit(ctx, "master") require.NoError(t, err) - headerRequest := headerRequest(repoProto, gittest.TestUser, targetBranchName, commitFilesMessage, "") + headerRequest := headerRequest(repoProto, gittest.TestUser, targetBranchName, commitFilesMessage, "", "") setStartSha(headerRequest, startCommit.Id) stream, err := client.UserCommitFiles(ctx) @@ -1412,7 +1452,7 @@ func testSuccessfulUserCommitFilesRemoteRepositoryRequest(setHeader func(header startCommit, err := repo.ReadCommit(ctx, "master") require.NoError(t, err) - headerRequest := headerRequest(newRepoProto, gittest.TestUser, targetBranchName, commitFilesMessage, "") + headerRequest := headerRequest(newRepoProto, gittest.TestUser, targetBranchName, commitFilesMessage, "", "") setHeader(headerRequest) setStartRepository(headerRequest, repoProto) @@ -1469,7 +1509,7 @@ func testSuccessfulUserCommitFilesRequestWithSpecialCharactersInSignature(t *tes for _, tc := range testCases { t.Run(tc.desc, func(t *testing.T) { - headerRequest := headerRequest(repoProto, tc.user, targetBranchName, commitFilesMessage, "") + headerRequest := headerRequest(repoProto, tc.user, targetBranchName, commitFilesMessage, "", "") setAuthorAndEmail(headerRequest, tc.user.Name, tc.user.Email) stream, err := client.UserCommitFiles(ctx) @@ -1503,7 +1543,7 @@ func testFailedUserCommitFilesRequestDueToHooks(t *testing.T, ctx context.Contex branchName := "feature" filePath := "my/file.txt" - headerRequest := headerRequest(repoProto, gittest.TestUser, branchName, commitFilesMessage, "") + headerRequest := headerRequest(repoProto, gittest.TestUser, branchName, commitFilesMessage, "", "") actionsRequest1 := createFileHeaderRequest(filePath) actionsRequest2 := actionContentRequest("My content") hookContent := []byte("#!/bin/sh\nprintenv | grep -e GL_ID -e GL_USERNAME | sort | paste -sd ' ' -\nexit 1") @@ -1570,7 +1610,7 @@ func testFailedUserCommitFilesRequestDueToIndexError(t *testing.T, ctx context.C { desc: "file already exists", requests: []*gitalypb.UserCommitFilesRequest{ - headerRequest(repo, gittest.TestUser, "feature", commitFilesMessage, ""), + headerRequest(repo, gittest.TestUser, "feature", commitFilesMessage, "", ""), createFileHeaderRequest("README.md"), actionContentRequest("This file already exists"), }, @@ -1580,7 +1620,7 @@ func testFailedUserCommitFilesRequestDueToIndexError(t *testing.T, ctx context.C { desc: "file doesn't exists", requests: []*gitalypb.UserCommitFilesRequest{ - headerRequest(repo, gittest.TestUser, "feature", commitFilesMessage, ""), + headerRequest(repo, gittest.TestUser, "feature", commitFilesMessage, "", ""), chmodFileHeaderRequest("documents/story.txt", true), }, indexError: "A file with this name doesn't exist", @@ -1589,7 +1629,7 @@ func testFailedUserCommitFilesRequestDueToIndexError(t *testing.T, ctx context.C { desc: "dir already exists", requests: []*gitalypb.UserCommitFilesRequest{ - headerRequest(repo, gittest.TestUser, "utf-dir", commitFilesMessage, ""), + headerRequest(repo, gittest.TestUser, "utf-dir", commitFilesMessage, "", ""), actionRequest(&gitalypb.UserCommitFilesAction{ UserCommitFilesActionPayload: &gitalypb.UserCommitFilesAction_Header{ Header: &gitalypb.UserCommitFilesActionHeader{ @@ -1653,7 +1693,7 @@ func testFailedUserCommitFilesRequest(t *testing.T, ctx context.Context) { }{ { desc: "empty Repository", - req: headerRequest(nil, gittest.TestUser, branchName, commitFilesMessage, ""), + req: headerRequest(nil, gittest.TestUser, branchName, commitFilesMessage, "", ""), expectedErr: status.Error(codes.InvalidArgument, testhelper.GitalyOrPraefect( "empty Repository", "repo scoped: empty Repository", @@ -1661,47 +1701,47 @@ func testFailedUserCommitFilesRequest(t *testing.T, ctx context.Context) { }, { desc: "empty User", - req: headerRequest(repo, nil, branchName, commitFilesMessage, ""), + req: headerRequest(repo, nil, branchName, commitFilesMessage, "", ""), expectedErr: status.Error(codes.InvalidArgument, "empty User"), }, { desc: "empty BranchName", - req: headerRequest(repo, gittest.TestUser, "", commitFilesMessage, ""), + req: headerRequest(repo, gittest.TestUser, "", commitFilesMessage, "", ""), expectedErr: status.Error(codes.InvalidArgument, "empty BranchName"), }, { desc: "empty CommitMessage", - req: headerRequest(repo, gittest.TestUser, branchName, nil, ""), + req: headerRequest(repo, gittest.TestUser, branchName, nil, "", ""), expectedErr: status.Error(codes.InvalidArgument, "empty CommitMessage"), }, { desc: "invalid object ID: \"foobar\"", - req: setStartSha(headerRequest(repo, gittest.TestUser, branchName, commitFilesMessage, ""), "foobar"), + req: setStartSha(headerRequest(repo, gittest.TestUser, branchName, commitFilesMessage, "", ""), "foobar"), expectedErr: status.Error(codes.InvalidArgument, `invalid object ID: "foobar"`), }, { desc: "failed to parse signature - Signature cannot have an empty name or email", - req: headerRequest(repo, &gitalypb.User{}, branchName, commitFilesMessage, ""), + req: headerRequest(repo, &gitalypb.User{}, branchName, commitFilesMessage, "", ""), expectedErr: status.Error(codes.InvalidArgument, "commit: commit: failed to parse signature - Signature cannot have an empty name or email"), }, { desc: "failed to parse signature - Signature cannot have an empty name or email", - req: headerRequest(repo, &gitalypb.User{Name: []byte(""), Email: []byte("")}, branchName, commitFilesMessage, ""), + req: headerRequest(repo, &gitalypb.User{Name: []byte(""), Email: []byte("")}, branchName, commitFilesMessage, "", ""), expectedErr: status.Error(codes.InvalidArgument, "commit: commit: failed to parse signature - Signature cannot have an empty name or email"), }, { desc: "failed to parse signature - Signature cannot have an empty name or email", - req: headerRequest(repo, &gitalypb.User{Name: []byte(" "), Email: []byte(" ")}, branchName, commitFilesMessage, ""), + req: headerRequest(repo, &gitalypb.User{Name: []byte(" "), Email: []byte(" ")}, branchName, commitFilesMessage, "", ""), expectedErr: status.Error(codes.InvalidArgument, "commit: commit: failed to parse signature - Signature cannot have an empty name or email"), }, { desc: "failed to parse signature - Signature cannot have an empty name or email", - req: headerRequest(repo, &gitalypb.User{Name: []byte("Jane Doe"), Email: []byte("")}, branchName, commitFilesMessage, ""), + req: headerRequest(repo, &gitalypb.User{Name: []byte("Jane Doe"), Email: []byte("")}, branchName, commitFilesMessage, "", ""), expectedErr: status.Error(codes.InvalidArgument, "commit: commit: failed to parse signature - Signature cannot have an empty name or email"), }, { desc: "failed to parse signature - Signature cannot have an empty name or email", - req: headerRequest(repo, &gitalypb.User{Name: []byte(""), Email: []byte("janedoe@gitlab.com")}, branchName, commitFilesMessage, ""), + req: headerRequest(repo, &gitalypb.User{Name: []byte(""), Email: []byte("janedoe@gitlab.com")}, branchName, commitFilesMessage, "", ""), expectedErr: status.Error(codes.InvalidArgument, "commit: commit: failed to parse signature - Signature cannot have an empty name or email"), }, } @@ -1736,7 +1776,7 @@ func testUserCommitFilesFailsIfRepositoryMissing(t *testing.T, ctx context.Conte } branchName := "feature" - req := headerRequest(repo, gittest.TestUser, branchName, commitFilesMessage, "") + req := headerRequest(repo, gittest.TestUser, branchName, commitFilesMessage, "", "") stream, err := client.UserCommitFiles(ctx) require.NoError(t, err) @@ -1752,7 +1792,7 @@ func testUserCommitFilesFailsIfRepositoryMissing(t *testing.T, ctx context.Conte } } -func headerRequest(repo *gitalypb.Repository, user *gitalypb.User, branchName string, commitMessage []byte, startBranchName string) *gitalypb.UserCommitFilesRequest { +func headerRequest(repo *gitalypb.Repository, user *gitalypb.User, branchName string, commitMessage []byte, startBranchName string, expectedOldOID string) *gitalypb.UserCommitFilesRequest { return &gitalypb.UserCommitFilesRequest{ UserCommitFilesRequestPayload: &gitalypb.UserCommitFilesRequest_Header{ Header: &gitalypb.UserCommitFilesRequestHeader{ @@ -1762,6 +1802,7 @@ func headerRequest(repo *gitalypb.Repository, user *gitalypb.User, branchName st CommitMessage: commitMessage, StartBranchName: []byte(startBranchName), StartRepository: nil, + ExpectedOldOid: expectedOldOID, }, }, } -- GitLab