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

[hf]

Keeping this inside the transaction can exhaust the database pool grinding full Auth to a halt, especially if the SMTP server or hook takes more than a few milliseconds to complete.

[coveralls]

Pull Request Test Coverage Report for Build 15113577233

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.

• For more information on this, see Tracking coverage changes with pull request builds.
• To avoid this issue with future PRs, see these Recommended CI Configurations.
• For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

• 57 of 79 (72.15%) changed or added relevant lines in 3 files are covered.
• 7 unchanged lines in 2 files lost coverage.
• Overall coverage decreased (-0.01%) to 69.627%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
internal/api/user.go	15	19	78.95%
internal/api/mail.go	27	36	75.0%
internal/api/phone.go	15	24	62.5%

Files with Coverage Reduction	New Missed Lines	%
internal/api/phone.go	3	83.19%
internal/api/user.go	4	65.8%

Totals
Change from base Build 15084415331:	-0.01%
Covered Lines:	10962
Relevant Lines:	15744

---

💛 - Coveralls

[cstockton]

Apologies if you already considered this, but I think it's worth noting this is a really big behavior change and I worry without extensive tests there could be potential bugs or potential security considerations we are missing. I think it's worth thinking about what is done and rolled back in the current transaction.

For example the transaction in UserUpdate will:

• UpdatePassword
• UpdateUserMetadata
• UpdateAppMetadata
• Conditionally:
  • Call emailChangeVerify
    • Calls ClearOneTimeTokenForUser
    • Updates confirmation status
    • Update identities and perform email confirmation

Knowing that I think it's worth evaluating what sendEmailChange does and how it fails. For example it will call validateSentWithinFrequencyLimit, have we carefully considered the consequences of leaving the transaction in UserUpdate committed without performing all the write operations within sendEmailChange.

[cstockton]

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

[hf]

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.

[hf]

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

[hf]

Done

[cemalkilic]

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.

[hf]

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

[cemalkilic]

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.

[cstockton]

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.

[cstockton]

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.