From 7ca3b6be14c53e16c3a8f4e7eb83efe0b0e7c88e Mon Sep 17 00:00:00 2001 From: Jonas Hungershausen Date: Fri, 31 Jan 2025 09:00:29 +0100 Subject: [PATCH] fix: accept login_challenge in SPA verification flows (#4284) --- selfservice/flow/verification/handler.go | 34 ++++++++++-- selfservice/flow/verification/handler_test.go | 54 +++++++++++++++++-- 2 files changed, 79 insertions(+), 9 deletions(-) diff --git a/selfservice/flow/verification/handler.go b/selfservice/flow/verification/handler.go index 785f627e7f6a..9f235ff0c54a 100644 --- a/selfservice/flow/verification/handler.go +++ b/selfservice/flow/verification/handler.go @@ -455,7 +455,9 @@ func (h *Handler) updateVerificationFlow(w http.ResponseWriter, r *http.Request, return } - if x.IsBrowserRequest(r) { + // API flows can receive requests from the browser, if the link strategy is used. + // However, x.IsBrowserRequest only checks for form submissions, not JSON requests made from a browser context + if x.IsBrowserRequest(r) || (f.Type == flow.TypeBrowser && x.IsJSONRequest(r)) { // Special case: If we ended up here through a OAuth2 login challenge, we need to accept the login request // and redirect back to the OAuth2 provider. if flow.HasReachedState(flow.StatePassedChallenge, f.State) && f.OAuth2LoginChallenge.String() != "" { @@ -489,12 +491,34 @@ func (h *Handler) updateVerificationFlow(w http.ResponseWriter, r *http.Request, return } - http.Redirect(w, r, callbackURL, http.StatusSeeOther) + if x.IsJSONRequest(r) { + // This intentionally works differently than the "form browser" flow, + // as it _does_ show the `verification success` UI, but the "Continue" + // button contains the link to the OAuth2 provider with the `login_verifier`. + continueNode := f.UI.Nodes.Find("continue") + if continueNode != nil { + if attr, ok := continueNode.Attributes.(*node.AnchorAttributes); ok { + attr.HREF = callbackURL + if err := h.d.VerificationFlowPersister().UpdateVerificationFlow(ctx, f); err != nil { + h.d.VerificationFlowErrorHandler().WriteFlowError(w, r, f, node.DefaultGroup, err) + return + } + + h.d.Writer().Write(w, r, f) + return + } + } + + // The flow does not have the `continue` node, which is an unknown state. + // This should never happen. + } else { + http.Redirect(w, r, callbackURL, http.StatusSeeOther) + return + } + } else if x.IsBrowserRequest(r) { + http.Redirect(w, r, f.AppendTo(h.d.Config().SelfServiceFlowVerificationUI(ctx)).String(), http.StatusSeeOther) return } - - http.Redirect(w, r, f.AppendTo(h.d.Config().SelfServiceFlowVerificationUI(ctx)).String(), http.StatusSeeOther) - return } updatedFlow, err := h.d.VerificationFlowPersister().GetVerificationFlow(ctx, f.ID) diff --git a/selfservice/flow/verification/handler_test.go b/selfservice/flow/verification/handler_test.go index 6f37956b6055..519568d82b65 100644 --- a/selfservice/flow/verification/handler_test.go +++ b/selfservice/flow/verification/handler_test.go @@ -9,6 +9,7 @@ import ( "io" "net/http" "net/url" + "strings" "testing" "time" @@ -24,6 +25,9 @@ import ( "github.com/ory/kratos/internal/testhelpers" "github.com/ory/kratos/selfservice/flow" "github.com/ory/kratos/selfservice/flow/verification" + "github.com/ory/kratos/text" + "github.com/ory/kratos/ui/container" + "github.com/ory/kratos/ui/node" "github.com/ory/kratos/x" ) @@ -196,7 +200,7 @@ func TestPostFlow(t *testing.T) { _ = testhelpers.NewErrorTestServer(t, reg) _ = testhelpers.NewRedirTS(t, "", conf) - t.Run("case=valid", func(t *testing.T) { + t.Run("client=browser/case=valid", func(t *testing.T) { f := &verification.Flow{ ID: uuid.Must(uuid.NewV4()), Type: "browser", @@ -206,16 +210,40 @@ func TestPostFlow(t *testing.T) { } require.NoError(t, reg.VerificationFlowPersister().CreateVerificationFlow(ctx, f)) - client := testhelpers.NewClientWithCookies(t) + client := testhelpers.NewNoRedirectClientWithCookies(t) u := public.URL + verification.RouteSubmitFlow + "?flow=" + f.ID.String() resp, err := client.PostForm(u, url.Values{"method": {"fake"}}) require.NoError(t, err) + assert.EqualValues(t, http.StatusSeeOther, resp.StatusCode) + assert.Equal(t, conf.SelfServiceFlowVerificationUI(ctx).String()+"?flow="+f.ID.String(), resp.Header.Get("Location")) + }) + + t.Run("client=spa/case=valid", func(t *testing.T) { + f := &verification.Flow{ + ID: uuid.Must(uuid.NewV4()), + Type: "browser", + ExpiresAt: time.Now().Add(1 * time.Hour), + IssuedAt: time.Now(), + State: flow.StateChooseMethod, + } + require.NoError(t, reg.VerificationFlowPersister().CreateVerificationFlow(ctx, f)) + + client := testhelpers.NewClientWithCookies(t) + + u := public.URL + verification.RouteSubmitFlow + "?flow=" + f.ID.String() + req, err := http.NewRequest("POST", u, strings.NewReader(`{"method": "fake"}`)) + require.NoError(t, err) + req.Header.Set("Content-Type", "application/json") + req.Header.Set("Accept", "application/json") + resp, err := client.Do(req) + require.NoError(t, err) assert.EqualValues(t, http.StatusOK, resp.StatusCode) }) t.Run("suite=with OIDC login challenge", func(t *testing.T) { - t.Run("case=succeeds with a session", func(t *testing.T) { + createFlow := func(t *testing.T) *verification.Flow { + t.Helper() s := testhelpers.CreateSession(t, reg) f := &verification.Flow{ @@ -229,10 +257,17 @@ func TestPostFlow(t *testing.T) { IdentityID: uuid.NullUUID{UUID: s.IdentityID, Valid: true}, AMR: s.AMR, }, + UI: &container.Container{ + Action: "http://action", + Nodes: []*node.Node{node.NewAnchorField("continue", "https://ory.sh", node.CodeGroup, text.NewInfoNodeLabelContinue())}, + }, State: flow.StatePassedChallenge, } require.NoError(t, reg.VerificationFlowPersister().CreateVerificationFlow(ctx, f)) - + return f + } + t.Run("client=browser/case=succeeds with a session", func(t *testing.T) { + f := createFlow(t) client := testhelpers.NewNoRedirectClientWithCookies(t) u := public.URL + verification.RouteSubmitFlow + "?flow=" + f.ID.String() @@ -241,6 +276,17 @@ func TestPostFlow(t *testing.T) { assert.Equal(t, http.StatusSeeOther, resp.StatusCode) assert.Equal(t, hydra.FakePostLoginURL, resp.Header.Get("Location")) }) + t.Run("client=spa/case=succeeds with a session", func(t *testing.T) { + f := createFlow(t) + client := testhelpers.NewNoRedirectClientWithCookies(t) + + u := public.URL + verification.RouteSubmitFlow + "?flow=" + f.ID.String() + resp, err := client.Post(u, "application/json", strings.NewReader(`{"method": "fake"}`)) + require.NoError(t, err) + assert.Equal(t, http.StatusOK, resp.StatusCode) + body := x.MustReadAll(resp.Body) + assert.Equal(t, hydra.FakePostLoginURL, gjson.GetBytes(body, "ui.nodes.#(attributes.id==continue).attributes.href").String(), "%s", body) + }) t.Run("case=fails without a session", func(t *testing.T) { client := testhelpers.NewClientWithCookies(t)