Fix password complexity regex for special characters (on master) (#8525)

guillep2k · lunny · commit 31655aabfc39 · 2019-10-16T11:09:58.000+08:00
* Fix extra space

* Fix regular expression

* Fix error template name

* Simplify check code, fix default values, add test

* Fix router tests

* Fix fmt

* Fix setting and lint

* Move cleaning up code to test, improve comments

* Tidy up variable declaration
diff --git 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
@@ -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
@@ -7,45 +7,60 @@ 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 (
+	matchComplexityOnce sync.Once
+	validChars          string
+	requiredChars       []string
+
+	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) {
+	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 {
+			// No valid character classes found; use all classes as default
+			for _, chars := range charComplexities {
+				validChars += chars
+				requiredChars = append(requiredChars, chars)
 			}
 		}
-	})
+	}
+	if validChars == "" {
+		// No complexities to check; 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.ContainsAny(req, pwd) {
 				return false
 			}
 		}
diff --git a/modules/password/password_test.go b/modules/password/password_test.go
@@ -0,0 +1,75 @@
+// 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 {
+		testComplextity(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
+	testComplextity([]string{"off"})
+}
+
+func TestComplexity_Generate(t *testing.T) {
+	matchComplexityOnce.Do(func() {})
+
+	const maxCount = 50
+	const pwdLen = 50
+
+	test := func(t *testing.T, modes []string) {
+		testComplextity(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
+	testComplextity([]string{"off"})
+}
+
+func testComplextity(values []string) {
+	// Cleanup previous values
+	validChars = ""
+	requiredChars = make([]string, 0, len(values))
+	setupComplexity(values)
+}
diff --git 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"))
diff --git 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.
diff --git 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.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
@@ -19,76 +19,64 @@ 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:            "settings.password_complexity",
-			PasswordComplexity: setting.PasswordComplexity,
+			Message:            "form.password_complexity",
+			PasswordComplexity: pcALL,
 		},
 		{
 			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,
 		},
 	} {
