From e614f6e94e1d0f66f48bd058b015ab467d6b1b07 Mon Sep 17 00:00:00 2001 From: aeneasr <3372410+aeneasr@users.noreply.github.com> Date: Fri, 25 Feb 2022 13:47:10 +0100 Subject: [PATCH] fix: improve password error resilience on settings flow --- selfservice/strategy/password/registration.go | 16 +++++------ selfservice/strategy/password/settings.go | 1 - .../strategy/password/settings_test.go | 28 ++++++++++++++----- selfservice/strategy/password/validator.go | 2 +- 4 files changed, 30 insertions(+), 17 deletions(-) diff --git a/selfservice/strategy/password/registration.go b/selfservice/strategy/password/registration.go index c3195a1305d7..b94270df9507 100644 --- a/selfservice/strategy/password/registration.go +++ b/selfservice/strategy/password/registration.go @@ -136,16 +136,16 @@ func (s *Strategy) validateCredentials(ctx context.Context, i *identity.Identity // This should never happen return errors.WithStack(x.PseudoPanic.WithReasonf("identity object did not provide the %s CredentialType unexpectedly", identity.CredentialsTypePassword)) } else if len(c.Identifiers) == 0 { - return schema.NewMissingIdentifierError() - } - - // Sometimes, no identifier is set, but we still want to validate the password! - ids := c.Identifiers - if len(ids) == 0 { - ids = []string{""} + if err := s.d.PasswordValidator().Validate(ctx, "", pw); err != nil { + if _, ok := errorsx.Cause(err).(*herodot.DefaultError); ok { + return err + } + return schema.NewPasswordPolicyViolationError("#/password", err.Error()) + } + return nil } - for _, id := range ids { + for _, id := range c.Identifiers { if err := s.d.PasswordValidator().Validate(ctx, id, pw); err != nil { if _, ok := errorsx.Cause(err).(*herodot.DefaultError); ok { return err diff --git a/selfservice/strategy/password/settings.go b/selfservice/strategy/password/settings.go index ee0c1f3e41dd..9b9018a9f834 100644 --- a/selfservice/strategy/password/settings.go +++ b/selfservice/strategy/password/settings.go @@ -141,7 +141,6 @@ func (s *Strategy) continueSettingsFlow( } ctxUpdate.UpdateIdentity(i) - return nil } diff --git a/selfservice/strategy/password/settings_test.go b/selfservice/strategy/password/settings_test.go index 5097798ae0f1..ea7c151943e2 100644 --- a/selfservice/strategy/password/settings_test.go +++ b/selfservice/strategy/password/settings_test.go @@ -59,6 +59,15 @@ func newEmptyIdentity() *identity.Identity { } } +func newIdentityWithoutCredentials(email string) *identity.Identity { + return &identity.Identity{ + ID: x.NewUUID(), + State: identity.StateActive, + Traits: identity.Traits(`{"email":"` + email + `"}`), + SchemaID: config.DefaultIdentityTraitsSchemaID, + } +} + func TestSettings(t *testing.T) { conf, reg := internal.NewFastRegistryWithMocks(t) conf.MustSet(config.ViperKeySelfServiceBrowserDefaultReturnTo, "https://www.ory.sh/") @@ -321,6 +330,7 @@ func TestSettings(t *testing.T) { } t.Run("description=should update the password even if no password was set before", func(t *testing.T) { + email := fmt.Sprintf("test+%s@ory.sh", x.NewUUID()) var check = func(t *testing.T, actual string, id *identity.Identity) { assert.Equal(t, "success", gjson.Get(actual, "state").String(), "%s", actual) assert.Empty(t, gjson.Get(actual, "ui.nodes.#(name==password).attributes.value").String(), "%s", actual) @@ -330,7 +340,7 @@ func TestSettings(t *testing.T) { cfg := string(actualIdentity.Credentials[identity.CredentialsTypePassword].Config) assert.Contains(t, cfg, "hashed_password", "%+v", actualIdentity.Credentials) require.Len(t, actualIdentity.Credentials[identity.CredentialsTypePassword].Identifiers, 1) - assert.Contains(t, actualIdentity.Credentials[identity.CredentialsTypePassword].Identifiers[0], "-4") + assert.Equal(t, email, actualIdentity.Credentials[identity.CredentialsTypePassword].Identifiers[0]) } var payload = func(v url.Values) { @@ -338,19 +348,23 @@ func TestSettings(t *testing.T) { v.Set("password", randx.MustString(16, randx.AlphaNum)) } + id := newIdentityWithoutCredentials(email) + browserUser := testhelpers.NewHTTPClientWithIdentitySessionCookie(t, reg, id) + apiUser := testhelpers.NewHTTPClientWithIdentitySessionToken(t, reg, id) + t.Run("type=api", func(t *testing.T) { - actual := expectSuccess(t, true, false, apiUser2, payload) - check(t, actual, apiIdentity2) + actual := expectSuccess(t, true, false, apiUser, payload) + check(t, actual, id) }) t.Run("type=spa", func(t *testing.T) { - actual := expectSuccess(t, false, true, browserUser2, payload) - check(t, actual, browserIdentity2) + actual := expectSuccess(t, false, true, browserUser, payload) + check(t, actual, id) }) t.Run("type=browser", func(t *testing.T) { - actual := expectSuccess(t, false, false, browserUser2, payload) - check(t, actual, browserIdentity2) + actual := expectSuccess(t, false, false, browserUser, payload) + check(t, actual, id) }) }) diff --git a/selfservice/strategy/password/validator.go b/selfservice/strategy/password/validator.go index 09d02960dd71..ad8088b05801 100644 --- a/selfservice/strategy/password/validator.go +++ b/selfservice/strategy/password/validator.go @@ -149,7 +149,7 @@ func (s *DefaultPasswordValidator) Validate(ctx context.Context, identifier, pas return errors.Errorf("password length must be at least %d characters but only got %d", passwordPolicyConfig.MinPasswordLength, len(password)) } - if passwordPolicyConfig.IdentifierSimilarityCheckEnabled { + if passwordPolicyConfig.IdentifierSimilarityCheckEnabled && len(identifier) > 0 { compIdentifier, compPassword := strings.ToLower(identifier), strings.ToLower(password) dist := levenshtein.Distance(compIdentifier, compPassword) lcs := float32(lcsLength(compIdentifier, compPassword)) / float32(len(compPassword))