From 76274dbf8459344621c367360b109672de69b756 Mon Sep 17 00:00:00 2001 From: Wenkai Yin 79628 Date: Tue, 22 May 2018 15:49:41 +0800 Subject: [PATCH] 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 | 71 ------------ src/common/dao/user.go | 68 ++---------- src/ui/api/user.go | 59 ++++++---- src/ui/api/user_test.go | 222 +++++++++++++++++++++++++------------ 4 files changed, 197 insertions(+), 223 deletions(-) diff --git a/src/common/dao/dao_test.go b/src/common/dao/dao_test.go index 504cbd3cb..ad2d1a466 100644 --- a/src/common/dao/dao_test.go +++ b/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{ diff --git a/src/common/dao/user.go b/src/common/dao/user.go index 7651920d4..3665e6937 100644 --- a/src/common/dao/user.go +++ b/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,34 +153,12 @@ 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 { - 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 +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 } // ResetUserPassword ... @@ -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() diff --git a/src/ui/api/user.go b/src/ui/api/user.go index 97b8036b1..5c3ec1aac 100644 --- a/src/ui/api/user.go +++ b/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 } } diff --git a/src/ui/api/user_test.go b/src/ui/api/user_test.go index 0af7e7a42..687bc0fd6 100644 --- a/src/ui/api/user_test.go +++ b/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 buildChangeUserPasswordURL(id int) string { + return fmt.Sprintf("/api/users/%d/password", id) +} + 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") - } + 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) {