Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: improve OIDC account linking UI #4036

Merged
merged 11 commits into from
Aug 30, 2024
4 changes: 2 additions & 2 deletions cmd/clidoc/main.go
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
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
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
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
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
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
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
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
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
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
Loading