Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement email verification #7161

Merged
merged 39 commits into from
Jul 25, 2023
Merged

Implement email verification #7161

merged 39 commits into from
Jul 25, 2023

Conversation

frcroth
Copy link
Member

@frcroth frcroth commented Jun 19, 2023

URL of deployed dev instance (used for testing):

  • https://___.webknossos.xyz (Does not make a lot of sense because you need to read the mails for testing)

Steps to test:

  • Sign up at webknossos.
  • Notice that your mail is not yet verified (you can check this in the database)
    • If the config requires mail verification and the grace period is 0, you will notice that you can not sign in (even after the account was activated)
  • Click the link in the mail (log mails to stdout)
  • Your mail will be verified (you can check in db multiUser table)
  • You can now log in.
  • Clicking the link again results in an error.

TODO

  • Questions about the Verification flow
    • When do users get the verification mail? (Maybe only after activation by admin)
    • Add verification link to welcome mail?
    • When verification is required (settings), after the grace period, users are not allowed to sign in. Don't know if that makes sense (also resending verification link currently needs sign in, at least that should be possible)
  • Once flow is clear, also write about it in docs
  • Write verification mail
  • Mark verification keys as used
  • What would a good default application.conf look like?
  • Update application.conf (Do this immediately before merging)
  • Frontend
  • Refresh snapshots (Does not work for me with yarn refresh-all-snapshots)

Issues:


(Please delete unneeded items, merge only when none are left open)

@frcroth frcroth marked this pull request as draft June 19, 2023 16:02
@philippotto
Copy link
Member

I implemented the front-end part so that the user can click on a verification link. In the happy path, a success message is shown. Otherwise, an error is shown with a "resend" link. In both cases, the user is directly redirected to the dashboard.

Add verification link to welcome mail?

I think, verification links in welcome mails are more likely to be overlooked. Therefore, I'd rather send a dedicated "Please verify" email.

When do users get the verification mail? (Maybe only after activation by admin)

No strong opinion. I think, it could happen directly after sign in. The sooner the better maybe?

When verification is required (settings), after the grace period, users are not allowed to sign in. Don't know if that makes sense (also resending verification link currently needs sign in, at least that should be possible)

I think, this is ok, but I would simply send a new verification email automatically if a sign in fails due to missing verification. This saves the user from an additional click and makes the UI simpler, too. Maybe this needs throttling to avoid that somebody uses WK to spam another email address with verification mails?


Another note: In my opinion the verify route should check if the user is already verified as the very first check. Then, a 200 could simply be returned. Otherwise, it can be weird to get a "verification failed" error when clicking on an old link even though one is already verified.

@frcroth
Copy link
Member Author

frcroth commented Jun 26, 2023

@philippotto

  • I changed the route to POST
  • When email verification fails (because the grace period is over), an email verification link is automatically resent. Throttling should not be necessary, since this requires pretty malicious intent. Logging was added to be able to identify if something like that were to happen
  • Currently, if the user ignores the verification mail (or it ends up in spam), the user does not interact with the system again until they are no longer able to sign in. This is pretty harsh. Maybe the frontend could display a banner or a pop up on sign in which states that email verification has not happened yet? Perhaps with a button to resend the verification mail.
  • When clicking an expired email verification link, the error message contains a "Resend verification mail" button. This does not work, as it requires the user to be signed in.

Another note: In my opinion the verify route should check if the user is already verified as the very first check. Then, a 200 could simply be returned. Otherwise, it can be weird to get a "verification failed" error when clicking on an old link even though one is already verified.

This has been implemented (although the frontend now misleadingly prints "Successfully verified your email")

@frcroth frcroth requested a review from fm3 June 26, 2023 13:36
@frcroth frcroth marked this pull request as ready for review June 26, 2023 13:36
@philippotto
Copy link
Member

@frcroth

  • I changed the route to POST
  • When email verification fails (because the grace period is over), an email verification link is automatically resent. Throttling should not be necessary, since this requires pretty malicious intent. Logging was added to be able to identify if something like that were to happen

👍
I didn't test it yet, but did you include an error message in that case which tells the user that a verification email was (re)sent?

  • Currently, if the user ignores the verification mail (or it ends up in spam), the user does not interact with the system again until they are no longer able to sign in. This is pretty harsh. Maybe the frontend could display a banner or a pop up on sign in which states that email verification has not happened yet? Perhaps with a button to resend the verification mail.

Yes, we can do that. How would the front-end know about this? Maybe include isEmailVerified in publicWrites of User?

  • When clicking an expired email verification link, the error message contains a "Resend verification mail" button. This does not work, as it requires the user to be signed in.

I changed it so that there's an error message telling the user to log in (if they tried to click resend without being logged in). If they can log in, they can click the link again. If they cannot log in, an email is resent automatically.

Another note: In my opinion the verify route should check if the user is already verified as the very first check. Then, a 200 could simply be returned. Otherwise, it can be weird to get a "verification failed" error when clicking on an old link even though one is already verified.

This has been implemented (although the frontend now misleadingly prints "Successfully verified your email")

I think, this is okay. The end result for the user is the same: the email is verified.

@frcroth
Copy link
Member Author

frcroth commented Jun 30, 2023

@philippotto

I didn't test it yet, but did you include an error message in that case which tells the user that a verification email was (re)sent?

Yes

Yes, we can do that. How would the front-end know about this? Maybe include isEmailVerified in publicWrites of User?

Added this now 👍

I changed it so that there's an error message telling the user to log in (if they tried to click resend without being logged in). If they can log in, they can click the link again. If they cannot log in, an email is resent automatically.

Yes, I think that works.

For possible frontend changes, do the application.conf settings need to be exposed somwhere (e.g. to say "If you don't verify in X days ...") or are they already automatically available?

@philippotto
Copy link
Member

Cool, I added the verification warning now. It appears every time the active user is set (e.g., log in, registration but also on each full page load). Might be a bit annoying, but we really want that the user verifies the email. So, it's fine in my opinion.

For possible frontend changes, do the application.conf settings need to be exposed somwhere (e.g. to say "If you don't verify in X days ...") or are they already automatically available?

Only the features block is automatically available afaik. However, I don't think we really need to explain the grace period. Email verification should be a no-brainer (and once lock-out happens it should be a matter of seconds to verify it since a new mail will arrive).

@hotzenklotz
Copy link
Member

hotzenklotz commented Jul 11, 2023

Observations from testing:

  • When clicking on the verify link in the email, WK is opened in a new browser window and there I am 1) unverified and then 2) immediately verified
    image.
  • When changing a user's email address (Admin -> Users), we need to re-verify. (Maybe follow up issue?)
  • I didn't like the email templates. Here is the OpenAI email in comparison:
    • Use a button
    • Add info about "link expires in x days"
    • Use their wording.
    image image

@frcroth
Copy link
Member Author

frcroth commented Jul 18, 2023

@hotzenklotz

When changing a user's email address (Admin -> Users), we need to re-verify. (Maybe follow up issue?)

This is already implemeted. When user data is updated, the email verification is revoked (in the last commit I changed it so it is only invalidated if the email has actually changed

I didn't like the email templates. Here is the OpenAI email in comparison:

Agreed, I like the button and the old template was only a placeholder. However, wouldn't it make sense to include the link as well? That's something I often see "If the button does not work, use this link". I guess for non-HTML mail clients?

@philippotto
Copy link
Member

I just pushed a fix for the confusing toast when verifying the email. Let me know in case anything front-end related needs to happen.

@frcroth frcroth requested a review from fm3 July 18, 2023 11:26
Copy link
Member

@fm3 fm3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Backend LGTM :)

Is it expected that the snapshots are different again now? (May be due to email address changes in the tests? Should I refresh the snapshots again?)

conf/application.conf Show resolved Hide resolved
@frcroth
Copy link
Member Author

frcroth commented Jul 20, 2023

Is it expected that the snapshots are different again now? (May be due to email address changes in the tests? Should I refresh the snapshots again?)

Yes, please refresh. I think it is because of changes in the initial data.

@frcroth
Copy link
Member Author

frcroth commented Jul 24, 2023

@fm3 What is the path for merging? Does this need another frontend review to get approval?

@fm3
Copy link
Member

fm3 commented Jul 24, 2023

Yes I think that would be helpful. Since tom is currently out of office, maybe @daniel-wer could have a look?

I also still see quite a few open checkboxes both in the PR on top and in some of the comments. Do you think that anything of that needs addressing @frcroth ?

@frcroth
Copy link
Member Author

frcroth commented Jul 24, 2023

@fm3

I also still see quite a few open checkboxes both in the PR on top and in some of the comments. Do you think that anything of that needs addressing @frcroth ?

Are docs useful/ necessary? Also opinions on the default application.conf? Maybe:

 emailVerification {
      activated = true
	  required = false
      gracePeriod = 7 days
      linkExpiry = 30 days
}

I think the flow questions are mostly settled now.

Copy link
Member

@daniel-wer daniel-wer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Frontend LGTM. Testing also went very well 👍

  • I wasn't able to check what the HTML-rendered email looks like, maybe you could upload a screenshot in this PR or confirm that someone checked that it renders correctly.

@frcroth frcroth requested a review from fm3 July 25, 2023 07:22
Copy link
Member

@fm3 fm3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Email looks alright to me :)

image

@frcroth frcroth merged commit 4d9fd8c into master Jul 25, 2023
@frcroth frcroth deleted the verify-emails branch July 25, 2023 09:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ask users to confirm their email address
5 participants