OAuthRestTemplate should take OAuthSecurityContext as method argument additionally to fetching it from OAuthSecurityContextHolder

Details

Description

The OAuthRestTemplate uses authentication information (the OAuth token) when it performs requests to authenticate the client against a server. It retrieves the token from the OAuthSecurityContext by accessing the singleton OAuthSecurityContextHolder which is tied to the current thread. This conforms to other similar patterns where information is associate with threads (especially in web apps).

The following use-case however is rather not so nicely supported:

Using OAuthRestTemplates for backend components which are not directly user-related and therefore do not have a thread per user. For example I implemented a service which needs to access the same resource under two different users to propagate in one logical operation. The second access is writing a piece of information to the service on behalf of the second user - the information itself was retrieved on behalf of the first user - all in the same thread. The procedure is as follows:

1) Replace the original OAuthSecurityContext O1 via OAuthSecurityContextHolder with the OAuthSecurityContext O2 for the first user & backup the original OAuthSecurityContext O1.
2) Perform the first request
3) Replace the 1st OAuthSecurityContext O2 with the OAuthSecurityContext O3 of the second user via OAuthSecurityContextHolder
4) Perform the second request
5) Restore the originally found OAuthSecurityContext O1 via OAuthSecurityContextHolder

Because of the fact that the OAuthRestTemplate implementation exclusively considers the "current" OAuthSecurityContext stored in OAuthSecurityContextHolder as the security context it's not possible to pass in an OAuthSecurityContext per request. Therefore the client code needs to hot-replace the OAuthSecurityContext and afterwards revert OAuthSecurityContext as required by the application process.

That's definitely a source of error especially in terms of security flaws and long running threads if the client code "makes mistakes" when switching security contexts (e.g. forgets to restore the original context after a few requests).

I'd propose that the OAuthSecurityContext could be pass as method argument for a single request which would clearly define and limit the scope of a specific OAuthSecurityContext to the lifetime of a single request. That would provide the following benefits:

It would be impossible to "forget" by accident a OAuthSecurityContext in the OAuthSecurityContextHolder and perform requests afterwards with "foreign" OAuthSecurityContexts

issuing multiple requests using multiple OAuthSecurityContexts would be easier and cleaner in terms of code

Ryan Heaton
added a comment - 29/Nov/10 10:08 AM Good analysis. We'll need to give this some thought.
Are you suggesting that the context be passed on a per-method basis? Or would it suffice to have a constructor for OAuthRestTemplate that took an instance of the context?

Right, I understand. The keyword in my question was "potentially". I suppose in your use case you would have to skip any userid for which you do not hold valid cached token? Or do you loop through all the tokens you do have that you believe are still valid and only execute the business logic for those accounts?

Which brings us back to the other question: what is it about creating 200 instances of RestTemplate that you don't like? I'm wondering if a custom Spring Scope might be able to do some heavy lifting for you, but I want to be clear: there has to be someone who is responsible for creating a separate context for each request.

Dave Syer
added a comment - 10/Aug/11 1:20 PM Right, I understand. The keyword in my question was "potentially". I suppose in your use case you would have to skip any userid for which you do not hold valid cached token? Or do you loop through all the tokens you do have that you believe are still valid and only execute the business logic for those accounts?
Which brings us back to the other question: what is it about creating 200 instances of RestTemplate that you don't like? I'm wondering if a custom Spring Scope might be able to do some heavy lifting for you, but I want to be clear: there has to be someone who is responsible for creating a separate context for each request.

We never got to the bottom of this really. I had a need myself to control the OAuth2Context a bit from my client. The solution, I believe, is not to pollute the interface of RestTemplate, or even to require a new instance of the template for each request, since the template is stateful (i.e. it has configuration options set that can be irrelevant for OAuth2). The correct approach is probably some other kind of template. This is what I ended up with:

Dave Syer
added a comment - 10/Jan/12 8:13 AM We never got to the bottom of this really. I had a need myself to control the OAuth2Context a bit from my client. The solution, I believe, is not to pollute the interface of RestTemplate, or even to require a new instance of the template for each request, since the template is stateful (i.e. it has configuration options set that can be irrelevant for OAuth2). The correct approach is probably some other kind of template. This is what I ended up with:
public <T> T doInOAuth2Context(OAuth2ContextCallback<T> callback) {
OAuth2ClientContext existingContext = OAuth2ClientContextHolder.getContext();
OAuth2ClientContextHolder.clearContext();
addAccessToken(resource, request);
try {
return callback.execute(client);
}
finally {
OAuth2ClientContextHolder.clearContext();
if (existingContext != null) {
OAuth2ClientContextHolder.setContext(existingContext);
}
}
}
and
public interface OAuth2ContextCallback<T> {
T execute(RestOperations client);
}

I agree that the existing RestTemplate interface should not be polluted just because of an additional requirement/use case in one of its implementations. I think (at the moment) that a separate template class is the way to go.

What I seriously care about is the potential of such a template to incorrectly access previously used OAuth tokens of e.g. other users (for example exceptions and dangling contexts/tokens due to incomplete exception handling come to my mind - remember that my Oauth tokens are not coupled to the web layer and I use during one request multiple tokens for passing data between OAuth enabled user accounts). This would result in bad, bad security issues and that risk should be reduced as much as possible by design. That's why I would favor explicit context passing (kind of method-level DI for the context) on each template invocation so that client code is "aware" of the context being provided. This leaves still room for such errors but at least it's obvious what will be used for authentication. As far as I can see this would be possible when breaking out of the existing RestTemplate interface.

Unfortunately I won't have time to dig into this issue again in the near future and my use-case is implemented so far and works. Feel free to decide on what to do with this issue. Although I think this issue has a point I would not be angry if you just close it

Alex Rau
added a comment - 10/Jan/12 11:12 PM Dave,
I agree that the existing RestTemplate interface should not be polluted just because of an additional requirement/use case in one of its implementations. I think (at the moment) that a separate template class is the way to go.
What I seriously care about is the potential of such a template to incorrectly access previously used OAuth tokens of e.g. other users (for example exceptions and dangling contexts/tokens due to incomplete exception handling come to my mind - remember that my Oauth tokens are not coupled to the web layer and I use during one request multiple tokens for passing data between OAuth enabled user accounts). This would result in bad, bad security issues and that risk should be reduced as much as possible by design. That's why I would favor explicit context passing (kind of method-level DI for the context) on each template invocation so that client code is "aware" of the context being provided. This leaves still room for such errors but at least it's obvious what will be used for authentication. As far as I can see this would be possible when breaking out of the existing RestTemplate interface.
Unfortunately I won't have time to dig into this issue again in the near future and my use-case is implemented so far and works. Feel free to decide on what to do with this issue. Although I think this issue has a point I would not be angry if you just close it

There were some pretty significant changes to the client support features last week which probably render this issue resolved. Some feedback would be good. See in particular the new constructors and public methods in OAuth2RestTemplate...

Dave Syer
added a comment - 02/Apr/12 1:22 AM There were some pretty significant changes to the client support features last week which probably render this issue resolved. Some feedback would be good. See in particular the new constructors and public methods in OAuth2RestTemplate...
For a client with an existing access token:
OAuth2ProtectedResourceDetails resource = ...;
OAuth2ClientContext context = new DefaultOAuth2ClientContext(accessToken);
OAuth2RestTemplate template = new OAuth2RestTemplate(resource, context);
// ... now use template as a normal RestTemplate
For a client that needs to obtain a token but has all the credentials needed:
OAuth2ProtectedResourceDetails resource = ...;
OAuth2ClientContext context = new DefaultOAuth2ClientContext(new AccessTokenRequest(parameters));
OAuth2RestTemplate template = new OAuth2RestTemplate(resource, context);
// ... now use template as a normal RestTemplate
There are some tweaks in the pipeline to make this last example work better with implicit and auth code grants. Password and client credentials should be OK already.

Alex Rau
added a comment - 21/Dec/12 10:21 AM I migrated my OAuth1 setup to OAuth2 for testing purposes. It's not yet fully tested but what you proposed looks very good. Feels much better than what had to be done with OAuth1. Thanks !