From be7259f304e322793ae311374fe5456a25c27736 Mon Sep 17 00:00:00 2001 From: splaunov Date: Mon, 10 Apr 2023 09:33:38 +0200 Subject: [PATCH 01/29] feat: link credentials when login --- cmd/clidoc/main.go | 1 + selfservice/flow/flow.go | 10 +++ selfservice/flow/login/handler.go | 69 +++++++++++++++++++ selfservice/flow/login/strategy.go | 5 ++ selfservice/flow/registration/flow.go | 32 +++++++++ selfservice/flow/registration/handler.go | 43 ++++++++++++ selfservice/flow/registration/hook.go | 36 ++++++++++ selfservice/strategy/oidc/strategy.go | 35 ++++++++++ selfservice/strategy/oidc/strategy_login.go | 2 + .../strategy/oidc/strategy_settings.go | 54 +++++++++------ selfservice/strategy/oidc/strategy_test.go | 68 +++++++++++++++++- text/id.go | 1 + text/message_login.go | 9 +++ 13 files changed, 342 insertions(+), 23 deletions(-) diff --git a/cmd/clidoc/main.go b/cmd/clidoc/main.go index 71e44d8fb785..8f4117adbf9f 100644 --- a/cmd/clidoc/main.go +++ b/cmd/clidoc/main.go @@ -131,6 +131,7 @@ func init() { "NewInfoSelfServiceContinueLoginWebAuthn": text.NewInfoSelfServiceContinueLoginWebAuthn(), "NewInfoSelfServiceLoginContinue": text.NewInfoSelfServiceLoginContinue(), "NewErrorValidationSuchNoWebAuthnUser": text.NewErrorValidationSuchNoWebAuthnUser(), + "NewInfoSelfServiceLoginLinkCredentials": text.NewInfoSelfServiceLoginLinkCredentials(), } } diff --git a/selfservice/flow/flow.go b/selfservice/flow/flow.go index 59ef84ce0649..ddce5db990c2 100644 --- a/selfservice/flow/flow.go +++ b/selfservice/flow/flow.go @@ -4,6 +4,8 @@ package flow import ( + "github.com/ory/kratos/identity" + "github.com/ory/x/sqlxx" "net/http" "net/url" @@ -19,6 +21,14 @@ import ( "github.com/ory/x/urlx" ) +const InternalContextDuplicateCredentialsPath = "registration_duplicate_credentials" +const InternalContextLinkCredentialsPath = "link_credentials" + +type RegistrationDuplicateCredentials struct { + CredentialsType identity.CredentialsType + CredentialsConfig sqlxx.JSONRawMessage +} + func AppendFlowTo(src *url.URL, id uuid.UUID) *url.URL { return urlx.CopyWithQuery(src, url.Values{"flow": {id.String()}}) } diff --git a/selfservice/flow/login/handler.go b/selfservice/flow/login/handler.go index 39086c26f1a3..13c377189884 100644 --- a/selfservice/flow/login/handler.go +++ b/selfservice/flow/login/handler.go @@ -4,6 +4,11 @@ package login import ( + "context" + "encoding/json" + "fmt" + "github.com/tidwall/gjson" + "github.com/tidwall/sjson" "net/http" "net/url" "time" @@ -694,6 +699,37 @@ continueLogin: return } + internalContextDuplicateCredentials := gjson.GetBytes(f.InternalContext, flow.InternalContextDuplicateCredentialsPath) + if internalContextDuplicateCredentials.IsObject() { + // If return_to was set before, we need to preserve it. + var opts []FlowOption + if len(f.ReturnTo) > 0 { + opts = append(opts, WithFlowReturnTo(f.ReturnTo)) + } + opts = append(opts, func(newFlow *Flow) { + newFlow.UI.Messages.Add(text.NewInfoSelfServiceLoginLinkCredentials()) + var linkCredentials flow.RegistrationDuplicateCredentials + if err := json.Unmarshal([]byte(internalContextDuplicateCredentials.Raw), &linkCredentials); err != nil { + h.d.LoginFlowErrorHandler().WriteFlowError(w, r, f, node.DefaultGroup, err) + return + } + newFlow.InternalContext, err = sjson.SetBytes(newFlow.InternalContext, flow.InternalContextLinkCredentialsPath, + linkCredentials) + if err != nil { + h.d.LoginFlowErrorHandler().WriteFlowError(w, r, f, node.DefaultGroup, err) + return + } + }) + loginFlow, _, err := h.NewLoginFlow(w, r, flow.TypeBrowser, opts...) + if err != nil { + h.d.LoginFlowErrorHandler().WriteFlowError(w, r, f, node.DefaultGroup, err) + return + } + + http.Redirect(w, r, loginFlow.AppendTo(h.d.Config().SelfServiceFlowLoginUI(r.Context())).String(), http.StatusSeeOther) + return + } + var i *identity.Identity var group node.UiNodeGroup for _, ss := range h.d.AllLoginStrategies() { @@ -714,6 +750,11 @@ continueLogin: sess = session.NewInactiveSession() } + if err := h.linkCredentials(r.Context(), sess, interim, f); err != nil { + h.d.LoginFlowErrorHandler().WriteFlowError(w, r, f, group, err) + return + } + method := ss.CompletedAuthenticationMethod(r.Context()) sess.CompletedLoginFor(method.Method, method.AAL) i = interim @@ -735,3 +776,31 @@ continueLogin: return } } + +func (h *Handler) linkCredentials(ctx context.Context, s *session.Session, i *identity.Identity, f *Flow) error { + internalContextLinkCredentials := gjson.GetBytes(f.InternalContext, flow.InternalContextLinkCredentialsPath) + if internalContextLinkCredentials.IsObject() { + var linkCredentials flow.RegistrationDuplicateCredentials + if err := json.Unmarshal([]byte(internalContextLinkCredentials.Raw), &linkCredentials); err != nil { + return err + } + strategy, err := h.d.AllLoginStrategies().Strategy(linkCredentials.CredentialsType) + if err != nil { + return err + } + + linkableStrategy, ok := strategy.(LinkableStrategy) + if !ok { + return errors.New(fmt.Sprintf("Strategy is not linkable: %T", linkableStrategy)) + } + + if err := linkableStrategy.Link(ctx, i, linkCredentials.CredentialsConfig); err != nil { + return err + } + + method := strategy.CompletedAuthenticationMethod(ctx) + s.CompletedLoginFor(method.Method, method.AAL) + } + + return nil +} diff --git a/selfservice/flow/login/strategy.go b/selfservice/flow/login/strategy.go index 017829eb5fa3..c54d46962f49 100644 --- a/selfservice/flow/login/strategy.go +++ b/selfservice/flow/login/strategy.go @@ -5,6 +5,7 @@ package login import ( "context" + "github.com/ory/x/sqlxx" "net/http" "github.com/ory/kratos/session" @@ -27,6 +28,10 @@ type Strategy interface { type Strategies []Strategy +type LinkableStrategy interface { + Link(ctx context.Context, i *identity.Identity, credentials sqlxx.JSONRawMessage) error +} + func (s Strategies) Strategy(id identity.CredentialsType) (Strategy, error) { ids := make([]identity.CredentialsType, len(s)) for k, ss := range s { diff --git a/selfservice/flow/registration/flow.go b/selfservice/flow/registration/flow.go index 33f1ea3ab6c5..9ce0b45d681b 100644 --- a/selfservice/flow/registration/flow.go +++ b/selfservice/flow/registration/flow.go @@ -6,6 +6,7 @@ package registration import ( "context" "encoding/json" + "github.com/tidwall/sjson" "net/http" "net/url" "time" @@ -203,3 +204,34 @@ func (f *Flow) AfterSave(*pop.Connection) error { func (f *Flow) GetUI() *container.Container { return f.UI } + +const internalContextOuterLoginFlowPath = "outer_flow" + +type OuterLoginFlow struct { + ID string +} + +func (f *Flow) GetOuterLoginFlowID() (*uuid.UUID, error) { + la := gjson.GetBytes(f.InternalContext, internalContextOuterLoginFlowPath) + if !la.IsObject() { + return nil, nil + } + var internalContextOuterFlow OuterLoginFlow + if err := json.Unmarshal([]byte(la.Raw), &internalContextOuterFlow); err != nil { + return nil, err + } + id, err := uuid.FromString(internalContextOuterFlow.ID) + if err != nil { + return nil, err + } + return &id, nil +} + +func (f *Flow) SetOuterLoginFlowID(flowID uuid.UUID) error { + var err error = nil + f.InternalContext, err = sjson.SetBytes(f.InternalContext, internalContextOuterLoginFlowPath, + OuterLoginFlow{ + ID: flowID.String(), + }) + return err +} diff --git a/selfservice/flow/registration/handler.go b/selfservice/flow/registration/handler.go index f1e4c0568d54..d911a957b2d3 100644 --- a/selfservice/flow/registration/handler.go +++ b/selfservice/flow/registration/handler.go @@ -4,6 +4,11 @@ package registration import ( + "encoding/json" + "github.com/gofrs/uuid" + "github.com/ory/kratos/selfservice/flow/login" + "github.com/tidwall/gjson" + "github.com/tidwall/sjson" "net/http" "net/url" "time" @@ -45,6 +50,7 @@ type ( config.Provider errorx.ManagementProvider hydra.HydraProvider + login.HandlerProvider session.HandlerProvider session.ManagementProvider x.WriterProvider @@ -106,6 +112,12 @@ func WithFlowReturnTo(returnTo string) FlowOption { } } +func WithOuterFlow(flowId uuid.UUID) FlowOption { + return func(f *Flow) { + f.SetOuterLoginFlowID(flowId) + } +} + func (h *Handler) NewRegistrationFlow(w http.ResponseWriter, r *http.Request, ft flow.Type, opts ...FlowOption) (*Flow, error) { if !h.d.Config().SelfServiceFlowRegistrationEnabled(r.Context()) { return nil, errors.WithStack(ErrRegistrationDisabled) @@ -522,6 +534,37 @@ func (h *Handler) updateRegistrationFlow(w http.ResponseWriter, r *http.Request, return } + internalContextDuplicateCredentials := gjson.GetBytes(f.InternalContext, flow.InternalContextDuplicateCredentialsPath) + if internalContextDuplicateCredentials.IsObject() { + // If return_to was set before, we need to preserve it. + var opts []login.FlowOption + if len(f.ReturnTo) > 0 { + opts = append(opts, login.WithFlowReturnTo(f.ReturnTo)) + } + opts = append(opts, func(newFlow *login.Flow) { + newFlow.UI.Messages.Add(text.NewInfoSelfServiceLoginLinkCredentials()) + var linkCredentials flow.RegistrationDuplicateCredentials + if err := json.Unmarshal([]byte(internalContextDuplicateCredentials.Raw), &linkCredentials); err != nil { + h.d.RegistrationFlowErrorHandler().WriteFlowError(w, r, f, node.DefaultGroup, err) + return + } + newFlow.InternalContext, err = sjson.SetBytes(newFlow.InternalContext, flow.InternalContextLinkCredentialsPath, + linkCredentials) + if err != nil { + h.d.RegistrationFlowErrorHandler().WriteFlowError(w, r, f, node.DefaultGroup, err) + return + } + }) + loginFlow, _, err := h.d.LoginHandler().NewLoginFlow(w, r, flow.TypeBrowser, opts...) + if err != nil { + h.d.RegistrationFlowErrorHandler().WriteFlowError(w, r, f, node.DefaultGroup, err) + return + } + + http.Redirect(w, r, loginFlow.AppendTo(h.d.Config().SelfServiceFlowLoginUI(r.Context())).String(), http.StatusSeeOther) + return + } + i := identity.NewIdentity(h.d.Config().DefaultIdentityTraitsSchemaID(r.Context())) var s Strategy for _, ss := range h.d.AllRegistrationStrategies() { diff --git a/selfservice/flow/registration/hook.go b/selfservice/flow/registration/hook.go index 5ac8a82a2d01..f5c5dd452cf9 100644 --- a/selfservice/flow/registration/hook.go +++ b/selfservice/flow/registration/hook.go @@ -6,6 +6,8 @@ package registration import ( "context" "fmt" + "github.com/ory/kratos/selfservice/flow/login" + "github.com/tidwall/sjson" "net/http" "time" @@ -73,9 +75,11 @@ type ( config.Provider identity.ManagementProvider identity.ValidationProvider + login.FlowPersistenceProvider session.PersistenceProvider session.ManagementProvider HooksProvider + FlowPersistenceProvider hydra.HydraProvider x.CSRFTokenGeneratorProvider x.HTTPClientProvider @@ -137,6 +141,38 @@ func (e *HookExecutor) PostRegistrationHook(w http.ResponseWriter, r *http.Reque // would imply that the identity has to exist already. } else if err := e.d.IdentityManager().Create(r.Context(), i); err != nil { if errors.Is(err, sqlcon.ErrUniqueViolation) { + registrationDuplicateCredentials := flow.RegistrationDuplicateCredentials{ + CredentialsType: ct, + CredentialsConfig: i.Credentials[ct].Config, + } + loginFlowID, err := a.GetOuterLoginFlowID() + if err != nil { + return err + } + if loginFlowID != nil { + loginFlow, err := e.d.LoginFlowPersister().GetLoginFlow(r.Context(), *loginFlowID) + if err != nil { + return err + } + loginFlow.InternalContext, err = sjson.SetBytes(loginFlow.InternalContext, flow.InternalContextDuplicateCredentialsPath, + registrationDuplicateCredentials) + if err != nil { + return err + } + if err := e.d.LoginFlowPersister().UpdateLoginFlow(r.Context(), loginFlow); err != nil { + return err + } + } + + a.InternalContext, err = sjson.SetBytes(a.InternalContext, flow.InternalContextDuplicateCredentialsPath, + registrationDuplicateCredentials) + if err != nil { + return err + } + if err := e.d.RegistrationFlowPersister().UpdateRegistrationFlow(r.Context(), a); err != nil { + return err + } + return schema.NewDuplicateCredentialsError() } return err diff --git a/selfservice/strategy/oidc/strategy.go b/selfservice/strategy/oidc/strategy.go index efba09d971d3..e3b67a0310df 100644 --- a/selfservice/strategy/oidc/strategy.go +++ b/selfservice/strategy/oidc/strategy.go @@ -488,3 +488,38 @@ func (s *Strategy) CompletedAuthenticationMethod(ctx context.Context) session.Au AAL: identity.AuthenticatorAssuranceLevel1, } } + +func (s *Strategy) linkCredentials(ctx context.Context, i *identity.Identity, idToken, accessToken, refreshToken, provider, subject string) error { + if i.Credentials == nil { + confidential, err := s.d.PrivilegedIdentityPool().GetIdentityConfidential(ctx, i.ID) + if err != nil { + return err + } + i.Credentials = confidential.Credentials + } + var conf identity.CredentialsOIDC + creds, err := i.ParseCredentials(s.ID(), &conf) + if errors.Is(err, herodot.ErrNotFound) { + var err error + if creds, err = identity.NewCredentialsOIDC(idToken, accessToken, refreshToken, provider, subject); err != nil { + return err + } + } else if err != nil { + return err + } else { + creds.Identifiers = append(creds.Identifiers, identity.OIDCUniqueID(provider, subject)) + conf.Providers = append(conf.Providers, identity.CredentialsOIDCProvider{ + Subject: subject, Provider: provider, + InitialAccessToken: accessToken, + InitialRefreshToken: refreshToken, + InitialIDToken: idToken, + }) + + creds.Config, err = json.Marshal(conf) + if err != nil { + return err + } + } + i.Credentials[s.ID()] = *creds + return nil +} diff --git a/selfservice/strategy/oidc/strategy_login.go b/selfservice/strategy/oidc/strategy_login.go index aec39c28e4cd..7fda6ad0fa04 100644 --- a/selfservice/strategy/oidc/strategy_login.go +++ b/selfservice/strategy/oidc/strategy_login.go @@ -96,6 +96,8 @@ func (s *Strategy) processLogin(w http.ResponseWriter, r *http.Request, a *login opts = append(opts, registration.WithFlowReturnTo(a.ReturnTo)) } + opts = append(opts, registration.WithOuterFlow(a.ID)) + // This flow only works for browsers anyways. aa, err := s.d.RegistrationHandler().NewRegistrationFlow(w, r, flow.TypeBrowser, opts...) if err != nil { diff --git a/selfservice/strategy/oidc/strategy_settings.go b/selfservice/strategy/oidc/strategy_settings.go index d89f3059b7d0..b30a414d7037 100644 --- a/selfservice/strategy/oidc/strategy_settings.go +++ b/selfservice/strategy/oidc/strategy_settings.go @@ -7,6 +7,7 @@ import ( "context" _ "embed" "encoding/json" + "github.com/ory/x/sqlxx" "net/http" "time" @@ -391,31 +392,10 @@ func (s *Strategy) linkProvider(w http.ResponseWriter, r *http.Request, ctxUpdat return s.handleSettingsError(w, r, ctxUpdate, p, err) } - var conf identity.CredentialsOIDC - creds, err := i.ParseCredentials(s.ID(), &conf) - if errors.Is(err, herodot.ErrNotFound) { - var err error - if creds, err = identity.NewCredentialsOIDC(it, cat, crt, provider.Config().ID, claims.Subject); err != nil { - return s.handleSettingsError(w, r, ctxUpdate, p, err) - } - } else if err != nil { + if err := s.linkCredentials(r.Context(), i, it, cat, crt, provider.Config().ID, claims.Subject); err != nil { return s.handleSettingsError(w, r, ctxUpdate, p, err) - } else { - creds.Identifiers = append(creds.Identifiers, identity.OIDCUniqueID(provider.Config().ID, claims.Subject)) - conf.Providers = append(conf.Providers, identity.CredentialsOIDCProvider{ - Subject: claims.Subject, Provider: provider.Config().ID, - InitialAccessToken: cat, - InitialRefreshToken: crt, - InitialIDToken: it, - }) - - creds.Config, err = json.Marshal(conf) - if err != nil { - return s.handleSettingsError(w, r, ctxUpdate, p, err) - } } - i.Credentials[s.ID()] = *creds if err := s.d.SettingsHookExecutor().PostSettingsHook(w, r, s.SettingsStrategyID(), ctxUpdate, i, settings.WithCallback(func(ctxUpdate *settings.UpdateContext) error { return s.PopulateSettingsMethod(r, ctxUpdate.Session.Identity, ctxUpdate.Flow) })); err != nil { @@ -503,3 +483,33 @@ func (s *Strategy) handleSettingsError(w http.ResponseWriter, r *http.Request, c return err } + +func (s *Strategy) Link(ctx context.Context, i *identity.Identity, credentialsConfig sqlxx.JSONRawMessage) error { + var credentialsOIDCConfig identity.CredentialsOIDC + if err := json.Unmarshal(credentialsConfig, &credentialsOIDCConfig); err != nil { + return err + } + if len(credentialsOIDCConfig.Providers) != 1 { + return errors.New("No oidc provider was set") + } + var credentialsOIDCProvider = credentialsOIDCConfig.Providers[0] + + if err := s.linkCredentials( + ctx, + i, + credentialsOIDCProvider.InitialIDToken, + credentialsOIDCProvider.InitialAccessToken, + credentialsOIDCProvider.InitialRefreshToken, + credentialsOIDCProvider.Provider, + credentialsOIDCProvider.Subject, + ); err != nil { + return err + } + + options := []identity.ManagerOption{identity.ManagerAllowWriteProtectedTraits} + if err := s.d.IdentityManager().Update(ctx, i, options...); err != nil { + return err + } + + return nil +} diff --git a/selfservice/strategy/oidc/strategy_test.go b/selfservice/strategy/oidc/strategy_test.go index 84387d15102e..7df7315271f5 100644 --- a/selfservice/strategy/oidc/strategy_test.go +++ b/selfservice/strategy/oidc/strategy_test.go @@ -178,7 +178,7 @@ func TestStrategy(t *testing.T) { // assert identity (success) var ai = func(t *testing.T, res *http.Response, body []byte) { - assert.Contains(t, res.Request.URL.String(), returnTS.URL) + assert.Contains(t, res.Request.URL.String(), returnTS.URL, "%s", body) assert.Equal(t, subject, gjson.GetBytes(body, "identity.traits.subject").String(), "%s", body) assert.Equal(t, claims.traits.website, gjson.GetBytes(body, "identity.traits.website").String(), "%s", body) assert.Equal(t, claims.metadataPublic.picture, gjson.GetBytes(body, "identity.metadata_public.picture").String(), "%s", body) @@ -547,6 +547,72 @@ func TestStrategy(t *testing.T) { assert.Greater(t, authAt2.Sub(authAt1).Milliseconds(), int64(0), "%s - %s : %s - %s", authAt2, authAt1, body2, body1) }) + t.Run("case=registration should start new login flow if duplicate credentials detected", func(t *testing.T) { + subject = "new-login-if-email-exist-with-password-strategy@ory.sh" + scope = []string{"openid"} + password := "lwkj52sdkjf" + + var i *identity.Identity + t.Run("case=create password identity", func(t *testing.T) { + i = identity.NewIdentity(config.DefaultIdentityTraitsSchemaID) + p, err := reg.Hasher(ctx).Generate(ctx, []byte(password)) + require.NoError(t, err) + i.SetCredentials(identity.CredentialsTypePassword, identity.Credentials{ + Identifiers: []string{subject}, + Config: sqlxx.JSONRawMessage(`{"hashed_password":"` + string(p) + `"}`), + }) + i.Traits = identity.Traits(`{"subject":"` + subject + `"}`) + + require.NoError(t, reg.PrivilegedIdentityPool().CreateIdentity(context.Background(), i)) + }) + + c := testhelpers.NewClientWithCookieJar(t, nil, false) + r := newLoginFlow(t, returnTS.URL, time.Minute) + t.Run("case=should fail login", func(t *testing.T) { + action := afv(t, r.ID, "valid") + res, err := c.PostForm(action, url.Values{"provider": {"valid"}}) + require.NoError(t, err, action) + body, err := io.ReadAll(res.Body) + require.NoError(t, res.Body.Close()) + require.NoError(t, err) + aue(t, res, body, "An account with the same identifier (email, phone, username, ...) exists already.") + }) + + var loginFlow *login.Flow + + t.Run("case=should start new login flow", func(t *testing.T) { + action := afv(t, r.ID, "valid") + res, err := c.PostForm(action, url.Values{"provider": {"valid"}}) + require.NoError(t, err, action) + body, err := io.ReadAll(res.Body) + require.NoError(t, res.Body.Close()) + require.NoError(t, err) + aue(t, res, body, "New credentials will be linked to existing account after login.") + loginFlow, err = reg.LoginFlowPersister().GetLoginFlow(context.Background(), uuid.FromStringOrNil(gjson.GetBytes(body, "id").String())) + assert.NotNil(t, loginFlow, "%s", body) + }) + + t.Run("case=should link oidc credentials to existing identity", func(t *testing.T) { + res, err := c.PostForm(loginFlow.UI.Action, url.Values{ + "csrf_token": {loginFlow.CSRFToken}, + "method": {"password"}, + "identifier": {subject}, + "password": {password}}) + require.NoError(t, err, loginFlow.UI.Action) + body, err := io.ReadAll(res.Body) + require.NoError(t, res.Body.Close()) + require.NoError(t, err) + assert.Contains(t, res.Request.URL.String(), returnTS.URL, "%s", body) + assert.Equal(t, subject, gjson.GetBytes(body, "identity.traits.subject").String(), "%s", body) + i, err = reg.PrivilegedIdentityPool().GetIdentityConfidential(ctx, i.ID) + require.NoError(t, err) + assert.NotEmpty(t, i.Credentials["oidc"], "%+v", i.Credentials) + assert.Equal(t, "valid", gjson.GetBytes(i.Credentials["oidc"].Config, "providers.0.provider").String(), + "%s", string(i.Credentials["oidc"].Config[:])) + assert.Equal(t, "oidc", gjson.GetBytes(body, "authentication_methods.0.method").String(), "%s", body) + }) + }) + t.Run("method=TestPopulateSignUpMethod", func(t *testing.T) { conf.MustSet(ctx, config.ViperKeyPublicBaseURL, "https://foo/") diff --git a/text/id.go b/text/id.go index 02143d615ea1..126443bb3e41 100644 --- a/text/id.go +++ b/text/id.go @@ -23,6 +23,7 @@ const ( InfoSelfServiceLoginContinueWebAuthn // 1010011 InfoSelfServiceLoginWebAuthnPasswordless // 1010012 InfoSelfServiceLoginContinue // 1010013 + InfoSelfServiceLoginLinkCredentials // 1010014 ) const ( diff --git a/text/message_login.go b/text/message_login.go index b1cda917989d..98170ca8b862 100644 --- a/text/message_login.go +++ b/text/message_login.go @@ -183,3 +183,12 @@ func NewInfoSelfServiceLoginContinue() *Message { Type: Info, } } + +func NewInfoSelfServiceLoginLinkCredentials() *Message { + return &Message{ + ID: InfoSelfServiceLoginLinkCredentials, + Text: "New credentials will be linked to existing account after login.", + Type: Info, + Context: context(nil), + } +} From 29a666745892b6e309c5cb460ec30a3b3ba9f8e2 Mon Sep 17 00:00:00 2001 From: splaunov Date: Tue, 11 Apr 2023 17:17:56 +0200 Subject: [PATCH 02/29] feat: link credentials when login - add LoginAndLinkCredentials method to login and registrations flows --- selfservice/flow/registration/hook.go | 58 +++++++++++++++------- selfservice/strategy/oidc/strategy.go | 10 ++++ selfservice/strategy/oidc/strategy_test.go | 4 +- ui/node/identifiers.go | 4 ++ 4 files changed, 57 insertions(+), 19 deletions(-) diff --git a/selfservice/flow/registration/hook.go b/selfservice/flow/registration/hook.go index f5c5dd452cf9..7b9f23dbf3e4 100644 --- a/selfservice/flow/registration/hook.go +++ b/selfservice/flow/registration/hook.go @@ -7,6 +7,7 @@ import ( "context" "fmt" "github.com/ory/kratos/selfservice/flow/login" + "github.com/ory/kratos/ui/node" "github.com/tidwall/sjson" "net/http" "time" @@ -76,6 +77,7 @@ type ( identity.ManagementProvider identity.ValidationProvider login.FlowPersistenceProvider + login.StrategyProvider session.PersistenceProvider session.ManagementProvider HooksProvider @@ -141,38 +143,58 @@ func (e *HookExecutor) PostRegistrationHook(w http.ResponseWriter, r *http.Reque // would imply that the identity has to exist already. } else if err := e.d.IdentityManager().Create(r.Context(), i); err != nil { if errors.Is(err, sqlcon.ErrUniqueViolation) { - registrationDuplicateCredentials := flow.RegistrationDuplicateCredentials{ - CredentialsType: ct, - CredentialsConfig: i.Credentials[ct].Config, - } - loginFlowID, err := a.GetOuterLoginFlowID() + strategy, err := e.d.AllLoginStrategies().Strategy(ct) if err != nil { return err } - if loginFlowID != nil { - loginFlow, err := e.d.LoginFlowPersister().GetLoginFlow(r.Context(), *loginFlowID) + + _, ok := strategy.(login.LinkableStrategy) + + if ok { + registrationDuplicateCredentials := flow.RegistrationDuplicateCredentials{ + CredentialsType: ct, + CredentialsConfig: i.Credentials[ct].Config, + } + loginFlowID, err := a.GetOuterLoginFlowID() if err != nil { return err } - loginFlow.InternalContext, err = sjson.SetBytes(loginFlow.InternalContext, flow.InternalContextDuplicateCredentialsPath, + if loginFlowID != nil { + loginFlow, err := e.d.LoginFlowPersister().GetLoginFlow(r.Context(), *loginFlowID) + if err != nil { + return err + } + loginFlow.InternalContext, err = sjson.SetBytes(loginFlow.InternalContext, flow.InternalContextDuplicateCredentialsPath, + registrationDuplicateCredentials) + if err != nil { + return err + } + loginFlow.UI.SetNode(node.NewInputField( + "method", + node.LoginAndLinkCredentials, + node.DefaultGroup, + node.InputAttributeTypeSubmit)) + if err := e.d.LoginFlowPersister().UpdateLoginFlow(r.Context(), loginFlow); err != nil { + return err + } + + } + + a.InternalContext, err = sjson.SetBytes(a.InternalContext, flow.InternalContextDuplicateCredentialsPath, registrationDuplicateCredentials) if err != nil { return err } - if err := e.d.LoginFlowPersister().UpdateLoginFlow(r.Context(), loginFlow); err != nil { + a.UI.SetNode(node.NewInputField( + "method", + node.LoginAndLinkCredentials, + node.DefaultGroup, + node.InputAttributeTypeSubmit)) + if err := e.d.RegistrationFlowPersister().UpdateRegistrationFlow(r.Context(), a); err != nil { return err } } - a.InternalContext, err = sjson.SetBytes(a.InternalContext, flow.InternalContextDuplicateCredentialsPath, - registrationDuplicateCredentials) - if err != nil { - return err - } - if err := e.d.RegistrationFlowPersister().UpdateRegistrationFlow(r.Context(), a); err != nil { - return err - } - return schema.NewDuplicateCredentialsError() } return err diff --git a/selfservice/strategy/oidc/strategy.go b/selfservice/strategy/oidc/strategy.go index e3b67a0310df..efd411e9b31d 100644 --- a/selfservice/strategy/oidc/strategy.go +++ b/selfservice/strategy/oidc/strategy.go @@ -449,7 +449,17 @@ func (s *Strategy) handleError(w http.ResponseWriter, r *http.Request, f flow.Fl // Reset all nodes to not confuse users. // This is kinda hacky and will probably need to be updated at some point. + var loginAndLinkCredentialsNode *node.Node + for _, n := range rf.UI.Nodes { + if n.ID() == "method" && n.GetValue() == node.LoginAndLinkCredentials { + loginAndLinkCredentialsNode = n + break + } + } rf.UI.Nodes = node.Nodes{} + if loginAndLinkCredentialsNode != nil { + rf.UI.Nodes.Upsert(loginAndLinkCredentialsNode) + } // Adds the "Continue" button rf.UI.SetCSRF(s.d.GenerateCSRFToken(r)) diff --git a/selfservice/strategy/oidc/strategy_test.go b/selfservice/strategy/oidc/strategy_test.go index 7df7315271f5..30d805000994 100644 --- a/selfservice/strategy/oidc/strategy_test.go +++ b/selfservice/strategy/oidc/strategy_test.go @@ -8,6 +8,7 @@ import ( "context" "encoding/json" "fmt" + "github.com/ory/kratos/ui/node" "io" "net/http" "net/http/cookiejar" @@ -576,13 +577,14 @@ func TestStrategy(t *testing.T) { require.NoError(t, res.Body.Close()) require.NoError(t, err) aue(t, res, body, "An account with the same identifier (email, phone, username, ...) exists already.") + assert.Equal(t, node.LoginAndLinkCredentials, gjson.GetBytes(body, "ui.nodes.#(attributes.name==\"method\").attributes.value").String(), "%s", body) }) var loginFlow *login.Flow t.Run("case=should start new login flow", func(t *testing.T) { action := afv(t, r.ID, "valid") - res, err := c.PostForm(action, url.Values{"provider": {"valid"}}) + res, err := c.PostForm(action, url.Values{"method": {node.LoginAndLinkCredentials}}) require.NoError(t, err, action) body, err := io.ReadAll(res.Body) require.NoError(t, res.Body.Close()) diff --git a/ui/node/identifiers.go b/ui/node/identifiers.go index 15385b5f84b6..af3bfc1cfae3 100644 --- a/ui/node/identifiers.go +++ b/ui/node/identifiers.go @@ -28,3 +28,7 @@ const ( WebAuthnRemove = "webauthn_remove" WebAuthnScript = "webauthn_script" ) + +const ( + LoginAndLinkCredentials = "login_and_link_credentials" +) From 5b93315b818f0a61dda8d0debbcfb19e29944573 Mon Sep 17 00:00:00 2001 From: splaunov Date: Tue, 25 Apr 2023 13:37:00 +0200 Subject: [PATCH 03/29] feat(saml): add linkCredentialsFlow parameter to update flow request (CORE-1986) --- .../.schema/link_credentials.schema.json | 10 ++++ selfservice/flow/login/handler.go | 60 +++++++++++++++---- 2 files changed, 60 insertions(+), 10 deletions(-) create mode 100644 selfservice/flow/login/.schema/link_credentials.schema.json diff --git a/selfservice/flow/login/.schema/link_credentials.schema.json b/selfservice/flow/login/.schema/link_credentials.schema.json new file mode 100644 index 000000000000..25d4a8855e4f --- /dev/null +++ b/selfservice/flow/login/.schema/link_credentials.schema.json @@ -0,0 +1,10 @@ +{ + "$id": "https://schemas.ory.sh/kratos/selfservice/flow/link_credentials.schema.json", + "$schema": "http://json-schema.org/draft-07/schema#", + "type": "object", + "properties": { + "linkCredentialsFlow": { + "type": "string" + } + } +} diff --git a/selfservice/flow/login/handler.go b/selfservice/flow/login/handler.go index 13c377189884..f9b2181ee94d 100644 --- a/selfservice/flow/login/handler.go +++ b/selfservice/flow/login/handler.go @@ -4,7 +4,7 @@ package login import ( - "context" + _ "embed" "encoding/json" "fmt" "github.com/tidwall/gjson" @@ -41,6 +41,9 @@ import ( "github.com/ory/kratos/x" ) +//go:embed .schema/link_credentials.schema.json +var linkCredentialsSchema []byte + const ( RouteInitBrowserFlow = "/self-service/login/browser" RouteInitAPIFlow = "/self-service/login/api" @@ -750,7 +753,7 @@ continueLogin: sess = session.NewInactiveSession() } - if err := h.linkCredentials(r.Context(), sess, interim, f); err != nil { + if err := h.linkCredentials(r, sess, interim, f); err != nil { h.d.LoginFlowErrorHandler().WriteFlowError(w, r, f, group, err) return } @@ -777,14 +780,41 @@ continueLogin: } } -func (h *Handler) linkCredentials(ctx context.Context, s *session.Session, i *identity.Identity, f *Flow) error { - internalContextLinkCredentials := gjson.GetBytes(f.InternalContext, flow.InternalContextLinkCredentialsPath) - if internalContextLinkCredentials.IsObject() { - var linkCredentials flow.RegistrationDuplicateCredentials - if err := json.Unmarshal([]byte(internalContextLinkCredentials.Raw), &linkCredentials); err != nil { +func (h *Handler) linkCredentials(r *http.Request, s *session.Session, i *identity.Identity, f *Flow) error { + var lc flow.RegistrationDuplicateCredentials + + var p struct { + FlowID string `json:"linkCredentialsFlow" form:"linkCredentialsFlow"` + } + + if err := h.hd.Decode(r, &p, + decoderx.HTTPDecoderSetValidatePayloads(true), + decoderx.MustHTTPRawJSONSchemaCompiler(linkCredentialsSchema), + decoderx.HTTPDecoderJSONFollowsFormFormat()); err != nil { + return err + } + + if p.FlowID != "" { + linkCredentialsFlowID, innerErr := uuid.FromString(p.FlowID) + if innerErr != nil { + return innerErr + } + linkCredentialsFlow, innerErr := h.d.LoginFlowPersister().GetLoginFlow(r.Context(), linkCredentialsFlowID) + innerErr = h.getInternalContextLinkCredentials(linkCredentialsFlow, flow.InternalContextDuplicateCredentialsPath, &lc) + if innerErr != nil { + return innerErr + } + } + + if lc.CredentialsType == "" { + err := h.getInternalContextLinkCredentials(f, flow.InternalContextLinkCredentialsPath, &lc) + if err != nil { return err } - strategy, err := h.d.AllLoginStrategies().Strategy(linkCredentials.CredentialsType) + } + + if lc.CredentialsType != "" { + strategy, err := h.d.AllLoginStrategies().Strategy(lc.CredentialsType) if err != nil { return err } @@ -794,13 +824,23 @@ func (h *Handler) linkCredentials(ctx context.Context, s *session.Session, i *id return errors.New(fmt.Sprintf("Strategy is not linkable: %T", linkableStrategy)) } - if err := linkableStrategy.Link(ctx, i, linkCredentials.CredentialsConfig); err != nil { + if err := linkableStrategy.Link(r.Context(), i, lc.CredentialsConfig); err != nil { return err } - method := strategy.CompletedAuthenticationMethod(ctx) + method := strategy.CompletedAuthenticationMethod(r.Context()) s.CompletedLoginFor(method.Method, method.AAL) } return nil } + +func (h *Handler) getInternalContextLinkCredentials(f *Flow, internalContextPath string, lc *flow.RegistrationDuplicateCredentials) error { + internalContextLinkCredentials := gjson.GetBytes(f.InternalContext, internalContextPath) + if internalContextLinkCredentials.IsObject() { + if err := json.Unmarshal([]byte(internalContextLinkCredentials.Raw), lc); err != nil { + return err + } + } + return nil +} From a38a5b1a6ed406ddbc88f257937490f694ea2bd2 Mon Sep 17 00:00:00 2001 From: splaunov Date: Thu, 27 Apr 2023 09:59:39 +0200 Subject: [PATCH 04/29] fix: panic when linked flow does not exist (CORE-2017) --- selfservice/flow/login/handler.go | 3 +++ selfservice/strategy/password/login_test.go | 23 +++++++++++++++++++++ 2 files changed, 26 insertions(+) diff --git a/selfservice/flow/login/handler.go b/selfservice/flow/login/handler.go index f9b2181ee94d..832f15e72b1f 100644 --- a/selfservice/flow/login/handler.go +++ b/selfservice/flow/login/handler.go @@ -800,6 +800,9 @@ func (h *Handler) linkCredentials(r *http.Request, s *session.Session, i *identi return innerErr } linkCredentialsFlow, innerErr := h.d.LoginFlowPersister().GetLoginFlow(r.Context(), linkCredentialsFlowID) + if innerErr != nil { + return innerErr + } innerErr = h.getInternalContextLinkCredentials(linkCredentialsFlow, flow.InternalContextDuplicateCredentialsPath, &lc) if innerErr != nil { return innerErr diff --git a/selfservice/strategy/password/login_test.go b/selfservice/strategy/password/login_test.go index 769598206c0f..642353952e5b 100644 --- a/selfservice/strategy/password/login_test.go +++ b/selfservice/strategy/password/login_test.go @@ -466,6 +466,29 @@ func TestCompleteLogin(t *testing.T) { }) }) + t.Run("case=should return an error because the linkCredentialsFlow does not exist", func(t *testing.T) { + identifier, pwd := x.NewUUID().String(), "password" + createIdentity(ctx, reg, t, identifier, pwd) + + var check = func(t *testing.T, actual string) { + assert.Equal(t, int64(http.StatusNotFound), gjson.Get(actual, "code").Int(), "%s", actual) + assert.Equal(t, "Not Found", gjson.Get(actual, "status").String(), "%s", actual) + assert.Contains(t, gjson.Get(actual, "message").String(), "Unable to locate the resource", "%s", actual) + } + + var values = func(v url.Values) { + v.Set("identifier", identifier) + v.Set("password", pwd) + v.Set("linkCredentialsFlow", "00000000-0000-0000-0000-000000000000") + } + + t.Run("type=api", func(t *testing.T) { + actual := testhelpers.SubmitLoginForm(t, true, nil, publicTS, values, + false, false, http.StatusNotFound, publicTS.URL+login.RouteSubmitFlow) + check(t, gjson.Get(actual, "error").Raw) + }) + }) + t.Run("should pass with real request", func(t *testing.T) { identifier, pwd := x.NewUUID().String(), "password" createIdentity(ctx, reg, t, identifier, pwd) From 4d2551e3246e2073bb3328377c43f4d10c503ad8 Mon Sep 17 00:00:00 2001 From: splaunov Date: Tue, 2 May 2023 08:24:47 +0200 Subject: [PATCH 05/29] feat: block linking credentials if they do not match identifier of logged in identity (CORE-2006) --- cmd/clidoc/main.go | 1 + schema/errors.go | 10 +++++++++ selfservice/flow/flow.go | 5 +++-- selfservice/flow/login/handler.go | 20 +++++++++++++++++ selfservice/flow/registration/hook.go | 26 ++++++++++++++++++++-- selfservice/strategy/oidc/strategy_test.go | 24 +++++++++++++++++++- text/id.go | 15 +++++++------ text/message_login.go | 8 +++++++ 8 files changed, 97 insertions(+), 12 deletions(-) diff --git a/cmd/clidoc/main.go b/cmd/clidoc/main.go index 8f4117adbf9f..e6bfceebbba5 100644 --- a/cmd/clidoc/main.go +++ b/cmd/clidoc/main.go @@ -132,6 +132,7 @@ func init() { "NewInfoSelfServiceLoginContinue": text.NewInfoSelfServiceLoginContinue(), "NewErrorValidationSuchNoWebAuthnUser": text.NewErrorValidationSuchNoWebAuthnUser(), "NewInfoSelfServiceLoginLinkCredentials": text.NewInfoSelfServiceLoginLinkCredentials(), + "NewErrorValidationLoginLinkedCredentialsDoNotMatch": text.NewErrorValidationLoginLinkedCredentialsDoNotMatch(), } } diff --git a/schema/errors.go b/schema/errors.go index 83513df8badc..f1437c23e877 100644 --- a/schema/errors.go +++ b/schema/errors.go @@ -303,3 +303,13 @@ func NewNoWebAuthnCredentials() error { Messages: new(text.Messages).Add(text.NewErrorValidationSuchNoWebAuthnUser()), }) } + +func NewLinkedCredentialsDoNotMatch() error { + return errors.WithStack(&ValidationError{ + ValidationError: &jsonschema.ValidationError{ + Message: `linked credentials do not match`, + InstancePtr: "#/", + }, + Messages: new(text.Messages).Add(text.NewErrorValidationLoginLinkedCredentialsDoNotMatch()), + }) +} diff --git a/selfservice/flow/flow.go b/selfservice/flow/flow.go index ddce5db990c2..413a1020f579 100644 --- a/selfservice/flow/flow.go +++ b/selfservice/flow/flow.go @@ -25,8 +25,9 @@ const InternalContextDuplicateCredentialsPath = "registration_duplicate_credenti const InternalContextLinkCredentialsPath = "link_credentials" type RegistrationDuplicateCredentials struct { - CredentialsType identity.CredentialsType - CredentialsConfig sqlxx.JSONRawMessage + CredentialsType identity.CredentialsType + CredentialsConfig sqlxx.JSONRawMessage + DuplicateIdentifier string } func AppendFlowTo(src *url.URL, id uuid.UUID) *url.URL { diff --git a/selfservice/flow/login/handler.go b/selfservice/flow/login/handler.go index 832f15e72b1f..622f658d9038 100644 --- a/selfservice/flow/login/handler.go +++ b/selfservice/flow/login/handler.go @@ -4,6 +4,7 @@ package login import ( + "context" _ "embed" "encoding/json" "fmt" @@ -67,6 +68,7 @@ type ( x.CSRFProvider config.Provider ErrorHandlerProvider + identity.PrivilegedPoolProvider } HandlerProvider interface { LoginHandler() *Handler @@ -817,6 +819,9 @@ func (h *Handler) linkCredentials(r *http.Request, s *session.Session, i *identi } if lc.CredentialsType != "" { + if err := h.checkDuplecateCredentialsIdentifierMatch(r.Context(), i.ID, lc.DuplicateIdentifier); err != nil { + return err + } strategy, err := h.d.AllLoginStrategies().Strategy(lc.CredentialsType) if err != nil { return err @@ -847,3 +852,18 @@ func (h *Handler) getInternalContextLinkCredentials(f *Flow, internalContextPath } return nil } + +func (h *Handler) checkDuplecateCredentialsIdentifierMatch(ctx context.Context, identityID uuid.UUID, match string) error { + i, err := h.d.PrivilegedIdentityPool().GetIdentityConfidential(ctx, identityID) + if err != nil { + return err + } + for _, credentials := range i.Credentials { + for _, identifier := range credentials.Identifiers { + if identifier == match { + return nil + } + } + } + return schema.NewLinkedCredentialsDoNotMatch() +} diff --git a/selfservice/flow/registration/hook.go b/selfservice/flow/registration/hook.go index 7b9f23dbf3e4..693660b9adec 100644 --- a/selfservice/flow/registration/hook.go +++ b/selfservice/flow/registration/hook.go @@ -75,6 +75,7 @@ type ( executorDependencies interface { config.Provider identity.ManagementProvider + identity.PrivilegedPoolProvider identity.ValidationProvider login.FlowPersistenceProvider login.StrategyProvider @@ -151,9 +152,14 @@ func (e *HookExecutor) PostRegistrationHook(w http.ResponseWriter, r *http.Reque _, ok := strategy.(login.LinkableStrategy) if ok { + duplicateIdentifier, err := e.getDuplicateIdentifier(r.Context(), i) + if err != nil { + return err + } registrationDuplicateCredentials := flow.RegistrationDuplicateCredentials{ - CredentialsType: ct, - CredentialsConfig: i.Credentials[ct].Config, + CredentialsType: ct, + CredentialsConfig: i.Credentials[ct].Config, + DuplicateIdentifier: duplicateIdentifier, } loginFlowID, err := a.GetOuterLoginFlowID() if err != nil { @@ -290,6 +296,22 @@ func (e *HookExecutor) PostRegistrationHook(w http.ResponseWriter, r *http.Reque return nil } +func (e *HookExecutor) getDuplicateIdentifier(ctx context.Context, i *identity.Identity) (string, error) { + for ct, credentials := range i.Credentials { + for _, identifier := range credentials.Identifiers { + _, _, err := e.d.PrivilegedIdentityPool().FindByCredentialsIdentifier(ctx, ct, identifier) + if err != nil { + if errors.Is(err, sqlcon.ErrNoRows) { + continue + } + return "", err + } + return identifier, nil + } + } + return "", errors.New("Duplicate credential not found") +} + func (e *HookExecutor) PreRegistrationHook(w http.ResponseWriter, r *http.Request, a *Flow) error { for _, executor := range e.d.PreRegistrationHooks(r.Context()) { if err := executor.ExecuteRegistrationPreHook(w, r, a); err != nil { diff --git a/selfservice/strategy/oidc/strategy_test.go b/selfservice/strategy/oidc/strategy_test.go index 30d805000994..0f840ae12c22 100644 --- a/selfservice/strategy/oidc/strategy_test.go +++ b/selfservice/strategy/oidc/strategy_test.go @@ -14,6 +14,7 @@ import ( "net/http/cookiejar" "net/http/httptest" "net/url" + "strconv" "strings" "testing" "time" @@ -550,6 +551,7 @@ func TestStrategy(t *testing.T) { t.Run("case=registration should start new login flow if duplicate credentials detected", func(t *testing.T) { subject = "new-login-if-email-exist-with-password-strategy@ory.sh" + subject2 := "new-login-subject2@ory.sh" scope = []string{"openid"} password := "lwkj52sdkjf" @@ -563,8 +565,15 @@ func TestStrategy(t *testing.T) { Config: sqlxx.JSONRawMessage(`{"hashed_password":"` + string(p) + `"}`), }) i.Traits = identity.Traits(`{"subject":"` + subject + `"}`) - require.NoError(t, reg.PrivilegedIdentityPool().CreateIdentity(context.Background(), i)) + + i2 := identity.NewIdentity(config.DefaultIdentityTraitsSchemaID) + i2.SetCredentials(identity.CredentialsTypePassword, identity.Credentials{ + Identifiers: []string{subject2}, + Config: sqlxx.JSONRawMessage(`{"hashed_password":"` + string(p) + `"}`), + }) + i2.Traits = identity.Traits(`{"subject":"` + subject2 + `"}`) + require.NoError(t, reg.PrivilegedIdentityPool().CreateIdentity(context.Background(), i2)) }) c := testhelpers.NewClientWithCookieJar(t, nil, false) @@ -594,6 +603,19 @@ func TestStrategy(t *testing.T) { assert.NotNil(t, loginFlow, "%s", body) }) + t.Run("case=should fail login if existing identity identifier doesn't match", func(t *testing.T) { + res, err := c.PostForm(loginFlow.UI.Action, url.Values{ + "csrf_token": {loginFlow.CSRFToken}, + "method": {"password"}, + "identifier": {subject2}, + "password": {password}}) + require.NoError(t, err, loginFlow.UI.Action) + body, err := io.ReadAll(res.Body) + require.NoError(t, res.Body.Close()) + require.NoError(t, err) + assert.Equal(t, strconv.Itoa(int(text.ErrorValidationLoginLinkedCredentialsDoNotMatch)), gjson.GetBytes(body, "ui.messages.0.id").String(), "%s", body) + }) + t.Run("case=should link oidc credentials to existing identity", func(t *testing.T) { res, err := c.PostForm(loginFlow.UI.Action, url.Values{ "csrf_token": {loginFlow.CSRFToken}, diff --git a/text/id.go b/text/id.go index 126443bb3e41..9e79ac68c871 100644 --- a/text/id.go +++ b/text/id.go @@ -112,13 +112,14 @@ const ( ) const ( - ErrorValidationLogin ID = 4010000 + iota // 4010000 - ErrorValidationLoginFlowExpired // 4010001 - ErrorValidationLoginNoStrategyFound // 4010002 - ErrorValidationRegistrationNoStrategyFound // 4010003 - ErrorValidationSettingsNoStrategyFound // 4010004 - ErrorValidationRecoveryNoStrategyFound // 4010005 - ErrorValidationVerificationNoStrategyFound // 4010006 + ErrorValidationLogin ID = 4010000 + iota // 4010000 + ErrorValidationLoginFlowExpired // 4010001 + ErrorValidationLoginNoStrategyFound // 4010002 + ErrorValidationRegistrationNoStrategyFound // 4010003 + ErrorValidationSettingsNoStrategyFound // 4010004 + ErrorValidationRecoveryNoStrategyFound // 4010005 + ErrorValidationVerificationNoStrategyFound // 4010006 + ErrorValidationLoginLinkedCredentialsDoNotMatch // 4010007 ) const ( diff --git a/text/message_login.go b/text/message_login.go index 98170ca8b862..0b5593a1b3f4 100644 --- a/text/message_login.go +++ b/text/message_login.go @@ -192,3 +192,11 @@ func NewInfoSelfServiceLoginLinkCredentials() *Message { Context: context(nil), } } + +func NewErrorValidationLoginLinkedCredentialsDoNotMatch() *Message { + return &Message{ + ID: ErrorValidationLoginLinkedCredentialsDoNotMatch, + Text: "Linked credentials do not match.", + Type: Error, + } +} From c06f46bd725bdc3f46794d83be32f7d9526d9531 Mon Sep 17 00:00:00 2001 From: splaunov Date: Mon, 8 May 2023 09:10:34 +0200 Subject: [PATCH 06/29] feat: link credentials when second login is OIDC (CORE-2041) --- selfservice/flow/login/handler.go | 94 -------- selfservice/flow/login/hook.go | 100 ++++++++ ...rategy-method=TestPopulateLoginMethod.json | 22 ++ ...ategy-method=TestPopulateSignUpMethod.json | 22 ++ selfservice/strategy/oidc/strategy_test.go | 224 ++++++++++++------ 5 files changed, 290 insertions(+), 172 deletions(-) diff --git a/selfservice/flow/login/handler.go b/selfservice/flow/login/handler.go index 622f658d9038..3ba703d4dab0 100644 --- a/selfservice/flow/login/handler.go +++ b/selfservice/flow/login/handler.go @@ -4,10 +4,8 @@ package login import ( - "context" _ "embed" "encoding/json" - "fmt" "github.com/tidwall/gjson" "github.com/tidwall/sjson" "net/http" @@ -68,7 +66,6 @@ type ( x.CSRFProvider config.Provider ErrorHandlerProvider - identity.PrivilegedPoolProvider } HandlerProvider interface { LoginHandler() *Handler @@ -755,11 +752,6 @@ continueLogin: sess = session.NewInactiveSession() } - if err := h.linkCredentials(r, sess, interim, f); err != nil { - h.d.LoginFlowErrorHandler().WriteFlowError(w, r, f, group, err) - return - } - method := ss.CompletedAuthenticationMethod(r.Context()) sess.CompletedLoginFor(method.Method, method.AAL) i = interim @@ -781,89 +773,3 @@ continueLogin: return } } - -func (h *Handler) linkCredentials(r *http.Request, s *session.Session, i *identity.Identity, f *Flow) error { - var lc flow.RegistrationDuplicateCredentials - - var p struct { - FlowID string `json:"linkCredentialsFlow" form:"linkCredentialsFlow"` - } - - if err := h.hd.Decode(r, &p, - decoderx.HTTPDecoderSetValidatePayloads(true), - decoderx.MustHTTPRawJSONSchemaCompiler(linkCredentialsSchema), - decoderx.HTTPDecoderJSONFollowsFormFormat()); err != nil { - return err - } - - if p.FlowID != "" { - linkCredentialsFlowID, innerErr := uuid.FromString(p.FlowID) - if innerErr != nil { - return innerErr - } - linkCredentialsFlow, innerErr := h.d.LoginFlowPersister().GetLoginFlow(r.Context(), linkCredentialsFlowID) - if innerErr != nil { - return innerErr - } - innerErr = h.getInternalContextLinkCredentials(linkCredentialsFlow, flow.InternalContextDuplicateCredentialsPath, &lc) - if innerErr != nil { - return innerErr - } - } - - if lc.CredentialsType == "" { - err := h.getInternalContextLinkCredentials(f, flow.InternalContextLinkCredentialsPath, &lc) - if err != nil { - return err - } - } - - if lc.CredentialsType != "" { - if err := h.checkDuplecateCredentialsIdentifierMatch(r.Context(), i.ID, lc.DuplicateIdentifier); err != nil { - return err - } - strategy, err := h.d.AllLoginStrategies().Strategy(lc.CredentialsType) - if err != nil { - return err - } - - linkableStrategy, ok := strategy.(LinkableStrategy) - if !ok { - return errors.New(fmt.Sprintf("Strategy is not linkable: %T", linkableStrategy)) - } - - if err := linkableStrategy.Link(r.Context(), i, lc.CredentialsConfig); err != nil { - return err - } - - method := strategy.CompletedAuthenticationMethod(r.Context()) - s.CompletedLoginFor(method.Method, method.AAL) - } - - return nil -} - -func (h *Handler) getInternalContextLinkCredentials(f *Flow, internalContextPath string, lc *flow.RegistrationDuplicateCredentials) error { - internalContextLinkCredentials := gjson.GetBytes(f.InternalContext, internalContextPath) - if internalContextLinkCredentials.IsObject() { - if err := json.Unmarshal([]byte(internalContextLinkCredentials.Raw), lc); err != nil { - return err - } - } - return nil -} - -func (h *Handler) checkDuplecateCredentialsIdentifierMatch(ctx context.Context, identityID uuid.UUID, match string) error { - i, err := h.d.PrivilegedIdentityPool().GetIdentityConfidential(ctx, identityID) - if err != nil { - return err - } - for _, credentials := range i.Credentials { - for _, identifier := range credentials.Identifiers { - if identifier == match { - return nil - } - } - } - return schema.NewLinkedCredentialsDoNotMatch() -} diff --git a/selfservice/flow/login/hook.go b/selfservice/flow/login/hook.go index 9e98cb922f81..79c79bcb0994 100644 --- a/selfservice/flow/login/hook.go +++ b/selfservice/flow/login/hook.go @@ -5,7 +5,12 @@ package login import ( "context" + "encoding/json" "fmt" + "github.com/gofrs/uuid" + "github.com/ory/kratos/schema" + "github.com/ory/x/decoderx" + "github.com/tidwall/gjson" "net/http" "time" @@ -43,13 +48,16 @@ type ( executorDependencies interface { config.Provider hydra.HydraProvider + identity.PrivilegedPoolProvider session.ManagementProvider session.PersistenceProvider x.CSRFTokenGeneratorProvider x.WriterProvider x.LoggingProvider + FlowPersistenceProvider HooksProvider + StrategyProvider } HookExecutor struct { d executorDependencies @@ -110,6 +118,10 @@ func (e *HookExecutor) handleLoginError(_ http.ResponseWriter, r *http.Request, } func (e *HookExecutor) PostLoginHook(w http.ResponseWriter, r *http.Request, g node.UiNodeGroup, a *Flow, i *identity.Identity, s *session.Session) error { + if err := e.linkCredentials(r, s, i, a); err != nil { + return err + } + if err := s.Activate(r, i, e.d.Config(), time.Now().UTC()); err != nil { return err } @@ -250,3 +262,91 @@ func (e *HookExecutor) PreLoginHook(w http.ResponseWriter, r *http.Request, a *F return nil } + +func (e *HookExecutor) linkCredentials(r *http.Request, s *session.Session, i *identity.Identity, f *Flow) error { + var lc flow.RegistrationDuplicateCredentials + + if r.Method == "POST" { + var p struct { + FlowID string `json:"linkCredentialsFlow" form:"linkCredentialsFlow"` + } + + if err := decoderx.NewHTTP().Decode(r, &p, + decoderx.HTTPDecoderSetValidatePayloads(true), + decoderx.MustHTTPRawJSONSchemaCompiler(linkCredentialsSchema), + decoderx.HTTPDecoderJSONFollowsFormFormat()); err != nil { + return err + } + + if p.FlowID != "" { + linkCredentialsFlowID, innerErr := uuid.FromString(p.FlowID) + if innerErr != nil { + return innerErr + } + linkCredentialsFlow, innerErr := e.d.LoginFlowPersister().GetLoginFlow(r.Context(), linkCredentialsFlowID) + if innerErr != nil { + return innerErr + } + innerErr = e.getInternalContextLinkCredentials(linkCredentialsFlow, flow.InternalContextDuplicateCredentialsPath, &lc) + if innerErr != nil { + return innerErr + } + } + } + + if lc.CredentialsType == "" { + err := e.getInternalContextLinkCredentials(f, flow.InternalContextLinkCredentialsPath, &lc) + if err != nil { + return err + } + } + + if lc.CredentialsType != "" { + if err := e.checkDuplecateCredentialsIdentifierMatch(r.Context(), i.ID, lc.DuplicateIdentifier); err != nil { + return err + } + strategy, err := e.d.AllLoginStrategies().Strategy(lc.CredentialsType) + if err != nil { + return err + } + + linkableStrategy, ok := strategy.(LinkableStrategy) + if !ok { + return errors.New(fmt.Sprintf("Strategy is not linkable: %T", linkableStrategy)) + } + + if err := linkableStrategy.Link(r.Context(), i, lc.CredentialsConfig); err != nil { + return err + } + + method := strategy.CompletedAuthenticationMethod(r.Context()) + s.CompletedLoginFor(method.Method, method.AAL) + } + + return nil +} + +func (e *HookExecutor) getInternalContextLinkCredentials(f *Flow, internalContextPath string, lc *flow.RegistrationDuplicateCredentials) error { + internalContextLinkCredentials := gjson.GetBytes(f.InternalContext, internalContextPath) + if internalContextLinkCredentials.IsObject() { + if err := json.Unmarshal([]byte(internalContextLinkCredentials.Raw), lc); err != nil { + return err + } + } + return nil +} + +func (e *HookExecutor) checkDuplecateCredentialsIdentifierMatch(ctx context.Context, identityID uuid.UUID, match string) error { + i, err := e.d.PrivilegedIdentityPool().GetIdentityConfidential(ctx, identityID) + if err != nil { + return err + } + for _, credentials := range i.Credentials { + for _, identifier := range credentials.Identifiers { + if identifier == match { + return nil + } + } + } + return schema.NewLinkedCredentialsDoNotMatch() +} diff --git a/selfservice/strategy/oidc/.snapshots/TestStrategy-method=TestPopulateLoginMethod.json b/selfservice/strategy/oidc/.snapshots/TestStrategy-method=TestPopulateLoginMethod.json index 9a93294a6040..04eba3e43565 100644 --- a/selfservice/strategy/oidc/.snapshots/TestStrategy-method=TestPopulateLoginMethod.json +++ b/selfservice/strategy/oidc/.snapshots/TestStrategy-method=TestPopulateLoginMethod.json @@ -36,6 +36,28 @@ } } }, + { + "type": "input", + "group": "oidc", + "attributes": { + "name": "provider", + "type": "submit", + "value": "secondProvider", + "disabled": false, + "node_type": "input" + }, + "messages": [], + "meta": { + "label": { + "id": 1010002, + "text": "Sign in with secondProvider", + "type": "info", + "context": { + "provider": "secondProvider" + } + } + } + }, { "type": "input", "group": "oidc", diff --git a/selfservice/strategy/oidc/.snapshots/TestStrategy-method=TestPopulateSignUpMethod.json b/selfservice/strategy/oidc/.snapshots/TestStrategy-method=TestPopulateSignUpMethod.json index 699d8d10ec62..e9b5dc03f8e6 100644 --- a/selfservice/strategy/oidc/.snapshots/TestStrategy-method=TestPopulateSignUpMethod.json +++ b/selfservice/strategy/oidc/.snapshots/TestStrategy-method=TestPopulateSignUpMethod.json @@ -36,6 +36,28 @@ } } }, + { + "type": "input", + "group": "oidc", + "attributes": { + "name": "provider", + "type": "submit", + "value": "secondProvider", + "disabled": false, + "node_type": "input" + }, + "messages": [], + "meta": { + "label": { + "id": 1040002, + "text": "Sign up with secondProvider", + "type": "info", + "context": { + "provider": "secondProvider" + } + } + } + }, { "type": "input", "group": "oidc", diff --git a/selfservice/strategy/oidc/strategy_test.go b/selfservice/strategy/oidc/strategy_test.go index 0f840ae12c22..da654c837f2f 100644 --- a/selfservice/strategy/oidc/strategy_test.go +++ b/selfservice/strategy/oidc/strategy_test.go @@ -75,6 +75,7 @@ func TestStrategy(t *testing.T) { t, conf, newOIDCProvider(t, ts, remotePublic, remoteAdmin, "valid"), + newOIDCProvider(t, ts, remotePublic, remoteAdmin, "secondProvider"), oidc.Configuration{ Provider: "generic", ID: "invalid-issuer", @@ -363,23 +364,25 @@ func TestStrategy(t *testing.T) { }) }) + expectTokens := func(t *testing.T, provider string, body []byte) uuid.UUID { + id := uuid.FromStringOrNil(gjson.GetBytes(body, "identity.id").String()) + i, err := reg.PrivilegedIdentityPool().GetIdentityConfidential(context.Background(), id) + require.NoError(t, err) + c := i.Credentials[identity.CredentialsTypeOIDC].Config + assert.NotEmpty(t, gjson.GetBytes(c, "providers.0.initial_access_token").String()) + assertx.EqualAsJSONExcept( + t, + json.RawMessage(fmt.Sprintf(`{"providers": [{"subject":"%s","provider":"%s"}]}`, subject, provider)), + json.RawMessage(c), + []string{"providers.0.initial_id_token", "providers.0.initial_access_token", "providers.0.initial_refresh_token"}, + ) + return id + } + t.Run("case=register and then login", func(t *testing.T) { subject = "register-then-login@ory.sh" scope = []string{"openid", "offline"} - expectTokens := func(t *testing.T, provider string, body []byte) { - i, err := reg.PrivilegedIdentityPool().GetIdentityConfidential(context.Background(), uuid.FromStringOrNil(gjson.GetBytes(body, "identity.id").String())) - require.NoError(t, err) - c := i.Credentials[identity.CredentialsTypeOIDC].Config - assert.NotEmpty(t, gjson.GetBytes(c, "providers.0.initial_access_token").String()) - assertx.EqualAsJSONExcept( - t, - json.RawMessage(fmt.Sprintf(`{"providers": [{"subject":"%s","provider":"%s"}]}`, subject, provider)), - json.RawMessage(c), - []string{"providers.0.initial_id_token", "providers.0.initial_access_token", "providers.0.initial_refresh_token"}, - ) - } - t.Run("case=should pass registration", func(t *testing.T) { r := newRegistrationFlow(t, returnTS.URL, time.Minute) action := afv(t, r.ID, "valid") @@ -550,90 +553,155 @@ func TestStrategy(t *testing.T) { }) t.Run("case=registration should start new login flow if duplicate credentials detected", func(t *testing.T) { - subject = "new-login-if-email-exist-with-password-strategy@ory.sh" - subject2 := "new-login-subject2@ory.sh" - scope = []string{"openid"} - password := "lwkj52sdkjf" - var i *identity.Identity - t.Run("case=create password identity", func(t *testing.T) { - i = identity.NewIdentity(config.DefaultIdentityTraitsSchemaID) - p, err := reg.Hasher(ctx).Generate(ctx, []byte(password)) - require.NoError(t, err) - i.SetCredentials(identity.CredentialsTypePassword, identity.Credentials{ - Identifiers: []string{subject}, - Config: sqlxx.JSONRawMessage(`{"hashed_password":"` + string(p) + `"}`), - }) - i.Traits = identity.Traits(`{"subject":"` + subject + `"}`) - require.NoError(t, reg.PrivilegedIdentityPool().CreateIdentity(context.Background(), i)) - - i2 := identity.NewIdentity(config.DefaultIdentityTraitsSchemaID) - i2.SetCredentials(identity.CredentialsTypePassword, identity.Credentials{ - Identifiers: []string{subject2}, - Config: sqlxx.JSONRawMessage(`{"hashed_password":"` + string(p) + `"}`), - }) - i2.Traits = identity.Traits(`{"subject":"` + subject2 + `"}`) - require.NoError(t, reg.PrivilegedIdentityPool().CreateIdentity(context.Background(), i2)) - }) - - c := testhelpers.NewClientWithCookieJar(t, nil, false) - r := newLoginFlow(t, returnTS.URL, time.Minute) - t.Run("case=should fail login", func(t *testing.T) { - action := afv(t, r.ID, "valid") - res, err := c.PostForm(action, url.Values{"provider": {"valid"}}) + loginWithOIDC := func(t *testing.T, c *http.Client, flowID uuid.UUID, provider string) (*http.Response, []byte) { + action := afv(t, flowID, provider) + res, err := c.PostForm(action, url.Values{"provider": {provider}}) require.NoError(t, err, action) body, err := io.ReadAll(res.Body) require.NoError(t, res.Body.Close()) require.NoError(t, err) - aue(t, res, body, "An account with the same identifier (email, phone, username, ...) exists already.") - assert.Equal(t, node.LoginAndLinkCredentials, gjson.GetBytes(body, "ui.nodes.#(attributes.name==\"method\").attributes.value").String(), "%s", body) - }) - - var loginFlow *login.Flow + return res, body + } - t.Run("case=should start new login flow", func(t *testing.T) { - action := afv(t, r.ID, "valid") + stratNewLoginFlowForLinking := func(t *testing.T, c *http.Client, flowID uuid.UUID) *login.Flow { + action := afv(t, flowID, "valid") res, err := c.PostForm(action, url.Values{"method": {node.LoginAndLinkCredentials}}) require.NoError(t, err, action) body, err := io.ReadAll(res.Body) require.NoError(t, res.Body.Close()) require.NoError(t, err) aue(t, res, body, "New credentials will be linked to existing account after login.") - loginFlow, err = reg.LoginFlowPersister().GetLoginFlow(context.Background(), uuid.FromStringOrNil(gjson.GetBytes(body, "id").String())) + loginFlow, err := reg.LoginFlowPersister().GetLoginFlow(context.Background(), uuid.FromStringOrNil(gjson.GetBytes(body, "id").String())) assert.NotNil(t, loginFlow, "%s", body) - }) - - t.Run("case=should fail login if existing identity identifier doesn't match", func(t *testing.T) { - res, err := c.PostForm(loginFlow.UI.Action, url.Values{ - "csrf_token": {loginFlow.CSRFToken}, - "method": {"password"}, - "identifier": {subject2}, - "password": {password}}) - require.NoError(t, err, loginFlow.UI.Action) - body, err := io.ReadAll(res.Body) - require.NoError(t, res.Body.Close()) - require.NoError(t, err) - assert.Equal(t, strconv.Itoa(int(text.ErrorValidationLoginLinkedCredentialsDoNotMatch)), gjson.GetBytes(body, "ui.messages.0.id").String(), "%s", body) - }) + return loginFlow + } - t.Run("case=should link oidc credentials to existing identity", func(t *testing.T) { - res, err := c.PostForm(loginFlow.UI.Action, url.Values{ - "csrf_token": {loginFlow.CSRFToken}, - "method": {"password"}, - "identifier": {subject}, - "password": {password}}) - require.NoError(t, err, loginFlow.UI.Action) - body, err := io.ReadAll(res.Body) - require.NoError(t, res.Body.Close()) - require.NoError(t, err) + checkCredentialsLinked := func(res *http.Response, body []byte, identityID uuid.UUID, provider string) { assert.Contains(t, res.Request.URL.String(), returnTS.URL, "%s", body) assert.Equal(t, subject, gjson.GetBytes(body, "identity.traits.subject").String(), "%s", body) - i, err = reg.PrivilegedIdentityPool().GetIdentityConfidential(ctx, i.ID) + i, err := reg.PrivilegedIdentityPool().GetIdentityConfidential(ctx, identityID) require.NoError(t, err) assert.NotEmpty(t, i.Credentials["oidc"], "%+v", i.Credentials) - assert.Equal(t, "valid", gjson.GetBytes(i.Credentials["oidc"].Config, "providers.0.provider").String(), + assert.Equal(t, provider, gjson.GetBytes(i.Credentials["oidc"].Config, "providers.0.provider").String(), "%s", string(i.Credentials["oidc"].Config[:])) - assert.Equal(t, "oidc", gjson.GetBytes(body, "authentication_methods.0.method").String(), "%s", body) + assert.Contains(t, gjson.GetBytes(body, "authentication_methods").String(), "oidc", "%s", body) + } + + t.Run("case=second login is password", func(t *testing.T) { + subject = "new-login-if-email-exist-with-password-strategy@ory.sh" + subject2 := "new-login-subject2@ory.sh" + scope = []string{"openid"} + password := "lwkj52sdkjf" + + var i *identity.Identity + t.Run("case=create password identity", func(t *testing.T) { + i = identity.NewIdentity(config.DefaultIdentityTraitsSchemaID) + p, err := reg.Hasher(ctx).Generate(ctx, []byte(password)) + require.NoError(t, err) + i.SetCredentials(identity.CredentialsTypePassword, identity.Credentials{ + Identifiers: []string{subject}, + Config: sqlxx.JSONRawMessage(`{"hashed_password":"` + string(p) + `"}`), + }) + i.Traits = identity.Traits(`{"subject":"` + subject + `"}`) + require.NoError(t, reg.PrivilegedIdentityPool().CreateIdentity(context.Background(), i)) + + i2 := identity.NewIdentity(config.DefaultIdentityTraitsSchemaID) + i2.SetCredentials(identity.CredentialsTypePassword, identity.Credentials{ + Identifiers: []string{subject2}, + Config: sqlxx.JSONRawMessage(`{"hashed_password":"` + string(p) + `"}`), + }) + i2.Traits = identity.Traits(`{"subject":"` + subject2 + `"}`) + require.NoError(t, reg.PrivilegedIdentityPool().CreateIdentity(context.Background(), i2)) + }) + + c := testhelpers.NewClientWithCookieJar(t, nil, false) + r := newLoginFlow(t, returnTS.URL, time.Minute) + t.Run("case=should fail login", func(t *testing.T) { + res, body := loginWithOIDC(t, c, r.ID, "valid") + aue(t, res, body, "An account with the same identifier (email, phone, username, ...) exists already.") + assert.Equal(t, node.LoginAndLinkCredentials, gjson.GetBytes(body, "ui.nodes.#(attributes.name==\"method\").attributes.value").String(), "%s", body) + }) + + var loginFlow *login.Flow + t.Run("case=should start new login flow", func(t *testing.T) { + loginFlow = stratNewLoginFlowForLinking(t, c, r.ID) + }) + + t.Run("case=should fail login if existing identity identifier doesn't match", func(t *testing.T) { + res, err := c.PostForm(loginFlow.UI.Action, url.Values{ + "csrf_token": {loginFlow.CSRFToken}, + "method": {"password"}, + "identifier": {subject2}, + "password": {password}}) + require.NoError(t, err, loginFlow.UI.Action) + body, err := io.ReadAll(res.Body) + require.NoError(t, res.Body.Close()) + require.NoError(t, err) + assert.Equal(t, strconv.Itoa(int(text.ErrorValidationLoginLinkedCredentialsDoNotMatch)), gjson.GetBytes(body, "ui.messages.0.id").String(), "%s", body) + }) + + t.Run("case=should link oidc credentials to existing identity", func(t *testing.T) { + res, err := c.PostForm(loginFlow.UI.Action, url.Values{ + "csrf_token": {loginFlow.CSRFToken}, + "method": {"password"}, + "identifier": {subject}, + "password": {password}}) + require.NoError(t, err, loginFlow.UI.Action) + body, err := io.ReadAll(res.Body) + require.NoError(t, res.Body.Close()) + require.NoError(t, err) + checkCredentialsLinked(res, body, i.ID, "valid") + }) + }) + + t.Run("case=second login is OIDC", func(t *testing.T) { + email1 := "existing-oidc-identity-1@ory.sh" + email2 := "existing-oidc-identity-2@ory.sh" + scope = []string{"openid", "offline"} + + var identityID uuid.UUID + t.Run("case=create OIDC identity", func(t *testing.T) { + subject = email1 + r := newRegistrationFlow(t, returnTS.URL, time.Minute) + action := afv(t, r.ID, "secondProvider") + res, body := makeRequest(t, "secondProvider", action, url.Values{}) + ai(t, res, body) + identityID = expectTokens(t, "secondProvider", body) + + subject = email2 + r = newRegistrationFlow(t, returnTS.URL, time.Minute) + action = afv(t, r.ID, "valid") + res, body = makeRequest(t, "valid", action, url.Values{}) + ai(t, res, body) + expectTokens(t, "valid", body) + }) + + subject = email1 + c := testhelpers.NewClientWithCookieJar(t, nil, false) + r := newLoginFlow(t, returnTS.URL, time.Minute) + t.Run("case=should fail login", func(t *testing.T) { + res, body := loginWithOIDC(t, c, r.ID, "valid") + aue(t, res, body, "An account with the same identifier (email, phone, username, ...) exists already.") + assert.Equal(t, node.LoginAndLinkCredentials, gjson.GetBytes(body, "ui.nodes.#(attributes.name==\"method\").attributes.value").String(), "%s", body) + }) + + var loginFlow *login.Flow + t.Run("case=should start new login flow", func(t *testing.T) { + loginFlow = stratNewLoginFlowForLinking(t, c, r.ID) + }) + + subject = email2 + t.Run("case=should fail login if existing identity identifier doesn't match", func(t *testing.T) { + res, body := loginWithOIDC(t, c, loginFlow.ID, "valid") + aue(t, res, body, "Linked credentials do not match.") + }) + + subject = email1 + t.Run("case=should link oidc credentials to existing identity", func(t *testing.T) { + res, body := loginWithOIDC(t, c, loginFlow.ID, "secondProvider") + checkCredentialsLinked(res, body, identityID, "secondProvider") + }) }) }) From f7c76beba5d564595abefd76661873bd23a25639 Mon Sep 17 00:00:00 2001 From: Henning Perl Date: Mon, 9 Oct 2023 12:50:05 +0200 Subject: [PATCH 07/29] chore: format --- selfservice/flow/login/handler.go | 5 +++-- selfservice/flow/login/hook.go | 8 +++++--- selfservice/flow/login/strategy.go | 3 ++- selfservice/flow/registration/flow.go | 18 ++++++------------ selfservice/flow/registration/handler.go | 2 +- text/id.go | 20 ++++++++++---------- ui/node/identifiers.go | 2 +- 7 files changed, 28 insertions(+), 30 deletions(-) diff --git a/selfservice/flow/login/handler.go b/selfservice/flow/login/handler.go index 65b619da340d..bc48599ce67b 100644 --- a/selfservice/flow/login/handler.go +++ b/selfservice/flow/login/handler.go @@ -6,12 +6,13 @@ package login import ( _ "embed" "encoding/json" - "github.com/tidwall/gjson" - "github.com/tidwall/sjson" "net/http" "net/url" "time" + "github.com/tidwall/gjson" + "github.com/tidwall/sjson" + "github.com/gofrs/uuid" "github.com/julienschmidt/httprouter" "github.com/pkg/errors" diff --git a/selfservice/flow/login/hook.go b/selfservice/flow/login/hook.go index 4bb235bd719d..abf1d84848e0 100644 --- a/selfservice/flow/login/hook.go +++ b/selfservice/flow/login/hook.go @@ -7,12 +7,14 @@ import ( "context" "encoding/json" "fmt" + "net/http" + "time" + "github.com/gofrs/uuid" + "github.com/tidwall/gjson" + "github.com/ory/kratos/schema" "github.com/ory/x/decoderx" - "github.com/tidwall/gjson" - "net/http" - "time" "go.opentelemetry.io/otel/trace" diff --git a/selfservice/flow/login/strategy.go b/selfservice/flow/login/strategy.go index 551104850780..69deadc908ad 100644 --- a/selfservice/flow/login/strategy.go +++ b/selfservice/flow/login/strategy.go @@ -5,9 +5,10 @@ package login import ( "context" - "github.com/ory/x/sqlxx" "net/http" + "github.com/ory/x/sqlxx" + "github.com/gofrs/uuid" "github.com/ory/kratos/session" diff --git a/selfservice/flow/registration/flow.go b/selfservice/flow/registration/flow.go index eac679e33071..1efa565714e5 100644 --- a/selfservice/flow/registration/flow.go +++ b/selfservice/flow/registration/flow.go @@ -6,31 +6,25 @@ package registration import ( "context" "encoding/json" - "github.com/tidwall/sjson" "net/http" "net/url" "time" "github.com/gobuffalo/pop/v6" - + "github.com/gofrs/uuid" + "github.com/pkg/errors" "github.com/tidwall/gjson" - - "github.com/ory/x/sqlxx" + "github.com/tidwall/sjson" hydraclientgo "github.com/ory/hydra-client-go/v2" - "github.com/ory/kratos/driver/config" "github.com/ory/kratos/hydra" - "github.com/ory/kratos/ui/container" - - "github.com/gofrs/uuid" - "github.com/pkg/errors" - - "github.com/ory/x/urlx" - "github.com/ory/kratos/identity" "github.com/ory/kratos/selfservice/flow" + "github.com/ory/kratos/ui/container" "github.com/ory/kratos/x" + "github.com/ory/x/sqlxx" + "github.com/ory/x/urlx" ) // swagger:model registrationFlow diff --git a/selfservice/flow/registration/handler.go b/selfservice/flow/registration/handler.go index 131cbbb09328..8fec5faf988c 100644 --- a/selfservice/flow/registration/handler.go +++ b/selfservice/flow/registration/handler.go @@ -120,7 +120,7 @@ func WithFlowOAuth2LoginChallenge(loginChallenge string) FlowOption { func WithOuterFlow(flowId uuid.UUID) FlowOption { return func(f *Flow) { - f.SetOuterLoginFlowID(flowId) + _ = f.SetOuterLoginFlowID(flowId) } } diff --git a/text/id.go b/text/id.go index 0f1e5d8a1a7a..c5068f9ad3ca 100644 --- a/text/id.go +++ b/text/id.go @@ -140,16 +140,16 @@ const ( ) const ( - ErrorValidationLogin ID = 4010000 + iota // 4010000 - ErrorValidationLoginFlowExpired // 4010001 - ErrorValidationLoginNoStrategyFound // 4010002 - ErrorValidationRegistrationNoStrategyFound // 4010003 - ErrorValidationSettingsNoStrategyFound // 4010004 - ErrorValidationRecoveryNoStrategyFound // 4010005 - ErrorValidationVerificationNoStrategyFound // 4010006 - ErrorValidationLoginRetrySuccess // 4010007 - ErrorValidationLoginCodeInvalidOrAlreadyUsed // 4010008 - ErrorValidationLoginLinkedCredentialsDoNotMatch // 4010009 + ErrorValidationLogin ID = 4010000 + iota // 4010000 + ErrorValidationLoginFlowExpired // 4010001 + ErrorValidationLoginNoStrategyFound // 4010002 + ErrorValidationRegistrationNoStrategyFound // 4010003 + ErrorValidationSettingsNoStrategyFound // 4010004 + ErrorValidationRecoveryNoStrategyFound // 4010005 + ErrorValidationVerificationNoStrategyFound // 4010006 + ErrorValidationLoginRetrySuccess // 4010007 + ErrorValidationLoginCodeInvalidOrAlreadyUsed // 4010008 + ErrorValidationLoginLinkedCredentialsDoNotMatch // 4010009 ) const ( diff --git a/ui/node/identifiers.go b/ui/node/identifiers.go index c07ba0c46022..c5c310fbf387 100644 --- a/ui/node/identifiers.go +++ b/ui/node/identifiers.go @@ -30,5 +30,5 @@ const ( ) const ( - LoginAndLinkCredentials = "login_and_link_credentials" + LoginAndLinkCredentials = "login_and_link_credentials" // #nosec G101 ) From bc0bcb3e8c08c325e87f148b7aa8036f2e5527b7 Mon Sep 17 00:00:00 2001 From: Henning Perl Date: Tue, 10 Oct 2023 11:05:48 +0200 Subject: [PATCH 08/29] fix: tests Signed-off-by: Henning Perl --- persistence/sql/migratest/migration_test.go | 5 ++- selfservice/strategy/oidc/strategy.go | 24 +++++----- selfservice/strategy/oidc/strategy_test.go | 49 ++++++++++++--------- 3 files changed, 44 insertions(+), 34 deletions(-) diff --git a/persistence/sql/migratest/migration_test.go b/persistence/sql/migratest/migration_test.go index 798afd54dc79..79704c868800 100644 --- a/persistence/sql/migratest/migration_test.go +++ b/persistence/sql/migratest/migration_test.go @@ -102,7 +102,10 @@ func TestMigrations_Cockroach(t *testing.T) { t.Skip("skipping testing in short mode") } t.Parallel() - testDatabase(t, "cockroach", dockertest.ConnectToTestCockroachDBPop(t)) + url := dockertest.RunTestCockroachDBWithVersion(t, "v22.2.14") + conn := dockertest.ConnectPop(t, url) + + testDatabase(t, "cockroach", conn) } func testDatabase(t *testing.T, db string, c *pop.Connection) { diff --git a/selfservice/strategy/oidc/strategy.go b/selfservice/strategy/oidc/strategy.go index 04e3abe0ed74..825d381e1bfb 100644 --- a/selfservice/strategy/oidc/strategy.go +++ b/selfservice/strategy/oidc/strategy.go @@ -543,18 +543,6 @@ func (s *Strategy) handleError(w http.ResponseWriter, r *http.Request, f flow.Fl // Reset all nodes to not confuse users. // This is kinda hacky and will probably need to be updated at some point. - if dup := new(identity.ErrDuplicateCredentials); errors.As(err, &dup) { - rf.UI.Messages.Add(text.NewErrorValidationDuplicateCredentialsOnOIDCLink()) - lf, err := s.registrationToLogin(w, r, rf, provider) - if err != nil { - return err - } - // return a new login flow with the error message embedded in the login flow. - x.AcceptToRedirectOrJSON(w, r, s.d.Writer(), lf, lf.AppendTo(s.d.Config().SelfServiceFlowLoginUI(r.Context())).String()) - // ensure the function does not continue to execute - return registration.ErrHookAbortFlow - } - var loginAndLinkCredentialsNode *node.Node for _, n := range rf.UI.Nodes { if n.ID() == "method" && n.GetValue() == node.LoginAndLinkCredentials { @@ -567,6 +555,18 @@ func (s *Strategy) handleError(w http.ResponseWriter, r *http.Request, f flow.Fl rf.UI.Nodes.Upsert(loginAndLinkCredentialsNode) } + //if dup := new(identity.ErrDuplicateCredentials); errors.As(err, &dup) { + // rf.UI.Messages.Add(text.NewErrorValidationDuplicateCredentialsOnOIDCLink()) + // lf, err := s.registrationToLogin(w, r, rf, provider) + // if err != nil { + // return err + // } + // // return a new login flow with the error message embedded in the login flow. + // x.AcceptToRedirectOrJSON(w, r, s.d.Writer(), lf, lf.AppendTo(s.d.Config().SelfServiceFlowLoginUI(r.Context())).String()) + // // ensure the function does not continue to execute + // return registration.ErrHookAbortFlow + //} + // Adds the "Continue" button rf.UI.SetCSRF(s.d.GenerateCSRFToken(r)) AddProvider(rf.UI, provider, text.NewInfoRegistrationContinue()) diff --git a/selfservice/strategy/oidc/strategy_test.go b/selfservice/strategy/oidc/strategy_test.go index c2c2bd56cb3f..77179645b035 100644 --- a/selfservice/strategy/oidc/strategy_test.go +++ b/selfservice/strategy/oidc/strategy_test.go @@ -222,8 +222,8 @@ func TestStrategy(t *testing.T) { var assertSystemErrorWithReason = func(t *testing.T, res *http.Response, body []byte, code int, reason string) { require.Contains(t, res.Request.URL.String(), errTS.URL, "%s", body) - assert.Equal(t, int64(code), gjson.GetBytes(body, "code").Int(), "%s", body) - assert.Contains(t, gjson.GetBytes(body, "reason").String(), reason, "%s", body) + assert.Equal(t, int64(code), gjson.GetBytes(body, "code").Int(), "%s", prettyJSON(t, body)) + assert.Contains(t, gjson.GetBytes(body, "reason").String(), reason, "%s", prettyJSON(t, body)) } // assert system error (redirect to error endpoint) @@ -237,15 +237,15 @@ func TestStrategy(t *testing.T) { // assert ui error (redirect to login/registration ui endpoint) var assertUIError = func(t *testing.T, res *http.Response, body []byte, reason string) { require.Contains(t, res.Request.URL.String(), uiTS.URL, "status: %d, body: %s", res.StatusCode, body) - assert.Contains(t, gjson.GetBytes(body, "ui.messages.0.text").String(), reason, "%s", body) + assert.Contains(t, gjson.GetBytes(body, "ui.messages.0.text").String(), reason, "%s", prettyJSON(t, body)) } // assert identity (success) var assertIdentity = func(t *testing.T, res *http.Response, body []byte) { assert.Contains(t, res.Request.URL.String(), returnTS.URL, "%s", body) - assert.Equal(t, subject, gjson.GetBytes(body, "identity.traits.subject").String(), "%s", body) - assert.Equal(t, claims.traits.website, gjson.GetBytes(body, "identity.traits.website").String(), "%s", body) - assert.Equal(t, claims.metadataPublic.picture, gjson.GetBytes(body, "identity.metadata_public.picture").String(), "%s", body) + assert.Equal(t, subject, gjson.GetBytes(body, "identity.traits.subject").String(), "%s", prettyJSON(t, body)) + assert.Equal(t, claims.traits.website, gjson.GetBytes(body, "identity.traits.website").String(), "%s", prettyJSON(t, body)) + assert.Equal(t, claims.metadataPublic.picture, gjson.GetBytes(body, "identity.metadata_public.picture").String(), "%s", prettyJSON(t, body)) } var newLoginFlow = func(t *testing.T, requestURL string, exp time.Duration, flowType flow.Type) (req *login.Flow) { @@ -886,8 +886,8 @@ func TestStrategy(t *testing.T) { r := newBrowserRegistrationFlow(t, returnTS.URL, time.Minute) action := assertFormValues(t, r.ID, "valid") res, body := makeRequest(t, "valid", action, url.Values{}) - assertUIError(t, res, body, "An account with the same identifier (email, phone, username, ...) exists already. Please sign in to your existing account and link your social profile in the settings page.") - require.Contains(t, gjson.GetBytes(body, "ui.action").String(), "/self-service/login") + assertUIError(t, res, body, "An account with the same identifier (email, phone, username, ...) exists already.") + require.Contains(t, gjson.GetBytes(body, "ui.action").String(), "/self-service/registration") }) t.Run("case=should fail login", func(t *testing.T) { @@ -1138,39 +1138,39 @@ func TestStrategy(t *testing.T) { require.NoError(t, reg.PrivilegedIdentityPool().CreateIdentity(context.Background(), i2)) }) - c := testhelpers.NewClientWithCookieJar(t, nil, false) - r := newLoginFlow(t, returnTS.URL, time.Minute, flow.TypeBrowser) + client := testhelpers.NewClientWithCookieJar(t, nil, false) + loginFlow := newLoginFlow(t, returnTS.URL, time.Minute, flow.TypeBrowser) t.Run("case=should fail login", func(t *testing.T) { - res, body := loginWithOIDC(t, c, r.ID, "valid") + res, body := loginWithOIDC(t, client, loginFlow.ID, "valid") assertUIError(t, res, body, "An account with the same identifier (email, phone, username, ...) exists already.") - assert.Equal(t, node.LoginAndLinkCredentials, gjson.GetBytes(body, "ui.nodes.#(attributes.name==\"method\").attributes.value").String(), "%s", body) + assert.Equal(t, node.LoginAndLinkCredentials, gjson.GetBytes(body, "ui.nodes.#(attributes.name==\"method\").attributes.value").String(), "%s", prettyJSON(t, body)) }) - var loginFlow *login.Flow + var linkingLoginFlow *login.Flow t.Run("case=should start new login flow", func(t *testing.T) { - loginFlow = stratNewLoginFlowForLinking(t, c, r.ID) + linkingLoginFlow = stratNewLoginFlowForLinking(t, client, loginFlow.ID) }) t.Run("case=should fail login if existing identity identifier doesn't match", func(t *testing.T) { - res, err := c.PostForm(loginFlow.UI.Action, url.Values{ - "csrf_token": {loginFlow.CSRFToken}, + res, err := client.PostForm(linkingLoginFlow.UI.Action, url.Values{ + "csrf_token": {linkingLoginFlow.CSRFToken}, "method": {"password"}, "identifier": {subject2}, "password": {password}}) - require.NoError(t, err, loginFlow.UI.Action) + require.NoError(t, err, linkingLoginFlow.UI.Action) body, err := io.ReadAll(res.Body) require.NoError(t, res.Body.Close()) require.NoError(t, err) - assert.Equal(t, strconv.Itoa(int(text.ErrorValidationLoginLinkedCredentialsDoNotMatch)), gjson.GetBytes(body, "ui.messages.0.id").String(), "%s", body) + assert.Equal(t, strconv.Itoa(int(text.ErrorValidationLoginLinkedCredentialsDoNotMatch)), gjson.GetBytes(body, "ui.messages.0.id").String(), "%s", prettyJSON(t, body)) }) t.Run("case=should link oidc credentials to existing identity", func(t *testing.T) { - res, err := c.PostForm(loginFlow.UI.Action, url.Values{ - "csrf_token": {loginFlow.CSRFToken}, + res, err := client.PostForm(linkingLoginFlow.UI.Action, url.Values{ + "csrf_token": {linkingLoginFlow.CSRFToken}, "method": {"password"}, "identifier": {subject}, "password": {password}}) - require.NoError(t, err, loginFlow.UI.Action) + require.NoError(t, err, linkingLoginFlow.UI.Action) body, err := io.ReadAll(res.Body) require.NoError(t, res.Body.Close()) require.NoError(t, err) @@ -1249,6 +1249,13 @@ func TestStrategy(t *testing.T) { }) } +func prettyJSON(t *testing.T, body []byte) string { + var out bytes.Buffer + require.NoError(t, json.Indent(&out, body, "", "\t")) + + return out.String() +} + func TestCountActiveFirstFactorCredentials(t *testing.T) { _, reg := internal.NewFastRegistryWithMocks(t) strategy := oidc.NewStrategy(reg) From 93f4168770ca3ad6f60cda573a026dd03dab7274 Mon Sep 17 00:00:00 2001 From: Henning Perl Date: Fri, 20 Oct 2023 11:49:29 +0200 Subject: [PATCH 09/29] cleanup --- selfservice/flow/login/flow.go | 11 +++ selfservice/flow/login/handler.go | 33 ++++---- selfservice/flow/login/hook.go | 85 +++++++-------------- selfservice/flow/registration/hook.go | 1 - selfservice/strategy/oidc/strategy.go | 12 --- selfservice/strategy/oidc/strategy_test.go | 7 +- selfservice/strategy/password/login_test.go | 23 ------ 7 files changed, 55 insertions(+), 117 deletions(-) diff --git a/selfservice/flow/login/flow.go b/selfservice/flow/login/flow.go index 1bd95c7b1fad..e10a51c1789b 100644 --- a/selfservice/flow/login/flow.go +++ b/selfservice/flow/login/flow.go @@ -231,6 +231,17 @@ func (f *Flow) EnsureInternalContext() { } } +func (f *Flow) duplicateCredentials() (*flow.RegistrationDuplicateCredentials, error) { + raw := gjson.GetBytes(f.InternalContext, flow.InternalContextLinkCredentialsPath) + if !raw.IsObject() { + return nil, nil + } + var creds flow.RegistrationDuplicateCredentials + err := json.Unmarshal([]byte(raw.Raw), &creds) + + return &creds, err +} + func (f Flow) MarshalJSON() ([]byte, error) { type local Flow f.SetReturnTo() diff --git a/selfservice/flow/login/handler.go b/selfservice/flow/login/handler.go index bc48599ce67b..aec61157591f 100644 --- a/selfservice/flow/login/handler.go +++ b/selfservice/flow/login/handler.go @@ -10,12 +10,11 @@ import ( "net/url" "time" - "github.com/tidwall/gjson" - "github.com/tidwall/sjson" - "github.com/gofrs/uuid" "github.com/julienschmidt/httprouter" "github.com/pkg/errors" + "github.com/tidwall/gjson" + "github.com/tidwall/sjson" "github.com/ory/herodot" hydraclientgo "github.com/ory/hydra-client-go/v2" @@ -37,9 +36,6 @@ import ( "github.com/ory/x/urlx" ) -//go:embed .schema/link_credentials.schema.json -var linkCredentialsSchema []byte - const ( RouteInitBrowserFlow = "/self-service/login/browser" RouteInitAPIFlow = "/self-service/login/api" @@ -782,26 +778,25 @@ continueLogin: internalContextDuplicateCredentials := gjson.GetBytes(f.InternalContext, flow.InternalContextDuplicateCredentialsPath) if internalContextDuplicateCredentials.IsObject() { - // If return_to was set before, we need to preserve it. - var opts []FlowOption - if len(f.ReturnTo) > 0 { - opts = append(opts, WithFlowReturnTo(f.ReturnTo)) + var linkCredentials flow.RegistrationDuplicateCredentials + if err := json.Unmarshal([]byte(internalContextDuplicateCredentials.Raw), &linkCredentials); err != nil { + h.d.LoginFlowErrorHandler().WriteFlowError(w, r, f, node.DefaultGroup, err) + return } - opts = append(opts, func(newFlow *Flow) { + + loginFlow, _, err := h.NewLoginFlow(w, r, f.Type, func(newFlow *Flow) { + newFlow.ReturnTo = f.ReturnTo + newFlow.HydraLoginRequest = f.HydraLoginRequest + newFlow.OAuth2LoginChallenge = f.OAuth2LoginChallenge + newFlow.OrganizationID = f.OrganizationID newFlow.UI.Messages.Add(text.NewInfoSelfServiceLoginLinkCredentials()) - var linkCredentials flow.RegistrationDuplicateCredentials - if err := json.Unmarshal([]byte(internalContextDuplicateCredentials.Raw), &linkCredentials); err != nil { - h.d.LoginFlowErrorHandler().WriteFlowError(w, r, f, node.DefaultGroup, err) - return - } - newFlow.InternalContext, err = sjson.SetBytes(newFlow.InternalContext, flow.InternalContextLinkCredentialsPath, - linkCredentials) + newFlow.InternalContext, err = sjson.SetBytes( + newFlow.InternalContext, flow.InternalContextLinkCredentialsPath, linkCredentials) if err != nil { h.d.LoginFlowErrorHandler().WriteFlowError(w, r, f, node.DefaultGroup, err) return } }) - loginFlow, _, err := h.NewLoginFlow(w, r, flow.TypeBrowser, opts...) if err != nil { h.d.LoginFlowErrorHandler().WriteFlowError(w, r, f, node.DefaultGroup, err) return diff --git a/selfservice/flow/login/hook.go b/selfservice/flow/login/hook.go index abf1d84848e0..baee3c36951f 100644 --- a/selfservice/flow/login/hook.go +++ b/selfservice/flow/login/hook.go @@ -13,11 +13,10 @@ import ( "github.com/gofrs/uuid" "github.com/tidwall/gjson" - "github.com/ory/kratos/schema" - "github.com/ory/x/decoderx" - "go.opentelemetry.io/otel/trace" + "github.com/ory/kratos/schema" + "github.com/ory/kratos/x/events" "github.com/pkg/errors" @@ -138,7 +137,7 @@ func (e *HookExecutor) PostLoginHook( r = r.WithContext(ctx) defer otelx.End(span, &err) - if err := e.linkCredentials(r, s, i, a); err != nil { + if err := e.maybeLinkCredentials(r, s, i, a); err != nil { return err } @@ -316,66 +315,36 @@ func (e *HookExecutor) PreLoginHook(w http.ResponseWriter, r *http.Request, a *F return nil } -func (e *HookExecutor) linkCredentials(r *http.Request, s *session.Session, i *identity.Identity, f *Flow) error { - var lc flow.RegistrationDuplicateCredentials - - if r.Method == "POST" { - var p struct { - FlowID string `json:"linkCredentialsFlow" form:"linkCredentialsFlow"` - } - - if err := decoderx.NewHTTP().Decode(r, &p, - decoderx.HTTPDecoderSetValidatePayloads(true), - decoderx.MustHTTPRawJSONSchemaCompiler(linkCredentialsSchema), - decoderx.HTTPDecoderJSONFollowsFormFormat()); err != nil { - return err - } - - if p.FlowID != "" { - linkCredentialsFlowID, innerErr := uuid.FromString(p.FlowID) - if innerErr != nil { - return innerErr - } - linkCredentialsFlow, innerErr := e.d.LoginFlowPersister().GetLoginFlow(r.Context(), linkCredentialsFlowID) - if innerErr != nil { - return innerErr - } - innerErr = e.getInternalContextLinkCredentials(linkCredentialsFlow, flow.InternalContextDuplicateCredentialsPath, &lc) - if innerErr != nil { - return innerErr - } - } +// maybeLinkCredentials links the identity with the credentials of the inner context of the login flow. +func (e *HookExecutor) maybeLinkCredentials(r *http.Request, s *session.Session, i *identity.Identity, f *Flow) error { + lc, err := f.duplicateCredentials() + if err != nil { + return err + } else if lc == nil { + return nil } - if lc.CredentialsType == "" { - err := e.getInternalContextLinkCredentials(f, flow.InternalContextLinkCredentialsPath, &lc) - if err != nil { - return err - } + if err := e.checkDuplicateCredentialsIdentifierMatch(r.Context(), i.ID, lc.DuplicateIdentifier); err != nil { + return err + } + strategy, err := e.d.AllLoginStrategies().Strategy(lc.CredentialsType) + if err != nil { + return err } - if lc.CredentialsType != "" { - if err := e.checkDuplecateCredentialsIdentifierMatch(r.Context(), i.ID, lc.DuplicateIdentifier); err != nil { - return err - } - strategy, err := e.d.AllLoginStrategies().Strategy(lc.CredentialsType) - if err != nil { - return err - } - - linkableStrategy, ok := strategy.(LinkableStrategy) - if !ok { - return errors.New(fmt.Sprintf("Strategy is not linkable: %T", linkableStrategy)) - } - - if err := linkableStrategy.Link(r.Context(), i, lc.CredentialsConfig); err != nil { - return err - } + linkableStrategy, ok := strategy.(LinkableStrategy) + if !ok { + // This should never happen because we check for this in the registration flow. + return fmt.Errorf("strategy is not linkable: %T", linkableStrategy) + } - method := strategy.CompletedAuthenticationMethod(r.Context()) - s.CompletedLoginFor(method.Method, method.AAL) + if err := linkableStrategy.Link(r.Context(), i, lc.CredentialsConfig); err != nil { + return err } + method := strategy.CompletedAuthenticationMethod(r.Context()) + s.CompletedLoginFor(method.Method, method.AAL) + return nil } @@ -389,7 +358,7 @@ func (e *HookExecutor) getInternalContextLinkCredentials(f *Flow, internalContex return nil } -func (e *HookExecutor) checkDuplecateCredentialsIdentifierMatch(ctx context.Context, identityID uuid.UUID, match string) error { +func (e *HookExecutor) checkDuplicateCredentialsIdentifierMatch(ctx context.Context, identityID uuid.UUID, match string) error { i, err := e.d.PrivilegedIdentityPool().GetIdentityConfidential(ctx, identityID) if err != nil { return err diff --git a/selfservice/flow/registration/hook.go b/selfservice/flow/registration/hook.go index 422500e26bfd..73196c4964f9 100644 --- a/selfservice/flow/registration/hook.go +++ b/selfservice/flow/registration/hook.go @@ -194,7 +194,6 @@ func (e *HookExecutor) PostRegistrationHook(w http.ResponseWriter, r *http.Reque if err := e.d.LoginFlowPersister().UpdateLoginFlow(r.Context(), loginFlow); err != nil { return err } - } a.InternalContext, err = sjson.SetBytes(a.InternalContext, flow.InternalContextDuplicateCredentialsPath, diff --git a/selfservice/strategy/oidc/strategy.go b/selfservice/strategy/oidc/strategy.go index 825d381e1bfb..e73be5e21826 100644 --- a/selfservice/strategy/oidc/strategy.go +++ b/selfservice/strategy/oidc/strategy.go @@ -555,18 +555,6 @@ func (s *Strategy) handleError(w http.ResponseWriter, r *http.Request, f flow.Fl rf.UI.Nodes.Upsert(loginAndLinkCredentialsNode) } - //if dup := new(identity.ErrDuplicateCredentials); errors.As(err, &dup) { - // rf.UI.Messages.Add(text.NewErrorValidationDuplicateCredentialsOnOIDCLink()) - // lf, err := s.registrationToLogin(w, r, rf, provider) - // if err != nil { - // return err - // } - // // return a new login flow with the error message embedded in the login flow. - // x.AcceptToRedirectOrJSON(w, r, s.d.Writer(), lf, lf.AppendTo(s.d.Config().SelfServiceFlowLoginUI(r.Context())).String()) - // // ensure the function does not continue to execute - // return registration.ErrHookAbortFlow - //} - // Adds the "Continue" button rf.UI.SetCSRF(s.d.GenerateCSRFToken(r)) AddProvider(rf.UI, provider, text.NewInfoRegistrationContinue()) diff --git a/selfservice/strategy/oidc/strategy_test.go b/selfservice/strategy/oidc/strategy_test.go index 77179645b035..7622b563ecbe 100644 --- a/selfservice/strategy/oidc/strategy_test.go +++ b/selfservice/strategy/oidc/strategy_test.go @@ -1076,7 +1076,6 @@ func TestStrategy(t *testing.T) { }) t.Run("case=registration should start new login flow if duplicate credentials detected", func(t *testing.T) { - loginWithOIDC := func(t *testing.T, c *http.Client, flowID uuid.UUID, provider string) (*http.Response, []byte) { action := assertFormValues(t, flowID, provider) res, err := c.PostForm(action, url.Values{"provider": {provider}}) @@ -1087,7 +1086,7 @@ func TestStrategy(t *testing.T) { return res, body } - stratNewLoginFlowForLinking := func(t *testing.T, c *http.Client, flowID uuid.UUID) *login.Flow { + startNewLoginFlowForLinking := func(t *testing.T, c *http.Client, flowID uuid.UUID) *login.Flow { action := assertFormValues(t, flowID, "valid") res, err := c.PostForm(action, url.Values{"method": {node.LoginAndLinkCredentials}}) require.NoError(t, err, action) @@ -1148,7 +1147,7 @@ func TestStrategy(t *testing.T) { var linkingLoginFlow *login.Flow t.Run("case=should start new login flow", func(t *testing.T) { - linkingLoginFlow = stratNewLoginFlowForLinking(t, client, loginFlow.ID) + linkingLoginFlow = startNewLoginFlowForLinking(t, client, loginFlow.ID) }) t.Run("case=should fail login if existing identity identifier doesn't match", func(t *testing.T) { @@ -1211,7 +1210,7 @@ func TestStrategy(t *testing.T) { var loginFlow *login.Flow t.Run("case=should start new login flow", func(t *testing.T) { - loginFlow = stratNewLoginFlowForLinking(t, c, r.ID) + loginFlow = startNewLoginFlowForLinking(t, c, r.ID) }) subject = email2 diff --git a/selfservice/strategy/password/login_test.go b/selfservice/strategy/password/login_test.go index 5ad11b44c426..c6357ff6920f 100644 --- a/selfservice/strategy/password/login_test.go +++ b/selfservice/strategy/password/login_test.go @@ -466,29 +466,6 @@ func TestCompleteLogin(t *testing.T) { }) }) - t.Run("case=should return an error because the linkCredentialsFlow does not exist", func(t *testing.T) { - identifier, pwd := x.NewUUID().String(), "password" - createIdentity(ctx, reg, t, identifier, pwd) - - var check = func(t *testing.T, actual string) { - assert.Equal(t, int64(http.StatusNotFound), gjson.Get(actual, "code").Int(), "%s", actual) - assert.Equal(t, "Not Found", gjson.Get(actual, "status").String(), "%s", actual) - assert.Contains(t, gjson.Get(actual, "message").String(), "Unable to locate the resource", "%s", actual) - } - - var values = func(v url.Values) { - v.Set("identifier", identifier) - v.Set("password", pwd) - v.Set("linkCredentialsFlow", "00000000-0000-0000-0000-000000000000") - } - - t.Run("type=api", func(t *testing.T) { - actual := testhelpers.SubmitLoginForm(t, true, nil, publicTS, values, - false, false, http.StatusNotFound, publicTS.URL+login.RouteSubmitFlow) - check(t, gjson.Get(actual, "error").Raw) - }) - }) - t.Run("should pass with real request", func(t *testing.T) { identifier, pwd := x.NewUUID().String(), "password" createIdentity(ctx, reg, t, identifier, pwd) From 76fa5b05e1011c382d38ec19f3c2eddeafe9d8bb Mon Sep 17 00:00:00 2001 From: Henning Perl Date: Mon, 23 Oct 2023 10:47:48 +0200 Subject: [PATCH 10/29] fix: simplify linking flow --- .../flow/login/.schema/link_credentials.schema.json | 10 ---------- 1 file changed, 10 deletions(-) delete mode 100644 selfservice/flow/login/.schema/link_credentials.schema.json diff --git a/selfservice/flow/login/.schema/link_credentials.schema.json b/selfservice/flow/login/.schema/link_credentials.schema.json deleted file mode 100644 index 25d4a8855e4f..000000000000 --- a/selfservice/flow/login/.schema/link_credentials.schema.json +++ /dev/null @@ -1,10 +0,0 @@ -{ - "$id": "https://schemas.ory.sh/kratos/selfservice/flow/link_credentials.schema.json", - "$schema": "http://json-schema.org/draft-07/schema#", - "type": "object", - "properties": { - "linkCredentialsFlow": { - "type": "string" - } - } -} From 5cb2ea88f068b27af15aea5ec8093ebba4bcec7a Mon Sep 17 00:00:00 2001 From: Henning Perl Date: Mon, 23 Oct 2023 11:20:17 +0200 Subject: [PATCH 11/29] fix: cleanup --- cmd/clidoc/main.go | 2 +- selfservice/flow/flow.go | 1 - selfservice/flow/login/flow.go | 2 +- selfservice/flow/login/handler.go | 55 ++++++------ selfservice/flow/registration/handler.go | 34 -------- selfservice/flow/registration/hook.go | 29 +------ selfservice/strategy/oidc/strategy.go | 18 ++-- .../strategy/oidc/strategy_registration.go | 4 + selfservice/strategy/oidc/strategy_test.go | 84 +++++++++---------- text/id.go | 1 + text/message_login.go | 8 -- text/message_node.go | 8 ++ text/message_validation.go | 5 +- 13 files changed, 97 insertions(+), 154 deletions(-) diff --git a/cmd/clidoc/main.go b/cmd/clidoc/main.go index d94cf72eec31..3821f26b0a62 100644 --- a/cmd/clidoc/main.go +++ b/cmd/clidoc/main.go @@ -144,6 +144,7 @@ func init() { "NewErrorValidationRecoveryStateFailure": text.NewErrorValidationRecoveryStateFailure(), "NewInfoNodeInputEmail": text.NewInfoNodeInputEmail(), "NewInfoNodeResendOTP": text.NewInfoNodeResendOTP(), + "NewInfoNodeLoginAndLinkCredential": text.NewInfoNodeLoginAndLinkCredential(), "NewInfoNodeLabelContinue": text.NewInfoNodeLabelContinue(), "NewInfoSelfServiceSettingsRegisterWebAuthn": text.NewInfoSelfServiceSettingsRegisterWebAuthn(), "NewInfoLoginWebAuthnPasswordless": text.NewInfoLoginWebAuthnPasswordless(), @@ -163,7 +164,6 @@ func init() { "NewInfoSelfServiceLoginCode": text.NewInfoSelfServiceLoginCode(), "NewErrorValidationRegistrationRetrySuccessful": text.NewErrorValidationRegistrationRetrySuccessful(), "NewInfoSelfServiceRegistrationRegisterCode": text.NewInfoSelfServiceRegistrationRegisterCode(), - "NewInfoSelfServiceLoginLinkCredentials": text.NewInfoSelfServiceLoginLinkCredentials(), "NewErrorValidationLoginLinkedCredentialsDoNotMatch": text.NewErrorValidationLoginLinkedCredentialsDoNotMatch(), } } diff --git a/selfservice/flow/flow.go b/selfservice/flow/flow.go index 32ae69404877..14a51d4dbdc4 100644 --- a/selfservice/flow/flow.go +++ b/selfservice/flow/flow.go @@ -21,7 +21,6 @@ import ( ) const InternalContextDuplicateCredentialsPath = "registration_duplicate_credentials" -const InternalContextLinkCredentialsPath = "link_credentials" type RegistrationDuplicateCredentials struct { CredentialsType identity.CredentialsType diff --git a/selfservice/flow/login/flow.go b/selfservice/flow/login/flow.go index e10a51c1789b..2229b1fc08a2 100644 --- a/selfservice/flow/login/flow.go +++ b/selfservice/flow/login/flow.go @@ -232,7 +232,7 @@ func (f *Flow) EnsureInternalContext() { } func (f *Flow) duplicateCredentials() (*flow.RegistrationDuplicateCredentials, error) { - raw := gjson.GetBytes(f.InternalContext, flow.InternalContextLinkCredentialsPath) + raw := gjson.GetBytes(f.InternalContext, flow.InternalContextDuplicateCredentialsPath) if !raw.IsObject() { return nil, nil } diff --git a/selfservice/flow/login/handler.go b/selfservice/flow/login/handler.go index aec61157591f..e0675ed50dfb 100644 --- a/selfservice/flow/login/handler.go +++ b/selfservice/flow/login/handler.go @@ -5,7 +5,6 @@ package login import ( _ "embed" - "encoding/json" "net/http" "net/url" "time" @@ -13,8 +12,6 @@ import ( "github.com/gofrs/uuid" "github.com/julienschmidt/httprouter" "github.com/pkg/errors" - "github.com/tidwall/gjson" - "github.com/tidwall/sjson" "github.com/ory/herodot" hydraclientgo "github.com/ory/hydra-client-go/v2" @@ -104,6 +101,12 @@ func WithFlowReturnTo(returnTo string) FlowOption { } } +func WithInternalContext(internalContext []byte) FlowOption { + return func(f *Flow) { + f.InternalContext = internalContext + } +} + func WithFormErrorMessage(messages []text.Message) FlowOption { return func(f *Flow) { for i := range messages { @@ -776,35 +779,35 @@ continueLogin: return } - internalContextDuplicateCredentials := gjson.GetBytes(f.InternalContext, flow.InternalContextDuplicateCredentialsPath) - if internalContextDuplicateCredentials.IsObject() { - var linkCredentials flow.RegistrationDuplicateCredentials - if err := json.Unmarshal([]byte(internalContextDuplicateCredentials.Raw), &linkCredentials); err != nil { - h.d.LoginFlowErrorHandler().WriteFlowError(w, r, f, node.DefaultGroup, err) - return - } + /* internalContextDuplicateCredentials := gjson.GetBytes(f.InternalContext, flow.InternalContextDuplicateCredentialsPath) + if internalContextDuplicateCredentials.IsObject() { + var linkCredentials flow.RegistrationDuplicateCredentials + if err := json.Unmarshal([]byte(internalContextDuplicateCredentials.Raw), &linkCredentials); err != nil { + h.d.LoginFlowErrorHandler().WriteFlowError(w, r, f, node.DefaultGroup, err) + return + } - loginFlow, _, err := h.NewLoginFlow(w, r, f.Type, func(newFlow *Flow) { - newFlow.ReturnTo = f.ReturnTo - newFlow.HydraLoginRequest = f.HydraLoginRequest - newFlow.OAuth2LoginChallenge = f.OAuth2LoginChallenge - newFlow.OrganizationID = f.OrganizationID - newFlow.UI.Messages.Add(text.NewInfoSelfServiceLoginLinkCredentials()) - newFlow.InternalContext, err = sjson.SetBytes( - newFlow.InternalContext, flow.InternalContextLinkCredentialsPath, linkCredentials) + loginFlow, _, err := h.NewLoginFlow(w, r, f.Type, func(newFlow *Flow) { + newFlow.ReturnTo = f.ReturnTo + newFlow.HydraLoginRequest = f.HydraLoginRequest + newFlow.OAuth2LoginChallenge = f.OAuth2LoginChallenge + newFlow.OrganizationID = f.OrganizationID + newFlow.UI.Messages.Add(text.NewInfoSelfServiceLoginLinkCredentials()) + newFlow.InternalContext, err = sjson.SetBytes( + newFlow.InternalContext, flow.InternalContextLinkCredentialsPath, linkCredentials) + if err != nil { + h.d.LoginFlowErrorHandler().WriteFlowError(w, r, f, node.DefaultGroup, err) + return + } + }) if err != nil { h.d.LoginFlowErrorHandler().WriteFlowError(w, r, f, node.DefaultGroup, err) return } - }) - if err != nil { - h.d.LoginFlowErrorHandler().WriteFlowError(w, r, f, node.DefaultGroup, err) - return - } - http.Redirect(w, r, loginFlow.AppendTo(h.d.Config().SelfServiceFlowLoginUI(r.Context())).String(), http.StatusSeeOther) - return - } + http.Redirect(w, r, loginFlow.AppendTo(h.d.Config().SelfServiceFlowLoginUI(r.Context())).String(), http.StatusSeeOther) + return + } */ var i *identity.Identity var group node.UiNodeGroup diff --git a/selfservice/flow/registration/handler.go b/selfservice/flow/registration/handler.go index 8fec5faf988c..dff97c16929b 100644 --- a/selfservice/flow/registration/handler.go +++ b/selfservice/flow/registration/handler.go @@ -4,7 +4,6 @@ package registration import ( - "encoding/json" "net/http" "net/url" "time" @@ -12,8 +11,6 @@ import ( "github.com/gofrs/uuid" "github.com/julienschmidt/httprouter" "github.com/pkg/errors" - "github.com/tidwall/gjson" - "github.com/tidwall/sjson" "github.com/ory/herodot" "github.com/ory/kratos/driver/config" @@ -602,37 +599,6 @@ func (h *Handler) updateRegistrationFlow(w http.ResponseWriter, r *http.Request, return } - internalContextDuplicateCredentials := gjson.GetBytes(f.InternalContext, flow.InternalContextDuplicateCredentialsPath) - if internalContextDuplicateCredentials.IsObject() { - // If return_to was set before, we need to preserve it. - var opts []login.FlowOption - if len(f.ReturnTo) > 0 { - opts = append(opts, login.WithFlowReturnTo(f.ReturnTo)) - } - opts = append(opts, func(newFlow *login.Flow) { - newFlow.UI.Messages.Add(text.NewInfoSelfServiceLoginLinkCredentials()) - var linkCredentials flow.RegistrationDuplicateCredentials - if err := json.Unmarshal([]byte(internalContextDuplicateCredentials.Raw), &linkCredentials); err != nil { - h.d.RegistrationFlowErrorHandler().WriteFlowError(w, r, f, node.DefaultGroup, err) - return - } - newFlow.InternalContext, err = sjson.SetBytes(newFlow.InternalContext, flow.InternalContextLinkCredentialsPath, - linkCredentials) - if err != nil { - h.d.RegistrationFlowErrorHandler().WriteFlowError(w, r, f, node.DefaultGroup, err) - return - } - }) - loginFlow, _, err := h.d.LoginHandler().NewLoginFlow(w, r, flow.TypeBrowser, opts...) - if err != nil { - h.d.RegistrationFlowErrorHandler().WriteFlowError(w, r, f, node.DefaultGroup, err) - return - } - - http.Redirect(w, r, loginFlow.AppendTo(h.d.Config().SelfServiceFlowLoginUI(r.Context())).String(), http.StatusSeeOther) - return - } - i := identity.NewIdentity(h.d.Config().DefaultIdentityTraitsSchemaID(r.Context())) var s Strategy for _, ss := range h.d.AllRegistrationStrategies() { diff --git a/selfservice/flow/registration/hook.go b/selfservice/flow/registration/hook.go index 78af02f84b97..a0b824da576f 100644 --- a/selfservice/flow/registration/hook.go +++ b/selfservice/flow/registration/hook.go @@ -21,6 +21,7 @@ import ( "github.com/ory/kratos/selfservice/flow/login" "github.com/ory/kratos/selfservice/sessiontokenexchange" "github.com/ory/kratos/session" + "github.com/ory/kratos/text" "github.com/ory/kratos/ui/node" "github.com/ory/kratos/x" "github.com/ory/kratos/x/events" @@ -175,29 +176,6 @@ func (e *HookExecutor) PostRegistrationHook(w http.ResponseWriter, r *http.Reque CredentialsConfig: i.Credentials[ct].Config, DuplicateIdentifier: duplicateIdentifier, } - loginFlowID, err := a.GetOuterLoginFlowID() - if err != nil { - return err - } - if loginFlowID != nil { - loginFlow, err := e.d.LoginFlowPersister().GetLoginFlow(r.Context(), *loginFlowID) - if err != nil { - return err - } - loginFlow.InternalContext, err = sjson.SetBytes(loginFlow.InternalContext, flow.InternalContextDuplicateCredentialsPath, - registrationDuplicateCredentials) - if err != nil { - return err - } - loginFlow.UI.SetNode(node.NewInputField( - "method", - node.LoginAndLinkCredentials, - node.DefaultGroup, - node.InputAttributeTypeSubmit)) - if err := e.d.LoginFlowPersister().UpdateLoginFlow(r.Context(), loginFlow); err != nil { - return err - } - } a.InternalContext, err = sjson.SetBytes(a.InternalContext, flow.InternalContextDuplicateCredentialsPath, registrationDuplicateCredentials) @@ -208,10 +186,7 @@ func (e *HookExecutor) PostRegistrationHook(w http.ResponseWriter, r *http.Reque "method", node.LoginAndLinkCredentials, node.DefaultGroup, - node.InputAttributeTypeSubmit)) - if err := e.d.RegistrationFlowPersister().UpdateRegistrationFlow(r.Context(), a); err != nil { - return err - } + node.InputAttributeTypeSubmit).WithMetaLabel(text.NewInfoNodeLoginAndLinkCredential())) } } return err diff --git a/selfservice/strategy/oidc/strategy.go b/selfservice/strategy/oidc/strategy.go index e73be5e21826..0c78733d93f3 100644 --- a/selfservice/strategy/oidc/strategy.go +++ b/selfservice/strategy/oidc/strategy.go @@ -543,17 +543,19 @@ func (s *Strategy) handleError(w http.ResponseWriter, r *http.Request, f flow.Fl // Reset all nodes to not confuse users. // This is kinda hacky and will probably need to be updated at some point. - var loginAndLinkCredentialsNode *node.Node - for _, n := range rf.UI.Nodes { - if n.ID() == "method" && n.GetValue() == node.LoginAndLinkCredentials { - loginAndLinkCredentialsNode = n - break + if dup := new(identity.ErrDuplicateCredentials); errors.As(err, &dup) { + rf.UI.Messages.Add(text.NewErrorValidationDuplicateCredentialsOnOIDCLink()) + lf, err := s.registrationToLogin(w, r, rf, provider) + if err != nil { + return err } + // return a new login flow with the error message embedded in the login flow. + x.AcceptToRedirectOrJSON(w, r, s.d.Writer(), lf, lf.AppendTo(s.d.Config().SelfServiceFlowLoginUI(r.Context())).String()) + // ensure the function does not continue to execute + return registration.ErrHookAbortFlow } + rf.UI.Nodes = node.Nodes{} - if loginAndLinkCredentialsNode != nil { - rf.UI.Nodes.Upsert(loginAndLinkCredentialsNode) - } // Adds the "Continue" button rf.UI.SetCSRF(s.d.GenerateCSRFToken(r)) diff --git a/selfservice/strategy/oidc/strategy_registration.go b/selfservice/strategy/oidc/strategy_registration.go index b6747ef9ad0a..a1e60ee7a639 100644 --- a/selfservice/strategy/oidc/strategy_registration.go +++ b/selfservice/strategy/oidc/strategy_registration.go @@ -261,6 +261,10 @@ func (s *Strategy) registrationToLogin(w http.ResponseWriter, r *http.Request, r opts = append(opts, login.WithFormErrorMessage(rf.UI.Messages)) } + if len(rf.InternalContext) > 0 { + opts = append(opts, login.WithInternalContext(rf.InternalContext)) + } + lf, _, err := s.d.LoginHandler().NewLoginFlow(w, r, rf.Type, opts...) if err != nil { return nil, err diff --git a/selfservice/strategy/oidc/strategy_test.go b/selfservice/strategy/oidc/strategy_test.go index 7622b563ecbe..279f6439b6a8 100644 --- a/selfservice/strategy/oidc/strategy_test.go +++ b/selfservice/strategy/oidc/strategy_test.go @@ -18,7 +18,6 @@ import ( "testing" "time" - "github.com/ory/kratos/ui/node" "github.com/ory/x/sqlxx" "github.com/ory/kratos/hydra" @@ -1086,19 +1085,6 @@ func TestStrategy(t *testing.T) { return res, body } - startNewLoginFlowForLinking := func(t *testing.T, c *http.Client, flowID uuid.UUID) *login.Flow { - action := assertFormValues(t, flowID, "valid") - res, err := c.PostForm(action, url.Values{"method": {node.LoginAndLinkCredentials}}) - require.NoError(t, err, action) - body, err := io.ReadAll(res.Body) - require.NoError(t, res.Body.Close()) - require.NoError(t, err) - assertUIError(t, res, body, "New credentials will be linked to existing account after login.") - loginFlow, err := reg.LoginFlowPersister().GetLoginFlow(context.Background(), uuid.FromStringOrNil(gjson.GetBytes(body, "id").String())) - assert.NotNil(t, loginFlow, "%s", body) - return loginFlow - } - checkCredentialsLinked := func(res *http.Response, body []byte, identityID uuid.UUID, provider string) { assert.Contains(t, res.Request.URL.String(), returnTS.URL, "%s", body) assert.Equal(t, subject, gjson.GetBytes(body, "identity.traits.subject").String(), "%s", body) @@ -1117,7 +1103,7 @@ func TestStrategy(t *testing.T) { password := "lwkj52sdkjf" var i *identity.Identity - t.Run("case=create password identity", func(t *testing.T) { + t.Run("step=create password identity", func(t *testing.T) { i = identity.NewIdentity(config.DefaultIdentityTraitsSchemaID) p, err := reg.Hasher(ctx).Generate(ctx, []byte(password)) require.NoError(t, err) @@ -1139,37 +1125,46 @@ func TestStrategy(t *testing.T) { client := testhelpers.NewClientWithCookieJar(t, nil, false) loginFlow := newLoginFlow(t, returnTS.URL, time.Minute, flow.TypeBrowser) - t.Run("case=should fail login", func(t *testing.T) { - res, body := loginWithOIDC(t, client, loginFlow.ID, "valid") - assertUIError(t, res, body, "An account with the same identifier (email, phone, username, ...) exists already.") - assert.Equal(t, node.LoginAndLinkCredentials, gjson.GetBytes(body, "ui.nodes.#(attributes.name==\"method\").attributes.value").String(), "%s", prettyJSON(t, body)) - }) - var linkingLoginFlow *login.Flow - t.Run("case=should start new login flow", func(t *testing.T) { - linkingLoginFlow = startNewLoginFlowForLinking(t, client, loginFlow.ID) + var linkingLoginFlow struct { + ID string + UIAction string + CSRFToken string + } + + t.Run("step=should fail login and start a new flow", func(t *testing.T) { + res, body := loginWithOIDC(t, client, loginFlow.ID, "valid") + assertUIError(t, res, body, "An account with the same identifier (email, phone, username, ...) exists already. Please sign in to your existing account to link your social profile.") + linkingLoginFlow.ID = gjson.GetBytes(body, "id").String() + linkingLoginFlow.UIAction = gjson.GetBytes(body, "ui.action").String() + linkingLoginFlow.CSRFToken = gjson.GetBytes(body, `ui.nodes.#(attributes.name=="csrf_token").attributes.value`).String() + assert.NotEqual(t, loginFlow.ID.String(), linkingLoginFlow.ID, "should have started a new flow") }) - t.Run("case=should fail login if existing identity identifier doesn't match", func(t *testing.T) { - res, err := client.PostForm(linkingLoginFlow.UI.Action, url.Values{ + t.Run("step=should fail login if existing identity identifier doesn't match", func(t *testing.T) { + res, err := client.PostForm(linkingLoginFlow.UIAction, url.Values{ "csrf_token": {linkingLoginFlow.CSRFToken}, "method": {"password"}, "identifier": {subject2}, "password": {password}}) - require.NoError(t, err, linkingLoginFlow.UI.Action) + require.NoError(t, err, linkingLoginFlow.UIAction) body, err := io.ReadAll(res.Body) require.NoError(t, res.Body.Close()) require.NoError(t, err) - assert.Equal(t, strconv.Itoa(int(text.ErrorValidationLoginLinkedCredentialsDoNotMatch)), gjson.GetBytes(body, "ui.messages.0.id").String(), "%s", prettyJSON(t, body)) + assert.Equal(t, + strconv.Itoa(int(text.ErrorValidationLoginLinkedCredentialsDoNotMatch)), + gjson.GetBytes(body, "ui.messages.0.id").String(), + prettyJSON(t, body), + ) }) - t.Run("case=should link oidc credentials to existing identity", func(t *testing.T) { - res, err := client.PostForm(linkingLoginFlow.UI.Action, url.Values{ + t.Run("step=should link oidc credentials to existing identity", func(t *testing.T) { + res, err := client.PostForm(linkingLoginFlow.UIAction, url.Values{ "csrf_token": {linkingLoginFlow.CSRFToken}, "method": {"password"}, "identifier": {subject}, "password": {password}}) - require.NoError(t, err, linkingLoginFlow.UI.Action) + require.NoError(t, err, linkingLoginFlow.UIAction) body, err := io.ReadAll(res.Body) require.NoError(t, res.Body.Close()) require.NoError(t, err) @@ -1183,7 +1178,7 @@ func TestStrategy(t *testing.T) { scope = []string{"openid", "offline"} var identityID uuid.UUID - t.Run("case=create OIDC identity", func(t *testing.T) { + t.Run("step=create OIDC identity", func(t *testing.T) { subject = email1 r := newRegistrationFlow(t, returnTS.URL, time.Minute, flow.TypeBrowser) action := assertFormValues(t, r.ID, "secondProvider") @@ -1200,28 +1195,25 @@ func TestStrategy(t *testing.T) { }) subject = email1 - c := testhelpers.NewClientWithCookieJar(t, nil, false) - r := newLoginFlow(t, returnTS.URL, time.Minute, flow.TypeBrowser) - t.Run("case=should fail login", func(t *testing.T) { - res, body := loginWithOIDC(t, c, r.ID, "valid") - assertUIError(t, res, body, "An account with the same identifier (email, phone, username, ...) exists already.") - assert.Equal(t, node.LoginAndLinkCredentials, gjson.GetBytes(body, "ui.nodes.#(attributes.name==\"method\").attributes.value").String(), "%s", body) - }) - - var loginFlow *login.Flow - t.Run("case=should start new login flow", func(t *testing.T) { - loginFlow = startNewLoginFlowForLinking(t, c, r.ID) + client := testhelpers.NewClientWithCookieJar(t, nil, false) + loginFlow := newLoginFlow(t, returnTS.URL, time.Minute, flow.TypeBrowser) + 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, "An account with the same identifier (email, phone, username, ...) exists already. Please sign in to your existing account to link your social profile.") + linkingLoginFlow.ID = gjson.GetBytes(body, "id").String() + assert.NotEqual(t, loginFlow.ID.String(), linkingLoginFlow.ID, "should have started a new flow") }) subject = email2 - t.Run("case=should fail login if existing identity identifier doesn't match", func(t *testing.T) { - res, body := loginWithOIDC(t, c, loginFlow.ID, "valid") + t.Run("step=should fail login if existing identity identifier doesn't match", func(t *testing.T) { + res, body := loginWithOIDC(t, client, uuid.Must(uuid.FromString(linkingLoginFlow.ID)), "valid") assertUIError(t, res, body, "Linked credentials do not match.") }) subject = email1 - t.Run("case=should link oidc credentials to existing identity", func(t *testing.T) { - res, body := loginWithOIDC(t, c, loginFlow.ID, "secondProvider") + t.Run("step=should link oidc credentials to existing identity", func(t *testing.T) { + res, body := loginWithOIDC(t, client, uuid.Must(uuid.FromString(linkingLoginFlow.ID)), "secondProvider") checkCredentialsLinked(res, body, identityID, "secondProvider") }) }) diff --git a/text/id.go b/text/id.go index c5068f9ad3ca..24cca62f4a5b 100644 --- a/text/id.go +++ b/text/id.go @@ -90,6 +90,7 @@ const ( InfoNodeLabelVerificationCode // 1070011 InfoNodeLabelRegistrationCode // 1070012 InfoNodeLabelLoginCode // 1070013 + InfoNodeLabelLoginAndLinkCredential ) const ( diff --git a/text/message_login.go b/text/message_login.go index d76bc9c4bf39..5abfab44dfb3 100644 --- a/text/message_login.go +++ b/text/message_login.go @@ -199,14 +199,6 @@ func NewInfoSelfServiceLoginCode() *Message { } } -func NewInfoSelfServiceLoginLinkCredentials() *Message { - return &Message{ - ID: InfoSelfServiceLoginLinkCredentials, - Text: "New credentials will be linked to existing account after login.", - Type: Info, - } -} - func NewErrorValidationLoginLinkedCredentialsDoNotMatch() *Message { return &Message{ ID: ErrorValidationLoginLinkedCredentialsDoNotMatch, diff --git a/text/message_node.go b/text/message_node.go index 3af122ae542b..e2dfb7d6dc32 100644 --- a/text/message_node.go +++ b/text/message_node.go @@ -109,3 +109,11 @@ func NewInfoNodeResendOTP() *Message { Type: Info, } } + +func NewInfoNodeLoginAndLinkCredential() *Message { + return &Message{ + ID: InfoNodeLabelLoginAndLinkCredential, + Text: "Login and link credential", + Type: Info, + } +} diff --git a/text/message_validation.go b/text/message_validation.go index 205c27304cdb..28396180c0c5 100644 --- a/text/message_validation.go +++ b/text/message_validation.go @@ -317,8 +317,9 @@ func NewErrorValidationDuplicateCredentialsWithHints(availableCredentialTypes [] func NewErrorValidationDuplicateCredentialsOnOIDCLink() *Message { return &Message{ - ID: ErrorValidationDuplicateCredentialsOnOIDCLink, - Text: "An account with the same identifier (email, phone, username, ...) exists already. Please sign in to your existing account and link your social profile in the settings page.", + ID: ErrorValidationDuplicateCredentialsOnOIDCLink, + Text: "An account with the same identifier (email, phone, username, ...) exists already. " + + "Please sign in to your existing account to link your social profile.", Type: Error, } } From c540231333a3dfe04ff5cf74de810356d5673817 Mon Sep 17 00:00:00 2001 From: Henning Perl Date: Mon, 23 Oct 2023 13:40:09 +0200 Subject: [PATCH 12/29] chore: cleanup and test fixes --- selfservice/flow/login/flow.go | 2 +- selfservice/flow/login/handler.go | 31 ------------------ selfservice/flow/login/hook.go | 2 +- selfservice/flow/login/strategy.go | 7 ++-- selfservice/flow/registration/flow.go | 32 ------------------- selfservice/flow/registration/handler.go | 8 ----- selfservice/flow/registration/hook.go | 7 ---- selfservice/strategy/oidc/strategy.go | 9 +++++- selfservice/strategy/oidc/strategy_login.go | 2 -- selfservice/strategy/oidc/strategy_test.go | 2 +- .../verification/settings/success.spec.ts | 12 ++++--- 11 files changed, 20 insertions(+), 94 deletions(-) diff --git a/selfservice/flow/login/flow.go b/selfservice/flow/login/flow.go index 2229b1fc08a2..bb8fb6d8db7d 100644 --- a/selfservice/flow/login/flow.go +++ b/selfservice/flow/login/flow.go @@ -231,7 +231,7 @@ func (f *Flow) EnsureInternalContext() { } } -func (f *Flow) duplicateCredentials() (*flow.RegistrationDuplicateCredentials, error) { +func (f *Flow) DuplicateCredentials() (*flow.RegistrationDuplicateCredentials, error) { raw := gjson.GetBytes(f.InternalContext, flow.InternalContextDuplicateCredentialsPath) if !raw.IsObject() { return nil, nil diff --git a/selfservice/flow/login/handler.go b/selfservice/flow/login/handler.go index e0675ed50dfb..bc85bbe27120 100644 --- a/selfservice/flow/login/handler.go +++ b/selfservice/flow/login/handler.go @@ -4,7 +4,6 @@ package login import ( - _ "embed" "net/http" "net/url" "time" @@ -779,36 +778,6 @@ continueLogin: return } - /* internalContextDuplicateCredentials := gjson.GetBytes(f.InternalContext, flow.InternalContextDuplicateCredentialsPath) - if internalContextDuplicateCredentials.IsObject() { - var linkCredentials flow.RegistrationDuplicateCredentials - if err := json.Unmarshal([]byte(internalContextDuplicateCredentials.Raw), &linkCredentials); err != nil { - h.d.LoginFlowErrorHandler().WriteFlowError(w, r, f, node.DefaultGroup, err) - return - } - - loginFlow, _, err := h.NewLoginFlow(w, r, f.Type, func(newFlow *Flow) { - newFlow.ReturnTo = f.ReturnTo - newFlow.HydraLoginRequest = f.HydraLoginRequest - newFlow.OAuth2LoginChallenge = f.OAuth2LoginChallenge - newFlow.OrganizationID = f.OrganizationID - newFlow.UI.Messages.Add(text.NewInfoSelfServiceLoginLinkCredentials()) - newFlow.InternalContext, err = sjson.SetBytes( - newFlow.InternalContext, flow.InternalContextLinkCredentialsPath, linkCredentials) - if err != nil { - h.d.LoginFlowErrorHandler().WriteFlowError(w, r, f, node.DefaultGroup, err) - return - } - }) - if err != nil { - h.d.LoginFlowErrorHandler().WriteFlowError(w, r, f, node.DefaultGroup, err) - return - } - - http.Redirect(w, r, loginFlow.AppendTo(h.d.Config().SelfServiceFlowLoginUI(r.Context())).String(), http.StatusSeeOther) - return - } */ - var i *identity.Identity var group node.UiNodeGroup for _, ss := range h.d.AllLoginStrategies() { diff --git a/selfservice/flow/login/hook.go b/selfservice/flow/login/hook.go index a94b851cced9..44a06f471bb1 100644 --- a/selfservice/flow/login/hook.go +++ b/selfservice/flow/login/hook.go @@ -333,7 +333,7 @@ func (e *HookExecutor) PreLoginHook(w http.ResponseWriter, r *http.Request, a *F // maybeLinkCredentials links the identity with the credentials of the inner context of the login flow. func (e *HookExecutor) maybeLinkCredentials(r *http.Request, s *session.Session, i *identity.Identity, f *Flow) error { - lc, err := f.duplicateCredentials() + lc, err := f.DuplicateCredentials() if err != nil { return err } else if lc == nil { diff --git a/selfservice/flow/login/strategy.go b/selfservice/flow/login/strategy.go index 69deadc908ad..c8ad84986a55 100644 --- a/selfservice/flow/login/strategy.go +++ b/selfservice/flow/login/strategy.go @@ -7,17 +7,14 @@ import ( "context" "net/http" - "github.com/ory/x/sqlxx" - "github.com/gofrs/uuid" - - "github.com/ory/kratos/session" - "github.com/pkg/errors" "github.com/ory/kratos/identity" + "github.com/ory/kratos/session" "github.com/ory/kratos/ui/node" "github.com/ory/kratos/x" + "github.com/ory/x/sqlxx" ) type Strategy interface { diff --git a/selfservice/flow/registration/flow.go b/selfservice/flow/registration/flow.go index 1efa565714e5..2f1ea3603b40 100644 --- a/selfservice/flow/registration/flow.go +++ b/selfservice/flow/registration/flow.go @@ -14,7 +14,6 @@ import ( "github.com/gofrs/uuid" "github.com/pkg/errors" "github.com/tidwall/gjson" - "github.com/tidwall/sjson" hydraclientgo "github.com/ory/hydra-client-go/v2" "github.com/ory/kratos/driver/config" @@ -262,34 +261,3 @@ func (f *Flow) GetFlowName() flow.FlowName { func (f *Flow) SetState(state State) { f.State = state } - -const internalContextOuterLoginFlowPath = "outer_flow" - -type OuterLoginFlow struct { - ID string -} - -func (f *Flow) GetOuterLoginFlowID() (*uuid.UUID, error) { - la := gjson.GetBytes(f.InternalContext, internalContextOuterLoginFlowPath) - if !la.IsObject() { - return nil, nil - } - var internalContextOuterFlow OuterLoginFlow - if err := json.Unmarshal([]byte(la.Raw), &internalContextOuterFlow); err != nil { - return nil, err - } - id, err := uuid.FromString(internalContextOuterFlow.ID) - if err != nil { - return nil, err - } - return &id, nil -} - -func (f *Flow) SetOuterLoginFlowID(flowID uuid.UUID) error { - var err error = nil - f.InternalContext, err = sjson.SetBytes(f.InternalContext, internalContextOuterLoginFlowPath, - OuterLoginFlow{ - ID: flowID.String(), - }) - return err -} diff --git a/selfservice/flow/registration/handler.go b/selfservice/flow/registration/handler.go index dff97c16929b..4e99604313b9 100644 --- a/selfservice/flow/registration/handler.go +++ b/selfservice/flow/registration/handler.go @@ -19,7 +19,6 @@ import ( "github.com/ory/kratos/schema" "github.com/ory/kratos/selfservice/errorx" "github.com/ory/kratos/selfservice/flow" - "github.com/ory/kratos/selfservice/flow/login" "github.com/ory/kratos/selfservice/flow/logout" "github.com/ory/kratos/selfservice/sessiontokenexchange" "github.com/ory/kratos/session" @@ -45,7 +44,6 @@ type ( config.Provider errorx.ManagementProvider hydra.Provider - login.HandlerProvider session.HandlerProvider session.ManagementProvider x.WriterProvider @@ -115,12 +113,6 @@ func WithFlowOAuth2LoginChallenge(loginChallenge string) FlowOption { } } -func WithOuterFlow(flowId uuid.UUID) FlowOption { - return func(f *Flow) { - _ = f.SetOuterLoginFlowID(flowId) - } -} - func (h *Handler) NewRegistrationFlow(w http.ResponseWriter, r *http.Request, ft flow.Type, opts ...FlowOption) (*Flow, error) { if !h.d.Config().SelfServiceFlowRegistrationEnabled(r.Context()) { return nil, errors.WithStack(ErrRegistrationDisabled) diff --git a/selfservice/flow/registration/hook.go b/selfservice/flow/registration/hook.go index a0b824da576f..9b8c346f4123 100644 --- a/selfservice/flow/registration/hook.go +++ b/selfservice/flow/registration/hook.go @@ -21,8 +21,6 @@ import ( "github.com/ory/kratos/selfservice/flow/login" "github.com/ory/kratos/selfservice/sessiontokenexchange" "github.com/ory/kratos/session" - "github.com/ory/kratos/text" - "github.com/ory/kratos/ui/node" "github.com/ory/kratos/x" "github.com/ory/kratos/x/events" "github.com/ory/x/otelx" @@ -182,11 +180,6 @@ func (e *HookExecutor) PostRegistrationHook(w http.ResponseWriter, r *http.Reque if err != nil { return err } - a.UI.SetNode(node.NewInputField( - "method", - node.LoginAndLinkCredentials, - node.DefaultGroup, - node.InputAttributeTypeSubmit).WithMetaLabel(text.NewInfoNodeLoginAndLinkCredential())) } } return err diff --git a/selfservice/strategy/oidc/strategy.go b/selfservice/strategy/oidc/strategy.go index 0c78733d93f3..c5d5b3f489ec 100644 --- a/selfservice/strategy/oidc/strategy.go +++ b/selfservice/strategy/oidc/strategy.go @@ -550,7 +550,13 @@ func (s *Strategy) handleError(w http.ResponseWriter, r *http.Request, f flow.Fl return err } // return a new login flow with the error message embedded in the login flow. - x.AcceptToRedirectOrJSON(w, r, s.d.Writer(), lf, lf.AppendTo(s.d.Config().SelfServiceFlowLoginUI(r.Context())).String()) + redirectURL := lf.AppendTo(s.d.Config().SelfServiceFlowLoginUI(r.Context())) + if err, dc := lf.DuplicateCredentials(); err == nil && dc != nil { + q := redirectURL.Query() + q.Set("skip_org_ui", "true") + redirectURL.RawQuery = q.Encode() + } + x.AcceptToRedirectOrJSON(w, r, s.d.Writer(), lf, redirectURL.String()) // ensure the function does not continue to execute return registration.ErrHookAbortFlow } @@ -663,5 +669,6 @@ func (s *Strategy) linkCredentials(ctx context.Context, i *identity.Identity, id } } i.Credentials[s.ID()] = *creds + return nil } diff --git a/selfservice/strategy/oidc/strategy_login.go b/selfservice/strategy/oidc/strategy_login.go index f9bc3248a383..c81537015bc9 100644 --- a/selfservice/strategy/oidc/strategy_login.go +++ b/selfservice/strategy/oidc/strategy_login.go @@ -133,8 +133,6 @@ func (s *Strategy) processLogin(w http.ResponseWriter, r *http.Request, loginFlo opts = append(opts, registration.WithFlowOAuth2LoginChallenge(loginFlow.OAuth2LoginChallenge.String())) } - opts = append(opts, registration.WithOuterFlow(loginFlow.ID)) - registrationFlow, err := s.d.RegistrationHandler().NewRegistrationFlow(w, r, loginFlow.Type, opts...) if err != nil { return nil, s.handleError(w, r, loginFlow, provider.Config().ID, nil, err) diff --git a/selfservice/strategy/oidc/strategy_test.go b/selfservice/strategy/oidc/strategy_test.go index 279f6439b6a8..717966dc2e53 100644 --- a/selfservice/strategy/oidc/strategy_test.go +++ b/selfservice/strategy/oidc/strategy_test.go @@ -886,7 +886,7 @@ func TestStrategy(t *testing.T) { action := assertFormValues(t, r.ID, "valid") res, body := makeRequest(t, "valid", action, url.Values{}) assertUIError(t, res, body, "An account with the same identifier (email, phone, username, ...) exists already.") - require.Contains(t, gjson.GetBytes(body, "ui.action").String(), "/self-service/registration") + require.Contains(t, gjson.GetBytes(body, "ui.action").String(), "/self-service/login") }) t.Run("case=should fail login", func(t *testing.T) { diff --git a/test/e2e/cypress/integration/profiles/verification/settings/success.spec.ts b/test/e2e/cypress/integration/profiles/verification/settings/success.spec.ts index bba960f280ae..d17ee11b9dda 100644 --- a/test/e2e/cypress/integration/profiles/verification/settings/success.spec.ts +++ b/test/e2e/cypress/integration/profiles/verification/settings/success.spec.ts @@ -50,12 +50,14 @@ context("Account Verification Settings Success", () => { .clear() .type(email) cy.get('[value="profile"]').click() - cy.expectSettingsSaved() - cy.get('input[name="traits.email"]').should("contain.value", email) - cy.getSession().then( - assertVerifiableAddress({ isVerified: false, email }), - ) + if (app == "express") { + cy.expectSettingsSaved() + cy.get('input[name="traits.email"]').should("contain.value", email) + cy.getSession().then( + assertVerifiableAddress({ isVerified: false, email }), + ) + } cy.verifyEmail({ expect: { email } }) }) From 14493bdccc8cf6e8b56b08653359175795584695 Mon Sep 17 00:00:00 2001 From: Henning Perl Date: Tue, 24 Oct 2023 09:46:05 +0200 Subject: [PATCH 13/29] fix: add no_org_ui param --- selfservice/strategy/oidc/strategy.go | 4 ++-- selfservice/strategy/oidc/strategy_test.go | 1 + 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/selfservice/strategy/oidc/strategy.go b/selfservice/strategy/oidc/strategy.go index c5d5b3f489ec..d212be535307 100644 --- a/selfservice/strategy/oidc/strategy.go +++ b/selfservice/strategy/oidc/strategy.go @@ -551,9 +551,9 @@ func (s *Strategy) handleError(w http.ResponseWriter, r *http.Request, f flow.Fl } // return a new login flow with the error message embedded in the login flow. redirectURL := lf.AppendTo(s.d.Config().SelfServiceFlowLoginUI(r.Context())) - if err, dc := lf.DuplicateCredentials(); err == nil && dc != nil { + if dc, err := lf.DuplicateCredentials(); err == nil && dc != nil { q := redirectURL.Query() - q.Set("skip_org_ui", "true") + q.Set("no_org_ui", "true") redirectURL.RawQuery = q.Encode() } x.AcceptToRedirectOrJSON(w, r, s.d.Writer(), lf, redirectURL.String()) diff --git a/selfservice/strategy/oidc/strategy_test.go b/selfservice/strategy/oidc/strategy_test.go index 717966dc2e53..a39564baf327 100644 --- a/selfservice/strategy/oidc/strategy_test.go +++ b/selfservice/strategy/oidc/strategy_test.go @@ -1134,6 +1134,7 @@ func TestStrategy(t *testing.T) { t.Run("step=should fail login and start a new flow", func(t *testing.T) { res, body := loginWithOIDC(t, client, loginFlow.ID, "valid") + assert.True(t, res.Request.URL.Query().Has("no_org_ui")) assertUIError(t, res, body, "An account with the same identifier (email, phone, username, ...) exists already. Please sign in to your existing account to link your social profile.") linkingLoginFlow.ID = gjson.GetBytes(body, "id").String() linkingLoginFlow.UIAction = gjson.GetBytes(body, "ui.action").String() From e5a350487d353d46ae153647e070ab5090685d64 Mon Sep 17 00:00:00 2001 From: Henning Perl Date: Tue, 24 Oct 2023 10:49:23 +0200 Subject: [PATCH 14/29] feat: handle org ID --- selfservice/strategy/oidc/strategy.go | 12 ++++++++++-- selfservice/strategy/oidc/strategy_registration.go | 4 +--- selfservice/strategy/oidc/strategy_settings.go | 3 ++- 3 files changed, 13 insertions(+), 6 deletions(-) diff --git a/selfservice/strategy/oidc/strategy.go b/selfservice/strategy/oidc/strategy.go index d212be535307..4520874a4677 100644 --- a/selfservice/strategy/oidc/strategy.go +++ b/selfservice/strategy/oidc/strategy.go @@ -637,7 +637,7 @@ func (s *Strategy) processIDToken(w http.ResponseWriter, r *http.Request, provid return claims, nil } -func (s *Strategy) linkCredentials(ctx context.Context, i *identity.Identity, idToken, accessToken, refreshToken, provider, subject string) error { +func (s *Strategy) linkCredentials(ctx context.Context, i *identity.Identity, idToken, accessToken, refreshToken, provider, subject, organization string) error { if i.Credentials == nil { confidential, err := s.d.PrivilegedIdentityPool().GetIdentityConfidential(ctx, i.ID) if err != nil { @@ -649,7 +649,7 @@ func (s *Strategy) linkCredentials(ctx context.Context, i *identity.Identity, id creds, err := i.ParseCredentials(s.ID(), &conf) if errors.Is(err, herodot.ErrNotFound) { var err error - if creds, err = identity.NewCredentialsOIDC(idToken, accessToken, refreshToken, provider, subject, ""); err != nil { + if creds, err = identity.NewCredentialsOIDC(idToken, accessToken, refreshToken, provider, subject, organization); err != nil { return err } } else if err != nil { @@ -661,6 +661,7 @@ func (s *Strategy) linkCredentials(ctx context.Context, i *identity.Identity, id InitialAccessToken: accessToken, InitialRefreshToken: refreshToken, InitialIDToken: idToken, + Organization: organization, }) creds.Config, err = json.Marshal(conf) @@ -669,6 +670,13 @@ func (s *Strategy) linkCredentials(ctx context.Context, i *identity.Identity, id } } i.Credentials[s.ID()] = *creds + if organization != "" { + orgID, err := uuid.FromString(organization) + if err != nil { + return err + } + i.OrganizationID = uuid.NullUUID{UUID: orgID, Valid: true} + } return nil } diff --git a/selfservice/strategy/oidc/strategy_registration.go b/selfservice/strategy/oidc/strategy_registration.go index a1e60ee7a639..d07e6c6fa8ed 100644 --- a/selfservice/strategy/oidc/strategy_registration.go +++ b/selfservice/strategy/oidc/strategy_registration.go @@ -261,9 +261,7 @@ func (s *Strategy) registrationToLogin(w http.ResponseWriter, r *http.Request, r opts = append(opts, login.WithFormErrorMessage(rf.UI.Messages)) } - if len(rf.InternalContext) > 0 { - opts = append(opts, login.WithInternalContext(rf.InternalContext)) - } + opts = append(opts, login.WithInternalContext(rf.InternalContext)) lf, _, err := s.d.LoginHandler().NewLoginFlow(w, r, rf.Type, opts...) if err != nil { diff --git a/selfservice/strategy/oidc/strategy_settings.go b/selfservice/strategy/oidc/strategy_settings.go index 0475e8aff6b2..a38e4e0b40d9 100644 --- a/selfservice/strategy/oidc/strategy_settings.go +++ b/selfservice/strategy/oidc/strategy_settings.go @@ -414,7 +414,7 @@ func (s *Strategy) linkProvider(w http.ResponseWriter, r *http.Request, ctxUpdat return s.handleSettingsError(w, r, ctxUpdate, p, err) } - if err := s.linkCredentials(r.Context(), i, it, cat, crt, provider.Config().ID, claims.Subject); err != nil { + if err := s.linkCredentials(r.Context(), i, it, cat, crt, provider.Config().ID, claims.Subject, provider.Config().OrganizationID); err != nil { return s.handleSettingsError(w, r, ctxUpdate, p, err) } @@ -533,6 +533,7 @@ func (s *Strategy) Link(ctx context.Context, i *identity.Identity, credentialsCo credentialsOIDCProvider.InitialRefreshToken, credentialsOIDCProvider.Provider, credentialsOIDCProvider.Subject, + credentialsOIDCProvider.Organization, ); err != nil { return err } From cd143905ad689e258b9d726e5d760a4094f5c24d Mon Sep 17 00:00:00 2001 From: Henning Perl Date: Tue, 24 Oct 2023 12:07:17 +0200 Subject: [PATCH 15/29] fix: write org id to amr --- selfservice/flow/login/handler.go | 2 +- selfservice/flow/login/hook.go | 2 +- selfservice/strategy/oidc/strategy.go | 1 + session/manager_http.go | 2 +- session/session.go | 10 +++++++--- 5 files changed, 11 insertions(+), 6 deletions(-) diff --git a/selfservice/flow/login/handler.go b/selfservice/flow/login/handler.go index bc85bbe27120..096a657c0869 100644 --- a/selfservice/flow/login/handler.go +++ b/selfservice/flow/login/handler.go @@ -799,7 +799,7 @@ continueLogin: } method := ss.CompletedAuthenticationMethod(r.Context()) - sess.CompletedLoginFor(method.Method, method.AAL) + sess.CompletedLoginForMethod(method) i = interim break } diff --git a/selfservice/flow/login/hook.go b/selfservice/flow/login/hook.go index 44a06f471bb1..6ec559370f72 100644 --- a/selfservice/flow/login/hook.go +++ b/selfservice/flow/login/hook.go @@ -359,7 +359,7 @@ func (e *HookExecutor) maybeLinkCredentials(r *http.Request, s *session.Session, } method := strategy.CompletedAuthenticationMethod(r.Context()) - s.CompletedLoginFor(method.Method, method.AAL) + s.CompletedLoginForMethod(method) return nil } diff --git a/selfservice/strategy/oidc/strategy.go b/selfservice/strategy/oidc/strategy.go index 4520874a4677..d58f2615ed17 100644 --- a/selfservice/strategy/oidc/strategy.go +++ b/selfservice/strategy/oidc/strategy.go @@ -669,6 +669,7 @@ func (s *Strategy) linkCredentials(ctx context.Context, i *identity.Identity, id return err } } + i.Credentials[s.ID()] = *creds if organization != "" { orgID, err := uuid.FromString(organization) diff --git a/session/manager_http.go b/session/manager_http.go index e1c31f94f03f..fbd6574e0b2c 100644 --- a/session/manager_http.go +++ b/session/manager_http.go @@ -364,7 +364,7 @@ func (s *ManagerHTTP) SessionAddAuthenticationMethods(ctx context.Context, sid u return err } for _, m := range ams { - sess.CompletedLoginFor(m.Method, m.AAL) + sess.CompletedLoginForMethod(m) } sess.SetAuthenticatorAssuranceLevel() return s.r.SessionPersister().UpsertSession(ctx, sess) diff --git a/session/session.go b/session/session.go index 74204be4215c..d11a05e3bf05 100644 --- a/session/session.go +++ b/session/session.go @@ -164,15 +164,19 @@ func (s Session) TableName(ctx context.Context) string { return "sessions" } +func (s *Session) CompletedLoginForMethod(method AuthenticationMethod) { + method.CompletedAt = time.Now().UTC() + s.AMR = append(s.AMR, method) +} + func (s *Session) CompletedLoginFor(method identity.CredentialsType, aal identity.AuthenticatorAssuranceLevel) { - s.AMR = append(s.AMR, AuthenticationMethod{Method: method, AAL: aal, CompletedAt: time.Now().UTC()}) + s.CompletedLoginForMethod(AuthenticationMethod{Method: method, AAL: aal}) } func (s *Session) CompletedLoginForWithProvider(method identity.CredentialsType, aal identity.AuthenticatorAssuranceLevel, providerID string, organizationID string) { - s.AMR = append(s.AMR, AuthenticationMethod{ + s.CompletedLoginForMethod(AuthenticationMethod{ Method: method, AAL: aal, - CompletedAt: time.Now().UTC(), Provider: providerID, Organization: organizationID, }) From 39b7bb6d39be80b1b523c0b530886b9fca71da57 Mon Sep 17 00:00:00 2001 From: Henning Perl Date: Thu, 26 Oct 2023 13:44:27 +0200 Subject: [PATCH 16/29] fix: linter errors --- selfservice/flow/continue_with.go | 3 +-- selfservice/flow/login/hook.go | 12 ------------ 2 files changed, 1 insertion(+), 14 deletions(-) diff --git a/selfservice/flow/continue_with.go b/selfservice/flow/continue_with.go index 5d6eb6ff1619..a54af5abef5a 100644 --- a/selfservice/flow/continue_with.go +++ b/selfservice/flow/continue_with.go @@ -17,9 +17,8 @@ type ContinueWith any // swagger:enum ContinueWithActionSetOrySessionToken type ContinueWithActionSetOrySessionToken string -// #nosec G101 -- only a key constant const ( - ContinueWithActionSetOrySessionTokenString ContinueWithActionSetOrySessionToken = "set_ory_session_token" + ContinueWithActionSetOrySessionTokenString ContinueWithActionSetOrySessionToken = "set_ory_session_token" // #nosec G101 -- only a key constant ) var _ ContinueWith = new(ContinueWithSetOrySessionToken) diff --git a/selfservice/flow/login/hook.go b/selfservice/flow/login/hook.go index 6ec559370f72..456ee7a9705b 100644 --- a/selfservice/flow/login/hook.go +++ b/selfservice/flow/login/hook.go @@ -5,14 +5,12 @@ package login import ( "context" - "encoding/json" "fmt" "net/http" "time" "github.com/gofrs/uuid" "github.com/pkg/errors" - "github.com/tidwall/gjson" "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/trace" @@ -364,16 +362,6 @@ func (e *HookExecutor) maybeLinkCredentials(r *http.Request, s *session.Session, return nil } -func (e *HookExecutor) getInternalContextLinkCredentials(f *Flow, internalContextPath string, lc *flow.RegistrationDuplicateCredentials) error { - internalContextLinkCredentials := gjson.GetBytes(f.InternalContext, internalContextPath) - if internalContextLinkCredentials.IsObject() { - if err := json.Unmarshal([]byte(internalContextLinkCredentials.Raw), lc); err != nil { - return err - } - } - return nil -} - func (e *HookExecutor) checkDuplicateCredentialsIdentifierMatch(ctx context.Context, identityID uuid.UUID, match string) error { i, err := e.d.PrivilegedIdentityPool().GetIdentityConfidential(ctx, identityID) if err != nil { From 5bff9ef9dda21b27f897a513a0382c10533d1eab Mon Sep 17 00:00:00 2001 From: Henning Perl Date: Mon, 30 Oct 2023 10:15:17 +0100 Subject: [PATCH 17/29] add consent label --- cmd/clidoc/main.go | 3 ++ selfservice/flow/duplicate_credentials.go | 57 ++++++++++++++++++++++ selfservice/flow/flow.go | 10 ---- selfservice/flow/login/flow.go | 11 +---- selfservice/flow/login/flow_test.go | 34 +++++++++++++ selfservice/flow/login/hook.go | 2 +- selfservice/flow/registration/flow.go | 4 ++ selfservice/flow/registration/hook.go | 42 ++++++++-------- selfservice/strategy/oidc/strategy.go | 31 +++++++++--- selfservice/strategy/oidc/strategy_test.go | 5 +- text/id.go | 4 +- text/message_login.go | 37 ++++++++++++++ 12 files changed, 189 insertions(+), 51 deletions(-) create mode 100644 selfservice/flow/duplicate_credentials.go diff --git a/cmd/clidoc/main.go b/cmd/clidoc/main.go index 3821f26b0a62..6a9ff0610017 100644 --- a/cmd/clidoc/main.go +++ b/cmd/clidoc/main.go @@ -119,10 +119,13 @@ func init() { "NewInfoLoginTOTPLabel": text.NewInfoLoginTOTPLabel(), "NewInfoLoginLookupLabel": text.NewInfoLoginLookupLabel(), "NewInfoLogin": text.NewInfoLogin(), + "NewInfoLoginAndLink": text.NewInfoLoginAndLink(), + "NewInfoLoginLinkMessage": text.NewInfoLoginLinkMessage("{duplicteIdentifier}", "{provider}", "{newLoginUrl}"), "NewInfoLoginTOTP": text.NewInfoLoginTOTP(), "NewInfoLoginLookup": text.NewInfoLoginLookup(), "NewInfoLoginVerify": text.NewInfoLoginVerify(), "NewInfoLoginWith": text.NewInfoLoginWith("{provider}"), + "NewInfoLoginWithAndLink": text.NewInfoLoginWithAndLink("{provider}"), "NewErrorValidationLoginFlowExpired": text.NewErrorValidationLoginFlowExpired(aSecondAgo), "NewErrorValidationLoginNoStrategyFound": text.NewErrorValidationLoginNoStrategyFound(), "NewErrorValidationRegistrationNoStrategyFound": text.NewErrorValidationRegistrationNoStrategyFound(), diff --git a/selfservice/flow/duplicate_credentials.go b/selfservice/flow/duplicate_credentials.go new file mode 100644 index 000000000000..9249aaacd60e --- /dev/null +++ b/selfservice/flow/duplicate_credentials.go @@ -0,0 +1,57 @@ +package flow + +import ( + "encoding/json" + + "github.com/tidwall/gjson" + "github.com/tidwall/sjson" + + "github.com/ory/kratos/identity" + "github.com/ory/x/sqlxx" +) + +const internalContextDuplicateCredentialsPath = "registration_duplicate_credentials" + +type DuplicateCredentialsData struct { + CredentialsType identity.CredentialsType + CredentialsConfig sqlxx.JSONRawMessage + DuplicateIdentifier string +} + +type InternalContexter interface { + EnsureInternalContext() + GetInternalContext() *sqlxx.JSONRawMessage +} + +// SetDuplicateCredentials sets the duplicate credentials data in the flow's internal context. +func SetDuplicateCredentials(flow InternalContexter, creds DuplicateCredentialsData) error { + if flow.GetInternalContext() == nil { + flow.EnsureInternalContext() + } + bytes, err := sjson.SetBytes( + *flow.GetInternalContext(), + internalContextDuplicateCredentialsPath, + creds, + ) + if err != nil { + return err + } + *flow.GetInternalContext() = bytes + + return nil +} + +// DuplicateCredentials returns the duplicate credentials data from the flow's internal context. +func DuplicateCredentials(flow InternalContexter) (*DuplicateCredentialsData, error) { + if flow.GetInternalContext() == nil { + flow.EnsureInternalContext() + } + raw := gjson.GetBytes(*flow.GetInternalContext(), internalContextDuplicateCredentialsPath) + if !raw.IsObject() { + return nil, nil + } + var creds DuplicateCredentialsData + err := json.Unmarshal([]byte(raw.Raw), &creds) + + return &creds, err +} diff --git a/selfservice/flow/flow.go b/selfservice/flow/flow.go index 14a51d4dbdc4..ee9fbba6638d 100644 --- a/selfservice/flow/flow.go +++ b/selfservice/flow/flow.go @@ -13,21 +13,11 @@ import ( "github.com/ory/herodot" "github.com/ory/kratos/driver/config" - "github.com/ory/kratos/identity" "github.com/ory/kratos/ui/container" "github.com/ory/kratos/x" - "github.com/ory/x/sqlxx" "github.com/ory/x/urlx" ) -const InternalContextDuplicateCredentialsPath = "registration_duplicate_credentials" - -type RegistrationDuplicateCredentials struct { - CredentialsType identity.CredentialsType - CredentialsConfig sqlxx.JSONRawMessage - DuplicateIdentifier string -} - func AppendFlowTo(src *url.URL, id uuid.UUID) *url.URL { return urlx.CopyWithQuery(src, url.Values{"flow": {id.String()}}) } diff --git a/selfservice/flow/login/flow.go b/selfservice/flow/login/flow.go index bb8fb6d8db7d..bb5441c3b09c 100644 --- a/selfservice/flow/login/flow.go +++ b/selfservice/flow/login/flow.go @@ -231,15 +231,8 @@ func (f *Flow) EnsureInternalContext() { } } -func (f *Flow) DuplicateCredentials() (*flow.RegistrationDuplicateCredentials, error) { - raw := gjson.GetBytes(f.InternalContext, flow.InternalContextDuplicateCredentialsPath) - if !raw.IsObject() { - return nil, nil - } - var creds flow.RegistrationDuplicateCredentials - err := json.Unmarshal([]byte(raw.Raw), &creds) - - return &creds, err +func (f *Flow) GetInternalContext() *sqlxx.JSONRawMessage { + return &f.InternalContext } func (f Flow) MarshalJSON() ([]byte, error) { diff --git a/selfservice/flow/login/flow_test.go b/selfservice/flow/login/flow_test.go index 1c7f1e200a53..c33ac1dc99e3 100644 --- a/selfservice/flow/login/flow_test.go +++ b/selfservice/flow/login/flow_test.go @@ -17,6 +17,7 @@ import ( "github.com/tidwall/gjson" "github.com/ory/x/jsonx" + "github.com/ory/x/sqlxx" "github.com/ory/kratos/driver/config" "github.com/ory/kratos/identity" @@ -35,6 +36,7 @@ import ( ) func TestFakeFlow(t *testing.T) { + t.Parallel() var r login.Flow require.NoError(t, faker.FakeData(&r)) @@ -47,6 +49,7 @@ func TestFakeFlow(t *testing.T) { } func TestNewFlow(t *testing.T) { + t.Parallel() ctx := context.Background() conf, _ := internal.NewFastRegistryWithMocks(t) @@ -130,6 +133,7 @@ func TestNewFlow(t *testing.T) { } func TestFlow(t *testing.T) { + t.Parallel() r := &login.Flow{ID: x.NewUUID()} assert.Equal(t, r.ID, r.GetID()) @@ -154,6 +158,7 @@ func TestFlow(t *testing.T) { } func TestGetType(t *testing.T) { + t.Parallel() for _, ft := range []flow.Type{ flow.TypeAPI, flow.TypeBrowser, @@ -166,18 +171,21 @@ func TestGetType(t *testing.T) { } func TestGetRequestURL(t *testing.T) { + t.Parallel() expectedURL := "http://foo/bar/baz" f := &login.Flow{RequestURL: expectedURL} assert.Equal(t, expectedURL, f.GetRequestURL()) } func TestFlowEncodeJSON(t *testing.T) { + t.Parallel() assert.EqualValues(t, "", gjson.Get(jsonx.TestMarshalJSONString(t, &login.Flow{RequestURL: "https://foo.bar?foo=bar"}), "return_to").String()) assert.EqualValues(t, "/bar", gjson.Get(jsonx.TestMarshalJSONString(t, &login.Flow{RequestURL: "https://foo.bar?return_to=/bar"}), "return_to").String()) assert.EqualValues(t, "/bar", gjson.Get(jsonx.TestMarshalJSONString(t, login.Flow{RequestURL: "https://foo.bar?return_to=/bar"}), "return_to").String()) } func TestFlowDontOverrideReturnTo(t *testing.T) { + t.Parallel() f := &login.Flow{ReturnTo: "/foo"} f.SetReturnTo() assert.Equal(t, "/foo", f.ReturnTo) @@ -186,3 +194,29 @@ func TestFlowDontOverrideReturnTo(t *testing.T) { f.SetReturnTo() assert.Equal(t, "/bar", f.ReturnTo) } + +func TestDuplicateCredentials(t *testing.T) { + t.Parallel() + t.Run("case=returns previous data", func(t *testing.T) { + t.Parallel() + f := new(login.Flow) + dc := flow.DuplicateCredentialsData{ + CredentialsType: "foo", + CredentialsConfig: sqlxx.JSONRawMessage(`{"bar":"baz"}`), + DuplicateIdentifier: "bar", + } + + require.NoError(t, flow.SetDuplicateCredentials(f, dc)) + actual, err := flow.DuplicateCredentials(f) + require.NoError(t, err) + assert.Equal(t, dc, *actual) + }) + + t.Run("case=returns nil data", func(t *testing.T) { + t.Parallel() + f := new(login.Flow) + actual, err := flow.DuplicateCredentials(f) + require.NoError(t, err) + assert.Nil(t, actual) + }) +} diff --git a/selfservice/flow/login/hook.go b/selfservice/flow/login/hook.go index 456ee7a9705b..68b27943d0cb 100644 --- a/selfservice/flow/login/hook.go +++ b/selfservice/flow/login/hook.go @@ -331,7 +331,7 @@ func (e *HookExecutor) PreLoginHook(w http.ResponseWriter, r *http.Request, a *F // maybeLinkCredentials links the identity with the credentials of the inner context of the login flow. func (e *HookExecutor) maybeLinkCredentials(r *http.Request, s *session.Session, i *identity.Identity, f *Flow) error { - lc, err := f.DuplicateCredentials() + lc, err := flow.DuplicateCredentials(f) if err != nil { return err } else if lc == nil { diff --git a/selfservice/flow/registration/flow.go b/selfservice/flow/registration/flow.go index 2f1ea3603b40..9a2b7c9cbdc4 100644 --- a/selfservice/flow/registration/flow.go +++ b/selfservice/flow/registration/flow.go @@ -202,6 +202,10 @@ func (f *Flow) EnsureInternalContext() { } } +func (f *Flow) GetInternalContext() *sqlxx.JSONRawMessage { + return &f.InternalContext +} + func (f Flow) MarshalJSON() ([]byte, error) { type local Flow f.SetReturnTo() diff --git a/selfservice/flow/registration/hook.go b/selfservice/flow/registration/hook.go index 9b8c346f4123..85d6078b5594 100644 --- a/selfservice/flow/registration/hook.go +++ b/selfservice/flow/registration/hook.go @@ -11,7 +11,6 @@ import ( "github.com/julienschmidt/httprouter" "github.com/pkg/errors" - "github.com/tidwall/sjson" "go.opentelemetry.io/otel/attribute" "github.com/ory/kratos/driver/config" @@ -102,7 +101,7 @@ func NewHookExecutor(d executorDependencies) *HookExecutor { return &HookExecutor{d: d} } -func (e *HookExecutor) PostRegistrationHook(w http.ResponseWriter, r *http.Request, ct identity.CredentialsType, provider string, a *Flow, i *identity.Identity) (err error) { +func (e *HookExecutor) PostRegistrationHook(w http.ResponseWriter, r *http.Request, ct identity.CredentialsType, provider string, registrationFlow *Flow, i *identity.Identity) (err error) { ctx := r.Context() ctx, span := e.d.Tracer(ctx).Tracer().Start(ctx, "HookExecutor.PostRegistrationHook") r = r.WithContext(ctx) @@ -114,7 +113,7 @@ func (e *HookExecutor) PostRegistrationHook(w http.ResponseWriter, r *http.Reque WithField("flow_method", ct). Debug("Running PostRegistrationPrePersistHooks.") for k, executor := range e.d.PostRegistrationPrePersistHooks(r.Context(), ct) { - if err := executor.ExecutePostRegistrationPrePersistHook(w, r, a, i); err != nil { + if err := executor.ExecutePostRegistrationPrePersistHook(w, r, registrationFlow, i); err != nil { if errors.Is(err, ErrHookAbortFlow) { e.d.Logger(). WithRequest(r). @@ -138,7 +137,7 @@ func (e *HookExecutor) PostRegistrationHook(w http.ResponseWriter, r *http.Reque Error("ExecutePostRegistrationPostPersistHook hook failed with an error.") traits := i.Traits - return flow.HandleHookError(w, r, a, traits, ct.ToUiNodeGroup(), err, e.d, e.d) + return flow.HandleHookError(w, r, registrationFlow, traits, ct.ToUiNodeGroup(), err, e.d, e.d) } e.d.Logger().WithRequest(r). @@ -169,14 +168,13 @@ func (e *HookExecutor) PostRegistrationHook(w http.ResponseWriter, r *http.Reque if err != nil { return err } - registrationDuplicateCredentials := flow.RegistrationDuplicateCredentials{ + registrationDuplicateCredentials := flow.DuplicateCredentialsData{ CredentialsType: ct, CredentialsConfig: i.Credentials[ct].Config, DuplicateIdentifier: duplicateIdentifier, } - a.InternalContext, err = sjson.SetBytes(a.InternalContext, flow.InternalContextDuplicateCredentialsPath, - registrationDuplicateCredentials) + err = flow.SetDuplicateCredentials(registrationFlow, registrationDuplicateCredentials) if err != nil { return err } @@ -188,8 +186,8 @@ func (e *HookExecutor) PostRegistrationHook(w http.ResponseWriter, r *http.Reque // Verify the redirect URL before we do any other processing. c := e.d.Config() returnTo, err := x.SecureRedirectTo(r, c.SelfServiceBrowserDefaultReturnTo(r.Context()), - x.SecureRedirectReturnTo(a.ReturnTo), - x.SecureRedirectUseSourceURL(a.RequestURL), + x.SecureRedirectReturnTo(registrationFlow.ReturnTo), + x.SecureRedirectUseSourceURL(registrationFlow.RequestURL), x.SecureRedirectAllowURLs(c.SelfServiceBrowserAllowedReturnToDomains(r.Context())), x.SecureRedirectAllowSelfServiceURLs(c.SelfPublicURL(r.Context())), x.SecureRedirectOverrideDefaultReturnTo(c.SelfServiceFlowRegistrationReturnTo(r.Context(), ct.String())), @@ -208,7 +206,7 @@ func (e *HookExecutor) PostRegistrationHook(w http.ResponseWriter, r *http.Reque WithField("identity_id", i.ID). Info("A new identity has registered using self-service registration.") - span.AddEvent(events.NewRegistrationSucceeded(r.Context(), i.ID, string(a.Type), a.Active.String(), provider)) + span.AddEvent(events.NewRegistrationSucceeded(r.Context(), i.ID, string(registrationFlow.Type), registrationFlow.Active.String(), provider)) s := session.NewInactiveSession() @@ -230,7 +228,7 @@ func (e *HookExecutor) PostRegistrationHook(w http.ResponseWriter, r *http.Reque WithField("flow_method", ct). Debug("Running PostRegistrationPostPersistHooks.") for k, executor := range e.d.PostRegistrationPostPersistHooks(r.Context(), ct) { - if err := executor.ExecutePostRegistrationPostPersistHook(w, r, a, s); err != nil { + if err := executor.ExecutePostRegistrationPostPersistHook(w, r, registrationFlow, s); err != nil { if errors.Is(err, ErrHookAbortFlow) { e.d.Logger(). WithRequest(r). @@ -259,7 +257,7 @@ func (e *HookExecutor) PostRegistrationHook(w http.ResponseWriter, r *http.Reque span.SetAttributes(attribute.String("redirect_reason", "hook error"), attribute.String("executor", fmt.Sprintf("%T", executor))) traits := i.Traits - return flow.HandleHookError(w, r, a, traits, ct.ToUiNodeGroup(), err, e.d, e.d) + return flow.HandleHookError(w, r, registrationFlow, traits, ct.ToUiNodeGroup(), err, e.d, e.d) } e.d.Logger().WithRequest(r). @@ -277,13 +275,13 @@ func (e *HookExecutor) PostRegistrationHook(w http.ResponseWriter, r *http.Reque WithField("identity_id", i.ID). Debug("Post registration execution hooks completed successfully.") - if a.Type == flow.TypeAPI || x.IsJSONRequest(r) { + if registrationFlow.Type == flow.TypeAPI || x.IsJSONRequest(r) { span.SetAttributes(attribute.String("flow_type", string(flow.TypeAPI))) - if a.IDToken != "" { + if registrationFlow.IDToken != "" { // We don't want to redirect with the code, if the flow was submitted with an ID token. // This is the case for Sign in with native Apple SDK or Google SDK. - } else if handled, err := e.d.SessionManager().MaybeRedirectAPICodeFlow(w, r, a, s.ID, ct.ToUiNodeGroup()); err != nil { + } else if handled, err := e.d.SessionManager().MaybeRedirectAPICodeFlow(w, r, registrationFlow, s.ID, ct.ToUiNodeGroup()); err != nil { return errors.WithStack(err) } else if handled { return nil @@ -291,21 +289,21 @@ func (e *HookExecutor) PostRegistrationHook(w http.ResponseWriter, r *http.Reque e.d.Writer().Write(w, r, &APIFlowResponse{ Identity: i, - ContinueWith: a.ContinueWith(), + ContinueWith: registrationFlow.ContinueWith(), }) return nil } finalReturnTo := returnTo.String() - if a.OAuth2LoginChallenge != "" { - if a.ReturnToVerification != "" { + if registrationFlow.OAuth2LoginChallenge != "" { + if registrationFlow.ReturnToVerification != "" { // Special case: If Kratos is used as a login UI *and* we want to show the verification UI, // redirect to the verification URL first and then return to Hydra. - finalReturnTo = a.ReturnToVerification + finalReturnTo = registrationFlow.ReturnToVerification } else { callbackURL, err := e.d.Hydra().AcceptLoginRequest(r.Context(), hydra.AcceptLoginRequestParams{ - LoginChallenge: string(a.OAuth2LoginChallenge), + LoginChallenge: string(registrationFlow.OAuth2LoginChallenge), IdentityID: i.ID.String(), SessionID: s.ID.String(), AuthenticationMethods: s.AMR, @@ -316,8 +314,8 @@ func (e *HookExecutor) PostRegistrationHook(w http.ResponseWriter, r *http.Reque finalReturnTo = callbackURL } span.SetAttributes(attribute.String("redirect_reason", "oauth2 login challenge")) - } else if a.ReturnToVerification != "" { - finalReturnTo = a.ReturnToVerification + } else if registrationFlow.ReturnToVerification != "" { + finalReturnTo = registrationFlow.ReturnToVerification span.SetAttributes(attribute.String("redirect_reason", "verification requested")) } span.SetAttributes(attribute.String("return_to", finalReturnTo)) diff --git a/selfservice/strategy/oidc/strategy.go b/selfservice/strategy/oidc/strategy.go index d58f2615ed17..b22e06c6fcc9 100644 --- a/selfservice/strategy/oidc/strategy.go +++ b/selfservice/strategy/oidc/strategy.go @@ -551,10 +551,31 @@ func (s *Strategy) handleError(w http.ResponseWriter, r *http.Request, f flow.Fl } // return a new login flow with the error message embedded in the login flow. redirectURL := lf.AppendTo(s.d.Config().SelfServiceFlowLoginUI(r.Context())) - if dc, err := lf.DuplicateCredentials(); err == nil && dc != nil { + if dc, err := flow.DuplicateCredentials(lf); err == nil && dc != nil { q := redirectURL.Query() q.Set("no_org_ui", "true") redirectURL.RawQuery = q.Encode() + + for i, n := range lf.UI.Nodes { + if n.Meta == nil || n.Meta.Label == nil { + continue + } + switch n.Meta.Label.ID { + case text.InfoSelfServiceLogin: + lf.UI.Nodes[i].Meta.Label = text.NewInfoLoginAndLink() + case text.InfoSelfServiceLoginWith: + p := gjson.GetBytes(n.Meta.Label.Context, "provider").String() + lf.UI.Nodes[i].Meta.Label = text.NewInfoLoginWithAndLink(p) + } + } + + newLoginURL := s.d.Config().SelfServiceFlowLoginUI(r.Context()).String() + lf.UI.Messages.Add(text.NewInfoLoginLinkMessage(dc.DuplicateIdentifier, provider, newLoginURL)) + + err := s.d.LoginFlowPersister().UpdateLoginFlow(r.Context(), lf) + if err != nil { + return err + } } x.AcceptToRedirectOrJSON(w, r, s.d.Writer(), lf, redirectURL.String()) // ensure the function does not continue to execute @@ -638,12 +659,8 @@ func (s *Strategy) processIDToken(w http.ResponseWriter, r *http.Request, provid } func (s *Strategy) linkCredentials(ctx context.Context, i *identity.Identity, idToken, accessToken, refreshToken, provider, subject, organization string) error { - if i.Credentials == nil { - confidential, err := s.d.PrivilegedIdentityPool().GetIdentityConfidential(ctx, i.ID) - if err != nil { - return err - } - i.Credentials = confidential.Credentials + if err := s.d.PrivilegedIdentityPool().HydrateIdentityAssociations(ctx, i, identity.ExpandCredentials); err != nil { + return err } var conf identity.CredentialsOIDC creds, err := i.ParseCredentials(s.ID(), &conf) diff --git a/selfservice/strategy/oidc/strategy_test.go b/selfservice/strategy/oidc/strategy_test.go index a39564baf327..a278cd6dcb53 100644 --- a/selfservice/strategy/oidc/strategy_test.go +++ b/selfservice/strategy/oidc/strategy_test.go @@ -1087,7 +1087,7 @@ func TestStrategy(t *testing.T) { checkCredentialsLinked := func(res *http.Response, body []byte, identityID uuid.UUID, provider string) { assert.Contains(t, res.Request.URL.String(), returnTS.URL, "%s", body) - assert.Equal(t, subject, gjson.GetBytes(body, "identity.traits.subject").String(), "%s", body) + assert.Equal(t, strings.ToLower(subject), gjson.GetBytes(body, "identity.traits.subject").String(), "%s", body) i, err := reg.PrivilegedIdentityPool().GetIdentityConfidential(ctx, identityID) require.NoError(t, err) assert.NotEmpty(t, i.Credentials["oidc"], "%+v", i.Credentials) @@ -1132,6 +1132,9 @@ func TestStrategy(t *testing.T) { CSRFToken string } + // To test that the subject is normalized properly + subject = strings.ToUpper(subject) + t.Run("step=should fail login and start a new flow", func(t *testing.T) { res, body := loginWithOIDC(t, client, loginFlow.ID, "valid") assert.True(t, res.Request.URL.Query().Has("no_org_ui")) diff --git a/text/id.go b/text/id.go index 24cca62f4a5b..fa8184f0ffaf 100644 --- a/text/id.go +++ b/text/id.go @@ -25,7 +25,9 @@ const ( InfoSelfServiceLoginContinue // 1010013 InfoSelfServiceLoginEmailWithCodeSent // 1010014 InfoSelfServiceLoginCode // 1010015 - InfoSelfServiceLoginLinkCredentials // 1010016 + InfoSelfServiceLoginLink // 1010016 + InfoSelfServiceLoginAndLink // 1010017 + InfoSelfServiceLoginWithAndLink // 1010018 ) const ( diff --git a/text/message_login.go b/text/message_login.go index 5abfab44dfb3..4918cb893701 100644 --- a/text/message_login.go +++ b/text/message_login.go @@ -56,6 +56,31 @@ func NewInfoLogin() *Message { } } +func NewInfoLoginLinkMessage(dupIdentifier, provider, newLoginURL string) *Message { + return &Message{ + ID: InfoSelfServiceLoginLink, + Type: Info, + Text: fmt.Sprintf( + "Signing in will link your account to %q at provider %q. If you do not wish to link that account, please start a new login flow.", + dupIdentifier, + provider, + ), + Context: context(map[string]any{ + "duplicateIdentifier": dupIdentifier, + "provider": provider, + "newLoginUrl": newLoginURL, + }), + } +} + +func NewInfoLoginAndLink() *Message { + return &Message{ + ID: InfoSelfServiceLoginAndLink, + Text: "Sign in and link", + Type: Info, + } +} + func NewInfoLoginTOTP() *Message { return &Message{ ID: InfoLoginTOTP, @@ -91,6 +116,18 @@ func NewInfoLoginWith(provider string) *Message { } } +func NewInfoLoginWithAndLink(provider string) *Message { + + return &Message{ + ID: InfoSelfServiceLoginWithAndLink, + Text: fmt.Sprintf("Sign in with %s and link credential", provider), + Type: Info, + Context: context(map[string]any{ + "provider": provider, + }), + } +} + func NewErrorValidationLoginFlowExpired(expiredAt time.Time) *Message { return &Message{ ID: ErrorValidationLoginFlowExpired, From 1297952ceb3799da085a2812feeeb29955080baf Mon Sep 17 00:00:00 2001 From: Henning Perl Date: Mon, 30 Oct 2023 11:48:37 +0100 Subject: [PATCH 18/29] handle conflicts in recoverable, verifiable addresses --- identity/manager.go | 79 +++++++++++++++------------ selfservice/flow/login/hook.go | 14 ++--- selfservice/flow/registration/hook.go | 16 ++---- 3 files changed, 54 insertions(+), 55 deletions(-) diff --git a/identity/manager.go b/identity/manager.go index 11220ec15f12..9bd02ce7451c 100644 --- a/identity/manager.go +++ b/identity/manager.go @@ -110,12 +110,7 @@ func (m *Manager) Create(ctx context.Context, i *Identity, opts ...ManagerOption return nil } -func (m *Manager) findExistingAuthMethod(ctx context.Context, e error, i *Identity) (err error) { - if !m.r.Config().SelfServiceFlowRegistrationLoginHints(ctx) { - return &ErrDuplicateCredentials{error: e} - } - // First we try to find the conflict in the identifiers table. This is most likely to have a conflict. - var found *Identity +func (m *Manager) ConflictingIdentity(ctx context.Context, i *Identity) (found *Identity, foundConflictAddress string, err error) { for ct, cred := range i.Credentials { for _, id := range cred.Identifiers { found, _, err = m.r.PrivilegedIdentityPool().FindByCredentialsIdentifier(ctx, ct, id) @@ -125,53 +120,65 @@ func (m *Manager) findExistingAuthMethod(ctx context.Context, e error, i *Identi // FindByCredentialsIdentifier does not expand identity credentials. if err = m.r.PrivilegedIdentityPool().HydrateIdentityAssociations(ctx, found, ExpandCredentials); err != nil { - return err + return nil, "", err } + + return found, id, nil } } // If the conflict is not in the identifiers table, it is coming from the verifiable or recovery address. - var foundConflictAddress string - if found == nil { - for _, va := range i.VerifiableAddresses { - conflictingAddress, err := m.r.PrivilegedIdentityPool().FindVerifiableAddressByValue(ctx, va.Via, va.Value) - if errors.Is(err, sqlcon.ErrNoRows) { - continue - } else if err != nil { - return err - } + for _, va := range i.VerifiableAddresses { + conflictingAddress, err := m.r.PrivilegedIdentityPool().FindVerifiableAddressByValue(ctx, va.Via, va.Value) + if errors.Is(err, sqlcon.ErrNoRows) { + continue + } else if err != nil { + return nil, "", err + } - foundConflictAddress = conflictingAddress.Value - found, err = m.r.PrivilegedIdentityPool().GetIdentity(ctx, conflictingAddress.IdentityID, ExpandCredentials) - if err != nil { - return err - } + foundConflictAddress = conflictingAddress.Value + found, err = m.r.PrivilegedIdentityPool().GetIdentity(ctx, conflictingAddress.IdentityID, ExpandCredentials) + if err != nil { + return nil, "", err } + + return found, foundConflictAddress, nil } // Last option: check the recovery address - if found == nil { - for _, va := range i.RecoveryAddresses { - conflictingAddress, err := m.r.PrivilegedIdentityPool().FindRecoveryAddressByValue(ctx, va.Via, va.Value) - if errors.Is(err, sqlcon.ErrNoRows) { - continue - } else if err != nil { - return err - } + for _, va := range i.RecoveryAddresses { + conflictingAddress, err := m.r.PrivilegedIdentityPool().FindRecoveryAddressByValue(ctx, va.Via, va.Value) + if errors.Is(err, sqlcon.ErrNoRows) { + continue + } else if err != nil { + return nil, "", err + } - foundConflictAddress = conflictingAddress.Value - found, err = m.r.PrivilegedIdentityPool().GetIdentity(ctx, conflictingAddress.IdentityID, ExpandCredentials) - if err != nil { - return err - } + foundConflictAddress = conflictingAddress.Value + found, err = m.r.PrivilegedIdentityPool().GetIdentity(ctx, conflictingAddress.IdentityID, ExpandCredentials) + if err != nil { + return nil, "", err } + + return found, foundConflictAddress, nil } - // Still not found? Return generic error. - if found == nil { + return nil, "", sqlcon.ErrNoRows +} + +func (m *Manager) findExistingAuthMethod(ctx context.Context, e error, i *Identity) (err error) { + if !m.r.Config().SelfServiceFlowRegistrationLoginHints(ctx) { return &ErrDuplicateCredentials{error: e} } + found, foundConflictAddress, err := m.ConflictingIdentity(ctx, i) + if err != nil { + if errors.Is(err, sqlcon.ErrNoRows) { + return &ErrDuplicateCredentials{error: e} + } + return err + } + // We need to sort the credentials for the error message to be deterministic. var creds []Credentials for _, cred := range found.Credentials { diff --git a/selfservice/flow/login/hook.go b/selfservice/flow/login/hook.go index 68b27943d0cb..271723635340 100644 --- a/selfservice/flow/login/hook.go +++ b/selfservice/flow/login/hook.go @@ -132,7 +132,7 @@ func (e *HookExecutor) PostLoginHook( r = r.WithContext(ctx) defer otelx.End(span, &err) - if err := e.maybeLinkCredentials(r, s, i, a); err != nil { + if err := e.maybeLinkCredentials(r.Context(), s, i, a); err != nil { return err } @@ -330,15 +330,15 @@ func (e *HookExecutor) PreLoginHook(w http.ResponseWriter, r *http.Request, a *F } // maybeLinkCredentials links the identity with the credentials of the inner context of the login flow. -func (e *HookExecutor) maybeLinkCredentials(r *http.Request, s *session.Session, i *identity.Identity, f *Flow) error { - lc, err := flow.DuplicateCredentials(f) +func (e *HookExecutor) maybeLinkCredentials(ctx context.Context, sess *session.Session, ident *identity.Identity, loginFlow *Flow) error { + lc, err := flow.DuplicateCredentials(loginFlow) if err != nil { return err } else if lc == nil { return nil } - if err := e.checkDuplicateCredentialsIdentifierMatch(r.Context(), i.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) @@ -352,12 +352,12 @@ func (e *HookExecutor) maybeLinkCredentials(r *http.Request, s *session.Session, return fmt.Errorf("strategy is not linkable: %T", linkableStrategy) } - if err := linkableStrategy.Link(r.Context(), i, lc.CredentialsConfig); err != nil { + if err := linkableStrategy.Link(ctx, ident, lc.CredentialsConfig); err != nil { return err } - method := strategy.CompletedAuthenticationMethod(r.Context()) - s.CompletedLoginForMethod(method) + method := strategy.CompletedAuthenticationMethod(ctx) + sess.CompletedLoginForMethod(method) return nil } diff --git a/selfservice/flow/registration/hook.go b/selfservice/flow/registration/hook.go index 85d6078b5594..a8582b5dca19 100644 --- a/selfservice/flow/registration/hook.go +++ b/selfservice/flow/registration/hook.go @@ -325,19 +325,11 @@ func (e *HookExecutor) PostRegistrationHook(w http.ResponseWriter, r *http.Reque } func (e *HookExecutor) getDuplicateIdentifier(ctx context.Context, i *identity.Identity) (string, error) { - for ct, credentials := range i.Credentials { - for _, identifier := range credentials.Identifiers { - _, _, err := e.d.PrivilegedIdentityPool().FindByCredentialsIdentifier(ctx, ct, identifier) - if err != nil { - if errors.Is(err, sqlcon.ErrNoRows) { - continue - } - return "", err - } - return identifier, nil - } + _, id, err := e.d.IdentityManager().ConflictingIdentity(ctx, i) + if err != nil { + return "", err } - return "", errors.New("Duplicate credential not found") + return id, nil } func (e *HookExecutor) PreRegistrationHook(w http.ResponseWriter, r *http.Request, a *Flow) error { From 6140287af3c76af97a0c6464ebf5e1b6d1a18ab2 Mon Sep 17 00:00:00 2001 From: Henning Perl Date: Mon, 30 Oct 2023 14:44:24 +0100 Subject: [PATCH 19/29] chore: formatting --- selfservice/flow/duplicate_credentials.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/selfservice/flow/duplicate_credentials.go b/selfservice/flow/duplicate_credentials.go index 9249aaacd60e..de060fbce894 100644 --- a/selfservice/flow/duplicate_credentials.go +++ b/selfservice/flow/duplicate_credentials.go @@ -1,3 +1,6 @@ +// Copyright © 2023 Ory Corp +// SPDX-License-Identifier: Apache-2.0 + package flow import ( From 81a31cb90cc15b2de097b327cabfab18025fc9b6 Mon Sep 17 00:00:00 2001 From: aeneasr <3372410+aeneasr@users.noreply.github.com> Date: Tue, 31 Oct 2023 10:43:54 +0100 Subject: [PATCH 20/29] chore: code review --- selfservice/flow/login/hook.go | 2 +- selfservice/flow/registration/hook.go | 7 ++----- selfservice/strategy/oidc/strategy.go | 12 ++++-------- ui/node/identifiers.go | 4 ---- 4 files changed, 7 insertions(+), 18 deletions(-) diff --git a/selfservice/flow/login/hook.go b/selfservice/flow/login/hook.go index 271723635340..2af0c3daf1fc 100644 --- a/selfservice/flow/login/hook.go +++ b/selfservice/flow/login/hook.go @@ -349,7 +349,7 @@ func (e *HookExecutor) maybeLinkCredentials(ctx context.Context, sess *session.S linkableStrategy, ok := strategy.(LinkableStrategy) if !ok { // This should never happen because we check for this in the registration flow. - return fmt.Errorf("strategy is not linkable: %T", linkableStrategy) + return errors.Errorf("strategy is not linkable: %T", linkableStrategy) } if err := linkableStrategy.Link(ctx, ident, lc.CredentialsConfig); err != nil { diff --git a/selfservice/flow/registration/hook.go b/selfservice/flow/registration/hook.go index a8582b5dca19..6a997009c1c5 100644 --- a/selfservice/flow/registration/hook.go +++ b/selfservice/flow/registration/hook.go @@ -161,9 +161,7 @@ func (e *HookExecutor) PostRegistrationHook(w http.ResponseWriter, r *http.Reque return err } - _, ok := strategy.(login.LinkableStrategy) - - if ok { + if _, ok := strategy.(login.LinkableStrategy); ok { duplicateIdentifier, err := e.getDuplicateIdentifier(r.Context(), i) if err != nil { return err @@ -174,8 +172,7 @@ func (e *HookExecutor) PostRegistrationHook(w http.ResponseWriter, r *http.Reque DuplicateIdentifier: duplicateIdentifier, } - err = flow.SetDuplicateCredentials(registrationFlow, registrationDuplicateCredentials) - if err != nil { + if err := flow.SetDuplicateCredentials(registrationFlow, registrationDuplicateCredentials); err != nil { return err } } diff --git a/selfservice/strategy/oidc/strategy.go b/selfservice/strategy/oidc/strategy.go index 9a04098c4290..f599c0ee7101 100644 --- a/selfservice/strategy/oidc/strategy.go +++ b/selfservice/strategy/oidc/strategy.go @@ -10,7 +10,9 @@ import ( "encoding/base64" "encoding/json" "fmt" + "github.com/ory/x/urlx" "net/http" + "net/url" "path/filepath" "strings" @@ -552,9 +554,7 @@ func (s *Strategy) handleError(w http.ResponseWriter, r *http.Request, f flow.Fl // return a new login flow with the error message embedded in the login flow. redirectURL := lf.AppendTo(s.d.Config().SelfServiceFlowLoginUI(r.Context())) if dc, err := flow.DuplicateCredentials(lf); err == nil && dc != nil { - q := redirectURL.Query() - q.Set("no_org_ui", "true") - redirectURL.RawQuery = q.Encode() + redirectURL = urlx.CopyWithQuery(redirectURL, url.Values{"no_org_ui": {"true"}}) for i, n := range lf.UI.Nodes { if n.Meta == nil || n.Meta.Label == nil { @@ -688,11 +688,7 @@ func (s *Strategy) linkCredentials(ctx context.Context, i *identity.Identity, id } i.Credentials[s.ID()] = *creds - if organization != "" { - orgID, err := uuid.FromString(organization) - if err != nil { - return err - } + if orgID, err := uuid.FromString(organization); err == nil { i.OrganizationID = uuid.NullUUID{UUID: orgID, Valid: true} } diff --git a/ui/node/identifiers.go b/ui/node/identifiers.go index c5c310fbf387..17ed2dd88c27 100644 --- a/ui/node/identifiers.go +++ b/ui/node/identifiers.go @@ -28,7 +28,3 @@ const ( WebAuthnRemove = "webauthn_remove" WebAuthnScript = "webauthn_script" ) - -const ( - LoginAndLinkCredentials = "login_and_link_credentials" // #nosec G101 -) From d844c6edbe089ce5527e9820b7f25271f56c9a15 Mon Sep 17 00:00:00 2001 From: Henning Perl Date: Tue, 31 Oct 2023 12:18:29 +0100 Subject: [PATCH 21/29] test: add Cypress e2e test Signed-off-by: Henning Perl --- selfservice/strategy/oidc/strategy.go | 3 +- .../profiles/oidc/login/success.spec.ts | 52 ++++++++++++++++--- 2 files changed, 46 insertions(+), 9 deletions(-) diff --git a/selfservice/strategy/oidc/strategy.go b/selfservice/strategy/oidc/strategy.go index f599c0ee7101..af75587f2e10 100644 --- a/selfservice/strategy/oidc/strategy.go +++ b/selfservice/strategy/oidc/strategy.go @@ -10,12 +10,13 @@ import ( "encoding/base64" "encoding/json" "fmt" - "github.com/ory/x/urlx" "net/http" "net/url" "path/filepath" "strings" + "github.com/ory/x/urlx" + "go.opentelemetry.io/otel/attribute" "golang.org/x/oauth2" diff --git a/test/e2e/cypress/integration/profiles/oidc/login/success.spec.ts b/test/e2e/cypress/integration/profiles/oidc/login/success.spec.ts index 51ccf8574a6f..82f98756eab7 100644 --- a/test/e2e/cypress/integration/profiles/oidc/login/success.spec.ts +++ b/test/e2e/cypress/integration/profiles/oidc/login/success.spec.ts @@ -1,24 +1,26 @@ // Copyright © 2023 Ory Corp // SPDX-License-Identifier: Apache-2.0 -import { gen, website } from "../../../../helpers" +import { appPrefix, gen, website } from "../../../../helpers" import { routes as express } from "../../../../helpers/express" import { routes as react } from "../../../../helpers/react" context("Social Sign In Successes", () => { ;[ - { - login: react.login, - registration: react.registration, - app: "react" as "react", - profile: "spa", - }, + // { + // login: react.login, + // registration: react.registration, + // settings: react.settings, + // app: "react" as "react", + // profile: "spa", + // }, { login: express.login, registration: express.registration, + settings: express.settings, app: "express" as "express", profile: "oidc", }, - ].forEach(({ login, registration, profile, app }) => { + ].forEach(({ login, registration, profile, app, settings }) => { describe(`for app ${app}`, () => { before(() => { cy.useConfigProfile(profile) @@ -37,6 +39,40 @@ context("Social Sign In Successes", () => { cy.loginOidc({ app, url: login }) }) + it.only("should be able to sign up and link existing account", () => { + const email = gen.email() + const password = gen.password() + + // Create a new account + cy.registerApi({ + email, + password, + fields: { "traits.website": website }, + }) + + // Try to log in with the same identifier through OIDC. This should fail and create a new login flow. + cy.registerOidc({ + app, + email, + website, + expectSession: false, + }) + cy.noSession() + + // Log in with the same identifier through the login flow. This should link the accounts. + cy.get(`${appPrefix(app)}input[name="identifier"]`).type(email) + cy.get('input[name="password"]').type(password) + cy.submitPasswordForm() + cy.location("pathname").should("not.contain", "/login") + cy.getSession() + + // Hydra OIDC should now be linked + cy.visit(settings) + cy.get('[value="hydra"]') + .should("have.attr", "name", "unlink") + .should("contain.text", "Unlink hydra") + }) + it("should be able to sign up with redirects", () => { const email = gen.email() cy.registerOidc({ From c7c205b08eaaaa3d723586e5ae1de30068ead0b8 Mon Sep 17 00:00:00 2001 From: Henning Perl Date: Tue, 31 Oct 2023 13:04:10 +0100 Subject: [PATCH 22/29] test: add ConflictingIdentity tests Signed-off-by: Henning Perl --- identity/manager_test.go | 63 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 63 insertions(+) diff --git a/identity/manager_test.go b/identity/manager_test.go index 800d84590470..81001c9ba9c6 100644 --- a/identity/manager_test.go +++ b/identity/manager_test.go @@ -10,6 +10,7 @@ import ( "time" "github.com/ory/x/pointerx" + "github.com/ory/x/sqlcon" "github.com/gofrs/uuid" @@ -556,6 +557,68 @@ func TestManager(t *testing.T) { checkExtensionFields(fromStore, "email-updatetraits-1@ory.sh")(t) }) }) + + t.Run("method=ConflictingIdentity", func(t *testing.T) { + ctx := context.Background() + + conflicOnIdentifier := identity.NewIdentity(config.DefaultIdentityTraitsSchemaID) + conflicOnIdentifier.Traits = identity.Traits(`{"email":"conflict-on-identifier@example.com"}`) + require.NoError(t, reg.IdentityManager().Create(ctx, conflicOnIdentifier)) + + conflicOnVerifiableAddress := identity.NewIdentity(config.DefaultIdentityTraitsSchemaID) + conflicOnVerifiableAddress.Traits = identity.Traits(`{"email":"user-va@example.com", "email_verify":"conflict-on-va@example.com"}`) + require.NoError(t, reg.IdentityManager().Create(ctx, conflicOnVerifiableAddress)) + + conflicOnRecoveryAddress := identity.NewIdentity(config.DefaultIdentityTraitsSchemaID) + conflicOnRecoveryAddress.Traits = identity.Traits(`{"email":"user-ra@example.com", "email_recovery":"conflict-on-ra@example.com"}`) + require.NoError(t, reg.IdentityManager().Create(ctx, conflicOnRecoveryAddress)) + + t.Run("case=returns not found if no conflict", func(t *testing.T) { + found, foundConflictAddress, err := reg.IdentityManager().ConflictingIdentity(ctx, &identity.Identity{ + Credentials: map[identity.CredentialsType]identity.Credentials{ + identity.CredentialsTypePassword: {Identifiers: []string{"no-conflict@example.com"}}, + }, + }) + assert.ErrorIs(t, err, sqlcon.ErrNoRows) + assert.Nil(t, found) + assert.Empty(t, foundConflictAddress) + }) + + t.Run("case=conflict on identifier", func(t *testing.T) { + found, foundConflictAddress, err := reg.IdentityManager().ConflictingIdentity(ctx, &identity.Identity{ + Credentials: map[identity.CredentialsType]identity.Credentials{ + identity.CredentialsTypePassword: {Identifiers: []string{"conflict-on-identifier@example.com"}}, + }, + }) + require.NoError(t, err) + assert.Equal(t, conflicOnIdentifier.ID, found.ID) + assert.Equal(t, "conflict-on-identifier@example.com", foundConflictAddress) + }) + + t.Run("case=conflict on verifiable address", func(t *testing.T) { + found, foundConflictAddress, err := reg.IdentityManager().ConflictingIdentity(ctx, &identity.Identity{ + VerifiableAddresses: []identity.VerifiableAddress{{ + Value: "conflict-on-va@example.com", + Via: "email", + }}, + }) + require.NoError(t, err) + assert.Equal(t, conflicOnVerifiableAddress.ID, found.ID) + assert.Equal(t, "conflict-on-va@example.com", foundConflictAddress) + }) + + t.Run("case=conflict on recovery address", func(t *testing.T) { + found, foundConflictAddress, err := reg.IdentityManager().ConflictingIdentity(ctx, &identity.Identity{ + RecoveryAddresses: []identity.RecoveryAddress{{ + Value: "conflict-on-ra@example.com", + Via: "email", + }}, + }) + require.NoError(t, err) + assert.Equal(t, conflicOnRecoveryAddress.ID, found.ID) + assert.Equal(t, "conflict-on-ra@example.com", foundConflictAddress) + }) + }) } func TestManagerNoDefaultNamedSchema(t *testing.T) { From e747d50ec21e25267fef1fb43747b6c8592bb4e1 Mon Sep 17 00:00:00 2001 From: Henning Perl Date: Tue, 31 Oct 2023 14:04:26 +0100 Subject: [PATCH 23/29] test: add test for maybeLinkCredentials Signed-off-by: Henning Perl --- selfservice/flow/login/hook_test.go | 59 +++++++++++++++++++++++++++-- 1 file changed, 56 insertions(+), 3 deletions(-) diff --git a/selfservice/flow/login/hook_test.go b/selfservice/flow/login/hook_test.go index ea3bd84bac72..052973317ca2 100644 --- a/selfservice/flow/login/hook_test.go +++ b/selfservice/flow/login/hook_test.go @@ -12,14 +12,15 @@ import ( "github.com/stretchr/testify/require" - "github.com/ory/kratos/hydra" - "github.com/ory/kratos/session" - "github.com/gobuffalo/httptest" "github.com/julienschmidt/httprouter" "github.com/stretchr/testify/assert" "github.com/tidwall/gjson" + "github.com/ory/kratos/hydra" + "github.com/ory/kratos/schema" + "github.com/ory/kratos/session" + "github.com/ory/kratos/driver/config" "github.com/ory/kratos/identity" "github.com/ory/kratos/internal" @@ -256,6 +257,58 @@ func TestLoginExecutor(t *testing.T) { assert.Contains(t, gjson.Get(body, "redirect_browser_to").String(), "/self-service/login/browser?aal=aal2", "%s", body) }) }) + + }) + t.Run("case=maybe links credential", func(t *testing.T) { + t.Cleanup(testhelpers.SelfServiceHookConfigReset(t, conf)) + + email := testhelpers.RandomEmail() + useIdentity := &identity.Identity{Credentials: map[identity.CredentialsType]identity.Credentials{ + identity.CredentialsTypePassword: { + Type: identity.CredentialsTypePassword, + Config: []byte(`{"hashed_password": "$argon2id$v=19$m=32,t=2,p=4$cm94YnRVOW5jZzFzcVE4bQ$MNzk5BtR2vUhrp6qQEjRNw"}`), + Identifiers: []string{email}, + }, + }} + require.NoError(t, reg.Persister().CreateIdentity(context.Background(), useIdentity)) + + credsOIDC, err := identity.NewCredentialsOIDC( + "id-token", + "access-token", + "refresh-token", + "my-provider", + email, + "", + ) + require.NoError(t, err) + + t.Run("sub-case=links matching identity", func(t *testing.T) { + res, _ := makeRequestPost(t, newServer(t, flow.TypeBrowser, useIdentity, func(l *login.Flow) { + require.NoError(t, flow.SetDuplicateCredentials(l, flow.DuplicateCredentialsData{ + CredentialsType: identity.CredentialsTypeOIDC, + CredentialsConfig: credsOIDC.Config, + DuplicateIdentifier: email, + })) + }), false, url.Values{}) + assert.EqualValues(t, http.StatusOK, res.StatusCode) + assert.EqualValues(t, "https://www.ory.sh/", res.Request.URL.String()) + + ident, err := reg.Persister().GetIdentity(ctx, useIdentity.ID, identity.ExpandCredentials) + require.NoError(t, err) + assert.Equal(t, 2, len(ident.Credentials)) + }) + + t.Run("sub-case=errors on non-matching identity", func(t *testing.T) { + res, body := makeRequestPost(t, newServer(t, flow.TypeBrowser, useIdentity, func(l *login.Flow) { + require.NoError(t, flow.SetDuplicateCredentials(l, flow.DuplicateCredentialsData{ + CredentialsType: identity.CredentialsTypeOIDC, + CredentialsConfig: credsOIDC.Config, + DuplicateIdentifier: "wrong@example.com", + })) + }), false, url.Values{}) + assert.EqualValues(t, http.StatusInternalServerError, res.StatusCode) + assert.Equal(t, schema.NewLinkedCredentialsDoNotMatch().Error(), body, "%s", body) + }) }) t.Run("type=api", func(t *testing.T) { From 3b1ba9b1c3a5d7c573e6a0a0432fff4115e1dcf0 Mon Sep 17 00:00:00 2001 From: Henning Perl Date: Tue, 31 Oct 2023 14:10:08 +0100 Subject: [PATCH 24/29] fix: add SetInternalContext Signed-off-by: Henning Perl --- selfservice/flow/duplicate_credentials.go | 9 +++++---- selfservice/flow/login/flow.go | 8 ++++++-- selfservice/flow/registration/flow.go | 8 ++++++-- 3 files changed, 17 insertions(+), 8 deletions(-) diff --git a/selfservice/flow/duplicate_credentials.go b/selfservice/flow/duplicate_credentials.go index de060fbce894..ddba57251ed2 100644 --- a/selfservice/flow/duplicate_credentials.go +++ b/selfservice/flow/duplicate_credentials.go @@ -23,7 +23,8 @@ type DuplicateCredentialsData struct { type InternalContexter interface { EnsureInternalContext() - GetInternalContext() *sqlxx.JSONRawMessage + GetInternalContext() sqlxx.JSONRawMessage + SetInternalContext(sqlxx.JSONRawMessage) } // SetDuplicateCredentials sets the duplicate credentials data in the flow's internal context. @@ -32,14 +33,14 @@ func SetDuplicateCredentials(flow InternalContexter, creds DuplicateCredentialsD flow.EnsureInternalContext() } bytes, err := sjson.SetBytes( - *flow.GetInternalContext(), + flow.GetInternalContext(), internalContextDuplicateCredentialsPath, creds, ) if err != nil { return err } - *flow.GetInternalContext() = bytes + flow.SetInternalContext(bytes) return nil } @@ -49,7 +50,7 @@ func DuplicateCredentials(flow InternalContexter) (*DuplicateCredentialsData, er if flow.GetInternalContext() == nil { flow.EnsureInternalContext() } - raw := gjson.GetBytes(*flow.GetInternalContext(), internalContextDuplicateCredentialsPath) + raw := gjson.GetBytes(flow.GetInternalContext(), internalContextDuplicateCredentialsPath) if !raw.IsObject() { return nil, nil } diff --git a/selfservice/flow/login/flow.go b/selfservice/flow/login/flow.go index bb5441c3b09c..5dd8422cdb84 100644 --- a/selfservice/flow/login/flow.go +++ b/selfservice/flow/login/flow.go @@ -231,8 +231,12 @@ func (f *Flow) EnsureInternalContext() { } } -func (f *Flow) GetInternalContext() *sqlxx.JSONRawMessage { - return &f.InternalContext +func (f *Flow) GetInternalContext() sqlxx.JSONRawMessage { + return f.InternalContext +} + +func (f *Flow) SetInternalContext(bytes sqlxx.JSONRawMessage) { + f.InternalContext = bytes } func (f Flow) MarshalJSON() ([]byte, error) { diff --git a/selfservice/flow/registration/flow.go b/selfservice/flow/registration/flow.go index 9a2b7c9cbdc4..b3dae8a6a1c5 100644 --- a/selfservice/flow/registration/flow.go +++ b/selfservice/flow/registration/flow.go @@ -202,8 +202,12 @@ func (f *Flow) EnsureInternalContext() { } } -func (f *Flow) GetInternalContext() *sqlxx.JSONRawMessage { - return &f.InternalContext +func (f *Flow) GetInternalContext() sqlxx.JSONRawMessage { + return f.InternalContext +} + +func (f *Flow) SetInternalContext(bytes sqlxx.JSONRawMessage) { + f.InternalContext = bytes } func (f Flow) MarshalJSON() ([]byte, error) { From c13a823e578ee54e2e7443edb79cfd9763158655 Mon Sep 17 00:00:00 2001 From: Henning Perl Date: Tue, 31 Oct 2023 14:11:43 +0100 Subject: [PATCH 25/29] fix: error message Signed-off-by: Henning Perl --- schema/errors.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/schema/errors.go b/schema/errors.go index ee9a22c08f59..d72e02af7d11 100644 --- a/schema/errors.go +++ b/schema/errors.go @@ -353,7 +353,7 @@ func NewLoginCodeInvalid() error { func NewLinkedCredentialsDoNotMatch() error { return errors.WithStack(&ValidationError{ ValidationError: &jsonschema.ValidationError{ - Message: `linked credentials do not match`, + Message: `linked credentials do not match; please start a new flow`, InstancePtr: "#/", }, Messages: new(text.Messages).Add(text.NewErrorValidationLoginLinkedCredentialsDoNotMatch()), From bc26f7845a42d300b5bf40bd8b140ab54db4a8fe Mon Sep 17 00:00:00 2001 From: Henning Perl Date: Mon, 6 Nov 2023 15:15:58 +0100 Subject: [PATCH 26/29] fix: uncomment test --- .../profiles/oidc/login/success.spec.ts | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/test/e2e/cypress/integration/profiles/oidc/login/success.spec.ts b/test/e2e/cypress/integration/profiles/oidc/login/success.spec.ts index 82f98756eab7..866f4344eda6 100644 --- a/test/e2e/cypress/integration/profiles/oidc/login/success.spec.ts +++ b/test/e2e/cypress/integration/profiles/oidc/login/success.spec.ts @@ -6,13 +6,13 @@ import { routes as react } from "../../../../helpers/react" context("Social Sign In Successes", () => { ;[ - // { - // login: react.login, - // registration: react.registration, - // settings: react.settings, - // app: "react" as "react", - // profile: "spa", - // }, + { + login: react.login, + registration: react.registration, + settings: react.settings, + app: "react" as "react", + profile: "spa", + }, { login: express.login, registration: express.registration, From 771bb9567806d6da51476cf798c3e6e5ee29745f Mon Sep 17 00:00:00 2001 From: Henning Perl Date: Tue, 7 Nov 2023 13:19:41 +0100 Subject: [PATCH 27/29] fix: add dup credentials error info --- selfservice/strategy/oidc/strategy.go | 11 ++++++++++- selfservice/strategy/oidc/strategy_test.go | 7 +++++-- 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/selfservice/strategy/oidc/strategy.go b/selfservice/strategy/oidc/strategy.go index af75587f2e10..45d03a400da5 100644 --- a/selfservice/strategy/oidc/strategy.go +++ b/selfservice/strategy/oidc/strategy.go @@ -547,7 +547,16 @@ func (s *Strategy) handleError(w http.ResponseWriter, r *http.Request, f flow.Fl // This is kinda hacky and will probably need to be updated at some point. if dup := new(identity.ErrDuplicateCredentials); errors.As(err, &dup) { - rf.UI.Messages.Add(text.NewErrorValidationDuplicateCredentialsOnOIDCLink()) + err = schema.NewDuplicateCredentialsError(dup) + + if validationErr := new(schema.ValidationError); errors.As(err, &validationErr) { + for _, m := range validationErr.Messages { + rf.UI.Messages.Add(&m) + } + } else { + rf.UI.Messages.Add(text.NewErrorValidationDuplicateCredentialsOnOIDCLink()) + } + lf, err := s.registrationToLogin(w, r, rf, provider) if err != nil { return err diff --git a/selfservice/strategy/oidc/strategy_test.go b/selfservice/strategy/oidc/strategy_test.go index a278cd6dcb53..cdbff20e0477 100644 --- a/selfservice/strategy/oidc/strategy_test.go +++ b/selfservice/strategy/oidc/strategy_test.go @@ -1075,6 +1075,7 @@ func TestStrategy(t *testing.T) { }) t.Run("case=registration should start new login flow if duplicate credentials detected", func(t *testing.T) { + require.NoError(t, reg.Config().Set(ctx, config.ViperKeySelfServiceRegistrationLoginHints, true)) loginWithOIDC := func(t *testing.T, c *http.Client, flowID uuid.UUID, provider string) (*http.Response, []byte) { action := assertFormValues(t, flowID, provider) res, err := c.PostForm(action, url.Values{"provider": {provider}}) @@ -1138,7 +1139,9 @@ func TestStrategy(t *testing.T) { t.Run("step=should fail login and start a new flow", func(t *testing.T) { res, body := loginWithOIDC(t, client, loginFlow.ID, "valid") assert.True(t, res.Request.URL.Query().Has("no_org_ui")) - assertUIError(t, res, body, "An account with the same identifier (email, phone, username, ...) exists already. Please sign in to your existing account to link your social profile.") + assertUIError(t, res, body, "You tried signing in with new-login-if-email-exist-with-password-strategy@ory.sh which is already in use by another account. You can sign in using your password.") + assert.Equal(t, "password", gjson.GetBytes(body, "ui.messages.#(id==4000028).context.available_credential_types.0").String()) + assert.Equal(t, "new-login-if-email-exist-with-password-strategy@ory.sh", gjson.GetBytes(body, "ui.messages.#(id==4000028).context.credential_identifier_hint").String()) linkingLoginFlow.ID = gjson.GetBytes(body, "id").String() linkingLoginFlow.UIAction = gjson.GetBytes(body, "ui.action").String() linkingLoginFlow.CSRFToken = gjson.GetBytes(body, `ui.nodes.#(attributes.name=="csrf_token").attributes.value`).String() @@ -1204,7 +1207,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, "An account with the same identifier (email, phone, username, ...) exists already. Please sign in to your existing account to link your social profile.") + 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") }) From 9f329431ee3453b4e7955bc8914385abf5178537 Mon Sep 17 00:00:00 2001 From: Henning Perl Date: Tue, 7 Nov 2023 16:27:37 +0100 Subject: [PATCH 28/29] fix: tests --- selfservice/strategy/oidc/strategy.go | 1 + .../integration/profiles/oidc/registration/success.spec.ts | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/selfservice/strategy/oidc/strategy.go b/selfservice/strategy/oidc/strategy.go index 45d03a400da5..2e536c8a35cf 100644 --- a/selfservice/strategy/oidc/strategy.go +++ b/selfservice/strategy/oidc/strategy.go @@ -551,6 +551,7 @@ func (s *Strategy) handleError(w http.ResponseWriter, r *http.Request, f flow.Fl if validationErr := new(schema.ValidationError); errors.As(err, &validationErr) { for _, m := range validationErr.Messages { + m := m rf.UI.Messages.Add(&m) } } else { diff --git a/test/e2e/cypress/integration/profiles/oidc/registration/success.spec.ts b/test/e2e/cypress/integration/profiles/oidc/registration/success.spec.ts index ca2e2700ef7a..fe32cbbf61a2 100644 --- a/test/e2e/cypress/integration/profiles/oidc/registration/success.spec.ts +++ b/test/e2e/cypress/integration/profiles/oidc/registration/success.spec.ts @@ -244,7 +244,7 @@ context("Social Sign Up Successes", () => { route: registration, }) - cy.get('[data-testid="ui/message/4000027"]').should("be.visible") + cy.get('[data-testid="ui/message/1010016"]').should("be.visible") cy.location("href").should("contain", "/login") From 51ed46657670beaa212598287bebfda86cf42a76 Mon Sep 17 00:00:00 2001 From: Henning Perl Date: Tue, 7 Nov 2023 18:21:42 +0100 Subject: [PATCH 29/29] fix: tests --- .../integration/profiles/oidc/settings/success.spec.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/e2e/cypress/integration/profiles/oidc/settings/success.spec.ts b/test/e2e/cypress/integration/profiles/oidc/settings/success.spec.ts index 69f8e0f7d737..dadbc06d9453 100644 --- a/test/e2e/cypress/integration/profiles/oidc/settings/success.spec.ts +++ b/test/e2e/cypress/integration/profiles/oidc/settings/success.spec.ts @@ -42,9 +42,9 @@ context("Social Sign In Settings Success", () => { cy.get('input[name="traits.website"]').clear().type(website) cy.triggerOidc(app, "hydra") - cy.get('[data-testid="ui/message/4000027"]').should( + cy.get('[data-testid="ui/message/1010016"]').should( "contain.text", - "An account with the same identifier", + "Signing in will link your account", ) cy.noSession()