Merge branch 'master' into 13682-review-requested-filter

Author: jpraet

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
 	}
 

integrations/admin_user_test.go

@@ -0,0 +1,82 @@
+// Copyright 2021 The Gitea Authors. All rights reserved.
+// Use of this source code is governed by a MIT-style
+// license that can be found in the LICENSE file.
+
+package integrations
+
+import (
+	"net/http"
+	"strconv"
+	"testing"
+
+	"code.gitea.io/gitea/models"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestAdminViewUsers(t *testing.T) {
+	prepareTestEnv(t)
+
+	session := loginUser(t, "user1")
+	req := NewRequest(t, "GET", "/admin/users")
+	session.MakeRequest(t, req, http.StatusOK)
+
+	session = loginUser(t, "user2")
+	req = NewRequest(t, "GET", "/admin/users")
+	session.MakeRequest(t, req, http.StatusForbidden)
+}
+
+func TestAdminViewUser(t *testing.T) {
+	prepareTestEnv(t)
+
+	session := loginUser(t, "user1")
+	req := NewRequest(t, "GET", "/admin/users/1")
+	session.MakeRequest(t, req, http.StatusOK)
+
+	session = loginUser(t, "user2")
+	req = NewRequest(t, "GET", "/admin/users/1")
+	session.MakeRequest(t, req, http.StatusForbidden)
+}
+
+func TestAdminEditUser(t *testing.T) {
+	prepareTestEnv(t)
+
+	testSuccessfullEdit(t, models.User{ID: 2, Name: "newusername", LoginName: "otherlogin", Email: "[email protected]"})
+}
+
+func testSuccessfullEdit(t *testing.T, formData models.User) {
+	makeRequest(t, formData, http.StatusFound)
+}
+
+func makeRequest(t *testing.T, formData models.User, headerCode int) {
+	session := loginUser(t, "user1")
+	csrf := GetCSRF(t, session, "/admin/users/"+strconv.Itoa(int(formData.ID)))
+	req := NewRequestWithValues(t, "POST", "/admin/users/"+strconv.Itoa(int(formData.ID)), map[string]string{
+		"_csrf":      csrf,
+		"user_name":  formData.Name,
+		"login_name": formData.LoginName,
+		"login_type": "0-0",
+		"email":      formData.Email,
+	})
+
+	session.MakeRequest(t, req, headerCode)
+	user := models.AssertExistsAndLoadBean(t, &models.User{ID: formData.ID}).(*models.User)
+	assert.Equal(t, formData.Name, user.Name)
+	assert.Equal(t, formData.LoginName, user.LoginName)
+	assert.Equal(t, formData.Email, user.Email)
+}
+
+func TestAdminDeleteUser(t *testing.T) {
+	defer prepareTestEnv(t)()
+
+	session := loginUser(t, "user1")
+
+	csrf := GetCSRF(t, session, "/admin/users/8")
+	req := NewRequestWithValues(t, "POST", "/admin/users/8/delete", map[string]string{
+		"_csrf": csrf,
+	})
+	session.MakeRequest(t, req, http.StatusOK)
+
+	assertUserDeleted(t, 8)
+	models.CheckConsistencyFor(t, &models.User{})
+}

integrations/delete_user_test.go

@@ -24,21 +24,6 @@ func assertUserDeleted(t *testing.T, userID int64) {
 	models.AssertNotExistsBean(t, &models.Star{UID: userID})
 }
 
-func TestAdminDeleteUser(t *testing.T) {
-	defer prepareTestEnv(t)()
-
-	session := loginUser(t, "user1")
-
-	csrf := GetCSRF(t, session, "/admin/users/8")
-	req := NewRequestWithValues(t, "POST", "/admin/users/8/delete", map[string]string{
-		"_csrf": csrf,
-	})
-	session.MakeRequest(t, req, http.StatusOK)
-
-	assertUserDeleted(t, 8)
-	models.CheckConsistencyFor(t, &models.User{})
-}
-
 func TestUserDeleteAccount(t *testing.T) {
 	defer prepareTestEnv(t)()
 

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/migrations/migrations.go

@@ -277,6 +277,8 @@ var migrations = []Migration{
 	NewMigration("Add scope and nonce columns to oauth2_grant table", addScopeAndNonceColumnsToOAuth2Grant),
 	// v165 -> v166
 	NewMigration("Convert hook task type from char(16) to varchar(16) and trim the column", convertHookTaskTypeToVarcharAndTrim),
+	// v166 -> v167
+	NewMigration("Where Password is Valid with Empty String delete it", recalculateUserEmptyPWD),
 }
 
 // GetCurrentDBVersion returns the current db version

models/migrations/v166.go

@@ -0,0 +1,115 @@
+// Copyright 2021 The Gitea Authors. All rights reserved.
+// Use of this source code is governed by a MIT-style
+// license that can be found in the LICENSE file.
+
+package migrations
+
+import (
+	"crypto/sha256"
+	"fmt"
+
+	"golang.org/x/crypto/argon2"
+	"golang.org/x/crypto/bcrypt"
+	"golang.org/x/crypto/pbkdf2"
+	"golang.org/x/crypto/scrypt"
+	"xorm.io/builder"
+	"xorm.io/xorm"
+)
+
+func recalculateUserEmptyPWD(x *xorm.Engine) (err error) {
+	const (
+		algoBcrypt = "bcrypt"
+		algoScrypt = "scrypt"
+		algoArgon2 = "argon2"
+		algoPbkdf2 = "pbkdf2"
+	)
+
+	type User struct {
+		ID                 int64  `xorm:"pk autoincr"`
+		Passwd             string `xorm:"NOT NULL"`
+		PasswdHashAlgo     string `xorm:"NOT NULL DEFAULT 'argon2'"`
+		MustChangePassword bool   `xorm:"NOT NULL DEFAULT false"`
+		LoginType          int
+		LoginName          string
+		Type               int
+		Salt               string `xorm:"VARCHAR(10)"`
+	}
+
+	// hashPassword hash password based on algo and salt
+	// state 461406070c
+	hashPassword := func(passwd, salt, algo string) string {
+		var tempPasswd []byte
+
+		switch algo {
+		case algoBcrypt:
+			tempPasswd, _ = bcrypt.GenerateFromPassword([]byte(passwd), bcrypt.DefaultCost)
+			return string(tempPasswd)
+		case algoScrypt:
+			tempPasswd, _ = scrypt.Key([]byte(passwd), []byte(salt), 65536, 16, 2, 50)
+		case algoArgon2:
+			tempPasswd = argon2.IDKey([]byte(passwd), []byte(salt), 2, 65536, 8, 50)
+		case algoPbkdf2:
+			fallthrough
+		default:
+			tempPasswd = pbkdf2.Key([]byte(passwd), []byte(salt), 10000, 50, sha256.New)
+		}
+
+		return fmt.Sprintf("%x", tempPasswd)
+	}
+
+	// ValidatePassword checks if given password matches the one belongs to the user.
+	// state 461406070c, changed since it's not necessary to be time constant
+	ValidatePassword := func(u *User, passwd string) bool {
+		tempHash := hashPassword(passwd, u.Salt, u.PasswdHashAlgo)
+
+		if u.PasswdHashAlgo != algoBcrypt && u.Passwd == tempHash {
+			return true
+		}
+		if u.PasswdHashAlgo == algoBcrypt && bcrypt.CompareHashAndPassword([]byte(u.Passwd), []byte(passwd)) == nil {
+			return true
+		}
+		return false
+	}
+
+	sess := x.NewSession()
+	defer sess.Close()
+
+	const batchSize = 100
+
+	for start := 0; ; start += batchSize {
+		users := make([]*User, 0, batchSize)
+		if err = sess.Limit(batchSize, start).Where(builder.Neq{"passwd": ""}, 0).Find(&users); err != nil {
+			return
+		}
+		if len(users) == 0 {
+			break
+		}
+
+		if err = sess.Begin(); err != nil {
+			return
+		}
+
+		for _, user := range users {
+			if ValidatePassword(user, "") {
+				user.Passwd = ""
+				user.Salt = ""
+				user.PasswdHashAlgo = ""
+				if _, err = sess.ID(user.ID).Cols("passwd", "salt", "passwd_hash_algo").Update(user); err != nil {
+					return err
+				}
+			}
+		}
+
+		if err = sess.Commit(); err != nil {
+			return
+		}
+	}
+
+	// delete salt and algo where password is empty
+	if _, err = sess.Where(builder.Eq{"passwd": ""}.And(builder.Neq{"salt": ""}.Or(builder.Neq{"passwd_hash_algo": ""}))).
+		Cols("salt", "passwd_hash_algo").Update(&User{}); err != nil {
+		return err
+	}
+
+	return sess.Commit()
+}

models/user.go

@@ -395,10 +395,23 @@ 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 len(passwd) == 0 {
+		u.Passwd = ""
+		u.Salt = ""
+		u.PasswdHashAlgo = ""
+		return nil
+	}
+
+	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 +429,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 +839,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
@@ -913,19 +925,19 @@ func ChangeUserName(u *User, newUserName string) (err error) {
 		return err
 	}
 
-	isExist, err := IsUserExist(0, newUserName)
-	if err != nil {
-		return err
-	} else if isExist {
-		return ErrUserAlreadyExist{newUserName}
-	}
-
 	sess := x.NewSession()
 	defer sess.Close()
 	if err = sess.Begin(); err != nil {
 		return err
 	}
 
+	isExist, err := isUserExist(sess, 0, newUserName)
+	if err != nil {
+		return err
+	} else if isExist {
+		return ErrUserAlreadyExist{newUserName}
+	}
+
 	if _, err = sess.Exec("UPDATE `repository` SET owner_name=? WHERE owner_name=?", newUserName, u.Name); err != nil {
 		return fmt.Errorf("Change repo owner name: %v", err)
 	}

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

modules/auth/admin.go

@@ -28,6 +28,7 @@ func (f *AdminCreateUserForm) Validate(ctx *macaron.Context, errs binding.Errors
 // AdminEditUserForm form for admin to create user
 type AdminEditUserForm struct {
 	LoginType               string `binding:"Required"`
+	UserName                string `binding:"AlphaDashDot;MaxSize(40)"`
 	LoginName               string
 	FullName                string `binding:"MaxSize(100)"`
 	Email                   string `binding:"Required;Email;MaxSize(254)"`

options/locale/locale_en-US.ini

@@ -359,6 +359,7 @@ password_not_match = The passwords do not match.
 lang_select_error = Select a language from the list.
 
 username_been_taken = The username is already taken.
+username_change_not_local_user = Non-local users are not allowed to change their username.
 repo_name_been_taken = The repository name is already used.
 repository_files_already_exist = Files already exist for this repository. Contact the system administrator.
 repository_files_already_exist.adopt = Files already exist for this repository and can only be Adopted.