Update change password API

Modify the changing password API to support that admin user can change the password of normal users without old password

src/common/dao/dao_test.go

@@ -204,41 +204,6 @@ func TestRegister(t *testing.T) {
 	}
 }
 
-func TestCheckUserPassword(t *testing.T) {
-	nonExistUser := models.User{
-		Username: "non-exist",
-	}
-	correctUser := models.User{
-		Username: username,
-		Password: password,
-	}
-	wrongPwd := models.User{
-		Username: username,
-		Password: "wrong",
-	}
-	u, err := CheckUserPassword(nonExistUser)
-	if err != nil {
-		t.Errorf("Failed in CheckUserPassword: %v", err)
-	}
-	if u != nil {
-		t.Errorf("Expected nil for Non exist user, but actual: %+v", u)
-	}
-	u, err = CheckUserPassword(wrongPwd)
-	if err != nil {
-		t.Errorf("Failed in CheckUserPassword: %v", err)
-	}
-	if u != nil {
-		t.Errorf("Expected nil for user with wrong password, but actual: %+v", u)
-	}
-	u, err = CheckUserPassword(correctUser)
-	if err != nil {
-		t.Errorf("Failed in CheckUserPassword: %v", err)
-	}
-	if u == nil {
-		t.Errorf("User should not be nil for correct user")
-	}
-}
-
 func TestUserExists(t *testing.T) {
 	var exists bool
 	var err error
@@ -397,42 +362,6 @@ func TestChangeUserPassword(t *testing.T) {
 		t.Errorf("The username returned by Login does not match, expected: %s, acutal: %s", username, loginedUser.Username)
 	}
 }
-
-func TestChangeUserPasswordWithOldPassword(t *testing.T) {
-	user := models.User{UserID: currentUser.UserID}
-	query, err := GetUser(user)
-	if err != nil {
-		t.Errorf("Error occurred when get user salt")
-	}
-	currentUser.Salt = query.Salt
-
-	err = ChangeUserPassword(models.User{UserID: currentUser.UserID, Password: "NewerHarborTester12345", Salt: currentUser.Salt}, "NewHarborTester12345")
-	if err != nil {
-		t.Errorf("Error occurred in ChangeUserPassword: %v", err)
-	}
-	loginedUser, err := LoginByDb(models.AuthModel{Principal: currentUser.Username, Password: "NewerHarborTester12345"})
-	if err != nil {
-		t.Errorf("Error occurred in LoginByDb: %v", err)
-	}
-	if loginedUser.Username != username {
-		t.Errorf("The username returned by Login does not match, expected: %s, acutal: %s", username, loginedUser.Username)
-	}
-}
-
-func TestChangeUserPasswordWithIncorrectOldPassword(t *testing.T) {
-	err := ChangeUserPassword(models.User{UserID: currentUser.UserID, Password: "NNewerHarborTester12345", Salt: currentUser.Salt}, "WrongNewerHarborTester12345")
-	if err == nil {
-		t.Errorf("Error does not occurred due to old password is incorrect.")
-	}
-	loginedUser, err := LoginByDb(models.AuthModel{Principal: currentUser.Username, Password: "NNewerHarborTester12345"})
-	if err != nil {
-		t.Errorf("Error occurred in LoginByDb: %v", err)
-	}
-	if loginedUser != nil {
-		t.Errorf("The login user is not nil, acutal: %+v", loginedUser)
-	}
-}
-
 func TestAddProject(t *testing.T) {
 
 	project := models.Project{

src/common/dao/user.go

@@ -15,9 +15,9 @@
 package dao
 
 import (
-	"database/sql"
 	"errors"
 	"fmt"
+	"time"
 
 	"github.com/astaxie/beego/orm"
 
@@ -32,7 +32,7 @@ func GetUser(query models.User) (*models.User, error) {
 
 	o := GetOrmer()
 
-	sql := `select user_id, username, email, realname, comment, reset_uuid, salt,
+	sql := `select user_id, username, password, email, realname, comment, reset_uuid, salt,
 		sysadmin_flag, creation_time, update_time
 		from harbor_user u
 		where deleted = false `
@@ -153,35 +153,13 @@ func ToggleUserAdminRole(userID int, hasAdmin bool) error {
 }
 
 // ChangeUserPassword ...
-func ChangeUserPassword(u models.User, oldPassword ...string) (err error) {
-	if len(oldPassword) > 1 {
-		return errors.New("wrong numbers of params")
-	}
-
-	o := GetOrmer()
-
-	var r sql.Result
-	salt := utils.GenerateRandomString()
-	if len(oldPassword) == 0 {
-		//In some cases, it may no need to check old password, just as Linux change password policies.
-		r, err = o.Raw(`update harbor_user set password=?, salt=? where user_id=?`, utils.Encrypt(u.Password, salt), salt, u.UserID).Exec()
-	} else {
-		r, err = o.Raw(`update harbor_user set password=?, salt=? where user_id=? and password = ?`, utils.Encrypt(u.Password, salt), salt, u.UserID, utils.Encrypt(oldPassword[0], u.Salt)).Exec()
-	}
-
-	if err != nil {
+func ChangeUserPassword(u models.User) error {
+	u.UpdateTime = time.Now()
+	u.Salt = utils.GenerateRandomString()
+	u.Password = utils.Encrypt(u.Password, u.Salt)
+	_, err := GetOrmer().Update(&u, "Password", "Salt", "UpdateTime")
 	return err
 }
-	c, err := r.RowsAffected()
-	if err != nil {
-		return err
-	}
-	if c == 0 {
-		return errors.New("no record has been modified, change password failed")
-	}
-
-	return nil
-}
 
 // ResetUserPassword ...
 func ResetUserPassword(u models.User) error {
@@ -207,36 +185,6 @@ func UpdateUserResetUUID(u models.User) error {
 	return err
 }
 
-// CheckUserPassword checks whether the password is correct.
-func CheckUserPassword(query models.User) (*models.User, error) {
-
-	currentUser, err := GetUser(query)
-	if err != nil {
-		return nil, err
-	}
-	if currentUser == nil {
-		return nil, nil
-	}
-
-	sql := `select user_id, username, salt from harbor_user where deleted = false and username = ? and password = ?`
-	queryParam := make([]interface{}, 1)
-	queryParam = append(queryParam, currentUser.Username)
-	queryParam = append(queryParam, utils.Encrypt(query.Password, currentUser.Salt))
-	o := GetOrmer()
-	var user []models.User
-
-	n, err := o.Raw(sql, queryParam).QueryRows(&user)
-	if err != nil {
-		return nil, err
-	}
-	if n == 0 {
-		log.Warning("User principal does not match password. Current:", currentUser)
-		return nil, nil
-	}
-
-	return &user[0], nil
-}
-
 // DeleteUser ...
 func DeleteUser(userID int) error {
 	o := GetOrmer()

src/ui/api/user.go

@@ -24,6 +24,7 @@ import (
 	"github.com/vmware/harbor/src/common"
 	"github.com/vmware/harbor/src/common/dao"
 	"github.com/vmware/harbor/src/common/models"
+	"github.com/vmware/harbor/src/common/utils"
 	"github.com/vmware/harbor/src/common/utils/log"
 	"github.com/vmware/harbor/src/ui/config"
 )
@@ -114,6 +115,7 @@ func (ua *UserAPI) Get() {
 			log.Errorf("Error occurred in GetUser, error: %v", err)
 			ua.CustomAbort(http.StatusInternalServerError, "Internal error.")
 		}
+		u.Password = ""
 		ua.Data["json"] = u
 		ua.ServeJSON()
 		return
@@ -273,33 +275,48 @@ func (ua *UserAPI) ChangePassword() {
 		return
 	}
 
+	changePwdOfOwn := ua.userID == ua.currentUserID
+
 	var req passwordReq
 	ua.DecodeJSONReq(&req)
-	if req.OldPassword == "" {
-		log.Error("Old password is blank")
-		ua.CustomAbort(http.StatusBadRequest, "Old password is blank")
-	}
 
-	queryUser := models.User{UserID: ua.userID, Password: req.OldPassword}
-	user, err := dao.CheckUserPassword(queryUser)
-	if err != nil {
-		log.Errorf("Error occurred in CheckUserPassword: %v", err)
-		ua.CustomAbort(http.StatusInternalServerError, "Internal error.")
-	}
-	if user == nil {
-		log.Warning("Password input is not correct")
-		ua.CustomAbort(http.StatusForbidden, "old_password_is_not_correct")
-	}
-
-	if req.NewPassword == "" {
-		ua.HandleBadRequest("new password is null")
+	if changePwdOfOwn && len(req.OldPassword) == 0 {
+		ua.HandleBadRequest("empty old_password")
 		return
 	}
-	updateUser := models.User{UserID: ua.userID, Password: req.NewPassword, Salt: user.Salt}
-	err = dao.ChangeUserPassword(updateUser, req.OldPassword)
+
+	if len(req.NewPassword) == 0 {
+		ua.HandleBadRequest("empty new_password")
+		return
+	}
+
+	user, err := dao.GetUser(models.User{UserID: ua.userID})
 	if err != nil {
-		log.Errorf("Error occurred in ChangeUserPassword: %v", err)
-		ua.CustomAbort(http.StatusInternalServerError, "Internal error.")
+		ua.HandleInternalServerError(fmt.Sprintf("failed to get user %d: %v", ua.userID, err))
+		return
+	}
+	if user == nil {
+		ua.HandleNotFound(fmt.Sprintf("user %d not found", ua.userID))
+		return
+	}
+	if changePwdOfOwn {
+		if user.Password != utils.Encrypt(req.OldPassword, user.Salt) {
+			ua.HandleForbidden("incorrect old_password")
+			return
+		}
+	}
+	if user.Password == utils.Encrypt(req.NewPassword, user.Salt) {
+		ua.HandleBadRequest("the new password can not be same with the old one")
+		return
+	}
+
+	updatedUser := models.User{
+		UserID:   ua.userID,
+		Password: req.NewPassword,
+	}
+	if err = dao.ChangeUserPassword(updatedUser); err != nil {
+		ua.HandleInternalServerError(fmt.Sprintf("failed to change password of user %d: %v", ua.userID, err))
+		return
 	}
 }
 

src/ui/api/user_test.go

@@ -15,10 +15,15 @@ package api
 
 import (
 	"fmt"
+	"net/http"
 	"testing"
 
+	"github.com/stretchr/testify/require"
+	"github.com/vmware/harbor/src/common/dao"
+
 	"github.com/stretchr/testify/assert"
 	"github.com/vmware/harbor/src/common/api"
+	"github.com/vmware/harbor/src/common/models"
 	"github.com/vmware/harbor/tests/apitests/apilib"
 
 	"github.com/astaxie/beego"
@@ -323,82 +328,157 @@ func TestUsersToggleAdminRole(t *testing.T) {
 		assert.Equal(200, code, "Toggle user admin role status should be 200")
 	}
 }
-func TestUsersUpdatePassword(t *testing.T) {
-	fmt.Println("Testing Update User Password")
-	assert := assert.New(t)
-	apiTest := newHarborAPI()
-	password := apilib.Password{OldPassword: "", NewPassword: ""}
-	t.Log("Update password-case 1")
-	//case 1: update user2 password with user3 auth
-	code, err := apiTest.UsersUpdatePassword(testUser0002ID, password, *testUser0003Auth)
-	if err != nil {
-		t.Error("Error occured while update user password", err.Error())
-		t.Log(err)
-	} else {
-		assert.Equal(403, code, "Update user password status should be 403")
-	}
-	t.Log("Update password-case 2")
-	//case 2: update user2 password with admin auth, but oldpassword is empty
-	code, err = apiTest.UsersUpdatePassword(testUser0002ID, password, *admin)
-	if err != nil {
-		t.Error("Error occured while update user password", err.Error())
-		t.Log(err)
-	} else {
-		assert.Equal(400, code, "Update user password status should be 400")
-	}
-	t.Log("Update password-case 3")
-	//case 3: update user2 password with admin auth, but oldpassword is wrong
-	password.OldPassword = "000"
-	code, err = apiTest.UsersUpdatePassword(testUser0002ID, password, *admin)
-	if err != nil {
-		t.Error("Error occured while update user password", err.Error())
-		t.Log(err)
-	} else {
-		assert.Equal(403, code, "Update user password status should be 403")
-	}
-	t.Log("Update password-case 4")
-	//case 4: update user2 password with admin auth, but newpassword is empty
-	password.OldPassword = "testUser0002"
-	code, err = apiTest.UsersUpdatePassword(testUser0002ID, password, *admin)
-	if err != nil {
-		t.Error("Error occured while update user password", err.Error())
-		t.Log(err)
-	} else {
-		assert.Equal(400, code, "Update user password status should be 400")
-	}
-	t.Log("Update password-case 5")
-	//case 5: update user2 password with admin auth, right parameters
-	password.NewPassword = "TestUser0002"
-	code, err = apiTest.UsersUpdatePassword(testUser0002ID, password, *admin)
-	if err != nil {
-		t.Error("Error occured while update user password", err.Error())
-		t.Log(err)
-	} else {
-		assert.Equal(200, code, "Update user password status should be 200")
-		testUser0002.Password = password.NewPassword
-		testUser0002Auth.Passwd = password.NewPassword
-		//verify the new password takes effect
-		code, user, err := apiTest.UsersGetByID(testUser0002.Username, *testUser0002Auth, testUser0002ID)
-		if err != nil {
-			t.Error("Error occured while get users", err.Error())
-			t.Log(err)
-		} else {
-			assert.Equal(200, code, "Get users status should be 200")
-			assert.Equal(testUser0002.Username, user.Username, "Get users username should be equal")
-			assert.Equal(testUser0002.Email, user.Email, "Get users email should be equal")
+
+func buildChangeUserPasswordURL(id int) string {
+	return fmt.Sprintf("/api/users/%d/password", id)
 }
 
+func TestUsersUpdatePassword(t *testing.T) {
+	fmt.Println("Testing Update User Password")
+	oldPassword := "old_password"
+	newPassword := "new_password"
+
+	user01 := models.User{
+		Username: "user01_for_testing_change_password",
+		Email:    "user01_for_testing_change_password@test.com",
+		Password: oldPassword,
 	}
-	t.Log("Update password-case 6")
-	//case 6: update user2 password setting the new password same as the old
-	password.OldPassword = password.NewPassword
-	code, err = apiTest.UsersUpdatePassword(testUser0002ID, password, *admin)
-	if err != nil {
-		t.Error("Error occured while update user password", err.Error())
-		t.Log(err)
-	} else {
-		assert.Equal(200, code, "When new password is same as old, update user password status should be 200")
+	id, err := dao.Register(user01)
+	require.Nil(t, err)
+	user01.UserID = int(id)
+	defer dao.DeleteUser(user01.UserID)
+
+	user02 := models.User{
+		Username: "user02_for_testing_change_password",
+		Email:    "user02_for_testing_change_password@test.com",
+		Password: oldPassword,
 	}
+	id, err = dao.Register(user02)
+	require.Nil(t, err)
+	user02.UserID = int(id)
+	defer dao.DeleteUser(user02.UserID)
+
+	cases := []*codeCheckingCase{
+		// unauthorized
+		&codeCheckingCase{
+			request: &testingRequest{
+				method: http.MethodPut,
+				url:    buildChangeUserPasswordURL(user01.UserID),
+			},
+			code: http.StatusUnauthorized,
+		},
+		// 404
+		&codeCheckingCase{
+			request: &testingRequest{
+				method: http.MethodPut,
+				url:    buildChangeUserPasswordURL(10000),
+				credential: &usrInfo{
+					Name:   user01.Username,
+					Passwd: user01.Password,
+				},
+			},
+			code: http.StatusNotFound,
+		},
+		// 403, a normal user tries to change password of others
+		&codeCheckingCase{
+			request: &testingRequest{
+				method: http.MethodPut,
+				url:    buildChangeUserPasswordURL(user02.UserID),
+				credential: &usrInfo{
+					Name:   user01.Username,
+					Passwd: user01.Password,
+				},
+			},
+			code: http.StatusForbidden,
+		},
+		// 400, empty old password
+		&codeCheckingCase{
+			request: &testingRequest{
+				method:   http.MethodPut,
+				url:      buildChangeUserPasswordURL(user01.UserID),
+				bodyJSON: &passwordReq{},
+				credential: &usrInfo{
+					Name:   user01.Username,
+					Passwd: user01.Password,
+				},
+			},
+			code: http.StatusBadRequest,
+		},
+		// 400, empty new password
+		&codeCheckingCase{
+			request: &testingRequest{
+				method: http.MethodPut,
+				url:    buildChangeUserPasswordURL(user01.UserID),
+				bodyJSON: &passwordReq{
+					OldPassword: oldPassword,
+				},
+				credential: &usrInfo{
+					Name:   user01.Username,
+					Passwd: user01.Password,
+				},
+			},
+			code: http.StatusBadRequest,
+		},
+		// 403, incorrect old password
+		&codeCheckingCase{
+			request: &testingRequest{
+				method: http.MethodPut,
+				url:    buildChangeUserPasswordURL(user01.UserID),
+				bodyJSON: &passwordReq{
+					OldPassword: "incorrect_old_password",
+					NewPassword: newPassword,
+				},
+				credential: &usrInfo{
+					Name:   user01.Username,
+					Passwd: user01.Password,
+				},
+			},
+			code: http.StatusForbidden,
+		},
+		// 200, normal user change own password
+		&codeCheckingCase{
+			request: &testingRequest{
+				method: http.MethodPut,
+				url:    buildChangeUserPasswordURL(user01.UserID),
+				bodyJSON: &passwordReq{
+					OldPassword: oldPassword,
+					NewPassword: newPassword,
+				},
+				credential: &usrInfo{
+					Name:   user01.Username,
+					Passwd: user01.Password,
+				},
+			},
+			code: http.StatusOK,
+		},
+		// 400, admin user change password of others.
+		// the new password is same with the old one
+		&codeCheckingCase{
+			request: &testingRequest{
+				method: http.MethodPut,
+				url:    buildChangeUserPasswordURL(user01.UserID),
+				bodyJSON: &passwordReq{
+					NewPassword: newPassword,
+				},
+				credential: admin,
+			},
+			code: http.StatusBadRequest,
+		},
+		// 200, admin user change password of others
+		&codeCheckingCase{
+			request: &testingRequest{
+				method: http.MethodPut,
+				url:    buildChangeUserPasswordURL(user01.UserID),
+				bodyJSON: &passwordReq{
+					NewPassword: "another_new_password",
+				},
+				credential: admin,
+			},
+			code: http.StatusOK,
+		},
+	}
+
+	runCodeCheckingCases(t, cases...)
 }
 
 func TestUsersDelete(t *testing.T) {