Fix for brute_force vulnerability #231

[futuretap]

extend the account lock for subsequent failed login attempts after the initial lock period

[joshbuker]

Considering this changes functionality in a potentially undesirable way, we'll want to implement this as a configuration option that defaults to false. See comments on #231

[futuretap]

While #231 is a clear and severe vulnerability (see my new comment), we could of course discuss what the best fix is. IMO, there are two resolution options:

1. When lock_expires_at has passed, reset the failed login counter back to 0 and set lock_expires_at to nil, effectively giving the user another full set of attempts.
2. When lock_expires_at has passed, extend the lock immediately for another login_lock_time_period at the first failed login attempt (if there wasn't a successful login in between).

I've opted for option 2, the more secure option. But option 1 would be certainly fine, too, and we could change this. I recommend against a config option which is hard to explain and understand.

[joshbuker]

Option 1 is already implemented, see comments on issue.

Edit:

The implementation of option 1 was flawed, and has since been patched by #235.

I agree that Option 2 would be more secure, but I think that option 1 is sufficient for slowing down brute force attacks while having the benefit of being less frustrating to end users. I think the explanation for the configuration wouldn't be that difficult to grok, for example:

# If an account receives failed login attempts while locked out, renew/extend the lockout period.
# default: false
# config.failures_extend_lockout_period = false

Thoughts?

[futuretap]

OK, would be a reasonable option and explanation!

Thanks for quickly fixing the vulnerability!