-
Notifications
You must be signed in to change notification settings - Fork 384
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: set email_confirmed_at
to null when autoconfirm is on
#1661
base: master
Are you sure you want to change the base?
Conversation
@@ -27,7 +27,7 @@ func (a *API) Reauthenticate(w http.ResponseWriter, r *http.Request) error { | |||
} | |||
|
|||
if email != "" { | |||
if !user.IsConfirmed() { | |||
if !user.IsConfirmed(config.Mailer.Autoconfirm) { |
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.
Maybe this should use false
always?
I agree in theory but in practice I worry that we might not be able to predict the full range of ways a developer might use the service. Contrived example: What if a developer turned on Auto-confirm for an initial sign up or pre-registration phase of trusted users and later turned it off after a private alpha to allow less trusted users to hold. They might then depend If not already considered, could we perhaps emit a warning for two weeks and then increment the minor version thereafter as per the backward compatibility advisory ? |
@hf thanks for looking into this and opening a PR to solve this problem. I have an interest in this PR going through. For context, I've posted about my use case in a discussion exploring an alternative solution to the problem: https://github.com/orgs/supabase/discussions/22363#discussioncomment-10033530 I'm leaving a link to the existing discussion here in case it's helpful in the conversation and deciding what the best solution is while minimizing impacts to users. |
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.
@hf @J0 discussed internally and decided that we'll need to collect more data first since this will change the expected behaviour of the MAILER_AUTOCONFIRM
config, which can result in a breaking change for many folks if they rely on the email_confirmed_at
column in RLS policies, since that would prevent an "auto-confirmed" email user to fail the check
A long-standing bug has been that when autoconfirm is on (should have been named "allow sign up without verifying email") it would set the
email_confirmed_at
column to the timestamp of the sign-up call. With this change it will set it to null instead.It is a backward compatible change because:
email_confirmed_at
are useless to you and you would not have relied on them anyway.