Proposal: Refactor email handling in accounts-password to separate package

I recently raised issue #4673 because I discovered there’s no way to control the flow concerning sending of password reset, account enrollment and email verification emails when using the accounts-password package.

That’s problematic in several ways:

  1. It opens up any Meteor server using accounts-password to potential DoS attacks, or at the very minimum permits abuse.

  2. No ability to customize the handling of emails. Maybe I want to turn it off altogether, or maybe I need to talk with a legacy message system.

@sashko said he would look at a PR that moves email handling out in a separate package, e.g. accounts-password-email so it would be easy to either go with the default implementation, or roll your own.

I’d be happy to start working on such a PR, but I’d like to hear any suggestions or input before I get down to the code.

Here’s what I’m currently thinking: All three kinds of emails make use of tokens that are sent back to the server and captured by handlers implemented in accounts-base, so let’s keep all the token handling intact to minimize the impact to existing packages.

Package accounts-password:

  • Add a new server-side option: Accounts.config.forbidForgotPassword - disables Accounts.forgotPassword().

  • Add server-side hook: Accounts.onHandleResetPasswordEmail(func) - registers callback that takes three arguments: token, email: String, and when: Date

  • Add server-side hook: Accounts.onHandleEnrollmentEmail(func) - registers callback that takes three arguments: token, email: String, and when: Date

  • Add server-side hook: Accounts.onHandleVerificationEmail(func) - registers callback that takes three arguments: token, email: String, and when: Date

  • Modify Accounts.sendResetPasswordEmail()

     Accounts.sendResetPasswordEmail = function (userId, email) {
       // Make sure the user exists, and email is one of their addresses.
       var user = Meteor.users.findOne(userId);
       if (!user)
         throw new Error("Can't find user");
       // pick the first email if we weren't passed an email.
       if (!email && user.emails && user.emails[0])
         email = user.emails[0].address;
       // make sure we have a valid email
       if (!email || !_.contains(_.pluck(user.emails || [], 'address'), email))
         throw new Error("No such email for user.");
    
       var token = Random.secret();
       var when = new Date();
       var tokenRecord = {
         token: token,
         email: email,
         when: when
       };
     
       // REPLACE REST OF METHOD WITH THIS
       Accounts._onSendResetPasswordEmailHook(email, when, token)
     }
    
  • Modify Accounts.sendEnrollmentEmail() and Accounts.sendVerificationEmail() similarly.

  • Initialize Accounts with default hooks that simply logs the action to the console and instructs the reader to either add accounts-password-email or roll own implementation.

accounts-password-email:

  • Override dummy handlers in accounts-password with the “bottom-half” of respectively Accounts.sendResetPasswordEmail, Accounts.sendEnrollmentEmail(), Accounts.sendVerificationEmail() – i.e. the portions that writes the token to the user’s account record, generates the email and sends it.

Overall, I believe this approach is transparent to existing packages and implementations (that don’t already mess with Accounts on the server-side…)

Thoughts?

3 Likes

Shouldn’t the default behavior be kept without the user having to add an additional package (“accounts-password-email”)? The idea here is to be able to override the default behavior, and in the spirit of providing sensible defaults and having a nice upgrade path, it should not be required to add an additional package, just to have it work the way it used to before. That’d be just bad UX (or at least not ideal, Meteor-level UX), from my perspective!

Other than that – good thing!

Makes sense to me. The idea to move the email handling to a separate package came from @sashko – I don’t have a personal preference yet, so I’m happy to go either way.

I mean, it’s possible that the package should be a dependency of accounts-password - I just thought introducing a package divide might make it easy to identify the correct abstraction level.

It’s not really necessary to do it that way.

agreed, maybe allow package creators to add a disableDefaultMailingBehaviour to the Meteor.settings object. this way it is also possible to add additional behaviour instead of just replacing it

I think the precedent we’re looking for here is in the way that the overriding of Accounts.onCreateUser works. If you don’t call it, then you get default functionality. If you do, you provide your own.
The only sad thing about that exact way of doing it is that you lose the default functionality if you just want to use that hook in order to get notified of the event of a new user having been created. (Which I personally would probably suggest fixing by adding a 2nd param to that method that indicates that you want to override the default instead of adding on to it.)
But I don’t think that in this case we will ever want to “add on” to the functionality, but rather only ever override. So that’d be a reasonable and Meteor-like way to do it.

It would be nice to be able to extend it for logging purposes. say I want to log something every time a password reset mail is sent.

When you can only override, you will have to write/copy the original function and add a simple logging statement. This increases complexity and boilerplate.

1 Like

Exactly. Sure, if we believe that extending is at all a reasonable desire, then there should be a way to accomplish it easily. So yes, then maybe allow that, your idea does sound reasonable indeed. My idea for that would be to have a 2nd param to the Accounts.on... method that, when true, would indicate overriding, and otherwise it would append behavior.
Handling the 2nd param the oppositie way, i.e. overriding by default, would make for a smoother upgrade path, so maybe initially it should be done like this, and flipped over in a later major version update or something. Just a few thoughts.

1 Like

There will always be people who think the other way is a better default. That is why I feel the smoother upgrade path would be the way to go.

1 Like

Another way to handle compatibility and upgrade concerns, would be to provide a package parallel or below accounts-password with support for all kinds of extensibility, i.e. more like a API for people who want to tinker with password handling without rolling their own from scratch.

I think part of the problem here is that accounts-password both tries to be a bit of an API where you can implement your own behavior, but then there are areas where you can’t. That means people who think they can add the package, yet customize the behavior eventually run into problems without an obvious strategy for proceeding. Worst case, they simply modify the core package, then have to deal with reconciling updates in Meteor releases.

So, maybe another solution would be to keep accounts-password as-is on the surface, and introduce accounts-password-api which has all the pieces but doesn’t do anything advanced on its own? That would still allow people to implement custom password behavior in their app code rather than having to tinker with the core packages or implement their own packages.

Under the hood, we could then refactor accounts-password to depend on accounts-password-api to remove any code duplication.

So… accounts-base <- accounts-password-api <- accounts-password. For anyone who uses accounts-password, everything would look and behave the same. Ditto for anyone who only uses accounts-base.

3 Likes

If what we are seeking mainly is preventing abuse, then perhaps the new ddp rate limiting feature request might just as well solve it.

Also coincidentally, one of the related threads discusses possibility of method hooks which might be a good starting point in extending accounts features.

EDIT:
Nevermind this post (keeping it here instead of deleting, for +1 sake on rate limiting), the original github issue already has ddp rate limiting related discussion.