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 @@ -218,6 +218,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 @@ -316,6 +325,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 @@ -192,7 +192,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 @@ -216,7 +216,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 @@ -241,7 +241,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 @@ -276,12 +276,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 @@ -306,7 +330,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 @@ -335,7 +359,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
Expand Up @@ -31,7 +31,31 @@
"text": "Sign up with valid",
"type": "info",
"context": {
"provider": "valid"
"provider": "valid",
"provider_id": "valid"
}
}
}
},
{
"type": "input",
"group": "oidc",
"attributes": {
"name": "provider",
"type": "submit",
"value": "valid2",
"disabled": false,
"node_type": "input"
},
"messages": [],
"meta": {
"label": {
"id": 1040002,
"text": "Sign up with valid2",
"type": "info",
"context": {
"provider": "valid2",
"provider_id": "valid2"
}
}
}
Expand All @@ -53,7 +77,8 @@
"text": "Sign up with secondProvider",
"type": "info",
"context": {
"provider": "secondProvider"
"provider": "secondProvider",
"provider_id": "secondProvider"
}
}
}
Expand All @@ -75,7 +100,8 @@
"text": "Sign up with claimsViaUserInfo",
"type": "info",
"context": {
"provider": "claimsViaUserInfo"
"provider": "claimsViaUserInfo",
"provider_id": "claimsViaUserInfo"
}
}
}
Expand All @@ -97,7 +123,8 @@
"text": "Sign up with invalid-issuer",
"type": "info",
"context": {
"provider": "invalid-issuer"
"provider": "invalid-issuer",
"provider_id": "invalid-issuer"
}
}
}
Expand Down
Loading
Loading