Allow OAuth2Client refresh token to be updated when grant_type is "refresh_token"

[jholzer-cciq]

Update the OAuth2 base client implementation so that the RefreshToken property is updated when refreshing access tokens (grant_type = "refresh_token"). Previously, the refresh token was only updated for requests where grant_type was not equal to "refresh_token".

This change is being made because many OAuth2 implementations will include an updated refresh token in the response to a "refresh_token" request. For the implementations that don't, the RefreshToken property will continue to be null after the response has been processed.

[niemyjski]

I'm not 100% sure about this change, can you dig into the oauth2 spec a bit more to see if it says anything about this. My thought process currently (without additional information which I'd love to have) is that we should parse it and set it if it returns anything. If it returns null than we leave what was there as it may only be set via the refresh_token grant type and we would be wiping that value out. But I see your point if the client always returns it, then this is the behavior we would want.

[jholzer-cciq]

I'll check out the specs and see what I can find. I can certainly put in a conditional that will only update RefreshToken if the refresh token value parsed from the response is non-null and non-whitespace (i.e. !string.IsNullOrWhitespace()).

[jholzer-cciq]

Looking at the original OAuth2 spec (RFC 6749, Section 1.5), it states the following in regards to requesting a new access token using a refresh token:

(G) The client requests a new access token by authenticating with
the authorization server and presenting the refresh token. The
client authentication requirements are based on the client type
and on the authorization server policies.

(H) The authorization server authenticates the client and validates
the refresh token, and if valid, issues a new access token (and,
optionally, a new refresh token).

The spec clearly states that issuing a new refresh token is optional.

I still think the most reasonable default behavior is to just update the RefreshToken based on whatever you get back in the response. However, as I mentioned previously, I'm happy to make an additional change to only update RefreshToken in the case where an actual refresh_token is included in the response.

Ultimately, I think the best long term solution, that should eliminate any debate about dealing with refresh tokens, would be to have the OAuth2 Client's methods return result objects containing the access and refresh tokens, rather than storing/caching the "last" access and refresh tokens as properties. This design change also seems like it could open up the possibility of using OAuth2 clients as singletons, rather than having to create a new client instance every time tokens are needed for a different user.

[jholzer-cciq]

@niemyjski any additional thoughts?

The spec is pretty clear that a new refresh token can be returned in response to a refresh request.

If you think the client should only update the RefreshToken property in the case where a new refresh token is returned, I can certainly make that change.

[niemyjski]

Sorry, I've been pretty busy. I agree with your assessment and research about creating an object and making these stateless.

I think for now so it's not a breaking change (and so we have to check every single integration if it returns a refresh token or not), we try and parse the response for a refresh token if it's not null we set it, otherwise we leave it be.

[jholzer-cciq]

@niemyjski I've updated the OAuth2 client so that RefreshToken will only be updated when a new value is provided in the response. I've updated unit tests as well.

[niemyjski]

Thanks for the PR

[niemyjski]

New release is out