Add authentication classes, move auth logic there #2528

EnricoMi · 2023-05-10T18:43:04Z

Reduces complexity from Requester and moves lots of auth-specific logic into Auth implementations.

Allows for easy fix of #2471 and #2504: EnricoMi#4.

Fixes bad code like

Line 94 in b94a83c

self._requester._Requester__authorizationHeader = None

https://github.com/PyGithub/PyGithub/pull/2528/files#diff-e1bc4144bb47d4a87072d8149e91f8c66463d81346d958b6f5d3d7dcd9f33375

This is backward compatible to existing auth arguments.

codecov-commenter · 2023-05-10T18:48:44Z

Codecov Report

Patch coverage: 91.36% and project coverage change: -0.12 ⚠️

Comparison is base (a8e7c42) 98.68% compared to head (756e953) 98.57%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2528      +/-   ##
==========================================
- Coverage   98.68%   98.57%   -0.12%     
==========================================
  Files         117      118       +1     
  Lines       11831    11980     +149     
==========================================
+ Hits        11676    11809     +133     
- Misses        155      171      +16

Impacted Files	Coverage Δ
github/Auth.py	`89.30% <89.30%> (ø)`
github/GithubIntegration.py	`96.36% <92.30%> (-0.25%)`	⬇️
github/Requester.py	`97.97% <95.65%> (+0.45%)`	⬆️
github/AppAuthentication.py	`100.00% <100.00%> (ø)`
github/ApplicationOAuth.py	`97.91% <100.00%> (+0.09%)`	⬆️
github/Consts.py	`100.00% <100.00%> (ø)`
github/MainClass.py	`99.27% <100.00%> (+0.04%)`	⬆️
github/__init__.py	`93.75% <100.00%> (+0.41%)`	⬆️

... and 1 file with indirect coverage changes

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

JLLeitschuh · 2023-06-06T15:41:49Z

github/Auth.py

+        self._app_id = app_id
+        self._private_key = private_key


Probably good to fail-fast here and verify that these aren't empty or None at this point

I agree, but asserting for not None is not common in this project, and I am too lazy to start this.

Given the arguments are not optional, None values would be surprising from a user point of view. Also, typing suggests None is not allowed.

Sure, but id suggest checking for empty.

The rationale being that, for most of the API, the use of the passed arguments is immediate, here they are passed and used later.

JLLeitschuh · 2023-06-07T12:59:12Z

github/Auth.py

+    """
+
+    def __init__(self, token: str):
+        self._token = token


I would add a type check, and an emptiness check here as a sanity check

JLLeitschuh · 2023-06-07T13:01:06Z

github/Auth.py

General suggestion for this class. Consider using the python @dataclass annotation on these classes with the frozen=True flag.

These classes should be immutable, right?

I would love to use immutable classes here, but I prefer to keep the code base consistent. Maybe a future rework.

Btw., some auth classes like the AppInstallationAuth has to refresh its token, so they are not all immutable.

JLLeitschuh · 2023-06-07T13:01:51Z

github/Auth.py

+        from github.GithubIntegration import GithubIntegration
+
+        self.__integration: Optional[GithubIntegration] = None
+        self.__installation_authorization: Optional[InstallationAuthorization] = None


Can't these be declared at the top of the class?

The import is here for typing. It is imported here because this would cause a circular import otherwise.

The declaration cannot happen in the class body as this would also cause the circular import. Suggestions welcome.

I stand corrected, the import can happen in the class body, will fix.

JLLeitschuh · 2023-06-07T13:06:34Z

github/MainClass.py

+        if password is not None:
+            warnings.warn(
+                "Arguments login_or_token and password are deprecated, please use "
+                "auth=github.Auth.Login(...) instead",
+                category=DeprecationWarning,
+            )
+            auth = Auth.Login(login_or_token, password)
+        elif login_or_token is not None:
+            warnings.warn(
+                "Argument login_or_token is deprecated, please use "
+                "auth=github.Auth.Token(...) instead",
+                category=DeprecationWarning,
+            )
+            auth = Auth.Token(login_or_token)
+        elif jwt is not None:
+            warnings.warn(
+                "Argument jwt is deprecated, please use "
+                "auth=github.Auth.AppAuth(...) or "
+                "auth=github.Auth.AppAuthToken(...) instead",
+                category=DeprecationWarning,
+            )
+            auth = Auth.AppAuthToken(jwt)
+        elif app_auth is not None:
+            warnings.warn(
+                "Argument app_auth is deprecated, please use "
+                "auth=github.Auth.AppInstallationAuth(...) instead",
+                category=DeprecationWarning,
+            )
+            auth = app_auth


What if no auth is provided, is that anonymous auth at that point? Should an Auth.Auth type for anonymous access be added too, just so that there isn't a 'None' case?

Also, is there any standard in the library for being explicit about what version release of the library will formally remove the support for these deprecated APIs?

Well, I think that should be removed in the next major release (v2) as this is quite fundamental / widespread use.

We have seen lots of small breaking changes in minor releases, recently, and given there is a lot of refactoring still need, I would refrain from following semantic versioning.

What if no auth is provided, is that anonymous auth at that point? Should an Auth.Auth type for anonymous access be added too, just so that there isn't a 'None' case?

Good point. I think auth=None is expressive enough. I don't see benefit for an NoAuth or Anonymous auth class.

JLLeitschuh · 2023-06-07T13:07:44Z

tests/Authentication.py

    def testBasicAuthentication(self):
-        g = github.Github(self.login, self.password)
+        g = github.Github(self.login.login, self.login.password)
        self.assertEqual(g.get_user("jacquev6").name, "Vincent Jacques")

    def testOAuthAuthentication(self):
-        g = github.Github(self.oauth_token)
+        g = github.Github(self.oauth_token.token)
        self.assertEqual(g.get_user("jacquev6").name, "Vincent Jacques")

    def testJWTAuthentication(self):
-        g = github.Github(jwt=self.jwt)
-        self.assertEqual(g.get_user("jacquev6").name, "Vincent Jacques")
-
-    def testUserAgent(self):
-        g = github.Github(user_agent="PyGithubTester")
+        g = github.Github(jwt=self.jwt.token)
        self.assertEqual(g.get_user("jacquev6").name, "Vincent Jacques")


Any assertions you want to put in here around verifying that the warning is logged?

JLLeitschuh · 2023-06-07T14:02:37Z

github/Auth.py

+)
+
+
+class Auth:


I think to be correct, this needs to also implement ABC

Suggested change

class Auth:

class Auth(abc.ABC):

JLLeitschuh

One final thing about ABC, and I think this is good to go

JLLeitschuh

LGTM!

…rnings

EnricoMi · 2023-06-08T08:08:33Z

Thanks!

EnricoMi force-pushed the requester-auth-rework branch from d6b0c07 to aefd5e6 Compare May 10, 2023 20:05

EnricoMi added this to the Version 1.59.0 milestone May 10, 2023

EnricoMi force-pushed the requester-auth-rework branch from aefd5e6 to 13ce695 Compare May 10, 2023 20:08

EnricoMi force-pushed the requester-auth-rework branch 2 times, most recently from b70a186 to 1add482 Compare June 1, 2023 07:43

EnricoMi mentioned this pull request Jun 2, 2023

401 unauthorized after upgrading to 1.58.0 #2431

Closed

EnricoMi requested review from JLLeitschuh, s-t-e-v-e-n-k and sfdye June 6, 2023 15:13

EnricoMi marked this pull request as ready for review June 6, 2023 15:13

EnricoMi force-pushed the requester-auth-rework branch from 91e65cf to 1b92df1 Compare June 6, 2023 15:13

JLLeitschuh requested changes Jun 6, 2023

View reviewed changes

JLLeitschuh requested changes Jun 7, 2023

View reviewed changes

EnricoMi added 16 commits June 7, 2023 15:56

Add authentication classes, move auth logic there

c2154a6

Move typing of Auth from .pyi to py file

3c49585

Access Requester's base url through property

5cc6633

Linting

4f04d13

Allow passing requester with AppAuth to AppAuth.get_installation_auth

6e9a7ec

Linting

dbf1542

Add tests, add github.Auth to typing

8a458b7

Add authentication examples to docs

1cb16bb

Use warnings package for MainClass args deprecation warning

39e2cdd

Move tests to new auth classes

1a9973f

Use AppAuth in GithubIntegration

763628e

Fix linting

a3fae5a

Fix imports and unwanted deprecation warnings

c18f6a6

Add missing replay data

ebb1813

One more deprecation

96d5fb7

More input assertions, assert warnings

1c2532c

EnricoMi force-pushed the requester-auth-rework branch from 4a9c4aa to 1c2532c Compare June 7, 2023 13:56

JLLeitschuh reviewed Jun 7, 2023

View reviewed changes

JLLeitschuh requested changes Jun 7, 2023

View reviewed changes

Derive Auth from abc.ABC

42772d8

JLLeitschuh approved these changes Jun 7, 2023

View reviewed changes

EnricoMi added 6 commits June 7, 2023 21:43

Fix record mode to work with empty credentials

5ceb792

Remove redundant headers from GithubIntegration, fixes deprecation wa…

3140c6b

…rnings

Move JWT tests into Authentication tests

2fb59d9

Mock time.time() with mock.patch

9f4a05c

Assert all warnings

ce8e9df

Fix lint

756e953

EnricoMi merged commit fc2d0e1 into PyGithub:master Jun 8, 2023

EnricoMi mentioned this pull request Jun 19, 2023

Reworking GithubIntegration #2461

Closed

araffin mentioned this pull request Nov 11, 2024

Add minimum version for pygithub optuna/optunahub#71

Merged

Uh oh!

Add authentication classes, move auth logic there #2528

Add authentication classes, move auth logic there #2528

Uh oh!

Conversation

EnricoMi commented May 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented May 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JLLeitschuh left a comment

Choose a reason for hiding this comment

Uh oh!

JLLeitschuh left a comment

Choose a reason for hiding this comment

Uh oh!

EnricoMi commented Jun 8, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

EnricoMi commented May 10, 2023 •

edited

Loading

codecov-commenter commented May 10, 2023 •

edited

Loading