Add remember-me support for CAS

[leleuj]

For SEC-1986.

I have signed and agree to the terms of the SpringSource Individual Contributor License Agreement.

[rwinch]

Thank you for your contribution. Overall I think the changes look pretty good. I do have some structural changes that I think we should make in order to clean things up a bit. Keep in mind I am open to constructive criticism on my suggestions as well.

• I noticed the code is using @since 3.1.1. Since this is a fairly significant enhancement it will be targeted at 3.2 (or likely 3.2 M1 once that version is added to JIRA). We will want the code to specify @since 3.2.0
• Commit styling
  • All commits should be prefixed with the JIRA ID so that they are appropriately linked to the associated JIRA. For example, you would use something like (SEC-1986: Added CAS remember me support)
  • Ensure your code is formatted appropriately and only edits what is necessary. Some things to look for is you should not have any tabs (use spaces), no trailing whitespaces, etc. If you use Eclipse the AnyEdit plugin can help a lot with this. You can also use .git/hooks/pre-commit.sample hooks to ensure you did not miss anything. You may have already done this, but just thought I would give you a heads up so if you had not you could do this in the upcoming changes.
  • While it makes sense to keep the commits broken up before you know what is going to be used, I would aim for a single-ish commit per JIRA so squash commits as necessary (you might have a local branch that keeps them separate and then squash on another branch before pushing).
  • Ensure to rebase commits rather than merging (this keeps a linear commit history)
• I am guessing one of your goals was to keep the changes isolated as much as possible. However, I believe this enhancement request the need for an improvement on how RememberMe works in general. I have created SEC-1987 to address this. You will need to implement these changes first. See the JIRA for a description and read it before moving on (as my other comments won't make sense until after reading it).
• CasAuthenticationToken will implement RememberMeAware
  • Remove CasAuthenticationTokenEvaluator since it will no longer be necessary
  • Remove CasRememberMeAuthenticationTrustResolverImpl since it will no longer be necessary
  • Remove CasRememberMeBeanPostProcessor as it is no longer necessary
  • Revert changes to DefaultWebSecurityExpressionHandler since they are no longer necessary
• CasRememberMeAccessDeniedHandlerImpl is something that is a bit too application specific. If others wish they can certainly implement this, but I do not think it should be provided.
• While I have suggested to remove CasAuthenticationTokenEvaluator we do need to decide on what the default attribute that will be used for remember me in the CasAuthenticationToken. Is there a reason for choosing longTermAuthenticationRequestTokenUsed? I am wondering if we should give this attribute name a bit of thought or explain why you have chosen it (perhaps you have already given in thought).
• Feel free to reuse the same pull request for both issues (just ensure distinct commits with appropriate JIRA ID's) or create a new pull request for the new JIRA (whichever you prefer).

Once again thank you for taking time to contribute back to the community.

[leleuj]

Thank you for taking time to review this pull request.

I agree with the targeted version : 3.2.0, the commit message format and the need to squash all commits associated to the same JIRA.
Regarding code formatting, I use tabs instead of spaces on purpose, trying to follow Spring framework contributor guidelines (https://github.com/SpringSource/spring-framework/wiki/Contributor-guidelines) which says "Tabs, not spaces". That said, I have no concern about using spaces instead of tabs.

I read carefully SEC-1987 and I understand it : my changes looked like a bit a trick just for CAS and it's better to have a global solution for remember-me. Nonetheless, I think we can't remove all classes straight-forward.

The problems I see :

1. The name of the CAS remember-me attribute saying that we are in a remember-mode is really longTermAuthenticationRequestTokenUsed. I proposed something shorter and more regular but to avoid overlap, CAS team choose to use this longer name. Here is the JIRA : https://issues.jasig.org/browse/CAS-973.
   So it's the name of the property defined on CAS server side which can be overriden. That's why I created a CasAuthenticationTokenEvaluator class which has the logic of remember-me computation for CAS, taking into account this settable property name.
   If we want to get rid of the CasAuthenticationTokenEvaluator and make CasAuthenticationToken implement RememberMeAware interface, we will need to set the longTermAuthenticationRequestTokenUsed property name value at creation, i.e. in the CasAuthenticationProvider. Is it what you had in mind ?

2. The CasRememberMeAccessDeniedHandlerImpl class looks like a bit application specific, you're right but there is specific application behaviour in access denied for CAS. In the handleSpringSecurityException method of the ExceptionTranslationFilter class, the logic is the following : if you're anonymous, you'll start the authentication process, if you're not, you use the default AccessDeniedHandlerImpl() which returns an error page (custom or 403 HTTP). In the specific case of CAS authentication, even if you are not anonymous, you can need to start a new authentication (the "new" part is handled by using the CAS renew parameter) if you are a remember-me CAS token. So I don't see a way to do without this class yet.

3. This follows the previous point. If a specific access denied handler is required for CAS remember-me, in case of spring security configuration done with the tag, we need a bean post processor to change the default access denied handler by a "CAS remember-me" one.

When discarding all my changes to restart working on SEC-1987, I closed this pull request by mistake. So I will open a new one for changes regarding SEC-1987 and wait for your feedback.

[rwinch]

Thank you for your prompt and detailed response. No problem on the code formatting thing. In short, each module within Spring operates slightly differently. The lack of documentation is something that is my fault for not having well documented and my note was a work around for the problem. I have created SEC-1988 to formally address this problem.

1. Thank you for explaining the thought process for the remember me attribute. This makes perfect sense and I agree with the name since this is the CAS standard. I have listed my initial thoughts on how to determine the CAS Authentication is remember me below. I haven't had a lot of time to think about this as I am trying to focus on the next release, so feedback is quite welcome.
   • The first option is to leave it as something that is not configurable for now. So long as we encapsulate this well, we should be able to modify the behavior later. Typically I find it good to keep things lean and add them when required. In this instance we could just have CasAuthenticationToken always use the same attribute. However, if you happen to need the ability to configure the attribute I view this as a non-option as you are the one contributing and we want it to work for you.
   • The second option seems aligns with your suggestion. We would add an attribute that could be set on the CasAuthenticationProvider that would determine which attribute would be used. The CasAuthenticationProvider would be in charge of reading the attribute from the assertion and then setting an additional attribute isRememberMe on the CasAuthenticationToken based upon the attribute in the Assertion.
2. The CasRememberMeAccessDeniedHandlerImpl assumes that they want to trigger a re-authentication when access is denied. Perhaps they just want to display an error page (which is all that is provided with Spring Security at the moment for non-CAS applications). Additionally, this is something that is very well abstracted and can be added with relative ease if someone actually wants this behavior.
3. If someone wants to use a custom AccessDeniedHandler, they can use the access-denied-handler@ref. Wouldn't this work rather than needing the BeanPostProcessor?

[leleuj]

1. I think it is good idea to be able to change the name of the remember-me attribute to mirror on Spring Security side what is possible on CAS server side. In addition, I see two reasons to keep the posibility to change this attribute name :

• there can be more discussion on the remember-me feature on CAS side and the name could be changed (a more global refactoring is planned on levels of authentication)
• for CAS users having old versions of CAS server, they can't use SAML validation and can only enable remember-me feature by some customization where they can choose the name of the remember-me attribute.

I would definitely choose the second option : it's not too much change and well designed.

2. I think we have trouble understanding each other and agreeing on this point. You're right : re-authentication is not always needed but I think that it's the most frequent and regular use case. Moreover, I'm not sure that users would be able to add this behaviour on their own as it relies on the CAS renew parameter which is not really known.

We can maybe get rid of the CasRememberMeAccessDeniedHandlerImpl class, but we can't remove the posibility to configure easily the need to re-authenticate if previously authenticated in CAS remember-me mode.

It also depends on the UserDetailsService used, if it grants or not different roles according to the remember-me nature of the CAS authentication token. It must be a pretty unsual use case though.

I guess that you don't like using the "access denied handler" concept as an entry point but right now, I don't see a better solution than a dedicated access denied handler for CAS remember-me feature.

If we assume that the UserDetailsService bean grants roles without taking into account the remember-me nature of the CAS token, the problem is that at the ExceptionTranslationFilter level, I don't know which vote has failed to lead to access denied, if I was sure that a vote regarding "isFullyAuthenticated" or "isRemembered" has failed, I would be sure that a re-authentication is necessary for a CAS remember-me token.

Sorry for being so long in my explanations.

3. Perfect. I didn't know this configuration possibility, so you're right, we don't need a bean post processor.

[rwinch]

No problem with the long explanations; they are necessary at times for effective collaboration :) I am still not convinced that CasRememberMeAccessDeniedHandlerImpl should be included. Perhaps it is because I am having trouble understanding a few of your points.

It also depends on the UserDetailsService used, if it grants or not different roles according to the remember-me nature of the CAS authentication token. It must be a pretty unsual use case though.

I'm not sure what you are getting at here is this related to the fact that you think that the CasRememberMeAccessDeniedHandlerImpl should be included? Is this just a general problem that you feel still needs to be solved?

If we assume that the UserDetailsService bean grants roles without taking into account the remember-me nature of the CAS token, the problem is that at the ExceptionTranslationFilter level, I don't know which vote has failed to lead to access denied, if I was sure that a vote regarding "isFullyAuthenticated" or "isRemembered" has failed, I would be sure that a re-authentication is necessary for a CAS remember-me token.

Same comment as above.

Keep in mind I am still very reluctant to include this feature but thought I would let you know some initial feedback on this. If we were to include CasRememberMeAccessDeniedHandlerImpl I would suggest a few changes.

• I realize the current AccessDeniedHandler has Impl in it, but I tend to shy away from adding any more classes with Impl in them. I would truncate the Impl off (rename to CasRememberMeAccessDeniedHandler) as the CasRememberMe prefix is a descriptive way of stating what type of AccessDeniedHandler it is.
• The mechanism for logging out should probably be using LogoutHandler implementation(s) rather than trying to implement Logout mechanisms directly.
• I realize the goal of the CasAuthenticationEntryPoint setter is to ensure that the renew parameter works properly. However, the implementation does not work if someone provided a subclass of CasAuthenticationEntryPoint or a subclass of ServiceProperties on the CasAuthenticationEntryPoint.

[leleuj]

To make things clearer, I tried to detail the use cases where the CasRememberMeAccessDeniedHandler is required if you use CAS authentication and remember-me mode, and therefore should be included in the Spring Security configuration of the element :

1. if you have security configuration (and therefore vote) regarding roles and if the UserDetailsService creates roles depending on remember-me nature of the CAS token
2. if you have security configuration (and therefore vote) regarding "isRemembered" or "isAuthenticated".

I don't see any better solution than this CasRememberMeAccessDeniedHandler, but as you look(ed) not convinced, I tried to give you as much information as possible to help you find maybe a better solution.

That being said,

a) About naming : I don't have any problem removing the Impl suffix.

b) About logout, I copied and pasted what is done in the ExceptionTranslationFilter : put null in the Authentication in the security context + save request + call the commence method of the entry point. You told me about using the real logout handler. I think that it's not a good idea : logout destroys web session and in this case (being in CAS remember-me mode and wanting to re-authenticate at CAS server), the user session should not be destroyed (the user should not lose data in its web session) but security information should be overriden by new ones provided by the new authentication. I think that I should just save the request + call the commence method of the entry point.

c) About the renew parameter and the copy mechanism, you're right : that's ugly and not really working if there is some inheritance (I won't be able to make a complete copy of the objects). I would propose a better solution :

• add a specific commence method for the CasAuthenticationEntryPoint with an additionnal boolean parameter forceRenew
• copy the code of the regular commence method in the new commence method
• make the regular commence method call the new commence method with forceRenew=false
• add behaviour in the new commence method to force the value of the renew parameter to true if forceRenew is true.
  In CasRememberMeAccessDeniedHandler, I would call the new commence method with forceRenew=true.

[rwinch]

Why don't I take a look at what you have with the proposed changes. Can you post the code with your updated changes? Perhaps at that I will see it more your way. If not, we can continue the discussion from there.

[leleuj]

Here we go : I committed all changes and sent pull request #14. I messed up again with git in squashing properly commits that's why pull request #12 was closed.

I'm pretty happy with the results : it looks well designed and well integrated in Spring Security. I followed at best what we agreed upon and what I proposed.

I did realize my first proposal has a bug as the isRememberMe() method was acting like the isAuthenticated() method.

I added several unit tests and two integration tests you can call with : gradlew spring-security-samples-cassample:integrationTest.

I'm looking forward to your feedback.