From 8bc114aa4e1646b3f8d8a0d27f89283d8b1b223c Mon Sep 17 00:00:00 2001 From: Guillermo Prandi Date: Tue, 15 Oct 2019 11:17:11 -0300 Subject: [PATCH 1/9] Fix extra space --- options/locale/locale_en-US.ini | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index 4a92b080300b1..94ded93c59c83 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -315,7 +315,7 @@ team_no_units_error = Allow access to at least one repository section. email_been_used = The email address is already used. openid_been_used = The OpenID address '%s' is already used. username_password_incorrect = Username or password is incorrect. -password_complexity = Password does not pass complexity requirements. +password_complexity = Password does not pass complexity requirements. enterred_invalid_repo_name = The repository name you entered is incorrect. enterred_invalid_owner_name = The new owner name is not valid. enterred_invalid_password = The password you entered is incorrect. From 91cb47b54d9e2a66c7baa5d6ad0f695bc3493d89 Mon Sep 17 00:00:00 2001 From: Guillermo Prandi Date: Tue, 15 Oct 2019 11:18:31 -0300 Subject: [PATCH 2/9] Fix regular expression --- modules/setting/setting.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/setting/setting.go b/modules/setting/setting.go index 278ed4b107e98..abba752c2f13f 100644 --- a/modules/setting/setting.go +++ b/modules/setting/setting.go @@ -779,7 +779,7 @@ func NewContext() { "lower": "[a-z]+", "upper": "[A-Z]+", "digit": "[0-9]+", - "spec": `][ !"#$%&'()*+,./:;<=>?@\\^_{|}~` + "`-", + "spec": `[][ !"#$%&'()*+,./:;<=>?@\\^_{|}~` + "`-]", } PasswordComplexity = make(map[string]string) cfgdata := sec.Key("PASSWORD_COMPLEXITY").Strings(",") From a7debc510e068cc54cb86859fe492e7bb12912c2 Mon Sep 17 00:00:00 2001 From: Guillermo Prandi Date: Tue, 15 Oct 2019 11:18:47 -0300 Subject: [PATCH 3/9] Fix error template name --- routers/user/setting/account.go | 2 +- routers/user/setting/account_test.go | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/routers/user/setting/account.go b/routers/user/setting/account.go index c7822242162f7..e7de2dffd40b4 100644 --- a/routers/user/setting/account.go +++ b/routers/user/setting/account.go @@ -54,7 +54,7 @@ func AccountPost(ctx *context.Context, form auth.ChangePasswordForm) { } else if form.Password != form.Retype { ctx.Flash.Error(ctx.Tr("form.password_not_match")) } else if !password.IsComplexEnough(form.Password) { - ctx.Flash.Error(ctx.Tr("settings.password_complexity")) + ctx.Flash.Error(ctx.Tr("form.password_complexity")) } else { var err error if ctx.User.Salt, err = models.GetUserSalt(); err != nil { diff --git a/routers/user/setting/account_test.go b/routers/user/setting/account_test.go index 497ee658b0ca5..4fa48871f58e7 100644 --- a/routers/user/setting/account_test.go +++ b/routers/user/setting/account_test.go @@ -74,21 +74,21 @@ func TestChangePassword(t *testing.T) { OldPassword: oldPassword, NewPassword: "Qwerty", Retype: "Qwerty", - Message: "settings.password_complexity", + Message: "form.password_complexity", PasswordComplexity: setting.PasswordComplexity, }, { OldPassword: oldPassword, NewPassword: "Qwerty", Retype: "Qwerty", - Message: "settings.password_complexity", + Message: "form.password_complexity", PasswordComplexity: pcLUN, }, { OldPassword: oldPassword, NewPassword: "QWERTY", Retype: "QWERTY", - Message: "settings.password_complexity", + Message: "form.password_complexity", PasswordComplexity: pcLU, }, } { From 64010184a851b6f0ccd72f74f8362bb93a3e19f9 Mon Sep 17 00:00:00 2001 From: Guillermo Prandi Date: Tue, 15 Oct 2019 16:11:17 -0300 Subject: [PATCH 4/9] Simplify check code, fix default values, add test --- custom/conf/app.ini.sample | 3 +- .../doc/advanced/config-cheat-sheet.en-us.md | 3 +- modules/password/password.go | 56 +++++++++------ modules/password/password_test.go | 68 +++++++++++++++++++ modules/setting/setting.go | 24 ++----- 5 files changed, 113 insertions(+), 41 deletions(-) create mode 100644 modules/password/password_test.go diff --git a/custom/conf/app.ini.sample b/custom/conf/app.ini.sample index 79d9960052490..eb629a9795f09 100644 --- a/custom/conf/app.ini.sample +++ b/custom/conf/app.ini.sample @@ -333,7 +333,8 @@ IMPORT_LOCAL_PATHS = false ; Set to true to prevent all users (including admin) from creating custom git hooks DISABLE_GIT_HOOKS = false ;Comma separated list of character classes required to pass minimum complexity. -;If left empty or no valid values are specified, the default values (`lower,upper,digit,spec`) will be used. +;If left empty or no valid values are specified, the default values ("lower,upper,digit,spec") will be used. +;Use "off" to disable checking. PASSWORD_COMPLEXITY = lower,upper,digit,spec ; Password Hash algorithm, either "pbkdf2", "argon2", "scrypt" or "bcrypt" PASSWORD_HASH_ALGO = pbkdf2 diff --git a/docs/content/doc/advanced/config-cheat-sheet.en-us.md b/docs/content/doc/advanced/config-cheat-sheet.en-us.md index 100bb229ee6ab..2e0ff91910e2b 100644 --- a/docs/content/doc/advanced/config-cheat-sheet.en-us.md +++ b/docs/content/doc/advanced/config-cheat-sheet.en-us.md @@ -212,7 +212,8 @@ Values containing `#` or `;` must be quoted using `` ` `` or `"""`. - lower - use one or more lower latin characters - upper - use one or more upper latin characters - digit - use one or more digits - - spec - use one or more special characters as ``][!"#$%&'()*+,./:;<=>?@\^_{|}~`-`` and space symbol. + - spec - use one or more special characters as ``!"#$%&'()*+,-./:;<=>?@[\\]^_`{|}~`` + - off - do not check password complexity ## OpenID (`openid`) diff --git a/modules/password/password.go b/modules/password/password.go index 54131b9641556..e666251a1bd6c 100644 --- a/modules/password/password.go +++ b/modules/password/password.go @@ -7,45 +7,59 @@ package password import ( "crypto/rand" "math/big" - "regexp" + "strings" "sync" "code.gitea.io/gitea/modules/setting" ) -var matchComplexities = map[string]regexp.Regexp{} var matchComplexityOnce sync.Once var validChars string -var validComplexities = map[string]string{ - "lower": "abcdefghijklmnopqrstuvwxyz", - "upper": "ABCDEFGHIJKLMNOPQRSTUVWXYZ", - "digit": "0123456789", - "spec": `][ !"#$%&'()*+,./:;<=>?@\^_{|}~` + "`-", +var requiredChars []string + +var charComplexities = map[string]string{ + "lower": `abcdefghijklmnopqrstuvwxyz`, + "upper": `ABCDEFGHIJKLMNOPQRSTUVWXYZ`, + "digit": `0123456789`, + "spec": ` !"#$%&'()*+,-./:;<=>?@[\]^_{|}~` + "`", } // NewComplexity for preparation func NewComplexity() { matchComplexityOnce.Do(func() { - if len(setting.PasswordComplexity) > 0 { - for key, val := range setting.PasswordComplexity { - matchComplexity := regexp.MustCompile(val) - matchComplexities[key] = *matchComplexity - validChars += validComplexities[key] + setupComplexity(setting.PasswordComplexity) + }) +} + +func setupComplexity(values []string) { + validChars = "" + requiredChars = make([]string, 0, len(values)) + if len(values) != 1 || values[0] != "off" { + for _, val := range values { + if chars, ok := charComplexities[val]; ok { + validChars += chars + requiredChars = append(requiredChars, chars) } - } else { - for _, val := range validComplexities { - validChars += val + } + if len(requiredChars) == 0 { + for _, chars := range charComplexities { + validChars += chars + requiredChars = append(requiredChars, chars) } } - }) + } + if validChars == "" { + // Provide a sensible default for password generation + validChars = charComplexities["lower"] + charComplexities["upper"] + charComplexities["digit"] + } } -// IsComplexEnough return True if password is Complexity +// IsComplexEnough return True if password meets complexity settings func IsComplexEnough(pwd string) bool { - if len(setting.PasswordComplexity) > 0 { - NewComplexity() - for _, val := range matchComplexities { - if !val.MatchString(pwd) { + NewComplexity() + if len(validChars) > 0 { + for _, req := range requiredChars { + if strings.IndexAny(req, pwd) == -1 { return false } } diff --git a/modules/password/password_test.go b/modules/password/password_test.go new file mode 100644 index 0000000000000..2081efb1e0404 --- /dev/null +++ b/modules/password/password_test.go @@ -0,0 +1,68 @@ +// Copyright 2019 The Gitea Authors. All rights reserved. +// Use of this source code is governed by a MIT-style +// license that can be found in the LICENSE file. + +package password + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestComplexity_IsComplexEnough(t *testing.T) { + matchComplexityOnce.Do(func() {}) + + testlist := []struct { + complexity []string + truevalues []string + falsevalues []string + }{ + {[]string{"lower"}, []string{"abc", "abc!"}, []string{"ABC", "123", "=!$", ""}}, + {[]string{"upper"}, []string{"ABC"}, []string{"abc", "123", "=!$", "abc!", ""}}, + {[]string{"digit"}, []string{"123"}, []string{"abc", "ABC", "=!$", "abc!", ""}}, + {[]string{"spec"}, []string{"=!$", "abc!"}, []string{"abc", "ABC", "123", ""}}, + {[]string{"off"}, []string{"abc", "ABC", "123", "=!$", "abc!", ""}, nil}, + {[]string{"lower", "spec"}, []string{"abc!"}, []string{"abc", "ABC", "123", "=!$", "abcABC123", ""}}, + {[]string{"lower", "upper", "digit"}, []string{"abcABC123"}, []string{"abc", "ABC", "123", "=!$", "abc!", ""}}, + } + + for _, test := range testlist { + setupComplexity(test.complexity) + for _, val := range test.truevalues { + assert.True(t, IsComplexEnough(val)) + } + for _, val := range test.falsevalues { + assert.False(t, IsComplexEnough(val)) + } + } + + // Remove settings for other tests + setupComplexity([]string{"off"}) +} + +func TestComplexity_Generate(t *testing.T) { + matchComplexityOnce.Do(func() {}) + + const maxCount = 50 + const pwdLen = 50 + + test := func(t *testing.T, modes []string) { + setupComplexity(modes) + for i := 0; i < maxCount; i++ { + pwd, err := Generate(pwdLen) + assert.NoError(t, err) + assert.Equal(t, pwdLen, len(pwd)) + assert.True(t, IsComplexEnough(pwd), "Failed complexities with modes %+v for generated: %s", modes, pwd) + } + } + + test(t, []string{"lower"}) + test(t, []string{"upper"}) + test(t, []string{"lower", "upper", "spec"}) + test(t, []string{"off"}) + test(t, []string{""}) + + // Remove settings for other tests + setupComplexity([]string{"off"}) +} diff --git a/modules/setting/setting.go b/modules/setting/setting.go index abba752c2f13f..64a981c1b6c30 100644 --- a/modules/setting/setting.go +++ b/modules/setting/setting.go @@ -146,7 +146,7 @@ var ( MinPasswordLength int ImportLocalPaths bool DisableGitHooks bool - PasswordComplexity map[string]string + PasswordComplexity []string PasswordHashAlgo string // UI settings @@ -775,26 +775,14 @@ func NewContext() { InternalToken = loadInternalToken(sec) - var dictPC = map[string]string{ - "lower": "[a-z]+", - "upper": "[A-Z]+", - "digit": "[0-9]+", - "spec": `[][ !"#$%&'()*+,./:;<=>?@\\^_{|}~` + "`-]", - } - PasswordComplexity = make(map[string]string) cfgdata := sec.Key("PASSWORD_COMPLEXITY").Strings(",") - for _, y := range cfgdata { - ts := strings.TrimSpace(y) - for a := range dictPC { - if strings.ToLower(ts) == a { - PasswordComplexity[ts] = dictPC[ts] - break - } + PasswordComplexity := make([]string, 0, len(cfgdata)) + for _, name := range cfgdata { + name := strings.ToLower(strings.Trim(name, `"`)) + if name != "" { + PasswordComplexity = append(PasswordComplexity, name) } } - if len(PasswordComplexity) == 0 { - PasswordComplexity = dictPC - } sec = Cfg.Section("attachment") AttachmentPath = sec.Key("PATH").MustString(path.Join(AppDataPath, "attachments")) From 95d8f0b8846a7b77e3c5263c8d98c3ebb9668212 Mon Sep 17 00:00:00 2001 From: Guillermo Prandi Date: Tue, 15 Oct 2019 16:29:52 -0300 Subject: [PATCH 5/9] Fix router tests --- routers/admin/users_test.go | 4 ++-- routers/user/setting/account_test.go | 30 +++++++++------------------- 2 files changed, 11 insertions(+), 23 deletions(-) diff --git a/routers/admin/users_test.go b/routers/admin/users_test.go index e054524fd1664..2b36b45d49cdd 100644 --- a/routers/admin/users_test.go +++ b/routers/admin/users_test.go @@ -34,7 +34,7 @@ func TestNewUserPost_MustChangePassword(t *testing.T) { LoginName: "local", UserName: username, Email: email, - Password: "xxxxxxxx", + Password: "abc123ABC!=$", SendNotify: false, MustChangePassword: true, } @@ -71,7 +71,7 @@ func TestNewUserPost_MustChangePasswordFalse(t *testing.T) { LoginName: "local", UserName: username, Email: email, - Password: "xxxxxxxx", + Password: "abc123ABC!=$", SendNotify: false, MustChangePassword: false, } diff --git a/routers/user/setting/account_test.go b/routers/user/setting/account_test.go index 4fa48871f58e7..a90add077d27a 100644 --- a/routers/user/setting/account_test.go +++ b/routers/user/setting/account_test.go @@ -19,63 +19,51 @@ import ( func TestChangePassword(t *testing.T) { oldPassword := "password" setting.MinPasswordLength = 6 - setting.PasswordComplexity = map[string]string{ - "lower": "[a-z]+", - "upper": "[A-Z]+", - "digit": "[0-9]+", - "spec": "[-_]+", - } - var pcLUN = map[string]string{ - "lower": "[a-z]+", - "upper": "[A-Z]+", - "digit": "[0-9]+", - } - var pcLU = map[string]string{ - "lower": "[a-z]+", - "upper": "[A-Z]+", - } + var pcALL = []string{"lower","upper","digit","spec"} + var pcLUN = []string{"lower","upper","digit"} + var pcLU = []string{"lower","upper"} for _, req := range []struct { OldPassword string NewPassword string Retype string Message string - PasswordComplexity map[string]string + PasswordComplexity []string }{ { OldPassword: oldPassword, NewPassword: "Qwerty123456-", Retype: "Qwerty123456-", Message: "", - PasswordComplexity: setting.PasswordComplexity, + PasswordComplexity: pcALL, }, { OldPassword: oldPassword, NewPassword: "12345", Retype: "12345", Message: "auth.password_too_short", - PasswordComplexity: setting.PasswordComplexity, + PasswordComplexity: pcALL, }, { OldPassword: "12334", NewPassword: "123456", Retype: "123456", Message: "settings.password_incorrect", - PasswordComplexity: setting.PasswordComplexity, + PasswordComplexity: pcALL, }, { OldPassword: oldPassword, NewPassword: "123456", Retype: "12345", Message: "form.password_not_match", - PasswordComplexity: setting.PasswordComplexity, + PasswordComplexity: pcALL, }, { OldPassword: oldPassword, NewPassword: "Qwerty", Retype: "Qwerty", Message: "form.password_complexity", - PasswordComplexity: setting.PasswordComplexity, + PasswordComplexity: pcALL, }, { OldPassword: oldPassword, From 393a1062a339f183b0d22fc76fcfe35c99f5ed8d Mon Sep 17 00:00:00 2001 From: Guillermo Prandi Date: Tue, 15 Oct 2019 16:33:49 -0300 Subject: [PATCH 6/9] Fix fmt --- routers/user/setting/account_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/routers/user/setting/account_test.go b/routers/user/setting/account_test.go index a90add077d27a..41783e19d736a 100644 --- a/routers/user/setting/account_test.go +++ b/routers/user/setting/account_test.go @@ -19,9 +19,9 @@ import ( func TestChangePassword(t *testing.T) { oldPassword := "password" setting.MinPasswordLength = 6 - var pcALL = []string{"lower","upper","digit","spec"} - var pcLUN = []string{"lower","upper","digit"} - var pcLU = []string{"lower","upper"} + var pcALL = []string{"lower", "upper", "digit", "spec"} + var pcLUN = []string{"lower", "upper", "digit"} + var pcLU = []string{"lower", "upper"} for _, req := range []struct { OldPassword string From 30f512f926522ad78e362e4bf2a3841c051355ab Mon Sep 17 00:00:00 2001 From: Guillermo Prandi Date: Tue, 15 Oct 2019 16:44:44 -0300 Subject: [PATCH 7/9] Fix setting and lint --- modules/password/password.go | 2 +- modules/setting/setting.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/modules/password/password.go b/modules/password/password.go index e666251a1bd6c..112af65be6de3 100644 --- a/modules/password/password.go +++ b/modules/password/password.go @@ -59,7 +59,7 @@ func IsComplexEnough(pwd string) bool { NewComplexity() if len(validChars) > 0 { for _, req := range requiredChars { - if strings.IndexAny(req, pwd) == -1 { + if !strings.ContainsAny(req, pwd) { return false } } diff --git a/modules/setting/setting.go b/modules/setting/setting.go index 64a981c1b6c30..663d2fb7b6cfe 100644 --- a/modules/setting/setting.go +++ b/modules/setting/setting.go @@ -776,7 +776,7 @@ func NewContext() { InternalToken = loadInternalToken(sec) cfgdata := sec.Key("PASSWORD_COMPLEXITY").Strings(",") - PasswordComplexity := make([]string, 0, len(cfgdata)) + PasswordComplexity = make([]string, 0, len(cfgdata)) for _, name := range cfgdata { name := strings.ToLower(strings.Trim(name, `"`)) if name != "" { From a1724bf8004a02e1ffbb0a1cfde9b282fc712836 Mon Sep 17 00:00:00 2001 From: Guillermo Prandi Date: Tue, 15 Oct 2019 18:45:19 -0300 Subject: [PATCH 8/9] Move cleaning up code to test, improve comments --- modules/password/password.go | 5 ++--- modules/password/password_test.go | 15 +++++++++++---- 2 files changed, 13 insertions(+), 7 deletions(-) diff --git a/modules/password/password.go b/modules/password/password.go index 112af65be6de3..bc07e4ae7ee8b 100644 --- a/modules/password/password.go +++ b/modules/password/password.go @@ -32,8 +32,6 @@ func NewComplexity() { } func setupComplexity(values []string) { - validChars = "" - requiredChars = make([]string, 0, len(values)) if len(values) != 1 || values[0] != "off" { for _, val := range values { if chars, ok := charComplexities[val]; ok { @@ -42,6 +40,7 @@ func setupComplexity(values []string) { } } if len(requiredChars) == 0 { + // No valid character classes found; use all classes as default for _, chars := range charComplexities { validChars += chars requiredChars = append(requiredChars, chars) @@ -49,7 +48,7 @@ func setupComplexity(values []string) { } } if validChars == "" { - // Provide a sensible default for password generation + // No complexities to check; provide a sensible default for password generation validChars = charComplexities["lower"] + charComplexities["upper"] + charComplexities["digit"] } } diff --git a/modules/password/password_test.go b/modules/password/password_test.go index 2081efb1e0404..d46a6d1571e9e 100644 --- a/modules/password/password_test.go +++ b/modules/password/password_test.go @@ -28,7 +28,7 @@ func TestComplexity_IsComplexEnough(t *testing.T) { } for _, test := range testlist { - setupComplexity(test.complexity) + testComplextity(test.complexity) for _, val := range test.truevalues { assert.True(t, IsComplexEnough(val)) } @@ -38,7 +38,7 @@ func TestComplexity_IsComplexEnough(t *testing.T) { } // Remove settings for other tests - setupComplexity([]string{"off"}) + testComplextity([]string{"off"}) } func TestComplexity_Generate(t *testing.T) { @@ -48,7 +48,7 @@ func TestComplexity_Generate(t *testing.T) { const pwdLen = 50 test := func(t *testing.T, modes []string) { - setupComplexity(modes) + testComplextity(modes) for i := 0; i < maxCount; i++ { pwd, err := Generate(pwdLen) assert.NoError(t, err) @@ -64,5 +64,12 @@ func TestComplexity_Generate(t *testing.T) { test(t, []string{""}) // Remove settings for other tests - setupComplexity([]string{"off"}) + testComplextity([]string{"off"}) +} + +func testComplextity(values []string) { + // Cleanup previous values + validChars = "" + requiredChars = make([]string, 0, len(values)) + setupComplexity(values) } From 31f4a1a85aeb339d2678ef83f2fb3f31cc1bceb3 Mon Sep 17 00:00:00 2001 From: Guillermo Prandi Date: Tue, 15 Oct 2019 22:44:07 -0300 Subject: [PATCH 9/9] Tidy up variable declaration --- modules/password/password.go | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/modules/password/password.go b/modules/password/password.go index bc07e4ae7ee8b..92986977ec6dd 100644 --- a/modules/password/password.go +++ b/modules/password/password.go @@ -13,16 +13,18 @@ import ( "code.gitea.io/gitea/modules/setting" ) -var matchComplexityOnce sync.Once -var validChars string -var requiredChars []string +var ( + matchComplexityOnce sync.Once + validChars string + requiredChars []string -var charComplexities = map[string]string{ - "lower": `abcdefghijklmnopqrstuvwxyz`, - "upper": `ABCDEFGHIJKLMNOPQRSTUVWXYZ`, - "digit": `0123456789`, - "spec": ` !"#$%&'()*+,-./:;<=>?@[\]^_{|}~` + "`", -} + charComplexities = map[string]string{ + "lower": `abcdefghijklmnopqrstuvwxyz`, + "upper": `ABCDEFGHIJKLMNOPQRSTUVWXYZ`, + "digit": `0123456789`, + "spec": ` !"#$%&'()*+,-./:;<=>?@[\]^_{|}~` + "`", + } +) // NewComplexity for preparation func NewComplexity() {