Support GitHub Apps Installation authentication #70
Conversation
- Nothing is working/fully-implemented - Some refactoring is still needed after implementation - Auth options error handling in the Build method need to go in their own helper - Middleware defaults still need to be loaded - App token creation and refresh still need to be implemented
…ow uses the more patform common package Microsoft.Identity..
|
👋 Hi! Thank you for this contribution! Just to let you know, our GitHub SDK team does a round of issue and PR reviews twice a week, every Monday and Friday! We have a process in place for prioritizing and responding to your input. Because you are a part of this community please feel free to comment, add to, or pick up any issues/PRs that are labled with |
Co-authored-by: Keegan Campbell <me@kfcampbell.com>
…instantiating a app installation token provider
…-sdk into apps-delegated-middleware
…ion on any platform
… to the user as a token descriptor attribute.
…duce a little bit of duplicate naming
There was a problem hiding this comment.
This is shaping up nicely! FWIW, I don't have strong opinions about the location of the Authentication helpers in the Middleware directory vs. the src root. I'm happy to defer to you, and would probably leave them as-is just for the sake of inertia if you didn't bring it up. Up to you!
.github/workflows/build.yml
Outdated
| @@ -19,6 +19,11 @@ jobs: | |||
| with: | |||
| dotnet-version: '8.x.x' | |||
|
|
|||
| # This is a temporary step. Since the CLI references the main project locally, the main | |||
| # project must be built first. When the CLI is removed, this may be removed as well. | |||
There was a problem hiding this comment.
Do we want to make the CLI a permanent part of this project? If so, this comment can go.
There was a problem hiding this comment.
Yeah, I think that would be a good move; at least for the short term as we flesh things out.
cli/Program.cs
Outdated
| } | ||
|
|
||
| // Personal Access Token authentication | ||
| // var tokenProvider = new TokenProvider(Environment.GetEnvironmentVariable("GITHUB_TOKEN") ?? ""); |
There was a problem hiding this comment.
Rather than comments separating multiple types of auth, I'd probably prefer to see separate .cs files for each example like we have on the Go side.
That would also mean we can avoid the ugly serial line commenting like we have below here.
There was a problem hiding this comment.
Yeah good call out... I'll get this cleaned up.
| /// <summary> | ||
| /// This class is responsible for creating, signing and refreshing access tokens | ||
| /// </summary> | ||
| public class GitHubAppTokenProvider : IGitHubAppTokenProvider |
There was a problem hiding this comment.
I picked the GitHubAppTokenProvider and IGitHubAppTokenProvider naming here, as it used to be AccessTokenProvider, which I felt clashed with Kiota's IAccessTokenProvider naming/interface, which this does not implement. Please let me know if you have thoughts or better ideas on naming here!
There was a problem hiding this comment.
This just doesn't feel right... perhaps because it's not consistent across all provider naming schemes or it feels redundant with the namespace.
My preference would be to keep it as it was, and just name the interface IGitHubAppTokenProvider or rename them all for consistency (including the test files). Perhaps in the future we'll implement other auth providers? IDK.
I understand the reasoning behind the change, I think my only critique is go all or none with the rename IMO.
There was a problem hiding this comment.
Also, I am wondering if we should move Authentication back to a root level folder (like Go), again, for consistency.
There was a problem hiding this comment.
Agreed about it not quite feeling right. My main concern with the way it was, was I found it very confusing that our naming scheme matched Kiota's naming scheme for an interface that we didn't implement. Perhaps we can pair on this tomorrow?
I don't have a huge preference on moving Auth code back to the root out of middleware...I'm happy to go either way, and make a decision to potentially revise later.
| var tokenDescriptorField = _tokenProvider.GetType() | ||
| .GetField("_tokenDescriptor", System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Instance); | ||
|
|
||
| if (tokenDescriptorField != null) |
There was a problem hiding this comment.
I mentioned this in the comment above, but I really don't like the reflection here. Please let me know if you have better ideas for testability (using internal instead of private maybe?).
There was a problem hiding this comment.
I think internal should be fine here.. where you thinking about using a decorator to make it available to the test project/file.. i.e.
[assembly: InternalsVisibleTo("test")] or something - I'm good with either way. While not inherently "bad", I'd like to stay away from reflection if possible.
There was a problem hiding this comment.
Agreed on staying away from reflection. I'm not sure of the exact incantation required to make the internals testable; if a decorator is the way to go, that's definitely better than reflection. Perhaps we can pair on this tomorrow?
Co-authored-by: Keegan Campbell <me@kfcampbell.com>
nickfloyd
changed the title
…-sdk into apps-delegated-middleware
Description
This PR introduces two things:
Changes
CLI
Authentication
Test coverage has been added to ensure that the new classes and methods are working as expected.
dotnet test -p:CollectCoverage=true -p:CoverletOutput=TestResults/ -p:CoverletOutputFormat=opencover -p:ExcludeByFile="**/GitHub/**/*.cs"Pull request checklist
Does this introduce a breaking change?
Please see our docs on breaking changes to help!