public/grafana
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

User: use update function for password updates (#86419)

Browse Source 
* Update password through Update function instead

* Remove duplicated to lower

* Refactor password code
This commit is contained in:
Karl Persson
2024-04-17 15:24:36 +02:00
committed by GitHub
parent f99d5a1c1a
commit 1a6777cb93
21 changed files with 182 additions and 205 deletions
Download Patch File Download Diff File Expand all files Collapse all files
pkg/services/org/orgimpl/store_test.go
+1 -1
View File
@@ -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)
pkg/services/user/error.go
+1
View File
@@ -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"))
)
pkg/services/user/model.go
+7 -6
View File
@@ -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"`
}
pkg/services/user/password.go
+10 -1
View File
@@ -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
pkg/services/user/password_test.go
+1 -1
View File
@@ -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,
},
pkg/services/user/user.go
-1
View File
@@ -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)
pkg/services/user/userimpl/store.go
+6 -18
View File
@@ -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
pkg/services/user/userimpl/store_test.go
+31 -3
View File
@@ -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
}
pkg/services/user/userimpl/user.go
+34 -10
View File
@@ -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,16 +223,37 @@ 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
}
if cmd.OldPassword != nil {
old, err := cmd.OldPassword.Hash(usr.Salt)
if err != nil {
return err
}
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) ChangePassword(ctx context.Context, cmd *user.ChangeUserPasswordCommand) error {
return s.store.ChangePassword(ctx, cmd)
}
func (s *Service) UpdateLastSeenAt(ctx context.Context, cmd *user.UpdateUserLastSeenAtCommand) error {
u, err := s.GetSignedInUserWithCacheCtx(ctx, &user.GetSignedInUserQuery{
UserID: cmd.UserID,
pkg/services/user/userimpl/user_test.go
+26 -4
View File
@@ -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
}
pkg/services/user/usertest/fake.go
-4
View File
@@ -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
}
pkg/services/user/usertest/mock.go
-18
View File
@@ -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)