[SECOAUTH-435] Access Token is always reset when refresh token is used.

[dsyer]

Priority: Minor
Original Assignee: Dave Syer
Reporter: Alan Hantke
Created At: Thu, 26 Dec 2013 22:32:13 +0000
Last Updated on Jira: Mon, 6 Jan 2014 16:49:55 +0000

Presently, a new access token is granted on each call using grant_type=refresh_token. Unfortunately this makes support of multi-threaded clients difficult. I would like to be able to optionally return the same access token until that token expires. At that point in time, a new token would be generated and returned.

With this approach, a client could potentially spawn multiple threads as might be expected between systems that exchange large amounts of information. Without the approach, the client must implement its own token sharing system which is onerous and redundant. A configurable option allows either the traditional approach or the new, shared, behavior. As best as I can tell, the RFC does not stipulate the intended behavior one way or the other.

This change requires a fairly minor modification to DefaultTokenServices.refreshAccessToken (and setters):

// Optionally return an existing, valid, access token.  This allows multiple threads to share
        // a single refresh_token because they can then share a single access_token; stated differently,
        // the request for an access token will not void a previous request made by a different thread.
        if(reuseAccessToken) {
            OAuth2AccessToken existingAccessToken = getAccessToken(authentication);
            if(existingAccessToken != null  &&  !existingAccessToken.isExpired())
                return existingAccessToken;
        }

If agreement on the change is established, I'm happy to contribute the new code. Unfortunately this is the first time I've submitted to GitHub Open Source, however, so it may take me a bit to establish the process.

Comments:

david_syer on Mon, 30 Dec 2013 10:52:33 +0000

Maybe your clients just need to handle their shared state better? There is an abstraction already in Spring OAuth (ClientTokenServices), so they could use that if they are Spring applications. Is that not appropriate?

Also you could use tokens whose values carry the authentication data (e.g. encoded in JWT as is available in 2.0 snapshots), and then the "reset" that you notice won't happen because it is an implementation detail of the TokenServices implementation that you are used to in 1.0.

alanh on Mon, 30 Dec 2013 20:48:38 +0000

Hi Dave,
The clients certainly could manage the process of obtaining and distributing a token internally, but this approach requires each and every client implement essentially identical code. My perspective is that going down this path doesn't make sense. We would be better off bringing token management back to the server so that everyone can benefit, hence the reason for the ticket. The change is fairly insignificant if it doesn't break some fundamental tenant of the RFC (which I don't believe is the case).

Although I'm familiar with Java, this confidence does not extend to OAuth2, Spring, or JWT. With this in mind, I'm not entirely sure how ClientTokenServices plays a role here (and, yes, Spring is being used). To help educate myself, I relied heavily on the Tonr/Sparklr samples and I didn't see any use of those abstractions there.

I am presently running with the Spring OAuth2 1.0.5.RELEASE implementation (sorry, I forgot to put that in the original ticket). I am not entirely sure where JWT fits into this unless it is a dependency of OAuth2 itself (which makes sense). I certainly don't see any JWT dependency in my own POM. Although I don't dispute that the token returned may be formatted with something more than just a GUID, which is what appears to be done in the Tonr/Sparklr samples, in my system (again, based on the samples) the tokens seem to be fairly "stupid." They are only mapped to an actual user by the OAuth2 code checking a database table (using JDBC token store). I am not aware of any unpacking of the tokens.

At any rate, the cinch point remains the refresh_token's retrieval/issuance of an access token... I think. Unless the newer version of Spring OAuth return a previously issued token, it seems like the original "problem" I described still exists. My present approach was to take DefaultTokenServices and 'extend' it so that it doesn't reset a token if one exists (if you'll recall, there is a step in the code that removes an access token from the table if a live one is present... I'm basically preventing that).

Sorry if I'm not clear. Again, this is new to me.

Thanks,
Alan

david_syer on Tue, 31 Dec 2013 08:15:19 +0000

Small changes that add new options and don't change existing behaviour are more than welcome. Maybe I haven't understood what you need yet. The code snippet you posted in the description looks like a proposed change to createAccessToken(OAuth2Authentication) but you seemed to be describing a change to refreshAccessToken(...).

Client-side token sharing is already implemented in ClientTokenServices (not used in the samples, so that's why you haven't come across it there). So it isn't a big deal if that's really the only feature you need.

The easiest way to discuss code changes in a concrete way is to have a pull request. If you haven't done it before it's easy and there are plenty of guides on how to do it on github itself.

alanh on Mon, 6 Jan 2014 16:49:55 +0000

Great....

I'm in the middle of a different task right at the moment, but when I finish I'll fork the code check it in under this ticket. I am assuming that this is the proper approach.

[benkiefer]

@dsyer

We are seeing the same issue here and were looking at customizing default token services to reuse the token, but before we do so, there is one line that we would like to understand a bit more.

 DefaultTokenServices:98
 // Re-store the access token in case the authentication has changed

When I look at the implementation of creating the access token in the JdbcTokenStore, it is just blowing away the old token and saving a new one, no matter what. There is no dirty checking. What about the authentication could have changed?

Can you see any harm in reusing the access token until it is expired?

Thank you for your help

[benkiefer]

A little more info for you, we are using the DefaultAuthenticationKeyGenerator, which means that if scopes/username/client change, we are never going to find the old access token, so the chances of the authorization changing seems unlikely.

[Floaz]

Hi,
i have the same problem.
There is another reason why deleting the "old" access token is a problem: Race condition.

Example:
The client side does some requests to resource services and has a slow connection, so these requests are somewhat slow. Meanwhile the access token becomes older, so a timer elapses and the corresponding thread tries to refresh the token with a request to the authorization service. Than we have a race condition.

If the refresh of the access token is faster, the first resource request would end in an error "Invalid Access Token", because the "old" access token was deleted. This arises often on mobile systems and on desktop systems (with fast network) with high request payloads on the resource service requests too.

In other threads I read about stopping all other requests until the refresh process is over.
And i can rebut this counter-argument:

• It is difficult to program such a stop and creates much more complexity on the clients.
• When a resource request takes more time, it is already send and the timer would elapse anyway. So it comes to the race condition before a possible stop of other requests.

The original reason for the current implemented approach as explained by the source code comments is the prevention of many refresh requests. So that the client could not create a hugh amount of access tokens.
But a solution would be to set a flag instead of deleting the access token, so only one access token can be retrieved by the combination of the old access token and the refresh token.
This means one new boolean column in the database table.

[Jaykob]

Hi,
we have the same problem and would be interested to know if there are any plans to solve this issue or if we need to find a workaround?