Skip to content

Conversation

kangmingtay
Copy link
Member

@kangmingtay kangmingtay commented Feb 8, 2022

What kind of change does this PR introduce?

API Additions / Changes

  1. POST /verify now accepts an email field which would be used together with the token to verify the user.
{
  "type": "signup",
  "email": "foo@example.com",
  "token": "random token",
  "redirect_to": "my_redirect_to_url"
}
  1. If type=email_change and MAILER_SECURE_EMAIL_CHANGE_ENABLED=true, the old email will be used for verifying both OTPs sent to the old and new email address.

Implementation Details

  1. I refactored the common parts in signupVerify, recoverVerify, emailChangeVerify and smsVerify into a new function called verifyUserAndToken
  2. verifyUserAndToken does the following:
    • Checks if the verification is an url or otp verification
    • If it's an otp verification, check if its an sms or email otp.
  3. Checks if the user is banned
  4. Checks if the otp is valid

To-Dos:

  • Allow url link / otp expiration to be configurable rather than restricted to 24hrs
  • Send a human-readable hash / slug instead of an otp (will be added as an enhancement in a separate PR)

@kangmingtay kangmingtay force-pushed the km/send-code-in-email branch from 6bf63b8 to 7d2a125 Compare February 8, 2022 06:54
@kangmingtay kangmingtay force-pushed the km/send-code-in-email branch from 7d2a125 to 352d36b Compare February 8, 2022 06:55
@kangmingtay kangmingtay requested review from J0 and awalias February 9, 2022 09:54
@kangmingtay kangmingtay changed the title [WIP] fix: send otp in email link fix: send otp in email link Feb 9, 2022
@kangmingtay kangmingtay force-pushed the km/send-code-in-email branch from a82ff9c to 99c2375 Compare February 9, 2022 10:55
Copy link
Contributor

@J0 J0 left a comment

Choose a reason for hiding this comment

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

Looks fine to me. But let me sleep on it and read once more before approving

return time.Now().Before(expiresAt) && (actual == expected)
}

func isUrlVerification(params *VerifyParams) bool {
Copy link
Contributor

@J0 J0 Feb 10, 2022

Choose a reason for hiding this comment

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

/nit Would be nice if we could check directly params.Type == URLVerification instead of via exclusion (e.g. it is not (email or sms) so it is url)

That said, it is not a priority and would require a refactor. Probably not within a scope of this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah i thought about that too but the type (signup vs recover) is tightly coupled to the ConfirmationToken & RecoveryToken fields, so it would be hard to tell whether the URL is a confirmation or recovery URL if we introduced a params.Type == URLVerification

Copy link
Contributor

@J0 J0 left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks for the PR :)

Copy link
Member

@awalias awalias left a comment

Choose a reason for hiding this comment

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

I may have overlooked it, but do we have a test case for: valid token used in combination with the email address of a different user?

@kangmingtay
Copy link
Member Author

I may have overlooked it, but do we have a test case for: valid token used in combination with the email address of a different user?

yeah i can add a test for that too, in this case, the verification should be rejected because the valid token doesn't belong to that user.

@kangmingtay kangmingtay merged commit f853f45 into master Feb 10, 2022
@kangmingtay kangmingtay deleted the km/send-code-in-email branch February 10, 2022 07:49
@github-actions
Copy link
Contributor

🎉 This PR is included in version 2.5.8 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@asyncink
Copy link

Hi guys and thank you for your work! Can you please provide some information about your plans to support email OTP in supabase-js client? @J0

bnjmnt4n added a commit to bnjmnt4n/auth that referenced this pull request Mar 23, 2022
This aligns the connection handling behaviour with the rest of the
functions in `api/verify.go`.

This looks like it might have been a leftover bug during refactoring in
PR supabase#379.
kangmingtay pushed a commit that referenced this pull request Mar 23, 2022
…424)

This aligns the connection handling behaviour with the rest of the
functions in `api/verify.go`.

This looks like it might have been a leftover bug during refactoring in
PR #379.
@cloudorbush
Copy link

This is awesome @kangmingtay.

Now the OTP code sent looks like this:

pLPxtiXsMC_D7hQqhuLQDw

Unfortunately that's too long to copy manually.

Especially on mobile that doesn't add much benefit over redirects.

Instead an example code should look like this:

234 432

That way, a user could type it manually from the email notification they receive, and not have to close the app.

I'm not sure how much of a headache it is to implement this, but it's the standard on most other services.

@kangmingtay
Copy link
Member Author

hey @vbylen, with this PR (#446), the email codes are now in the following format: ycjho-qjvns-nadyy-kcpbu which would be much easier for the user to either copy manually or type it manually since they are divided into even chunks.

@cloudorbush
Copy link

great job @kangmingtay

Now if it was possible to replace it with just 6 digits like in two-factor authentication... 😄

@zebaroni
Copy link

ycjho-qjvns-nadyy-kcpbu Is not easy at all for a user to copy...

@cloudorbush
Copy link

@bicijay agreed, this was a missed opportunity

@kiwicopple thoughts?

@kangmingtay kangmingtay mentioned this pull request Jun 25, 2022
2 tasks
@kangmingtay
Copy link
Member Author

@bicijay @vbylen we're working on a PR to shorten the email otp to [6-10] digits long, thanks for the feedback everyone!

@kaaloo
Copy link

kaaloo commented Sep 2, 2022

Is there any ongoing work on this @kangmingtay ?

@bicijay @vbylen we're working on a PR to shorten the email otp to [6-10] digits long, thanks for the feedback everyone!

@J0
Copy link
Contributor

J0 commented Sep 2, 2022

Hey @kaaloo,

The work by @kangmingtay to shorten the email otp to 6-10 digits has been merged. Do check out the/generate_link endpoint on the README and let us know if you have any questions.

Thanks!

uxodb pushed a commit to uxodb/auth that referenced this pull request Nov 13, 2024
* fix: send otp in email links

* fix: allow verifying otps from emails

* add tests for email_change verification

* fix: add env var to configure email link / token expiry

* docs: update README

* fix: verify phone & email before fetching user

* add test for invalid email otp
uxodb pushed a commit to uxodb/auth that referenced this pull request Nov 13, 2024
…upabase#424)

This aligns the connection handling behaviour with the rest of the
functions in `api/verify.go`.

This looks like it might have been a leftover bug during refactoring in
PR supabase#379.
LashaJini pushed a commit to LashaJini/auth that referenced this pull request Nov 13, 2024
* fix: send otp in email links

* fix: allow verifying otps from emails

* add tests for email_change verification

* fix: add env var to configure email link / token expiry

* docs: update README

* fix: verify phone & email before fetching user

* add test for invalid email otp
LashaJini pushed a commit to LashaJini/auth that referenced this pull request Nov 13, 2024
…upabase#424)

This aligns the connection handling behaviour with the rest of the
functions in `api/verify.go`.

This looks like it might have been a leftover bug during refactoring in
PR supabase#379.
LashaJini pushed a commit to LashaJini/auth that referenced this pull request Nov 15, 2024
* fix: send otp in email links

* fix: allow verifying otps from emails

* add tests for email_change verification

* fix: add env var to configure email link / token expiry

* docs: update README

* fix: verify phone & email before fetching user

* add test for invalid email otp
LashaJini pushed a commit to LashaJini/auth that referenced this pull request Nov 15, 2024
…upabase#424)

This aligns the connection handling behaviour with the rest of the
functions in `api/verify.go`.

This looks like it might have been a leftover bug during refactoring in
PR supabase#379.
cemalkilic pushed a commit that referenced this pull request Aug 7, 2025
* fix: send otp in email links

* fix: allow verifying otps from emails

* add tests for email_change verification

* fix: add env var to configure email link / token expiry

* docs: update README

* fix: verify phone & email before fetching user

* add test for invalid email otp
cemalkilic pushed a commit that referenced this pull request Aug 7, 2025
…424)

This aligns the connection handling behaviour with the rest of the
functions in `api/verify.go`.

This looks like it might have been a leftover bug during refactoring in
PR #379.
xeladotbe pushed a commit to xeladotbe/supabase-auth that referenced this pull request Sep 27, 2025
* fix: send otp in email links

* fix: allow verifying otps from emails

* add tests for email_change verification

* fix: add env var to configure email link / token expiry

* docs: update README

* fix: verify phone & email before fetching user

* add test for invalid email otp
xeladotbe pushed a commit to xeladotbe/supabase-auth that referenced this pull request Sep 27, 2025
…upabase#424)

This aligns the connection handling behaviour with the rest of the
functions in `api/verify.go`.

This looks like it might have been a leftover bug during refactoring in
PR supabase#379.
issuedat pushed a commit that referenced this pull request Sep 30, 2025
* fix: send otp in email links

* fix: allow verifying otps from emails

* add tests for email_change verification

* fix: add env var to configure email link / token expiry

* docs: update README

* fix: verify phone & email before fetching user

* add test for invalid email otp
issuedat pushed a commit that referenced this pull request Sep 30, 2025
…424)

This aligns the connection handling behaviour with the rest of the
functions in `api/verify.go`.

This looks like it might have been a leftover bug during refactoring in
PR #379.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Email Confirm and Password reset via code

7 participants