Support full GitHub app authentication #1986

dblanchette · 2021-06-30T21:42:31Z

This adds support for GitHub App authentication using the main class which is simpler for users (see #1855)

import Github

github_client = Github(app_id=MY_APP_ID, app_private_key=MY_PRIVATE_KEY)

Also, the credentials are refreshed when needed (tokens lasts for one hour). This should happen transparently to the users, as the token expiration is checked at every call.

Caveats and other notes:

Uses the first found installation for an app. This means that it only works in the context of private apps
I'm not sure how to add tests correctly and there does not seem to be any for GithubIntegration, so I skipped that part. Manual testing works and existing tests are not broken
I could not resist fixing some type stubs along the way

codecov-commenter · 2021-06-30T21:45:28Z

Codecov Report

Base: 98.75% // Head: 98.77% // Increases project coverage by +0.01% 🎉

Coverage data is based on head (38fb3cd) compared to base (7cf3dfc).
Patch coverage: 95.86% of modified lines in pull request are covered.

❗ Current head 38fb3cd differs from pull request most recent head a7ca0cb. Consider uploading reports for the commit a7ca0cb to get more accurate results

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1986      +/-   ##
==========================================
+ Coverage   98.75%   98.77%   +0.01%     
==========================================
  Files         115      116       +1     
  Lines       11595    11669      +74     
==========================================
+ Hits        11451    11526      +75     
+ Misses        144      143       -1

Impacted Files	Coverage Δ
github/Requester.py	`97.46% <90.62%> (-0.69%)`	⬇️
github/GithubIntegration.py	`96.87% <96.87%> (ø)`
github/Consts.py	`100.00% <100.00%> (ø)`
github/InstallationAuthorization.py	`92.10% <100.00%> (-0.21%)`	⬇️
github/MainClass.py	`99.23% <100.00%> (+2.29%)`	⬆️
github/__init__.py	`100.00% <100.00%> (ø)`
github/GitRelease.py	`99.35% <0.00%> (-0.03%)`	⬇️

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.

dblanchette

Added some comments for easier reviewing.

dblanchette · 2021-06-30T21:43:23Z

github/Consts.py

@@ -125,3 +125,12 @@

 # https://developer.github.com/changes/2018-10-16-deployments-environments-states-and-auto-inactive-updates/
 deploymentStatusEnhancementsPreview = "application/vnd.github.flash-preview+json"
+
+
+DEFAULT_BASE_URL = "https://api.github.com"


Moved those here since they are used in 2 files now (MainClass and GithubIntegration)

Comparing this is really difficult. Would have been better to do the refactoring (without changes) in a separate PR and base this PR on that. Then we would see only changes, not moving unchanged code around.

dblanchette · 2021-06-30T21:44:04Z

github/GithubException.py

@@ -38,7 +38,7 @@ class GithubException(Exception):
    Some other types of exceptions might be raised by underlying libraries, for example for network-related issues.
    """

-    def __init__(self, status, data, headers):
+    def __init__(self, status, data, headers=None):


I had a 404 while testing and had an exception because the headers parameters was not passed.

There seems to be a fix there for the same issue: #2079

I think fixing the calling code as in #2079 is better than changing the signature.

dblanchette · 2021-06-30T21:44:58Z

github/GithubIntegration.py

@@ -0,0 +1,140 @@
+import time


Moved from MainClass, it's already big as it is and that does not really belong there.

dblanchette · 2021-06-30T21:46:23Z

github/GithubIntegration.py

+            status=response.status_code, data=response.text
+        )
+
+    @deprecated.deprecated("Use get_repo_installation")


Let me know if that does not suit you. I feel that this call is badly named

This is the existing call? It should probably also state it's deprecated in the docstring. Also, it should mention the class, not just the bare function.

dblanchette · 2021-06-30T21:47:57Z

github/GithubIntegration.py

+    def get_repo_installation(self, owner, repo):
+        return self.get_installation(owner, repo)
+
+    def get_installations(self):


This is the part I added in that file. Users having a private app with more than one installations can use this call as the main class always uses the first found installation.

dblanchette · 2021-06-30T21:49:32Z

github/Requester.py

@@ -304,6 +310,9 @@ def __init__(
    ):
        self._initializeDebugFeature()

+        self.__installation_authorization = None


The heart of the changes I made is here!

maxdanilov · 2021-09-02T08:54:17Z

also very interested in seeing this implemented, are there any updates on this feature?

simkimsia · 2021-09-27T07:45:02Z

Will this be integrated?

jonapich · 2021-09-27T11:05:31Z

@s-t-e-v-e-n-k sorry to ping you directly; there hasn't been any feedback from the maintainers on this PR for close to a month now and I'm not sure who's in charge. What can we do to speed up things and help you out?

tscully49 · 2021-10-08T17:24:14Z

Would also really like to see this feature! How can we help?

stale · 2022-01-08T23:20:09Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

vitalii94lukianenko · 2022-01-10T09:43:55Z

Hi, Could you merge this PR?

ThomasJRyan · 2022-01-10T20:23:21Z

Would definitely appreciate this feature being merged in

github/MainClass.pyi

EnricoMi · 2022-01-10T21:34:32Z

github/GithubException.py

@@ -38,7 +38,7 @@ class GithubException(Exception):
    Some other types of exceptions might be raised by underlying libraries, for example for network-related issues.
    """

-    def __init__(self, status, data, headers):
+    def __init__(self, status, data, headers=None):


I think fixing the calling code as in #2079 is better than changing the signature.

github/GithubIntegration.py

dblanchette · 2022-01-11T15:53:40Z

Thanks for the review! I made the requested changes.

dblanchette · 2022-01-13T14:17:26Z

@EnricoMi @ThomasJRyan Anything else I should do?

github/ApplicationOAuth.pyi

github/Requester.py

github/CheckSuite.pyi

github/Commit.pyi

…authentication

dblanchette · 2022-01-17T14:39:51Z

I went and look at all the files I changed to make sure there were no default values left in pyi. Crossing my fingers that everything is ok now 🤞

dblanchette · 2022-01-21T20:50:44Z

I hate to ask again, but is it all good?

I'm looking forward to go back to using the official library instead of our fork.

Thanks for your work maintaining this library!

…on' into feature/github-app-authentication

dblanchette · 2023-02-01T16:20:03Z

Now fails tests with E AttributeError: module 'github' has no attribute 'AppAuthentication'

That is fixed. I must say I find the imports in this project to be done in a peculiar fashion.

dblanchette · 2023-02-01T16:23:01Z

Comparing this is really difficult. Would have been better to do the refactoring (without changes) in a separate PR and base this PR on that. Then we would see only changes, not moving unchanged code around.

Agreed. I tend to go all out in refactoring as you've seen with the unrelated changes. I'll know to do that in the future.

Thank you both for the reviews!

github/AppAuthentication.py

github/MainClass.py

dblanchette · 2023-02-02T14:34:51Z

All good now? I'm excited to see this release :)

EnricoMi · 2023-02-19T12:25:58Z

@dblanchette can you please take a look at #2420 / #2425?

EnricoMi · 2023-02-21T09:51:29Z

@dblanchette another issue likely being introduced by this PR: #2431

EnricoMi · 2023-02-22T08:30:39Z

Another issue: #2432

EnricoMi · 2023-03-01T06:32:50Z

This is quite some fallout, another issue reported: #2436

Support full GitHub app authentication

885a522

dblanchette commented Jun 30, 2021

View reviewed changes

dblanchette mentioned this pull request Jul 5, 2021

Support full GitHub app authentication coveord/PyGithub#2

Merged

Fix main class type

a719746

dblanchette added 3 commits October 15, 2021 15:20

Merge master and fix conflict

c39a183

Remove unused imports

d8b20e1

Merge branch 'master' into feature/github-app-authentication

30a11a0

stale bot added the stale label Jan 8, 2022

stale bot removed the stale label Jan 10, 2022

EnricoMi reviewed Jan 10, 2022

View reviewed changes

ThomasJRyan reviewed Jan 11, 2022

View reviewed changes

github/GithubIntegration.py Outdated Show resolved Hide resolved

Code review changes

c15de72

Fix type annotation

f8a071b

EnricoMi reviewed Jan 14, 2022

View reviewed changes

github/ApplicationOAuth.pyi Outdated Show resolved Hide resolved

github/Requester.py Show resolved Hide resolved

Remove default values in type hints

e957467

EnricoMi reviewed Jan 16, 2022

View reviewed changes

github/CheckSuite.pyi Outdated Show resolved Hide resolved

github/Commit.pyi Outdated Show resolved Hide resolved

dblanchette added 2 commits January 17, 2022 09:22

Remove default values in .pyi (again)

add6683

Merge remote-tracking branch 'origin/master' into feature/github-app-…

3ba6e76

…authentication

dblanchette added 4 commits February 1, 2023 10:59

Merge remote-tracking branch 'coveord/feature/github-app-authenticati…

a7ca0cb

…on' into feature/github-app-authentication

Fix an assert

7dd9fa9

black

7e6d5b7

Remove useless check

148c14f

dblanchette requested review from s-t-e-v-e-n-k and EnricoMi and removed request for s-t-e-v-e-n-k and EnricoMi February 1, 2023 16:23

EnricoMi reviewed Feb 2, 2023

View reviewed changes

github/AppAuthentication.py Outdated Show resolved Hide resolved

github/MainClass.py Outdated Show resolved Hide resolved

Fix an assert and a docstring

3e16166

dblanchette requested a review from EnricoMi February 3, 2023 14:29

s-t-e-v-e-n-k approved these changes Feb 4, 2023

View reviewed changes

s-t-e-v-e-n-k merged commit 5e27c10 into PyGithub:master Feb 6, 2023

bluek1te mentioned this pull request Feb 14, 2023

#1986 seems to break setting GitHub base URL #2420

Closed

EnricoMi mentioned this pull request Feb 21, 2023

401 unauthorized after upgrading to 1.58.0 #2431

Closed

jodelasur mentioned this pull request Mar 1, 2023

Add cryptography as dependency (for app-based auth) #2441

Closed

This was referenced Mar 19, 2023

Fix GithubIntegration that uses expiring jwt #2460

Merged

Does PyGitHub follow semantic versioning? #2472

Closed

EnricoMi mentioned this pull request Jun 7, 2023

github/MainClass: Fix GitHubException instantiation #2079

Closed

jeremyd2019 mentioned this pull request Jul 15, 2023

Organization self-hosted runner registration missing #2117

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support full GitHub app authentication #1986

Support full GitHub app authentication #1986

dblanchette commented Jun 30, 2021

codecov-commenter commented Jun 30, 2021 •

edited

dblanchette left a comment

dblanchette Jun 30, 2021

EnricoMi Feb 1, 2023

dblanchette Jun 30, 2021

dblanchette Oct 15, 2021

EnricoMi Jan 10, 2022

dblanchette Jun 30, 2021

dblanchette Jun 30, 2021

s-t-e-v-e-n-k Jan 26, 2023

dblanchette Jun 30, 2021

dblanchette Jun 30, 2021

maxdanilov commented Sep 2, 2021

simkimsia commented Sep 27, 2021

jonapich commented Sep 27, 2021

tscully49 commented Oct 8, 2021

stale bot commented Jan 8, 2022

vitalii94lukianenko commented Jan 10, 2022

ThomasJRyan commented Jan 10, 2022

EnricoMi Jan 10, 2022

dblanchette commented Jan 11, 2022

dblanchette commented Jan 13, 2022

dblanchette commented Jan 17, 2022

dblanchette commented Jan 21, 2022

dblanchette commented Feb 1, 2023

dblanchette commented Feb 1, 2023

dblanchette commented Feb 2, 2023

EnricoMi commented Feb 19, 2023

EnricoMi commented Feb 21, 2023

EnricoMi commented Feb 22, 2023

EnricoMi commented Mar 1, 2023 •

edited

Support full GitHub app authentication #1986

Support full GitHub app authentication #1986

Conversation

dblanchette commented Jun 30, 2021

codecov-commenter commented Jun 30, 2021 • edited

Codecov Report

dblanchette left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxdanilov commented Sep 2, 2021

simkimsia commented Sep 27, 2021

jonapich commented Sep 27, 2021

tscully49 commented Oct 8, 2021

stale bot commented Jan 8, 2022

vitalii94lukianenko commented Jan 10, 2022

ThomasJRyan commented Jan 10, 2022

Choose a reason for hiding this comment

dblanchette commented Jan 11, 2022

dblanchette commented Jan 13, 2022

dblanchette commented Jan 17, 2022

dblanchette commented Jan 21, 2022

dblanchette commented Feb 1, 2023

dblanchette commented Feb 1, 2023

dblanchette commented Feb 2, 2023

EnricoMi commented Feb 19, 2023

EnricoMi commented Feb 21, 2023

EnricoMi commented Feb 22, 2023

EnricoMi commented Mar 1, 2023 • edited

codecov-commenter commented Jun 30, 2021 •

edited

EnricoMi commented Mar 1, 2023 •

edited