From 00246ee13bddd6a0711ff17139695773bd8ae803 Mon Sep 17 00:00:00 2001 From: Henning Perl Date: Thu, 25 Apr 2024 13:55:05 +0200 Subject: [PATCH 1/4] fix: don't return password cred type if empty --- identity/manager.go | 13 +++++++++- identity/manager_test.go | 25 +++++++++++++++++++ .../passkey/passkey_registration_test.go | 2 +- .../strategy/webauthn/registration_test.go | 2 +- 4 files changed, 39 insertions(+), 3 deletions(-) diff --git a/identity/manager.go b/identity/manager.go index c5ab32bbe293..ca8a7e70fc9a 100644 --- a/identity/manager.go +++ b/identity/manager.go @@ -205,8 +205,19 @@ func (m *Manager) findExistingAuthMethod(ctx context.Context, e error, i *Identi if len(cred.Identifiers) > 0 { identifierHint = cred.Identifiers[0] } - duplicateCredErr.AddCredentialsType(cred.Type) duplicateCredErr.SetIdentifierHint(identifierHint) + + var cfg CredentialsPassword + if err := json.Unmarshal(cred.Config, &cfg); err != nil { + // just ignore this credential if the config is invalid + continue + } + if cfg.HashedPassword == "" { + // just ignore this credential if the hashed password is empty + continue + } + + duplicateCredErr.AddCredentialsType(cred.Type) case CredentialsTypeOIDC: var cfg CredentialsOIDC if err := json.Unmarshal(cred.Config, &cfg); err != nil { diff --git a/identity/manager_test.go b/identity/manager_test.go index 5eb3e61aa58c..e0346b8ee0c0 100644 --- a/identity/manager_test.go +++ b/identity/manager_test.go @@ -223,6 +223,31 @@ func TestManager(t *testing.T) { assert.Equal(t, verr.IdentifierHint(), email) }) + t.Run("type=oidc", func(t *testing.T) { + email := uuid.Must(uuid.NewV4()).String() + "@ory.sh" + creds := map[identity.CredentialsType]identity.Credentials{ + identity.CredentialsTypeOIDC: { + Type: identity.CredentialsTypeOIDC, + // Identifiers in OIDC are not email addresses, but a unique user ID. + Identifiers: []string{"google:" + uuid.Must(uuid.NewV4()).String()}, + Config: sqlxx.JSONRawMessage(`{"providers":[{"provider": "google"},{"provider": "github"}]}`), + }, + } + + first := createIdentity(email, "email_creds", creds) + require.NoError(t, reg.IdentityManager().Create(context.Background(), first)) + + second := createIdentity(email, "email_creds", creds) + err := reg.IdentityManager().Create(context.Background(), second) + require.Error(t, err) + + var verr = new(identity.ErrDuplicateCredentials) + assert.ErrorAs(t, err, &verr) + assert.ElementsMatch(t, []string{"oidc"}, verr.AvailableCredentials()) + assert.ElementsMatch(t, []string{"google", "github"}, verr.AvailableOIDCProviders()) + assert.Equal(t, email, verr.IdentifierHint()) + }) + t.Run("type=password+oidc+webauthn", func(t *testing.T) { email := uuid.Must(uuid.NewV4()).String() + "@ory.sh" creds := map[identity.CredentialsType]identity.Credentials{ diff --git a/selfservice/strategy/passkey/passkey_registration_test.go b/selfservice/strategy/passkey/passkey_registration_test.go index 1a4759dfa09e..6b071cd9dc82 100644 --- a/selfservice/strategy/passkey/passkey_registration_test.go +++ b/selfservice/strategy/passkey/passkey_registration_test.go @@ -372,7 +372,7 @@ func TestRegistration(t *testing.T) { assert.Contains(t, gjson.Get(actual, "ui.action").String(), fix.publicTS.URL+registration.RouteSubmitFlow, "%s", actual) registrationhelpers.CheckFormContent(t, []byte(actual), "csrf_token", "traits.username") assert.Equal(t, - "You tried signing in with "+email+" which is already in use by another account. You can sign in using your password.", + "You tried signing in with "+email+" which is already in use by another account.", gjson.Get(actual, "ui.messages.0.text").String(), "%s", actual) }) } diff --git a/selfservice/strategy/webauthn/registration_test.go b/selfservice/strategy/webauthn/registration_test.go index c65a2c1ec030..c0503b151ed8 100644 --- a/selfservice/strategy/webauthn/registration_test.go +++ b/selfservice/strategy/webauthn/registration_test.go @@ -438,7 +438,7 @@ func TestRegistration(t *testing.T) { actual, _, _ = makeRegistration(t, f, values(email)) assert.Contains(t, gjson.Get(actual, "ui.action").String(), publicTS.URL+registration.RouteSubmitFlow, "%s", actual) registrationhelpers.CheckFormContent(t, []byte(actual), node.WebAuthnRegisterTrigger, "csrf_token", "traits.username") - assert.Equal(t, "You tried signing in with "+email+" which is already in use by another account. You can sign in using your password, or your passkey or a security key.", gjson.Get(actual, "ui.messages.0.text").String(), "%s", actual) + assert.Equal(t, "You tried signing in with "+email+" which is already in use by another account. You can sign in using your passkey or a security key.", gjson.Get(actual, "ui.messages.0.text").String(), "%s", actual) }) } }) From 81dd37c5cb59f70e989800844f5e3ec804679164 Mon Sep 17 00:00:00 2001 From: Henning Perl Date: Thu, 25 Apr 2024 14:32:42 +0200 Subject: [PATCH 2/4] fix: better index for config.user_handle on identity_credentials --- persistence/sql/identity/persister_identity.go | 4 ++-- ...ity_credentials_fix_user_handle_index.cockroach.down.sql | 5 +++++ ...ntity_credentials_fix_user_handle_index.cockroach.up.sql | 6 ++++++ ...0000_identity_credentials_fix_user_handle_index.down.sql | 0 ...000000_identity_credentials_fix_user_handle_index.up.sql | 0 5 files changed, 13 insertions(+), 2 deletions(-) create mode 100644 persistence/sql/migrations/sql/20240425095000000000_identity_credentials_fix_user_handle_index.cockroach.down.sql create mode 100644 persistence/sql/migrations/sql/20240425095000000000_identity_credentials_fix_user_handle_index.cockroach.up.sql create mode 100644 persistence/sql/migrations/sql/20240425095000000000_identity_credentials_fix_user_handle_index.down.sql create mode 100644 persistence/sql/migrations/sql/20240425095000000000_identity_credentials_fix_user_handle_index.up.sql diff --git a/persistence/sql/identity/persister_identity.go b/persistence/sql/identity/persister_identity.go index ea532485944e..52daaf2fc71b 100644 --- a/persistence/sql/identity/persister_identity.go +++ b/persistence/sql/identity/persister_identity.go @@ -267,9 +267,9 @@ INNER JOIN identity_credentials FROM identity_credential_types WHERE name = ? ) -WHERE identity_credentials.config ->> '%s' = ? +WHERE identity_credentials.config ->> '%s' = ? AND identity_credentials.config ->> '%s' IS NOT NULL AND identities.nid = ? -LIMIT 1`, jsonPath), +LIMIT 1`, jsonPath, jsonPath), identity.CredentialsTypeWebAuthn, base64.StdEncoding.EncodeToString(userHandle), p.NetworkID(ctx), diff --git a/persistence/sql/migrations/sql/20240425095000000000_identity_credentials_fix_user_handle_index.cockroach.down.sql b/persistence/sql/migrations/sql/20240425095000000000_identity_credentials_fix_user_handle_index.cockroach.down.sql new file mode 100644 index 000000000000..d61a10ca9f98 --- /dev/null +++ b/persistence/sql/migrations/sql/20240425095000000000_identity_credentials_fix_user_handle_index.cockroach.down.sql @@ -0,0 +1,5 @@ +DROP INDEX identity_credentials_config_user_handle_idx; + +CREATE INVERTED INDEX identity_credentials_user_handle_idx + ON identity_credentials (config) + WHERE config ->> 'user_handle' IS NOT NULL; \ No newline at end of file diff --git a/persistence/sql/migrations/sql/20240425095000000000_identity_credentials_fix_user_handle_index.cockroach.up.sql b/persistence/sql/migrations/sql/20240425095000000000_identity_credentials_fix_user_handle_index.cockroach.up.sql new file mode 100644 index 000000000000..27e938a4519b --- /dev/null +++ b/persistence/sql/migrations/sql/20240425095000000000_identity_credentials_fix_user_handle_index.cockroach.up.sql @@ -0,0 +1,6 @@ +DROP INDEX identity_credentials_user_handle_idx; + +CREATE INDEX identity_credentials_config_user_handle_idx + ON identity_credentials ((config ->> 'user_handle')) + WHERE config ->> 'user_handle' IS NOT NULL +; diff --git a/persistence/sql/migrations/sql/20240425095000000000_identity_credentials_fix_user_handle_index.down.sql b/persistence/sql/migrations/sql/20240425095000000000_identity_credentials_fix_user_handle_index.down.sql new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/persistence/sql/migrations/sql/20240425095000000000_identity_credentials_fix_user_handle_index.up.sql b/persistence/sql/migrations/sql/20240425095000000000_identity_credentials_fix_user_handle_index.up.sql new file mode 100644 index 000000000000..e69de29bb2d1 From 189f8b42cee1d9d8af623010cd5be604b958ff28 Mon Sep 17 00:00:00 2001 From: Henning Perl Date: Fri, 26 Apr 2024 09:10:17 +0200 Subject: [PATCH 3/4] fix: also offer passkey on duplicate cred err --- identity/manager.go | 18 ++++++++++++++++++ selfservice/strategy/oidc/strategy_test.go | 2 +- .../passkey/passkey_registration_test.go | 2 +- text/message_validation.go | 2 ++ 4 files changed, 22 insertions(+), 2 deletions(-) diff --git a/identity/manager.go b/identity/manager.go index ca8a7e70fc9a..04fb3edae500 100644 --- a/identity/manager.go +++ b/identity/manager.go @@ -243,6 +243,24 @@ func (m *Manager) findExistingAuthMethod(ctx context.Context, e error, i *Identi identifierHint = cred.Identifiers[0] } + for _, webauthn := range cfg.Credentials { + if webauthn.IsPasswordless { + duplicateCredErr.AddCredentialsType(cred.Type) + duplicateCredErr.SetIdentifierHint(identifierHint) + break + } + } + case CredentialsTypePasskey: + var cfg CredentialsWebAuthnConfig + if err := json.Unmarshal(cred.Config, &cfg); err != nil { + return errors.WithStack(herodot.ErrInternalServerError.WithReasonf("Unable to JSON decode identity credentials %s for identity %s.", cred.Type, found.ID)) + } + + identifierHint := foundConflictAddress + if len(cred.Identifiers) > 0 { + identifierHint = cred.Identifiers[0] + } + for _, webauthn := range cfg.Credentials { if webauthn.IsPasswordless { duplicateCredErr.AddCredentialsType(cred.Type) diff --git a/selfservice/strategy/oidc/strategy_test.go b/selfservice/strategy/oidc/strategy_test.go index 0b6d068b21fc..b02e4fc45853 100644 --- a/selfservice/strategy/oidc/strategy_test.go +++ b/selfservice/strategy/oidc/strategy_test.go @@ -1285,7 +1285,7 @@ func TestStrategy(t *testing.T) { var linkingLoginFlow struct{ ID string } t.Run("step=should fail login and start a new login", func(t *testing.T) { res, body := loginWithOIDC(t, client, loginFlow.ID, "valid") - assertUIError(t, res, body, "You tried signing in with existing-oidc-identity-1@ory.sh which is already in use by another account. You can sign in using social sign in, or your password. You can sign in using one of the following social sign in providers: Secondprovider.") + assertUIError(t, res, body, "You tried signing in with existing-oidc-identity-1@ory.sh which is already in use by another account. You can sign in using social sign in. You can sign in using one of the following social sign in providers: Secondprovider.") linkingLoginFlow.ID = gjson.GetBytes(body, "id").String() assert.NotEqual(t, loginFlow.ID.String(), linkingLoginFlow.ID, "should have started a new flow") }) diff --git a/selfservice/strategy/passkey/passkey_registration_test.go b/selfservice/strategy/passkey/passkey_registration_test.go index 6b071cd9dc82..d495e8c4dfe4 100644 --- a/selfservice/strategy/passkey/passkey_registration_test.go +++ b/selfservice/strategy/passkey/passkey_registration_test.go @@ -372,7 +372,7 @@ func TestRegistration(t *testing.T) { assert.Contains(t, gjson.Get(actual, "ui.action").String(), fix.publicTS.URL+registration.RouteSubmitFlow, "%s", actual) registrationhelpers.CheckFormContent(t, []byte(actual), "csrf_token", "traits.username") assert.Equal(t, - "You tried signing in with "+email+" which is already in use by another account.", + "You tried signing in with "+email+" which is already in use by another account. You can sign in using your passkey.", gjson.Get(actual, "ui.messages.0.text").String(), "%s", actual) }) } diff --git a/text/message_validation.go b/text/message_validation.go index b3dff02c3234..c10fddead805 100644 --- a/text/message_validation.go +++ b/text/message_validation.go @@ -286,6 +286,8 @@ func NewErrorValidationDuplicateCredentialsWithHints(availableCredentialTypes [] humanReadable = append(humanReadable, "social sign in") case "webauthn": humanReadable = append(humanReadable, "your passkey or a security key") + case "passkey": + humanReadable = append(humanReadable, "your passkey") } } if len(humanReadable) == 0 { From d335e71b65ca16a3cc84068797cd01fc0a26f9e6 Mon Sep 17 00:00:00 2001 From: Henning Perl Date: Fri, 26 Apr 2024 10:26:57 +0200 Subject: [PATCH 4/4] fix: split migration --- internal/client-go/go.sum | 1 + ...ity_credentials_fix_user_handle_index.cockroach.down.sql | 6 +----- ...ntity_credentials_fix_user_handle_index.cockroach.up.sql | 2 -- ...ity_credentials_fix_user_handle_index.cockroach.down.sql | 3 +++ ...ntity_credentials_fix_user_handle_index.cockroach.up.sql | 1 + ...0001_identity_credentials_fix_user_handle_index.down.sql | 0 ...000001_identity_credentials_fix_user_handle_index.up.sql | 0 7 files changed, 6 insertions(+), 7 deletions(-) create mode 100644 persistence/sql/migrations/sql/20240425095000000001_identity_credentials_fix_user_handle_index.cockroach.down.sql create mode 100644 persistence/sql/migrations/sql/20240425095000000001_identity_credentials_fix_user_handle_index.cockroach.up.sql create mode 100644 persistence/sql/migrations/sql/20240425095000000001_identity_credentials_fix_user_handle_index.down.sql create mode 100644 persistence/sql/migrations/sql/20240425095000000001_identity_credentials_fix_user_handle_index.up.sql diff --git a/internal/client-go/go.sum b/internal/client-go/go.sum index c966c8ddfd0d..6cc3f5911d11 100644 --- a/internal/client-go/go.sum +++ b/internal/client-go/go.sum @@ -4,6 +4,7 @@ github.com/golang/protobuf v1.2.0/go.mod h1:6lQm79b+lXiMfvg/cZm0SGofjICqVBUtrP5y golang.org/x/net v0.0.0-20180724234803-3673e40ba225/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4= golang.org/x/net v0.0.0-20190108225652-1e06a53dbb7e h1:bRhVy7zSSasaqNksaRZiA5EEI+Ei4I1nO5Jh72wfHlg= golang.org/x/net v0.0.0-20190108225652-1e06a53dbb7e/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4= +golang.org/x/oauth2 v0.0.0-20190604053449-0f29369cfe45/go.mod h1:gOpvHmFTYa4IltrdGE7lF6nIHvwfUNPOp7c8zoXwtLw= golang.org/x/sync v0.0.0-20181221193216-37e7f081c4d4 h1:YUO/7uOKsKeq9UokNS62b8FYywz3ker1l1vDZRCRefw= golang.org/x/sync v0.0.0-20181221193216-37e7f081c4d4/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ= diff --git a/persistence/sql/migrations/sql/20240425095000000000_identity_credentials_fix_user_handle_index.cockroach.down.sql b/persistence/sql/migrations/sql/20240425095000000000_identity_credentials_fix_user_handle_index.cockroach.down.sql index d61a10ca9f98..dd8f3d45ff55 100644 --- a/persistence/sql/migrations/sql/20240425095000000000_identity_credentials_fix_user_handle_index.cockroach.down.sql +++ b/persistence/sql/migrations/sql/20240425095000000000_identity_credentials_fix_user_handle_index.cockroach.down.sql @@ -1,5 +1 @@ -DROP INDEX identity_credentials_config_user_handle_idx; - -CREATE INVERTED INDEX identity_credentials_user_handle_idx - ON identity_credentials (config) - WHERE config ->> 'user_handle' IS NOT NULL; \ No newline at end of file +DROP INDEX identity_credentials_config_user_handle_idx; \ No newline at end of file diff --git a/persistence/sql/migrations/sql/20240425095000000000_identity_credentials_fix_user_handle_index.cockroach.up.sql b/persistence/sql/migrations/sql/20240425095000000000_identity_credentials_fix_user_handle_index.cockroach.up.sql index 27e938a4519b..a953dd44b8b1 100644 --- a/persistence/sql/migrations/sql/20240425095000000000_identity_credentials_fix_user_handle_index.cockroach.up.sql +++ b/persistence/sql/migrations/sql/20240425095000000000_identity_credentials_fix_user_handle_index.cockroach.up.sql @@ -1,5 +1,3 @@ -DROP INDEX identity_credentials_user_handle_idx; - CREATE INDEX identity_credentials_config_user_handle_idx ON identity_credentials ((config ->> 'user_handle')) WHERE config ->> 'user_handle' IS NOT NULL diff --git a/persistence/sql/migrations/sql/20240425095000000001_identity_credentials_fix_user_handle_index.cockroach.down.sql b/persistence/sql/migrations/sql/20240425095000000001_identity_credentials_fix_user_handle_index.cockroach.down.sql new file mode 100644 index 000000000000..14c295e29c5d --- /dev/null +++ b/persistence/sql/migrations/sql/20240425095000000001_identity_credentials_fix_user_handle_index.cockroach.down.sql @@ -0,0 +1,3 @@ +CREATE INVERTED INDEX identity_credentials_user_handle_idx + ON identity_credentials (config) + WHERE config ->> 'user_handle' IS NOT NULL; \ No newline at end of file diff --git a/persistence/sql/migrations/sql/20240425095000000001_identity_credentials_fix_user_handle_index.cockroach.up.sql b/persistence/sql/migrations/sql/20240425095000000001_identity_credentials_fix_user_handle_index.cockroach.up.sql new file mode 100644 index 000000000000..91e0c2a6c2c2 --- /dev/null +++ b/persistence/sql/migrations/sql/20240425095000000001_identity_credentials_fix_user_handle_index.cockroach.up.sql @@ -0,0 +1 @@ +DROP INDEX identity_credentials_user_handle_idx; diff --git a/persistence/sql/migrations/sql/20240425095000000001_identity_credentials_fix_user_handle_index.down.sql b/persistence/sql/migrations/sql/20240425095000000001_identity_credentials_fix_user_handle_index.down.sql new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/persistence/sql/migrations/sql/20240425095000000001_identity_credentials_fix_user_handle_index.up.sql b/persistence/sql/migrations/sql/20240425095000000001_identity_credentials_fix_user_handle_index.up.sql new file mode 100644 index 000000000000..e69de29bb2d1