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

fix: apply mailer autoconfirm config to update user email #1646

Merged
merged 2 commits into from
Jul 4, 2024

Conversation

kangmingtay
Copy link
Member

What kind of change does this PR introduce?

  • GOTRUE_MAILER_AUTOCONFIRM setting should be respected in the update email flow via PUT /user

What is the current behavior?

What is the new behavior?

  • When GOTRUE_MAILER_AUTOCONFIRM=true, updating a user's email will not require email confirmation.

Additional context

Add any other context or screenshots.

@kangmingtay kangmingtay requested a review from a team as a code owner July 3, 2024 19:42
@coveralls
Copy link

coveralls commented Jul 3, 2024

Pull Request Test Coverage Report for Build 9783906918

Details

  • 11 of 22 (50.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.01%) to 57.965%

Changes Missing Coverage Covered Lines Changed/Added Lines %
internal/api/user.go 7 18 38.89%
Totals Coverage Status
Change from base Build 9783578837: -0.01%
Covered Lines: 8802
Relevant Lines: 15185

💛 - Coveralls

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.

Nice!

Looks like we added a new if branch / clause to handle the autoconfirm case and we only do verification checks on the /verify route for email change when there's mailer autoconfirm enabled.

LGTM

@kangmingtay
Copy link
Member Author

@J0 thanks for reviewing, will add some tests before merging

@kangmingtay kangmingtay merged commit a518505 into master Jul 4, 2024
2 checks passed
@kangmingtay kangmingtay deleted the km/fix-update-user-email branch July 4, 2024 21:47
@coveralls
Copy link

coveralls commented Jul 4, 2024

Pull Request Test Coverage Report for Build 9800151798

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 16 of 22 (72.73%) changed or added relevant lines in 2 files are covered.
  • 4 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.03%) to 58.004%

Changes Missing Coverage Covered Lines Changed/Added Lines %
internal/api/user.go 12 18 66.67%
Files with Coverage Reduction New Missed Lines %
internal/api/user.go 4 65.03%
Totals Coverage Status
Change from base Build 9783578837: 0.03%
Covered Lines: 8809
Relevant Lines: 15187

💛 - Coveralls

kangmingtay pushed a commit that referenced this pull request Jul 4, 2024
🤖 I have created a release *beep* *boop*
---


##
[2.155.1](v2.155.0...v2.155.1)
(2024-07-04)


### Bug Fixes

* apply mailer autoconfirm config to update user email
([#1646](#1646))
([a518505](a518505))
* check for empty aud string
([#1649](#1649))
([42c1d45](42c1d45))
* return proper error if sms rate limit is exceeded
([#1647](#1647))
([3c8d765](3c8d765))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@bnjmnt4n
Copy link
Contributor

Hey @kangmingtay! This change had some unexpected effects for us (we have enabled GOTRUE_MAILER_AUTOCONFIRM=true). We were expecting a confirmation email when switching to a new email, whereas following this update the email was immediately updated. This isn't ideal behavior because it's easy for a user to lose all access immediately just by entering the wrong email. IMO, autoconfirm on creating an account with a new email is fine because even if the user has entered the wrong email, they have a new account, so there is little chance of data loss, and they can always follow up by updating their email. However, autoconfirm on updating a user's email doesn't allow the user any way to correct their mistake if entering the wrong email.

Would it be possible to either tweak this behavior? A possible change is to autoconfirm on update of user email if there is no existing email address (which handles #1619), but require confirmation if there is an existing email address. Otherwise, can we add a new configuration option to restore the original behavior?

@kangmingtay
Copy link
Member Author

@bnjmnt4n yeah we have a fix for this here - #1679

@bnjmnt4n
Copy link
Contributor

Thanks for the fast response @kangmingtay! 🔥

uxodb pushed a commit to uxodb/auth that referenced this pull request Nov 13, 2024
)

## What kind of change does this PR introduce?
* `GOTRUE_MAILER_AUTOCONFIRM` setting should be respected in the update
email flow via `PUT /user`

## What is the current behavior?
* When `GOTRUE_MAILER_AUTOCONFIRM=true`, updating a user's email still
sends an email and requires user confirmation
* Difficult for anonymous users to upgrade to a permanent user
seamlessly without requiring email confirmation
* Fixes supabase#1619 

## What is the new behavior?
* When `GOTRUE_MAILER_AUTOCONFIRM=true`, updating a user's email will
not require email confirmation.

## Additional context

Add any other context or screenshots.
uxodb pushed a commit to uxodb/auth that referenced this pull request Nov 13, 2024
🤖 I have created a release *beep* *boop*
---


##
[2.155.1](supabase/auth@v2.155.0...v2.155.1)
(2024-07-04)


### Bug Fixes

* apply mailer autoconfirm config to update user email
([supabase#1646](supabase#1646))
([a518505](supabase@a518505))
* check for empty aud string
([supabase#1649](supabase#1649))
([42c1d45](supabase@42c1d45))
* return proper error if sms rate limit is exceeded
([supabase#1647](supabase#1647))
([3c8d765](supabase@3c8d765))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
LashaJini pushed a commit to LashaJini/auth that referenced this pull request Nov 13, 2024
)

## What kind of change does this PR introduce?
* `GOTRUE_MAILER_AUTOCONFIRM` setting should be respected in the update
email flow via `PUT /user`

## What is the current behavior?
* When `GOTRUE_MAILER_AUTOCONFIRM=true`, updating a user's email still
sends an email and requires user confirmation
* Difficult for anonymous users to upgrade to a permanent user
seamlessly without requiring email confirmation
* Fixes supabase#1619 

## What is the new behavior?
* When `GOTRUE_MAILER_AUTOCONFIRM=true`, updating a user's email will
not require email confirmation.

## Additional context

Add any other context or screenshots.
LashaJini pushed a commit to LashaJini/auth that referenced this pull request Nov 13, 2024
🤖 I have created a release *beep* *boop*
---


##
[2.155.1](supabase/auth@v2.155.0...v2.155.1)
(2024-07-04)


### Bug Fixes

* apply mailer autoconfirm config to update user email
([supabase#1646](supabase#1646))
([a518505](supabase@a518505))
* check for empty aud string
([supabase#1649](supabase#1649))
([42c1d45](supabase@42c1d45))
* return proper error if sms rate limit is exceeded
([supabase#1647](supabase#1647))
([3c8d765](supabase@3c8d765))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants