fix: pass doer's ID for CRUD instance signing (#8304)

- When doing CRUD actions, the commiter and author are reconstructed and
do not contain the doer's ID. Make sure to pass this ID along so it can
be used to verify the rules of instance signing for CRUD actions.
- Regression of forgejo/forgejo#7693. It seems that previously this
didn't work correctly as it would not care about a empty ID.
- Resolves forgejo/forgejo#8278

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/8304
Reviewed-by: Michael Kriese <michael.kriese@gmx.de>
Reviewed-by: Beowulf <beowulf@beocode.eu>
Reviewed-by: Earl Warren <earl-warren@noreply.codeberg.org>
Co-authored-by: Gusted <postmaster@gusted.xyz>
Co-committed-by: Gusted <postmaster@gusted.xyz>

services/repository/files/file.go

@@ -104,36 +104,35 @@ func GetAuthorAndCommitterUsers(author, committer *IdentityOptions, doer *user_m
 	// then we use bogus User objects for them to store their FullName and Email.
 	// If only one of the two are provided, we set both of them to it.
 	// If neither are provided, both are the doer.
-	if committer != nil && committer.Email != "" {
+	getUser := func(identity *IdentityOptions) *user_model.User {
-		if doer != nil && strings.EqualFold(doer.Email, committer.Email) {
+		if identity == nil || identity.Email == "" {
-			committerUser = doer // the committer is the doer, so will use their user object
+			return nil
-			if committer.Name != "" {
+		}
-				committerUser.FullName = committer.Name
+
+		if doer != nil && strings.EqualFold(doer.Email, identity.Email) {
+			user := doer // the committer is the doer, so will use their user object
+			if identity.Name != "" {
+				user.FullName = identity.Name
 			}
 			// Use the provided email and not revert to placeholder mail.
-			committerUser.KeepEmailPrivate = false
+			user.KeepEmailPrivate = false
-		} else {
+			return user
-			committerUser = &user_model.User{
+		}
-				FullName: committer.Name,
+
-				Email:    committer.Email,
+		var id int64
-			}
+		if doer != nil {
-		}
+			id = doer.ID
-	}
+		}
-	if author != nil && author.Email != "" {
+		return &user_model.User{
-		if doer != nil && strings.EqualFold(doer.Email, author.Email) {
+			ID:       id, // Needed to ensure the doer is checked to pass rules for instance signing of CRUD actions.
-			authorUser = doer // the author is the doer, so will use their user object
+			FullName: identity.Name,
-			if authorUser.Name != "" {
+			Email:    identity.Email,
-				authorUser.FullName = author.Name
-			}
-			// Use the provided email and not revert to placeholder mail.
-			authorUser.KeepEmailPrivate = false
-		} else {
-			authorUser = &user_model.User{
-				FullName: author.Name,
-				Email:    author.Email,
-			}
 		}
 	}
+
+	committerUser = getUser(committer)
+	authorUser = getUser(author)
+
 	if authorUser == nil {
 		if committerUser != nil {
 			authorUser = committerUser // No valid author was given so use the committer

tests/integration/signing_git_test.go

@@ -235,7 +235,7 @@ func testCRUD(t *testing.T, u *url.URL, signingFormat string, objectFormat git.O
 			}))
 		})
 
-		t.Run("No publickey", func(t *testing.T) {
+		t.Run("No 2fa", func(t *testing.T) {
 			defer tests.PrintCurrentTest(t)()
 
 			testCtx := NewAPITestContext(t, "user4", "initial-no-2fa"+suffix, auth_model.AccessTokenScopeWriteRepository, auth_model.AccessTokenScopeWriteUser)
@@ -287,6 +287,65 @@ func testCRUD(t *testing.T, u *url.URL, signingFormat string, objectFormat git.O
 			}))
 	})
 
+	t.Run("AlwaysSign-Initial-CRUD-Pubkey", func(t *testing.T) {
+		setting.Repository.Signing.CRUDActions = []string{"pubkey"}
+
+		t.Run("Has publickey", func(t *testing.T) {
+			defer tests.PrintCurrentTest(t)()
+
+			testCtx := NewAPITestContext(t, username, "initial-always-pubkey"+suffix, auth_model.AccessTokenScopeWriteRepository, auth_model.AccessTokenScopeWriteUser)
+			t.Run("CreateRepository", doAPICreateRepository(testCtx, false, objectFormat))
+			t.Run("CreateCRUDFile-Pubkey", crudActionCreateFile(
+				t, testCtx, user, "master", "pubkey", "signed-pubkey.txt", func(t *testing.T, response api.FileResponse) {
+					assert.True(t, response.Verification.Verified)
+					assert.Equal(t, "fox@example.com", response.Verification.Signer.Email)
+				}))
+		})
+
+		t.Run("No publickey", func(t *testing.T) {
+			defer tests.PrintCurrentTest(t)()
+
+			testCtx := NewAPITestContext(t, "user4", "initial-always-no-pubkey"+suffix, auth_model.AccessTokenScopeWriteRepository, auth_model.AccessTokenScopeWriteUser)
+			t.Run("CreateRepository", doAPICreateRepository(testCtx, false, objectFormat))
+			t.Run("CreateCRUDFile-Pubkey", crudActionCreateFile(
+				t, testCtx, user, "master", "pubkey", "unsigned-pubkey.txt", func(t *testing.T, response api.FileResponse) {
+					assert.False(t, response.Verification.Verified)
+				}))
+		})
+	})
+
+	t.Run("AlwaysSign-Initial-CRUD-Twofa", func(t *testing.T) {
+		setting.Repository.Signing.CRUDActions = []string{"twofa"}
+
+		t.Run("Has 2fa", func(t *testing.T) {
+			defer tests.PrintCurrentTest(t)()
+
+			t.Cleanup(func() {
+				unittest.AssertSuccessfulDelete(t, &auth_model.WebAuthnCredential{UserID: user.ID})
+			})
+
+			testCtx := NewAPITestContext(t, username, "initial-always-twofa"+suffix, auth_model.AccessTokenScopeWriteRepository, auth_model.AccessTokenScopeWriteUser)
+			unittest.AssertSuccessfulInsert(t, &auth_model.WebAuthnCredential{UserID: user.ID})
+			t.Run("CreateRepository", doAPICreateRepository(testCtx, false, objectFormat))
+			t.Run("CreateCRUDFile-Twofa", crudActionCreateFile(
+				t, testCtx, user, "master", "twofa", "signed-twofa.txt", func(t *testing.T, response api.FileResponse) {
+					assert.True(t, response.Verification.Verified)
+					assert.Equal(t, "fox@example.com", response.Verification.Signer.Email)
+				}))
+		})
+
+		t.Run("No 2fa", func(t *testing.T) {
+			defer tests.PrintCurrentTest(t)()
+
+			testCtx := NewAPITestContext(t, "user4", "initial-always-no-twofa"+suffix, auth_model.AccessTokenScopeWriteRepository, auth_model.AccessTokenScopeWriteUser)
+			t.Run("CreateRepository", doAPICreateRepository(testCtx, false, objectFormat))
+			t.Run("CreateCRUDFile-Pubkey", crudActionCreateFile(
+				t, testCtx, user, "master", "twofa", "unsigned-twofa.txt", func(t *testing.T, response api.FileResponse) {
+					assert.False(t, response.Verification.Verified)
+				}))
+		})
+	})
+
 	t.Run("AlwaysSign-Initial-CRUD-Always", func(t *testing.T) {
 		defer tests.PrintCurrentTest(t)()
 		setting.Repository.Signing.CRUDActions = []string{"always"}