Lock user after a configurable amount of failed login attempts #169
Conversation
After a configurable amount of consecutive failed login attempts the new attribute `locked_until` will be set. The default amount of attempts is 5 and the default time the user is locked is 5 minutes. After this commit locked users will still be able to log in. Locking out locked users is part of the next commit.
When a user is locked (locked_until is in the future) he can't log in anymore.
Yes, I agree. But IMHO we should use I also would like to see the lock duration configurable, what do you think @calmyournerves, @pencil? |
It is no possible to disable the feature by setting the configuration for max failed login attempts to -1.
|
@calmyournerves @luxflux @pencil The feature can now be disabled by setting |
|
LGTM |
|
@pencil Good to merge? |
|
I believe calling |
| @@ -4,6 +4,8 @@ module CASino::SessionsHelper | |||
| include CASino::TicketGrantingTicketProcessor | |||
| include CASino::ServiceTicketProcessor | |||
|
|
|||
| LOCK_TIMEOUT = 5.minutes | |||
There was a problem hiding this comment.
This constant seems to be unused.
To prevent possible timing attacks, a concern raised in [1], this defers the credentials check until it's validated that the user is not currently locked. [1] rbCAS#169 (comment)
This reverts commit a32fa91.
1 similar comment
Prevents timing attack concern by checking the user lock status first. The concern seemed very valid and was raised in rbCAS#169 (comment) .
| def log_failed_login(username) | ||
| def user_locked?(username) | ||
| CASino::User.where(username: username).to_a.any? do |user| | ||
| user.locked? |
There was a problem hiding this comment.
Is .to_a.any? necessary? Also, I would return after the first locked one:
CASino::User.where(username: username).each do |user|
return true if user.locked?
end
There was a problem hiding this comment.
to_a is necessary to use Enumerable.any?:
any? [{ |obj| block }] → true or false
Passes each element of the collection to the given block. The method returns true if the block ever returns a value other than false or nil.
vs. the ActiveRecord::Relation.any?:
any?()
Returns true if there are any records.
There was a problem hiding this comment.
It would be totally possible to do it through each if that's somehow more efficient.
There was a problem hiding this comment.
Why is where used anyway? Isn't this sufficient?
!!CASino::User.find_by(username: username).try(:locked?)There was a problem hiding this comment.
The same username can exist within different adapters. That's why.
There was a problem hiding this comment.
Hmm... if there are the same usernames on several backends, all of them should be locked? So just fetching the first one would be okay for this check...?
There was a problem hiding this comment.
We could also just add a scope for locked users:
CASino::User.locked.where(username: username).any?
|
@pencil could you spend a few Minutes to review the latest changes? |
There was a problem hiding this comment.
There is an error in the code, which I discovered when we deployed our fork. Don't merge this branch until further notice! Never mind. It's all good. Just the migrations where missing. Usually we run them automatically, which it didn't this time (because of reasons).
There should not be a separate message when the account is locked due to too many login attempts, as this would be an information leak.
Before, the user was claimed to be locked when any CASino::User sharing the same username was locked. But a user is actually only locked, if he does not have any CASino::User left, which is unlocked. The reason herefore is, that he could have any secondary/legacy authentication providers, on which he can never login. Therefore, one of his 'alter egos' may be getting locked on any regular (and otherwise successful) login attempt and he would be locked out regardless. That is now changed, so that all CASino::Users sharing the same username must be first be in locked state before one is considered locked.
This PR introduces a new configuration option
max_failed_login_attemptswith a default value of 5.When a user unsuccessfully tries to login 5 times in a row he gets locked for 5 minutes. Technically this is done by setting the attribute
locked_untilwith an offset of 5 minutes.