From 838b9840295a25812402d527f6378115babb7f3c Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Wed, 9 Jul 2025 12:47:44 +0800 Subject: [PATCH] refactor --- models/asymkey/gpg_key_commit_verification.go | 23 +------ modules/git/commit.go | 6 +- services/asymkey/commit.go | 65 +++++++++---------- services/asymkey/commit_test.go | 2 +- services/git/commit.go | 7 -- templates/repo/commit_page.tmpl | 2 +- 6 files changed, 37 insertions(+), 68 deletions(-) diff --git a/models/asymkey/gpg_key_commit_verification.go b/models/asymkey/gpg_key_commit_verification.go index 39ec893606..b85374e073 100644 --- a/models/asymkey/gpg_key_commit_verification.go +++ b/models/asymkey/gpg_key_commit_verification.go @@ -15,25 +15,6 @@ import ( "github.com/ProtonMail/go-crypto/openpgp/packet" ) -// __________________ ________ ____ __. -// / _____/\______ \/ _____/ | |/ _|____ ___.__. -// / \ ___ | ___/ \ ___ | <_/ __ < | | -// \ \_\ \| | \ \_\ \ | | \ ___/\___ | -// \______ /|____| \______ / |____|__ \___ > ____| -// \/ \/ \/ \/\/ -// _________ .__ __ -// \_ ___ \ ____ _____ _____ |__|/ |_ -// / \ \/ / _ \ / \ / \| \ __\ -// \ \___( <_> ) Y Y \ Y Y \ || | -// \______ /\____/|__|_| /__|_| /__||__| -// \/ \/ \/ -// ____ ____ .__ _____.__ __ .__ -// \ \ / /___________|__|/ ____\__| ____ _____ _/ |_|__| ____ ____ -// \ Y // __ \_ __ \ \ __\| |/ ___\\__ \\ __\ |/ _ \ / \ -// \ /\ ___/| | \/ || | | \ \___ / __ \| | | ( <_> ) | \ -// \___/ \___ >__| |__||__| |__|\___ >____ /__| |__|\____/|___| / -// \/ \/ \/ \/ - // This file provides functions relating commit verification // CommitVerification represents a commit validation of signature @@ -41,8 +22,8 @@ type CommitVerification struct { Verified bool Warning bool Reason string - SigningUser *user_model.User - CommittingUser *user_model.User + SigningUser *user_model.User // if Verified, then SigningUser is non-nil + CommittingUser *user_model.User // if Verified, then CommittingUser is non-nil SigningEmail string SigningKey *GPGKey SigningSSHKey *PublicKey diff --git a/modules/git/commit.go b/modules/git/commit.go index ed4876e7b3..bcb71c8f8f 100644 --- a/modules/git/commit.go +++ b/modules/git/commit.go @@ -22,9 +22,9 @@ import ( type Commit struct { Tree // FIXME: bad design, this field can be nil if the commit is from "last commit cache" - ID ObjectID // The ID of this commit object - Author *Signature - Committer *Signature + ID ObjectID // The ID of this commit object + Author *Signature // never nil + Committer *Signature // never nil CommitMessage string Signature *CommitSignature diff --git a/services/asymkey/commit.go b/services/asymkey/commit.go index 148f51fd10..54c980d32f 100644 --- a/services/asymkey/commit.go +++ b/services/asymkey/commit.go @@ -24,47 +24,42 @@ import ( // ParseCommitWithSignature check if signature is good against keystore. func ParseCommitWithSignature(ctx context.Context, c *git.Commit) *asymkey_model.CommitVerification { - var committer *user_model.User - if c.Committer != nil { - var err error - // Find Committer account - committer, err = user_model.GetUserByEmail(ctx, c.Committer.Email) // This finds the user by primary email or activated email so commit will not be valid if email is not - if err != nil { // Skipping not user for committer - committer = &user_model.User{ - Name: c.Committer.Name, - Email: c.Committer.Email, - } - // We can expect this to often be an ErrUserNotExist. in the case - // it is not, however, it is important to log it. - if !user_model.IsErrUserNotExist(err) { - log.Error("GetUserByEmail: %v", err) - return &asymkey_model.CommitVerification{ - CommittingUser: committer, - Verified: false, - Reason: "gpg.error.no_committer_account", - } - } + committer, err := user_model.GetUserByEmail(ctx, c.Committer.Email) + if err != nil && !user_model.IsErrUserNotExist(err) { + log.Error("GetUserByEmail: %v", err) + return &asymkey_model.CommitVerification{ + Verified: false, + Reason: "gpg.error.no_committer_account", // this error is not right, but such error should seldom happen } } - return ParseCommitWithSignatureCommitter(ctx, c, committer) } +// ParseCommitWithSignatureCommitter parses a commit's GPG or SSH signature. +// If the commit is singed by an instance key, then committer is nil. func ParseCommitWithSignatureCommitter(ctx context.Context, c *git.Commit, committer *user_model.User) *asymkey_model.CommitVerification { - // If no signature just report the committer + // If no signature, just report the committer if c.Signature == nil { return &asymkey_model.CommitVerification{ CommittingUser: committer, - Verified: false, // Default value - Reason: "gpg.error.not_signed_commit", // Default value + Verified: false, + Reason: "gpg.error.not_signed_commit", } } - - // If this a SSH signature handle it differently - if strings.HasPrefix(c.Signature.Signature, "-----BEGIN SSH SIGNATURE-----") { - return ParseCommitWithSSHSignature(ctx, c, committer) + // to support instance key, we need a fake committer user (not really needed, but legacy code accesses the committer without nil-check) + if committer == nil { + committer = &user_model.User{ + Name: c.Committer.Name, + Email: c.Committer.Email, + } } + if strings.HasPrefix(c.Signature.Signature, "-----BEGIN SSH SIGNATURE-----") { + return parseCommitWithSSHSignature(ctx, c, committer) + } + return parseCommitWithGPGSignature(ctx, c, committer) +} +func parseCommitWithGPGSignature(ctx context.Context, c *git.Commit, committer *user_model.User) *asymkey_model.CommitVerification { // Parsing signature sig, err := asymkey_model.ExtractSignature(c.Signature.Signature) if err != nil { // Skipping failed to extract sign @@ -96,7 +91,7 @@ func ParseCommitWithSignatureCommitter(ctx context.Context, c *git.Commit, commi } // Now try to associate the signature with the committer, if present - if committer.ID != 0 { + if committer != nil && committer.ID != 0 { keys, err := db.Find[asymkey_model.GPGKey](ctx, asymkey_model.FindGPGKeyOptions{ OwnerID: committer.ID, }) @@ -165,7 +160,7 @@ func ParseCommitWithSignatureCommitter(ctx context.Context, c *git.Commit, commi } if err := gpgSettings.LoadPublicKeyContent(); err != nil { log.Error("Error getting default signing key: %s %v", gpgSettings.KeyID, err) - } else if commitVerification := VerifyWithGPGSettings(ctx, &gpgSettings, sig, c.Signature.Payload, committer, keyID); commitVerification != nil { + } else if commitVerification := verifyWithGPGSettings(ctx, &gpgSettings, sig, c.Signature.Payload, committer, keyID); commitVerification != nil { if commitVerification.Reason == asymkey_model.BadSignature { defaultReason = asymkey_model.BadSignature } else { @@ -180,7 +175,7 @@ func ParseCommitWithSignatureCommitter(ctx context.Context, c *git.Commit, commi } else if defaultGPGSettings == nil { log.Warn("Unable to get defaultGPGSettings for unattached commit: %s", c.ID.String()) } else if defaultGPGSettings.Sign { - if commitVerification := VerifyWithGPGSettings(ctx, defaultGPGSettings, sig, c.Signature.Payload, committer, keyID); commitVerification != nil { + if commitVerification := verifyWithGPGSettings(ctx, defaultGPGSettings, sig, c.Signature.Payload, committer, keyID); commitVerification != nil { if commitVerification.Reason == asymkey_model.BadSignature { defaultReason = asymkey_model.BadSignature } else { @@ -295,7 +290,7 @@ func HashAndVerifyForKeyID(ctx context.Context, sig *packet.Signature, payload s } } -func VerifyWithGPGSettings(ctx context.Context, gpgSettings *git.GPGSettings, sig *packet.Signature, payload string, committer *user_model.User, keyID string) *asymkey_model.CommitVerification { +func verifyWithGPGSettings(ctx context.Context, gpgSettings *git.GPGSettings, sig *packet.Signature, payload string, committer *user_model.User, keyID string) *asymkey_model.CommitVerification { // First try to find the key in the db if commitVerification := HashAndVerifyForKeyID(ctx, sig, payload, committer, gpgSettings.KeyID, gpgSettings.Name, gpgSettings.Email); commitVerification != nil { return commitVerification @@ -375,10 +370,10 @@ func verifySSHCommitVerificationByInstanceKey(c *git.Commit, committerUser, sign return verifySSHCommitVerification(c.Signature.Signature, c.Signature.Payload, sshPubKey, committerUser, signerUser, committerGitEmail) } -// ParseCommitWithSSHSignature check if signature is good against keystore. -func ParseCommitWithSSHSignature(ctx context.Context, c *git.Commit, committerUser *user_model.User) *asymkey_model.CommitVerification { +// parseCommitWithSSHSignature check if signature is good against keystore. +func parseCommitWithSSHSignature(ctx context.Context, c *git.Commit, committerUser *user_model.User) *asymkey_model.CommitVerification { // Now try to associate the signature with the committer, if present - if committerUser.ID != 0 { + if committerUser != nil && committerUser.ID != 0 { keys, err := db.Find[asymkey_model.PublicKey](ctx, asymkey_model.FindPublicKeyOptions{ OwnerID: committerUser.ID, NotKeytype: asymkey_model.KeyTypePrincipal, diff --git a/services/asymkey/commit_test.go b/services/asymkey/commit_test.go index 0438209a61..6bcb6997f4 100644 --- a/services/asymkey/commit_test.go +++ b/services/asymkey/commit_test.go @@ -41,7 +41,7 @@ Initial commit with signed file Name: "User Two", Email: "user2@example.com", } - ret := ParseCommitWithSSHSignature(t.Context(), commit, committingUser) + ret := parseCommitWithSSHSignature(t.Context(), commit, committingUser) require.NotNil(t, ret) assert.True(t, ret.Verified) assert.False(t, ret.Warning) diff --git a/services/git/commit.go b/services/git/commit.go index 2e0e8a5096..e4755ef93d 100644 --- a/services/git/commit.go +++ b/services/git/commit.go @@ -35,13 +35,6 @@ func ParseCommitsWithSignature(ctx context.Context, repo *repo_model.Repository, for _, c := range oldCommits { committerUser := emailUsers.GetByEmail(c.Committer.Email) // FIXME: why ValidateCommitsWithEmails uses "Author", but ParseCommitsWithSignature uses "Committer"? - if committerUser == nil { - committerUser = &user_model.User{ - Name: c.Committer.Name, - Email: c.Committer.Email, - } - } - signCommit := &asymkey_model.SignCommit{ UserCommit: c, Verification: asymkey_service.ParseCommitWithSignatureCommitter(ctx, c.Commit, committerUser), diff --git a/templates/repo/commit_page.tmpl b/templates/repo/commit_page.tmpl index 46f641824b..68ccf9d275 100644 --- a/templates/repo/commit_page.tmpl +++ b/templates/repo/commit_page.tmpl @@ -147,7 +147,7 @@
{{if or (ne .Commit.Committer.Name .Commit.Author.Name) (ne .Commit.Committer.Email .Commit.Author.Email)}} {{ctx.Locale.Tr "repo.diff.committed_by"}} - {{if ne .Verification.CommittingUser.ID 0}} + {{if and .Verification.CommittingUser .Verification.CommittingUser.ID}} {{ctx.AvatarUtils.Avatar .Verification.CommittingUser 20}} {{.Commit.Committer.Name}} {{else}}