SEC-1987 & SEC-1986 #14

leleuj · 2012-07-06T18:57:33Z

SEC-1987:

I create the RememberMeAware interface with the isRemember() method
I make the RememberMeAuthenticationToken class implement this interface with a final isRemember() method which always returns true
I change the isRememberMe() method behaviour of the AuthenticationTrustResolverImpl class to test if the authentication is an instance of the RememberMeAware interface and in this case, return true if the isRemember() method returns true too. This test is made after the "old" ones I keep for backward compatibility reasons
I make the TestingAuthenticationToken class implement the RememberMeAware interface for testing
I update the AuthenticationTrustResolverImplTests class to test the RememberMeAware implementation.

SEC-1986:

I update the CasAuthenticationProvider class with rememberMeAttributeName and also the CasAuthenticationToken
I add a new commence method with a forceRenew parameter in the CasAuthenticationEntryPoint class
I add the CasRememberMeAccessDeniedHandler class to handle CAS remember-me authentication when access is denied
I update associated unit tests (AbstractStatelessTicketCacheTests, CasAuthenticationProviderTests, CasAuthenticationTokenTests, CasAuthenticationEntryPointTests)
I configure CAS server to support remember-me mode
I update CAS sample to add integration tests regarding remember-me (CasSampleRememberMeTests)

rwinch · 2012-07-09T21:16:51Z

Thanks for submitting the updates. I have provided some additional feedback below.

Anything a user may be interested in is typically logged as a distinct JIRA. This ensures our changelog reflects all the work that has been done. Can you please remove the updates to the CAS Server dependencies? If it is necessary for these features we can create a distinct JIRA and commit for it.
I am still not convinced we should include CasAccessDeniedHandler
- User's will likely need a way to know what happened. For example, they will likely want to know why they got a login prompt in the middle of their workflow despite being active in the application. There is no standard means of communicating the failure to the CAS Server so it can be displayed.
- The CasAccessDeniedHandler does not differentiate between access denied due to the user is anonymous and a missing role role (or any other type of failure). Consider trying to access the extreme secure page as scott / scott when scott is "remembered". In all likelihood, he should not be sent back to the CAS Server.
- In summary, solving this is rather complex and is likely necessary to be done on an app by app basis. We do not actually know the common use cases without concrete evidence.
It seems like there should probably be some refining of the Remember Me of the CAS Server. Consider the following:
- Select the Remember Me checkbox and login with scott / scott
- Logout of the application locally (stay signed into CAS) and navigate to isFullyAuthenticated page
- The user is marked as Remember Me. This seems weird that the user is so quickly indicated as Remember Me.
In the situation above, there appears to be no reason that their credentials should be downgraded (i.e. marked as Remember Me). Typically remember me indicates one of a few things has happened:
- The user has closed the browser, opened the application again, and been signed in
- The user has been inactive for an extended period of time (i.e. longer than a standard session timeout)
If we follow the exact same flow without checking remember me the user is able to access the isFullyAuthenticated page successfully. This mismatch probably means we will need some updates to ensure this feature provides the maximum value and aligns with the existing remember me Spring Security feature. I understand this likely reflects changes to the CAS Server setup, but we want to make sure all the pieces fit together before adding functionality to Spring Security.

Thanks again for all your hard work and for your patience as we refine this feature.

Cheers,
Rob

SEC-2004: Upgrade CAS server to last release version : 3.5.0

leleuj · 2012-07-13T19:08:35Z

For CAS server dependency, I created SEC-2004, I added it in the commit message.
I keep on hoping to convince you.
You're right, they should know on CAS server that it is not an authentication, but a re-authentication. The renew parameter used in the /login url notifies CAS server of that and I planned to update CAS server to have a different behaviour in case of re-authentication. I'll post a message to the CAS dev mailing list about that in the next few days.
You're pointing an interesting problem : we don't know which vote has failed and that's why it's up to the developper to know if the CasRememberMeAccessDeniedHandler is needed for security configuration, if the user should be redirected to CAS server when in remember-me mode and after a failed vote.
Yes, there can be quite complex use cases. Each time role accesses are involved, the CasRememberMeAccessDeniedHandler is needed if the UserDetailsService can grant different roles depending on the remember-me nature or not of the CAS authentication. Every time isRemembered and isAuthenticated are used in security configuration, the CasRememberMeAccessDeniedHandler is required if CAS server supports remember-me. If roles and isRemembered/isAuthenticated are used, it can be hard to define that should be the right behaviour. That's why the best solution is to document everything and let the developer choose the right configuration on app by app basis.
I'm not surprised by this use case : if you're logged in CAS remember-me mode, even if you locally logout from your web app, you're still connected at SSO level and you will be automatically authenticated in remember-me mode in your web app.
Exactly the same will happen in regular CAS authentication : call a protected page, logout locally, call the same protected page again, you're still logged in.
It's SSO that brings such "surprising" behaviour.
For me, the local logout is the same as closing the browser : there is an action from the user to say that he wants to quit the application. In this case, the user can't be considered anymore as fully authenticated : the web app session has been closed and the web session started after logout will be a new one, that's why the authentication in CAS server is marked as remember-me (the "strong" authentication happened in the previous web app session, not in this new one). I would say that it's a normal and safe behaviour, not a downgraded one. By the way, if the user tries to access a protected page (requiring fully authentication), the user will be redirected to CAS server thanks to the CasRememberMeAccessDeniedHandler which is therefore necessary.

I gave you all the information and ideas I have on my side : what do you expect from me to merge this pull request ? what is the next step ?
Thanks.
Jérôme

rwinch · 2012-07-16T16:01:30Z

For CAS server dependency, I created SEC-2004, I added it in the commit message.
I didn't make this very clear so I can do it this time, but typically a JIRA will have it's own distinct pull request. The reason is we can accept the cas-server updates before we figure out this JIRA. I will go ahead and cherry-pick your commit and push it out to master. You will need to rebase your commits once we are ready for them anyways.

You're right, they should know on CAS server that it is not an authentication, but a re-authentication. The renew parameter used in the /login url notifies CAS server of that and I planned to update CAS server to have a different behaviour in case of re-authentication. I'll post a message to the CAS dev mailing list about that in the next few days

The renew parameter indicates that it is re-authentication but it does not state why.

You're pointing an interesting problem : we don't know which vote has failed and that's why it's up to the developper to know if the CasRememberMeAccessDeniedHandler is needed for security configuration, if the user should be redirected to CAS server when in remember-me mode and after a failed vote.

The problem is that the CasRememberMeAccessDeniedHandler does not even know why it failed. It only knows that authorization failed and that the user is marked as remember me. It also means that this is more of an edge case if they are using CAS remember me and may not be using this handler.

Exactly the same will happen in regular CAS authentication : call a protected page, logout locally, call the same protected page again, you're still logged in. It's SSO that brings such "surprising" behaviour.

I am not surprised by SSO. I am surprised that the user is marked as remembered.

The user requests access to a cas service. The user is sent to the login page
The user checks remember me and logs in. The user is logged into the CAS Service
The user request access to another cas service (or the same one after local logout) and the user is marked as remember me.

It seems inconsistent that the user is marked as remember me here. Typically remember me does not take place until after a session has timed out or until the browser has closed. In this instance the user was just active with the CAS server and they have not closed their browser. Why should the user have lower level privileges in this instance?

For me, the local logout is the same as closing the browser : there is an action from the user to say that he wants to quit the application. In this case, the user can't be considered anymore as fully authenticated : the web app session has been closed and the web session started after logout will be a new one, that's why the authentication in CAS server is marked as remember-me (the "strong" authentication happened in the previous web app session, not in this new one). I would say that it's a normal and safe behaviour, not a downgraded one.

I perhaps could be persuaded on this point. The difference to me is that one is an explicit action and the other is implicit. Regardless, this still does not explain that if you have multiple applications why the second application is considered remember me.

rwinch · 2012-07-16T16:32:52Z

When trying to update the Spring Security CAS version I noticed a few more things:

Pull requests should be based off topic branches it will make things easier for both of us in the long run
Your commits should not merge in my changes they should rebased (this will be easier if you are using a topic branch)
Each JIRA should have its own commit

Note: If you wan the gory details you can refer to the Spring Security Contributor Guidelines

Obviously I can update the CAS Server version myself since it is a simple change, but then you wouldn't get credit in the commit logs :) If you want credit please make this a distinct commit, on it's own topic branch, it's own pull request, etc.

leleuj · 2012-07-18T18:11:17Z

As I commited both updates at the same time (tests and CAS server update), I changed the commit message with both JIRA referenced inside. But I notice all recommendations on topic branches.

Do we really need to know if it's about remember me on CAS server side ? I don't see the need.

The use of the CasRememberMeAccessDeniedHandler has to be configured according to the rest of the security configuration, it may (will ?) be tricky.

OK, I got it now. I completely missed the point. I'm sorry about that.
You're right, second application should not be in remember-me mode, that's not what I expected and when I checked the code in CAS server, I saw that only the first application accessed is not in remember-me mode.

I will pause on SEC-1986 until I find a good solution on CAS server side.
I let you know. Thanks for you help.

rwinch · 2012-07-19T13:44:06Z

The updates for remember me (CAS server or CAS client/service) can be in a single commit. When I group my commits I try to think what is the theme of this commit (this one would be remember me support). If it doesn't fit in that theme, it should be in its own commit (i.e. CAS Server update). Another reason for a distinct commit is if it has value outside of the theme (i.e. RememberMeAware refactoring). Commits can be a bit more of the art side (i.e. not right/wrong) than the science side of programming. However, I try to stick to those rules for breaking up commits to make it easier to identify new features and to leverage tools like git bisect when issues do come up.

battags · 2012-07-20T07:32:05Z

Please note that the server isn't in "remember me mode". We have no "mode". What we have is, hey "this person requested a long term session, and since its not the initial sign on, we're letting you know they requested that." If you care, do something about it.

Since each client has their own definition of at what point something is "remembered" and thus less trustworthy, they should be checking the time elapsed since the authentication. The flag is merely there to assist in determining whether they should bother to check.

leleuj · 2012-07-20T08:24:17Z

A link on CAS discussion : https://lists.wisc.edu/read/messages?id=18419320

rwinch · 2012-07-24T02:54:26Z

@battags @leleuj

I think that the new definition of remember me aligns with what I would have expected. It sounds like we all agree on this new definition, so the next step would be to update the pull request to correspond to this new definition. Specifically, we would need to

Add the authentication creation time to the CAS response of our sample CAS Server. If there is a standard attribute name and format we would probably want to use that.
We would then need to update the Spring Security CAS client code to be able to look for that authentication and depending on a configured amount of time specify the Authentication as Remember Me.
Given that there is no "remember me mode" we can probably simplify the code a bit by removing the check for the longTermAuthenticationRequestTokenUsed but instead look for the creation of the authentication and compare that.

Let me know if this seems to align with what you both feel should be done.

PS: Sorry for the delayed response. I had typed something up, but must have not sent it out and closed the tab instead. My apologies again.

leleuj · 2012-07-24T16:58:26Z

Looks good to me : first, I will work on CAS server side to return authentication creation time and, as soon as it will be ok, I will create a new pull request on Spring Security side.

battags · 2012-07-24T17:02:18Z

The SAML response should already be including authentication time. Is it not?

leleuj · 2012-07-24T18:03:56Z

You're right again Scott : it's included in the SAML response, but it's not pushed into the Assertion object...

battags · 2012-07-24T18:13:17Z

You mean the assertion in the CAS Java Client? If its not, if you open a JIRA issue I'll get it in there. I'm working through a number of other JIRA issues anyway :-)

leleuj · 2012-07-24T18:15:36Z

Right, I will open a JIRA and send a pull request (it's a small change)...

battags · 2012-07-24T18:16:27Z

If you wouldn't mind not sending a pull request, just yet, that would be good. I have a commit out to upgrade us to OpenSAML2 so your change won't work once that is merged in :-)

leleuj · 2012-07-24T18:23:01Z

No problem. I opened JIRA CASC-185. Let me know when I can work on the pull request. By the way, let's move back to the JIRA for discussion...

leleuj · 2012-07-25T17:45:48Z

Rob, the change has been done in Java CAS client. I'll be back as soon as it will be released...

rwinch · 2012-07-25T17:59:14Z

Thanks for the update and thanks for your patience as we iron things out. I know there has been quite a bit of back and forth, but I think our collaboration is already paying off; I hope you agree. Thanks again for the update.

leleuj · 2012-07-25T18:06:31Z

No problem. We're addressing very interesting issue and it takes time to find the best solution.

rwinch · 2014-03-18T20:11:07Z

@leleuj I'm just touching base on this Are the proper components released for this to move forward?

leleuj · 2014-03-19T11:01:27Z

I'm still willing to bring the CAS remember-me support in Spring Security. Though, things are taking a lot more time than expected.

Explanations:
CASC-189 is postponed for CAS client 3.4 (in a few months). In any case, it's not completely satisfactory as it would return remember-me information just for the SAML endpoint.
@battags : confident we can put that in CAS client 3.4? I'm committed to re-develop it of course...

As we have now a new endpoint in the CAS server 4.0.0 (/p3/serviceValidate) able to push user attributes, this one will be upgraded to also return remember-me information (apereo/cas#412). Target: CAS 4.1 (in a few months)

Then, we could update the CAS client to retrieve remember-me information both from SAML and /p3 endpoints.

I have no assurance about the planning, but that's the idea...

rwinch · 2014-03-19T20:21:28Z

Thanks for the fast reply @leleuj :) No problem on the timeline. I'm trying to get things prioritized and planned for the next Spring Security release and wanted to make sure I wasn't leaving you hanging on anything. I will wait till I hear back from you on this.

pivotal-issuemaster · 2016-06-15T04:24:09Z

@leleuj Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

rwinch · 2016-07-07T18:33:30Z

@leleuj Any updates on getting this standardized?

leleuj · 2016-07-11T08:53:53Z

Thanks for the reminder. It's now fully feasible as the /p3 endpoint is a standard for CAS and returns a remember-me information.
I don't plan to work on that for now though as I'm more focused on pac4j development.

Update for changes in github pages

vborcea · 2017-02-08T15:34:11Z

cas/src/main/java/org/springframework/security/cas/authentication/CasAuthenticationToken.java

     *
     * @throws IllegalArgumentException if a <code>null</code> was passed
     */
    public CasAuthenticationToken(final String key, final Object principal, final Object credentials,
-        final Collection<? extends GrantedAuthority> authorities, final UserDetails userDetails, final Assertion assertion) {
+        final Collection<? extends GrantedAuthority> authorities, final UserDetails userDetails, final Assertion assertion,


this constructor receives way too many parameters. Maybe consider a builder?

vborcea · 2017-02-08T15:38:56Z

cas/src/main/java/org/springframework/security/cas/authentication/CasAuthenticationToken.java

     *
     * @throws IllegalArgumentException if a <code>null</code> was passed
     */
    public CasAuthenticationToken(final String key, final Object principal, final Object credentials,
-        final Collection<? extends GrantedAuthority> authorities, final UserDetails userDetails, final Assertion assertion) {
+        final Collection<? extends GrantedAuthority> authorities, final UserDetails userDetails, final Assertion assertion,
+        String rememberMeAttributeName) {
        super(authorities);

        if ((key == null) || ("".equals(key)) || (principal == null) || "".equals(principal) || (credentials == null)


Instead of this long if for parameter validations could be used
Assert.notNull(myParameter, "myParamter must not be null"); which is a replacement for already java 8 Objects.requireNonNull()

vborcea · 2017-02-08T15:40:51Z

cas/src/main/java/org/springframework/security/cas/web/CasAuthenticationEntryPoint.java


        final String urlEncodedService = createServiceUrl(servletRequest, response);
-        final String redirectUrl = createRedirectUrl(urlEncodedService);
+        final String redirectUrl = createRedirectUrl(urlEncodedService, forceRenew);


it's not a good practice to pass boolean as parameter, just add another explicit method for this case

vborcea · 2017-02-08T15:43:01Z

...est/java/org/springframework/security/cas/authentication/CasAuthenticationProviderTests.java


        Authentication laterResult = cap.authenticate(result);
        assertEquals(result, laterResult);
    }

    @Test
+    public void testRememberMeDefaultAttributeName() throws Exception {


From the documentation of pull requests we shouldn't make smurfnamings: add preffix or suffix for method names with "test", but just say what we test

vborcea · 2017-02-08T15:43:12Z

...est/java/org/springframework/security/cas/authentication/CasAuthenticationProviderTests.java

+    }
+
+    @Test
+    public void testRememberMeNewAttributeName() throws Exception {


From the documentation of pull requests we shouldn't make smurfnamings: add preffix or suffix for method names with "test", but just say what we test

rwinch · 2021-11-17T19:37:18Z

Closing this as it has not been updated to use the standard CAS support.

SEC-1987: Create RememberMeAware interface

1a6c782

leleuj mentioned this pull request Jul 6, 2012

Add remember-me support for CAS #11

Merged

leleuj added 2 commits July 13, 2012 20:51

SEC-1986: Add remember-me support for CAS

fa2a1f3

SEC-2004: Upgrade CAS server to last release version : 3.5.0

Merge remote-tracking branch 'upstream/master'

c0b1882

leleuj mentioned this pull request Sep 22, 2012

CASC-189 : Handle authentication date returned by custom CAS 2.0 service ticket validation apereo/java-cas-client#19

Closed

leleuj mentioned this pull request Mar 14, 2014

CAS-1413: other CAS-protocol attributes in the validation response apereo/cas#412

Merged

rwinch added the waiting-for-feedback label Mar 18, 2014

rwinch force-pushed the master branch from 9cc7189 to aa28ae7 Compare July 8, 2015 16:46

rwinch force-pushed the master branch from aa28ae7 to aed288d Compare July 8, 2015 16:48

This was referenced Feb 5, 2016

SEC-1986: Add remember-me support for CAS #2209

Open

SEC-1994: Spring security 3.1.1.RELEASE is not JDK 1.5 compatible #2215

Closed

bclozel added a commit to bclozel/spring-security that referenced this pull request Jan 4, 2017

Merge pull request spring-projects#14 from dsyer/gh-pages-upstream

9191438

Update for changes in github pages

vborcea suggested changes Feb 8, 2017

View reviewed changes

rwinch force-pushed the master branch 2 times, most recently from 0e114c6 to fd244eb Compare June 8, 2017 22:26

rwinch force-pushed the master branch from 5737fd7 to 62e7762 Compare November 27, 2017 19:48

rwinch force-pushed the master branch from f8d5f50 to eb067bc Compare April 30, 2018 17:16

marcusdacoregio added status: waiting-for-feedback We need additional information before we can continue and removed status: waiting-for-feedback We need additional information before we can continue labels Jun 10, 2021

rwinch closed this Nov 17, 2021

rwinch added status: invalid An issue that we don't feel is valid and removed status: waiting-for-feedback We need additional information before we can continue labels Nov 17, 2021

rwinch self-assigned this Nov 17, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SEC-1987 & SEC-1986 #14

SEC-1987 & SEC-1986 #14

leleuj commented Jul 6, 2012

rwinch commented Jul 9, 2012

leleuj commented Jul 13, 2012

rwinch commented Jul 16, 2012

rwinch commented Jul 16, 2012

leleuj commented Jul 18, 2012

rwinch commented Jul 19, 2012

battags commented Jul 20, 2012

leleuj commented Jul 20, 2012

rwinch commented Jul 24, 2012

leleuj commented Jul 24, 2012

battags commented Jul 24, 2012

leleuj commented Jul 24, 2012

battags commented Jul 24, 2012

leleuj commented Jul 24, 2012

battags commented Jul 24, 2012

leleuj commented Jul 24, 2012

leleuj commented Jul 25, 2012

rwinch commented Jul 25, 2012

leleuj commented Jul 25, 2012

rwinch commented Mar 18, 2014

leleuj commented Mar 19, 2014

rwinch commented Mar 19, 2014

pivotal-issuemaster commented Jun 15, 2016

rwinch commented Jul 7, 2016

leleuj commented Jul 11, 2016

vborcea Feb 8, 2017

vborcea Feb 8, 2017

vborcea Feb 8, 2017

vborcea Feb 8, 2017

vborcea Feb 8, 2017

rwinch commented Nov 17, 2021

SEC-1987 & SEC-1986 #14

SEC-1987 & SEC-1986 #14

Conversation

leleuj commented Jul 6, 2012

rwinch commented Jul 9, 2012

leleuj commented Jul 13, 2012

rwinch commented Jul 16, 2012

rwinch commented Jul 16, 2012

leleuj commented Jul 18, 2012

rwinch commented Jul 19, 2012

battags commented Jul 20, 2012

leleuj commented Jul 20, 2012

rwinch commented Jul 24, 2012

leleuj commented Jul 24, 2012

battags commented Jul 24, 2012

leleuj commented Jul 24, 2012

battags commented Jul 24, 2012

leleuj commented Jul 24, 2012

battags commented Jul 24, 2012

leleuj commented Jul 24, 2012

leleuj commented Jul 25, 2012

rwinch commented Jul 25, 2012

leleuj commented Jul 25, 2012

rwinch commented Mar 18, 2014

leleuj commented Mar 19, 2014

rwinch commented Mar 19, 2014

pivotal-issuemaster commented Jun 15, 2016

rwinch commented Jul 7, 2016

leleuj commented Jul 11, 2016

vborcea Feb 8, 2017

Choose a reason for hiding this comment

vborcea Feb 8, 2017

Choose a reason for hiding this comment

vborcea Feb 8, 2017

Choose a reason for hiding this comment

vborcea Feb 8, 2017

Choose a reason for hiding this comment

vborcea Feb 8, 2017

Choose a reason for hiding this comment

rwinch commented Nov 17, 2021