refactor

Author: wxiaoguang

models/asymkey/gpg_key_commit_verification.go

@@ -15,34 +15,15 @@ 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
 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

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
 

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",
+		}
+	}
+	// 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 this a SSH signature handle it differently
 	if strings.HasPrefix(c.Signature.Signature, "-----BEGIN SSH SIGNATURE-----") {
-		return ParseCommitWithSSHSignature(ctx, c, committer)
+		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,

services/asymkey/commit_test.go

@@ -41,7 +41,7 @@ Initial commit with signed file
 			Name:  "User Two",
 			Email: "[email protected]",
 		}
-		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)

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

templates/repo/commit_page.tmpl

@@ -147,7 +147,7 @@
 			<div class="flex-text-inline">
 				{{if or (ne .Commit.Committer.Name .Commit.Author.Name) (ne .Commit.Committer.Email .Commit.Author.Email)}}
 					<span class="text grey">{{ctx.Locale.Tr "repo.diff.committed_by"}}</span>
-					{{if ne .Verification.CommittingUser.ID 0}}
+					{{if and .Verification.CommittingUser .Verification.CommittingUser.ID}}
 						{{ctx.AvatarUtils.Avatar .Verification.CommittingUser 20}}
 						<a href="{{.Verification.CommittingUser.HomeLink}}"><strong>{{.Commit.Committer.Name}}</strong></a>
 					{{else}}