Sergio Berna wrote:
I have added ExpirationDetails as a separate interface to keep backwards compatibility with existing code that implementes UserDetails.
Hi Sergio
Good to see backward compatibility is a priority, particular in such a sensitive (ie commonly-deployed and extended) area as DaoAuthenticationProvider and UserDetails.
I am just wondering whether it would be simpler, though, to modify the UserDetails interface so it contains the isAccountExpired() and isCredentialsExpired() methods? Then the existing constructor of the User implementation - which is what most people use - could set the properties to false. There would also be an additional constructor which AuthenticationDaos could use if they had access to the additional properties. We should probably also deprecate the existing constructor, to prompt people to consider the change (and move the decision to set the properties to a false default into their AuthenticationDao construction of User).
For the small minority of people who have chosen NOT to extend User (which goes against our recommendations, but there are legitimate scenarios such as having a domain object that already represents the user), I don't think adding two methods to their implementation is going to cause much concern - especially as they can simply return false.
This alternative would still provide 95% of users with full backwards compatibility, but avoid an extra interface. As the project also provides basic implementations of each interface, it also avoids us needing to write a UserExpirationDetails (for example). It is also cleaner to avoid these extra classes given that people often cast the contents of ((SecureContext)ContextHolder.getContext()).getAuthentication().getPrincipal(). It also makes these new properties and exceptions non-optional concepts in the overall framework, which means we will modify the included AuthenticationDaos (eg in-memory and JDBC), as well as the exception resolvers, to accommodate them.
One other thing is the method names. I think it would be better to keep "true" being consistently returned as the affirmative/positive indication from each isXxxxx() method on UserDetails (there is already UserDetails.isEnabled()). So perhaps rename the methods to isCredentialsNonExpired() and isAccountNonExpired(), or something similar.
Best regards Ben
------------------------------------------------------- The SF.Net email is sponsored by: Beat the post-holiday blues Get a FREE limited edition SourceForge.net t-shirt from ThinkGeek. It's fun and FREE -- well, almost....http://www.thinkgeek.com/sfshirt _______________________________________________ Home: http://acegisecurity.sourceforge.net Acegisecurity-developer mailing list Acegisecurity-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/acegisecurity-developer
