Skip to content

Commit

Permalink
fix: improve OIDC account linking UI (#4036)
Browse files Browse the repository at this point in the history
  • Loading branch information
@jonas-jonas
jonas-jonas authored Aug 30, 2024
1 parent dbe9d10 commit 2b4a618
Show file tree
Hide file tree
Showing 23 changed files with 556 additions and 106 deletions.
4 changes: 2 additions & 2 deletions cmd/clidoc/main.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ func init() {
"NewInfoLoginTOTP": text.NewInfoLoginTOTP(),
"NewInfoLoginLookup": text.NewInfoLoginLookup(),
"NewInfoLoginVerify": text.NewInfoLoginVerify(),
"NewInfoLoginWith": text.NewInfoLoginWith("{provider}"),
"NewInfoLoginWith": text.NewInfoLoginWith("{provider}", "{providerID}"),
"NewInfoLoginWithAndLink": text.NewInfoLoginWithAndLink("{provider}"),
"NewErrorValidationLoginFlowExpired": text.NewErrorValidationLoginFlowExpired(aSecondAgo),
"NewErrorValidationLoginNoStrategyFound": text.NewErrorValidationLoginNoStrategyFound(),
Expand All @@ -135,7 +135,7 @@ func init() {
"NewErrorValidationVerificationNoStrategyFound": text.NewErrorValidationVerificationNoStrategyFound(),
"NewInfoSelfServiceLoginWebAuthn": text.NewInfoSelfServiceLoginWebAuthn(),
"NewInfoRegistration": text.NewInfoRegistration(),
"NewInfoRegistrationWith": text.NewInfoRegistrationWith("{provider}"),
"NewInfoRegistrationWith": text.NewInfoRegistrationWith("{provider}", "{providerID}"),
"NewInfoRegistrationContinue": text.NewInfoRegistrationContinue(),
"NewInfoRegistrationBack": text.NewInfoRegistrationBack(),
"NewInfoSelfServiceChooseCredentials": text.NewInfoSelfServiceChooseCredentials(),
Expand Down
10 changes: 10 additions & 0 deletions identity/manager.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,15 @@ func (m *Manager) findExistingAuthMethod(ctx context.Context, e error, i *Identi
}

duplicateCredErr.AddCredentialsType(cred.Type)

case CredentialsTypeCodeAuth:
identifierHint := foundConflictAddress
if len(cred.Identifiers) > 0 {
identifierHint = cred.Identifiers[0]
}

duplicateCredErr.SetIdentifierHint(identifierHint)
duplicateCredErr.AddCredentialsType(cred.Type)
case CredentialsTypeOIDC:
var cfg CredentialsOIDC
if err := json.Unmarshal(cred.Config, &cfg); err != nil {
Expand Down Expand Up @@ -312,6 +321,7 @@ func (e *ErrDuplicateCredentials) AvailableOIDCProviders() []string {
func (e *ErrDuplicateCredentials) IdentifierHint() string {
return e.identifierHint
}

func (e *ErrDuplicateCredentials) HasHints() bool {
return len(e.availableCredentials) > 0 || len(e.availableOIDCProviders) > 0 || len(e.identifierHint) > 0
}
Expand Down
36 changes: 30 additions & 6 deletions identity/manager_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -229,7 +229,7 @@ func TestManager(t *testing.T) {
err := reg.IdentityManager().Create(ctx, second)
require.Error(t, err)

var verr = new(identity.ErrDuplicateCredentials)
verr := new(identity.ErrDuplicateCredentials)
assert.ErrorAs(t, err, &verr)
assert.EqualValues(t, []string{identity.CredentialsTypePassword.String()}, verr.AvailableCredentials())
assert.Len(t, verr.AvailableOIDCProviders(), 0)
Expand All @@ -253,7 +253,7 @@ func TestManager(t *testing.T) {
err := reg.IdentityManager().Create(ctx, second)
require.Error(t, err)

var verr = new(identity.ErrDuplicateCredentials)
verr := new(identity.ErrDuplicateCredentials)
assert.ErrorAs(t, err, &verr)
assert.EqualValues(t, []string{identity.CredentialsTypeWebAuthn.String()}, verr.AvailableCredentials())
assert.Len(t, verr.AvailableOIDCProviders(), 0)
Expand All @@ -278,7 +278,7 @@ func TestManager(t *testing.T) {
err := reg.IdentityManager().Create(ctx, second)
require.Error(t, err)

var verr = new(identity.ErrDuplicateCredentials)
verr := new(identity.ErrDuplicateCredentials)
assert.ErrorAs(t, err, &verr)
assert.ElementsMatch(t, []string{"oidc"}, verr.AvailableCredentials())
assert.ElementsMatch(t, []string{"google", "github"}, verr.AvailableOIDCProviders())
Expand Down Expand Up @@ -313,12 +313,36 @@ func TestManager(t *testing.T) {
err := reg.IdentityManager().Create(ctx, second)
require.Error(t, err)

var verr = new(identity.ErrDuplicateCredentials)
verr := new(identity.ErrDuplicateCredentials)
assert.ErrorAs(t, err, &verr)
assert.ElementsMatch(t, []string{"password", "oidc", "webauthn"}, verr.AvailableCredentials())
assert.ElementsMatch(t, []string{"google", "github"}, verr.AvailableOIDCProviders())
assert.Equal(t, email, verr.IdentifierHint())
})

t.Run("type=code", func(t *testing.T) {
email := uuid.Must(uuid.NewV4()).String() + "@ory.sh"
creds := map[identity.CredentialsType]identity.Credentials{
identity.CredentialsTypeCodeAuth: {
Type: identity.CredentialsTypeCodeAuth,
Identifiers: []string{email},
Config: sqlxx.JSONRawMessage(`{}`),
},
}

first := createIdentity(email, "email_creds", creds)
require.NoError(t, reg.IdentityManager().Create(ctx, first))

second := createIdentity(email, "email_creds", creds)
err := reg.IdentityManager().Create(ctx, second)
require.Error(t, err)

verr := new(identity.ErrDuplicateCredentials)
assert.ErrorAs(t, err, &verr)
assert.EqualValues(t, []string{identity.CredentialsTypeCodeAuth.String()}, verr.AvailableCredentials())
assert.Len(t, verr.AvailableOIDCProviders(), 0)
assert.Equal(t, verr.IdentifierHint(), email)
})
})

runAddress := func(t *testing.T, field string) {
Expand All @@ -343,7 +367,7 @@ func TestManager(t *testing.T) {
err := reg.IdentityManager().Create(ctx, second)
require.Error(t, err)

var verr = new(identity.ErrDuplicateCredentials)
verr := new(identity.ErrDuplicateCredentials)
assert.ErrorAs(t, err, &verr)
assert.EqualValues(t, []string{identity.CredentialsTypePassword.String()}, verr.AvailableCredentials())
assert.Len(t, verr.AvailableOIDCProviders(), 0)
Expand Down Expand Up @@ -372,7 +396,7 @@ func TestManager(t *testing.T) {
err := reg.IdentityManager().Create(ctx, second)
require.Error(t, err)

var verr = new(identity.ErrDuplicateCredentials)
verr := new(identity.ErrDuplicateCredentials)
assert.ErrorAs(t, err, &verr)
assert.EqualValues(t, []string{identity.CredentialsTypeOIDC.String()}, verr.AvailableCredentials())
assert.EqualValues(t, verr.AvailableOIDCProviders(), []string{"github", "google"})
Expand Down
21 changes: 0 additions & 21 deletions internal/testhelpers/sdk.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import (
"net/http/httptest"
"net/url"

"github.com/ory/kratos/text"
"github.com/ory/kratos/ui/node"

"github.com/ory/kratos/x"
Expand Down Expand Up @@ -88,26 +87,6 @@ func NewSDKEmailNode(group string) *kratos.UiNode {
}
}

func NewSDKOIDCNode(name, provider string) *kratos.UiNode {
t := text.NewInfoRegistrationWith(provider)
return &kratos.UiNode{
Group: node.OpenIDConnectGroup.String(),
Type: "input",
Attributes: kratos.UiNodeInputAttributesAsUiNodeAttributes(&kratos.UiNodeInputAttributes{
Name: name,
Type: "submit",
Value: provider,
}),
Meta: kratos.UiNodeMeta{
Label: &kratos.UiText{
Id: int64(t.ID),
Text: t.Text,
Type: string(t.Type),
},
},
}
}

func NewMethodSubmit(group, value string) *kratos.UiNode {
return &kratos.UiNode{
Type: "input",
Expand Down
2 changes: 1 addition & 1 deletion selfservice/strategy/idfirst/strategy_login_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ func TestCompleteLogin(t *testing.T) {
})

t.Run("case=should return an error because the request is expired", func(t *testing.T) {
conf.MustSet(ctx, config.ViperKeySelfServiceLoginRequestLifespan, time.Millisecond*10)
conf.MustSet(ctx, config.ViperKeySelfServiceLoginRequestLifespan, time.Millisecond*30)
conf.MustSet(ctx, config.ViperKeySecurityAccountEnumerationMitigate, true)
t.Cleanup(func() {
conf.MustSet(ctx, config.ViperKeySelfServiceLoginRequestLifespan, time.Hour)
Expand Down
3 changes: 2 additions & 1 deletion ...ice/strategy/oidc/.snapshots/TestFormHydration-method=PopulateLoginMethodFirstFactor.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,8 @@
"text": "Sign in with test-provider",
"type": "info",
"context": {
"provider": "test-provider"
"provider": "test-provider",
"provider_id": "test-provider"
}
}
}
Expand Down
3 changes: 2 additions & 1 deletion ...ategy/oidc/.snapshots/TestFormHydration-method=PopulateLoginMethodFirstFactorRefresh.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,8 @@
"text": "Sign in with test-provider",
"type": "info",
"context": {
"provider": "test-provider"
"provider": "test-provider",
"provider_id": "test-provider"
}
}
}
Expand Down
3 changes: 2 additions & 1 deletion ...WithIdentityHint-case=account_enumeration_mitigation_disabled-case=identity_has_oidc.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,8 @@
"text": "Sign in with test-provider",
"type": "info",
"context": {
"provider": "test-provider"
"provider": "test-provider",
"provider_id": "test-provider"
}
}
}
Expand Down
3 changes: 2 additions & 1 deletion ....snapshots/TestFormHydration-method=PopulateLoginMethodIdentifierFirstIdentification.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,8 @@
"text": "Sign in with test-provider",
"type": "info",
"context": {
"provider": "test-provider"
"provider": "test-provider",
"provider_id": "test-provider"
}
}
}
Expand Down
185 changes: 185 additions & 0 deletions ...n_flow_if_email_is_already_being_used_by_password_credentials-case=should_fail_login.json
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,185 @@
{
"organization_id": null,
"type": "browser",
"active": "oidc",
"ui": {
"method": "POST",
"nodes": [
{
"type": "input",
"group": "oidc",
"attributes": {
"name": "provider",
"type": "submit",
"value": "claimsViaUserInfo",
"disabled": false,
"node_type": "input"
},
"messages": [],
"meta": {
"label": {
"id": 1010018,
"text": "Confirm with claimsViaUserInfo",
"type": "info",
"context": {
"provider": "claimsViaUserInfo"
}
}
}
},
{
"type": "input",
"group": "oidc",
"attributes": {
"name": "provider",
"type": "submit",
"value": "invalid-issuer",
"disabled": false,
"node_type": "input"
},
"messages": [],
"meta": {
"label": {
"id": 1010018,
"text": "Confirm with invalid-issuer",
"type": "info",
"context": {
"provider": "invalid-issuer"
}
}
}
},
{
"type": "input",
"group": "oidc",
"attributes": {
"name": "provider",
"type": "submit",
"value": "secondProvider",
"disabled": false,
"node_type": "input"
},
"messages": [],
"meta": {
"label": {
"id": 1010018,
"text": "Confirm with secondProvider",
"type": "info",
"context": {
"provider": "secondProvider"
}
}
}
},
{
"type": "input",
"group": "oidc",
"attributes": {
"name": "provider",
"type": "submit",
"value": "valid2",
"disabled": false,
"node_type": "input"
},
"messages": [],
"meta": {
"label": {
"id": 1010018,
"text": "Confirm with valid2",
"type": "info",
"context": {
"provider": "valid2"
}
}
}
},
{
"type": "input",
"group": "default",
"attributes": {
"name": "csrf_token",
"type": "hidden",
"required": true,
"disabled": false,
"node_type": "input"
},
"messages": [],
"meta": {}
},
{
"type": "input",
"group": "default",
"attributes": {
"name": "identifier",
"type": "hidden",
"value": "[email protected]",
"required": true,
"disabled": false,
"node_type": "input"
},
"messages": [],
"meta": {
"label": {
"id": 1070004,
"text": "ID",
"type": "info"
}
}
},
{
"type": "input",
"group": "password",
"attributes": {
"name": "password",
"type": "password",
"required": true,
"autocomplete": "current-password",
"disabled": false,
"node_type": "input"
},
"messages": [],
"meta": {
"label": {
"id": 1070001,
"text": "Password",
"type": "info"
}
}
},
{
"type": "input",
"group": "password",
"attributes": {
"name": "method",
"type": "submit",
"value": "password",
"disabled": false,
"node_type": "input"
},
"messages": [],
"meta": {
"label": {
"id": 1010022,
"text": "Sign in with password",
"type": "info"
}
}
}
],
"messages": [
{
"id": 1010016,
"text": "You tried to sign in with \"[email protected]\", but that email is already used by another account. Sign in to your account with one of the options below to add your account \"[email protected]\" at \"generic\" as another way to sign in.",
"type": "info",
"context": {
"duplicateIdentifier": "[email protected]",
"provider": "generic"
}
}
]
},
"refresh": false,
"requested_aal": "aal1",
"state": "choose_method"
}

Loading

0 comments on commit 2b4a618

Please sign in to comment.