From 6a0a9149b9828ba994bec9b48a43f9d70245f43f Mon Sep 17 00:00:00 2001 From: Alano Terblanche <18033717+Benehiko@users.noreply.github.com> Date: Mon, 23 Oct 2023 17:24:57 +0200 Subject: [PATCH] fix: on verification required after registration, preserve return_to (#3589) * fix: on verification required after registration, preserve return_to * test: return_to on verification flow * chore: refactor Co-authored-by: Jonas Hungershausen --------- Co-authored-by: Jonas Hungershausen --- selfservice/flow/verification/flow.go | 6 +++- selfservice/flow/verification/flow_test.go | 2 +- selfservice/hook/show_verification_ui.go | 5 +-- selfservice/hook/verification.go | 34 ++++++++++++------- .../verification/registration/success.spec.ts | 21 ++++++++++++ 5 files changed, 51 insertions(+), 17 deletions(-) diff --git a/selfservice/flow/verification/flow.go b/selfservice/flow/verification/flow.go index 28a71e47a977..264e356da476 100644 --- a/selfservice/flow/verification/flow.go +++ b/selfservice/flow/verification/flow.go @@ -185,7 +185,11 @@ func NewPostHookFlow(conf *config.Config, exp time.Duration, csrf string, r *htt requestURL = new(url.URL) } query := requestURL.Query() - query.Set("return_to", query.Get("after_verification_return_to")) + // we need to keep the return_to in-tact if the `after_verification_return_to` is empty + // otherwise we take the `after_verification_return_to` query parameter over the current `return_to` + if afterVerificationReturn := query.Get("after_verification_return_to"); afterVerificationReturn != "" { + query.Set("return_to", afterVerificationReturn) + } query.Del("after_verification_return_to") requestURL.RawQuery = query.Encode() f.RequestURL = requestURL.String() diff --git a/selfservice/flow/verification/flow_test.go b/selfservice/flow/verification/flow_test.go index 3485f3454fc4..e05482ffa39d 100644 --- a/selfservice/flow/verification/flow_test.go +++ b/selfservice/flow/verification/flow_test.go @@ -108,7 +108,7 @@ func TestNewPostHookFlow(t *testing.T) { }) t.Run("case=return_to supplied", func(t *testing.T) { - expectReturnTo(t, url.Values{"return_to": {"http://foo.com/original_flow_callback"}}, "") + expectReturnTo(t, url.Values{"return_to": {"http://foo.com/original_flow_callback"}}, "http://foo.com/original_flow_callback") }) t.Run("case=return_to and after_verification_return_to supplied", func(t *testing.T) { diff --git a/selfservice/hook/show_verification_ui.go b/selfservice/hook/show_verification_ui.go index 1f7e83ce5c12..51f29a2aa6ce 100644 --- a/selfservice/hook/show_verification_ui.go +++ b/selfservice/hook/show_verification_ui.go @@ -59,9 +59,10 @@ func (e *ShowVerificationUIHook) execute(r *http.Request, f *registration.Flow) } } + ctx := r.Context() if vf != nil { - redir := e.d.Config().SelfServiceFlowVerificationUI(r.Context()) - f.ReturnToVerification = vf.AppendTo(redir).String() + redirURL := e.d.Config().SelfServiceFlowVerificationUI(ctx) + f.ReturnToVerification = vf.AppendTo(redirURL).String() } return nil diff --git a/selfservice/hook/verification.go b/selfservice/hook/verification.go index e041164f372d..c75cf1ce073b 100644 --- a/selfservice/hook/verification.go +++ b/selfservice/hook/verification.go @@ -70,29 +70,37 @@ func (e *Verifier) do( ) error { // This is called after the identity has been created so we can safely assume that all addresses are available // already. + ctx := r.Context() - strategy, err := e.r.GetActiveVerificationStrategy(r.Context()) + strategy, err := e.r.GetActiveVerificationStrategy(ctx) if err != nil { return err } + isBrowserFlow := f.GetType() == flow.TypeBrowser + isRegistrationFlow := f.GetFlowName() == flow.RegistrationFlow + for k := range i.VerifiableAddresses { address := &i.VerifiableAddresses[k] if address.Status != identity.VerifiableAddressStatusPending { continue } - csrf := "" + + var csrf string + // TODO: this is pretty ugly, we should probably have a better way to handle CSRF tokens here. - if f.GetType() != flow.TypeBrowser { - } else if _, ok := f.(*registration.Flow); ok { - // If this hook is executed from a registration flow, we need to regenerate the CSRF token. - csrf = e.r.CSRFHandler().RegenerateToken(w, r) - } else { - // If it came from a settings flow, there already is a CSRF token, so we can just use that. - csrf = e.r.GenerateCSRFToken(r) + if isBrowserFlow { + if isRegistrationFlow { + // If this hook is executed from a registration flow, we need to regenerate the CSRF token. + csrf = e.r.CSRFHandler().RegenerateToken(w, r) + } else { + // If it came from a settings flow, there already is a CSRF token, so we can just use that. + csrf = e.r.GenerateCSRFToken(r) + } } + verificationFlow, err := verification.NewPostHookFlow(e.r.Config(), - e.r.Config().SelfServiceFlowVerificationRequestLifespan(r.Context()), + e.r.Config().SelfServiceFlowVerificationRequestLifespan(ctx), csrf, r, strategy, f) if err != nil { return err @@ -108,17 +116,17 @@ func (e *Verifier) do( return err } - if err := e.r.VerificationFlowPersister().CreateVerificationFlow(r.Context(), verificationFlow); err != nil { + if err := e.r.VerificationFlowPersister().CreateVerificationFlow(ctx, verificationFlow); err != nil { return err } - if err := strategy.SendVerificationEmail(r.Context(), verificationFlow, i, address); err != nil { + if err := strategy.SendVerificationEmail(ctx, verificationFlow, i, address); err != nil { return err } flowURL := "" if verificationFlow.Type == flow.TypeBrowser { - flowURL = verificationFlow.AppendTo(e.r.Config().SelfServiceFlowVerificationUI(r.Context())).String() + flowURL = verificationFlow.AppendTo(e.r.Config().SelfServiceFlowVerificationUI(ctx)).String() } f.AddContinueWith(flow.NewContinueWithVerificationUI(verificationFlow, address.Value, flowURL)) diff --git a/test/e2e/cypress/integration/profiles/verification/registration/success.spec.ts b/test/e2e/cypress/integration/profiles/verification/registration/success.spec.ts index eac70c02e597..5065b5f2e30d 100644 --- a/test/e2e/cypress/integration/profiles/verification/registration/success.spec.ts +++ b/test/e2e/cypress/integration/profiles/verification/registration/success.spec.ts @@ -70,6 +70,7 @@ context("Account Verification Registration Success", () => { email, password, query: { + return_to: "http://localhost:4455/verification_return_to_callback", after_verification_return_to: "http://localhost:4455/verification_callback", }, @@ -83,6 +84,26 @@ context("Account Verification Registration Success", () => { }, }) }) + + it("is redirected to `return_to` after verification", () => { + cy.clearAllCookies() + const { email, password } = gen.identity() + cy.register({ + email, + password, + query: { + return_to: "http://localhost:4455/verification_return_to_callback", + }, + }) + cy.login({ email, password }) + cy.verifyEmail({ + expect: { + email, + password, + redirectTo: "http://localhost:4455/verification_return_to_callback", + }, + }) + }) }) }) })