Skip to content
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

Unhandled SMTP error when sending confirmation email in createUser() #71

Open
mugwhump opened this issue Mar 25, 2023 · 2 comments · May be fixed by #84
Open

Unhandled SMTP error when sending confirmation email in createUser() #71

mugwhump opened this issue Mar 25, 2023 · 2 comments · May be fixed by #84

Comments

@mugwhump
Copy link
Contributor

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 error

/usr/src/app/node_modules/@perfood/couch-auth/node_modules/nodemailer/lib/smtp-connection/index.js:787
            err = new Error(message);
                  ^

Error: Invalid login: 535 Authentication failed
    at SMTPConnection._formatError (/usr/src/app/node_modules/@perfood/couch-auth/node_modules/nodemailer/lib/smtp-connection/index.js:787:19)
    at SMTPConnection._actionAUTHComplete (/usr/src/app/node_modules/@perfood/couch-auth/node_modules/nodemailer/lib/smtp-connection/index.js:1539:34)
    at SMTPConnection.<anonymous> (/usr/src/app/node_modules/@perfood/couch-auth/node_modules/nodemailer/lib/smtp-connection/index.js:543:26)
    at SMTPConnection._processResponse (/usr/src/app/node_modules/@perfood/couch-auth/node_modules/nodemailer/lib/smtp-connection/index.js:950:20)
    at SMTPConnection._onData (/usr/src/app/node_modules/@perfood/couch-auth/node_modules/nodemailer/lib/smtp-connection/index.js:752:14)
    at TLSSocket.SMTPConnection._onSocketData (/usr/src/app/node_modules/@perfood/couch-auth/node_modules/nodemailer/lib/smtp-connection/index.js:191:44)
    at TLSSocket.emit (node:events:513:28)
    at addChunk (node:internal/streams/readable:315:12)
    at readableAddChunk (node:internal/streams/readable:289:9)
    at TLSSocket.Readable.push (node:internal/streams/readable:228:10) {
  code: 'EAUTH',
  response: '535 Authentication failed',
  responseCode: 535,
  command: 'AUTH PLAIN'
}

and 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():

    return new Promise(async (resolve, reject) => {
      newUser = await this.prepareNewUser(newUser);
      if (hasError || !this.config.security.loginOnRegistration) {
        resolve(hasError ? undefined : (newUser as SlUserDoc));
      }
      if (!hasError) {
        const finalUser = await this.insertNewUserDocument(newUser, req);
        this.emitter.emit('signup', finalUser, 'local');
        if (this.config.security.loginOnRegistration) {
          resolve(finalUser);
        }
      }
    });

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:

  1. Resolve createUser() if the user is created successfully, even if the confirmation email doesn't send? In this case I would prefer a different status be returned from /register, so that I can tell my users there was an issue sending the email. Then I'd either delete their user doc so they can try again, or add some page they can visit to re-send the confirmation email.
  2. Reject createUser() if the email doesn't send? In this case the newly-added doc should be deleted from sl-users.

2 is more atomic and less work for me. But people who don't have config.local.requireEmailConfirm might prefer approach 1.

@fynnlyte
Copy link
Collaborator

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.

@mugwhump
Copy link
Contributor Author

mugwhump commented Jun 2, 2023

Just for now could I submit a PR wrapping the problematic line (user.ts:418) in a try/catch as a temporary fix?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants