Re: [Acegisecurity-developer] DaoAuthenticationProvider: Ordering of exceptions...
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=7412alloc_id=16344op=click ___ Home: http://acegisecurity.sourceforge.net Acegisecurity-developer mailing list Acegisecurity-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/acegisecurity-developer
[Acegisecurity-developer] DaoAuthenticationProvider: Ordering of exceptions...
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. 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=7393alloc_id=16281op=click ___ Home: http://acegisecurity.sourceforge.net Acegisecurity-developer mailing list Acegisecurity-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/acegisecurity-developer
RE: [Acegisecurity-developer] Suggestions for changes to AbstractProcessingFilter
Ben, Comments below... I've committed this one, minus the UsernameNotFoundException (because it gets re-thrown by DaoAuthenticationProvider in a BadCredentialsException). If people need to support additional application-specific (rather than Acegi Security-specific) exceptions, we could provide a hook method that subclasses can optionally override which returns a target URL. Alternatively, people can perform additional conditional processing in the target URL servlet/filter/page etc based on HttpSession's ACEGI_SECURITY_LAST_EXCEPTION_KEY attribute. Yes, I wasnt too sure about the distiction between UsernameNotFoundException and BadCredentialsException, I probably should have looked into this a little more deeply, but the assumption I made was that the former meant an incorrect username and the latter an incorrect password. On the second point, I did spend a little time considering how to handle custom subclasses of AuthenticationException and I considered implementing both the hook method you describe and also a 'AuthenticationFailureUrlResolver' that could be plugged in in the application context XML file. However, I got a little nervous about the code that would be required to do the dynamic type checking on the custom exceptions. J2EE classloading is not known for its simplicity and I found it difficult to think my way through all the end-cases. I eventually came to the same conclusion as you have, that the end-user can use the exception stored in the session to perform more specific logic and that it may be better to wait until there is a specific requirement for the core ACEGI classes to handle these specific exception types. The current types seem pretty comprehensive anway. I haven't committed this one because I believe re-throwing the exception loses the all-important lower-down stack trace. I believe it's probably better to change individual AuthenticationProviders to use the enhanced AuthenticationException subclasses properly, rather than relying on ProviderManager to do it for them. Do you agree, or is there some other issue that I haven't considered? This would actually be my preference too. I dont think the stack trace will be affected unless a new exception is created, but I would have liked to set the Authentication object in an overloaded constructor. The problem is that it seemed that in some cases the Authenication instance wasnt available. The advantage of intercepting it in the ProviderManager is that it seems to be a central location where there is access to the Authenication instance, so no matter how the user decides to implement their custom AuthenicationProvider or Dao the Authenication object will always be available when it counts. I guess there are pros and cons of both approches, but I didnt want to dive in and start making very invasive changes and given that the ProviderManager interception represented the least invasive change, I eventually opted for this route. Thanks again for your contributions and feedback, Wesley. You are very welcome, and thank you for your work on creating a wonderful security framework, and finally freeing me and many other developers for the inflexible nightmare that is J2EE container-managed authentication ;o) Regards Wesley Hall --- This SF.Net email is sponsored by BEA Weblogic Workshop FREE Java Enterprise J2EE developer tools! Get your free copy of BEA WebLogic Workshop 8.1 today. http://ads.osdn.com/?ad_id=4721alloc_id=10040op=click ___ Acegisecurity-developer mailing list [EMAIL PROTECTED] https://lists.sourceforge.net/lists/listinfo/acegisecurity-developer
RE: [Acegisecurity-developer] Suggestions for changes to AbstractProcessingFilter
Ben, Thanks for taking the time to respond. (Comments below) -Original Message- From: [EMAIL PROTECTED] [mailto:[EMAIL PROTECTED] Behalf Of Ben Alex Sent: 20 July 2004 04:02 To: [EMAIL PROTECTED] Subject: Re: [Acegisecurity-developer] Suggestions for changes to AbstractProcessingFilter Hi Wesley Thanks for the suggestions. Comments below. Wesley Hall wrote: Hello all, I have a couple of suggestions for changes to AbstractProcessingFilter. I am not sure on process for submitting patches but I am happy to make these changes myself if somebody would care to provide this information. My first suggestion is to provide alternate failure URLs for the different failure reasons. These URLs shouldnt need to be madatory as the system could default to the mandatory failure URL. I have looked at the code for this class and it seems that the system catches an AuthenticatationException and if this is caught redirects the user to the specified failure URL. If this catch block was extended to catch the relevant AuthenticationException subtypes the functionality could be easily extended to redirect to different URLs on different events if required by the developer. If there is no URL configured for the particular exception type then the system should default to redirecting to the existing failure URL. Most Acegi Security implementations use SecurityEnforcementFilter. It is that class which distinguishes between authorization vs authentication failures in lower-down classes, providing a 403 in the event of an AccessDeniedException and a redirect to the AuthenticationEntryPoint in the event of an AuthenticationException. The AuthenticationProcessingFilter logic is only executed when an actual authentication attempt is made (eg a BASIC authentication header is detected, a form-based login is submitted etc) so it is unlikely it would ever see say an AccessDeniedException. So if you'd like alternative handling of AccessDeniedExceptions, it might be more helpful to put it within the SecurityEnforcementFilter. The functionality I am suggesting relates purely to when an authentication attempt is made. I have written an AuthenticationProvider implementation as my requirements go slightly beyond the features provided within the DaoProvider. My implementation of the authenticate method throws different AuthenticationException subtypes (currently only subtypes provided by acegi such as DisabledException). It would be useful for me to redirect the user to different URLs depending on which exception type was thrown by the provider. My suggestion to provide this feature without breaking existing functionality is to add some extra properties to AbstractProcessingFilter (which currently holds the generic 'authenticationFailureUrl') to include optional URLs to redirect the user in the event of a DisabledException or LockedException (for instance). Extend the catch block (that currently catches the AuthenticationException superclass) to catch the main subclasses and in each catch block check if the URL property for that type of failure is null. If it is null, continue with existing behaviour and forward to the mandatory authenticationFailureUrl, but if it is not null, forward to this specific URL instead. The definition for the bean in the spring configuration file becomes... bean id=authenticationProcessingFilter class=net.sf.acegisecurity.ui.webapp.AuthenticationProcessingFilter property name=authenticationManagerref bean=authenticationManager//property property name=authenticationFailureUrlvalue/logon.jsp?error=1/value/property property name=accountDisabledUrlvalue/logon.jsp?error=2/value/property property name=accountLockedUrlvalue/logon.jsp?error=3/value/property property name=defaultTargetUrlvalue/secure/index.html/value/property property name=filterProcessesUrlvalue/j_acegi_security_check/value/property /bean ...for example. Here the system provides the error page with the parameter error=2 for DisabledException and error=3 for LockedException and defaults to error=1 for all other exceptions. As the new properties are optional, and the system defaults to the existing mandatory url, backward compatibility is maintained. I may have some time this evening to make these changes, I will need them for my project anyway, I will of course provide them back to you and you can decide whether there is merit in including them in a future version. As I am not sure of the preferred format to supply patches I will just email the altered files to this mailing list. Ideally I would like to take a branch of the current head to make these changes on, but I assume sourceforge doesnt have seperate permissions for branch creation and committing to the head and I understand that you may not want to give commit access to anyone that comes along and asks for it. The second suggestion is that, upon authentication failure, the system could
RE: [Acegisecurity-developer] Assigning roles to URL's and MethodInvocations
Ben, Thanks for your response. This is certainly not a big problem, I see some minor advatages to attaching the security to the bean or the controller in question but these are not big enough advantages to warrent a large development effort. If I find, during the course of my project, that the advantages are bigger than they currently seem then I will certainly develop the classes required to support this method and submit them. Once again, thanks for your time. Wesley P.S. Notice the lack of a 't' in that spelling ;o) -Original Message- From: [EMAIL PROTECTED] [mailto:[EMAIL PROTECTED] Behalf Of Ben Alex Sent: 19 July 2004 04:16 To: [EMAIL PROTECTED] Subject: Re: [Acegisecurity-developer] Assigning roles to URL's and MethodInvocations Hi Westley Wesley Hall wrote: Lets take the example of URL based security (The FilterInvocationInterceptor). The mapping between the security role required and the actual code that handles that URL invocation seems to be based on a list of URL's and the roles required to access them. I can understand the benefits of this approach but I have found that if I decide to change my controller mappings I also have to go back and change the information supplied to the objectDefinitionSource attribute of the FilterInvocationInterceptor. I wonder whether it would be possible to obtain the information provided by the objectDefinitionSource from the Controller itself. Lets say I have a controller called 'AdminController' that provides the model and the view for a system administration screen. This controller is mapped to /admin/index.html. The objectDefinitionSource in the FilterInvocationInterceptor has '\A/admin/.*\Z=ROLE_ADMIN'. If I later decide that the admin URL shoudl be '/tools/admin.html' I have to change both the controller mapping and the objectDefinitionSource. This is not a major problem, and perhaps I am splitting hairs but if the 'ROLE_ADMIN' was assigned directly to the controller then I could remap URLs without needing to reconfigure the security system. The current approach is based on the Servlet spec. I felt it was a reasonable basis because people are already familiar with how it works, and providing it via Acegi Security frees people from needing container security via web.xml. The implementation also provides web framework independence, as I know we have users utilising a range of web frameworks. AFAIK, there is nothing preventing anyone from writing a new class that behaves the way you describe. It's just a question of priorities. Nobody has felt strongly enough about the potential maintenance issue to go write one. And I guess the alternative - web.xml security constraints - would require the same (or probably more, due to the inflexible path mappings) maintenance. I suspect that a similar approch could be used to wrap the 'BankManager' bean in your provided example, this would allow the security information to remain 'attached' to the object that it is securing. The MethodSecurityInterceptor approach was based on Spring's own transaction definition approach.Once again, so people migrating would recognise it and feel comfortable. As an aside, some people feel Acegi Security is too complex. This is probably because it has somewhat deep documentation, covering all of its pluggable points, and a lot of bean definitions, so you can utilise all of that pluggable support. In its most commonly used form, the two security areas (URL and method interception) are configured just like official Servlet spec or Spring standards. As always, I welcome any patches which improve the project. Best regards Ben --- This SF.Net email is sponsored by BEA Weblogic Workshop FREE Java Enterprise J2EE developer tools! Get your free copy of BEA WebLogic Workshop 8.1 today. http://ads.osdn.com/?ad_id=4721alloc_id=10040op=click ___ Acegisecurity-developer mailing list [EMAIL PROTECTED] https://lists.sourceforge.net/lists/listinfo/acegisecurity-developer --- This SF.Net email is sponsored by BEA Weblogic Workshop FREE Java Enterprise J2EE developer tools! Get your free copy of BEA WebLogic Workshop 8.1 today. http://ads.osdn.com/?ad_id=4721alloc_id=10040op=click ___ Acegisecurity-developer mailing list [EMAIL PROTECTED] https://lists.sourceforge.net/lists/listinfo/acegisecurity-developer
[Acegisecurity-developer] Suggestions for changes to AbstractProcessingFilter
Hello all, I have a couple of suggestions for changes to AbstractProcessingFilter. I am not sure on process for submitting patches but I am happy to make these changes myself if somebody would care to provide this information. My first suggestion is to provide alternate failure URLs for the different failure reasons. These URLs shouldnt need to be madatory as the system could default to the mandatory failure URL. I have looked at the code for this class and it seems that the system catches an AuthenticatationException and if this is caught redirects the user to the specified failure URL. If this catch block was extended to catch the relevant AuthenticationException subtypes the functionality could be easily extended to redirect to different URLs on different events if required by the developer. If there is no URL configured for the particular exception type then the system should default to redirecting to the existing failure URL. The second suggestion is that, upon authentication failure, the system could place the authentication object (that failed) into the session. If the failure pages are dynamic then the failure pages could perform some application specific logic to display even more information to the user. For example... Authentication has failed. Your account was disabled by 'joe_superuser' at 19/07/04 at 14:22. The problem with this is finding an appropriate time to remove this value from the session Perhaps it would be better to use a RequestDispatcher to forward the user onto the failure url and place the failed Authentication object in the request. This way the object wouldnt 'hang around' past its scope. Would this work? I guess this would prevent the failure pages from residing in a different webapp or on a different server... is this a common requirement? Regards Wesley Hall --- This SF.Net email is sponsored by BEA Weblogic Workshop FREE Java Enterprise J2EE developer tools! Get your free copy of BEA WebLogic Workshop 8.1 today. http://ads.osdn.com/?ad_id=4721alloc_id=10040op=click ___ Acegisecurity-developer mailing list [EMAIL PROTECTED] https://lists.sourceforge.net/lists/listinfo/acegisecurity-developer
