From 1a6777cb93511e258e75ba6be41647c3c877fc53 Mon Sep 17 00:00:00 2001 From: Karl Persson Date: Wed, 17 Apr 2024 15:24:36 +0200 Subject: [PATCH] User: use update function for password updates (#86419) * Update password through Update function instead * Remove duplicated to lower * Refactor password code --- pkg/api/admin_users.go | 41 ++---------- pkg/api/admin_users_test.go | 3 +- pkg/api/org_invite.go | 4 -- pkg/api/password.go | 35 +++------- pkg/api/signup.go | 4 -- pkg/api/user.go | 66 +++---------------- pkg/api/user_test.go | 9 ++- pkg/api/utils.go | 31 +++++++++ .../commands/reset_password_command.go | 10 +-- pkg/services/org/orgimpl/store_test.go | 2 +- pkg/services/user/error.go | 1 + pkg/services/user/model.go | 13 ++-- pkg/services/user/password.go | 11 +++- pkg/services/user/password_test.go | 2 +- pkg/services/user/user.go | 1 - pkg/services/user/userimpl/store.go | 24 ++----- pkg/services/user/userimpl/store_test.go | 34 +++++++++- pkg/services/user/userimpl/user.go | 44 ++++++++++--- pkg/services/user/userimpl/user_test.go | 30 +++++++-- pkg/services/user/usertest/fake.go | 4 -- pkg/services/user/usertest/mock.go | 18 ----- 21 files changed, 182 insertions(+), 205 deletions(-) diff --git a/pkg/api/admin_users.go b/pkg/api/admin_users.go index 8f8df9eaad6..3908900f8e6 100644 --- a/pkg/api/admin_users.go +++ b/pkg/api/admin_users.go @@ -5,7 +5,6 @@ import ( "fmt" "net/http" "strconv" - "strings" "golang.org/x/sync/errgroup" @@ -19,7 +18,6 @@ import ( "github.com/grafana/grafana/pkg/services/login" "github.com/grafana/grafana/pkg/services/org" "github.com/grafana/grafana/pkg/services/user" - "github.com/grafana/grafana/pkg/util" "github.com/grafana/grafana/pkg/web" ) @@ -46,9 +44,6 @@ func (hs *HTTPServer) AdminCreateUser(c *contextmodel.ReqContext) response.Respo return response.Error(http.StatusBadRequest, "bad request data", err) } - form.Email = strings.TrimSpace(form.Email) - form.Login = strings.TrimSpace(form.Login) - cmd := user.CreateUserCommand{ Login: form.Login, Email: form.Email, @@ -57,10 +52,6 @@ func (hs *HTTPServer) AdminCreateUser(c *contextmodel.ReqContext) response.Respo OrgID: form.OrgId, } - if len(cmd.Password) < 4 { - return response.Error(http.StatusBadRequest, "Password is missing or too short", nil) - } - usr, err := hs.userService.Create(c.Req.Context(), &cmd) if err != nil { if errors.Is(err, org.ErrOrgNotFound) { @@ -115,37 +106,17 @@ func (hs *HTTPServer) AdminUpdateUserPassword(c *contextmodel.ReqContext) respon return response.Error(http.StatusBadRequest, "id is invalid", err) } - if err := form.Password.Validate(hs.Cfg); err != nil { - return response.Err(err) + if response := hs.errOnExternalUser(c.Req.Context(), userID); response != nil { + return response } - userQuery := user.GetUserByIDQuery{ID: userID} - - usr, err := hs.userService.GetByID(c.Req.Context(), &userQuery) - if err != nil { - return response.Error(http.StatusInternalServerError, "Could not read user from database", err) - } - - getAuthQuery := login.GetAuthInfoQuery{UserId: usr.ID} - if authInfo, err := hs.authInfoService.GetAuthInfo(c.Req.Context(), &getAuthQuery); err == nil { - oauthInfo := hs.SocialService.GetOAuthInfoProvider(authInfo.AuthModule) - if login.IsProviderEnabled(hs.Cfg, authInfo.AuthModule, oauthInfo) { - return response.Error(http.StatusBadRequest, "Cannot update external user password", err) - } + if err := hs.userService.Update(c.Req.Context(), &user.UpdateUserCommand{UserID: userID, Password: &form.Password}); err != nil { + return response.Error(http.StatusInternalServerError, "Failed to update user password", err) } - passwordHashed, err := util.EncodePassword(string(form.Password), usr.Salt) + usr, err := hs.userService.GetByID(c.Req.Context(), &user.GetUserByIDQuery{ID: userID}) if err != nil { - return response.Error(http.StatusInternalServerError, "Could not encode password", err) - } - - cmd := user.ChangeUserPasswordCommand{ - UserID: userID, - NewPassword: user.Password(passwordHashed), - } - - if err := hs.userService.ChangePassword(c.Req.Context(), &cmd); err != nil { - return response.Error(http.StatusInternalServerError, "Failed to update user password", err) + return response.Error(http.StatusInternalServerError, "Could not read user from database", err) } if err := hs.loginAttemptService.Reset(c.Req.Context(), diff --git a/pkg/api/admin_users_test.go b/pkg/api/admin_users_test.go index d40417f72f3..0b6011d95e2 100644 --- a/pkg/api/admin_users_test.go +++ b/pkg/api/admin_users_test.go @@ -355,8 +355,9 @@ func putAdminScenario(t *testing.T, desc string, url string, routePattern string hs := &HTTPServer{ Cfg: setting.NewCfg(), SQLStore: sqlStore, - authInfoService: &authinfotest.FakeService{}, + authInfoService: &authinfotest.FakeService{ExpectedError: user.ErrUserNotFound}, userService: userSvc, + SocialService: &mockSocialService{}, } sc := setupScenarioContext(t, url) diff --git a/pkg/api/org_invite.go b/pkg/api/org_invite.go index a2f08b9607b..51054a22ada 100644 --- a/pkg/api/org_invite.go +++ b/pkg/api/org_invite.go @@ -270,10 +270,6 @@ func (hs *HTTPServer) CompleteInvite(c *contextmodel.ReqContext) response.Respon } } - if err := completeInvite.Password.Validate(hs.Cfg); err != nil { - return response.Err(err) - } - cmd := user.CreateUserCommand{ Email: completeInvite.Email, Name: completeInvite.Name, diff --git a/pkg/api/password.go b/pkg/api/password.go index f3b265029e4..fb6db45d939 100644 --- a/pkg/api/password.go +++ b/pkg/api/password.go @@ -11,7 +11,6 @@ import ( "github.com/grafana/grafana/pkg/services/login" "github.com/grafana/grafana/pkg/services/notifications" "github.com/grafana/grafana/pkg/services/user" - "github.com/grafana/grafana/pkg/util" "github.com/grafana/grafana/pkg/web" ) @@ -65,6 +64,11 @@ func (hs *HTTPServer) ResetPassword(c *contextmodel.ReqContext) response.Respons if err := web.Bind(c.Req, &form); err != nil { return response.Error(http.StatusBadRequest, "bad request data", err) } + + if form.NewPassword != form.ConfirmPassword { + return response.Error(http.StatusBadRequest, "Passwords do not match", nil) + } + query := notifications.ValidateResetPasswordCodeQuery{Code: form.Code} // For now the only way to know the username to clear login attempts for is @@ -85,33 +89,12 @@ func (hs *HTTPServer) ResetPassword(c *contextmodel.ReqContext) response.Respons return response.Error(http.StatusInternalServerError, "Unknown error validating email code", err) } - getAuthQuery := login.GetAuthInfoQuery{UserId: userResult.ID} - if authInfo, err := hs.authInfoService.GetAuthInfo(c.Req.Context(), &getAuthQuery); err == nil { - oauthInfo := hs.SocialService.GetOAuthInfoProvider(authInfo.AuthModule) - if login.IsProviderEnabled(hs.Cfg, authInfo.AuthModule, oauthInfo) { - return response.Error(http.StatusBadRequest, "Cannot update external user password", err) - } - } - - if form.NewPassword != form.ConfirmPassword { - return response.Error(http.StatusBadRequest, "Passwords do not match", nil) - } - - if err := form.NewPassword.Validate(hs.Cfg); err != nil { - c.Logger.Warn("the new password doesn't meet the password policy criteria", "err", err) - return response.Err(err) - } - - cmd := user.ChangeUserPasswordCommand{} - cmd.UserID = userResult.ID - encodedPassword, err := util.EncodePassword(string(form.NewPassword), userResult.Salt) - if err != nil { - return response.Error(http.StatusInternalServerError, "Failed to encode password", err) + if response := hs.errOnExternalUser(c.Req.Context(), userResult.ID); response != nil { + return response } - cmd.NewPassword = user.Password(encodedPassword) - if err := hs.userService.ChangePassword(c.Req.Context(), &cmd); err != nil { - return response.Error(http.StatusInternalServerError, "Failed to change user password", err) + if err := hs.userService.Update(c.Req.Context(), &user.UpdateUserCommand{UserID: userResult.ID, Password: &form.ConfirmPassword}); err != nil { + return response.ErrOrFallback(http.StatusInternalServerError, "Failed to change user password", err) } if err := hs.loginAttemptService.Reset(c.Req.Context(), username); err != nil { diff --git a/pkg/api/signup.go b/pkg/api/signup.go index 935817aaed7..3309a028e28 100644 --- a/pkg/api/signup.go +++ b/pkg/api/signup.go @@ -4,7 +4,6 @@ import ( "context" "errors" "net/http" - "strings" "github.com/grafana/grafana/pkg/api/dtos" "github.com/grafana/grafana/pkg/api/response" @@ -89,9 +88,6 @@ func (hs *HTTPServer) SignUpStep2(c *contextmodel.ReqContext) response.Response return response.Error(http.StatusUnauthorized, "User signup is disabled", nil) } - form.Email = strings.TrimSpace(form.Email) - form.Username = strings.TrimSpace(form.Username) - createUserCmd := user.CreateUserCommand{ Email: form.Email, Login: form.Username, diff --git a/pkg/api/user.go b/pkg/api/user.go index e529497546f..f774d8ae133 100644 --- a/pkg/api/user.go +++ b/pkg/api/user.go @@ -226,13 +226,8 @@ func (hs *HTTPServer) UpdateUserActiveOrg(c *contextmodel.ReqContext) response.R func (hs *HTTPServer) handleUpdateUser(ctx context.Context, cmd user.UpdateUserCommand) response.Response { // external user -> user data cannot be updated - isExternal, err := hs.isExternalUser(ctx, cmd.UserID) - if err != nil { - return response.Error(http.StatusInternalServerError, "Failed to validate User", err) - } - - if isExternal { - return response.Error(http.StatusForbidden, "User info cannot be updated for external Users", nil) + if response := hs.errOnExternalUser(ctx, cmd.UserID); response != nil { + return response } if len(cmd.Login) == 0 { @@ -344,20 +339,6 @@ func (hs *HTTPServer) UpdateUserEmail(c *contextmodel.ReqContext) response.Respo return response.Redirect(hs.Cfg.AppSubURL + "/profile") } -func (hs *HTTPServer) isExternalUser(ctx context.Context, userID int64) (bool, error) { - getAuthQuery := login.GetAuthInfoQuery{UserId: userID} - var err error - if _, err = hs.authInfoService.GetAuthInfo(ctx, &getAuthQuery); err == nil { - return true, nil - } - - if errors.Is(err, user.ErrUserNotFound) { - return false, nil - } - - return false, err -} - // swagger:route GET /user/orgs signed_in_user getSignedInUserOrgList // // Organizations of the actual User. @@ -571,8 +552,8 @@ func (hs *HTTPServer) ChangeActiveOrgAndRedirectToHome(c *contextmodel.ReqContex // 403: forbiddenError // 500: internalServerError func (hs *HTTPServer) ChangeUserPassword(c *contextmodel.ReqContext) response.Response { - cmd := user.ChangeUserPasswordCommand{} - if err := web.Bind(c.Req, &cmd); err != nil { + form := user.ChangeUserPasswordCommand{} + if err := web.Bind(c.Req, &form); err != nil { return response.Error(http.StatusBadRequest, "bad request data", err) } @@ -581,43 +562,12 @@ func (hs *HTTPServer) ChangeUserPassword(c *contextmodel.ReqContext) response.Re return errResponse } - userQuery := user.GetUserByIDQuery{ID: userID} - - usr, err := hs.userService.GetByID(c.Req.Context(), &userQuery) - if err != nil { - return response.Error(http.StatusInternalServerError, "Could not read user from database", err) - } - - getAuthQuery := login.GetAuthInfoQuery{UserId: usr.ID} - if authInfo, err := hs.authInfoService.GetAuthInfo(c.Req.Context(), &getAuthQuery); err == nil { - oauthInfo := hs.SocialService.GetOAuthInfoProvider(authInfo.AuthModule) - if login.IsProviderEnabled(hs.Cfg, authInfo.AuthModule, oauthInfo) { - return response.Error(http.StatusBadRequest, "Cannot update external user password", err) - } - } - - passwordHashed, err := util.EncodePassword(string(cmd.OldPassword), usr.Salt) - if err != nil { - return response.Error(http.StatusInternalServerError, "Failed to encode password", err) - } - if user.Password(passwordHashed) != usr.Password { - return response.Error(http.StatusUnauthorized, "Invalid old password", nil) - } - - if err := cmd.NewPassword.Validate(hs.Cfg); err != nil { - c.Logger.Warn("the new password doesn't meet the password policy criteria", "err", err) - return response.Err(err) - } - - cmd.UserID = userID - encodedPassword, err := util.EncodePassword(string(cmd.NewPassword), usr.Salt) - if err != nil { - return response.Error(http.StatusInternalServerError, "Failed to encode password", err) + if response := hs.errOnExternalUser(c.Req.Context(), userID); response != nil { + return response } - cmd.NewPassword = user.Password(encodedPassword) - if err := hs.userService.ChangePassword(c.Req.Context(), &cmd); err != nil { - return response.Error(http.StatusInternalServerError, "Failed to change user password", err) + if err := hs.userService.Update(c.Req.Context(), &user.UpdateUserCommand{UserID: userID, Password: &form.NewPassword, OldPassword: &form.OldPassword}); err != nil { + return response.ErrOrFallback(http.StatusInternalServerError, "Failed to change user password", err) } return response.Success("User password changed") diff --git a/pkg/api/user_test.go b/pkg/api/user_test.go index e66cbd5209e..55bd2dc61d3 100644 --- a/pkg/api/user_test.go +++ b/pkg/api/user_test.go @@ -345,12 +345,14 @@ func Test_GetUserByID(t *testing.T) { func TestHTTPServer_UpdateUser(t *testing.T) { settings := setting.NewCfg() + settings.SAMLAuthEnabled = true sqlStore := db.InitTestDB(t) hs := &HTTPServer{ Cfg: settings, SQLStore: sqlStore, AccessControl: acmock.New(), + SocialService: &socialtest.FakeSocialService{ExpectedAuthInfoProvider: &social.OAuthInfo{Enabled: true}}, } updateUserCommand := user.UpdateUserCommand{ @@ -366,7 +368,8 @@ func TestHTTPServer_UpdateUser(t *testing.T) { routePattern: "/api/users/:id", cmd: updateUserCommand, fn: func(sc *scenarioContext) { - sc.authInfoService.ExpectedUserAuth = &login.UserAuth{} + sc.authInfoService.ExpectedUserAuth = &login.UserAuth{AuthModule: login.SAMLAuthModule} + sc.fakeReqWithParams("PUT", sc.url, map[string]string{"id": "1"}).exec() assert.Equal(t, 403, sc.resp.Code) }, @@ -1086,11 +1089,13 @@ func updateUserScenario(t *testing.T, ctx updateUserContext, hs *HTTPServer) { func TestHTTPServer_UpdateSignedInUser(t *testing.T) { settings := setting.NewCfg() sqlStore := db.InitTestDB(t) + settings.SAMLAuthEnabled = true hs := &HTTPServer{ Cfg: settings, SQLStore: sqlStore, AccessControl: acmock.New(), + SocialService: &socialtest.FakeSocialService{}, } updateUserCommand := user.UpdateUserCommand{ @@ -1106,7 +1111,7 @@ func TestHTTPServer_UpdateSignedInUser(t *testing.T) { routePattern: "/api/users/", cmd: updateUserCommand, fn: func(sc *scenarioContext) { - sc.authInfoService.ExpectedUserAuth = &login.UserAuth{} + sc.authInfoService.ExpectedUserAuth = &login.UserAuth{AuthModule: login.SAMLAuthModule} sc.fakeReqWithParams("PUT", sc.url, map[string]string{"id": "1"}).exec() assert.Equal(t, 403, sc.resp.Code) }, diff --git a/pkg/api/utils.go b/pkg/api/utils.go index 08a1c525393..dae4c2da137 100644 --- a/pkg/api/utils.go +++ b/pkg/api/utils.go @@ -1,10 +1,16 @@ package api import ( + "context" + "errors" + "net/http" "net/mail" + "github.com/grafana/grafana/pkg/api/response" "github.com/grafana/grafana/pkg/middleware/cookies" contextmodel "github.com/grafana/grafana/pkg/services/contexthandler/model" + "github.com/grafana/grafana/pkg/services/login" + "github.com/grafana/grafana/pkg/services/user" ) func (hs *HTTPServer) GetRedirectURL(c *contextmodel.ReqContext) string { @@ -20,6 +26,31 @@ func (hs *HTTPServer) GetRedirectURL(c *contextmodel.ReqContext) string { return redirectURL } +func (hs *HTTPServer) errOnExternalUser(ctx context.Context, userID int64) response.Response { + isExternal, err := hs.isExternalUser(ctx, userID) + if err != nil { + return response.Error(http.StatusInternalServerError, "Failed to validate User", err) + } + if isExternal { + return response.Error(http.StatusForbidden, "Cannot update external User", nil) + } + return nil +} + +func (hs *HTTPServer) isExternalUser(ctx context.Context, userID int64) (bool, error) { + info, err := hs.authInfoService.GetAuthInfo(ctx, &login.GetAuthInfoQuery{UserId: userID}) + + if errors.Is(err, user.ErrUserNotFound) { + return false, nil + } + + if err != nil { + return true, err + } + + return login.IsProviderEnabled(hs.Cfg, info.AuthModule, hs.SocialService.GetOAuthInfoProvider(info.AuthModule)), nil +} + func ValidateAndNormalizeEmail(email string) (string, error) { if email == "" { return "", nil diff --git a/pkg/cmd/grafana-cli/commands/reset_password_command.go b/pkg/cmd/grafana-cli/commands/reset_password_command.go index 08c8b7edb57..78f0ef6b0df 100644 --- a/pkg/cmd/grafana-cli/commands/reset_password_command.go +++ b/pkg/cmd/grafana-cli/commands/reset_password_command.go @@ -12,7 +12,6 @@ import ( "github.com/grafana/grafana/pkg/cmd/grafana-cli/utils" "github.com/grafana/grafana/pkg/server" "github.com/grafana/grafana/pkg/services/user" - "github.com/grafana/grafana/pkg/util" ) const DefaultAdminUserId = 1 @@ -58,17 +57,12 @@ func resetPassword(adminId int64, newPassword user.Password, userSvc user.Servic return ErrMustBeAdmin } - passwordHashed, err := util.EncodePassword(string(newPassword), usr.Salt) + password, err := newPassword.Hash(usr.Salt) if err != nil { return err } - cmd := user.ChangeUserPasswordCommand{ - UserID: adminId, - NewPassword: user.Password(passwordHashed), - } - - if err := userSvc.ChangePassword(context.Background(), &cmd); err != nil { + if err := userSvc.Update(context.Background(), &user.UpdateUserCommand{UserID: adminId, Password: &password}); err != nil { return fmt.Errorf("failed to update user password: %w", err) } diff --git a/pkg/services/org/orgimpl/store_test.go b/pkg/services/org/orgimpl/store_test.go index a25cb650b6e..1c62a471641 100644 --- a/pkg/services/org/orgimpl/store_test.go +++ b/pkg/services/org/orgimpl/store_test.go @@ -773,7 +773,7 @@ func TestIntegration_SQLStore_GetOrgUsers_PopulatesCorrectly(t *testing.T) { assert.Equal(t, int64(1), actual.UserID) assert.Equal(t, "viewer@localhost", actual.Email) assert.Equal(t, "Viewer Localhost", actual.Name) - assert.Equal(t, "Viewer", actual.Login) + assert.Equal(t, "viewer", actual.Login) assert.Equal(t, "Viewer", actual.Role) assert.Equal(t, constNow.AddDate(-10, 0, 0), actual.LastSeenAt) assert.Equal(t, constNow, actual.Created) diff --git a/pkg/services/user/error.go b/pkg/services/user/error.go index 8fd192704fb..9460ed6f7f9 100644 --- a/pkg/services/user/error.go +++ b/pkg/services/user/error.go @@ -22,4 +22,5 @@ var ( ErrEmptyUsernameAndEmail = errutil.BadRequest( "user.empty-username-and-email", errutil.WithPublicMessage("Need to specify either username or email"), ) + ErrPasswordMissmatch = errutil.BadRequest("user.password-missmatch", errutil.WithPublicMessage("Invalid old password")) ) diff --git a/pkg/services/user/model.go b/pkg/services/user/model.go index 1a4382d2f02..13aaed4ae78 100644 --- a/pkg/services/user/model.go +++ b/pkg/services/user/model.go @@ -86,13 +86,9 @@ type UpdateUserCommand struct { IsDisabled *bool `json:"-"` EmailVerified *bool `json:"-"` IsGrafanaAdmin *bool `json:"-"` -} - -type ChangeUserPasswordCommand struct { - OldPassword Password `json:"oldPassword"` - NewPassword Password `json:"newPassword"` - UserID int64 `json:"-"` + Password *Password `json:"-"` + OldPassword *Password `json:"-"` } type UpdateUserLastSeenAtCommand struct { @@ -296,3 +292,8 @@ type AdminCreateUserResponse struct { ID int64 `json:"id"` Message string `json:"message"` } + +type ChangeUserPasswordCommand struct { + OldPassword Password `json:"oldPassword"` + NewPassword Password `json:"newPassword"` +} diff --git a/pkg/services/user/password.go b/pkg/services/user/password.go index 70e0e131e58..78c4f8585ab 100644 --- a/pkg/services/user/password.go +++ b/pkg/services/user/password.go @@ -4,6 +4,7 @@ import ( "unicode" "github.com/grafana/grafana/pkg/setting" + "github.com/grafana/grafana/pkg/util" "github.com/grafana/grafana/pkg/util/errutil" ) @@ -26,13 +27,21 @@ func (p Password) Validate(config *setting.Cfg) error { return ValidatePassword(string(p), config) } +func (p Password) Hash(salt string) (Password, error) { + hashed, err := util.EncodePassword(string(p), salt) + if err != nil { + return "", err + } + return Password(hashed), nil +} + // ValidatePassword checks if a new password meets the required criteria based on the given configuration. // If BasicAuthStrongPasswordPolicy is disabled, it only checks for password length. // Otherwise, it ensures the password meets the minimum length requirement and contains at least one uppercase letter, // one lowercase letter, one number, and one symbol. func ValidatePassword(newPassword string, config *setting.Cfg) error { if !config.BasicAuthStrongPasswordPolicy { - if len(newPassword) <= 4 { + if len(newPassword) < 4 { return ErrPasswordTooShort.Errorf("new password is too short") } return nil diff --git a/pkg/services/user/password_test.go b/pkg/services/user/password_test.go index 1953d1a1529..3240c122524 100644 --- a/pkg/services/user/password_test.go +++ b/pkg/services/user/password_test.go @@ -25,7 +25,7 @@ func TestPasswowrdService_ValidatePasswordHardcodePolicy(t *testing.T) { strongPasswordPolicyEnabled: false, }, {name: "should not return error when the password has 4 characters and strong password policy is disabled", - passwordTest: LOWERCASE, + passwordTest: "test", expectedError: nil, strongPasswordPolicyEnabled: false, }, diff --git a/pkg/services/user/user.go b/pkg/services/user/user.go index a02cd15f38e..06d8b113978 100644 --- a/pkg/services/user/user.go +++ b/pkg/services/user/user.go @@ -16,7 +16,6 @@ type Service interface { GetByLogin(context.Context, *GetUserByLoginQuery) (*User, error) GetByEmail(context.Context, *GetUserByEmailQuery) (*User, error) Update(context.Context, *UpdateUserCommand) error - ChangePassword(context.Context, *ChangeUserPasswordCommand) error UpdateLastSeenAt(context.Context, *UpdateUserLastSeenAtCommand) error SetUsingOrg(context.Context, *SetUsingOrgCommand) error GetSignedInUserWithCacheCtx(context.Context, *GetSignedInUserQuery) (*SignedInUser, error) diff --git a/pkg/services/user/userimpl/store.go b/pkg/services/user/userimpl/store.go index e9eb275a9e3..c0bc043e499 100644 --- a/pkg/services/user/userimpl/store.go +++ b/pkg/services/user/userimpl/store.go @@ -28,7 +28,6 @@ type store interface { GetByLogin(context.Context, *user.GetUserByLoginQuery) (*user.User, error) GetByEmail(context.Context, *user.GetUserByEmailQuery) (*user.User, error) Update(context.Context, *user.UpdateUserCommand) error - ChangePassword(context.Context, *user.ChangeUserPasswordCommand) error UpdateLastSeenAt(context.Context, *user.UpdateUserLastSeenAtCommand) error GetSignedInUser(context.Context, *user.GetSignedInUserQuery) (*user.SignedInUser, error) UpdateUser(context.Context, *user.User) error @@ -300,20 +299,21 @@ func (ss *sqlStore) loginConflict(ctx context.Context, sess *db.Session, login, } func (ss *sqlStore) Update(ctx context.Context, cmd *user.UpdateUserCommand) error { - cmd.Login = strings.ToLower(cmd.Login) - cmd.Email = strings.ToLower(cmd.Email) - return ss.db.WithTransactionalDbSession(ctx, func(sess *db.Session) error { user := user.User{ Name: cmd.Name, - Email: cmd.Email, - Login: cmd.Login, + Email: strings.ToLower(cmd.Email), + Login: strings.ToLower(cmd.Login), Theme: cmd.Theme, Updated: time.Now(), } q := sess.ID(cmd.UserID).Where(ss.notServiceAccountFilter()) + if cmd.Password != nil { + user.Password = *cmd.Password + } + if cmd.IsDisabled != nil { sess.UseBool("is_disabled") user.IsDisabled = *cmd.IsDisabled @@ -352,18 +352,6 @@ func (ss *sqlStore) Update(ctx context.Context, cmd *user.UpdateUserCommand) err }) } -func (ss *sqlStore) ChangePassword(ctx context.Context, cmd *user.ChangeUserPasswordCommand) error { - return ss.db.WithTransactionalDbSession(ctx, func(sess *db.Session) error { - user := user.User{ - Password: cmd.NewPassword, - Updated: time.Now(), - } - - _, err := sess.ID(cmd.UserID).Where(ss.notServiceAccountFilter()).Update(&user) - return err - }) -} - func (ss *sqlStore) UpdateLastSeenAt(ctx context.Context, cmd *user.UpdateUserLastSeenAtCommand) error { if cmd.UserID <= 0 { return user.ErrUpdateInvalidID diff --git a/pkg/services/user/userimpl/store_test.go b/pkg/services/user/userimpl/store_test.go index 68f62f7c346..cf559938a87 100644 --- a/pkg/services/user/userimpl/store_test.go +++ b/pkg/services/user/userimpl/store_test.go @@ -419,8 +419,31 @@ func TestIntegrationUserDataAccess(t *testing.T) { }) t.Run("Change user password", func(t *testing.T) { - err := userStore.ChangePassword(context.Background(), &user.ChangeUserPasswordCommand{}) + id, err := userStore.Insert(context.Background(), &user.User{ + Email: "password@test.com", + Name: "password", + Login: "password", + Password: "password", + Salt: "salt", + Created: time.Now(), + Updated: time.Now(), + }) + require.NoError(t, err) + + err = userStore.Update(context.Background(), &user.UpdateUserCommand{ + UserID: id, + Password: passwordPtr("updated"), + }) require.NoError(t, err) + + updated, err := userStore.GetByID(context.Background(), id) + require.NoError(t, err) + + assert.Equal(t, updated.Salt, "salt") + assert.Equal(t, updated.Name, "password") + assert.Equal(t, updated.Login, "password") + assert.Equal(t, updated.Email, "password@test.com") + assert.Equal(t, updated.Password, user.Password("updated")) }) t.Run("update last seen at", func(t *testing.T) { @@ -956,8 +979,8 @@ func TestIntegrationUserUpdate(t *testing.T) { require.Equal(t, "Change Name", result.Name) // Unchanged - require.Equal(t, "loginUSER3", result.Login) - require.Equal(t, "USER3@test.com", result.Email) + require.Equal(t, "loginuser3", result.Login) + require.Equal(t, "user3@test.com", result.Email) }) } @@ -1034,6 +1057,11 @@ func createOrgAndUserSvc(t *testing.T, store db.DB, cfg *setting.Cfg) (org.Servi return orgService, usrSvc } +func passwordPtr(s string) *user.Password { + password := user.Password(s) + return &password +} + func boolPtr(b bool) *bool { return &b } diff --git a/pkg/services/user/userimpl/user.go b/pkg/services/user/userimpl/user.go index dd935d96f6a..9a085c39ccb 100644 --- a/pkg/services/user/userimpl/user.go +++ b/pkg/services/user/userimpl/user.go @@ -134,9 +134,9 @@ func (s *Service) Create(ctx context.Context, cmd *user.CreateUserCommand) (*use // create user usr := &user.User{ UID: cmd.UID, - Email: cmd.Email, + Email: strings.ToLower(cmd.Email), Name: cmd.Name, - Login: cmd.Login, + Login: strings.ToLower(cmd.Login), Company: cmd.Company, IsAdmin: cmd.IsAdmin, IsDisabled: cmd.IsDisabled, @@ -160,11 +160,14 @@ func (s *Service) Create(ctx context.Context, cmd *user.CreateUserCommand) (*use usr.Rands = rands if len(cmd.Password) > 0 { - encodedPassword, err := util.EncodePassword(string(cmd.Password), usr.Salt) + if err := cmd.Password.Validate(s.cfg); err != nil { + return nil, err + } + + usr.Password, err = cmd.Password.Hash(usr.Salt) if err != nil { return nil, err } - usr.Password = user.Password(encodedPassword) } _, err = s.store.Insert(ctx, usr) @@ -220,14 +223,35 @@ func (s *Service) GetByEmail(ctx context.Context, query *user.GetUserByEmailQuer } func (s *Service) Update(ctx context.Context, cmd *user.UpdateUserCommand) error { - cmd.Login = strings.ToLower(cmd.Login) - cmd.Email = strings.ToLower(cmd.Email) + usr, err := s.store.GetByID(ctx, cmd.UserID) + if err != nil { + return err + } - return s.store.Update(ctx, cmd) -} + if cmd.OldPassword != nil { + old, err := cmd.OldPassword.Hash(usr.Salt) + if err != nil { + return err + } -func (s *Service) ChangePassword(ctx context.Context, cmd *user.ChangeUserPasswordCommand) error { - return s.store.ChangePassword(ctx, cmd) + if old != usr.Password { + return user.ErrPasswordMissmatch.Errorf("old password does not match stored password") + } + } + + if cmd.Password != nil { + if err := cmd.Password.Validate(s.cfg); err != nil { + return err + } + + hashed, err := cmd.Password.Hash(usr.Salt) + if err != nil { + return err + } + cmd.Password = &hashed + } + + return s.store.Update(ctx, cmd) } func (s *Service) UpdateLastSeenAt(ctx context.Context, cmd *user.UpdateUserLastSeenAtCommand) error { diff --git a/pkg/services/user/userimpl/user_test.go b/pkg/services/user/userimpl/user_test.go index bc6eccb5e1b..6e362ae92ab 100644 --- a/pkg/services/user/userimpl/user_test.go +++ b/pkg/services/user/userimpl/user_test.go @@ -154,6 +154,32 @@ func TestUserService(t *testing.T) { }) } +func TestService_Update(t *testing.T) { + t.Run("should return error if old password does not match stored password", func(t *testing.T) { + stored, err := user.Password("test").Hash("salt") + require.NoError(t, err) + service := &Service{store: &FakeUserStore{ExpectedUser: &user.User{Password: stored, Salt: "salt"}}} + + err = service.Update(context.Background(), &user.UpdateUserCommand{ + OldPassword: passwordPtr("test123"), + }) + + assert.ErrorIs(t, err, user.ErrPasswordMissmatch) + }) + + t.Run("should return error new password is not valid", func(t *testing.T) { + stored, err := user.Password("test").Hash("salt") + require.NoError(t, err) + service := &Service{cfg: setting.NewCfg(), store: &FakeUserStore{ExpectedUser: &user.User{Password: stored, Salt: "salt"}}} + + err = service.Update(context.Background(), &user.UpdateUserCommand{ + OldPassword: passwordPtr("test"), + Password: passwordPtr("asd"), + }) + require.ErrorIs(t, err, user.ErrPasswordTooShort) + }) +} + func TestMetrics(t *testing.T) { userStore := newUserStoreFake() orgService := orgtest.NewOrgServiceFake() @@ -234,10 +260,6 @@ func (f *FakeUserStore) Update(ctx context.Context, cmd *user.UpdateUserCommand) return f.ExpectedError } -func (f *FakeUserStore) ChangePassword(ctx context.Context, cmd *user.ChangeUserPasswordCommand) error { - return f.ExpectedError -} - func (f *FakeUserStore) UpdateLastSeenAt(ctx context.Context, cmd *user.UpdateUserLastSeenAtCommand) error { return f.ExpectedError } diff --git a/pkg/services/user/usertest/fake.go b/pkg/services/user/usertest/fake.go index bc7962e7c7b..b33a04ce552 100644 --- a/pkg/services/user/usertest/fake.go +++ b/pkg/services/user/usertest/fake.go @@ -67,10 +67,6 @@ func (f *FakeUserService) Update(ctx context.Context, cmd *user.UpdateUserComman return f.ExpectedError } -func (f *FakeUserService) ChangePassword(ctx context.Context, cmd *user.ChangeUserPasswordCommand) error { - return f.ExpectedError -} - func (f *FakeUserService) UpdateLastSeenAt(ctx context.Context, cmd *user.UpdateUserLastSeenAtCommand) error { return f.ExpectedError } diff --git a/pkg/services/user/usertest/mock.go b/pkg/services/user/usertest/mock.go index 75ff3812e88..7668bf3548f 100644 --- a/pkg/services/user/usertest/mock.go +++ b/pkg/services/user/usertest/mock.go @@ -32,24 +32,6 @@ func (_m *MockService) BatchDisableUsers(_a0 context.Context, _a1 *user.BatchDis return r0 } -// ChangePassword provides a mock function with given fields: _a0, _a1 -func (_m *MockService) ChangePassword(_a0 context.Context, _a1 *user.ChangeUserPasswordCommand) error { - ret := _m.Called(_a0, _a1) - - if len(ret) == 0 { - panic("no return value specified for ChangePassword") - } - - var r0 error - if rf, ok := ret.Get(0).(func(context.Context, *user.ChangeUserPasswordCommand) error); ok { - r0 = rf(_a0, _a1) - } else { - r0 = ret.Error(0) - } - - return r0 -} - // Create provides a mock function with given fields: _a0, _a1 func (_m *MockService) Create(_a0 context.Context, _a1 *user.CreateUserCommand) (*user.User, error) { ret := _m.Called(_a0, _a1)