Local kratos email verification with mailslurper #1648

thehabbos007 · 2022-12-12T15:35:45Z

🌟 What is the purpose of this PR?

This PR adds email verification flows to Kratos, configuring the code method and custom mail templates.
There are some potential type issues with the kratos SDK for typescript, see ory/kratos#2943 which means the current implementation casts a type in a place where it isn't expected.

The current setup allows for verification through the UI, but it isn't neatly integreated/expected. I've included the UI scaffold for prettifying

🔗 Related links

Asana task (internal)

🚫 Blocked by

🔍 What does this change?

Kratos dev config (and anticipation for prod config)
Adds new frontend endpoint to receive verification tokens
Email sending through mailslurper locally
Email templates for HTML and plaintext

📜 Does this require a change to the docs?

⚠️ Known issues

The typescript types on the frontend for submitting the verification flow seems to be incorrect, hence some casting to get it to work.
Production does not have this enabled yet

🐾 Next steps

Prettier email templates
Enabling n production

🛡 What tests cover this?

none

❓ How to test this?

Checkout the branch as you would normally (external-service + yarn dev)
Sign up a new user and choose a shortname/preferred name
Go to http://localhost:4436/# to access mailslurper
open network explorer tab in dev tools
Click verification link and verify email
Observe next whoami request contains a verified mail

📹 Demo

mail_verify.mp4

codecov · 2022-12-12T15:41:05Z

Codecov Report

Attention: Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Project coverage is 56.85%. Comparing base (1a3de5f) to head (8ff88d9).
Report is 3448 commits behind head on main.

Files with missing lines	Patch %	Lines
packages/hash/api/src/auth/index.ts	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1648      +/-   ##
==========================================
+ Coverage   56.76%   56.85%   +0.08%     
==========================================
  Files         224      224              
  Lines       15850    15826      -24     
  Branches      388      384       -4     
==========================================
  Hits         8998     8998              
+ Misses       6847     6823      -24     
  Partials        5        5

Flag	Coverage Δ
backend-integration-tests	`2.70% <ø> (ø)`
unit-tests	`1.74% <0.00%> (+0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

benwerner01

Thanks Ahmad. Here are some comments, still need to test this out locally.

benwerner01 · 2022-12-13T09:28:31Z

packages/hash/external-services/kratos/kratos.dev.yml

+ config:
+ # The URL for verification emails are set through the link method
+ # but we're using the code method, so we disable this method for usage.
+ enabled: false
+ # Set through SELFSERVICE_METHODS_LINK_CONFIG_BASE_URL
+ base_url: http://localhost:3000/api/ory
+ code:
+ config:
+ # and make sure to enable the code method.
+ enabled: true


Nice, should we make an effort from here on to start copying in the documentation from the ory kratos website?

I think it's very helpful for values that we know we will inject new values into such as this and the UI_URLs. Fortunately their env naming matches the path of the yaml but in upper case and with underscores. I think it's helpful to have the env var to be easily-copied, though

benwerner01 · 2022-12-13T09:30:57Z

packages/hash/external-services/kratos/kratos.dev.yml

+courier:
+ template_override_path: /etc/config/kratos/templates
+ smtp:
+ connection_uri: set-through-env-arg-COURIER_SMTP_CONNECTION_URI


Are you able to set this dummy value because the environment variable overrides whatever we specify in the config?

Yes, exactly, I think for dev mode, we should maybe just let it be the mailslurper url and add a comment as we do for SELFSERVICE_METHODS_LINK_CONFIG_BASE_URL above. I will do this now

benwerner01 · 2022-12-13T09:39:08Z

...es/hash/external-services/kratos/templates/recovery_code/invalid/email.body.plaintext.gotmpl

@@ -0,0 +1,9 @@
+Hi,
+
+you (or someone else) entered this email address when trying to recover access to an account.


I think it would make it less ambiguous for now what these emails are referring to if we refer to a "HASH account" rather than just "account". (this suggestion applies to most templates)

Suggested change

you (or someone else) entered this email address when trying to recover access to an account.

you (or someone else) entered this email address when trying to recover access to a HASH account.

cc @nonparibus for thoughts on the initial phrasing here

Agreed "HASH account" preferable.

I would like to do a big pass at all of our transactional email copy soon.

Not necessarily as part of this PR, but could we consider how we might centrally collect all of our transactional email text in one place?

This could be creating an index pointing to the relevant lines of files like this, it could be a single directory that contains the actual emails themselves, or something like else (better) entirely. Either way we're not well set up for this at the moment.

@nonparibus for now all the kratos transactional emails are in this folder https://github.com/hashintel/hash/tree/e98a86be821317929dfaa15edfd255af51222726/packages/hash/external-services/kratos/templates
for emails that we will send through HASH, we have to consider what we do to either co-locate them together or make them easily searchable. For now I think this kratos grouping is a nice start

Just want to flag that this suggestion also applies to the remaining email templates (including email subjects). Let me know if you intend on addressing this @thehabbos007

Sorry, missed that this was for all: f710e62

Thanks -- wasn't aware we had the dir already. This is great.

I don't know what ultimate best-practice looks like here, but I would be happy with either:

somewhere I can go that links out to all the different collections of transactional emails (like this), provided we actually keep it up to date;

a single directory in which we store all transactional emails from across the app.

My sense is the latter probably makes marginally more sense, and will be easier to track.

I'll review/update Kratos email copy in a separate PR. Please make any adjustments between the two of you that you think make sense in the meantime.

I agree that a single directory would make most sense, it may be difficult to do currently without knowing how we structure mails in the HASH api. I'll make an index that links to the email templates in-use from /packages/hash/README.md where we already have a seciton on sending emails. Does that sound good for now?

Changed in 8ff88d9
Index at https://github.com/hashintel/hash/blob/8ff88d9d44cff7c81be30829c55b8ea2b39cc691/packages/hash/README.md#sending-emails

…/invalid/email.body.plaintext.gotmpl Co-authored-by: Ben Werner <42802102+benwerner01@users.noreply.github.com>

packages/hash/frontend/src/pages/verification.page.tsx

benwerner01 · 2022-12-13T11:58:47Z

packages/hash/frontend/src/pages/verification.page.tsx

+ })
+ .then(({ data }) => {
+ setFlow(data);
+ extractFlowCodeValue(data);


Could we programmatically submit the form when it is populated from the query params for the first time? This would prevent the user from having to click anything.

I think we could, yes. But the reason to use these codes in the first place is to prevent mail apps to auto-open the links on the user's behalf. I appreciate that we'd gate this behind the /login interface and not submit, but I think we're better off not automating it for now

Ah I hadn't considered that, let's keep it as is for now then.

Co-authored-by: Ben Werner <42802102+benwerner01@users.noreply.github.com>

benwerner01

Thanks Ahmad, this is working locally for me 👍

kachkaev · 2022-12-13T17:20:20Z

@thehabbos007 I got CI failing in #1644 after merging main with this change:

@hashintel/hash-frontend:lint:eslint: /home/runner/work/hash/hash/packages/hash/frontend/src/pages/verification.page.tsx
@hashintel/hash-frontend:lint:eslint:    55:36  error  Unnecessary optional chain on a non-nullish value  @typescript-eslint/no-unnecessary-condition
@hashintel/hash-frontend:lint:eslint:   114:51  error  Unnecessary optional chain on a non-nullish value  @typescript-eslint/no-unnecessary-condition
@hashintel/hash-frontend:lint:eslint:   118:26  error  Unnecessary optional chain on a non-nullish value  @typescript-eslint/no-unnecessary-condition
@hashintel/hash-frontend:lint:eslint:   133:35  error  Unnecessary optional chain on a non-nullish value  @typescript-eslint/no-unnecessary-condition
@hashintel/hash-frontend:lint:eslint:   171:18  error  Unnecessary optional chain on a non-nullish value  @typescript-eslint/no-unnecessary-condition
@hashintel/hash-frontend:lint:eslint:

I’ve removed a few ? instances, but this may break things if typings are incorrect. Could you please comment on #1644 if you see potential issues?

thehabbos007 added 4 commits December 12, 2022 13:32

Add email flow

78593bf

Enable email sending in dev mode to mailslurper

61fe1a5

Disable prod email sending

bb07373

Make kratos error a debug

539846e

thehabbos007 requested a review from benwerner01 December 12, 2022 15:35

github-actions bot added area/apps > hash* Affects HASH (a `hash-*` app) area/apps > hash-api Affects the HASH API (app) frontend labels Dec 12, 2022

vercel bot deployed to Preview – hash December 12, 2022 15:39 View deployment

Merge branch 'main' into asa/kratos-mail-sending

9ecc234

vercel bot deployed to Preview – hash December 12, 2022 16:19 View deployment

Merge branch 'main' into asa/kratos-mail-sending

460834c

vercel bot deployed to Preview – hash December 13, 2022 09:49 View deployment

benwerner01 reviewed Dec 13, 2022

View reviewed changes

thehabbos007 and others added 2 commits December 13, 2022 10:57

add comment for smtp uri

38b3db1

Update packages/hash/external-services/kratos/templates/recovery_code…

e98a86b

…/invalid/email.body.plaintext.gotmpl Co-authored-by: Ben Werner <42802102+benwerner01@users.noreply.github.com>

vercel bot deployed to Preview – hash December 13, 2022 10:06 View deployment

benwerner01 reviewed Dec 13, 2022

View reviewed changes

Update packages/hash/frontend/src/pages/verification.page.tsx

75e8521

Co-authored-by: Ben Werner <42802102+benwerner01@users.noreply.github.com>

vercel bot deployed to Preview – hash December 13, 2022 12:08 View deployment

Email template update to HASH account

f710e62

vercel bot deployed to Preview – hash December 13, 2022 13:00 View deployment

benwerner01 previously approved these changes Dec 13, 2022

View reviewed changes

Email index in readme

8ff88d9

thehabbos007 dismissed benwerner01’s stale review via 8ff88d9 December 13, 2022 13:15

benwerner01 approved these changes Dec 13, 2022

View reviewed changes

vercel bot deployed to Preview – hash December 13, 2022 13:20 View deployment

thehabbos007 enabled auto-merge (squash) December 13, 2022 13:21

thehabbos007 merged commit 8e4dbfb into main Dec 13, 2022

thehabbos007 deleted the asa/kratos-mail-sending branch December 13, 2022 13:25

vilkinsons added type/eng > frontend Owned by the @frontend team and removed area: frontend labels Jul 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Local kratos email verification with mailslurper #1648

Local kratos email verification with mailslurper #1648

thehabbos007 commented Dec 12, 2022 •

edited by vilkinsons

Loading

codecov bot commented Dec 12, 2022 •

edited

Loading

benwerner01 left a comment

benwerner01 Dec 13, 2022

thehabbos007 Dec 13, 2022

benwerner01 Dec 13, 2022

thehabbos007 Dec 13, 2022

thehabbos007 Dec 13, 2022

benwerner01 Dec 13, 2022

vilkinsons Dec 13, 2022 •

edited

Loading

thehabbos007 Dec 13, 2022

benwerner01 Dec 13, 2022

thehabbos007 Dec 13, 2022

vilkinsons Dec 13, 2022

thehabbos007 Dec 13, 2022

thehabbos007 Dec 13, 2022

benwerner01 Dec 13, 2022

thehabbos007 Dec 13, 2022

benwerner01 Dec 13, 2022

benwerner01 left a comment

kachkaev commented Dec 13, 2022

		@@ -0,0 +1,9 @@
		Hi,

		you (or someone else) entered this email address when trying to recover access to an account.

Local kratos email verification with mailslurper #1648

Local kratos email verification with mailslurper #1648

Conversation

thehabbos007 commented Dec 12, 2022 • edited by vilkinsons Loading

🌟 What is the purpose of this PR?

🔗 Related links

🚫 Blocked by

🔍 What does this change?

📜 Does this require a change to the docs?

⚠️ Known issues

🐾 Next steps

🛡 What tests cover this?

❓ How to test this?

📹 Demo

codecov bot commented Dec 12, 2022 • edited Loading

Codecov Report

benwerner01 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vilkinsons Dec 13, 2022 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benwerner01 left a comment

Choose a reason for hiding this comment

kachkaev commented Dec 13, 2022

thehabbos007 commented Dec 12, 2022 •

edited by vilkinsons

Loading

codecov bot commented Dec 12, 2022 •

edited

Loading

vilkinsons Dec 13, 2022 •

edited

Loading