From 4d7439939da21306f1fa633cc5e1aeec173fa171 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 45b86dea9ac3c..76a1daa4518f6 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 a76aad2c2add70d686c4b9c7575447b5162a13b4 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 144882976ff5d..e9a62b1f15422 100644 --- a/modules/setting/setting.go +++ b/modules/setting/setting.go @@ -785,7 +785,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 20d45dee19ff0d1e4b6733c05afb8b6feda96021 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 b76b144831933d62076780ba4791bd810599cc4e 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 08a2c8e32f5c1..aa526804f27ab 100644 --- a/custom/conf/app.ini.sample +++ b/custom/conf/app.ini.sample @@ -345,7 +345,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 6fb92f27218c3..959607dd11ec3 100644 --- a/docs/content/doc/advanced/config-cheat-sheet.en-us.md +++ b/docs/content/doc/advanced/config-cheat-sheet.en-us.md @@ -219,7 +219,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 e9a62b1f15422..753835cea4597 100644 --- a/modules/setting/setting.go +++ b/modules/setting/setting.go @@ -149,7 +149,7 @@ var ( MinPasswordLength int ImportLocalPaths bool DisableGitHooks bool - PasswordComplexity map[string]string + PasswordComplexity []string PasswordHashAlgo string // UI settings @@ -781,26 +781,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 630e16a47662e9710f7c01069d9c5e9c462d16a3 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 611c8b962f0c8fcbc829dcc173ff379cd065997c 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 b163ba9efeb636497742cad4b8debc9cd2b04ad9 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 753835cea4597..08a215effc352 100644 --- a/modules/setting/setting.go +++ b/modules/setting/setting.go @@ -782,7 +782,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 df7a449273e2bd967eded006b6d699c2b554dc59 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 c441ec73028b02ee904364c9e0f5abef6517ed05 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() {