From 0d5d0e62aa2fa0cdb5641605190e8ba37a6c9f2b Mon Sep 17 00:00:00 2001 From: Fabian Meyer <3982806+meyfa@users.noreply.github.com> Date: Sat, 1 Jan 2022 03:12:51 +0100 Subject: [PATCH 1/4] fix: Do not send session after registration without hook See #2093 --- internal/testhelpers/errorx.go | 17 +++++ selfservice/flow/registration/hook.go | 10 ++- .../strategy/password/registration_test.go | 65 +++++++++++++++++-- 3 files changed, 84 insertions(+), 8 deletions(-) diff --git a/internal/testhelpers/errorx.go b/internal/testhelpers/errorx.go index 0c5d47b93c15..428368ac0c06 100644 --- a/internal/testhelpers/errorx.go +++ b/internal/testhelpers/errorx.go @@ -57,6 +57,7 @@ func NewRedirSessionEchoTS(t *testing.T, reg interface { config.Provider }) *httptest.Server { ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // verify that the client has a session, and echo it back sess, err := reg.SessionManager().FetchFromRequest(r.Context(), r) require.NoError(t, err, "Headers: %+v", r.Header) reg.Writer().Write(w, r, sess) @@ -65,3 +66,19 @@ func NewRedirSessionEchoTS(t *testing.T, reg interface { reg.Config(context.Background()).MustSet(config.ViperKeySelfServiceBrowserDefaultReturnTo, ts.URL+"/return-ts") return ts } + +func NewRedirNoSessionTS(t *testing.T, reg interface { + x.WriterProvider + session.ManagementProvider + config.Provider +}) *httptest.Server { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // verify that the client DOES NOT have a session + _, err := reg.SessionManager().FetchFromRequest(r.Context(), r) + require.Error(t, err, "Headers: %+v", r.Header) + reg.Writer().Write(w, r, nil) + })) + t.Cleanup(ts.Close) + reg.Config(context.Background()).MustSet(config.ViperKeySelfServiceBrowserDefaultReturnTo, ts.URL+"/return-ts") + return ts +} diff --git a/selfservice/flow/registration/hook.go b/selfservice/flow/registration/hook.go index 72ea65336907..06638b02c1ab 100644 --- a/selfservice/flow/registration/hook.go +++ b/selfservice/flow/registration/hook.go @@ -184,7 +184,15 @@ func (e *HookExecutor) PostRegistrationHook(w http.ResponseWriter, r *http.Reque Debug("Post registration execution hooks completed successfully.") if a.Type == flow.TypeAPI || x.IsJSONRequest(r) { - e.d.Writer().Write(w, r, &APIFlowResponse{Identity: i, Session: s}) + body := APIFlowResponse{Identity: i} + // only include the session if it is actually persisted + for _, afterHook := range c.SelfServiceFlowRegistrationAfterHooks(ct.String()) { + if afterHook.Name == "session" { + body.Session = s + break + } + } + e.d.Writer().Write(w, r, &body) return nil } diff --git a/selfservice/strategy/password/registration_test.go b/selfservice/strategy/password/registration_test.go index 4af4c074853d..f6b6392a2cb8 100644 --- a/selfservice/strategy/password/registration_test.go +++ b/selfservice/strategy/password/registration_test.go @@ -5,6 +5,7 @@ import ( "encoding/json" "fmt" "net/http" + "net/http/httptest" "net/url" "strings" "testing" @@ -72,10 +73,16 @@ func TestRegistration(t *testing.T) { errTS := testhelpers.NewErrorTestServer(t, reg) uiTS := testhelpers.NewRegistrationUIFlowEchoServer(t, reg) redirTS := testhelpers.NewRedirSessionEchoTS(t, reg) + redirNoSessionTS := testhelpers.NewRedirNoSessionTS(t, reg) - // Overwrite these two to ensure that they run - conf.MustSet(config.ViperKeySelfServiceBrowserDefaultReturnTo, redirTS.URL+"/default-return-to") - conf.MustSet(config.ViperKeySelfServiceRegistrationAfter+"."+config.DefaultBrowserReturnURL, redirTS.URL+"/registration-return-ts") + // set the "return to" server, which will assert the session state + // (redirTS: enforce that a session exists, redirNoSessionTS: enforce that no session exists) + var useReturnToFromTS = func(ts *httptest.Server) { + conf.MustSet(config.ViperKeySelfServiceBrowserDefaultReturnTo, ts.URL+"/default-return-to") + conf.MustSet(config.ViperKeySelfServiceRegistrationAfter+"."+config.DefaultBrowserReturnURL, ts.URL+"/registration-return-ts") + } + + useReturnToFromTS(redirTS) conf.MustSet(config.ViperKeyDefaultIdentitySchemaURL, "file://./stub/registration.schema.json") apiClient := testhelpers.NewDebugClient(t) @@ -470,7 +477,7 @@ func TestRegistration(t *testing.T) { }) }) - var expectSuccessfulLogin = func(t *testing.T, isAPI, isSPA bool, hc *http.Client, values func(url.Values)) string { + var expectLoginBody = func(t *testing.T, browserRedirTS *httptest.Server, isAPI, isSPA bool, hc *http.Client, values func(url.Values)) string { if isAPI { return testhelpers.SubmitRegistrationForm(t, isAPI, hc, publicTS, values, isSPA, http.StatusOK, @@ -481,7 +488,7 @@ func TestRegistration(t *testing.T) { hc = testhelpers.NewClientWithCookies(t) } - expectReturnTo := redirTS.URL + "/registration-return-ts" + expectReturnTo := browserRedirTS.URL + "/registration-return-ts" if isSPA { expectReturnTo = publicTS.URL } @@ -490,6 +497,19 @@ func TestRegistration(t *testing.T) { isSPA, http.StatusOK, expectReturnTo) } + var expectSuccessfulLogin = func(t *testing.T, isAPI, isSPA bool, hc *http.Client, values func(url.Values)) string { + useReturnToFromTS(redirTS) + return expectLoginBody(t, redirTS, isAPI, isSPA, hc, values) + } + + var expectNoLogin = func(t *testing.T, isAPI, isSPA bool, hc *http.Client, values func(url.Values)) string { + useReturnToFromTS(redirNoSessionTS) + t.Cleanup(func() { + useReturnToFromTS(redirTS) + }) + return expectLoginBody(t, redirNoSessionTS, isAPI, isSPA, hc, values) + } + t.Run("case=should pass and set up a session", func(t *testing.T) { conf.MustSet(config.ViperKeyDefaultIdentitySchemaURL, "file://./stub/registration.schema.json") conf.MustSet(config.HookStrategyKey(config.ViperKeySelfServiceRegistrationAfter, identity.CredentialsTypePassword.String()), []config.SelfServiceHook{{Name: "session"}}) @@ -509,8 +529,7 @@ func TestRegistration(t *testing.T) { }) t.Run("type=spa", func(t *testing.T) { - hc := testhelpers.NewClientWithCookies(t) - body := expectSuccessfulLogin(t, false, true, hc, func(v url.Values) { + body := expectSuccessfulLogin(t, false, true, nil, func(v url.Values) { v.Set("traits.username", "registration-identifier-8-spa") v.Set("password", x.NewUUID().String()) v.Set("traits.foobar", "bar") @@ -530,6 +549,38 @@ func TestRegistration(t *testing.T) { }) }) + t.Run("case=should not set up a session if hook is not configured", func(t *testing.T) { + conf.MustSet(config.ViperKeyDefaultIdentitySchemaURL, "file://./stub/registration.schema.json") + conf.MustSet(config.HookStrategyKey(config.ViperKeySelfServiceRegistrationAfter, identity.CredentialsTypePassword.String()), nil) + + t.Run("type=api", func(t *testing.T) { + body := expectNoLogin(t, true, false, nil, func(v url.Values) { + v.Set("traits.username", "registration-identifier-8-api-nosession") + v.Set("password", x.NewUUID().String()) + v.Set("traits.foobar", "bar") + }) + assert.Equal(t, `registration-identifier-8-api-nosession`, gjson.Get(body, "identity.traits.username").String(), "%s", body) + assert.Empty(t, gjson.Get(body, "session_token").String(), "%s", body) + assert.Empty(t, gjson.Get(body, "session.id").String(), "%s", body) + }) + + t.Run("type=spa", func(t *testing.T) { + expectNoLogin(t, false, true, nil, func(v url.Values) { + v.Set("traits.username", "registration-identifier-8-spa-nosession") + v.Set("password", x.NewUUID().String()) + v.Set("traits.foobar", "bar") + }) + }) + + t.Run("type=browser", func(t *testing.T) { + expectNoLogin(t, false, false, nil, func(v url.Values) { + v.Set("traits.username", "registration-identifier-8-browser-nosession") + v.Set("password", x.NewUUID().String()) + v.Set("traits.foobar", "bar") + }) + }) + }) + t.Run("case=should fail to register the same user again", func(t *testing.T) { conf.MustSet(config.ViperKeyDefaultIdentitySchemaURL, "file://./stub/registration.schema.json") conf.MustSet(config.HookStrategyKey(config.ViperKeySelfServiceRegistrationAfter, identity.CredentialsTypePassword.String()), []config.SelfServiceHook{{Name: "session"}}) From e771b9843b26c557f0ce2361f82f3402f8a54a8c Mon Sep 17 00:00:00 2001 From: aeneasr <3372410+aeneasr@users.noreply.github.com> Date: Sat, 1 Jan 2022 13:59:37 +0200 Subject: [PATCH 2/4] chore: code review --- selfservice/flow/registration/hook.go | 10 +--------- selfservice/hook/session_issuer.go | 3 ++- 2 files changed, 3 insertions(+), 10 deletions(-) diff --git a/selfservice/flow/registration/hook.go b/selfservice/flow/registration/hook.go index 06638b02c1ab..986e98aa12a4 100644 --- a/selfservice/flow/registration/hook.go +++ b/selfservice/flow/registration/hook.go @@ -184,15 +184,7 @@ func (e *HookExecutor) PostRegistrationHook(w http.ResponseWriter, r *http.Reque Debug("Post registration execution hooks completed successfully.") if a.Type == flow.TypeAPI || x.IsJSONRequest(r) { - body := APIFlowResponse{Identity: i} - // only include the session if it is actually persisted - for _, afterHook := range c.SelfServiceFlowRegistrationAfterHooks(ct.String()) { - if afterHook.Name == "session" { - body.Session = s - break - } - } - e.d.Writer().Write(w, r, &body) + e.d.Writer().Write(w, r, &APIFlowResponse{Identity: i}) return nil } diff --git a/selfservice/hook/session_issuer.go b/selfservice/hook/session_issuer.go index 42649ab9b6ff..b0bd513c06a2 100644 --- a/selfservice/hook/session_issuer.go +++ b/selfservice/hook/session_issuer.go @@ -42,7 +42,8 @@ func (e *SessionIssuer) ExecutePostRegistrationPostPersistHook(w http.ResponseWr if a.Type == flow.TypeAPI { e.r.Writer().Write(w, r, ®istration.APIFlowResponse{ - Session: s, Token: s.Token, + Session: s, + Token: s.Token, Identity: s.Identity, }) return errors.WithStack(registration.ErrHookAbortFlow) From 92b2ac5b64814dfa3d2e8ee8d67163f23b51e22d Mon Sep 17 00:00:00 2001 From: Fabian Meyer <3982806+meyfa@users.noreply.github.com> Date: Sat, 1 Jan 2022 13:45:20 +0100 Subject: [PATCH 3/4] fix: Let session issuer hook abort registration flow also for SPA --- selfservice/hook/session_issuer.go | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/selfservice/hook/session_issuer.go b/selfservice/hook/session_issuer.go index b0bd513c06a2..2c738a24be5b 100644 --- a/selfservice/hook/session_issuer.go +++ b/selfservice/hook/session_issuer.go @@ -49,5 +49,19 @@ func (e *SessionIssuer) ExecutePostRegistrationPostPersistHook(w http.ResponseWr return errors.WithStack(registration.ErrHookAbortFlow) } - return e.r.SessionManager().IssueCookie(r.Context(), w, r, s) + // cookie is issued both for browser and for SPA flows + if err := e.r.SessionManager().IssueCookie(r.Context(), w, r, s); err != nil { + return err + } + + // SPA flows additionally send the session + if x.IsJSONRequest(r) { + e.r.Writer().Write(w, r, ®istration.APIFlowResponse{ + Session: s, + Identity: s.Identity, + }) + return errors.WithStack(registration.ErrHookAbortFlow) + } + + return nil } From 5beecc7589400b61056b0b808178d965b9a9747b Mon Sep 17 00:00:00 2001 From: aeneasr <3372410+aeneasr@users.noreply.github.com> Date: Sat, 1 Jan 2022 22:59:14 +0200 Subject: [PATCH 4/4] chore: code review --- selfservice/hook/session_issuer_test.go | 25 ++++++++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/selfservice/hook/session_issuer_test.go b/selfservice/hook/session_issuer_test.go index 15bb2ca1b1f9..469cbae6cd26 100644 --- a/selfservice/hook/session_issuer_test.go +++ b/selfservice/hook/session_issuer_test.go @@ -57,7 +57,7 @@ func TestSessionIssuer(t *testing.T) { f := ®istration.Flow{Type: flow.TypeAPI} require.NoError(t, reg.PrivilegedIdentityPool().CreateIdentity(context.Background(), i)) - err := h.ExecutePostRegistrationPostPersistHook(w, &r, f, s) + err := h.ExecutePostRegistrationPostPersistHook(w, &http.Request{Header: http.Header{"Accept": {"application/json"}}}, f, s) require.True(t, errors.Is(err, registration.ErrHookAbortFlow), "%+v", err) got, err := reg.SessionPersister().GetSession(context.Background(), s.ID) @@ -71,5 +71,28 @@ func TestSessionIssuer(t *testing.T) { assert.Equal(t, s.ID.String(), gjson.GetBytes(body, "session.id").String()) assert.Equal(t, got.Token, gjson.GetBytes(body, "session_token").String()) }) + + t.Run("flow=spa", func(t *testing.T) { + w := httptest.NewRecorder() + + i := identity.NewIdentity(config.DefaultIdentityTraitsSchemaID) + s := &session.Session{ID: x.NewUUID(), Identity: i, Token: randx.MustString(12, randx.AlphaLowerNum), LogoutToken: randx.MustString(12, randx.AlphaLowerNum)} + f := ®istration.Flow{Type: flow.TypeBrowser} + + require.NoError(t, reg.PrivilegedIdentityPool().CreateIdentity(context.Background(), i)) + err := h.ExecutePostRegistrationPostPersistHook(w, &http.Request{Header: http.Header{"Accept": {"application/json"}}}, f, s) + require.True(t, errors.Is(err, registration.ErrHookAbortFlow), "%+v", err) + + got, err := reg.SessionPersister().GetSession(context.Background(), s.ID) + require.NoError(t, err) + assert.Equal(t, s.ID.String(), got.ID.String()) + assert.True(t, got.AuthenticatedAt.After(time.Now().Add(-time.Minute))) + + assert.NotEmpty(t, w.Header().Get("Set-Cookie")) + body := w.Body.Bytes() + assert.Equal(t, i.ID.String(), gjson.GetBytes(body, "identity.id").String()) + assert.Equal(t, s.ID.String(), gjson.GetBytes(body, "session.id").String()) + assert.Empty(t, gjson.GetBytes(body, "session_token").String()) + }) }) }