Re: [Acegisecurity-developer] Suggestions for changes to AbstractProcessingFilter
Wesley Hall wrote: Hi Ben, Hi Wesley If you'd like to submit patches which take advantage of the enhanced AuthenticationException.getAuthentication() method, I'd be pleased to apply them to CVS. Best regards Ben What I have done (and this is a little more invasive, I know), is moved the catch-rethrow up from the ProviderManager to the AuthenticationManager rather than down to the indivual providers. To do this I have created an 'AbstractAuthenticationManager' class that is extended by ProviderManager. The abstract class uses the template pattern to perform a central 'catch-rethrow' setting of the Authentication object. This resolves a problem that was eating at me, which was that the setAuthentication was public in AuthenticationException. Now that this work is done by the AbstractAuthenticationManager the set method can be package scope. Hi Wesley I like it. I've committed it to CVS. Thanks 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=4721&alloc_id=10040&op=click ___ Acegisecurity-developer mailing list [EMAIL PROTECTED] https://lists.sourceforge.net/lists/listinfo/acegisecurity-developer
RE: [Acegisecurity-developer] Suggestions for changes to AbstractProcessingFilter
Hi Ben, > Hi Wesley > > If you'd like to submit patches which take advantage of the enhanced > AuthenticationException.getAuthentication() method, I'd be pleased to > apply them to CVS. > > Best regards > Ben OK, I started an attempt at adding the setting of the Authentication object right down at individual 'AuthenticationProvider' level. This became very problematic. Consider CasProxyTicketValidator, the 'validateNow' method of this class throws BadCredentialsException and AuthenticationServiceException, both of which would benefit from containing the failed Authentication. However, I had to trace back up through confirmTicketValid and into CasAuthenticationProvider.authenticateNow until there was an Authentication object in scope. This would require the 'catch-rethrow' approach and I suspect this would be the case elsewhere. This catch rethrow code would spread throughout the code and it would be very easy to forget to include it when adding another provider. What I have done (and this is a little more invasive, I know), is moved the catch-rethrow up from the ProviderManager to the AuthenticationManager rather than down to the indivual providers. To do this I have created an 'AbstractAuthenticationManager' class that is extended by ProviderManager. The abstract class uses the template pattern to perform a central 'catch-rethrow' setting of the Authentication object. This resolves a problem that was eating at me, which was that the setAuthentication was public in AuthenticationException. Now that this work is done by the AbstractAuthenticationManager the set method can be package scope. There may be other benefits to having this AbstractAuthenticationManager class in that now there is a central place in which yourself or contributers can put code that is required on authentication regardless of the AuthenticationManager used. How do you feel about this? Regards Wesley Hall AbstractAuthenticationManager.java Description: Binary data AuthenticationException.java Description: Binary data ProviderManager.java Description: Binary data MockAuthenticationManager.java Description: Binary data
Re: [Acegisecurity-developer] Suggestions for changes to AbstractProcessingFilter
Wesley Hall wrote: 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. Hi Wesley If you'd like to submit patches which take advantage of the enhanced AuthenticationException.getAuthentication() method, I'd be pleased to apply them to CVS. 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=4721&alloc_id=10040&op=click ___ Acegisecurity-developer mailing list [EMAIL PROTECTED] 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=4721&alloc_id=10040&op=click ___ Acegisecurity-developer mailing list [EMAIL PROTECTED] https://lists.sourceforge.net/lists/listinfo/acegisecurity-developer
Re: [Acegisecurity-developer] Suggestions for changes to AbstractProcessingFilter
Hi Wesley Thanks for the contribution. Wesley Hall wrote: Hi Ben, I have made some changes to the attached classes... AbstractProcessingFilter - authenticationServiceFailureUrl - AuthenticationServiceException authenticationCredentialCheckFailureUrl - BadCredentialsException authenticationDisabledFailureUrl - DisabledException authenticationLockedFailureUrl - LockedException authenticationProxyUntrustedFailureUrl - ProxyUntrustedException authenticationUsernameNotFoundFailureUrl - UsernameNotFoundException 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. AuthenicationException --- I have added an 'Authentication' attribute to the exception. I've also committed this one. ProviderManager --- I have wrapped the calls to provider.authenticate in a try block that catchs AuthenticationException. This is so I can intercept the exception to populate the authentication object into the Exception, it is then rethrown. Also the final ProviderNotFoundException is created first so that the authentication may be set before the instance is thrown. An implication of this approch is that custom implementations of AuthenticationManager will need to do their own work to populate the exception with the authentication object. I did not see this as a major problem as there seems to be little reason to create a custom AuthenticationManager rather than a AuthenticationProvider. An alternative might be to make AuthenticationManager an abstract class and use the template pattern to move the authentication population up into AuthenticationManager but this change was too invasive for my taste. I have performed some tests of these changes by integrating them into my project and doing some functional tests. Everything seems to work ok for me. Perhaps you would like to integrate these changes into the next version of acegi. If not, perhaps somebody on the list has similar requirements to me and would like to include these changes in their project. In either case, please accept these changes as is and distributed under the apache license as the rest of the acegi code. 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? Thanks again for your contributions and feedback, Wesley. 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=4721&alloc_id=10040&op=click ___ Acegisecurity-developer mailing list [EMAIL PROTECTED] https://lists.sourceforge.net/lists/listinfo/acegisecurity-developer
RE: [Acegisecurity-developer] Suggestions for changes to AbstractProcessingFilter
Hi Ben, I have made some changes to the attached classes... AbstractProcessingFilter - I have added some properties to the class. Each of the properties represents a URL that the user is redirected to in the event that the AuthenticationManager implementation throws a specific subclass of AuthenticationException. authenticationServiceFailureUrl - AuthenticationServiceException authenticationCredentialCheckFailureUrl - BadCredentialsException authenticationDisabledFailureUrl - DisabledException authenticationLockedFailureUrl - LockedException authenticationProxyUntrustedFailureUrl - ProxyUntrustedException authenticationUsernameNotFoundFailureUrl - UsernameNotFoundException The catch block in the doFilter method catches 'AuthenticationException' and instanceof checks are performed along with a check to dermine if the corresponding property is null. If the exception type is found and the corresponding URL is not null, the user is redirected to that URL. If none of the checks match then the current 'authenticationFailureUrl' is used. I didnt use multiple catch blocks to do this switching because the instanceof enabled me to reuse the existing code in the existing catch block. AuthenicationException --- I have added an 'Authentication' attribute to the exception. This attribute is not mandatory and not set in the constructor. It is set at a later time. It could be placed in the constructor but this would involve changing all the subclasses and all the places the subclasses are instansiated. ProviderManager --- I have wrapped the calls to provider.authenticate in a try block that catchs AuthenticationException. This is so I can intercept the exception to populate the authentication object into the Exception, it is then rethrown. Also the final ProviderNotFoundException is created first so that the authentication may be set before the instance is thrown. An implication of this approch is that custom implementations of AuthenticationManager will need to do their own work to populate the exception with the authentication object. I did not see this as a major problem as there seems to be little reason to create a custom AuthenticationManager rather than a AuthenticationProvider. An alternative might be to make AuthenticationManager an abstract class and use the template pattern to move the authentication population up into AuthenticationManager but this change was too invasive for my taste. I have performed some tests of these changes by integrating them into my project and doing some functional tests. Everything seems to work ok for me. Perhaps you would like to integrate these changes into the next version of acegi. If not, perhaps somebody on the list has similar requirements to me and would like to include these changes in their project. In either case, please accept these changes as is and distributed under the apache license as the rest of the acegi code. Regards Wesley Hall > -Original Message- > From: [EMAIL PROTECTED] > [mailto:[EMAIL PROTECTED] Behalf Of > Wesley Hall > Sent: 21 July 2004 20:26 > To: [EMAIL PROTECTED] > Subject: 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 n
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... /logon.jsp?error=1 /logon.jsp?error=2 /logon.jsp?error=3 /secure/index.html /j_acegi_security_check ...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 > >place the authentication object (that failed) into the session. If the > >failure pages are dynamic then the fai
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 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". A challenge with this would be to obtain a "failed" Authentication object. Subclasses of AuthenticationProcessingFilter implement a "public abstract Authentication attemptAuthentication(HttpServletRequest)" method which is responsible for returning the successful Authentication or throwing a relevant exception. Of course there are ways to do it (mandate the attempted Authentication inside the returned exception, have subclasses place the attempted Authentication in a well-known location etc) but it would require subclass-level code changes. Your more informative error message requirement can presently be accommodated via the use of more detailed exception messages. The exception causing a failed authentication is put into the HttpSession against the ACEGI_SECURITY_LAST_EXCEPTION_KEY attribute. So if you wanted the sort of details provided in your example, you could modify your AuthenticationManager (or more likely, a custom DaoAuthenticationProvider subclass) to provide this extra level of information. HTH 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=4721&alloc_id=10040&op=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=4721&alloc_id=10040&op=click ___ Acegisecurity-developer mailing list [EMAIL PROTECTED] https://lists.sourceforge.net/lists/listinfo/acegisecurity-developer
