titarenko / OAuth2

OAuth2 client implementation for .NET
413 stars 154 forks source link

New Refresh Tokens are not saved when using OAuth2Client.GetCurrentTokenAsync #149

Closed ghost closed 1 year ago

ghost commented 1 year ago

I have been using the OAuth2 library to create a custom provider, in my case for the GoTo platform (i.e. GoToMeeting), and the library has worked great and been easy to use thus far.

However, the one issue I've had to work around is that when refreshing an access token via the GetCurrentTokenAsync method, the client's RefreshToken property is not updated.

I don't think it's uncommon that a request to an OAuth2 API to refresh an expired access token may result in returning both an updated access and refresh token.

Digging in to the OAuth2Client class, I can see why this is the case. The QueryAccessTokenAsync method explicitly ignores checking for a new refresh token if the request's grant type is refresh_token

if (GrantType != "refresh_token")
    RefreshToken = ParseTokenResponse(response.Content, RefreshTokenKey);

Shouldn't the RefreshToken property be updated if a refresh token value is present in the response, regardless of the grant type?

niemyjski commented 1 year ago

I would think so, but how common is it for the response to contain a refresh token. Is this on a per client basis?

ghost commented 1 year ago

@niemyjski in the case of the GoTo OAuth2 API, it includes a refresh token in every response. However, it may be the same refresh token that was just used, and not necessarily a new one. It just depends on how close the refresh token was to expiring.

Regardless of how common or uncommon including a refresh token is, why shouldn't the base OAuth2Client capture the value if it's present in the response to a "refresh_token" request?

Looking at this page, it sounds like it's pretty common practice that a refresh_token response can include a new refresh token.

And even in cases where a new refresh token isn't included in the response, it sounds like that is intended to indicate that the current refresh token is still valid. It would be nice if client implementations could somehow indicate that, by setting the RefreshToken property to the same value that was passed in to GetCurrentTokenAsync. This is currently not possible for custom clients, since setter is private

kevinbrill commented 1 year ago

If I'm reading this correct, this scenario also occurs with the twitch API as well.

https://dev.twitch.tv/docs/authentication/refresh-tokens/

Calls to https://id.twitch.tv/oauth2/token with a grant_type of refresh_token will return a valid and/or new refresh token.

curl -X POST https://id.twitch.tv/oauth2/token \
-H 'Content-Type: application/x-www-form-urlencoded' \
-d 'grant_type=refresh_token&refresh_token=gdw3k62zpqi0kw01escg7zgbdhtxi6hm0155tiwcztxczkx17&client_id=<your client id goes here>&client_secret=<your client secret goes here>'

Returns:

{
  "access_token": "1ssjqsqfy6bads1ws7m03gras79zfr",
  "refresh_token": "eyJfMzUtNDU0OC4MWYwLTQ5MDY5ODY4NGNlMSJ9%asdfasdf=",
  "scope": [
    "channel:read:subscriptions",
    "channel:manage:polls"
  ],
  "token_type": "bearer"
}
niemyjski commented 1 year ago

It's probably a pretty good idea to update the existing refresh token if the response includes it. Would you mind submitting a pr for this.

ghost commented 1 year ago

Not at all! Should have it available shortly

ghost commented 1 year ago

@niemyjski Submitted PR 150

I didn't realize that I was signed in with a different GH account when I forked, so that's why the PR fork owned by a different account. By the time I realized it, I had already submitted the PR and didn't feel like closing it and starting over!

niemyjski commented 1 year ago

Closed via https://github.com/titarenko/OAuth2/pull/151