move SaltGeneration into HashPasswort and rename it to what it does

Author: 6543

cmd/admin.go

@@ -349,12 +349,11 @@ func runChangePassword(c *cli.Context) error {
 	if err != nil {
 		return err
 	}
-	if user.Salt, err = models.GetUserSalt(); err != nil {
+	if err = user.SetPassword(c.String("password")); err != nil {
 		return err
 	}
-	user.HashPassword(c.String("password"))
 
-	if err := models.UpdateUserCols(user, "passwd", "passwd_hash_algo", "salt"); err != nil {
+	if err = models.UpdateUserCols(user, "passwd", "passwd_hash_algo", "salt"); err != nil {
 		return err
 	}
 

models/login_source.go

@@ -771,8 +771,10 @@ func UserSignIn(username, password string) (*User, error) {
 
 				// Update password hash if server password hash algorithm have changed
 				if user.PasswdHashAlgo != setting.PasswordHashAlgo {
-					user.HashPassword(password)
-					if err := UpdateUserCols(user, "passwd", "passwd_hash_algo"); err != nil {
+					if err = user.SetPassword(password); err != nil {
+						return nil, err
+					}
+					if err := UpdateUserCols(user, "passwd", "passwd_hash_algo", "salt"); err != nil {
 						return nil, err
 					}
 				}

models/user.go

@@ -395,10 +395,16 @@ func hashPassword(passwd, salt, algo string) string {
 	return fmt.Sprintf("%x", tempPasswd)
 }
 
-// HashPassword hashes a password using the algorithm defined in the config value of PASSWORD_HASH_ALGO.
-func (u *User) HashPassword(passwd string) {
+// SetPassword hashes a password using the algorithm defined in the config value of PASSWORD_HASH_ALGO
+// change passwd, salt and passwd_hash_algo fields
+func (u *User) SetPassword(passwd string) (err error) {
+	if u.Salt, err = GetUserSalt(); err != nil {
+		return err
+	}
 	u.PasswdHashAlgo = setting.PasswordHashAlgo
 	u.Passwd = hashPassword(passwd, u.Salt, setting.PasswordHashAlgo)
+
+	return nil
 }
 
 // ValidatePassword checks if given password matches the one belongs to the user.
@@ -416,7 +422,7 @@ func (u *User) ValidatePassword(passwd string) bool {
 
 // IsPasswordSet checks if the password is set or left empty
 func (u *User) IsPasswordSet() bool {
-	return !u.ValidatePassword("")
+	return len(u.Passwd) != 0
 }
 
 // IsOrganization returns true if user is actually a organization.
@@ -826,10 +832,9 @@ func CreateUser(u *User) (err error) {
 	if u.Rands, err = GetUserSalt(); err != nil {
 		return err
 	}
-	if u.Salt, err = GetUserSalt(); err != nil {
+	if err = u.SetPassword(u.Passwd); err != nil {
 		return err
 	}
-	u.HashPassword(u.Passwd)
 	u.AllowCreateOrganization = setting.Service.DefaultAllowCreateOrganization && !setting.Admin.DisableRegularOrgCreation
 	u.EmailNotificationsPreference = setting.Admin.DefaultEmailNotification
 	u.MaxRepoCreation = -1

models/user_test.go

@@ -220,8 +220,7 @@ func TestEmailNotificationPreferences(t *testing.T) {
 
 func TestHashPasswordDeterministic(t *testing.T) {
 	b := make([]byte, 16)
-	rand.Read(b)
-	u := &User{Salt: string(b)}
+	u := &User{}
 	algos := []string{"argon2", "pbkdf2", "scrypt", "bcrypt"}
 	for j := 0; j < len(algos); j++ {
 		u.PasswdHashAlgo = algos[j]
@@ -231,19 +230,15 @@ func TestHashPasswordDeterministic(t *testing.T) {
 			pass := string(b)
 
 			// save the current password in the user - hash it and store the result
-			u.HashPassword(pass)
+			u.SetPassword(pass)
 			r1 := u.Passwd
 
 			// run again
-			u.HashPassword(pass)
+			u.SetPassword(pass)
 			r2 := u.Passwd
 
-			// assert equal (given the same salt+pass, the same result is produced) except bcrypt
-			if u.PasswdHashAlgo == "bcrypt" {
-				assert.NotEqual(t, r1, r2)
-			} else {
-				assert.Equal(t, r1, r2)
-			}
+			assert.NotEqual(t, r1, r2)
+			assert.True(t, u.ValidatePassword(pass))
 		}
 	}
 }
@@ -252,12 +247,10 @@ func BenchmarkHashPassword(b *testing.B) {
 	// BenchmarkHashPassword ensures that it takes a reasonable amount of time
 	// to hash a password - in order to protect from brute-force attacks.
 	pass := "password1337"
-	bs := make([]byte, 16)
-	rand.Read(bs)
-	u := &User{Salt: string(bs), Passwd: pass}
+	u := &User{Passwd: pass}
 	b.ResetTimer()
 	for i := 0; i < b.N; i++ {
-		u.HashPassword(pass)
+		u.SetPassword(pass)
 	}
 }
 

routers/admin/users.go

@@ -266,7 +266,10 @@ func EditUserPost(ctx *context.Context, form auth.AdminEditUserForm) {
 			ctx.ServerError("UpdateUser", err)
 			return
 		}
-		u.HashPassword(form.Password)
+		if err = u.SetPassword(form.Password); err != nil {
+			ctx.InternalServerError(err)
+			return
+		}
 	}
 
 	if form.Reset2FA {

routers/api/v1/admin/user.go

@@ -174,7 +174,10 @@ func EditUser(ctx *context.APIContext, form api.EditUserOption) {
 			ctx.Error(http.StatusInternalServerError, "UpdateUser", err)
 			return
 		}
-		u.HashPassword(form.Password)
+		if err = u.SetPassword(form.Password); err != nil {
+			ctx.InternalServerError(err)
+			return
+		}
 	}
 
 	if form.MustChangePassword != nil {

routers/user/auth.go

@@ -1517,11 +1517,10 @@ func ResetPasswdPost(ctx *context.Context) {
 		ctx.ServerError("UpdateUser", err)
 		return
 	}
-	if u.Salt, err = models.GetUserSalt(); err != nil {
+	if err = u.SetPassword(passwd); err != nil {
 		ctx.ServerError("UpdateUser", err)
 		return
 	}
-	u.HashPassword(passwd)
 	u.MustChangePassword = false
 	if err := models.UpdateUserCols(u, "must_change_password", "passwd", "passwd_hash_algo", "rands", "salt"); err != nil {
 		ctx.ServerError("UpdateUser", err)
@@ -1591,12 +1590,11 @@ func MustChangePasswordPost(ctx *context.Context, cpt *captcha.Captcha, form aut
 	}
 
 	var err error
-	if u.Salt, err = models.GetUserSalt(); err != nil {
+	if err = u.SetPassword(form.Password); err != nil {
 		ctx.ServerError("UpdateUser", err)
 		return
 	}
 
-	u.HashPassword(form.Password)
 	u.MustChangePassword = false
 
 	if err := models.UpdateUserCols(u, "must_change_password", "passwd", "passwd_hash_algo", "salt"); err != nil {

routers/user/setting/account.go

@@ -63,11 +63,10 @@ func AccountPost(ctx *context.Context, form auth.ChangePasswordForm) {
 		ctx.Flash.Error(errMsg)
 	} else {
 		var err error
-		if ctx.User.Salt, err = models.GetUserSalt(); err != nil {
+		if err = ctx.User.SetPassword(form.Password); err != nil {
 			ctx.ServerError("UpdateUser", err)
 			return
 		}
-		ctx.User.HashPassword(form.Password)
 		if err := models.UpdateUserCols(ctx.User, "salt", "passwd_hash_algo", "passwd"); err != nil {
 			ctx.ServerError("UpdateUser", err)
 			return