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

Add GitHub#getInstallation to access endpoints related to the authenticated installation #1523

Merged
merged 1 commit into from Sep 27, 2022

Conversation

yrodiere
Copy link
Contributor

Description

Fixes #1082.

Relevant documentation: https://docs.github.com/en/rest/apps/installations#list-repositories-accessible-to-the-app-installation
As you can see, this endpoint can only be reached with an installation token; but GHAppInstallation can only be obtained through GHApp with a JWT, so its method listRepositories just cannot work, except with hacks (setRoot, ...).
That's the problem we're trying to solve.

This fix just adds a method to GitHub to obtain an object representing the current installation, which exposes a listRepositories method.
Original idea by @dwnusbaum (#1082 (comment)).

Before submitting a PR:

  • Changes must not break binary backwards compatibility. If you are unclear on how to make the change you think is needed while maintaining backward compatibility, CONTRIBUTING.md for details.
  • Add JavaDocs and other comments as appropriate. Consider including links in comments to relevant documentation on https://docs.github.com/en/rest .
  • Add tests that cover any added or changed code. This generally requires capturing snapshot test data. See CONTRIBUTING.md for details.
  • Run mvn -D enable-ci clean install site locally. If this command doesn't succeed, your change will not pass CI.
  • Push your changes to a branch other than main. You will create your PR from that branch.

When creating a PR:

  • Fill in the "Description" above with clear summary of the changes. This includes:
    • If this PR fixes one or more issues, include "Fixes #" lines for each issue.
    • Provide links to relevant documentation on https://docs.github.com/en/rest where possible.
  • All lines of new code should be covered by tests as reported by code coverage. Any lines that are not covered must have PR comments explaining why they cannot be covered. For example, "Reaching this particular exception is hard and is not a particular common scenario."
  • Enable "Allow edits from maintainers".

@gsmet
Copy link
Contributor

gsmet commented Sep 21, 2022

Hey @bitwiseman ,

This one is kinda in the way of our work on Quarkus so we would appreciate a lot if you could review and cook a release once everything is fine.

Thanks!

Copy link
Contributor

@dwnusbaum dwnusbaum left a comment

Choose a reason for hiding this comment

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

Thanks for working on this! It looks good to me in general. I think that adding the GHAuthenticatedAppInstallation class is a good idea to avoid polluting the GitHub class with methods that only work in some cases. Support for DELETE /installation/token can easily be added to the class later, and perhaps GitHub will add other similar /installation API endpoints that could live in the new class in the future.

(I am not a maintainer.)

*/
public class GHAuthenticatedAppInstallation extends GitHubInteractiveObject {
protected GHAuthenticatedAppInstallation(@Nonnull GitHub root) {
super(root);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible with the current codebase to detect whether the current GitHub client is authenticated as an installation when this object is instantiated on the client side? If so, maybe it would be nice to throw an exception with a helpful error message.

That said, do you know what the error is from GitHub when you call listRepositories with incorrect authentication? If it is already pretty clear, then there's probably no reason to do anything extra here.

Copy link
Contributor Author

@yrodiere yrodiere Sep 22, 2022

Choose a reason for hiding this comment

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

Is it possible with the current codebase to detect whether the current GitHub client is authenticated as an installation when this object is instantiated on the client side?

Judging from how GitHub#getApp() fails with an obscure HTTP authentication error when we try to use it with an installation token, I would say there is no such feature yet. That would probably be a nice improvement in another PR that would be applied across the board to all relevant methods, not just this one? I think the hardest part would probably be testing...

That said, do you know what the error is from GitHub when you call listRepositories with incorrect authentication? If it is already pretty clear, then there's probably no reason to do anything extra here.

We get this, so I suppose it depends what you call "pretty clear":

org.kohsuke.github.HttpException: {"message":"Bad credentials","documentation_url":"https://docs.github.com/rest"}
        at org.kohsuke.github.GitHubConnectorResponseErrorHandler$1.onError(GitHubConnectorResponseErrorHandler.java:56)
        at org.kohsuke.github.GitHubClient.detectKnownErrors(GitHubClient.java:424)
        at org.kohsuke.github.GitHubClient.sendRequest(GitHubClient.java:386)
        at org.kohsuke.github.GitHubPageIterator.fetch(GitHubPageIterator.java:142)
        at org.kohsuke.github.GitHubPageIterator.hasNext(GitHubPageIterator.java:89)
        at org.kohsuke.github.PagedSearchIterable$1.hasNext(PagedSearchIterable.java:86)
        at org.kohsuke.github.PagedIterator.fetch(PagedIterator.java:106)
        at org.kohsuke.github.PagedIterator.nextPageArray(PagedIterator.java:134)
        at org.kohsuke.github.PagedIterable.toArray(PagedIterable.java:78)
        at org.kohsuke.github.PagedIterable.toArray(PagedIterable.java:106)
        at org.kohsuke.github.PagedIterable.toList(PagedIterable.java:118)

Copy link
Contributor

Choose a reason for hiding this comment

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

{"message":"Bad credentials","documentation_url":"https://docs.github.com/rest"}

Well that is true, but the message is a bit opaque 😄

Either way this is something that could easily be improved in the future if users are actually getting confused by it, so there's probably no reason to complicate this PR.

src/main/java/org/kohsuke/github/GitHub.java Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Sep 22, 2022

Codecov Report

Base: 78.95% // Head: 78.97% // Increases project coverage by +0.01% 🎉

Coverage data is based on head (cc85d4a) compared to base (319e943).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #1523      +/-   ##
============================================
+ Coverage     78.95%   78.97%   +0.01%     
- Complexity     2129     2132       +3     
============================================
  Files           204      205       +1     
  Lines          6463     6469       +6     
  Branches        362      362              
============================================
+ Hits           5103     5109       +6     
  Misses         1148     1148              
  Partials        212      212              
Impacted Files Coverage Δ
...ain/java/org/kohsuke/github/GHAppInstallation.java 91.89% <ø> (ø)
...kohsuke/github/GHAuthenticatedAppInstallation.java 100.00% <100.00%> (ø)
src/main/java/org/kohsuke/github/GitHub.java 82.27% <100.00%> (+0.08%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@yrodiere
Copy link
Contributor Author

Hey @bitwiseman , thanks for allowing the CI build. It looks like the build passes, for this PR as well as for my two other PRs (#1524, #1525).

I'll be happy to address any comment you have on those PRs.

If the PR look good to you, do you think you could merge them and release a new version of github-api? As Guillaume mentioned earlier, we would need these patches in a Quarkus-related project :)

@bitwiseman bitwiseman merged commit 586eabd into hub4j:main Sep 27, 2022
@bitwiseman
Copy link
Member

@yrodiere
Nice work!

@bitwiseman
Copy link
Member

Release coming soon.

@yrodiere
Copy link
Contributor Author

Thanks @bitwiseman!

@bitwiseman
Copy link
Member

@yrodiere
I'm blocked on publishing a new release by #1528 . I've spent for more time on this project tonight than I can spare, but I apparently can't produce a new release until that issue is fixed. 😡

@bitwiseman
Copy link
Member

I was able to release by doing some hacking. Enjoy!

@yrodiere
Copy link
Contributor Author

Great to see that @bitwiseman , thanks! We're already using it in production :)
I opened #1531 to address the javadoc issue.

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

Successfully merging this pull request may close these issues.

Some "GHAppInstallation" methods need an installation token, not a JWT token
4 participants