Closed mugwhump closed 1 month ago
nodeMailer will throw an exception which doesn't seem to be caught properly.
This definitely needs to be fixed.
I'm not so sure about returning an internal Server error in case the email isn't sent out correctly. I'd say that rather not send that information to the client. But I'll look into it.
Couch-Auth could emit an event and then you could retry sending the email later in case this was a temporary error.
Just for now could I submit a PR wrapping the problematic line (user.ts:418) in a try/catch as a temporary fix?
Background
If createUser() in user.ts is called (in my case via the /register endpoint), and
config.local.sendConfirmEmail
is true, and your SMTP credentials are messed up (due to incorrect credentials, some other configuration error, creds being revoked, etc), nodeMailer will throw an exception which doesn't seem to be caught properly.Actual Behavior
The promise returned by createUser() actually resolves successfully, the user is added to sl-users, and the /register route does call this line (routes.ts:153):
res.status(200).json({ success: 'Request processed.' });
But after this is executed, the async call to send the confirmation email runs, at insertNewUserDocument() (user.ts:475). My console prints this errorand express hangs since it's not handled. It doesn't matter if you set
config.security.forwardErrors
.Expected behavior
The problematic code in the return statement of createUser():
It's "fixed", as in it doesn't hang express, if I wrap
const finalUser = await this.insertNewUserDocument(newUser, req);
in try/catch. But the async function inside the promise constructor looks like an instance of the Promise constructor anti-pattern. So you may want to rework that part.I'm not sure if you want createUser() to reject when there's a problem sending the confirmation email; after all, the user WAS created, it's just that the confirmation email didn't go out. Basically createUser() in this case is a two-part non-atomic operation, so do you:
2 is more atomic and less work for me. But people who don't have
config.local.requireEmailConfirm
might prefer approach 1.