From 9d5ac4526e69f2d0fc5c027a8b53a534f44f86f2 Mon Sep 17 00:00:00 2001 From: James Elliott Date: Sat, 9 Apr 2022 09:21:49 +1000 Subject: [PATCH] fix(configuration): remove unused password policy option (#3149) Removes the min score option from the ZXCVBN policy and adds tests. --- api/openapi.yml | 3 - config.template.yml | 1 - internal/configuration/config.template.yml | 1 - .../configuration/schema/password_policy.go | 14 +-- internal/configuration/validator/const.go | 6 +- internal/configuration/validator/keys_test.go | 2 +- .../validator/password_policy.go | 28 ++--- .../validator/password_policy_test.go | 113 ++++++++++++++++++ .../handler_configuration_password_policy.go | 3 +- internal/handlers/types.go | 1 - internal/middlewares/password_policy_test.go | 2 +- web/src/components/PasswordMeter.test.tsx | 2 - web/src/models/PasswordPolicy.ts | 1 - .../services/PasswordPolicyConfiguration.ts | 1 - .../ResetPassword/ResetPasswordStep2.tsx | 1 - 15 files changed, 138 insertions(+), 41 deletions(-) create mode 100644 internal/configuration/validator/password_policy_test.go diff --git a/api/openapi.yml b/api/openapi.yml index 10ec36b9d..62c8ee3b4 100644 --- a/api/openapi.yml +++ b/api/openapi.yml @@ -730,9 +730,6 @@ components: max_length: type: integer description: The maximum password length when using the standard mode. - min_score: - type: integer - description: The minimum password score when using the zxcvbn mode. require_uppercase: type: boolean description: If uppercase characters are required when using the standard mode. diff --git a/config.template.yml b/config.template.yml index 4e45513b8..1cc564f53 100644 --- a/config.template.yml +++ b/config.template.yml @@ -364,7 +364,6 @@ password_policy: ## zxcvbn is a well known and used password strength algorithm. It does not have tunable settings. zxcvbn: enabled: false - min_score: 0 ## ## Access Control Configuration diff --git a/internal/configuration/config.template.yml b/internal/configuration/config.template.yml index 4e45513b8..1cc564f53 100644 --- a/internal/configuration/config.template.yml +++ b/internal/configuration/config.template.yml @@ -364,7 +364,6 @@ password_policy: ## zxcvbn is a well known and used password strength algorithm. It does not have tunable settings. zxcvbn: enabled: false - min_score: 0 ## ## Access Control Configuration diff --git a/internal/configuration/schema/password_policy.go b/internal/configuration/schema/password_policy.go index a87627f0f..7b247725a 100644 --- a/internal/configuration/schema/password_policy.go +++ b/internal/configuration/schema/password_policy.go @@ -11,16 +11,15 @@ type PasswordPolicyStandardParams struct { RequireSpecial bool `koanf:"require_special"` } -// PasswordPolicyZxcvbnParams represents the configuration related to zxcvbn parameters of password policy. -type PasswordPolicyZxcvbnParams struct { - Enabled bool `koanf:"enabled"` - MinScore int `koanf:"min_score"` +// PasswordPolicyZXCVBNParams represents the configuration related to ZXCVBN parameters of password policy. +type PasswordPolicyZXCVBNParams struct { + Enabled bool `koanf:"enabled"` } // PasswordPolicyConfiguration represents the configuration related to password policy. type PasswordPolicyConfiguration struct { Standard PasswordPolicyStandardParams `koanf:"standard"` - Zxcvbn PasswordPolicyZxcvbnParams `koanf:"zxcvbn"` + ZXCVBN PasswordPolicyZXCVBNParams `koanf:"zxcvbn"` } // DefaultPasswordPolicyConfiguration is the default password policy configuration. @@ -30,8 +29,7 @@ var DefaultPasswordPolicyConfiguration = PasswordPolicyConfiguration{ MinLength: 8, MaxLength: 0, }, - Zxcvbn: PasswordPolicyZxcvbnParams{ - Enabled: false, - MinScore: 0, + ZXCVBN: PasswordPolicyZXCVBNParams{ + Enabled: false, }, } diff --git a/internal/configuration/validator/const.go b/internal/configuration/validator/const.go index 0698c8995..02f48f875 100644 --- a/internal/configuration/validator/const.go +++ b/internal/configuration/validator/const.go @@ -243,6 +243,11 @@ const ( errFmtServerBufferSize = "server: option '%s_buffer_size' must be above 0 but it is configured as '%d'" ) +const ( + errFmtPasswordPolicyMinLengthNotGreaterThanZero = "password_policy: standard: option 'min_length' must be greater than 0 but is configured as %d" + errPasswordPolicyMultipleDefined = "password_policy: only a single password policy mechanism can be specified" +) + // Error constants. const ( /* @@ -519,7 +524,6 @@ var ValidKeys = []string{ "password_policy.standard.require_number", "password_policy.standard.require_special", "password_policy.zxcvbn.enabled", - "password_policy.zxcvbn.min_score", } var replacedKeys = map[string]string{ diff --git a/internal/configuration/validator/keys_test.go b/internal/configuration/validator/keys_test.go index 23153666b..57fd0e06b 100644 --- a/internal/configuration/validator/keys_test.go +++ b/internal/configuration/validator/keys_test.go @@ -54,7 +54,7 @@ func TestAllSpecificErrorKeys(t *testing.T) { var uniqueValues []string - // Setup configKeys and uniqueValues expected. + // Setup configKeys and uniqueValues expectedErrs. for key, value := range specificErrorKeys { configKeys = append(configKeys, key) diff --git a/internal/configuration/validator/password_policy.go b/internal/configuration/validator/password_policy.go index ba5cf570c..0e7408f6b 100644 --- a/internal/configuration/validator/password_policy.go +++ b/internal/configuration/validator/password_policy.go @@ -1,33 +1,27 @@ package validator import ( - "errors" + "fmt" "github.com/authelia/authelia/v4/internal/configuration/schema" "github.com/authelia/authelia/v4/internal/utils" ) // ValidatePasswordPolicy validates and update Password Policy configuration. -func ValidatePasswordPolicy(configuration *schema.PasswordPolicyConfiguration, validator *schema.StructValidator) { - if !utils.IsBoolCountLessThanN(1, true, configuration.Standard.Enabled, configuration.Zxcvbn.Enabled) { - validator.Push(errors.New("password_policy:only one password policy can be enabled at a time")) +func ValidatePasswordPolicy(config *schema.PasswordPolicyConfiguration, validator *schema.StructValidator) { + if !utils.IsBoolCountLessThanN(1, true, config.Standard.Enabled, config.ZXCVBN.Enabled) { + validator.Push(fmt.Errorf(errPasswordPolicyMultipleDefined)) } - if configuration.Standard.Enabled { - if configuration.Standard.MinLength == 0 { - configuration.Standard.MinLength = schema.DefaultPasswordPolicyConfiguration.Standard.MinLength - } else if configuration.Standard.MinLength < 0 { - validator.Push(errors.New("password_policy: min_length must be > 0")) + if config.Standard.Enabled { + if config.Standard.MinLength == 0 { + config.Standard.MinLength = schema.DefaultPasswordPolicyConfiguration.Standard.MinLength + } else if config.Standard.MinLength < 0 { + validator.Push(fmt.Errorf(errFmtPasswordPolicyMinLengthNotGreaterThanZero, config.Standard.MinLength)) } - if configuration.Standard.MaxLength == 0 { - configuration.Standard.MaxLength = schema.DefaultPasswordPolicyConfiguration.Standard.MaxLength - } - } else if configuration.Zxcvbn.Enabled { - if configuration.Zxcvbn.MinScore == 0 { - configuration.Zxcvbn.MinScore = schema.DefaultPasswordPolicyConfiguration.Zxcvbn.MinScore - } else if configuration.Zxcvbn.MinScore < 0 || configuration.Zxcvbn.MinScore > 4 { - validator.Push(errors.New("min_score must be between 0 and 4")) + if config.Standard.MaxLength == 0 { + config.Standard.MaxLength = schema.DefaultPasswordPolicyConfiguration.Standard.MaxLength } } } diff --git a/internal/configuration/validator/password_policy_test.go b/internal/configuration/validator/password_policy_test.go new file mode 100644 index 000000000..5f341672d --- /dev/null +++ b/internal/configuration/validator/password_policy_test.go @@ -0,0 +1,113 @@ +package validator + +import ( + "fmt" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/authelia/authelia/v4/internal/configuration/schema" +) + +func TestValidatePasswordPolicy(t *testing.T) { + testCases := []struct { + desc string + have, expected *schema.PasswordPolicyConfiguration + expectedErrs []string + }{ + { + desc: "ShouldRaiseErrorsWhenMisconfigured", + have: &schema.PasswordPolicyConfiguration{ + Standard: schema.PasswordPolicyStandardParams{ + Enabled: true, + MinLength: -1, + }, + ZXCVBN: schema.PasswordPolicyZXCVBNParams{ + Enabled: true, + }, + }, + expected: &schema.PasswordPolicyConfiguration{ + Standard: schema.PasswordPolicyStandardParams{ + Enabled: true, + MinLength: -1, + }, + ZXCVBN: schema.PasswordPolicyZXCVBNParams{ + Enabled: true, + }, + }, + expectedErrs: []string{ + "password_policy: only a single password policy mechanism can be specified", + "password_policy: standard: option 'min_length' must be greater than 0 but is configured as -1", + }, + }, + { + desc: "ShouldNotRaiseErrorsStandard", + have: &schema.PasswordPolicyConfiguration{ + Standard: schema.PasswordPolicyStandardParams{ + Enabled: true, + MinLength: 8, + }, + }, + expected: &schema.PasswordPolicyConfiguration{ + Standard: schema.PasswordPolicyStandardParams{ + Enabled: true, + MinLength: 8, + }, + }, + }, + { + desc: "ShouldNotRaiseErrorsZXCVBN", + have: &schema.PasswordPolicyConfiguration{ + ZXCVBN: schema.PasswordPolicyZXCVBNParams{ + Enabled: true, + }, + }, + expected: &schema.PasswordPolicyConfiguration{ + ZXCVBN: schema.PasswordPolicyZXCVBNParams{ + Enabled: true, + }, + }, + }, + { + desc: "ShouldSetDefaultstandard", + have: &schema.PasswordPolicyConfiguration{ + Standard: schema.PasswordPolicyStandardParams{ + Enabled: true, + MinLength: 0, + }, + }, + expected: &schema.PasswordPolicyConfiguration{ + Standard: schema.PasswordPolicyStandardParams{ + Enabled: true, + MinLength: 8, + }, + }, + }, + } + + for _, tc := range testCases { + t.Run(tc.desc, func(t *testing.T) { + validator := &schema.StructValidator{} + ValidatePasswordPolicy(tc.have, validator) + + assert.Len(t, validator.Warnings(), 0) + + assert.Equal(t, tc.expected.Standard.MaxLength, tc.have.Standard.MaxLength) + assert.Equal(t, tc.expected.Standard.MinLength, tc.have.Standard.MinLength) + assert.Equal(t, tc.expected.Standard.RequireNumber, tc.have.Standard.RequireNumber) + assert.Equal(t, tc.expected.Standard.RequireSpecial, tc.have.Standard.RequireSpecial) + assert.Equal(t, tc.expected.Standard.RequireUppercase, tc.have.Standard.RequireUppercase) + assert.Equal(t, tc.expected.Standard.RequireLowercase, tc.have.Standard.RequireLowercase) + + errs := validator.Errors() + require.Len(t, errs, len(tc.expectedErrs)) + + for i := 0; i < len(errs); i++ { + t.Run(fmt.Sprintf("Err%d", i+1), func(t *testing.T) { + assert.EqualError(t, errs[i], tc.expectedErrs[i]) + }) + } + }) + } +} diff --git a/internal/handlers/handler_configuration_password_policy.go b/internal/handlers/handler_configuration_password_policy.go index 1840a2ad5..9e827ef6d 100644 --- a/internal/handlers/handler_configuration_password_policy.go +++ b/internal/handlers/handler_configuration_password_policy.go @@ -18,9 +18,8 @@ func PasswordPolicyConfigurationGet(ctx *middlewares.AutheliaCtx) { policyResponse.RequireUppercase = ctx.Configuration.PasswordPolicy.Standard.RequireUppercase policyResponse.RequireNumber = ctx.Configuration.PasswordPolicy.Standard.RequireNumber policyResponse.RequireSpecial = ctx.Configuration.PasswordPolicy.Standard.RequireSpecial - } else if ctx.Configuration.PasswordPolicy.Zxcvbn.Enabled { + } else if ctx.Configuration.PasswordPolicy.ZXCVBN.Enabled { policyResponse.Mode = "zxcvbn" - policyResponse.MinScore = ctx.Configuration.PasswordPolicy.Zxcvbn.MinScore } var err error diff --git a/internal/handlers/types.go b/internal/handlers/types.go index 33e096c25..6b7a4d8da 100644 --- a/internal/handlers/types.go +++ b/internal/handlers/types.go @@ -120,7 +120,6 @@ type PassworPolicyBody struct { Mode string `json:"mode"` MinLength int `json:"min_length"` MaxLength int `json:"max_length"` - MinScore int `json:"min_score"` RequireUppercase bool `json:"require_uppercase"` RequireLowercase bool `json:"require_lowercase"` RequireNumber bool `json:"require_number"` diff --git a/internal/middlewares/password_policy_test.go b/internal/middlewares/password_policy_test.go index 35e2d8811..8828f63a9 100644 --- a/internal/middlewares/password_policy_test.go +++ b/internal/middlewares/password_policy_test.go @@ -23,7 +23,7 @@ func TestNewPasswordPolicyProvider(t *testing.T) { }, { desc: "ShouldReturnUnconfiguredProviderWhenZxcvbn", - have: schema.PasswordPolicyConfiguration{Zxcvbn: schema.PasswordPolicyZxcvbnParams{Enabled: true}}, + have: schema.PasswordPolicyConfiguration{ZXCVBN: schema.PasswordPolicyZXCVBNParams{Enabled: true}}, expected: PasswordPolicyProvider{}, }, { diff --git a/web/src/components/PasswordMeter.test.tsx b/web/src/components/PasswordMeter.test.tsx index 790ddd6f3..cc77ab77e 100644 --- a/web/src/components/PasswordMeter.test.tsx +++ b/web/src/components/PasswordMeter.test.tsx @@ -12,7 +12,6 @@ it("renders without crashing", () => { policy={{ max_length: 0, min_length: 4, - min_score: 0, require_lowercase: false, require_number: false, require_special: false, @@ -30,7 +29,6 @@ it("renders adjusted height without crashing", () => { policy={{ max_length: 0, min_length: 4, - min_score: 0, require_lowercase: false, require_number: false, require_special: false, diff --git a/web/src/models/PasswordPolicy.ts b/web/src/models/PasswordPolicy.ts index 32bec62f3..454a90ffa 100644 --- a/web/src/models/PasswordPolicy.ts +++ b/web/src/models/PasswordPolicy.ts @@ -8,7 +8,6 @@ export interface PasswordPolicyConfiguration { mode: PasswordPolicyMode; min_length: number; max_length: number; - min_score: number; require_uppercase: boolean; require_lowercase: boolean; require_number: boolean; diff --git a/web/src/services/PasswordPolicyConfiguration.ts b/web/src/services/PasswordPolicyConfiguration.ts index 6b979c74d..a816d8b93 100644 --- a/web/src/services/PasswordPolicyConfiguration.ts +++ b/web/src/services/PasswordPolicyConfiguration.ts @@ -6,7 +6,6 @@ interface PasswordPolicyConfigurationPayload { mode: ModePasswordPolicy; min_length: number; max_length: number; - min_score: number; require_uppercase: boolean; require_lowercase: boolean; require_number: boolean; diff --git a/web/src/views/ResetPassword/ResetPasswordStep2.tsx b/web/src/views/ResetPassword/ResetPasswordStep2.tsx index bf1e61463..809938a3d 100644 --- a/web/src/views/ResetPassword/ResetPasswordStep2.tsx +++ b/web/src/views/ResetPassword/ResetPasswordStep2.tsx @@ -32,7 +32,6 @@ const ResetPasswordStep2 = function () { const [pPolicy, setPPolicy] = useState({ max_length: 0, min_length: 8, - min_score: 0, require_lowercase: false, require_number: false, require_special: false,