Auth updates

[andrewplummer]

Attempt to address in as simple a way as possible a couple potential security issues with authentication:

1. Login throttling

Will apply a lockout on password login that kicks in after 3 bad login attempts and scales up to an hour at 10. The min, max, lockout time and scaling are all something we can tweak to whatever.

2. Reset password token reuse

Once a reset password token is consumed it should no longer be usable. My goal here is to do this in a very simple way that doesn't involve holding onto an ever growing list of consumed tokens. The jti JWT claim seems to be designed for this purpose, allowing a token ID to be generated that can be attached to the user on request-password and compared on set-password, which will only allow the agreed upon token to be used, everything else will be rejected.

3. Password length

Bumped the password min length to 12 chars. I'm not hard on this one but it's what was recommended by the security audit and seems to be pretty standard these days after a simple search. I think people are moving to longer passwords in an attempt to encourage passphrases over weird characters.

[dominiek]

Very cool

[kaareal]

some what sceptical about the testing part but looks fine otherwise

[kaareal]

think this is overkill, you can just changed the db model, instead of messing around with 3 party libraries or extra tests helpers.

[andrewplummer]

this is actually a bit different, it needs to mock Date.now as this isn't only DB bound... this is all very standard though we shouldn't be afraid of mocking the time it's more reliable and communicates use case more clearly

[kaareal]

I guess I am not following. I see nothing in the tests below that couldn't be handled by changed the db values. Could be wrong.

The library might be standard, but i am just thinking that we only gonna use it for this one test ... doubt we will have other uses for it? Well we havn't in the past

[andrewplummer]

https://github.com/bedrockio/bedrock-core/pull/122/files#diff-31a9ea2d93265b6340d2ff8feac24b00fa533ae6cc118838481452eab8b21fafR32

The above line uses new Date() so that needs to be mocked.
also I don't necessarily mean this library is standard (sinon is probably more standard but this seems well maintained and lighter) but mocking time is very common. For Bedrock out of the box I agree it's probably only needed here but for any project where you can say "a bit later this happens" you should be mocking time like the above. One example of this is "reminders" where I've also had to do this (notification is sent after 30 days etc).

[kaareal]

I was thinking you could just write

  const user = await createUser({
        password,
        loginAttempts: 3,
        lastLoginAttemptAt: new Date(Date.now() - x),
      });

To validate it works

[kaareal]

Of course I could reset those fields after each request as well
Ideally I would split each request until its own tests if its testing some different aspect of the code, makes it easier to debug and maintain. More isolated. Input-output.

honestly I don't see this being any different from mocking the time
yeah i means thats my points, its just slimmer no 3 party library (+ no snyk errors about some dev dependency), no extra tests helpers to maintain.

But fair enough perhaps it does read nicer.

[andrewplummer]

I dunno I see mocking time functions as an extremely fundamental and common practice... to the point where Jest supports it out of the box: jestjs/jest#2234

looking at that issue though I think I can align better by using the same lib that is a dependency of Jest, so moved to that and removed mockdate dependency

as for the testing itself I think in most cases you're not testing across time, however this case is doing explicitly that so it belongs in a single test. I see the DB state as something of a hack to stand in for what you're really testing, a change in time. it works for this case but what if that state was kept in memory and private?

[andrewplummer]

ok I'm going to take a slightly hard stance on this one, I think mocking date is totally kosher, I do it a lot, and although I DO try to isolate tests as well I see our route tests as something more like an integration test so you're not doing 100% strict code isolation but sometimes testing mutations in state across time catches bugs that isolated tests would not otherwise...

at any rate I've at least moved to @sinon/fake-timers .. this is what Jest uses under the hood so there's no extra dependency now... (Jest's implementation of it is a shit show but that's a separate issue and also partially why I'm hiding it under the rug in the helpers)

[kaareal]

Awesome re the dependency, thats at least one less issue out of the way.

as for the testing itself I think in most cases you're not testing across time, however this case is doing explicitly that so it belongs in a single test.

yeah perhaps you are right in this case.
I just want to avoid these huge monster tests with multiples api requests, tests all sorts of stuff.

I see the DB state as something of a hack to stand in for what you're really testing, a change in time. it works for this case but what if that state was kept in memory and private?

Right if we where to test in memory we could be forced down that path, but it's somewhat an edge case (perhaps workers/jobs might use in memory) but yeah the test is for a REST api no?

Anyway we should come back to that one day, and talk that over.

[andrewplummer]

I just want to avoid these huge monster tests

Totally with you on that one 👍

but it's somewhat an edge case

Yeah I meant actually in this case only...its a special case to be sure