-
Notifications
You must be signed in to change notification settings - Fork 8
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 GitHub Apps Installation authentication #69
Conversation
👋 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 |
Apologies for the unasked-for commentary. I'd love to see App support in this library, but I'm unsure about directly referencing the As used also, the only APIs accessible will be those that accept the installation token. APIs that require the App JWT itself will not work. Could I suggest:
There is potential here to have a |
@jamestelfer no worries at all, we appreciate and welcome the thoughtfulness! Let's discuss a little further if that's okay since I'm not quite sure I have a handle on what you're proposing. Great point about the indirect reference to When you mention that you'd like to support the I agree that we should support using an application JWT directly for those meta endpoints that don't support an App installation token. What I'm a bit unsure of is the best way to go about doing so: whether that's forking the library or rewriting its functionality ourselves. I'll have to do a little research on our policies before making a decision here. Finally, can you talk to me a little more about your |
I was going to contribute that to
Ideally this information would be encoded using the I'll come back to the how on this one.
Fair enough: my description was sketchy at best 😀 Based on what may be a fairly shallow understanding of Kiota, it seems that the abstraction of the There are implementations required of the So kinda like: flowchart LR
A[Client] -->|Authenticate| B[FilterProvider]
B --> C{URL auth type?}
C -->|JWT| D[JWTProvider]
C -->|Token| E[InstallationTokenProvider]
E -->|create token via| D
Where the filter provider selects an authenticator based on the URL, the JWT provider creates and sets the JWT as the token, and the Token provider can make a call via the JWT provider to create its token as needed. Given this separation, the API user need only provide the private key in order to get the behaviour they need, and batteries are included. Given the options though, the JWT provider could be switched out by the API user for a variant that used a KMS service to sign the JWT, and the user could also ensure that the implementation held the JWT for a short period to avoid lots of KMS API calls. So they don't need to BYO JWT or Signer, but they can. This is not dissimilar to
I hope this better explains what I was getting at, and I really appreciate the conversation! Footnotes
|
Nice! I've forked
I've also been thinking about the "how" on this one, and I've been hoping to avoid decorating the OpenAPI spec if possible just because of the friction involved, but if there's no other way, I think it would be feasible.
It was weird to me when I implemented things in the PR that the PAT auth was a TokenProvider and the App auth worked at the RoundTripper level. I rationalized it at the time by thinking that the PAT stayed constant the whole time, whereas the App token needs to be renewed periodically, and the Kiota Otherwise, the design of the FilterProvider you've proposed looks good to me. |
I summarized a proposed approach here: bradleyfalzon/ghinstallation#118. Whether or not this is more polished is an open question 😁 I can put this together in a PR. If you were looking to bring this into the octokit org, would it be an opportunity to consider breaking API changes, or ease transition by keeping the original API?
TBH, I haven't looked closely at the |
I've added my This can be further refined and tests included if the approach seems sound. |
For our purposes, I'm okay with breaking changes (and I did make them in the fork). Now would be the time to make them.
Please let me know if you do! I'd be very curious to hear your thoughts.
Nice! I've subscribed to both the issue and the PR you've created and will stay tuned. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than the one nit about nesting this LGTM! ❤️
log.Fatalf("error parsing installation ID from string to int64: %v", err) | ||
} | ||
|
||
client, err := pkg.NewApiClient( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fluent syntax here makes me want to consider how we might be able to propagate that to other SDKs - it feels really good.
Co-authored-by: Nick Floyd <[email protected]>
👀 Rendered README changes 👀
The API for Apps initialization looks like:
Future work is planned to make authentication for Apps meta endpoints that require a JWT transparent to the user.
Test coverage:
pkg
go test -v -coverpkg=./pkg -coverprofile=pkg.cov ./pkg
For comparison:
authentication
go test -v -coverpkg=./pkg/authentication/... -coverprofile=pkg.cov ./pkg/authentication/...
handlers
go test -v -coverpkg=./pkg/handlers/... -coverprofile=pkg.cov ./pkg/handlers/...