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

Support refreshtokens in OAuth flow #2749

Merged
merged 6 commits into from
Jul 27, 2023

Conversation

Kencdk
Copy link
Contributor

@Kencdk Kencdk commented Jul 18, 2023

Resolves #2731


Behavior

Before the change?

  • OAuth Client doesn't support getting the refresh token, nor utilizing it to get a new access token.
  • Additionally, it doesn't return information about when the access token expires.

After the change?

  • CreateAccessToken now returns all fields from the returned body, including token expiration, refresh token and refresh token expiration time.
  • CreateAccessTokenFromRefreshToken accepts a clientId, clientSecret and refresh token, in order to create a new access token on behalf of the user, the refresh token was created for.

Other information

While the endpoint is the same for creating an access token from a response code and for creating an access token based on the refresh token, there are key differences in the expected parameters. When creating based on a refresh token, the grant_type needs to be set.

As a result, I opted to create a new function instead of adding a mix of optional and required parameters (depending on input) to the existing CreateAccessToken function. Please advise if you'd prefer otherwise.


Additional info

Added <inheritdoc /> instead of having the xml doc in both interfaces and implementation.
The end-result is the same, it just avoids having to keep both up-to-date.

Pull request checklist

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)
  • Added the appropriate label for the given change

Does this introduce a breaking change?

Please see our docs on breaking changes to help!

  • Yes (Please add the Type: Breaking change label)
  • No

If Yes, what's the impact:

  • N/A

Pull request type

Type: Feature


Copy link
Member

@kfcampbell kfcampbell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This approach seems reasonable and I love the functionality!

@@ -20,6 +20,19 @@ public OauthToken(string tokenType, string accessToken, IReadOnlyList<string> sc
this.ErrorUri = errorUri;
}

public OauthToken(string tokenType, string accessToken, int expiresIn, string refreshToken, int refreshTokenExpiresIn, IReadOnlyList<string> scope, string error, string errorDescription, string errorUri)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think it'd be worthwhile to add a comment here and above describing the situations when you'd want to use each constructor?

Copy link
Contributor Author

@Kencdk Kencdk Jul 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I primarily kept the old one, to keep the change binary compatible - to avoid breaking changes :)
Added a summary for good measure.

These constructors aren't really meant to be used manually - so wondering if the one without the refresh token should be marked as deprecated, so it can be removed sometime in the future? :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, thank you! I think it's reasonable to mark the old one as deprecated to be removed in a future major release. After that's done, I can merge and release this!

@kfcampbell
Copy link
Member

@Kencdk are you okay with the updates I've made? If so, I can ship this!

@Kencdk
Copy link
Contributor Author

Kencdk commented Jul 27, 2023

@Kencdk are you okay with the updates I've made? If so, I can ship this!

@kfcampbell, Yup, looks good to me!

@kfcampbell kfcampbell merged commit bbcd33d into octokit:main Jul 27, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[FEAT]: Support refreshtokens in OAuth flow
2 participants