Refresh token handling

[sg3s]

I've been working on our oauth2-server implementation recently, and refresh tokens became a subject of interest. Research made me conclude that there are some other things that could be improved upon regarding refresh tokens.

It seems a bit more fitting to require a special scope from a client before it is included
Most implementations in the wild seem to require the offline (e.g. Google) or offline_access (e.g. Okta) scope or similar to enable refresh tokens.

This seems to stem from an OpenID thing, which this library does not implement, but that also means Google is not OpenID spec compliant but does implement it similarly.
https://openid.net/specs/openid-connect-core-1_0.html#OfflineAccess

In any case, the approach makes sense in an OAuth2 context. As a scope, a user needs to grant access to this permission explicitly, and it makes it possible to allow/deny refresh tokens on a client-by-client basis.

Expiration of access tokens should be communicated to clients
In issue #946 we're discussing if expiration for access tokens should be optional. Regardless of what becomes of that, if a token has an expiration we should include an expires_in so clients can anticipate whether a refresh token should still be usable.

This is not specified in IETF RFCs afaik but I've seen several approaches. The most sleek one I encountered simple adds a refresh_token_expires_in property in the code exchange response. This is an implementation by a dutch bank.
https://developer.ing.com/api-marketplace/marketplace/2d00fd5f-88cd-4416-bbca-f309ebb03bfe/reference#endpoint__1

Refresh tokens should probably be optional and maybe even not included by default
This is somewhat possible through the PR #1000 but a more explicit system and changing the default would probably be a good idea. The PR makes it possible, but more as a hack rather than a feature to improve security.

In conclusion
I would propose the following changes; And I am willing to take a stab at making a PR for it, but I wanted input first before I spent too much effort into something that will not be accepted.

• Make an option to configure a scope required for clients to receive a refresh token
• If the above config is set, an auth code grant no longer includes a refresh token by default
• If after running finalizeScopes() the configured scope is present, include the refresh token
• Add refresh_token_expires_in as a property in the authorization code exchange response

As this changes the behaviour only after a settings is set, it is backwards compatible enough to not need a major release.

I am very interested in thoughts about this.

[Sephster]

Hi @sg3s - sorry for the delay in responding. The library aims to conform to spec as much as possible. While I can see the benefits of some of the suggestions above, I think it would make this library a bit more opinionated than it has been in the past. I would prefer for the library to remain spec compliant where possible and be flexible enough to allow developers to add in these non-spec modifications should they wish, with little effort.

W.R.T the optional refresh tokens, I agree that the proposal in PR #1000 isn't an ideal solution but it allows people to have optional refresh tokens just now without breaking BC. I'm conscious at the moment that there are already a number of options that can be set on the auth server so would like to keep this to a minimum.

Long term, I'd prefer to go with Alex's original suggestion of indicating whether you want a refresh token or not by either passing or not passing the refresh token repository to the grant. I think this would be the way to go long term and will be looking to implement this in a future major release.

[sg3s]

No worries - I have my own busy weeks too, obviously.

My aim in going in this direction was definitely to keep the library spec compliant. I think there is enough precedent (in open standards) to substantiate an option which makes the refresh token require a specific, but configurable, scope on the authorization request. I do not think this makes the library very opinionated. By default refresh tokens would still be generated, but setting the option just prevents that unless the configured scope is present.

There should also be functionality to prevent access tokens based on which client is asking for authorization; but as I noted this could then be handled by finalizeScopes by users of the library in a fairly clean way, next to other similar logic. It is an option that needs to be supported by the library to do it properly, which means not generating a refresh token at all or requiring pre-processing requests, as is now often required to implement logic specific to our use-cases.

If we go the way of not enabling refresh tokens by not passing the RefreshTokenRepository

1. I would still somehow want to make that choice based on a scope present in the request
2. I would still somehow want to make that choice based on the client making the request
3. I would want to be able to make that choice based on other variables (like resource owner IP address)

I'm not sure how you could fit that flexibility in the current architecture by doing things like making repository optional on grants, but maybe there is a way I am not seeing. Is pre-processing much of the request to determine how to setup the server the correct way of doing this?

Different issue but we currently allow/disallow clients from using specific grants by having some logic in the ClientRepository to only find clients explicitly linked to a grant type. It works but I consider it more of a hack needed due to the library not offering flexibility elsewhere. It definitely prevents us from providing a more fitting 'client not allowed to use this grant' error message instead of 'client not found'.

Keeping things strict is important in not allowing for mistakes, but realize too strict/closed also stifles the usability of the library a lot.

I am still willing to give creating the proposed feature/option a go, but if you're not interested I think you can close this issue.

[christiaangoossens]

I agree with @sg3s that some way of enabling refresh tokens based on the token request or the client also needs to be implemented and that #1000 does not account for this yet. Is adding a simple function call to check if refresh tokens should be enabled, passing the request, an option? Then @sg3s could implement the scope checking as described above himself without it influencing the library.

[Sephster]

I see where you are coming from now. I suppose my main reservation is that according to the spec, the issuing of a refresh token is usually at the discretion of the authorization server, and not the client.

The problem with this stance is that we could be issuing a refresh token that is neither wanted or used... which isn't ideal. I will need to do a bit more reading around this but definitely food for thought!

Sorry as it might take me a while to fully process this as I imagine I will go down the rabbit hole once I start reading around the topic. I will keep this open for now while I try to research this a bit more. Thanks to both of you for your thoughts regarding this.

[matt-allan]

They are still going back and forth about it on the mailing list but currently the recommendation is to not issue refresh tokens to public clients.

Assuming the IETF sticks with that recommendation if refresh tokens become optional in the server it would be good to allow choosing based on the client so you could prevent public clients from obtaining them.