diff --git a/server/channels/app/user.go b/server/channels/app/user.go index 7e6ee51e3f..121ad8cf4c 100644 --- a/server/channels/app/user.go +++ b/server/channels/app/user.go @@ -1445,7 +1445,11 @@ func (a *App) UpdatePassword(rctx request.CTX, user *model.User, newPassword str return model.NewAppError("UpdatePassword", "api.user.update_password.failed.app_error", nil, "", http.StatusInternalServerError) } - hashedPassword := model.HashPassword(newPassword) + hashedPassword, err := model.HashPassword(newPassword) + if err != nil { + // can't be password length (checked in IsPasswordValid) + return model.NewAppError("UpdatePassword", "api.user.update_password.password_hash.app_error", nil, "user_id="+user.Id, http.StatusInternalServerError).Wrap(err) + } if err := a.Srv().Store().User().UpdatePassword(user.Id, hashedPassword); err != nil { return model.NewAppError("UpdatePassword", "api.user.update_password.failed.app_error", nil, "", http.StatusInternalServerError).Wrap(err) diff --git a/server/channels/app/users/password.go b/server/channels/app/users/password.go index bc6f6a59b4..b96ffbf373 100644 --- a/server/channels/app/users/password.go +++ b/server/channels/app/users/password.go @@ -20,16 +20,6 @@ func CheckUserPassword(user *model.User, password string) error { return nil } -// HashPassword generates a hash using the bcrypt.GenerateFromPassword -func HashPassword(password string) string { - hash, err := bcrypt.GenerateFromPassword([]byte(password), 10) - if err != nil { - panic(err) - } - - return string(hash) -} - func ComparePassword(hash string, password string) error { if password == "" || hash == "" { return errors.New("empty password or hash") diff --git a/server/channels/app/users/password_test.go b/server/channels/app/users/password_test.go index f98d8f12b1..c889e8760e 100644 --- a/server/channels/app/users/password_test.go +++ b/server/channels/app/users/password_test.go @@ -13,13 +13,6 @@ import ( "github.com/mattermost/mattermost/server/public/model" ) -func TestComparePassword(t *testing.T) { - hash := HashPassword("Test") - - assert.NoError(t, ComparePassword(hash, "Test"), "Passwords don't match") - assert.Error(t, ComparePassword(hash, "Test2"), "Passwords should not have matched") -} - func TestIsPasswordValidWithSettings(t *testing.T) { for name, tc := range map[string]struct { Password string diff --git a/server/channels/store/sqlstore/user_store.go b/server/channels/store/sqlstore/user_store.go index db36c220e4..eea7b64997 100644 --- a/server/channels/store/sqlstore/user_store.go +++ b/server/channels/store/sqlstore/user_store.go @@ -112,7 +112,9 @@ func (us SqlUserStore) Save(rctx request.CTX, user *model.User) (*model.User, er return nil, store.NewErrInvalidInput("User", "id", user.Id) } - user.PreSave() + if err := user.PreSave(); err != nil { + return nil, err + } if err := user.IsValid(); err != nil { return nil, err } diff --git a/server/channels/store/storetest/user_store.go b/server/channels/store/storetest/user_store.go index 342b61a64d..6424135d59 100644 --- a/server/channels/store/storetest/user_store.go +++ b/server/channels/store/storetest/user_store.go @@ -12,6 +12,8 @@ import ( "testing" "time" + "golang.org/x/crypto/bcrypt" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -2294,7 +2296,11 @@ func testUserStoreUpdatePassword(t *testing.T, rctx request.CTX, ss store.Store) _, nErr := ss.Team().SaveMember(rctx, &model.TeamMember{TeamId: teamId, UserId: u1.Id}, -1) require.NoError(t, nErr) - hashedPassword := model.HashPassword("newpwd") + _, err = model.HashPassword(strings.Repeat("1234567890", 8)) + require.ErrorIs(t, err, bcrypt.ErrPasswordTooLong) + + hashedPassword, err := model.HashPassword("newpwd") + require.NoError(t, err) err = ss.User().UpdatePassword(u1.Id, hashedPassword) require.NoError(t, err) diff --git a/server/i18n/en.json b/server/i18n/en.json index 71f414e7b7..66bf7bb087 100644 --- a/server/i18n/en.json +++ b/server/i18n/en.json @@ -4146,6 +4146,10 @@ "id": "api.user.update_password.oauth.app_error", "translation": "Update password failed because the user is logged in through an OAuth service." }, + { + "id": "api.user.update_password.password_hash.app_error", + "translation": "There was an internal error saving the password." + }, { "id": "api.user.update_password.user_and_hashed.app_error", "translation": "Only system administrators can set already-hashed passwords." @@ -9758,6 +9762,14 @@ "id": "model.user.is_valid.username.app_error", "translation": "Username must begin with a letter, and contain between 3 to 22 lowercase characters made up of numbers, letters, and the symbols \".\", \"-\", and \"_\"." }, + { + "id": "model.user.pre_save.password_hash.app_error", + "translation": "There was an internal error saving the password." + }, + { + "id": "model.user.pre_save.password_too_long.app_error", + "translation": "Your password must contain no more than 72 characters." + }, { "id": "model.user_access_token.is_valid.description.app_error", "translation": "Invalid description, must be 255 or less characters." diff --git a/server/public/model/user.go b/server/public/model/user.go index e29e43d46f..378bd807a6 100644 --- a/server/public/model/user.go +++ b/server/public/model/user.go @@ -14,6 +14,8 @@ import ( "time" "unicode/utf8" + "github.com/pkg/errors" + "golang.org/x/crypto/bcrypt" "golang.org/x/text/language" @@ -378,10 +380,6 @@ func (u *User) IsValid() *AppError { return InvalidUserError("auth_data_pwd", u.Id, *u.AuthData) } - if len(u.Password) > UserPasswordMaxLength { - return InvalidUserError("password_limit", u.Id, "") - } - if !IsValidLocale(u.Locale) { return InvalidUserError("locale", u.Id, u.Locale) } @@ -430,7 +428,7 @@ func NormalizeEmail(email string) string { // PreSave will set the Id and Username if missing. It will also fill // in the CreateAt, UpdateAt times. It will also hash the password. It should // be run before saving the user to the db. -func (u *User) PreSave() { +func (u *User) PreSave() *AppError { if u.Id == "" { u.Id = NewId() } @@ -477,7 +475,15 @@ func (u *User) PreSave() { } if u.Password != "" { - u.Password = HashPassword(u.Password) + hashed, err := HashPassword(u.Password) + if errors.Is(err, bcrypt.ErrPasswordTooLong) { + return NewAppError("User.PreSave", "model.user.pre_save.password_too_long.app_error", + nil, "user_id="+u.Id, http.StatusBadRequest).Wrap(err) + } else if err != nil { + return NewAppError("User.PreSave", "model.user.pre_save.password_hash.app_error", + nil, "user_id="+u.Id, http.StatusBadRequest).Wrap(err) + } + u.Password = hashed } cs := u.GetCustomStatus() @@ -485,6 +491,8 @@ func (u *User) PreSave() { cs.PreSave() u.SetCustomStatus(cs) } + + return nil } // PreUpdate should be run before updating the user in the db. @@ -928,13 +936,13 @@ func (u *UserPatch) SetField(fieldName string, fieldValue string) { } // HashPassword generates a hash using the bcrypt.GenerateFromPassword -func HashPassword(password string) string { +func HashPassword(password string) (string, error) { hash, err := bcrypt.GenerateFromPassword([]byte(password), 10) if err != nil { - panic(err) + return "", err } - return string(hash) + return string(hash), nil } var validUsernameChars = regexp.MustCompile(`^[a-z0-9\.\-_]+$`) diff --git a/server/public/model/user_test.go b/server/public/model/user_test.go index 0ffd20d024..9292468483 100644 --- a/server/public/model/user_test.go +++ b/server/public/model/user_test.go @@ -9,6 +9,8 @@ import ( "strings" "testing" + "golang.org/x/crypto/bcrypt" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -47,12 +49,19 @@ func TestUserDeepCopy(t *testing.T) { func TestUserPreSave(t *testing.T) { user := User{Password: "test"} - user.PreSave() + err := user.PreSave() + require.Nil(t, err) user.Etag(true, true) assert.NotNil(t, user.Timezone, "Timezone is nil") assert.Equal(t, user.Timezone["useAutomaticTimezone"], "true", "Timezone is not set to default") } +func TestUserPreSavePwdTooLong(t *testing.T) { + user := User{Password: strings.Repeat("1234567890", 8)} + err := user.PreSave() + assert.ErrorIs(t, err, bcrypt.ErrPasswordTooLong) +} + func TestUserPreUpdate(t *testing.T) { user := User{Password: "test"} user.PreUpdate()