fix: do not send session after registration without hook (ory#2094)

See ory#2093

Co-authored-by: aeneasr <[email protected]>

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
+}

selfservice/flow/registration/hook.go

@@ -184,7 +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) {
-		e.d.Writer().Write(w, r, &APIFlowResponse{Identity: i, Session: s})
+		e.d.Writer().Write(w, r, &APIFlowResponse{Identity: i})
 		return nil
 	}
 

selfservice/hook/session_issuer.go

@@ -42,11 +42,26 @@ func (e *SessionIssuer) ExecutePostRegistrationPostPersistHook(w http.ResponseWr
 
 	if a.Type == flow.TypeAPI {
 		e.r.Writer().Write(w, r, &registration.APIFlowResponse{
-			Session: s, Token: s.Token,
+			Session:  s,
+			Token:    s.Token,
 			Identity: s.Identity,
 		})
 		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, &registration.APIFlowResponse{
+			Session:  s,
+			Identity: s.Identity,
+		})
+		return errors.WithStack(registration.ErrHookAbortFlow)
+	}
+
+	return nil
 }

selfservice/hook/session_issuer_test.go

@@ -57,7 +57,7 @@ func TestSessionIssuer(t *testing.T) {
 			f := &registration.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 := &registration.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())
+		})
 	})
 }

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"}})