search

deitch / activator

Easy user activation and password reset, with email confirmation, for nodejs
85 stars 42 forks source link

Support for password generation, strength enforcement and change notification. #43

Closed sjlongland closed 8 years ago

sjlongland commented 8 years ago

I've just been working on extending activator to do three things:

So far so good, I think I've managed to get something together here: https://github.com/vrtsystems/activator/commits/send-generated-password

HOWEVER, the test suite in activator is doing my head in. Specifically, I am having difficulties with understanding how the email tests work. It seems when I try to extend a test to pick up the "new password" email to check it against the supplied password, I wind up with a copy of the previous email in that test case. I'm not sure if this is the test suite in activator or whether it is smtp-tester doing something funny. Any guidance on this would be appreciated.

So right now, no test cases, but I'm trying to address this. In the meantime, I've published the code written so far for review.

deitch commented 8 years ago

Hey @sjlongland this is just great! I am right in the middle of solving a different issue - boot issues with grub, EFI, on a container OS - but I will get to it soon. This looks really good. I love the ideas, and will figure out the tests.

I see you are based in Brisbane. Heading Down Under over the (northern) summer with the family.

If you want to connect directly, my Skype ID is my last name, and my email can be found on my Web site www.atomicinc.com. Like to hear more in a private conversation how you are using it.

deitch commented 8 years ago

send an email with the newly changed password on reset

Why do you want to send the actual new password by email? The whole point here is not to send a password by email. When a user selects to reset, they get a one-time token, which is like a password in that it provides the ability to change your password and thus gain access, but it is time-limited.

After the expiry time (default is 60 mins, I believe), however, the link is dead. Without that, leaking your email 2 hours (or 2 months) later can let hackers in. An email breach could lead to complete compromise.

I am sure I am misunderstanding something. If I am not, drop me a line (or connect over Skype) and let's discuss alternative designs, that satisfy the fundamental business requirements without breaking a good security model.

I think I've managed to get something together here

Yeah. I am sure I am misunderstanding the third one, but the first two look great:

Can you make a quick branch for each of those and put in a Pull Request? We can use those as basis for working discussion and then pull them in from there. I love these.

Tests

Heh, email testing always was a pain. This system works really well, once you understand what it does. This was not the project that drove me to create https://github.com/deitch/smtp-tester but it had an impact.

The way it works is thusly (always wanted to use that word).

Initialization

  1. smtp-tester creates an smtp server listening on a local port at https://github.com/deitch/activator/blob/master/test/test.js#L1342 and with the url to that port configured at https://github.com/deitch/activator/blob/master/test/test.js#L90
  2. Each time activator is initialized, that URL is passed to activator.init(transport: url, ...) e.g. https://github.com/deitch/activator/blob/master/test/test.js#L1010

At this point, you have a test smtp server listening and activator set to send to it. Since the mail server implements the smtp protocol, activator does not know (or care) that the smtp server is a special test server; from its perspective, it could just as well be gmail

Capturing and Analyzing Emails

The way smtp tester works is that you register and deregister a handler for a given address, or for all emails. So for each test, you do:

  1. Register handler
  2. Do the activation or password reset, which causes emails to get sent, smtp-tester to receive them, the handler to be invoked
  3. Check that the received email matches the correct format
  4. Deregister the handler

BTW, smtp-tester can register a handler after the email was received. The moment you register for "stuart@longland.au" (or whatever), any email already in the queue and received will be passed to the handler, in addition to all future emails received, until such time as the handler is deregistered.

You can see this pattern, e.g. in the basic "success for known user test" at https://github.com/deitch/activator/blob/master/test/test.js#L384

  1. POST to /users/ https://github.com/deitch/activator/blob/master/test/test.js#L387
  2. Create a handler for activation https://github.com/deitch/activator/blob/master/test/test.js#L391
  3. Register the handler https://github.com/deitch/activator/blob/master/test/test.js#L392
  4. Deregister the handler https://github.com/deitch/activator/blob/master/test/test.js#L395

The shortcut aHandler referenced in the 2nd step above is a shortcut that creates an activation handler ready to be registered and includes a series of activation tests. Same for rHandler.

sjlongland commented 8 years ago

send an email with the newly changed password on reset

Why do you want to send the actual new password by email? The whole point here is not to send a password by email. When a user selects to reset, they get a one-time token, which is like a password in that it provides the ability to change your password and thus gain access, but it is time-limited.

After the expiry time (default is 60 mins, I believe), however, the link is dead. Without that, leaking your email 2 hours (or 2 months) later can let hackers in. An email breach could lead to complete compromise.

I am sure I am misunderstanding something. If I am not, drop me a line (or connect over Skype) and let's discuss alternative designs, that satisfy the fundamental business requirements without breaking a good security model.

Well, the primary reason is because we'll be generating the passwords rather than letting the user choose their own. Not ideal, but it does mean that we guarantee the user meets a certain minimum strength and isn't re-used. So in the event of a breach: we just reset the password, the password any cracker previously obtained becomes useless.

Long-term, we'll be moving to social authentication (Google, FaceBook). For privacy reasons however, some may choose to not use this, and this is their choice. (I for one, refuse to touch FaceBook/Twitter.)

As there'll be no box for the user to supply a password, they'll need to know what it is somehow, and delivery by email is one such mechanism. An alternative would be to display it on the page to the user, which is possibly better, but I'm not sure what changes are needed to pass that back to the front-end.

I agree that sending passwords in clear-text by email is less than ideal. I guess we're relying on the security of that person's inbox being up to snuff.

The thing is: being a template, you can decide to include the password or not, that's the programmer's decision to put the field in the template or omit it. Being told "your password changed", perhaps when you weren't aware of someone doing it, is still useful to know.

The way smtp tester works is that you register and deregister a handler for a given address, or for all emails. So for each test, you do:

  1. Register handler
  2. Do the activation or password reset, which causes emails to get sent, smtp-tester to receive them, the handler to be invoked
  3. Check that the received email matches the correct format
  4. Deregister the handler

Yeah, I did see that pattern, so I tried tacking my tasks onto one of the existing tests like this: https://github.com/vrtsystems/activator/blob/0c7997f367b58ab0efe283bee1a778fb6a4c89d9/test/test.js#L1258

The idea was:

  1. Post a password reset request
  2. Register a handler to pick up the token
  3. Post the received token, ensure it responds with 200 OK
  4. Register a handler to pick up the "changed password" response
  5. Check the password in the email matches what was used earlier

Steps 1-3 are pretty much the original test case. What I found was that I either wound up with the handler registered in step 4 receiving the password reset email sent in step 1, or I'd receive nothing and the test would time out.

As an experiment, I tried a different buffering technique; registering a general handler that would receive all mail and stuff it into an array. It took a single handler, and if a handler was changed or a new email arrived, it'd try to send all the buffered email to that handler.

https://github.com/vrtsystems/activator/commit/12d54760d3a4eb704fa48289e82a019989af8657 is what I changed to do that. There, tests failed left-right and centre:

    initialized
      with string transport
        activate
          ✓ should send 500 for user property not added
          auth header
RECV: 2@foo.com : Activate Email
DEL: 2@foo.com : Activate Email
            ✓ should fail for known user but bad code (109ms)
RECV: 2@foo.com : Activate Email
DEL: 2@foo.com : Activate Email
            ✓ should fail for known user but bad code with handler (66ms)
RECV: 2@foo.com : Activate Email
RECV: 3@foo.com : Activate Email
DEL: 3@foo.com : Activate Email
DEL: 2@foo.com : Activate Email
Exception hit: AssertionError: expected '2@foo.com' to equal '3@foo.com'
            1) should fail for another user
            ✓ should fail for another user (125ms)
RECV: 2@foo.com : Activate Email
RECV: 3@foo.com : Activate Email
DEL: 3@foo.com : Activate Email
Exception hit: AssertionError: expected '3@foo.com' to equal '2@foo.com'
            2) should succeed for known user
RECV: 2@foo.com : Activate Email
DEL: 2@foo.com : Activate Email
            3) should succeed for known user with handler

Seems the receivers part of the incoming email doesn't necessarily match what would have appeared in addr for an address-specific email. So clearly that isn't the answer and I'll look at backing that change out.

I'll admit that I'm having something of a baptism of fire with JavaScript and NodeJS: last time I did anything serious with it (early 2000s), the browser market was a duopoly of Netscape and Internet Explorer (≤5.5) and web site design guides recommended designing for 800x600 pixels and a 10 second load time on 28.8kbps dial-up. So to say I'm rusty is an understatement. :-)

As to Skype: did have it installed some time back but lately I've found it more hassle than it was worth, so ultimately I haven't bothered to re-install it.

sjlongland commented 8 years ago

A heads up, I'm having a closer look at smtp-tester as I suspect something there. There didn't appear to be a multi-email test case, so maybe that's a starting point.

Regarding passwords in email: this was seen as lower risk than displaying the generated password on the page.

deitch commented 8 years ago

did have it installed some time back but lately I've found it more hassle than it was worth, so ultimately I haven't bothered to re-install it.

LOL! Yeah, the quality has dropped since Microsoft bought them. I am not some anti-Microsoft hater - I actually love what Nadella has done with them in the last few years - but I really don't like the quality. I also do Google Hangouts/Chat, and email, and WhatsApp, etc. I would like to hear more about what you are using it for and how. The more real-world stories I hear, the better I can manage this and every other thing I do. Plus I learn ideas to share with clients, and can share back. And in any case, I love Aussies.

More in second....

deitch commented 8 years ago

I guess we're relying on the security of that person's inbox being up to snuff.

Sounds like you know that is asking for trouble. But you are right, not my job to prove it one way or the other. I am not your CISO.

The thing is: being a template, you can decide to include the password or not, that's the programmer's decision to put the field in the template or omit it.

That is a very good point. OK, I cannot argue with this. Well, I can, but as we said, I am not your CISO. We just need to put in a nice warning.

An alternative would be to display it on the page to the user, which is possibly better, but I'm not sure what changes are needed to pass that back to the front-end.

Yeah, that is what I was thinking. I really like the idea of having an option to generate the user's token (i.e. password) and using a policy mechanism to set it. I don't think it would be too hard to do it. Let's discuss separately.

OK, so let's do the following:

  1. Open a PR for each of the three: add new password to template options; add password policy engine option; generate a password subject to policy.
  2. Open a new issue for "add option to return new password (whether created by user or auto-generated) to Web request response. I will look at it quickly, then comment as to whether it is something that is easy and quick or requires more effort (which means figuring it out in between clients and such).
  3. Keep this issue open for the smtp testing issues. You added so much detail that we should be able to figure it out quickly.

I will look at the smtp issues next week.

Sounds like a plan.

sjlongland commented 8 years ago

Okay, good point: I'll break out the different features and we'll go from there.

sjlongland commented 8 years ago

Okay, I've broken out password generation and checking into separate branches; the checking branch forks off the generation branch.

These are:

I'll see if I can bang a test suite into some sort of shape for the third feature.

deitch commented 8 years ago

Yup, I saw them, thank you. Always great to have good people both think of good ideas and help contribute.

So are PRs standard fare for Sunday evening in Brisbane? :-)

deitch commented 8 years ago

Just been skimming your blog, you really do cover the gamut: hardware, software development, operating systems, I see you messing around with Samba, kernel booting, really across the board. Love to have a conversation around technology. You said Skype is out (cannot blame you), but the stuartl@ address appears on a number of mailing lists. Any desire to chat?

sjlongland commented 8 years ago

On 22/05/16 19:00, Avi Deitcher wrote:

Yup, I saw them, thank you. Always great to have good people both think of good ideas and help contribute.

No problems. :-)

So are PRs standard fare for Sunday evening in Brisbane? :-)

Not normally, it's a matter that's been niggling on my mind and normally 7PM is about my bed time. (I'll be getting up at 3AM tomorrow for work.)

That, and a not entirely successful day getting a battery charge controller working: https://hackaday.io/project/10529-solar-powered-cloud-computing/log/38421-attiny24a-charge-controller-fail-so-far

I figured I'd get those bashed out first then I'll tackle the new password email notification in the morning. At least I can then say

I've accomplished something for today. :-)

Stuart Longland (aka Redhatter, VK4MSL)

I haven't lost my mind... ...it's backed up on a tape somewhere.

sjlongland commented 8 years ago

On 22/05/16 19:08, Avi Deitcher wrote:

Just been skimming your blog, you really do cover the gamut: hardware, software development, operating systems, I see you messing around with Samba, kernel booting, really across the board. Love to have a conversation around technology. You said Skype is out (cannot blame you), but the stuartl@ address appears on a number of mailing lists. Any desire to chat?

Indeed, I have many fingers in many pies. By "trade", I'm in telecommunications engineering and software engineering, and bounce between those two fields regularly. My home projects tend to be more electronics-oriented, since I do enough "IT" stuff at work.

We do "systems integration" at work, mostly in the energy management sector. Here's hoping there'll be an announcement of the energy management portal we're developing (which is where activator is getting used). My involvement there has been mostly at the lower levels: data collection, and in some cases, kernel hacking. This is my first real foray into the upper layers.

Mailing lists are more or less standard fare. I find email the easiest mechanism to communicate these days as it's universal and asynchronous; the latter being useful when people are in different time zones.

Seems amusing that the world is running back to the walled gardens, when it wasn't that long ago (early 90s) that the Internet brought an end to the walled gardens of the day (BBSes, Compuserve, etc).

But I digress. Feel free to take the discussion offline so we don't

clog up the bug. :-)

Stuart Longland (aka Redhatter, VK4MSL)

I haven't lost my mind... ...it's backed up on a tape somewhere.

deitch commented 8 years ago

There didn't appear to be a multi-email test case in smtp-tester

What does "multi-email test case" mean?

sjlongland commented 8 years ago

On 23/05/16 01:26, Avi Deitcher wrote:

There didn't appear to be a multi-email test case in smtp-tester

What does "multi-email test case" mean?

Okay, a test case where we test receipt of multiple emails (individually; perhaps with switching handlers in between). So something like:

… etc. I will admit I did only a quick search though so I might've

blinked and missed it.

Stuart Longland (aka Redhatter, VK4MSL)

I haven't lost my mind... ...it's backed up on a tape somewhere.

deitch commented 8 years ago

Right you are, there is something strange going on with multiple handlers for the same address. Something to do with unbinding.

deitch commented 8 years ago

Nope, definitely not that. I added tests for multiple emails to the same handler, and email+handler+unbind+email+handler+unbind and all is good.

sjlongland commented 8 years ago

Yep, I'm not ruling out that the problem exists between the keyboard and chair at this point as I'm both new to NodeJS as well as its test suites (e.g. Mocha). Now that I've got test cases for password generation and for checking strength down pat, that just leaves the new password notification emails.

sjlongland commented 8 years ago

Okay, I must've been doing something wrong. No idea why but it works now. Third pull request coming right up! :-)

deitch commented 8 years ago

A few things:

  1. All of the PRs have been merged in. Mostly manually, because they were missing README updates, and some style changes, etc. but all in.
  2. The problem here was that you weren't doing mail.removeAll(). smtp-tester is not a "deliver-exactly-once" message queue. If a handler handled a message and unbinds, and then a new one comes along for the same address, it will get the same message again. If you do not want a message to be delivered again, you need to do mail.remove() for the specific ID or mail.removeAll().
  3. We added in the option to send the password in email, but I really don't like it. Open an issue to send the generated email back in the REST response and we will do that.
sjlongland commented 8 years ago

Many thanks for looking into this and merging the pull requests. This is a big help. :-)

deitch commented 8 years ago

@sjlongland I am glad it works. I enjoy these projects, and they sometimes lead to good consulting work.

Will you open a ticket for sending a generated password in a response to the REST API call?

sjlongland commented 8 years ago

I might have a look into it at some point, but it won't be for a while, as I've got a lot on the next few weekends, and some days the last thing I want to look at when I get home is the computer. :-)

deitch commented 8 years ago

Yeah, I know that one. But I though all of this was for work?

sjlongland commented 8 years ago

On 26/05/16 14:43, Avi Deitcher wrote:

Yeah, I know that one. But I though all of this was for work?

The pull requests that have been merged are, I did ask about displaying the password on the page but two reasons it was voted down:

  1. The user's password is visible to "shoulder surfers"
  2. The user has no permanent record of the password

I'd argue that #2 is their problem and that #1 applies to the emails too, but that is what the team decided so I more or less have to tag along.

I think it'd be worthwhile however as a matter of completeness, just there's no urgency for it to be done for this project.

deitch commented 8 years ago
  1. The user's password is visible to "shoulder surfers"

There are ways around it, like having a UI that shows it only when you click "I am ready" or similar, after checking around. But this is minute compared to the emails.