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

Adding support for the Device Flow Oauth authentication pattern #2310

Merged
merged 6 commits into from
Apr 20, 2022
Merged

Adding support for the Device Flow Oauth authentication pattern #2310

merged 6 commits into from
Apr 20, 2022

Conversation

palenshus
Copy link
Contributor

@palenshus palenshus commented Mar 2, 2021

Adds support for the Device flow authentication pattern: https://docs.github.com/en/developers/apps/authorizing-oauth-apps#device-flow

Resolves #2308

Usage:

var github = new GitHubClient(new ProductHeaderValue("test"));
string clientId = "<clientId>";
var request = new OauthDeviceFlowRequest(clientId);
request.Scopes.Add("repo");

var deviceFlowResponse = await github.Oauth.InitiateDeviceFlow(request);
Console.WriteLine(deviceFlowResponse.UserCode);
OpenWebPage(deviceFlowResponse.VerificationUri);
var token = await github.Oauth.CreateAccessTokenForDeviceFlow(clientId, deviceFlowResponse);
Console.WriteLine(token.AccessToken);

@codecov
Copy link

codecov bot commented Mar 2, 2021

Codecov Report

Merging #2310 (662033f) into main (15887d9) will not change coverage.
The diff coverage is 20.00%.

❗ Current head 662033f differs from pull request most recent head fad9a41. Consider uploading reports for the commit fad9a41 to get more accurate results

@@           Coverage Diff           @@
##             main    #2310   +/-   ##
=======================================
  Coverage   65.99%   65.99%           
=======================================
  Files         554      554           
  Lines       14519    14519           
  Branches      857      857           
=======================================
  Hits         9582     9582           
  Misses       4779     4779           
  Partials      158      158           
Impacted Files Coverage Δ
Octokit.Reactive/Clients/ObservableOauthClient.cs 0.00% <ø> (ø)
Octokit/Clients/OAuthClient.cs 100.00% <ø> (ø)
Octokit/Models/Response/OauthToken.cs 69.23% <0.00%> (ø)
Octokit/Helpers/ApiUrls.cs 97.53% <100.00%> (ø)

@palenshus
Copy link
Contributor Author

This failure looks unrelated to my change, possibly just an infrastructure hiccup? Can it be re-run?

@palenshus
Copy link
Contributor Author

/azp run

@palenshus palenshus closed this Jul 22, 2021
@palenshus palenshus reopened this Jul 22, 2021
Copy link
Contributor

@nickfloyd nickfloyd left a comment

Choose a reason for hiding this comment

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

@palenshus This is a great addition, thank you for taking the time to do it!

I just had a few comments on how you've set up some of the class property's accessibility. I was wondering, were you favoring protected over private because it's the dominant pattern for most of the models in Octokit.net? It seems that most of those now implement ctors in one form or another so that makes it where I think we could change most of the protected access modifiers to the more appropriate private ones.

I didn't see where we needed to use the class as a derived class but I could've just missed something. Let me know your thoughts on the suggestions when you get the chance.

Thanks again for putting in the effort here - it is sincerely appreciated and will result in something that will help our community.

Octokit/Models/Response/OauthToken.cs Show resolved Hide resolved
Octokit/Models/Response/OauthDeviceFlowResponse.cs Outdated Show resolved Hide resolved
Copy link
Contributor

@nickfloyd nickfloyd left a comment

Choose a reason for hiding this comment

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

It looks like we are almost there. 3 properties need to have their setters made private and I think we'll be good to go. Thanks again for putting in the work on this!

Octokit/Models/Response/OauthToken.cs Outdated Show resolved Hide resolved
Octokit/Models/Response/OauthToken.cs Outdated Show resolved Hide resolved
Octokit/Models/Response/OauthToken.cs Outdated Show resolved Hide resolved
Copy link
Contributor

@nickfloyd nickfloyd left a comment

Choose a reason for hiding this comment

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

This looks great, thanks!

@nickfloyd nickfloyd merged commit 7721343 into octokit:main Apr 20, 2022
@palenshus
Copy link
Contributor Author

Thanks Nick, it's been a long road, I'm so glad to have this in now!

@nickfloyd
Copy link
Contributor

release_notes: Adds support for the Device Flow Oauth authentication pattern

@nickfloyd nickfloyd added Type: Feature New feature or request and removed category: feature labels Oct 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Octokit should support Device Flow authentication (in C#)
2 participants