From d26989147655300b1ef7ba4039f21bd590e8f142 Mon Sep 17 00:00:00 2001 From: Daniel Schalla Date: Tue, 21 May 2019 20:03:36 +0200 Subject: [PATCH] [MM-15490] Rework default password requirements (#10844) * Rework default password requirements * Update API Test Lib Default User PW * Remove unused function; Disable password reqs in dev mode * Disable strict password requirements for unit tests * Update unit tests --- api4/apitestlib.go | 13 ++++- app/authentication.go | 5 ++ app/helper_test.go | 9 ++++ cmd/mattermost/commands/cmdtestlib.go | 8 +++ config/default.json | 10 ++-- model/config.go | 10 ++-- utils/password.go | 8 --- utils/password_test.go | 74 +++++++++++++-------------- web/web_test.go | 9 ++++ 9 files changed, 89 insertions(+), 57 deletions(-) diff --git a/api4/apitestlib.go b/api4/apitestlib.go index 0d2c13303e..6438a850fd 100644 --- a/api4/apitestlib.go +++ b/api4/apitestlib.go @@ -108,6 +108,15 @@ func setupTestHelper(enterprise bool, updateConfig func(*model.Config)) *TestHel th.App.UpdateConfig(func(cfg *model.Config) { *cfg.TeamSettings.EnableOpenServer = true }) + // Disable strict password requirements for test + th.App.UpdateConfig(func(cfg *model.Config) { + *cfg.PasswordSettings.MinimumLength = 5 + *cfg.PasswordSettings.Lowercase = false + *cfg.PasswordSettings.Uppercase = false + *cfg.PasswordSettings.Symbol = false + *cfg.PasswordSettings.Number = false + }) + if enterprise { th.App.SetLicense(model.NewTestLicense()) } else { @@ -274,7 +283,7 @@ func (me *TestHelper) CreateUserWithClient(client *model.Client4) *model.User { Nickname: "nn_" + id, FirstName: "f_" + id, LastName: "l_" + id, - Password: "Password1", + Password: "Pa$$word11", } utils.DisableDebugLogForTest() @@ -283,7 +292,7 @@ func (me *TestHelper) CreateUserWithClient(client *model.Client4) *model.User { panic(response.Error) } - ruser.Password = "Password1" + ruser.Password = "Pa$$word11" store.Must(me.App.Srv.Store.User().VerifyEmail(ruser.Id, ruser.Email)) utils.EnableDebugLogForTest() return ruser diff --git a/app/authentication.go b/app/authentication.go index 673229a723..fc0adab520 100644 --- a/app/authentication.go +++ b/app/authentication.go @@ -37,6 +37,11 @@ func (tl TokenLocation) String() string { } func (a *App) IsPasswordValid(password string) *model.AppError { + + if *a.Config().ServiceSettings.EnableDeveloper { + return nil + } + return utils.IsPasswordValidWithSettings(password, &a.Config().PasswordSettings) } diff --git a/app/helper_test.go b/app/helper_test.go index f006c11f41..4a24388ea5 100644 --- a/app/helper_test.go +++ b/app/helper_test.go @@ -71,6 +71,15 @@ func setupTestHelper(enterprise bool, tb testing.TB) *TestHelper { th.App.UpdateConfig(func(cfg *model.Config) { *cfg.TeamSettings.EnableOpenServer = true }) + // Disable strict password requirements for test + th.App.UpdateConfig(func(cfg *model.Config) { + *cfg.PasswordSettings.MinimumLength = 5 + *cfg.PasswordSettings.Lowercase = false + *cfg.PasswordSettings.Uppercase = false + *cfg.PasswordSettings.Symbol = false + *cfg.PasswordSettings.Number = false + }) + if enterprise { th.App.SetLicense(model.NewTestLicense()) } else { diff --git a/cmd/mattermost/commands/cmdtestlib.go b/cmd/mattermost/commands/cmdtestlib.go index 7abc8d57c4..1727520994 100644 --- a/cmd/mattermost/commands/cmdtestlib.go +++ b/cmd/mattermost/commands/cmdtestlib.go @@ -81,6 +81,14 @@ func (h *testHelper) ConfigPath() string { // SetConfig replaces the configuration passed to a running command. func (h *testHelper) SetConfig(config *model.Config) { config.SqlSettings = *mainHelper.GetSqlSettings() + + // Disable strict password requirements for test + *config.PasswordSettings.MinimumLength = 5 + *config.PasswordSettings.Lowercase = false + *config.PasswordSettings.Uppercase = false + *config.PasswordSettings.Symbol = false + *config.PasswordSettings.Number = false + h.config = config if err := ioutil.WriteFile(h.configFilePath, []byte(config.ToJson()), 0600); err != nil { diff --git a/config/default.json b/config/default.json index def18c8ea3..f1edfa2ca4 100644 --- a/config/default.json +++ b/config/default.json @@ -159,11 +159,11 @@ "FileLocation": "" }, "PasswordSettings": { - "MinimumLength": 5, - "Lowercase": false, - "Number": false, - "Uppercase": false, - "Symbol": false + "MinimumLength": 10, + "Lowercase": true, + "Number": true, + "Uppercase": true, + "Symbol": true }, "FileSettings": { "EnableFileAttachments": true, diff --git a/model/config.go b/model/config.go index 6dec9ddcf2..6112d775b3 100644 --- a/model/config.go +++ b/model/config.go @@ -962,23 +962,23 @@ type PasswordSettings struct { func (s *PasswordSettings) SetDefaults() { if s.MinimumLength == nil { - s.MinimumLength = NewInt(PASSWORD_MINIMUM_LENGTH) + s.MinimumLength = NewInt(10) } if s.Lowercase == nil { - s.Lowercase = NewBool(false) + s.Lowercase = NewBool(true) } if s.Number == nil { - s.Number = NewBool(false) + s.Number = NewBool(true) } if s.Uppercase == nil { - s.Uppercase = NewBool(false) + s.Uppercase = NewBool(true) } if s.Symbol == nil { - s.Symbol = NewBool(false) + s.Symbol = NewBool(true) } } diff --git a/utils/password.go b/utils/password.go index b24d6c2ecf..087b99a3dd 100644 --- a/utils/password.go +++ b/utils/password.go @@ -10,14 +10,6 @@ import ( "github.com/mattermost/mattermost-server/model" ) -func IsPasswordValid(password string) *model.AppError { - if len(password) > model.PASSWORD_MAXIMUM_LENGTH || len(password) < model.PASSWORD_MINIMUM_LENGTH { - return model.NewAppError("User.IsValid", "model.user.is_valid.pwd.app_error", map[string]interface{}{"Min": model.PASSWORD_MINIMUM_LENGTH}, "", http.StatusBadRequest) - } - - return nil -} - func IsPasswordValidWithSettings(password string, settings *model.PasswordSettings) *model.AppError { id := "model.user.is_valid.pwd" isError := false diff --git a/utils/password_test.go b/utils/password_test.go index 4daf25018f..e0d120eed4 100644 --- a/utils/password_test.go +++ b/utils/password_test.go @@ -9,36 +9,6 @@ import ( "github.com/mattermost/mattermost-server/model" ) -func TestIsPasswordValid(t *testing.T) { - for name, tc := range map[string]struct { - Password string - ExpectedError string - }{ - "Short": { - Password: strings.Repeat("x", model.PASSWORD_MINIMUM_LENGTH), - }, - "Long": { - Password: strings.Repeat("x", model.PASSWORD_MAXIMUM_LENGTH), - }, - "TooShort": { - Password: strings.Repeat("x", model.PASSWORD_MINIMUM_LENGTH-1), - ExpectedError: "model.user.is_valid.pwd.app_error", - }, - "TooLong": { - Password: strings.Repeat("x", model.PASSWORD_MAXIMUM_LENGTH+1), - ExpectedError: "model.user.is_valid.pwd.app_error", - }, - } { - t.Run(name, func(t *testing.T) { - if err := IsPasswordValid(tc.Password); tc.ExpectedError == "" { - assert.Nil(t, err) - } else { - assert.Equal(t, tc.ExpectedError, err.Id) - } - }) - } -} - func TestIsPasswordValidWithSettings(t *testing.T) { for name, tc := range map[string]struct { Password string @@ -49,54 +19,84 @@ func TestIsPasswordValidWithSettings(t *testing.T) { Password: strings.Repeat("x", 3), Settings: &model.PasswordSettings{ MinimumLength: model.NewInt(3), + Lowercase: model.NewBool(false), + Uppercase: model.NewBool(false), + Number: model.NewBool(false), + Symbol: model.NewBool(false), }, }, "Long": { Password: strings.Repeat("x", model.PASSWORD_MAXIMUM_LENGTH), - Settings: &model.PasswordSettings{}, + Settings: &model.PasswordSettings{ + Lowercase: model.NewBool(false), + Uppercase: model.NewBool(false), + Number: model.NewBool(false), + Symbol: model.NewBool(false), + }, }, "TooShort": { Password: strings.Repeat("x", 2), Settings: &model.PasswordSettings{ MinimumLength: model.NewInt(3), + Lowercase: model.NewBool(false), + Uppercase: model.NewBool(false), + Number: model.NewBool(false), + Symbol: model.NewBool(false), }, ExpectedError: "model.user.is_valid.pwd.app_error", }, "TooLong": { Password: strings.Repeat("x", model.PASSWORD_MAXIMUM_LENGTH+1), - Settings: &model.PasswordSettings{}, + Settings: &model.PasswordSettings{ + Lowercase: model.NewBool(false), + Uppercase: model.NewBool(false), + Number: model.NewBool(false), + Symbol: model.NewBool(false), + }, ExpectedError: "model.user.is_valid.pwd.app_error", }, "MissingLower": { - Password: "ASD123!@#", + Password: "AAAAAAAAAAASD123!@#", Settings: &model.PasswordSettings{ Lowercase: model.NewBool(true), + Uppercase: model.NewBool(false), + Number: model.NewBool(false), + Symbol: model.NewBool(false), }, ExpectedError: "model.user.is_valid.pwd_lowercase.app_error", }, "MissingUpper": { - Password: "asd123!@#", + Password: "aaaaaaaaaaaaasd123!@#", Settings: &model.PasswordSettings{ Uppercase: model.NewBool(true), + Lowercase: model.NewBool(false), + Number: model.NewBool(false), + Symbol: model.NewBool(false), }, ExpectedError: "model.user.is_valid.pwd_uppercase.app_error", }, "MissingNumber": { - Password: "asdASD!@#", + Password: "asasdasdsadASD!@#", Settings: &model.PasswordSettings{ Number: model.NewBool(true), + Lowercase: model.NewBool(false), + Uppercase: model.NewBool(false), + Symbol: model.NewBool(false), }, ExpectedError: "model.user.is_valid.pwd_number.app_error", }, "MissingSymbol": { - Password: "asdASD123", + Password: "asdasdasdasdasdASD123", Settings: &model.PasswordSettings{ Symbol: model.NewBool(true), + Lowercase: model.NewBool(false), + Uppercase: model.NewBool(false), + Number: model.NewBool(false), }, ExpectedError: "model.user.is_valid.pwd_symbol.app_error", }, "MissingMultiple": { - Password: "asd", + Password: "asdasdasdasdasdasd", Settings: &model.PasswordSettings{ Lowercase: model.NewBool(true), Uppercase: model.NewBool(true), diff --git a/web/web_test.go b/web/web_test.go index 91e1f94633..0a11386779 100644 --- a/web/web_test.go +++ b/web/web_test.go @@ -66,6 +66,15 @@ func Setup() *TestHelper { } a.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.ListenAddress = prevListenAddress }) + // Disable strict password requirements for test + a.UpdateConfig(func(cfg *model.Config) { + *cfg.PasswordSettings.MinimumLength = 5 + *cfg.PasswordSettings.Lowercase = false + *cfg.PasswordSettings.Uppercase = false + *cfg.PasswordSettings.Symbol = false + *cfg.PasswordSettings.Number = false + }) + web := New(s, s.AppOptions, s.Router) URL = fmt.Sprintf("http://localhost:%v", a.Srv.ListenAddr.Port) ApiClient = model.NewAPIv4Client(URL)