Joachim provided some feedback about the code specifically, but I think an important question is:
Why write a Spring HandlerInterceptorAdaptor instead of using the existing filters. What does this provide? What does it solve that the existing filters didn't? The existing filter solution allows for configuration in both the web.xml, JNDI, and Spring Bean Configuration. The community has collectively developed and guided this client, and its pretty robust. As Joachim pointed out the adaptor appears to be lacking in some areas. Beyond that, the adaptor appears to be missing critical features including integration with the existing JEE security model and the ability to execute before other filters. Running a custom filter pretty much kills any help the community could provide in debugging and troubleshooting. No one is going to take the time to learn someone's custom code because filters offended them (or whatever the reason is) Even better integration with Spring can be achieved through Spring Security which builds on the CAS client (the Spring Security release has an example CASified application) (even the Spring team doesn't really recommend doing security through handlerinterceptors). This seems to be a classic case of NIH syndrome ( http://en.wikipedia.org/wiki/Not_Invented_Here) though I may be missing something. If there's something lacking in the CAS client I'd rather us collectively improve the CAS client. On Wed, Jul 14, 2010 at 4:11 PM, Joachim Fritschi < [email protected]> wrote: > Hi Bryan, > > Am 14.07.2010 20:25, schrieb Bryan Wooten: > > The following is from a Spring HandlerInterceptorAdaptor. The developer >> claims this is sufficient to CASify his application without the need for >> web.xml filter configuration. I believe this design is woefully >> inadequate and does not truly CASify the application. Unfortunately this >> design has gained traction amongst some developers and I need strong >> reasons why developers should use CAS as designed using the CAS client >> and its filters. If I am wrong and this is a sufficient design I would >> like to know. >> > i think in principal the client does what most other non java cas clients > do in other languages. (phpcas,ruby,python etc.) > > The web.xml filter has the charm and added security bonus that it is > actually a transparent filter between your app and the evil internet. You > seperate your security login logic from the fancy app logic which might be > modified constantly. This is a big bonus for the container solutions like > web.xml and mod_auth_cas for example. These filters fully secure everthing > inside the container while adding the code inside your application means you > yourself have to ensure that everything is protected. I have seen many > developer fail in this area. But this is more a personal preference and is a > matter of opinion i guess. > > Now from the logic i see a few flaws that could cause problems and _might_ > even be security related: > > > //See if they already have a session. If so, return true. >> >> if (CasSessionManager.isAuthenticated(request)) { // this simply checks >> for a session parameter that we put on the session after cas login, it >> is the principal (unid) >> >> Logger.getLogger(CasInterceptor.class.getName()).log(Level.FINE, "Found >> session, allowing request."); >> >> return true; >> >> } //Check to see if they are attempting to validate a ticket. This would >> be evident by a param named ticket. >> >> else if (request.getParameter("ticket") != null) { >> >> Logger.getLogger(CasInterceptor.class.getName()).log(Level.FINE, "Found >> ticket, validating..."); >> >> //Validate the ticket >> >> String myTicket = request.getParameter("ticket"); >> >> try { >> >> Assertion casAssertion = this.ticketValidator.validate(myTicket, >> localServiceUrl); >> >> String unid = casAssertion.getPrincipal().getName(); >> >> //create a session >> >> CasSessionManager.setUnid(request, unid); >> >> //If the ldap factory is present, attempt to retreive attributes from it, >> >> //and set them as a user on the session as well. >> >> if (this.ldapFactory != null) { >> >> CasSessionManager.setUserPrivate(request, >> this.ldapFactory.getPrivateUserByUnid(unid)); >> >> } >> >> //This is where the many implementing web-apps have an opporunity to do >> extra stuff after the login procedure succeeds. >> >> if (this.loginManager != null) { >> >> this.loginManager.completeLoginProcedure(unid, request); >> >> } >> >> Logger.getLogger(CasInterceptor.class.getName()).log(Level.FINE, >> "..Validation passed. Creating session."); >> >> } catch (TicketValidationException e) { >> >> Logger.getLogger(CasInterceptor.class.getName()).log(Level.FINE, >> "..Validation failed, directing to login page."); >> >> //If they are not able to validate the ticket, send them away. >> >> response.sendRedirect(this.casLoginUrl + "?service=" + localServiceUrl); >> > localServiceUrl should be urlencoded. Can't tell if it is or not from here. > This should be done especially if you just take the RequestURI or something > like that. Otherwise you could maybe do header injections and maybe > something else? > > >> return false; >> >> } catch (UnauthorizedUserException e) { >> >> Logger.getLogger(CasInterceptor.class.getName()).log(Level.FINE, "User >> was un-authorized. directing to login page."); >> >> //If they are not able to validate the ticket, send them away. >> >> response.sendRedirect(this.casLoginUrl + "?service=" + localServiceUrl); >> > Same here for urlenoding. > >> >> return false; >> >> } >> > There should be addition catch(Exception){return false}. You don't want > some uncaught exception to fire back from here. This might be caught in some > other place an cause who knows what. > > >> return true; >> >> } //Looks like they do not have a session, and they are not trying to >> validate a ticket, send them to the login screen. >> >> else { >> >> Logger.getLogger(CasInterceptor.class.getName()).log(Level.FINE, "No >> ticket, No Session. redirect to login"); >> >> //First save any params for later use. This may create a sesson, but it >> is not a validated session. >> >> CasSessionManager.setRequestParams(request); >> > Saving request parameters during an unvalidated session? I don't have a > good feeling here. Not that i can actually tell you why but it somehow seems > unwise imho. > > >> //send them to the login screen. >> >> response.sendRedirect(this.casLoginUrl + "?service=" + localServiceUrl); >> > urlencoding again? > >> >> return false; >> >> } >> >> } >> > > Cheers, > > Joachim > > > -- > You are currently subscribed to [email protected] as: > [email protected] > To unsubscribe, change settings or access archives, see > http://www.ja-sig.org/wiki/display/JSG/cas-user > -- You are currently subscribed to [email protected] as: [email protected] To unsubscribe, change settings or access archives, see http://www.ja-sig.org/wiki/display/JSG/cas-user
