Merge pull request supabase#132 from supabase/pr-email-change

Fix: email change improvements

api/errors.go

@@ -72,6 +72,10 @@ func notFoundError(fmtString string, args ...interface{}) *HTTPError {
 	return httpError(http.StatusNotFound, fmtString, args...)
 }
 
+func acceptedTokenError(fmtString string, args ...interface{}) *HTTPError {
+	return httpError(http.StatusAccepted, fmtString, args...)
+}
+
 func expiredTokenError(fmtString string, args ...interface{}) *HTTPError {
 	return httpError(http.StatusGone, fmtString, args...)
 }

api/hooks.go

@@ -31,6 +31,7 @@ const (
 	gotrueIssuer        = "gotrue"
 	ValidateEvent       = "validate"
 	SignupEvent         = "signup"
+	EmailChangeEvent    = "email_change"
 	LoginEvent          = "login"
 )
 

api/mail.go

@@ -230,19 +230,15 @@ func (a *API) sendMagicLink(tx *storage.Connection, u *models.User, mailer maile
 }
 
 func (a *API) sendEmailChange(tx *storage.Connection, u *models.User, mailer mailer.Mailer, email string, referrerURL string) error {
-	oldToken := u.EmailChangeToken
-	oldEmail := u.EmailChange
-	u.EmailChangeToken = crypto.SecureToken()
+	u.EmailChangeTokenCurrent, u.EmailChangeTokenNew = crypto.SecureToken(), crypto.SecureToken()
 	u.EmailChange = email
 	now := time.Now()
 	if err := mailer.EmailChangeMail(u, referrerURL); err != nil {
-		u.EmailChangeToken = oldToken
-		u.EmailChange = oldEmail
 		return err
 	}
 
 	u.EmailChangeSentAt = &now
-	return errors.Wrap(tx.UpdateOnly(u, "email_change_token", "email_change", "email_change_sent_at"), "Database error updating user for email change")
+	return errors.Wrap(tx.UpdateOnly(u, "email_change_token_current", "email_change_token_new", "email_change", "email_change_sent_at"), "Database error updating user for email change")
 }
 
 func (a *API) validateEmail(ctx context.Context, email string) error {

api/user.go

@@ -12,12 +12,11 @@ import (
 
 // UserUpdateParams parameters for updating a user
 type UserUpdateParams struct {
-	Email            string                 `json:"email"`
-	Password         string                 `json:"password"`
-	EmailChangeToken string                 `json:"email_change_token"`
-	Data             map[string]interface{} `json:"data"`
-	AppData          map[string]interface{} `json:"app_metadata,omitempty"`
-	Phone            string                 `json:"phone"`
+	Email    string                 `json:"email"`
+	Password string                 `json:"password"`
+	Data     map[string]interface{} `json:"data"`
+	AppData  map[string]interface{} `json:"app_metadata,omitempty"`
+	Phone    string                 `json:"phone"`
 }
 
 // UserGet returns a user
@@ -107,17 +106,7 @@ func (a *API) UserUpdate(w http.ResponseWriter, r *http.Request) error {
 			}
 		}
 
-		if params.EmailChangeToken != "" {
-			log.Debugf("Got change token %v", params.EmailChangeToken)
-
-			if params.EmailChangeToken != user.EmailChangeToken {
-				return unauthorizedError("Email Change Token didn't match token on file")
-			}
-
-			if terr = user.ConfirmEmailChange(tx); terr != nil {
-				return internalServerError("Error updating user").WithInternalError(terr)
-			}
-		} else if params.Email != "" && params.Email != user.GetEmail() {
+		if params.Email != "" && params.Email != user.GetEmail() {
 			if terr = a.validateEmail(ctx, params.Email); terr != nil {
 				return terr
 			}

api/verify.go

@@ -20,11 +20,12 @@ var (
 )
 
 const (
-	signupVerification    = "signup"
-	recoveryVerification  = "recovery"
-	inviteVerification    = "invite"
-	magicLinkVerification = "magiclink"
-	smsVerification       = "sms"
+	signupVerification      = "signup"
+	recoveryVerification    = "recovery"
+	inviteVerification      = "invite"
+	magicLinkVerification   = "magiclink"
+	emailChangeVerification = "email_change"
+	smsVerification         = "sms"
 )
 
 // VerifyParams are the parameters the Verify endpoint accepts
@@ -79,6 +80,8 @@ func (a *API) Verify(w http.ResponseWriter, r *http.Request) error {
 			user, terr = a.signupVerify(ctx, tx, params)
 		case recoveryVerification, magicLinkVerification:
 			user, terr = a.recoverVerify(ctx, tx, params)
+		case emailChangeVerification:
+			user, terr = a.emailChangeVerify(ctx, tx, params)
 		case smsVerification:
 			if params.Phone == "" {
 				return unprocessableEntityError("Sms Verification requires a phone number")
@@ -288,3 +291,69 @@ func (a *API) prepErrorRedirectURL(err *HTTPError, r *http.Request, rurl string)
 	q.Set("error_description", err.Message)
 	return rurl + "#" + q.Encode()
 }
+
+func (a *API) emailChangeVerify(ctx context.Context, conn *storage.Connection, params *VerifyParams) (*models.User, error) {
+	instanceID := getInstanceID(ctx)
+	config := a.getConfig(ctx)
+	user, err := models.FindUserByEmailChangeToken(conn, params.Token)
+	if err != nil {
+		if models.IsNotFoundError(err) {
+			return nil, notFoundError(err.Error()).WithInternalError(redirectWithQueryError)
+		}
+		return nil, internalServerError("Database error finding user").WithInternalError(err)
+	}
+
+	nextDay := user.EmailChangeSentAt.Add(24 * time.Hour)
+	if user.EmailChangeSentAt != nil && time.Now().After(nextDay) {
+		err = a.db.Transaction(func(tx *storage.Connection) error {
+			user.EmailChangeConfirmStatus = 0
+			return tx.UpdateOnly(user, "email_change_confirm_status")
+		})
+		if err != nil {
+			return nil, err
+		}
+		return nil, expiredTokenError("Email change token expired").WithInternalError(redirectWithQueryError)
+	}
+
+	if user.EmailChangeConfirmStatus < 1 {
+		err = a.db.Transaction(func(tx *storage.Connection) error {
+			user.EmailChangeConfirmStatus += 1
+			if params.Token == user.EmailChangeTokenCurrent {
+				user.EmailChangeTokenCurrent = ""
+			} else if params.Token == user.EmailChangeTokenNew {
+				user.EmailChangeTokenNew = ""
+			}
+			if terr := tx.UpdateOnly(user, "email_change_confirm_status", "email_change_token_current", "email_change_token_new"); terr != nil {
+				return terr
+			}
+			return nil
+		})
+		if err != nil {
+			return nil, err
+		}
+		return nil, acceptedTokenError("Email change request accepted")
+	}
+
+	err = a.db.Transaction(func(tx *storage.Connection) error {
+		var terr error
+
+		if terr = models.NewAuditLogEntry(tx, instanceID, user, models.UserModifiedAction, nil); terr != nil {
+			return terr
+		}
+
+		if terr = triggerEventHooks(ctx, tx, EmailChangeEvent, user, instanceID, config); terr != nil {
+			return terr
+		}
+
+		if terr = user.ConfirmEmailChange(tx); terr != nil {
+			return internalServerError("Error confirm email").WithInternalError(terr)
+		}
+
+		return nil
+	})
+	if err != nil {
+		return nil, err
+	}
+
+	return user, nil
+}

mailer/template.go

@@ -111,31 +111,58 @@ func (m *TemplateMailer) ConfirmationMail(user *models.User, referrerURL string)
 
 // EmailChangeMail sends an email change confirmation mail to a user
 func (m *TemplateMailer) EmailChangeMail(user *models.User, referrerURL string) error {
+	globalConfig, err := conf.LoadGlobal(configFile)
+	if err != nil {
+		return err
+	}
+
 	redirectParam := ""
 	if len(referrerURL) > 0 {
 		redirectParam = "&redirect_to=" + referrerURL
 	}
 
-	url, err := getSiteURL(referrerURL, m.Config.SiteURL, m.Config.Mailer.URLPaths.EmailChange, "email_change_token="+user.EmailChangeToken+"&type=email_change"+redirectParam)
-	if err != nil {
-		return err
+	errors := make(chan error)
+	tokens := map[string]string{
+		user.GetEmail():  user.EmailChangeTokenCurrent,
+		user.EmailChange: user.EmailChangeTokenNew,
 	}
-	data := map[string]interface{}{
-		"SiteURL":         m.Config.SiteURL,
-		"ConfirmationURL": url,
-		"Email":           user.GetEmail(),
-		"NewEmail":        user.EmailChange,
-		"Token":           user.EmailChangeToken,
-		"Data":            user.UserMetaData,
+	for email, token := range tokens {
+		url, err := getSiteURL(
+			referrerURL,
+			globalConfig.API.ExternalURL,
+			m.Config.Mailer.URLPaths.EmailChange,
+			"token="+token+"&type=email_change"+redirectParam,
+		)
+		if err != nil {
+			return err
+		}
+		go func(e, t string) {
+			data := map[string]interface{}{
+				"SiteURL":         m.Config.SiteURL,
+				"ConfirmationURL": url,
+				"Email":           user.GetEmail(),
+				"NewEmail":        user.EmailChange,
+				"Token":           t,
+				"Data":            user.UserMetaData,
+			}
+			errors <- m.Mailer.Mail(
+				e,
+				string(withDefault(m.Config.Mailer.Subjects.EmailChange, "Confirm Email Change")),
+				m.Config.Mailer.Templates.EmailChange,
+				defaultEmailChangeMail,
+				data,
+			)
+		}(email, token)
 	}
 
-	return m.Mailer.Mail(
-		user.EmailChange,
-		string(withDefault(m.Config.Mailer.Subjects.EmailChange, "Confirm Email Change")),
-		m.Config.Mailer.Templates.EmailChange,
-		defaultEmailChangeMail,
-		data,
-	)
+	for i := 0; i < len(tokens); i++ {
+		e := <-errors
+		if e != nil {
+			return e
+		}
+	}
+
+	return nil
 }
 
 // RecoveryMail sends a password recovery mail

migrations/20210730183235_add_email_change_confirmed.up.sql

@@ -0,0 +1,15 @@
+-- adds email_change_confirmed
+
+ALTER TABLE auth.users
+ADD COLUMN IF NOT EXISTS email_change_token_current varchar(255) null DEFAULT '', 
+ADD COLUMN IF NOT EXISTS email_change_confirm_status smallint DEFAULT 0 CHECK (email_change_confirm_status >= 0 AND email_change_confirm_status <= 2);
+
+DO $$
+BEGIN
+  IF NOT EXISTS(SELECT *
+    FROM information_schema.columns
+    WHERE table_schema = 'auth' and table_name='users' and column_name='email_change_token_new')
+  THEN
+      ALTER TABLE "auth"."users" RENAME COLUMN "email_change_token" TO "email_change_token_new";
+  END IF;
+END $$;

models/user.go

@@ -40,9 +40,11 @@ type User struct {
 	RecoveryToken  string     `json:"-" db:"recovery_token"`
 	RecoverySentAt *time.Time `json:"recovery_sent_at,omitempty" db:"recovery_sent_at"`
 
-	EmailChangeToken  string     `json:"-" db:"email_change_token"`
-	EmailChange       string     `json:"new_email,omitempty" db:"email_change"`
-	EmailChangeSentAt *time.Time `json:"email_change_sent_at,omitempty" db:"email_change_sent_at"`
+	EmailChangeTokenCurrent  string     `json:"-" db:"email_change_token_current"`
+	EmailChangeTokenNew      string     `json:"-" db:"email_change_token_new"`
+	EmailChange              string     `json:"new_email,omitempty" db:"email_change"`
+	EmailChangeSentAt        *time.Time `json:"email_change_sent_at,omitempty" db:"email_change_sent_at"`
+	EmailChangeConfirmStatus int        `json:"email_change_confirm_status" db:"email_change_confirm_status"`
 
 	PhoneChangeToken  string     `json:"-" db:"phone_change_token"`
 	PhoneChange       string     `json:"new_phone,omitempty" db:"phone_change"`
@@ -269,8 +271,17 @@ func (u *User) UpdateLastSignInAt(tx *storage.Connection) error {
 func (u *User) ConfirmEmailChange(tx *storage.Connection) error {
 	u.Email = storage.NullString(u.EmailChange)
 	u.EmailChange = ""
-	u.EmailChangeToken = ""
-	return tx.UpdateOnly(u, "email", "email_change", "email_change_token")
+	u.EmailChangeTokenCurrent = ""
+	u.EmailChangeTokenNew = ""
+	u.EmailChangeConfirmStatus = 0
+	return tx.UpdateOnly(
+		u,
+		"email",
+		"email_change",
+		"email_change_token_current",
+		"email_change_token_new",
+		"email_change_confirm_status",
+	)
 }
 
 // ConfirmPhoneChange confirms the change of phone for a user
@@ -341,6 +352,11 @@ func FindUserByRecoveryToken(tx *storage.Connection, token string) (*User, error
 	return findUser(tx, "recovery_token = ?", token)
 }
 
+// FindUserByEmailChangeToken finds a user with the matching email change token.
+func FindUserByEmailChangeToken(tx *storage.Connection, token string) (*User, error) {
+	return findUser(tx, "email_change_token_current = ? or email_change_token_new = ?", token, token)
+}
+
 // FindUserWithRefreshToken finds a user from the provided refresh token.
 func FindUserWithRefreshToken(tx *storage.Connection, token string) (*User, *RefreshToken, error) {
 	refreshToken := &RefreshToken{}