Skip to content

fix: move email & sms send out of the POST /user transaction #2022

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
+91 −60
Open

fix: move email & sms send out of the POST /user transaction #2022

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
01c04ef
fix: move email & sms send out of the `POST /user` transaction
hf May 14, 2025
2e61228
restructure more so updates and inserts are atomic
hf May 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 39 additions & 30 deletions internal/api/mail.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -303,7 +303,7 @@ func (a *API) sendConfirmation(r *http.Request, tx *storage.Connection, u *model
maxFrequency := config.SMTP.MaxFrequency
otpLength := config.Mailer.OtpLength

if err = validateSentWithinFrequencyLimit(u.ConfirmationSentAt, maxFrequency); err != nil {
if err = validateSentWithinFrequencyLimitEmail(u.ConfirmationSentAt, maxFrequency); err != nil {
return err
}
oldToken := u.ConfirmationToken
Expand Down Expand Up @@ -370,7 +370,7 @@ func (a *API) sendPasswordRecovery(r *http.Request, tx *storage.Connection, u *m
config := a.config
otpLength := config.Mailer.OtpLength

if err := validateSentWithinFrequencyLimit(u.RecoverySentAt, config.SMTP.MaxFrequency); err != nil {
if err := validateSentWithinFrequencyLimitEmail(u.RecoverySentAt, config.SMTP.MaxFrequency); err != nil {
return err
}

Expand Down Expand Up @@ -407,7 +407,7 @@ func (a *API) sendReauthenticationOtp(r *http.Request, tx *storage.Connection, u
maxFrequency := config.SMTP.MaxFrequency
otpLength := config.Mailer.OtpLength

if err := validateSentWithinFrequencyLimit(u.ReauthenticationSentAt, maxFrequency); err != nil {
if err := validateSentWithinFrequencyLimitEmail(u.ReauthenticationSentAt, maxFrequency); err != nil {
return err
}

Expand Down Expand Up @@ -445,7 +445,7 @@ func (a *API) sendMagicLink(r *http.Request, tx *storage.Connection, u *models.U

// since Magic Link is just a recovery with a different template and behaviour
// around new users we will reuse the recovery db timer to prevent potential abuse
if err := validateSentWithinFrequencyLimit(u.RecoverySentAt, config.SMTP.MaxFrequency); err != nil {
if err := validateSentWithinFrequencyLimitEmail(u.RecoverySentAt, config.SMTP.MaxFrequency); err != nil {
return err
}

Expand Down Expand Up @@ -478,11 +478,11 @@ func (a *API) sendMagicLink(r *http.Request, tx *storage.Connection, u *models.U
}

// sendEmailChange sends out an email change token to the new email.
func (a *API) sendEmailChange(r *http.Request, tx *storage.Connection, u *models.User, email string, flowType models.FlowType) error {
func (a *API) sendEmailChange(r *http.Request, db *storage.Connection, u *models.User, email string, flowType models.FlowType) error {
config := a.config
otpLength := config.Mailer.OtpLength

if err := validateSentWithinFrequencyLimit(u.EmailChangeSentAt, config.SMTP.MaxFrequency); err != nil {
if err := validateSentWithinFrequencyLimitEmail(u.EmailChangeSentAt, config.SMTP.MaxFrequency); err != nil {
return err
}

Expand All @@ -503,7 +503,7 @@ func (a *API) sendEmailChange(r *http.Request, tx *storage.Connection, u *models
u.EmailChangeConfirmStatus = zeroConfirmation
now := time.Now()

if err := a.sendEmail(r, tx, u, mail.EmailChangeVerification, otpCurrent, otpNew, u.EmailChangeTokenNew); err != nil {
if err := a.sendEmail(r, db, u, mail.EmailChangeVerification, otpCurrent, otpNew, u.EmailChangeTokenNew); err != nil {
if errors.Is(err, EmailRateLimitExceeded) {
return apierrors.NewTooManyRequestsError(apierrors.ErrorCodeOverEmailSendRateLimit, EmailRateLimitExceeded.Error())
} else if herr, ok := err.(*HTTPError); ok {
Expand All @@ -512,31 +512,33 @@ func (a *API) sendEmailChange(r *http.Request, tx *storage.Connection, u *models
return apierrors.NewInternalServerError("Error sending email change email").WithInternalError(err)
}

u.EmailChangeSentAt = &now
if err := tx.UpdateOnly(
u,
"email_change_token_current",
"email_change_token_new",
"email_change",
"email_change_sent_at",
"email_change_confirm_status",
); err != nil {
return apierrors.NewInternalServerError("Error sending email change email").WithInternalError(errors.Wrap(err, "Database error updating user for email change"))
}
return db.Transaction(func(tx *storage.Connection) error {
u.EmailChangeSentAt = &now
if err := tx.UpdateOnly(
u,
"email_change_token_current",
"email_change_token_new",
"email_change",
"email_change_sent_at",
"email_change_confirm_status",
); err != nil {
return apierrors.NewInternalServerError("Error sending email change email").WithInternalError(errors.Wrap(err, "Database error updating user for email change"))
}

if u.EmailChangeTokenCurrent != "" {
if err := models.CreateOneTimeToken(tx, u.ID, u.GetEmail(), u.EmailChangeTokenCurrent, models.EmailChangeTokenCurrent); err != nil {
return apierrors.NewInternalServerError("Error sending email change email").WithInternalError(errors.Wrap(err, "Database error creating email change token current"))
if u.EmailChangeTokenCurrent != "" {
if err := models.CreateOneTimeToken(tx, u.ID, u.GetEmail(), u.EmailChangeTokenCurrent, models.EmailChangeTokenCurrent); err != nil {
return apierrors.NewInternalServerError("Error sending email change email").WithInternalError(errors.Wrap(err, "Database error creating email change token current"))
}
}
}

if u.EmailChangeTokenNew != "" {
if err := models.CreateOneTimeToken(tx, u.ID, u.EmailChange, u.EmailChangeTokenNew, models.EmailChangeTokenNew); err != nil {
return apierrors.NewInternalServerError("Error sending email change email").WithInternalError(errors.Wrap(err, "Database error creating email change token new"))
if u.EmailChangeTokenNew != "" {
if err := models.CreateOneTimeToken(tx, u.ID, u.EmailChange, u.EmailChangeTokenNew, models.EmailChangeTokenNew); err != nil {
return apierrors.NewInternalServerError("Error sending email change email").WithInternalError(errors.Wrap(err, "Database error creating email change token new"))
}
}
}

return nil
return nil
})
}

func (a *API) validateEmail(email string) (string, error) {
Expand All @@ -553,13 +555,20 @@ func (a *API) validateEmail(email string) (string, error) {
return strings.ToLower(email), nil
}

func validateSentWithinFrequencyLimit(sentAt *time.Time, frequency time.Duration) error {
func validateSentWithinFrequencyLimitEmail(sentAt *time.Time, frequency time.Duration) error {
if sentAt != nil && sentAt.Add(frequency).After(time.Now()) {
return apierrors.NewTooManyRequestsError(apierrors.ErrorCodeOverEmailSendRateLimit, generateFrequencyLimitErrorMessage(sentAt, frequency))
}
return nil
}

func validateSentWithinFrequencyLimitSMS(sentAt *time.Time, frequency time.Duration) error {
if sentAt != nil && sentAt.Add(frequency).After(time.Now()) {
return apierrors.NewTooManyRequestsError(apierrors.ErrorCodeOverSMSSendRateLimit, generateFrequencyLimitErrorMessage(sentAt, frequency))
}
return nil
}

var emailLabelPattern = regexp.MustCompile("[+][^@]+@")

func (a *API) checkEmailAddressAuthorization(email string) bool {
Expand All @@ -579,7 +588,7 @@ func (a *API) checkEmailAddressAuthorization(email string) bool {
return true
}

func (a *API) sendEmail(r *http.Request, tx *storage.Connection, u *models.User, emailActionType, otp, otpNew, tokenHashWithPrefix string) error {
func (a *API) sendEmail(r *http.Request, db *storage.Connection, u *models.User, emailActionType, otp, otpNew, tokenHashWithPrefix string) error {
ctx := r.Context()
config := a.config
referrerURL := utilities.GetReferrer(r, config)
Expand Down Expand Up @@ -650,7 +659,7 @@ func (a *API) sendEmail(r *http.Request, tx *storage.Connection, u *models.User,
EmailData: emailData,
}
output := v0hooks.SendEmailOutput{}
return a.hooksMgr.InvokeHook(tx, r, &input, &output)
return a.hooksMgr.InvokeHook(db, r, &input, &output)
}

mr := a.Mailer()
Expand Down
50 changes: 26 additions & 24 deletions internal/api/phone.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ func formatPhoneNumber(phone string) string {
}

// sendPhoneConfirmation sends an otp to the user's phone number
func (a *API) sendPhoneConfirmation(r *http.Request, tx *storage.Connection, user *models.User, phone, otpType string, channel string) (string, error) {
func (a *API) sendPhoneConfirmation(r *http.Request, db *storage.Connection, user *models.User, phone, otpType string, channel string) (string, error) {
config := a.config

var token *string
Expand All @@ -70,8 +70,8 @@ func (a *API) sendPhoneConfirmation(r *http.Request, tx *storage.Connection, use

// intentionally keeping this before the test OTP, so that the behavior
// of regular and test OTPs is similar
if sentAt != nil && !sentAt.Add(config.Sms.MaxFrequency).Before(time.Now()) {
return "", apierrors.NewTooManyRequestsError(apierrors.ErrorCodeOverSMSSendRateLimit, generateFrequencyLimitErrorMessage(sentAt, config.Sms.MaxFrequency))
if err := validateSentWithinFrequencyLimitSMS(sentAt, config.Sms.MaxFrequency); err != nil {
return "", err
}

now := time.Now()
Expand Down Expand Up @@ -102,7 +102,7 @@ func (a *API) sendPhoneConfirmation(r *http.Request, tx *storage.Connection, use
},
}
output := v0hooks.SendSMSOutput{}
err := a.hooksMgr.InvokeHook(tx, r, &input, &output)
err := a.hooksMgr.InvokeHook(db, r, &input, &output)
if err != nil {
return "", err
}
Expand Down Expand Up @@ -133,29 +133,31 @@ func (a *API) sendPhoneConfirmation(r *http.Request, tx *storage.Connection, use
user.ReauthenticationSentAt = &now
}

if err := tx.UpdateOnly(user, includeFields...); err != nil {
return messageID, errors.Wrap(err, "Database error updating user for phone")
}

var ottErr error
switch otpType {
case phoneConfirmationOtp:
if err := models.CreateOneTimeToken(tx, user.ID, user.GetPhone(), user.ConfirmationToken, models.ConfirmationToken); err != nil {
ottErr = errors.Wrap(err, "Database error creating confirmation token for phone")
return messageID, db.Transaction(func(tx *storage.Connection) error {
if err := tx.UpdateOnly(user, includeFields...); err != nil {
return errors.Wrap(err, "Database error updating user for phone")
}
case phoneChangeVerification:
if err := models.CreateOneTimeToken(tx, user.ID, user.PhoneChange, user.PhoneChangeToken, models.PhoneChangeToken); err != nil {
ottErr = errors.Wrap(err, "Database error creating phone change token")

var ottErr error
switch otpType {
case phoneConfirmationOtp:
if err := models.CreateOneTimeToken(tx, user.ID, user.GetPhone(), user.ConfirmationToken, models.ConfirmationToken); err != nil {
ottErr = errors.Wrap(err, "Database error creating confirmation token for phone")
}
case phoneChangeVerification:
if err := models.CreateOneTimeToken(tx, user.ID, user.PhoneChange, user.PhoneChangeToken, models.PhoneChangeToken); err != nil {
ottErr = errors.Wrap(err, "Database error creating phone change token")
}
case phoneReauthenticationOtp:
if err := models.CreateOneTimeToken(tx, user.ID, user.GetPhone(), user.ReauthenticationToken, models.ReauthenticationToken); err != nil {
ottErr = errors.Wrap(err, "Database error creating reauthentication token for phone")
}
}
case phoneReauthenticationOtp:
if err := models.CreateOneTimeToken(tx, user.ID, user.GetPhone(), user.ReauthenticationToken, models.ReauthenticationToken); err != nil {
ottErr = errors.Wrap(err, "Database error creating reauthentication token for phone")
if ottErr != nil {
return apierrors.NewInternalServerError("error creating one time token").WithInternalError(ottErr)
}
}
if ottErr != nil {
return messageID, apierrors.NewInternalServerError("error creating one time token").WithInternalError(ottErr)
}
return messageID, nil
return nil
})
}

func generateSMSFromTemplate(SMSTemplate *template.Template, otp string) (string, error) {
Expand Down
32 changes: 26 additions & 6 deletions internal/api/user.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,9 @@ func (a *API) UserUpdate(w http.ResponseWriter, r *http.Request) error {
}
}

var sendEmailChange, sendPhoneConfirmation bool
flowType := getFlowFromChallenge(params.CodeChallenge)

err := db.Transaction(func(tx *storage.Connection) error {
var terr error
if params.Password != nil {
Expand Down Expand Up @@ -223,17 +226,18 @@ func (a *API) UserUpdate(w http.ResponseWriter, r *http.Request) error {
}

} else {
flowType := getFlowFromChallenge(params.CodeChallenge)
if isPKCEFlow(flowType) {
_, terr := generateFlowState(tx, models.EmailChange.String(), models.EmailChange, params.CodeChallengeMethod, params.CodeChallenge, &user.ID)
if terr != nil {
return terr
}

}
if terr = a.sendEmailChange(r, tx, user, params.Email, flowType); terr != nil {
return terr

if err := validateSentWithinFrequencyLimitEmail(user.EmailChangeSentAt, config.SMTP.MaxFrequency); err != nil {
return err
}

sendEmailChange = true
}
}

Expand All @@ -247,9 +251,11 @@ func (a *API) UserUpdate(w http.ResponseWriter, r *http.Request) error {
return terr
}
} else {
if _, terr := a.sendPhoneConfirmation(r, tx, user, params.Phone, phoneChangeVerification, params.Channel); terr != nil {
return terr
if err := validateSentWithinFrequencyLimitSMS(user.ReauthenticationSentAt, config.SMTP.MaxFrequency); err != nil {
return err
}

sendPhoneConfirmation = true
}
}

Expand All @@ -263,5 +269,19 @@ func (a *API) UserUpdate(w http.ResponseWriter, r *http.Request) error {
return err
}

if sendEmailChange {
// email sending should not hold a database transaction open as latency incurred by SMTP or HTTP hooks can exhaust the database pool
if err := a.sendEmailChange(r, db, user, params.Email, flowType); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Have we thought through the consequences of not rolling back the previous values in the transaction when sendEmailChange fails at every failure point?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In general I think it should be fine. Yea the email send will fail, but the request will also fail and user can ask for re-send when email sending / SMS sending is back online.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe we need to do the rate limit prior the transaction as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link

Choose a reason for hiding this comment

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

Agree with the concerns that we lose the atomicity: if the user updates multiple attributes in one API call(e.g raw_user_meta_data and email) and sendEmail fails, the user will have the partial updates(raw_user_meta_data will be updated even if the call fails). However I don't have the number how many users make multiple changes in one request, so could be OK to ignore.

As far as I check the api.UserUpdate, all the updates are idempotent (except logging audit entries) so retrying a request in case of sending email/sms fails should be fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I see. Changed up the code a bit more.

Copy link

Choose a reason for hiding this comment

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

That wasn't exactly what I was describing, but it's nice we added the transaction handling to the sendEmailChange function 👍

I was highlighting the (possible) confusion for the users, when they send the payload below to api.UpdateUser and the sendEmail function fails:

{ 
  "email": "[email protected]",
  "raw_user_meta_data": {"test":"test"}
}

In that case, users would get a non-200 response from API but the database would be updated with the new raw_user_meta_data values.

What about returning success even if sending email/sms is failing? IMHO decoupling notification logic from the request handling would be way to go and in the long run, we can also process the notifications completely async.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it may be worth evaluating some other options, such as:

  • Writing a task to a queue in the db instead of sending the mail. It would be simply to spin up some workers to pull tasks at startup.
  • Using hookafter in a more general way, such as taskafter. This would be clean, simple and keep the operation within the request.

Copy link
Contributor

Choose a reason for hiding this comment

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

What about returning success even if sending email/sms is failing? IMHO decoupling notification logic from the request handling would be way to go and in the long run, we can also process the notifications completely async.

I agree with you here, me and Stojan discussed this a little over the weekend and I just have a lot of concerns with changing the behavior of so many flows with the number of writes we do in this tx. I feel there is a subtle security or high impact bug in here somewhere.

return err
}
}

if sendPhoneConfirmation {
// SMS sending should not hold a database transaction open as latency incurred by SMTP or HTTP hooks can exhaust the database pool
if _, err := a.sendPhoneConfirmation(r, db, user, params.Phone, phoneChangeVerification, params.Channel); err != nil {
return err
}
}

return sendJSON(w, http.StatusOK, user)
}
Loading