Ben Alex wrote:
Wesley Hall wrote:
Hello,
I hope everyone is well.
I wanted to query the ordering of the exceptions thrown by the DaoAuthenticationProvider class. It seems that the authenticate method will first check that the user (with the specified username) can be loaded, next it will check the status of this user, such as whether the account is disabled, locked etc. Then it will check the password.
I would propose that maybe the disabled/locked checks should come AFTER the password check. I am currently able to determine the status of an account without knowing the password and I would rather that the system only informs a user that there account is disabled/locked if they provided the correct credentials. Need to know basis....
I was going to submit a patch for this but I am getting some compile errors with the latest CVS head. It seems net.sf.acegisecurity.util.MockFilterChain is missing.
Ben, Colin et al... any objections to such a change? Would you like me to issue the patch?
Thanks chaps.
Hi Wesley
The reason the locked checks occur BEFORE the password comparison is because the main purpose in locking an account is to stop brute force password attacks. If say 5 invalid passwords are received, an ApplicationListener can set that user's account to locked. Then the sixth password attempt will be responded to with LockedException instead of BadCredentialsException. The pairing of disabled checking alongside the locked checking was done because initially we only recognised disabled accounts (not locked accounts). Locking was added to make the exception reporting more granular.
Cheers Ben
Thanks for the reply Ben,
I see your point about locked accounts, I definatly should have considered the brute forcing case. Implementing a maximum number of failed logons before locking the account is exactly what I have done. I agree that this should be checked prior to password checking for this reason.
I have implemented the booleans in User as follows:
enabled: An explicitally set value indicating whether the account is enabled or not.
accountExpired: Each account entity has an expiry date (or null). If this is not null and it is in the past I consider the account expired. (Could also represent a maximum inactivity period)
credentialsExpired: User hasnt changed their password in X amount of time.
accountLocked: User has exceeded X number of failed logons.
I assume these are the intended meanings.
The current ordering of the checks seems to be:
username valid? account enabled? account expired? account locked? password correct? password expired?
I suppose my 'revised' suggestion would be to move the enabled and expired checks after password check. Assuming my definitions for each check are correct then enabled and expired essentially amount to the same thing except the former is immediate and the latter is scheduled. Information on this status should only be presented to the legitimate user (who has entered a correct password), otherwise, an attacker may be able to discover a disabled account, via brute forcing usernames, and convince the administrator to enable it and perhaps even reset the password.
It probably makes sense to put the enabled and expired checks after the password check but BEFORE the password expired check, otherwise, an account that is disabled 1 day after creation will report as disabled until the max password age is reached and then it will display password expired (without specific logic in the DAO). It seems more consistent to report account disabled regardless of password expiry status. A consequence of this is that a user would be able to use an expired password to discover that the account has been disabled (or expired), but the alternative is to tell them that their password has expired only to find out at some point down the 'reset expired password' use case that there is a broader problem with their account. I would lean towards the first option... unless there is a third?
So in conclusion, I am suggesting a change of order to...
username valid? account locked? password correct? account enabled? account expired? password expired?
Does this make sense? Am I missing anything else?
Thanks once again.
Regards
Wesley Hall
------------------------------------------------------- This SF.Net email is sponsored by Oracle Space Sweepstakes Want to be the first software developer in space? Enter now for the Oracle Space Sweepstakes! http://ads.osdn.com/?ad_id=7412&alloc_id=16344&op=click _______________________________________________ Home: http://acegisecurity.sourceforge.net Acegisecurity-developer mailing list Acegisecurity-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/acegisecurity-developer
