From 4a870a678dd3676abda7afc9803399dec4411b05 Mon Sep 17 00:00:00 2001 From: Henning Perl Date: Thu, 7 Nov 2024 09:59:46 +0100 Subject: [PATCH] fix: account linking with 2FA (#4188) This fixes some edge cases with OIDC account linking for accounts with 2FA enabled. --- internal/client-go/go.sum | 1 + selfservice/flow/duplicate_credentials.go | 2 - selfservice/flow/login/flow_test.go | 7 +-- selfservice/flow/login/handler.go | 20 ++++++- selfservice/flow/login/hook.go | 55 +++++++++++++++++-- selfservice/flow/login/hook_test.go | 48 +++++++++++++--- selfservice/flow/login/strategy.go | 3 + selfservice/flow/registration/hook.go | 17 +++--- selfservice/strategy/oidc/strategy_login.go | 1 + .../strategy/oidc/strategy_settings.go | 47 +++++++++++++++- selfservice/strategy/oidc/strategy_test.go | 5 +- 11 files changed, 170 insertions(+), 36 deletions(-) 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/selfservice/flow/duplicate_credentials.go b/selfservice/flow/duplicate_credentials.go index 8e263240cedb..ddba57251ed2 100644 --- a/selfservice/flow/duplicate_credentials.go +++ b/selfservice/flow/duplicate_credentials.go @@ -6,7 +6,6 @@ package flow import ( "encoding/json" - "github.com/gofrs/uuid" "github.com/tidwall/gjson" "github.com/tidwall/sjson" @@ -20,7 +19,6 @@ type DuplicateCredentialsData struct { CredentialsType identity.CredentialsType CredentialsConfig sqlxx.JSONRawMessage DuplicateIdentifier string - OrganizationID uuid.UUID } type InternalContexter interface { diff --git a/selfservice/flow/login/flow_test.go b/selfservice/flow/login/flow_test.go index 1ac990200479..1df47e132974 100644 --- a/selfservice/flow/login/flow_test.go +++ b/selfservice/flow/login/flow_test.go @@ -16,12 +16,10 @@ import ( "github.com/tidwall/gjson" - "github.com/ory/x/jsonx" - "github.com/ory/x/sqlxx" - "github.com/ory/x/uuidx" - "github.com/ory/kratos/driver/config" "github.com/ory/kratos/identity" + "github.com/ory/x/jsonx" + "github.com/ory/x/sqlxx" "github.com/ory/kratos/internal" @@ -225,7 +223,6 @@ func TestDuplicateCredentials(t *testing.T) { CredentialsType: "foo", CredentialsConfig: sqlxx.JSONRawMessage(`{"bar":"baz"}`), DuplicateIdentifier: "bar", - OrganizationID: uuidx.NewV4(), } require.NoError(t, flow.SetDuplicateCredentials(f, dc)) diff --git a/selfservice/flow/login/handler.go b/selfservice/flow/login/handler.go index d86641470ef0..267b33217216 100644 --- a/selfservice/flow/login/handler.go +++ b/selfservice/flow/login/handler.go @@ -104,6 +104,18 @@ func WithFlowReturnTo(returnTo string) FlowOption { } } +func WithOrganizationID(organizationID uuid.NullUUID) FlowOption { + return func(f *Flow) { + f.OrganizationID = organizationID + } +} + +func WithRequestedAAL(aal identity.AuthenticatorAssuranceLevel) FlowOption { + return func(f *Flow) { + f.RequestedAAL = aal + } +} + func WithInternalContext(internalContext []byte) FlowOption { return func(f *Flow) { f.InternalContext = internalContext @@ -217,7 +229,13 @@ preLoginHook: if orgID.Valid { f.OrganizationID = orgID - strategyFilters = []StrategyFilter{func(s Strategy) bool { return s.ID() == identity.CredentialsTypeOIDC }} + if f.RequestedAAL == identity.AuthenticatorAssuranceLevel1 { + // We only apply the filter on AAL1, because the OIDC strategy can only satsify + // AAL1. + strategyFilters = []StrategyFilter{func(s Strategy) bool { + return s.ID() == identity.CredentialsTypeOIDC + }} + } } for _, s := range h.d.LoginStrategies(r.Context(), strategyFilters...) { diff --git a/selfservice/flow/login/hook.go b/selfservice/flow/login/hook.go index 402aacef665b..5d4cb270eb41 100644 --- a/selfservice/flow/login/hook.go +++ b/selfservice/flow/login/hook.go @@ -7,6 +7,7 @@ import ( "context" "fmt" "net/http" + "net/url" "time" "github.com/gofrs/uuid" @@ -55,6 +56,7 @@ type ( x.LoggingProvider x.TracingProvider sessiontokenexchange.PersistenceProvider + HandlerProvider FlowPersistenceProvider HooksProvider @@ -273,8 +275,28 @@ func (e *HookExecutor) PostLoginHook( // If we detect that whoami would require a higher AAL, we redirect! if err := e.checkAAL(ctx, classified, f); err != nil { if aalErr := new(session.ErrAALNotSatisfied); errors.As(err, &aalErr) { - span.SetAttributes(attribute.String("return_to", aalErr.RedirectTo), attribute.String("redirect_reason", "requires aal2")) - e.d.Writer().WriteError(w, r, flow.NewBrowserLocationChangeRequiredError(aalErr.RedirectTo)) + if data, _ := flow.DuplicateCredentials(f); data == nil { + span.SetAttributes(attribute.String("return_to", aalErr.RedirectTo), attribute.String("redirect_reason", "requires aal2")) + e.d.Writer().WriteError(w, r, flow.NewBrowserLocationChangeRequiredError(aalErr.RedirectTo)) + return nil + } + + // Special case: If we are in a flow that wants to link credentials, we create a + // new login flow here that asks for the require AAL, but also copies over the + // internal context and the organization ID. + r.URL, err = url.Parse(aalErr.RedirectTo) + if err != nil { + return errors.WithStack(err) + } + newFlow, _, err := e.d.LoginHandler().NewLoginFlow(w, r, flow.TypeBrowser, + WithInternalContext(f.InternalContext), + WithOrganizationID(f.OrganizationID), + ) + if err != nil { + return errors.WithStack(err) + } + + x.AcceptToRedirectOrJSON(w, r, e.d.Writer(), newFlow, newFlow.AppendTo(e.d.Config().SelfServiceFlowLoginUI(ctx)).String()) return nil } return err @@ -309,7 +331,27 @@ func (e *HookExecutor) PostLoginHook( // If we detect that whoami would require a higher AAL, we redirect! if err := e.checkAAL(ctx, classified, f); err != nil { if aalErr := new(session.ErrAALNotSatisfied); errors.As(err, &aalErr) { - http.Redirect(w, r, aalErr.RedirectTo, http.StatusSeeOther) + if data, _ := flow.DuplicateCredentials(f); data == nil { + http.Redirect(w, r, aalErr.RedirectTo, http.StatusSeeOther) + return nil + } + + // Special case: If we are in a flow that wants to link credentials, we create a + // new login flow here that asks for the require AAL, but also copies over the + // internal context and the organization ID. + r.URL, err = url.Parse(aalErr.RedirectTo) + if err != nil { + return errors.WithStack(err) + } + newFlow, _, err := e.d.LoginHandler().NewLoginFlow(w, r, flow.TypeBrowser, + WithInternalContext(f.InternalContext), + WithOrganizationID(f.OrganizationID), + ) + if err != nil { + return errors.WithStack(err) + } + + x.AcceptToRedirectOrJSON(w, r, e.d.Writer(), newFlow, newFlow.AppendTo(e.d.Config().SelfServiceFlowLoginUI(ctx)).String()) return nil } return errors.WithStack(err) @@ -362,7 +404,7 @@ func (e *HookExecutor) maybeLinkCredentials(ctx context.Context, sess *session.S return nil } - if err := e.checkDuplicateCredentialsIdentifierMatch(ctx, ident.ID, lc.DuplicateIdentifier); err != nil { + if err = e.checkDuplicateCredentialsIdentifierMatch(ctx, ident.ID, lc.DuplicateIdentifier); err != nil { return err } strategy, err := e.d.AllLoginStrategies().Strategy(lc.CredentialsType) @@ -380,8 +422,9 @@ func (e *HookExecutor) maybeLinkCredentials(ctx context.Context, sess *session.S return err } - method := strategy.CompletedAuthenticationMethod(ctx) - sess.CompletedLoginForMethod(method) + if err = linkableStrategy.CompletedLogin(sess, lc); err != nil { + return err + } return nil } diff --git a/selfservice/flow/login/hook_test.go b/selfservice/flow/login/hook_test.go index 160373a47955..c75118f369e6 100644 --- a/selfservice/flow/login/hook_test.go +++ b/selfservice/flow/login/hook_test.go @@ -12,6 +12,7 @@ import ( "testing" "time" + "github.com/gofrs/uuid" "github.com/julienschmidt/httprouter" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -27,6 +28,7 @@ import ( "github.com/ory/kratos/selfservice/flow" "github.com/ory/kratos/selfservice/flow/login" "github.com/ory/kratos/session" + "github.com/ory/kratos/ui/node" "github.com/ory/kratos/x" ) @@ -42,6 +44,7 @@ func TestLoginExecutor(t *testing.T) { reg.WithHydra(hydra.NewFake()) testhelpers.SetDefaultIdentitySchema(conf, "file://./stub/login.schema.json") conf.MustSet(ctx, config.ViperKeySelfServiceBrowserDefaultReturnTo, "https://www.ory.sh/") + _ = testhelpers.NewLoginUIFlowEchoServer(t, reg) newServer := func(t *testing.T, ft flow.Type, useIdentity *identity.Identity, flowCallback ...func(*login.Flow)) *httptest.Server { router := httprouter.New() @@ -222,7 +225,6 @@ func TestLoginExecutor(t *testing.T) { t.Run("case=work normally if AAL is satisfied", func(t *testing.T) { conf.MustSet(ctx, config.ViperKeySessionWhoAmIAAL, "aal1") - _ = testhelpers.NewLoginUIFlowEchoServer(t, reg) t.Cleanup(testhelpers.SelfServiceHookConfigReset(t, conf)) useIdentity := &identity.Identity{Credentials: map[identity.CredentialsType]identity.Credentials{ @@ -255,7 +257,6 @@ func TestLoginExecutor(t *testing.T) { t.Run("case=redirect to login if AAL is too low", func(t *testing.T) { conf.MustSet(ctx, config.ViperKeySessionWhoAmIAAL, "highest_available") - _ = testhelpers.NewLoginUIFlowEchoServer(t, reg) t.Cleanup(func() { conf.MustSet(ctx, config.ViperKeySessionWhoAmIAAL, "aal1") }) @@ -320,6 +321,7 @@ func TestLoginExecutor(t *testing.T) { t.Run("case=maybe links credential", func(t *testing.T) { t.Cleanup(testhelpers.SelfServiceHookConfigReset(t, conf)) conf.MustSet(ctx, config.ViperKeySessionWhoAmIAAL, config.HighestAvailableAAL) + conf.MustSet(ctx, "selfservice.methods.totp.enabled", true) email1, email2 := testhelpers.RandomEmail(), testhelpers.RandomEmail() passwordOnlyIdentity := &identity.Identity{Credentials: map[identity.CredentialsType]identity.Credentials{ @@ -360,15 +362,43 @@ func TestLoginExecutor(t *testing.T) { require.NoError(t, err) t.Run("sub-case=does not link after first factor when second factor is available", func(t *testing.T) { + duplicateCredentialsData := flow.DuplicateCredentialsData{ + CredentialsType: identity.CredentialsTypeOIDC, + CredentialsConfig: credsOIDC2FA.Config, + DuplicateIdentifier: email2, + } ts := newServer(t, flow.TypeBrowser, twoFAIdentitiy, func(l *login.Flow) { - require.NoError(t, flow.SetDuplicateCredentials(l, flow.DuplicateCredentialsData{ - CredentialsType: identity.CredentialsTypeOIDC, - CredentialsConfig: credsOIDC2FA.Config, - DuplicateIdentifier: email2, - })) + require.NoError(t, flow.SetDuplicateCredentials(l, duplicateCredentialsData)) }) - res, body := makeRequestPost(t, ts, false, url.Values{}) - assert.Equal(t, res.Request.URL.String(), ts.URL+login.RouteInitBrowserFlow+"?aal=aal2", "%s", body) + res, _ := makeRequestPost(t, ts, false, url.Values{}) + + assert.Equal(t, reg.Config().SelfServiceFlowLoginUI(ctx).Host, res.Request.URL.Host) + assert.Equal(t, reg.Config().SelfServiceFlowLoginUI(ctx).Path, res.Request.URL.Path) + newFlowID := res.Request.URL.Query().Get("flow") + assert.NotEmpty(t, newFlowID) + + newFlow, err := reg.LoginFlowPersister().GetLoginFlow(ctx, uuid.Must(uuid.FromString(newFlowID))) + require.NoError(t, err) + newFlowDuplicateCredentialsData, err := flow.DuplicateCredentials(newFlow) + require.NoError(t, err) + + // Duplicate credentials data should have been copied over + assert.Equal(t, duplicateCredentialsData.CredentialsType, newFlowDuplicateCredentialsData.CredentialsType) + assert.Equal(t, duplicateCredentialsData.DuplicateIdentifier, newFlowDuplicateCredentialsData.DuplicateIdentifier) + assert.JSONEq(t, string(duplicateCredentialsData.CredentialsConfig), string(newFlowDuplicateCredentialsData.CredentialsConfig)) + + // AAL should be AAL2 + assert.Equal(t, identity.AuthenticatorAssuranceLevel2, newFlow.RequestedAAL) + + // TOTP nodes should be present + found := false + for _, n := range newFlow.UI.Nodes { + if n.Group == node.TOTPGroup { + found = true + break + } + } + assert.True(t, found, "could not find TOTP nodes in %+v", newFlow.UI.Nodes) ident, err := reg.Persister().GetIdentity(ctx, twoFAIdentitiy.ID, identity.ExpandCredentials) require.NoError(t, err) diff --git a/selfservice/flow/login/strategy.go b/selfservice/flow/login/strategy.go index 8ea671343c76..2291b760079b 100644 --- a/selfservice/flow/login/strategy.go +++ b/selfservice/flow/login/strategy.go @@ -10,6 +10,7 @@ import ( "github.com/pkg/errors" "github.com/ory/kratos/identity" + "github.com/ory/kratos/selfservice/flow" "github.com/ory/kratos/session" "github.com/ory/kratos/ui/node" "github.com/ory/kratos/x" @@ -28,6 +29,8 @@ type Strategies []Strategy type LinkableStrategy interface { Link(ctx context.Context, i *identity.Identity, credentials sqlxx.JSONRawMessage) error + CompletedLogin(sess *session.Session, data *flow.DuplicateCredentialsData) error + SetDuplicateCredentials(f flow.InternalContexter, duplicateIdentifier string, credentials identity.Credentials, provider string) error } func (s Strategies) Strategy(id identity.CredentialsType) (Strategy, error) { diff --git a/selfservice/flow/registration/hook.go b/selfservice/flow/registration/hook.go index f2356d2c8277..33379368c265 100644 --- a/selfservice/flow/registration/hook.go +++ b/selfservice/flow/registration/hook.go @@ -161,21 +161,18 @@ func (e *HookExecutor) PostRegistrationHook(w http.ResponseWriter, r *http.Reque return err } - if _, ok := strategy.(login.LinkableStrategy); ok { + if strategy, ok := strategy.(login.LinkableStrategy); ok { duplicateIdentifier, err := e.getDuplicateIdentifier(ctx, i) if err != nil { return err } - registrationDuplicateCredentials := flow.DuplicateCredentialsData{ - CredentialsType: ct, - CredentialsConfig: i.Credentials[ct].Config, - DuplicateIdentifier: duplicateIdentifier, - } - if registrationFlow.OrganizationID.Valid { - registrationDuplicateCredentials.OrganizationID = registrationFlow.OrganizationID.UUID - } - if err := flow.SetDuplicateCredentials(registrationFlow, registrationDuplicateCredentials); err != nil { + if err := strategy.SetDuplicateCredentials( + registrationFlow, + duplicateIdentifier, + i.Credentials[ct], + provider, + ); err != nil { return err } } diff --git a/selfservice/strategy/oidc/strategy_login.go b/selfservice/strategy/oidc/strategy_login.go index 4805919f80d1..07be40194d40 100644 --- a/selfservice/strategy/oidc/strategy_login.go +++ b/selfservice/strategy/oidc/strategy_login.go @@ -169,6 +169,7 @@ func (s *Strategy) processLogin(ctx context.Context, w http.ResponseWriter, r *h sess := session.NewInactiveSession() sess.CompletedLoginForWithProvider(s.ID(), identity.AuthenticatorAssuranceLevel1, provider.Config().ID, provider.Config().OrganizationID) + for _, c := range oidcCredentials.Providers { if c.Subject == claims.Subject && c.Provider == provider.Config().ID { if err = s.d.LoginHookExecutor().PostLoginHook(w, r, node.OpenIDConnectGroup, loginFlow, i, sess, provider.Config().ID); err != nil { diff --git a/selfservice/strategy/oidc/strategy_settings.go b/selfservice/strategy/oidc/strategy_settings.go index 9a97a54e5521..4c721225c134 100644 --- a/selfservice/strategy/oidc/strategy_settings.go +++ b/selfservice/strategy/oidc/strategy_settings.go @@ -8,6 +8,7 @@ import ( "context" _ "embed" "encoding/json" + "fmt" "net/http" "time" @@ -527,7 +528,7 @@ func (s *Strategy) Link(ctx context.Context, i *identity.Identity, credentialsCo return err } if len(credentialsOIDCConfig.Providers) != 1 { - return errors.New("No oidc provider was set") + return errors.New("no oidc provider was set") } credentialsOIDCProvider := credentialsOIDCConfig.Providers[0] @@ -550,3 +551,47 @@ func (s *Strategy) Link(ctx context.Context, i *identity.Identity, credentialsCo return nil } + +func (s *Strategy) CompletedLogin(sess *session.Session, data *flow.DuplicateCredentialsData) error { + var credentialsOIDCConfig identity.CredentialsOIDC + if err := json.Unmarshal(data.CredentialsConfig, &credentialsOIDCConfig); err != nil { + return err + } + if len(credentialsOIDCConfig.Providers) != 1 { + return errors.New("no oidc provider was set") + } + credentialsOIDCProvider := credentialsOIDCConfig.Providers[0] + + sess.CompletedLoginForWithProvider( + s.ID(), + identity.AuthenticatorAssuranceLevel1, + credentialsOIDCProvider.Provider, + credentialsOIDCProvider.Organization, + ) + + return nil +} + +func (s *Strategy) SetDuplicateCredentials(f flow.InternalContexter, duplicateIdentifier string, credentials identity.Credentials, provider string) error { + var credentialsOIDCConfig identity.CredentialsOIDC + if err := json.Unmarshal(credentials.Config, &credentialsOIDCConfig); err != nil { + return err + } + + // We want to only set the provider in the credentials config that was used to authenticate the user. + for _, p := range credentialsOIDCConfig.Providers { + if p.Provider == provider { + credentialsOIDCConfig.Providers = []identity.CredentialsOIDCProvider{p} + config, err := json.Marshal(credentialsOIDCConfig) + if err != nil { + return err + } + return flow.SetDuplicateCredentials(f, flow.DuplicateCredentialsData{ + CredentialsType: s.ID(), + CredentialsConfig: config, + DuplicateIdentifier: duplicateIdentifier, + }) + } + } + return fmt.Errorf("provider %q not found in credentials", provider) +} diff --git a/selfservice/strategy/oidc/strategy_test.go b/selfservice/strategy/oidc/strategy_test.go index 4bb7bec9bf2f..dde3e1bda607 100644 --- a/selfservice/strategy/oidc/strategy_test.go +++ b/selfservice/strategy/oidc/strategy_test.go @@ -82,6 +82,8 @@ func TestStrategy(t *testing.T) { routerA := x.NewRouterAdmin() ts, _ := testhelpers.NewKratosServerWithRouters(t, reg, routerP, routerA) invalid := newOIDCProvider(t, ts, remotePublic, remoteAdmin, "invalid-issuer") + + orgID := uuidx.NewV4() viperSetProviderConfig( t, conf, @@ -1532,7 +1534,6 @@ func TestStrategy(t *testing.T) { subject2 := "new-login-subject2@ory.sh" scope = []string{"openid"} password := "lwkj52sdkjf" - orgID := uuidx.NewV4() var i *identity.Identity t.Run("step=create password identity", func(t *testing.T) { @@ -1557,7 +1558,7 @@ func TestStrategy(t *testing.T) { client := testhelpers.NewClientWithCookieJar(t, nil, nil) loginFlow := newLoginFlow(t, returnTS.URL, time.Minute, flow.TypeBrowser) - loginFlow.OrganizationID = uuid.NullUUID{orgID, true} + loginFlow.OrganizationID = uuid.NullUUID{UUID: orgID, Valid: true} require.NoError(t, reg.LoginFlowPersister().UpdateLoginFlow(context.Background(), loginFlow)) var linkingLoginFlow struct {