-
Notifications
You must be signed in to change notification settings - Fork 407
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
feat(server): [nan-869] email verification on signup #2173
feat(server): [nan-869] email verification on signup #2173
Conversation
packages/server/lib/controllers/v1/account/resendVerificationEmail.ts
Outdated
Show resolved
Hide resolved
…-validate-email-on-signup-with-workos
…-validate-email-on-signup-with-workos
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Massive work, especially on the API it's so much clearer like this 🚀
const user = getUser.value; | ||
|
||
if (!user.email_verified) { | ||
sendVerificationEmail(user.email, user.name, user.email_verification_token as string); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like it's a huge side effect imo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, wasn't sure what to do here, but refactored it so that a button appears to re-trigger the verification email.
verification-email.mov
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
amazing 😍
const salt = crypto.randomBytes(16).toString('base64'); | ||
const hashedPassword = (await util.promisify(crypto.pbkdf2)(password, salt, 310000, 32, 'sha256')).toString('base64'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could be worth factorizing this since it's used multiples times
} | ||
}; | ||
|
||
if (!loaded) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is seems more related to the button that the page, it flickers when clicking
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't reproduce. Can you show an example of this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Screen.Recording.2024-05-22.at.09.49.45.mov
…-validate-email-on-signup-with-workos
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good 💯
A few open comments but nothing blocking
Describe your changes
Verify a user's email on signup and route them through an email verification flow.
Issue ticket number and link
NAN-869
Checklist before requesting a review (skip if just adding/editing APIs & templates)