Making refresh tokens optional

[xiehan]

I'm working through the V4->V5 upgrade (we're using Diactoros to work around the PSR-7 requirement for now) and one thing that has me stumped is how to make refresh tokens optional. This was very easy to do in V4, but it isn't clear to me how to do it in V5.

I realize it's a good idea to use them (not using them when we first implemented OAuth using V4 has bitten us in a number of ways) and we're in the process of migrating our existing production clients to use them, but not all of them are ready for that transition yet, and we're contractually obligated to respect that.

Looking at how RefreshTokenRepository is set up, I originally thought I could just have getNewRefreshToken return null, but that breaks everything in AbstractGrant::issueRefreshToken(). I suppose I could just have persistNewRefreshToken not do anything (i.e. not actually save the token to the database), but the token will still get included in the response, which we're trying to avoid because it could potentially break a client application that isn't expecting that field to be in the response (or, conversely, someone using a standard OAuth client that can parse it, and that then may try to use a refresh token that was never saved to the database).

Any guidance? Am I missing something?

[blacklizard]

@xiehan why don't you just exclude refresh token from the response?

[xiehan]

@blacklizard That alone isn't enough -- I don't want to do that without also making sure the refresh token doesn't get saved to the database, as I don't want to fill our database with tokens that are never going to be used. Right now, I've implemented this by persistNewRefreshToken() in RefreshTokenRepository only conditionally saving to the database based on whether the client is whitelisted to receive refresh tokens or not, but that feels incredibly hacky to me. Somebody else working on this code after me is never going to think of looking there for that logic, and we're going to run into problems maintaining this down the road.

Given that refresh tokens aren't required by the OAuth spec, it seems to me like there should be a flag either on the AuthorizationServer or at the grant level to turn refresh tokens on/off.

[alexbilbie]

In v6.0.0 I've updated the grants so that if you don't pass in a RefreshTokenRepository then a refresh token won't be created

[lookyman]

@alexbilbie Off topic, how is v6 going? Can we help somehow?

[alexbilbie]

@lookyman I've just got to finish updating the auth code and implicit grants then I'm going to release an alpha

[jacobweber]

Was this already released?

[Sephster]

@jacobweber no it is listed as a release for v8. I don't have a definite schedule for that release yet. I was hoping to get it out late December/early Jan but these timescales have now slipped somewhat.

[jacobweber]

No problem.

For now (in case anyone needs this), I did something similar to what @xiehan did. For public clients, it will always generate and return a refresh_token, but it won't persist it unless you pass scope=refresh_token in the initial auth request.

My ClientEntityInterface implementation has a custom isConfidential method, which returns true if the client is known to be able to store secrets. If this is the case, it will always persist the refresh token.

class MyRefreshTokenRepository implements RefreshTokenRepositoryInterface {
	...
	public function persistNewRefreshToken(RefreshTokenEntityInterface $refreshTokenEntity) {
		$persist = false;
		if ($refreshTokenEntity->getAccessToken()->getClient()->isConfidential()) {
			// For confidential clients, always persist refresh token
			$persist = true;
		} else {
			// For public clients, only persist refresh token if "refresh_token" is in requested scopes.
			foreach ($refreshTokenEntity->getAccessToken()->getScopes() as $scope) {
				if ($scope->getIdentifier() === 'refresh_token') {
					$persist = true;
					break;
				}
			}
		}
		if (!$persist) return;
		// store in database
}

[filecage]

Couldn't find this in the unreleased section of the 8.0.0 branch so I assume it is not implemented yet. @Sephster do you need any help with that issue?

Making Refresh Tokens optional and especially setting their TTL would be a really nice feature.

[Sephster]

PRs are always welcome so please feel free to have a stab at this. According to the spec (and I am saying this from memory rather than checking it), I don't believe refresh tokens should have TTLs. Making the refresh tokens optional would be great though. Thanks for offering assistance.

[filecage]

Yes, that is why I've wanted to make the TTL configurable, so I can set it to forever :) I'm not spec-safe either but as far as I know, refresh tokens do not have a TTL and can only be revoked, not expire.

However I think that having an optional TTL for Refresh Tokens still is a pretty good idea.

Edit: The spec does not clearly state whether a refresh token can expire or not so I'd say it's optional. However, there is no spec-safe way to communicate an expiry timestamp for refresh tokens. As there wouldn't be a huge difference from a client's perspective (it never knows whether a refresh grant has been rejected due to expiry or revocation), I guess that supporting a TTL for the refresh tokens would be a nice feature while there definitely should be an option to make them permanent.

[Sephster]

Reopening this as want to eventually implement Alex's solution of not passing refresh token repository to grants.