feat: return verification flow ID after registration flow #3144

jonas-jonas · 2023-03-03T17:35:19Z

Related issue(s)

Closes #2975

Checklist

~~Discuss, how an E2E test for this could look?~~ See feat: return verification flow ID after registration flow #3144 (comment)
~~Better way to pass data from the hook executors back to the handler?~~ We attached it to the flow object which is modified by other hooks, anyway.

selfservice/hook/verification.go

selfservice/hook/session_issuer.go

zepatrik

Nice, looking pretty good 👍
What other actions do we expect to be added?

selfservice/flow/continue_with.go

selfservice/flow/registration/session.go

selfservice/flow/settings/hook.go

selfservice/hook/verification.go

codecov · 2023-03-09T15:47:21Z

Codecov Report

Merging #3144 (56d29b0) into master (0002668) will increase coverage by 0.04%.
The diff coverage is 94.44%.

❗ Current head 56d29b0 differs from pull request most recent head 6330e71. Consider uploading reports for the commit 6330e71 to get more accurate results

@@            Coverage Diff             @@
##           master    #3144      +/-   ##
==========================================
+ Coverage   77.62%   77.66%   +0.04%     
==========================================
  Files         317      319       +2     
  Lines       20046    20110      +64     
==========================================
+ Hits        15561    15619      +58     
- Misses       3292     3296       +4     
- Partials     1193     1195       +2

Impacted Files	Coverage Δ
driver/registry_default.go	`86.92% <ø> (ø)`
identity/identity_verification.go	`70.96% <ø> (ø)`
selfservice/flow/verification/flow.go	`92.00% <ø> (ø)`
selfservice/hook/verification.go	`73.68% <85.71%> (+1.27%)`	⬆️
selfservice/flow/continue_with.go	`88.88% <88.88%> (ø)`
selfservice/strategy/code/strategy_verification.go	`74.25% <93.54%> (-1.24%)`	⬇️
driver/registry_default_hooks.go	`91.83% <100.00%> (+1.13%)`	⬆️
selfservice/flow/registration/flow.go	`97.10% <100.00%> (+0.17%)`	⬆️
selfservice/flow/registration/hook.go	`82.53% <100.00%> (+0.42%)`	⬆️
selfservice/flow/settings/flow.go	`92.00% <100.00%> (+0.45%)`	⬆️
... and 3 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

jonas-jonas · 2023-03-09T16:06:22Z

What other actions do we expect to be added?

For the recovery flow, we could use this and add settings_ui.

For the various expired type of errors, we are planning on also using this, so we would add these for all the flows recovery_ui, etc.

Then we will also have a special one for OIDC.

jonas-jonas · 2023-03-10T11:03:43Z

selfservice/flow/registration/flow.go

+	// registration.
+	//
+	// required: false
+	// swagger:ignore


Since we aren't returning the registration flow itself, once the registration finishes, this is just used internally, and should not be sent to the client.

Remove the required field, and swagger ignore. json:"-" already omits it from swagger

selfservice/flow/registration/hook.go

jonas-jonas · 2023-03-10T15:37:35Z

...snapshots/TestVerification-description=should_set_all_the_correct_verification_payloads.json

+  {
+    "attributes": {
+      "disabled": false,
+      "name": "csrf_token",


not sure if re-ordering CSRF token "inputs" will be an issue?

It should not matter.

jonas-jonas · 2023-03-13T14:44:31Z

As for E2E tests, right now they (should be) are passing, because the changes are not breaking. We are merely adding new fields to payloads.

I would propose, adding this feature to the reference implementations, once this is merged and adjusting the E2E tests, once that's done. That way we don't need to update the SDK from a branch, etc.

zepatrik

Nice 👍

selfservice/hook/session_issuer_test.go

zepatrik · 2023-03-13T16:44:22Z

...snapshots/TestVerification-description=should_set_all_the_correct_verification_payloads.json

+  {
+    "attributes": {
+      "disabled": false,
+      "name": "csrf_token",


It should not matter.

Benehiko

Nice! This looks really good :)

selfservice/hook/verification.go

aeneasr

First sweep

selfservice/flow/continue_with.go

aeneasr · 2023-03-14T10:01:20Z

selfservice/flow/continue_with.go

+	// Flow contains the ID of the verification flow
+	//
+	// required: true
+	Flow struct {


Please use a dedicated struct for this and give it a swagger:model annotation

The names get quite long. Would've liked to avoid those.. But fine with me, done.

selfservice/flow/continue_with.go

aeneasr · 2023-03-14T10:02:24Z

selfservice/flow/registration/flow.go

+	// registration.
+	//
+	// required: false
+	// swagger:ignore


Remove the required field, and swagger ignore. json:"-" already omits it from swagger

aeneasr · 2023-03-20T12:28:13Z

Is this good to review?

jonas-jonas · 2023-03-20T12:29:39Z

Is this good to review?

If CI is green, yes from my POV.

aeneasr

This is looking grand! The only thing I'm missing is some proof that continue_with works as intended. Basically, this means an end to end test with registration + verification on mobile and/or SPA?

.schema/openapi/patches/schema.yaml

contrib/quickstart/kratos/email-password/kratos.yml

selfservice/flow/continue_with.go

selfservice/hook/show_verification_ui.go

jonas-jonas · 2023-03-20T13:30:59Z

The only thing I'm missing is some proof that continue_with works as intended. Basically, this means an end to end test with registration + verification on mobile and/or SPA?

Yes, that's true. I wanted to merge this first and write some E2E tests then, to avoid the SDK release-dance needed to update both. But I can do that before we merge this PR.

aeneasr · 2023-03-20T13:34:39Z

Yes, that's true. I wanted to merge this first and write some E2E tests then, to avoid the SDK release-dance needed to update both. But I can do that before we merge this PR.

Yes please do that first :) For the SDK - do you know how this works?

jonas-jonas · 2023-03-20T13:35:47Z

do you know how this works?

No :)

aeneasr · 2023-03-20T13:38:12Z

https://github.com/ory/kratos-selfservice-ui-node/blob/7a838e2a2900592817df07067f998c4d6325a7ff/Makefile#L27

Make sure to have the paths set correctly, and you'll also need access to the npm package - if you give me your ID i can add you

jonas-jonas · 2023-03-20T18:44:25Z

.github/workflows/ci.yaml

+        with:
+          repository: ory/kratos-selfservice-ui-react-nextjs
+          path: react-ui
+          ref: jonas-jonas/feat/showVerificationAfterRegistration


selfservice/hook/verification.go

aeneasr · 2023-03-22T10:35:19Z

selfservice/hook/verification_test.go

+			assert.IsType(t, &flow.ContinueWithVerificationUI{}, vf)
+			fView := vf.(*flow.ContinueWithVerificationUI).Flow
+
+			expectedVerificationFlow, err := reg.VerificationFlowPersister().GetVerificationFlow(ctx, fView.ID)


This test assertion changed - previously we synthetically created a new verification URL and compared that with the assertion from the DB. Now we fetch both verification flows from the DB and compare them. Is this still a useful test?

Do you mean removing https://github.com/ory/kratos/pull/3144/files/8169ac2945655590e9210777cf26b7766da2c4a5#diff-24ee502b250d67d70d70ba02ad18f303654b9af76552128958425b1c9043cc18R97-R100? I think that would still be fine, indeed.

aeneasr · 2023-03-22T10:36:13Z

selfservice/hook/verification_test.go

+			switch k {
+			case "settings":
+				originalFlow = &settings.Flow{RequestURL: "http://foo.com/settings?after_verification_return_to=verification_callback"}
+			case "register":
+				originalFlow = &registration.Flow{RequestURL: "http://foo.com/registration?after_verification_return_to=verification_callback"}
+			default:
+				t.FailNow()
+			}
 			require.NoError(t, hf(h, i, originalFlow))
-			expectedVerificationFlow, err = verification.NewPostHookFlow(conf, conf.SelfServiceFlowVerificationRequestLifespan(ctx), "", u, s, originalFlow)
+
+			assert.Emptyf(t, originalFlow.ContinueWith(), "%+v", originalFlow.ContinueWith())


Not sure if I understand what this is doing - you're creating a new original flow and then checking whether it has continue with. That of course will be true, because it's not added in the switch statement.

OR is hf() somehow setting that value?

hf is executing the verification hook, which persists the verification flow, and since all verifiable addresses are now verified, we expect no continue_with items anymore.

This test could use some cleanup, for sure. But OOS of this PR IMO.

jonas-jonas force-pushed the jonas-jonas/fix/verificationIdAfterRegistration branch from c125149 to 5e64e93 Compare March 9, 2023 14:44

jonas-jonas commented Mar 9, 2023

View reviewed changes

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

jonas-jonas commented Mar 9, 2023

View reviewed changes

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

jonas-jonas force-pushed the jonas-jonas/fix/verificationIdAfterRegistration branch 3 times, most recently from 0e4f251 to 1da0e40 Compare March 9, 2023 15:19

zepatrik reviewed Mar 9, 2023

View reviewed changes

jonas-jonas force-pushed the jonas-jonas/fix/verificationIdAfterRegistration branch 2 times, most recently from d45195a to 4c7f9cf Compare March 10, 2023 11:02

jonas-jonas commented Mar 10, 2023

View reviewed changes

selfservice/flow/registration/hook.go Show resolved Hide resolved

jonas-jonas force-pushed the jonas-jonas/fix/verificationIdAfterRegistration branch from 4c7f9cf to cf9c1d9 Compare March 10, 2023 15:33

jonas-jonas commented Mar 10, 2023

View reviewed changes

jonas-jonas force-pushed the jonas-jonas/fix/verificationIdAfterRegistration branch from cf9c1d9 to 22a143a Compare March 13, 2023 07:47

jonas-jonas mentioned this pull request Mar 13, 2023

How to disable sending verification email after registration? #3162

Open

6 tasks

jonas-jonas force-pushed the jonas-jonas/fix/verificationIdAfterRegistration branch from e48561a to 9e986f3 Compare March 13, 2023 08:57

jonas-jonas self-assigned this Mar 13, 2023

jonas-jonas marked this pull request as ready for review March 13, 2023 16:28

jonas-jonas requested a review from aeneasr as a code owner March 13, 2023 16:28

zepatrik previously approved these changes Mar 13, 2023

View reviewed changes

jonas-jonas dismissed zepatrik’s stale review via e7b98b1 March 13, 2023 16:46

Benehiko previously approved these changes Mar 14, 2023

View reviewed changes

CaptainStandby reviewed Mar 14, 2023

View reviewed changes

selfservice/hook/verification.go Show resolved Hide resolved

CaptainStandby reviewed Mar 14, 2023

View reviewed changes

selfservice/hook/verification.go Show resolved Hide resolved

aeneasr reviewed Mar 14, 2023

View reviewed changes

jonas-jonas dismissed Benehiko’s stale review via 847115a March 14, 2023 12:38

jonas-jonas requested a review from aeneasr March 15, 2023 11:12

jonas-jonas force-pushed the jonas-jonas/fix/verificationIdAfterRegistration branch from add0917 to 468484a Compare March 20, 2023 11:43

aeneasr requested changes Mar 20, 2023

View reviewed changes

jonas-jonas force-pushed the jonas-jonas/fix/verificationIdAfterRegistration branch from 468484a to d21afcb Compare March 20, 2023 13:15

jonas-jonas mentioned this pull request Mar 20, 2023

feat: show verification UI after registration ory/kratos-selfservice-ui-react-nextjs#55

Merged

jonas-jonas force-pushed the jonas-jonas/fix/verificationIdAfterRegistration branch from d21afcb to 6ffe86c Compare March 20, 2023 18:43

jonas-jonas commented Mar 20, 2023

View reviewed changes

jonas-jonas force-pushed the jonas-jonas/fix/verificationIdAfterRegistration branch 2 times, most recently from e9462d3 to 0037f09 Compare March 21, 2023 06:45

feat: return verification_flow_id after registration

f0fd9ff

jonas-jonas force-pushed the jonas-jonas/fix/verificationIdAfterRegistration branch from 0037f09 to e87ddef Compare March 21, 2023 11:34

chore: add mobile e2e tests

8169ac2

jonas-jonas force-pushed the jonas-jonas/fix/verificationIdAfterRegistration branch from e87ddef to 8169ac2 Compare March 21, 2023 15:29

aeneasr previously approved these changes Mar 22, 2023

View reviewed changes

aeneasr reviewed Mar 22, 2023

View reviewed changes

jonas-jonas added 3 commits March 22, 2023 14:52

chore: remove persister call

9e7d90c

chore: add verification ui hook tests

0709683

test: add e2e tests for settings

d3ab6da

jonas-jonas dismissed aeneasr’s stale review via d3ab6da March 22, 2023 13:54

jonas-jonas added 2 commits March 22, 2023 14:55

chore: format

5c19201

chore: use master as base for example repos

c6c26e3

jonas-jonas force-pushed the jonas-jonas/fix/verificationIdAfterRegistration branch from 231ec43 to c6c26e3 Compare March 23, 2023 07:51

chore: last cleanup

6330e71

aeneasr merged commit eb854be into master Mar 23, 2023
24 checks passed

aeneasr deleted the jonas-jonas/fix/verificationIdAfterRegistration branch March 23, 2023 09:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: return verification flow ID after registration flow #3144

feat: return verification flow ID after registration flow #3144

jonas-jonas commented Mar 3, 2023 •

edited

zepatrik left a comment

codecov bot commented Mar 9, 2023 •

edited

jonas-jonas commented Mar 9, 2023

jonas-jonas Mar 10, 2023

aeneasr Mar 14, 2023

jonas-jonas Mar 14, 2023

jonas-jonas Mar 10, 2023

zepatrik Mar 13, 2023

jonas-jonas commented Mar 13, 2023

zepatrik left a comment

zepatrik Mar 13, 2023

Benehiko left a comment

aeneasr left a comment

aeneasr Mar 14, 2023

jonas-jonas Mar 14, 2023

aeneasr Mar 14, 2023

aeneasr commented Mar 20, 2023

jonas-jonas commented Mar 20, 2023

aeneasr left a comment

jonas-jonas commented Mar 20, 2023

aeneasr commented Mar 20, 2023

jonas-jonas commented Mar 20, 2023

aeneasr commented Mar 20, 2023

jonas-jonas Mar 20, 2023

aeneasr Mar 22, 2023

jonas-jonas Mar 22, 2023

jonas-jonas Mar 22, 2023

aeneasr Mar 22, 2023

jonas-jonas Mar 22, 2023

jonas-jonas Mar 22, 2023

feat: return verification flow ID after registration flow #3144

feat: return verification flow ID after registration flow #3144

Conversation

jonas-jonas commented Mar 3, 2023 • edited

Related issue(s)

Checklist

zepatrik left a comment

Choose a reason for hiding this comment

codecov bot commented Mar 9, 2023 • edited

Codecov Report

jonas-jonas commented Mar 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonas-jonas commented Mar 13, 2023

zepatrik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Benehiko left a comment

Choose a reason for hiding this comment

aeneasr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aeneasr commented Mar 20, 2023

jonas-jonas commented Mar 20, 2023

aeneasr left a comment

Choose a reason for hiding this comment

jonas-jonas commented Mar 20, 2023

aeneasr commented Mar 20, 2023

jonas-jonas commented Mar 20, 2023

aeneasr commented Mar 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonas-jonas commented Mar 3, 2023 •

edited

codecov bot commented Mar 9, 2023 •

edited