refactor(db): migrate password methods off `user.go` (#7205)

13 files changed, 181 insertions, 43 deletions

diff --git a/internal/db/pull.go b/internal/db/pull.go
index 11298e0c..e2f55361 100644
--- a/internal/db/pull.go
+++ b/internal/db/pull.go
@@ -270,10 +270,9 @@ func (pr *PullRequest) Merge(doer *User, baseGitRepo *git.Repository, mergeStyle
 		}
 
 		// Create a merge commit for the base branch.
-		sig := doer.NewGitSig()
 		if _, stderr, err = process.ExecDir(-1, tmpBasePath,
 			fmt.Sprintf("PullRequest.Merge (git merge): %s", tmpBasePath),
-			"git", "commit", fmt.Sprintf("--author='%s <%s>'", sig.Name, sig.Email),
+			"git", "commit", fmt.Sprintf("--author='%s <%s>'", doer.DisplayName(), doer.Email),
 			"-m", fmt.Sprintf("Merge branch '%s' of %s/%s into %s", pr.HeadBranch, pr.HeadUserName, pr.HeadRepo.Name, pr.BaseBranch),
 			"-m", commitDescription); err != nil {
 			return fmt.Errorf("git commit [%s]: %v - %s", tmpBasePath, err, stderr)
diff --git a/internal/db/repo.go b/internal/db/repo.go
index 97228015..7c307144 100644
--- a/internal/db/repo.go
+++ b/internal/db/repo.go
@@ -1040,7 +1040,15 @@ func initRepository(e Engine, repoPath string, doer *User, repo *Repository, opt
 		}
 
 		// Apply changes and commit.
-		if err = initRepoCommit(tmpDir, doer.NewGitSig()); err != nil {
+		err = initRepoCommit(
+			tmpDir,
+			&git.Signature{
+				Name:  doer.DisplayName(),
+				Email: doer.Email,
+				When:  time.Now(),
+			},
+		)
+		if err != nil {
 			return fmt.Errorf("initRepoCommit: %v", err)
 		}
 	}
diff --git a/internal/db/repo_editor.go b/internal/db/repo_editor.go
index 63c88b76..3edb16e2 100644
--- a/internal/db/repo_editor.go
+++ b/internal/db/repo_editor.go
@@ -183,7 +183,18 @@ func (repo *Repository) UpdateRepoFile(doer *User, opts UpdateRepoFileOptions) (
 
 	if err = git.Add(localPath, git.AddOptions{All: true}); err != nil {
 		return fmt.Errorf("git add --all: %v", err)
-	} else if err = git.CreateCommit(localPath, doer.NewGitSig(), opts.Message); err != nil {
+	}
+
+	err = git.CreateCommit(
+		localPath,
+		&git.Signature{
+			Name:  doer.DisplayName(),
+			Email: doer.Email,
+			When:  time.Now(),
+		},
+		opts.Message,
+	)
+	if err != nil {
 		return fmt.Errorf("commit changes on %q: %v", localPath, err)
 	}
 
@@ -294,7 +305,18 @@ func (repo *Repository) DeleteRepoFile(doer *User, opts DeleteRepoFileOptions) (
 
 	if err = git.Add(localPath, git.AddOptions{All: true}); err != nil {
 		return fmt.Errorf("git add --all: %v", err)
-	} else if err = git.CreateCommit(localPath, doer.NewGitSig(), opts.Message); err != nil {
+	}
+
+	err = git.CreateCommit(
+		localPath,
+		&git.Signature{
+			Name:  doer.DisplayName(),
+			Email: doer.Email,
+			When:  time.Now(),
+		},
+		opts.Message,
+	)
+	if err != nil {
 		return fmt.Errorf("commit changes to %q: %v", localPath, err)
 	}
 
@@ -531,7 +553,18 @@ func (repo *Repository) UploadRepoFiles(doer *User, opts UploadRepoFileOptions)
 
 	if err = git.Add(localPath, git.AddOptions{All: true}); err != nil {
 		return fmt.Errorf("git add --all: %v", err)
-	} else if err = git.CreateCommit(localPath, doer.NewGitSig(), opts.Message); err != nil {
+	}
+
+	err = git.CreateCommit(
+		localPath,
+		&git.Signature{
+			Name:  doer.DisplayName(),
+			Email: doer.Email,
+			When:  time.Now(),
+		},
+		opts.Message,
+	)
+	if err != nil {
 		return fmt.Errorf("commit changes on %q: %v", localPath, err)
 	}
 
diff --git a/internal/db/user.go b/internal/db/user.go
index 95ee70c2..bf432a6a 100644
--- a/internal/db/user.go
+++ b/internal/db/user.go
@@ -7,8 +7,6 @@ package db
 import (
 	"bytes"
 	"context"
-	"crypto/sha256"
-	"crypto/subtle"
 	"encoding/hex"
 	"fmt"
 	"image"
@@ -22,7 +20,6 @@ import (
 
 	"github.com/nfnt/resize"
 	"github.com/unknwon/com"
-	"golang.org/x/crypto/pbkdf2"
 	log "unknwon.dev/clog/v2"
 	"xorm.io/xorm"
 
@@ -61,28 +58,6 @@ func (u *User) AfterSet(colName string, _ xorm.Cell) {
 	}
 }
 
-// NewGitSig generates and returns the signature of given user.
-func (u *User) NewGitSig() *git.Signature {
-	return &git.Signature{
-		Name:  u.DisplayName(),
-		Email: u.Email,
-		When:  time.Now(),
-	}
-}
-
-// EncodePassword encodes password to safe format.
-func (u *User) EncodePassword() {
-	newPasswd := pbkdf2.Key([]byte(u.Password), []byte(u.Salt), 10000, 50, sha256.New)
-	u.Password = fmt.Sprintf("%x", newPasswd)
-}
-
-// ValidatePassword checks if given password matches the one belongs to the user.
-func (u *User) ValidatePassword(passwd string) bool {
-	newUser := &User{Password: passwd, Salt: u.Salt}
-	newUser.EncodePassword()
-	return subtle.ConstantTimeCompare([]byte(u.Password), []byte(newUser.Password)) == 1
-}
-
 // UploadAvatar saves custom avatar for user.
 // FIXME: split uploads to different subdirs in case we have massive number of users.
 func (u *User) UploadAvatar(data []byte) error {
@@ -343,7 +318,7 @@ func CreateUser(u *User) (err error) {
 	if u.Salt, err = GetUserSalt(); err != nil {
 		return err
 	}
-	u.EncodePassword()
+	u.Password = userutil.EncodePassword(u.Password, u.Salt)
 	u.MaxRepoCreation = -1
 
 	sess := x.NewSession()
diff --git a/internal/db/users.go b/internal/db/users.go
index d537b8d3..fa327157 100644
--- a/internal/db/users.go
+++ b/internal/db/users.go
@@ -117,7 +117,7 @@ func (db *users) Authenticate(ctx context.Context, login, password string, login
 
 		// Validate password hash fetched from database for local accounts.
 		if user.IsLocal() {
-			if user.ValidatePassword(password) {
+			if userutil.ValidatePassword(user.Password, user.Salt, password) {
 				return user, nil
 			}
 
@@ -262,7 +262,7 @@ func (db *users) Create(ctx context.Context, username, email string, opts Create
 	if err != nil {
 		return nil, err
 	}
-	user.EncodePassword()
+	user.Password = userutil.EncodePassword(user.Password, user.Salt)
 
 	return user, db.WithContext(ctx).Create(user).Error
 }
diff --git a/internal/db/wiki.go b/internal/db/wiki.go
index 9f184929..8606b9a5 100644
--- a/internal/db/wiki.go
+++ b/internal/db/wiki.go
@@ -12,6 +12,7 @@ import (
 	"path"
 	"path/filepath"
 	"strings"
+	"time"
 
 	"github.com/unknwon/com"
 
@@ -130,7 +131,18 @@ func (repo *Repository) updateWikiPage(doer *User, oldTitle, title, content, mes
 	}
 	if err = git.Add(localPath, git.AddOptions{All: true}); err != nil {
 		return fmt.Errorf("add all changes: %v", err)
-	} else if err = git.CreateCommit(localPath, doer.NewGitSig(), message); err != nil {
+	}
+
+	err = git.CreateCommit(
+		localPath,
+		&git.Signature{
+			Name:  doer.DisplayName(),
+			Email: doer.Email,
+			When:  time.Now(),
+		},
+		message,
+	)
+	if err != nil {
 		return fmt.Errorf("commit changes: %v", err)
 	} else if err = git.Push(localPath, "origin", "master"); err != nil {
 		return fmt.Errorf("push: %v", err)
@@ -166,7 +178,18 @@ func (repo *Repository) DeleteWikiPage(doer *User, title string) (err error) {
 
 	if err = git.Add(localPath, git.AddOptions{All: true}); err != nil {
 		return fmt.Errorf("add all changes: %v", err)
-	} else if err = git.CreateCommit(localPath, doer.NewGitSig(), message); err != nil {
+	}
+
+	err = git.CreateCommit(
+		localPath,
+		&git.Signature{
+			Name:  doer.DisplayName(),
+			Email: doer.Email,
+			When:  time.Now(),
+		},
+		message,
+	)
+	if err != nil {
 		return fmt.Errorf("commit changes: %v", err)
 	} else if err = git.Push(localPath, "origin", "master"); err != nil {
 		return fmt.Errorf("push: %v", err)
diff --git a/internal/route/admin/users.go b/internal/route/admin/users.go
index 9b273b36..ad876011 100644
--- a/internal/route/admin/users.go
+++ b/internal/route/admin/users.go
@@ -16,6 +16,7 @@ import (
 	"gogs.io/gogs/internal/email"
 	"gogs.io/gogs/internal/form"
 	"gogs.io/gogs/internal/route"
+	"gogs.io/gogs/internal/userutil"
 )
 
 const (
@@ -192,7 +193,7 @@ func EditUserPost(c *context.Context, f form.AdminEditUser) {
 			c.Error(err, "get user salt")
 			return
 		}
-		u.EncodePassword()
+		u.Password = userutil.EncodePassword(u.Password, u.Salt)
 	}
 
 	u.LoginName = f.LoginName
diff --git a/internal/route/api/v1/admin/user.go b/internal/route/api/v1/admin/user.go
index 7fad5ed1..59550f4c 100644
--- a/internal/route/api/v1/admin/user.go
+++ b/internal/route/api/v1/admin/user.go
@@ -15,6 +15,7 @@ import (
 	"gogs.io/gogs/internal/db"
 	"gogs.io/gogs/internal/email"
 	"gogs.io/gogs/internal/route/api/v1/user"
+	"gogs.io/gogs/internal/userutil"
 )
 
 func parseLoginSource(c *context.APIContext, u *db.User, sourceID int64, loginName string) {
@@ -88,7 +89,7 @@ func EditUser(c *context.APIContext, form api.EditUserOption) {
 			c.Error(err, "get user salt")
 			return
 		}
-		u.EncodePassword()
+		u.Password = userutil.EncodePassword(u.Password, u.Salt)
 	}
 
 	u.LoginName = form.LoginName
diff --git a/internal/route/repo/webhook.go b/internal/route/repo/webhook.go
index 3ccb205e..f52e80c0 100644
--- a/internal/route/repo/webhook.go
+++ b/internal/route/repo/webhook.go
@@ -9,6 +9,7 @@ import (
 	"net/http"
 	"net/url"
 	"strings"
+	"time"
 
 	"github.com/gogs/git-module"
 	api "github.com/gogs/go-gogs-client"
@@ -475,8 +476,12 @@ func TestWebhook(c *context.Context) {
 		commitID = git.EmptyID
 		commitMessage = "This is a fake commit"
 		ghost := db.NewGhostUser()
-		author = ghost.NewGitSig()
-		committer = ghost.NewGitSig()
+		author = &git.Signature{
+			Name:  ghost.DisplayName(),
+			Email: ghost.Email,
+			When:  time.Now(),
+		}
+		committer = author
 		authorUsername = ghost.Name
 		committerUsername = ghost.Name
 		nameStatus = &git.NameStatus{}
diff --git a/internal/route/user/auth.go b/internal/route/user/auth.go
index 213ff050..d06a7488 100644
--- a/internal/route/user/auth.go
+++ b/internal/route/user/auth.go
@@ -19,6 +19,7 @@ import (
 	"gogs.io/gogs/internal/email"
 	"gogs.io/gogs/internal/form"
 	"gogs.io/gogs/internal/tool"
+	"gogs.io/gogs/internal/userutil"
 )
 
 const (
@@ -554,7 +555,7 @@ func ResetPasswdPost(c *context.Context) {
 			c.Error(err, "get user salt")
 			return
 		}
-		u.EncodePassword()
+		u.Password = userutil.EncodePassword(u.Password, u.Salt)
 		if err := db.UpdateUser(u); err != nil {
 			c.Error(err, "update user")
 			return
diff --git a/internal/route/user/setting.go b/internal/route/user/setting.go
index aae38d60..cdb5eee2 100644
--- a/internal/route/user/setting.go
+++ b/internal/route/user/setting.go
@@ -198,7 +198,7 @@ func SettingsPasswordPost(c *context.Context, f form.ChangePassword) {
 		return
 	}
 
-	if !c.User.ValidatePassword(f.OldPassword) {
+	if !userutil.ValidatePassword(c.User.Password, c.User.Salt, f.OldPassword) {
 		c.Flash.Error(c.Tr("settings.password_incorrect"))
 	} else if f.Password != f.Retype {
 		c.Flash.Error(c.Tr("form.password_not_match"))
@@ -209,7 +209,7 @@ func SettingsPasswordPost(c *context.Context, f form.ChangePassword) {
 			c.Errorf(err, "get user salt")
 			return
 		}
-		c.User.EncodePassword()
+		c.User.Password = userutil.EncodePassword(c.User.Password, c.User.Salt)
 		if err := db.UpdateUser(c.User); err != nil {
 			c.Errorf(err, "update user")
 			return
diff --git a/internal/userutil/userutil.go b/internal/userutil/userutil.go
index d5c74325..8063aef0 100644
--- a/internal/userutil/userutil.go
+++ b/internal/userutil/userutil.go
@@ -5,6 +5,8 @@
 package userutil
 
 import (
+	"crypto/sha256"
+	"crypto/subtle"
 	"encoding/hex"
 	"fmt"
 	"image/png"
@@ -14,6 +16,7 @@ import (
 	"strings"
 
 	"github.com/pkg/errors"
+	"golang.org/x/crypto/pbkdf2"
 
 	"gogs.io/gogs/internal/avatar"
 	"gogs.io/gogs/internal/conf"
@@ -77,3 +80,16 @@ func GenerateRandomAvatar(userID int64, name, email string) error {
 	}
 	return nil
 }
+
+// EncodePassword encodes password using PBKDF2 SHA256 with given salt.
+func EncodePassword(password, salt string) string {
+	newPasswd := pbkdf2.Key([]byte(password), []byte(salt), 10000, 50, sha256.New)
+	return fmt.Sprintf("%x", newPasswd)
+}
+
+// ValidatePassword returns true if the given password matches the encoded
+// version with given salt.
+func ValidatePassword(encoded, salt, password string) bool {
+	got := EncodePassword(password, salt)
+	return subtle.ConstantTimeCompare([]byte(encoded), []byte(got)) == 1
+}
diff --git a/internal/userutil/userutil_test.go b/internal/userutil/userutil_test.go
index e90c9235..10ffa12a 100644
--- a/internal/userutil/userutil_test.go
+++ b/internal/userutil/userutil_test.go
@@ -77,3 +77,79 @@ func TestGenerateRandomAvatar(t *testing.T) {
 	got := osutil.IsFile(CustomAvatarPath(1))
 	assert.True(t, got)
 }
+
+func TestEncodePassword(t *testing.T) {
+	want := EncodePassword("123456", "rands")
+	tests := []struct {
+		name      string
+		password  string
+		rands     string
+		wantEqual bool
+	}{
+		{
+			name:      "correct",
+			password:  "123456",
+			rands:     "rands",
+			wantEqual: true,
+		},
+
+		{
+			name:      "wrong password",
+			password:  "111333",
+			rands:     "rands",
+			wantEqual: false,
+		},
+		{
+			name:      "wrong salt",
+			password:  "111333",
+			rands:     "salt",
+			wantEqual: false,
+		},
+	}
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			got := EncodePassword(test.password, test.rands)
+			if test.wantEqual {
+				assert.Equal(t, want, got)
+			} else {
+				assert.NotEqual(t, want, got)
+			}
+		})
+	}
+}
+
+func TestValidatePassword(t *testing.T) {
+	want := EncodePassword("123456", "rands")
+	tests := []struct {
+		name      string
+		password  string
+		rands     string
+		wantEqual bool
+	}{
+		{
+			name:      "correct",
+			password:  "123456",
+			rands:     "rands",
+			wantEqual: true,
+		},
+
+		{
+			name:      "wrong password",
+			password:  "111333",
+			rands:     "rands",
+			wantEqual: false,
+		},
+		{
+			name:      "wrong salt",
+			password:  "111333",
+			rands:     "salt",
+			wantEqual: false,
+		},
+	}
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			got := ValidatePassword(want, test.rands, test.password)
+			assert.Equal(t, test.wantEqual, got)
+		})
+	}
+}