feat: add return_url to verification flow #1149

mattbonnell · 2021-03-15T00:49:16Z

Related issue

Proposed changes

Adds "return_to" parameter support to the verification flow. If supplied, the user-agent will be redirected there after clicking the email link.
Additionally, add support for passing an "after_verification_return_to" query parameter to flows which have a verification post hook (eg registration), which the user-agent will be redirected to after the post hook verification flow is completed.
Eg: initialize a registration flow with /self-service/registration/browser?return_to=[url1]&after_verification_return_to=[url2]
After completing the registration, the user will be redirected to url1, and a verification email will be sent to them.
After clicking the link in the verification email, they'll be redirected to url2.

Checklist

I have read the contributing guidelines.
I have read the security policy.
I confirm that this pull request does not address a security
vulnerability. If this pull request addresses a security. vulnerability, I
confirm that I got green light (please contact
security@ory.sh) from the maintainers to push
the changes.
I have added tests that prove my fix is effective or that my feature
works.
I have added or changed the documentation.

Further comments

I need to add some tests here, but I'm not sure where the best places to add them would be. Any pointers there?
Additionally, will need to add some documentation for the after_verification parameter, but just wanted to get some feedback on that part before I do.

x/http_secure_redirect.go

aeneasr

Awesome, thank you for your contribution! This looks pretty good and I have some ideas how to improve it further :)

aeneasr · 2021-03-16T11:46:04Z

selfservice/strategy/link/strategy_verification.go

+	returnTo, err := x.SecureRedirectTo(r, nil,
+		x.SecureRedirectAllowSelfServiceURLs(s.d.Config(r.Context()).SelfPublicURL(r)),
+		x.SecureRedirectAllowURLs(s.d.Config(r.Context()).SelfServiceBrowserWhitelistedReturnToDomains()),
+		x.SecureRedirectUseReturnToKey("after_verification"),
+	)
+	if err != nil {
+		s.d.SelfServiceErrorManager().Forward(r.Context(), w, r, err)
+		return
+	}
+	if returnTo != nil {
+		http.Redirect(w, r, returnTo.String(), http.StatusFound)
+		return
+	}
+
+	returnTo, err = x.SecureRedirectTo(r, s.d.Config(r.Context()).SelfServiceFlowVerificationReturnTo(f.AppendTo(s.d.Config(r.Context()).SelfServiceFlowVerificationUI())),
+		x.SecureRedirectAllowSelfServiceURLs(s.d.Config(r.Context()).SelfPublicURL(r)),
+		x.SecureRedirectAllowURLs(s.d.Config(r.Context()).SelfServiceBrowserWhitelistedReturnToDomains()),
+	)
+	if err != nil {
+		s.d.SelfServiceErrorManager().Forward(r.Context(), w, r, err)
+		return
+	}
+
+	http.Redirect(w, r, returnTo.String(), http.StatusFound)


This looks like duplication and can probably be refactored. It also needs tests!

Yeah, will refactor.
Where would be the best place to add tests? As new cases in TestVerification in selfservice/strategy/link/strategy_verification_test.go?

Sorry, I missed this - do you still need guidance on this?

Hey, all good for this bit I think, thanks :)

selfservice/hook/verification.go

selfservice/flow/registration/flow.go

selfservice/strategy/link/strategy_verification.go

mattbonnell · 2021-03-18T15:37:09Z

hey @aeneasr, what's the best way to debug the e2e test? I'm failing a couple of the Cypress test cases but all I get from those logs is that the server returned a 500. When I try to run the end-to-end tests locally, for example the sqlite ones, the test script hangs on waiting for 1 resources: http-get://127.0.0.1:4457, which appears to be the kratos-selfservice-ui-react-native.

Do you have any insight you could share in how you go about debugging failures in the e2e tests? Perhaps there's a way to have the server logs included in the output when tests fail.

aeneasr · 2021-03-19T17:20:35Z

@mattbonnell sorry my week was really really hectic, I hope it tones down starting wed next week. I will try to address your PRs and questions latest then!

mattbonnell · 2021-03-19T17:21:40Z

@aeneasr hey, no worries. Thanks for letting me know :)

aeneasr

What I would suggest also is an e2e test to cover the full thing from start to finish. Basically copying https://github.com/ory/kratos/blob/master/test/e2e/cypress/integration/profiles/verification/registration/success.spec.js and setting the new parameter and at the end of the flow expect that we end up at what we specified during registration. You might need to add a new parameter to cy.register

selfservice/hook/verification.go

aeneasr · 2021-03-24T09:27:24Z

Looks like you got lots of work done :) Let me know when you want another 👀

mattbonnell · 2021-03-24T10:37:25Z

Looks like you got lots of work done :) Let me know when you want another 👀

Will do! Just going to add a few more tests this morning and then will ask you to take another look.

mattbonnell · 2021-03-24T14:52:00Z

Hmm, failing some courier unit test now, didn't change anything which courier touches though, I don't think.

mattbonnell · 2021-03-25T12:57:14Z

hey @aeneasr, what's the best way to debug the e2e test? I'm failing a couple of the Cypress test cases but all I get from those logs is that the server returned a 500. When I try to run the end-to-end tests locally, for example the sqlite ones, the test script hangs on waiting for 1 resources: http-get://127.0.0.1:4457, which appears to be the kratos-selfservice-ui-react-native.

Do you have any insight you could share in how you go about debugging failures in the e2e tests? Perhaps there's a way to have the server logs included in the output when tests fail.

hey just wanted to echo this question, still something I'm struggling with.

aeneasr · 2021-03-29T09:00:36Z

Hm not sure what's going on - do you have some logs for me? The logs are in the e2e directory

mattbonnell · 2021-03-29T12:43:43Z

Hm not sure what's going on - do you have some logs for me? The logs are in the e2e directory

Hey, ended up fixing that first issue, now I'm getting stuck here:

+ npm run test -- --record --config integrationFolder=test/e2e/cypress/integration/profiles/email

> test
> cypress run "--record" "--config" "integrationFolder=test/e2e/cypress/integration/profiles/email"

It looks like this is your first time using Cypress: 6.0.1

  ✔  Verified Cypress! /Users/mattbonnell/Library/Caches/Cypress/6.0.1/Cypress.app

Opening Cypress...
You passed the --record flag but did not provide us your Record Key.

You can pass us your Record Key like this:

  cypress run --record --key <record_key>

You can also set the key as an environment variable with the name CYPRESS_RECORD_KEY.

https://on.cypress.io/how-do-i-record-runs
npm ERR! code 1
npm ERR! path /Users/mattbonnell/Documents/dev/go/github.com/ory/kratos
npm ERR! command failed
npm ERR! command sh -c cypress run "--record" "--config" "integrationFolder=test/e2e/cypress/integration/profiles/email"

npm ERR! A complete log of this run can be found in:
npm ERR!     /Users/mattbonnell/.npm/_logs/2021-03-29T12_41_19_702Z-debug.log
make: *** [test-e2e] Error 1

I tried following their steps to obtain the record key for the Kratos project but it seems my access is limited. also weirdly, it seems in CI this lack of record key doesn't cause failure.

Going to remove the --record flag and try again
Ok, I'm able to run them now. Continuing debugging

aeneasr · 2021-03-29T19:52:52Z

I believe e2e tests fail on master currently as well...

aeneasr · 2021-03-31T08:51:56Z

I will take a look at the e2e now

aeneasr · 2021-03-31T10:46:41Z

I have resolved the e2e tests so this should work again - rerunning tests now.

mattbonnell · 2021-03-31T12:06:34Z

I have resolved the e2e tests so this should work again - rerunning tests now.

Seems there's some issue with reading from the CI cache. Perhaps clearing the cache would resolve.

mattbonnell · 2021-03-31T12:35:53Z

@aeneasr nice, passing :D thanks for unblocking

mattbonnell · 2021-03-31T13:03:13Z

e2e failing on timeout waiting for resources now 🤕

wait-on(12504) Timed out waiting for: http-get://127.0.0.1:4455/health, http-get://127.0.0.1:4456/health; exiting with error
Error: Timed out waiting for: http-get://127.0.0.1:4455/health, http-get://127.0.0.1:4456/health
    at MergeMapSubscriber.project (/home/circleci/project/node_modules/wait-on/lib/wait-on.js:130:25)
    at MergeMapSubscriber._tryNext (/home/circleci/project/node_modules/rxjs/internal/operators/mergeMap.js:69:27)
    at MergeMapSubscriber._next (/home/circleci/project/node_modules/rxjs/internal/operators/mergeMap.js:59:18)
    at MergeMapSubscriber.Subscriber.next (/home/circleci/project/node_modules/rxjs/internal/Subscriber.js:66:18)
    at AsyncAction.dispatch [as work] (/home/circleci/project/node_modules/rxjs/internal/observable/timer.js:31:16)
    at AsyncAction._execute (/home/circleci/project/node_modules/rxjs/internal/scheduler/AsyncAction.js:71:18)
    at AsyncAction.execute (/home/circleci/project/node_modules/rxjs/internal/scheduler/AsyncAction.js:59:26)
    at AsyncScheduler.flush (/home/circleci/project/node_modules/rxjs/internal/scheduler/AsyncScheduler.js:52:32)
    at listOnTimeout (internal/timers.js:554:17)
    at processTimers (internal/timers.js:497:7)

this might have been transitive, going to trigger a re-run

…mattbonnell/kratos into mattbonnell/feat/verification-return-to

mattbonnell · 2021-03-31T14:07:22Z

@aeneasr okay yep, it was transitive, things look good here

aeneasr

Awesome, good job!

mattbonnell force-pushed the mattbonnell/feat/verification-return-to branch 3 times, most recently from 8e92ce2 to 217d4e3 Compare March 15, 2021 01:21

aeneasr requested changes Mar 15, 2021

View reviewed changes

x/http_secure_redirect.go Outdated Show resolved Hide resolved

aeneasr requested changes Mar 16, 2021

View reviewed changes

mattbonnell force-pushed the mattbonnell/feat/verification-return-to branch from dffb625 to b732722 Compare March 17, 2021 01:12

aeneasr reviewed Mar 17, 2021

View reviewed changes

selfservice/strategy/link/strategy_verification.go Outdated Show resolved Hide resolved

aeneasr reviewed Mar 23, 2021

View reviewed changes

selfservice/hook/verification.go Outdated Show resolved Hide resolved

mattbonnell force-pushed the mattbonnell/feat/verification-return-to branch 2 times, most recently from 44e9120 to a04b09b Compare March 23, 2021 22:00

mattbonnell added 13 commits March 25, 2021 07:38

feat: add return_url to verification flow

4b3581e

add returnToKey coalesce

4768e95

refactor redirectTo duplication into helper method

0bb36ab

add GetType tests

7bf9f5b

refactor verificationUseToken redirectTo logic further

dc27c9c

improve refactor

d328690

add Flow.GetRequestURL tests

708ff91

fmt

55d538d

add tests, fixes

dbb1b9f

rename after_verification->after_verification_return_to

dd43569

simplify logic considerably

5df71e3

fix NewPostHookFlow, add tests

d68522e

add tests for verification hook

dc30ad1

mattbonnell force-pushed the mattbonnell/feat/verification-return-to branch from be80f1b to dc30ad1 Compare March 25, 2021 11:38

redirect to default on SecureRedirectTo error

7769863

aeneasr self-assigned this Mar 29, 2021

Merge branch 'master' into mattbonnell/feat/verification-return-to

27baf9f

mattbonnell mentioned this pull request Mar 29, 2021

feat: populate email templates at delivery time, add plaintext defaults #1155

Merged

5 tasks

e2e tests passing

b9a7083

mattbonnell force-pushed the mattbonnell/feat/verification-return-to branch from 329c60a to b9a7083 Compare March 29, 2021 14:29

mattbonnell added 3 commits March 29, 2021 20:52

add e2e test cases

b8ed95c

revert change to test/e2e/run.sh

af26b0b

run format

f68362e

mattbonnell added 2 commits March 31, 2021 08:38

Merge branch 'master' into mattbonnell/feat/verification-return-to

b131b55

fix: error check order in test

4a140be

mattbonnell added 2 commits March 31, 2021 09:11

fix: error check order in test

c77c13e

Merge branch 'mattbonnell/feat/verification-return-to' of github.com:…

896bab2

…mattbonnell/kratos into mattbonnell/feat/verification-return-to

aeneasr self-requested a review March 31, 2021 16:10

aeneasr approved these changes Apr 2, 2021

View reviewed changes

aeneasr merged commit bb99912 into ory:master Apr 2, 2021

mattbonnell deleted the mattbonnell/feat/verification-return-to branch April 2, 2021 15:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add return_url to verification flow #1149

feat: add return_url to verification flow #1149

mattbonnell commented Mar 15, 2021 •

edited

Loading

aeneasr left a comment

aeneasr Mar 16, 2021

mattbonnell Mar 16, 2021 •

edited

Loading

aeneasr Mar 24, 2021

mattbonnell Mar 24, 2021

mattbonnell commented Mar 18, 2021

aeneasr commented Mar 19, 2021

mattbonnell commented Mar 19, 2021

aeneasr left a comment

aeneasr commented Mar 24, 2021

mattbonnell commented Mar 24, 2021

mattbonnell commented Mar 24, 2021

mattbonnell commented Mar 25, 2021

aeneasr commented Mar 29, 2021

mattbonnell commented Mar 29, 2021 •

edited

Loading

aeneasr commented Mar 29, 2021

aeneasr commented Mar 31, 2021

aeneasr commented Mar 31, 2021

mattbonnell commented Mar 31, 2021

mattbonnell commented Mar 31, 2021

mattbonnell commented Mar 31, 2021 •

edited

Loading

mattbonnell commented Mar 31, 2021

aeneasr left a comment

feat: add return_url to verification flow #1149

feat: add return_url to verification flow #1149

Conversation

mattbonnell commented Mar 15, 2021 • edited Loading

Related issue

Proposed changes

Checklist

Further comments

aeneasr left a comment

Choose a reason for hiding this comment

aeneasr Mar 16, 2021

Choose a reason for hiding this comment

mattbonnell Mar 16, 2021 • edited Loading

Choose a reason for hiding this comment

aeneasr Mar 24, 2021

Choose a reason for hiding this comment

mattbonnell Mar 24, 2021

Choose a reason for hiding this comment

mattbonnell commented Mar 18, 2021

aeneasr commented Mar 19, 2021

mattbonnell commented Mar 19, 2021

aeneasr left a comment

Choose a reason for hiding this comment

aeneasr commented Mar 24, 2021

mattbonnell commented Mar 24, 2021

mattbonnell commented Mar 24, 2021

mattbonnell commented Mar 25, 2021

aeneasr commented Mar 29, 2021

mattbonnell commented Mar 29, 2021 • edited Loading

aeneasr commented Mar 29, 2021

aeneasr commented Mar 31, 2021

aeneasr commented Mar 31, 2021

mattbonnell commented Mar 31, 2021

mattbonnell commented Mar 31, 2021

mattbonnell commented Mar 31, 2021 • edited Loading

mattbonnell commented Mar 31, 2021

aeneasr left a comment

Choose a reason for hiding this comment

mattbonnell commented Mar 15, 2021 •

edited

Loading

mattbonnell Mar 16, 2021 •

edited

Loading

mattbonnell commented Mar 29, 2021 •

edited

Loading

mattbonnell commented Mar 31, 2021 •

edited

Loading