Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
ghost opened this issue Jan 10, 2023 · 7 comments
Closed

Comments

@ghost
Copy link

ghost commented Jan 10, 2023

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
Copy link
Collaborator

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
Copy link
Author

ghost commented Jan 13, 2023

@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
Copy link

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
Copy link
Collaborator

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
Copy link
Author

ghost commented Jan 17, 2023

Not at all! Should have it available shortly

@ghost
Copy link
Author

ghost commented Jan 17, 2023

@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
Copy link
Collaborator

Closed via #151

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants