From c085d6c9ac7afe822a59eccba589c67eb1ac9b1c Mon Sep 17 00:00:00 2001 From: Gusted Date: Fri, 27 Jun 2025 15:43:31 +0200 Subject: [PATCH] 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 Reviewed-by: Beowulf Reviewed-by: Earl Warren Co-authored-by: Gusted Co-committed-by: Gusted --- services/repository/files/file.go | 51 +++++++++++----------- tests/integration/signing_git_test.go | 61 ++++++++++++++++++++++++++- 2 files changed, 85 insertions(+), 27 deletions(-) diff --git a/services/repository/files/file.go b/services/repository/files/file.go index ef9a87dbcf..5b93258840 100644 --- a/services/repository/files/file.go +++ b/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 != "" { - if doer != nil && strings.EqualFold(doer.Email, committer.Email) { - committerUser = doer // the committer is the doer, so will use their user object - if committer.Name != "" { - committerUser.FullName = committer.Name + getUser := func(identity *IdentityOptions) *user_model.User { + if identity == nil || identity.Email == "" { + return nil + } + + 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 - } else { - committerUser = &user_model.User{ - FullName: committer.Name, - Email: committer.Email, - } - } - } - if author != nil && author.Email != "" { - if doer != nil && strings.EqualFold(doer.Email, author.Email) { - authorUser = doer // the author is the doer, so will use their user object - if authorUser.Name != "" { - 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, - } + user.KeepEmailPrivate = false + return user + } + + var id int64 + if doer != nil { + id = doer.ID + } + return &user_model.User{ + ID: id, // Needed to ensure the doer is checked to pass rules for instance signing of CRUD actions. + FullName: identity.Name, + Email: identity.Email, } } + + committerUser = getUser(committer) + authorUser = getUser(author) + if authorUser == nil { if committerUser != nil { authorUser = committerUser // No valid author was given so use the committer diff --git a/tests/integration/signing_git_test.go b/tests/integration/signing_git_test.go index 9d69306e0a..c4759aaddb 100644 --- a/tests/integration/signing_git_test.go +++ b/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"}