fix: settings should persist return_to after required mfa login flow (

#3263)

* fix: get settings should persist `return_to` when redirecting to aal2

* feat(e2e): verify `return_to` persists in recovery flows

* test: recovery strategy with mfa account

* test: code recovery return to persists to settings with aal2

* u

* fix: return to settings flow after mfa login

* fix(test): login handler

* fix: flow between settings and mfa

* fix: get settings endpoint should redirect to settings ui instead of to itself

* feat(test): preserve URL from various settings flows through login mfa flow

* chore: cleanup

* fix(e2e): recovery return to spa tests

* fix: e2e proxy

* fix: do not always redirect back to settings on mfa

* fix: new settings flow with required mfa shouldn't be added to login flow return_to unless it contains a return_to parameter

* fix(e2e): let test dynamically handle required_aal

* chore: cleanup unused code

* test: `DoesSessionSatisfy` with method options

* test: recovery strategy with aal2

selfservice/flow/login/handler.go

@@ -452,7 +452,6 @@ func (h *Handler) createBrowserLoginFlow(w http.ResponseWriter, r *http.Request,
  }
 
  rt, err := h.d.Hydra().AcceptLoginRequest(r.Context(), string(hydraLoginChallenge), sess.IdentityID.String(), sess.AMR)
-
  if err != nil {
  h.d.SelfServiceErrorManager().Forward(r.Context(), w, r, err)
  return

selfservice/flow/login/handler_test.go

@@ -14,6 +14,7 @@ import (
  "testing"
  "time"
 
+ "github.com/julienschmidt/httprouter"
  "github.com/pkg/errors"
 
  "github.com/ory/x/urlx"
@@ -22,6 +23,11 @@ import (
 
  "github.com/ory/kratos/hydra"
  "github.com/ory/kratos/selfservice/flow"
+ "github.com/ory/kratos/selfservice/strategy/totp"
+ "github.com/ory/kratos/session"
+
+ stdtotp "github.com/pquerna/otp/totp"
+
  "github.com/ory/kratos/ui/container"
 
  "github.com/ory/kratos/text"
@@ -42,6 +48,7 @@ import (
  "github.com/ory/kratos/internal"
  "github.com/ory/kratos/internal/testhelpers"
  "github.com/ory/kratos/selfservice/flow/login"
+ "github.com/ory/kratos/selfservice/flow/settings"
  "github.com/ory/kratos/x"
 )
 
@@ -405,6 +412,122 @@ func TestFlowLifecycle(t *testing.T) {
  assertx.EqualAsJSONExcept(t, flow.NewFlowExpiredError(expired), json.RawMessage(actual), []string{"use_flow_id", "since"}, "expired", "%s", actual)
  })
  })
+
+ t.Run("case=should return to settings flow after successful mfa login after recovery", func(t *testing.T) {
+ conf.MustSet(ctx, config.ViperKeySelfServiceSettingsRequiredAAL, config.HighestAvailableAAL)
+ conf.MustSet(ctx, config.ViperKeySessionWhoAmIAAL, config.HighestAvailableAAL)
+ testhelpers.StrategyEnable(t, conf, identity.CredentialsTypeTOTP.String(), true)
+ conf.MustSet(ctx, config.ViperKeyURLsAllowedReturnToDomains, []string{"https://www.ory.sh/"})
+
+ t.Cleanup(func() {
+ conf.MustSet(ctx, config.ViperKeySelfServiceSettingsRequiredAAL, string(identity.AuthenticatorAssuranceLevel1))
+ conf.MustSet(ctx, config.ViperKeySessionWhoAmIAAL, string(identity.AuthenticatorAssuranceLevel1))
+ testhelpers.StrategyEnable(t, conf, identity.CredentialsTypeTOTP.String(), false)
+ })
+
+ key, err := totp.NewKey(context.Background(), "foo", reg)
+ require.NoError(t, err)
+ email := testhelpers.RandomEmail()
+ var id = &identity.Identity{
+ Credentials: map[identity.CredentialsType]identity.Credentials{
+ "password": {
+ Type: "password",
+ Identifiers: []string{email},
+ Config: sqlxx.JSONRawMessage(`{"hashed_password":"foo"}`),
+ },
+ },
+ Traits: identity.Traits(fmt.Sprintf(`{"email":"%s"}`, email)),
+ SchemaID: config.DefaultIdentityTraitsSchemaID,
+ }
+
+ require.NoError(t, reg.PrivilegedIdentityPool().CreateIdentities(context.Background(), id))
+
+ id.SetCredentials(identity.CredentialsTypeTOTP, identity.Credentials{
+ Type: identity.CredentialsTypeTOTP,
+ Identifiers: []string{id.ID.String()},
+ Config: sqlxx.JSONRawMessage(`{"totp_url":"` + string(key.URL()) + `"}`),
+ })
+ require.NoError(t, reg.PrivilegedIdentityPool().UpdateIdentity(context.Background(), id))
+
+ h := func(w http.ResponseWriter, r *http.Request, _ httprouter.Params) {
+ sess, err := session.NewActiveSession(r, id, reg.Config(), time.Now().UTC(), identity.CredentialsTypePassword, identity.AuthenticatorAssuranceLevel1)
+ require.NoError(t, err)
+ sess.AuthenticatorAssuranceLevel = identity.AuthenticatorAssuranceLevel1
+ require.NoError(t, reg.SessionPersister().UpsertSession(context.Background(), sess))
+ require.NoError(t, reg.SessionManager().IssueCookie(context.Background(), w, r, sess))
+ require.Equal(t, identity.AuthenticatorAssuranceLevel1, sess.AuthenticatorAssuranceLevel)
+
+ }
+
+ router.GET("/mock-session", h)
+
+ client := testhelpers.NewClientWithCookies(t)
+
+ testhelpers.MockHydrateCookieClient(t, client, ts.URL+"/mock-session")
+
+ settingsURL := ts.URL + settings.RouteInitBrowserFlow + "?return_to=https://www.ory.sh"
+ req, err := http.NewRequest("GET", settingsURL, nil)
+ require.NoError(t, err)
+
+ // we initialize the settings flow with a session that has AAL1 set
+ resp, err := client.Do(req)
+ require.NoError(t, err)
+ require.Equal(t, http.StatusOK, resp.StatusCode)
+ // we expect the request to redirect to the login flow because the AAL1 session is not sufficient
+ requestURL, err := url.Parse(resp.Request.Referer())
+ require.NoError(t, err)
+ require.Equal(t, login.RouteInitBrowserFlow, requestURL.Path)
+ require.Equal(t, "aal2", requestURL.Query().Get("aal"))
+ require.Equal(t, settingsURL, requestURL.Query().Get("return_to"))
+
+ // we expect to be on the login page now
+ respURL := resp.Request.URL
+ require.NoError(t, err)
+ require.Equal(t, "/login-ts", respURL.Path)
+ flowID := respURL.Query().Get("flow")
+ require.NotEmpty(t, flowID)
+
+ code, err := stdtotp.GenerateCode(key.Secret(), time.Now())
+ require.NoError(t, err)
+
+ req, err = http.NewRequest("GET", ts.URL+login.RouteGetFlow+"?id="+flowID, nil)
+ require.NoError(t, err)
+
+ req.Header.Add("Content-Type", "application/json")
+
+ resp, err = client.Do(req)
+ require.NoError(t, err)
+ require.Equal(t, http.StatusOK, resp.StatusCode)
+ body := string(x.MustReadAll(resp.Body))
+ defer resp.Body.Close()
+
+ totpNode := gjson.Get(body, "ui.nodes.#(attributes.name==totp_code)").String()
+ require.NotEmpty(t, totpNode)
+ require.NotEmpty(t, gjson.Get(body, "ui.action").String())
+
+ csrfToken := gjson.Get(body, "ui.nodes.#(attributes.name==csrf_token).attributes.value").String()
+
+ req, err = http.NewRequest("POST", ts.URL+login.RouteSubmitFlow+"?flow="+flowID, strings.NewReader(url.Values{
+ "method": {"totp"},
+ "totp_code": {code},
+ "csrf_token": {csrfToken},
+ }.Encode()))
+
+ require.NoError(t, err)
+ req.Header.Add("Content-Type", "application/x-www-form-urlencoded")
+
+ client.CheckRedirect = func(req *http.Request, via []*http.Request) error {
+ return http.ErrUseLastResponse
+ }
+
+ resp, err = client.Do(req)
+ require.NoError(t, err)
+ require.Equal(t, http.StatusSeeOther, resp.StatusCode)
+
+ location, err := resp.Location()
+ require.NoError(t, err)
+ require.Equal(t, settings.RouteInitBrowserFlow, location.Path)
+ })
  })
 
  t.Run("lifecycle=init", func(t *testing.T) {

selfservice/flow/login/hook.go

@@ -132,15 +132,17 @@ func (e *HookExecutor) PostLoginHook(
  return err
  }
 
- // Verify the redirect URL before we do any other processing.
  c := e.d.Config()
- returnTo, err := x.SecureRedirectTo(r, c.SelfServiceBrowserDefaultReturnTo(r.Context()),
+ // Verify the redirect URL before we do any other processing.
+ returnTo, err := x.SecureRedirectTo(r,
+ c.SelfServiceBrowserDefaultReturnTo(r.Context()),
  x.SecureRedirectReturnTo(a.ReturnTo),
  x.SecureRedirectUseSourceURL(a.RequestURL),
  x.SecureRedirectAllowURLs(c.SelfServiceBrowserAllowedReturnToDomains(r.Context())),
  x.SecureRedirectAllowSelfServiceURLs(c.SelfPublicURL(r.Context())),
- x.SecureRedirectOverrideDefaultReturnTo(e.d.Config().SelfServiceFlowLoginReturnTo(r.Context(), a.Active.String())),
+ x.SecureRedirectOverrideDefaultReturnTo(c.SelfServiceFlowLoginReturnTo(r.Context(), a.Active.String())),
  )
+
  if err != nil {
  return err
  }

selfservice/flow/settings/error.go

@@ -32,9 +32,7 @@ import (
  "github.com/ory/kratos/x"
 )
 
-var (
- ErrHookAbortFlow = errors.New("aborted settings hook execution")
-)
+var ErrHookAbortFlow = errors.New("aborted settings hook execution")
 
 type (
  errorHandlerDependencies interface {
@@ -85,7 +83,8 @@ func (e *FlowNeedsReAuth) EnhanceJSONError() interface{} {
 func NewFlowNeedsReAuth() *FlowNeedsReAuth {
  return &FlowNeedsReAuth{
  DefaultError: herodot.ErrForbidden.WithID(text.ErrIDNeedsPrivilegedSession).
- WithReasonf("The login session is too old and thus not allowed to update these fields. Please re-authenticate.")}
+ WithReasonf("The login session is too old and thus not allowed to update these fields. Please re-authenticate."),
+ }
 }
 
 func NewErrorHandler(d errorHandlerDependencies) *ErrorHandler {
@@ -99,9 +98,12 @@ func (s *ErrorHandler) reauthenticate(
  err *FlowNeedsReAuth,
 ) {
  returnTo := urlx.CopyWithQuery(urlx.AppendPaths(s.d.Config().SelfPublicURL(r.Context()), r.URL.Path), r.URL.Query())
- redirectTo := urlx.AppendPaths(urlx.CopyWithQuery(s.d.Config().SelfPublicURL(r.Context()),
- url.Values{"refresh": {"true"}, "return_to": {returnTo.String()}}),
- login.RouteInitBrowserFlow).String()
+
+ params := url.Values{}
+ params.Set("refresh", "true")
+ params.Set("return_to", returnTo.String())
+
+ redirectTo := urlx.AppendPaths(urlx.CopyWithQuery(s.d.Config().SelfPublicURL(r.Context()), params), login.RouteInitBrowserFlow).String()
  err.RedirectBrowserTo = redirectTo
  if f.Type == flow.TypeAPI || x.IsJSONRequest(r) {
  s.d.Writer().WriteError(w, r, err)
@@ -163,9 +165,7 @@ func (s *ErrorHandler) WriteFlowError(
  if shouldRespondWithJSON {
  s.d.Writer().WriteError(w, r, aalErr)
  } else {
- http.Redirect(w, r, urlx.CopyWithQuery(
- urlx.AppendPaths(s.d.Config().SelfPublicURL(r.Context()), login.RouteInitBrowserFlow),
- url.Values{"aal": {string(identity.AuthenticatorAssuranceLevel2)}}).String(), http.StatusSeeOther)
+ http.Redirect(w, r, aalErr.RedirectTo, http.StatusSeeOther)
  }
  return
  }

selfservice/flow/settings/error_test.go

@@ -53,7 +53,7 @@ func TestHandleError(t *testing.T) {
  t.Cleanup(ts.Close)
 
  _ = testhelpers.NewSettingsUIFlowEchoServer(t, reg)
- errorTS := testhelpers.NewErrorTestServer(t, reg)
+ _ = testhelpers.NewErrorTestServer(t, reg)
  loginTS := testhelpers.NewLoginUIFlowEchoServer(t, reg)
 
  h := reg.SettingsFlowErrorHandler()
@@ -72,6 +72,10 @@ func TestHandleError(t *testing.T) {
  h.WriteFlowError(w, r, flowMethod, settingsFlow, &id, flowError)
  })
 
+ router.GET("/fake-redirect", func(w http.ResponseWriter, r *http.Request, p httprouter.Params) {
+ reg.LoginHandler().NewLoginFlow(w, r, flow.TypeBrowser)
+ })
+
  reset := func() {
  settingsFlow = nil
  flowError = nil
@@ -318,17 +322,42 @@ func TestHandleError(t *testing.T) {
 
  settingsFlow = newFlow(t, time.Minute, flow.TypeBrowser)
  settingsFlow.IdentityID = id.ID
- flowError = errors.WithStack(session.NewErrAALNotSatisfied(""))
+ flowError = errors.WithStack(session.NewErrAALNotSatisfied(conf.SelfServiceFlowLoginUI(ctx).String()))
  flowMethod = settings.StrategyProfile
 
- res, err := ts.Client().Get(ts.URL + "/error")
+ client := &http.Client{}
+ *client = *ts.Client()
+
+ // disable redirects
+ client.CheckRedirect = func(req *http.Request, via []*http.Request) error {
+ return http.ErrUseLastResponse
+ }
+
+ res, err := client.Get(ts.URL + "/error")
+ require.NoError(t, err)
+
+ loc, err := res.Location()
  require.NoError(t, err)
- assert.Contains(t, res.Request.URL.String(), errorTS.URL)
+
+ // we should end up at the login URL since the NewALLNotSatisfied takes in a redirect to URL.
+ assert.Contains(t, loc.String(), loginTS.URL)
+
+ // test the JSON resoponse as well
+ request, err := http.NewRequest("GET", ts.URL+"/error", nil)
+ require.NoError(t, err)
+
+ request.Header.Add("Accept", "application/json")
+
+ res, err = client.Do(request)
+ require.NoError(t, err)
+
+ // the body should contain the reason for the error
  body := x.MustReadAll(res.Body)
  require.NoError(t, res.Body.Close())
 
+ require.NotEmpty(t, gjson.GetBytes(body, "error.reason").String(), "%s", body)
  // We end up at the error endpoint with an aal2 error message because ts.client has no session.
- assert.Equal(t, "You can not requested a higher AAL (AAL2/AAL3) without an active session.", gjson.GetBytes(body, "reason").String(), "%s", body)
+ assert.Equal(t, session.NewErrAALNotSatisfied("").Reason(), gjson.GetBytes(body, "error.reason").String(), "%s", body)
  })
 
  t.Run("case=session old error", func(t *testing.T) {

selfservice/flow/settings/handler.go

@@ -5,6 +5,7 @@ package settings
 
 import (
  "net/http"
+ "net/url"
  "time"
 
  "github.com/julienschmidt/httprouter"
@@ -50,8 +51,6 @@ type (
 
  config.Provider
 
- continuity.ManagementProvider
-
  session.HandlerProvider
  session.ManagementProvider
 
@@ -61,12 +60,17 @@ type (
 
  errorx.ManagementProvider
 
+ continuity.ManagementProvider
+
  ErrorHandlerProvider
  FlowPersistenceProvider
  StrategyProvider
  HookExecutorProvider
+ x.CSRFTokenGeneratorProvider
 
  schema.IdentityTraitsProvider
+
+ login.HandlerProvider
  }
  HandlerProvider interface {
  SettingsHandler() *Handler
@@ -298,7 +302,13 @@ func (h *Handler) createBrowserSettingsFlow(w http.ResponseWriter, r *http.Reque
  return
  }
 
- if err := h.d.SessionManager().DoesSessionSatisfy(r, s, h.d.Config().SelfServiceSettingsRequiredAAL(r.Context())); err != nil {
+ var managerOptions []session.ManagerOptions
+ requestURL := x.RequestURL(r)
+ if requestURL.Query().Get("return_to") != "" {
+ managerOptions = append(managerOptions, session.WithRequestURL(requestURL.String()))
+ }
+
+ if err := h.d.SessionManager().DoesSessionSatisfy(r, s, h.d.Config().SelfServiceSettingsRequiredAAL(r.Context()), managerOptions...); err != nil {
  h.d.SettingsFlowErrorHandler().WriteFlowError(w, r, node.DefaultGroup, nil, nil, err)
  return
  }
@@ -406,7 +416,11 @@ func (h *Handler) fetchFlow(w http.ResponseWriter, r *http.Request) error {
  return errors.WithStack(herodot.ErrForbidden.WithID(text.ErrIDInitiatedBySomeoneElse).WithReasonf("The request was made for another identity and has been blocked for security reasons."))
  }
 
- if err := h.d.SessionManager().DoesSessionSatisfy(r, sess, h.d.Config().SelfServiceSettingsRequiredAAL(r.Context())); err != nil {
+ // we cannot redirect back to the request URL (/self-service/settings/flows?id=...) since it would just redirect
+ // to a page displaying raw JSON to the client (browser), which is not what we want.
+ // Let's rather carry over the flow ID as a query parameter and redirect to the settings UI URL.
+ requestURL := urlx.CopyWithQuery(h.d.Config().SelfServiceFlowSettingsUI(r.Context()), url.Values{"flow": {rid.String()}})
+ if err := h.d.SessionManager().DoesSessionSatisfy(r, sess, h.d.Config().SelfServiceSettingsRequiredAAL(r.Context()), session.WithRequestURL(requestURL.String())); err != nil {
  return err
  }
 
@@ -564,7 +578,8 @@ func (h *Handler) updateSettingsFlow(w http.ResponseWriter, r *http.Request, ps
  return
  }
 
- if err := h.d.SessionManager().DoesSessionSatisfy(r, ss, h.d.Config().SelfServiceSettingsRequiredAAL(r.Context())); err != nil {
+ requestURL := x.RequestURL(r).String()
+ if err := h.d.SessionManager().DoesSessionSatisfy(r, ss, h.d.Config().SelfServiceSettingsRequiredAAL(r.Context()), session.WithRequestURL(requestURL)); err != nil {
  h.d.SettingsFlowErrorHandler().WriteFlowError(w, r, node.DefaultGroup, f, nil, err)
  return
  }