Add support for get_app() with App authentication #2549

chantra · 2023-06-09T07:16:31Z

An authenticated app can call /app to get details about itself.

Authentication must be done using jwt for authentication or it will fail with an error like https://gist.github.com/chantra/209819389b2dec6cc6e03a13d301f5e5

chantra · 2023-06-09T07:19:04Z

@EnricoMi , I am not super satisfied of the current solution. Let me know how you would see this better integrated.

It seems to me that wether through GithubIntegration, or Github main object, we need to detect that the authentication will be done using JWT. In MainClass.py, I use the token_type as a proxy for this.
I don't think GithubIntegration is exclusively JWT auth based, so the same should probably apply.
In any cases, the current solution is wonky. Happy to iterate on it.

codecov-commenter · 2023-06-09T07:19:17Z

Codecov Report

Patch coverage: 92.85% and no project coverage change.

Comparison is base (6a21761) 98.50% compared to head (88df8f4) 98.50%.

❗ 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    #2549   +/-   ##
=======================================
  Coverage   98.50%   98.50%           
=======================================
  Files         128      128           
  Lines       12753    12757    +4     
=======================================
+ Hits        12562    12566    +4     
  Misses        191      191

Impacted Files	Coverage Δ
github/Auth.py	`93.27% <66.66%> (ø)`
github/AccessToken.py	`100.00% <100.00%> (ø)`
github/AuthenticatedUser.py	`99.04% <100.00%> (ø)`
github/GithubIntegration.py	`96.61% <100.00%> (+0.24%)`	⬆️
github/MainClass.py	`99.27% <100.00%> (ø)`
github/Repository.py	`97.13% <100.00%> (ø)`

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

github/MainClass.py

tests/GithubApp.py

github/MainClass.py

tests/GithubApp.py

github/MainClass.py

github/GithubIntegration.py

chantra · 2023-06-09T21:51:26Z

Thanks @EnricoMi

Applied your recommendations in the second commit. If that works out, feel free to squash it into the first one.

chantra · 2023-06-12T18:33:55Z

running manually, this is failing due to utcnow deprecation:

tests/GithubApp.py .F                                                                                                                                                                                                                                                                                            [100%]

======================================================================================================================================================= FAILURES =======================================================================================================================================================
________________________________________________________________________________________________________________________________________ GithubAppAuth.testGetAuthenticatedApp _________________________________________________________________________________________________________________________________________

self = <tests.GithubApp.GithubAppAuth testMethod=testGetAuthenticatedApp>

    def testGetAuthenticatedApp(self):
        with self.assertWarns(DeprecationWarning) as warning:
            app = self.g.get_app()

>           self.assertWarning(
                warning,
                "Argument slug is mandatory, calling this method without the slug argument is deprecated, please use github.GithubIntegration(auth=github.Auth.AppAuth(...)).get_app() instead",
            )

tests/GithubApp.py:111:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
tests/Framework.py:334: in assertWarning
    self.assertWarnings(warning, expected)
tests/Framework.py:337: in assertWarnings
    self.assertEqual(len(warning.warnings), len(expecteds))
E   AssertionError: 2 != 1
=================================================================================================================================================== warnings summary ===================================================================================================================================================
github/Repository.py:3476
  /Users/chantra/devel/PyGithub/github/Repository.py:3476: DeprecationWarning: datetime.utcnow() is deprecated and scheduled for removal in a future version. Use timezone-aware objects to represent datetimes in UTC: datetime.now(datetime.UTC).
    def mark_notifications_as_read(self, last_read_at=datetime.datetime.utcnow()):

github/AuthenticatedUser.py:1232
  /Users/chantra/devel/PyGithub/github/AuthenticatedUser.py:1232: DeprecationWarning: datetime.utcnow() is deprecated and scheduled for removal in a future version. Use timezone-aware objects to represent datetimes in UTC: datetime.now(datetime.UTC).
    def mark_notifications_as_read(self, last_read_at=datetime.datetime.utcnow()):

tests/GithubApp.py::GithubApp::testGetPublicApp
  /Users/chantra/devel/PyGithub/.py312/lib/python3.12/site-packages/httpretty/core.py:1077: DeprecationWarning: datetime.utcnow() is deprecated and scheduled for removal in a future version. Use timezone-aware objects to represent datetimes in UTC: datetime.now(datetime.UTC).
    now = datetime.utcnow()

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
=============================================================================================================================================== short test summary info ================================================================================================================================================
FAILED tests/GithubApp.py::GithubAppAuth::testGetAuthenticatedApp - AssertionError: 2 != 1

While cI can fix the deprecation for pygithub, httpretty will still generate a warning.

chantra · 2023-06-12T18:34:49Z

I am introducing assertWarningIn to be able to check for a specific warning in the list of warnings.

EnricoMi · 2023-06-13T05:58:18Z

running manually, this is failing due to utcnow deprecation:

Then use the recommended datetime.now(datetime.UTC).

EnricoMi

Almost there!

tests/Framework.py

EnricoMi · 2023-06-13T06:11:38Z

tests/GithubApp.py

+
+class GithubAppAuth(Framework.TestCase):
+    def setUp(self):
+        self.appAuthMode = True


This sets auth mode for all subsequent tests, this should only be used by tests.conftest.pytest_configure via command line arguments like --record.

Please use the same way of authentication as used in tests/GithubIntegration.py:

auth = github.Auth.AppAuth(APP_ID, PRIVATE_KEY) g = github.Github(auth=auth) app = g.get_app()

You can import APP_ID and PRIVATE_KEY from tests/GithubIntegration.py.

So this GithubAppAuth class and def setUp can be removed.

ok, I removed this code from the PR then as it is not required. This feature would then be orthogonal to this PR.

EnricoMi · 2023-06-13T06:12:51Z

tests/Framework.py

@@ -410,6 +437,10 @@ def activateJWTAuthMode():  # pragma no cover (Function useful only when recordi
    BasicTestCase.jwtAuthMode = True


+def activateAppAuthMode():  # pragma no cover (Function useful only when recording new tests, not used during automated tests)


this needs to be added to tests.conftest.pytest_configure to be useful

github/GithubIntegration.py

EnricoMi · 2023-06-13T06:36:52Z

github/MainClass.py

+                "github.GithubIntegration(auth=github.Auth.AppAuth(...)).get_app() instead",
+                category=DeprecationWarning,
+            )
+            return GithubIntegration(auth=self.__requester._Requester__auth).get_app()


Please rebase and access Requester.auth:

Suggested change

return GithubIntegration(auth=self.__requester._Requester__auth).get_app()

return GithubIntegration(auth=self.__requester.auth).get_app()

chantra · 2023-06-13T08:12:19Z

running manually, this is failing due to utcnow deprecation:

Then use the recommended datetime.now(datetime.UTC).

Per my commit message in c3a2d35 , datetime.UTC was introduced in 3.11, datetime.timezone.utc works across the board of python 3.X tested in this project.

chantra · 2023-06-13T20:18:28Z

ok, I force-pushed an update, but TL;DR:

removed appAuthMode from Framework as it is not actively used after I followed your suggestion
access .auth directly through requester
test that we assert when provided a non AppAuth to GithubIntegration

EnricoMi · 2023-06-14T06:03:16Z

tests/GithubApp.py

+            self.assertWarningIn(
+                warning,
+                "Argument slug is mandatory, calling this method without the slug argument is deprecated, please use github.GithubIntegration(auth=github.Auth.AppAuth(...)).get_app() instead",
+            )


Suggested change

self.assertWarningIn(

warning,

"Argument slug is mandatory, calling this method without the slug argument is deprecated, please use github.GithubIntegration(auth=github.Auth.AppAuth(...)).get_app() instead",

)

self.assertWarningIn(

warning,

"Argument slug is mandatory, calling this method without the slug argument is deprecated, "

"please use github.GithubIntegration(auth=github.Auth.AppAuth(...)).get_app() instead",

)

EnricoMi · 2023-06-14T06:04:10Z

tests/GithubIntegration.py

+            with self.assertRaisesRegex(
+                AssertionError,
+                "GithubIntegration requires github.Auth.AppAuth authentication, not .*",
+            ):
+                github.GithubIntegration(auth=self.oauth_token)


Suggested change

with self.assertRaisesRegex(

AssertionError,

"GithubIntegration requires github.Auth.AppAuth authentication, not .*",

):

github.GithubIntegration(auth=self.oauth_token)

with self.assertRaises(

AssertionError,

f"GithubIntegration requires github.Auth.AppAuth authentication, not {type(auth)}",

):

github.GithubIntegration(auth=auth)

good catch on the auth vs self.oauth_token :)

EnricoMi · 2023-06-14T06:04:56Z

tests/Framework.py

-                )
-                if GithubCredentials.app_id and GithubCredentials.app_private_key
-                else None
-            )


No need to remove this existing code.

I had added this, do you want me to re-add it even if this is not used here?

This diff tells me, this was there before and your pull-request removes it.

This is the last bit left. I'd say there is no need to remove this code by this pull request.

hum, I was sure I had added that when I was creating a new test class.... I may have it mixed up with something else. Will re-introduce.

EnricoMi · 2023-06-14T06:16:33Z

tests/GithubApp.py

+        with self.assertWarns(DeprecationWarning) as warning:
+            app = g.get_app()
+
+            self.assertWarningIn(


Suggested change

self.assertWarningIn(

self.assertWarning(

The problem is that httpretty/core generates another warning, see #2549 (comment)

Which Python version are you using?

That's happening with 3.12. See this run/failure: https://github.com/PyGithub/PyGithub/actions/runs/5226324529/jobs/9436862034#step:5:153

EnricoMi · 2023-06-15T19:08:40Z

tests/GithubApp.py

+        with self.assertWarns(DeprecationWarning) as warning:
+            app = g.get_app()
+
+            self.assertWarningIn(


To be honest, I don't like assertWarningIn, because it may hide unexpected warnings that we want to fail on.

Having httpretty warn here is annoying, but we can disable that and everything works as expected:

Suggested change

with self.assertWarns(DeprecationWarning) as warning:

app = g.get_app()

self.assertWarningIn(

with self.assertWarns(DeprecationWarning) as warning:

# we ignore warnings from httpretty dependency

import warnings

warnings.filterwarnings("ignore", module="httpretty")

app = g.get_app()

self.assertWarning(

oh great, I did not know we could filter the warnings. Will change. Thanks

EnricoMi · 2023-06-15T19:10:15Z

tests/Framework.py

+    def assertWarnings(self, warning, *expecteds):
+        self.assertEqual(len(warning.warnings), len(expecteds))
+        for message, expected in zip(warning.warnings, expecteds):
+            self.assertIsInstance(message, warnings.WarningMessage)
+            self.assertIsInstance(message.message, DeprecationWarning)
+            self.assertEqual(message.message.args, (expected,))


An error like

E AssertionError: 2 != 1

is not really helpful, lets improve this:

Suggested change

def assertWarnings(self, warning, *expecteds):

self.assertEqual(len(warning.warnings), len(expecteds))

for message, expected in zip(warning.warnings, expecteds):

self.assertIsInstance(message, warnings.WarningMessage)

self.assertIsInstance(message.message, DeprecationWarning)

self.assertEqual(message.message.args, (expected,))

def assertWarnings(self, warning, *expecteds):

actual = [(type(message), type(message.message), message.message.args) for message in warning.warnings]

expected = [(warnings.WarningMessage, DeprecationWarning, (expected,)) for expected in expecteds]

self.assertSequenceEqual(actual, expected)

An authenticated app can call `/app` to get details about itself. Authentication must be done using jwt for authentication or it will fail with an error like https://gist.github.com/chantra/209819389b2dec6cc6e03a13d301f5e5

chantra · 2023-06-16T05:16:43Z

rebased, re-ported utcnow change and got rid of assertWarningIn.

…e can re-use it across tests. * assert deprecation warning and transparently call GithubIntegration.get_app() when no slug is provided * check that deprecation warning happens in test case * assert that auth is an AppAuth in GithubIntegration and test it

https://docs.python.org/dev/whatsnew/3.12.html > datetime.datetime’s utcnow() and utcfromtimestamp() are deprecated and will be removed in a future version. Instead, use timezone-aware objects to represent datetimes in UTC: respectively, call now() and fromtimestamp() with the tz parameter set to datetime.UTC. (Contributed by Paul Ganssle in gh-103857.) `datetime.UTC is new in 3.11, but is essentially an alias to datetime.timezone.utc This change replaces the occurences of utcnow() with datetime.timezone.utc so it works on older python releases.

In some cases, some deprecation messages may be generated by third-party libraries depedending on the python version we are using. This is a problem when we want to insure that a specific message is raised. This change introduce `assertWarningIn`, which will check if a specific deprecation warning is in the list of warnings.

chantra · 2023-06-16T05:55:33Z

ok, if I did not mess up, the CI should be green in a few and all feedback addressed :D

EnricoMi

LGTM

EnricoMi · 2023-06-16T06:05:31Z

Thank you for your patience!

chantra · 2023-06-16T19:03:34Z

Thanks to you too!

EnricoMi requested changes Jun 9, 2023

View reviewed changes

github/MainClass.py Show resolved Hide resolved

tests/GithubApp.py Outdated Show resolved Hide resolved

EnricoMi changed the title ~~support get_app() for App authentication mode~~ Add support for get_app() with App authentication Jun 9, 2023

EnricoMi requested changes Jun 9, 2023

View reviewed changes

github/MainClass.py Outdated Show resolved Hide resolved

tests/GithubApp.py Outdated Show resolved Hide resolved

github/MainClass.py Show resolved Hide resolved

github/GithubIntegration.py Show resolved Hide resolved

chantra force-pushed the ghapp_getapp branch from 525cc3d to 82be8eb Compare June 10, 2023 00:06

chantra requested a review from EnricoMi June 12, 2023 18:52

EnricoMi requested changes Jun 13, 2023

View reviewed changes

chantra force-pushed the ghapp_getapp branch from ffdbe46 to b0ede65 Compare June 13, 2023 20:15

chantra force-pushed the ghapp_getapp branch 3 times, most recently from bf72672 to f09c6b8 Compare June 14, 2023 04:29

chantra requested a review from EnricoMi June 14, 2023 05:01

EnricoMi requested changes Jun 14, 2023

View reviewed changes

chantra force-pushed the ghapp_getapp branch from f09c6b8 to f99c125 Compare June 14, 2023 07:35

chantra requested a review from EnricoMi June 15, 2023 16:41

EnricoMi requested changes Jun 15, 2023

View reviewed changes

chantra force-pushed the ghapp_getapp branch from c3f7bb5 to 043cd23 Compare June 16, 2023 04:42

support get_app() for App authentication mode

33421c8

An authenticated app can call `/app` to get details about itself. Authentication must be done using jwt for authentication or it will fail with an error like https://gist.github.com/chantra/209819389b2dec6cc6e03a13d301f5e5

chantra force-pushed the ghapp_getapp branch 2 times, most recently from 6d5e5eb to 8b1df3f Compare June 16, 2023 05:11

chantra requested a review from EnricoMi June 16, 2023 05:16

chantra added 4 commits June 15, 2023 22:53

[tests] filter httpretty warnings

88df8f4

chantra force-pushed the ghapp_getapp branch from 8b1df3f to 88df8f4 Compare June 16, 2023 05:55

EnricoMi approved these changes Jun 16, 2023

View reviewed changes

EnricoMi merged commit 6d4b6d1 into PyGithub:master Jun 16, 2023
10 checks passed

chantra deleted the ghapp_getapp branch June 16, 2023 19:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for get_app() with App authentication #2549

Add support for get_app() with App authentication #2549

chantra commented Jun 9, 2023

chantra commented Jun 9, 2023

codecov-commenter commented Jun 9, 2023 •

edited

chantra commented Jun 9, 2023

chantra commented Jun 12, 2023

chantra commented Jun 12, 2023

EnricoMi commented Jun 13, 2023

EnricoMi left a comment

EnricoMi Jun 13, 2023

chantra Jun 13, 2023

EnricoMi Jun 13, 2023

EnricoMi Jun 13, 2023

chantra commented Jun 13, 2023

chantra commented Jun 13, 2023

EnricoMi Jun 14, 2023

EnricoMi Jun 14, 2023

chantra Jun 14, 2023

EnricoMi Jun 14, 2023

chantra Jun 14, 2023

EnricoMi Jun 14, 2023

EnricoMi Jun 16, 2023

chantra Jun 16, 2023

EnricoMi Jun 14, 2023

chantra Jun 14, 2023

EnricoMi Jun 14, 2023

chantra Jun 15, 2023

EnricoMi Jun 15, 2023

chantra Jun 16, 2023

EnricoMi Jun 15, 2023

chantra commented Jun 16, 2023

chantra commented Jun 16, 2023

EnricoMi left a comment

EnricoMi commented Jun 16, 2023

chantra commented Jun 16, 2023

		@@ -410,6 +437,10 @@ def activateJWTAuthMode(): # pragma no cover (Function useful only when recordi
		BasicTestCase.jwtAuthMode = True


		def activateAppAuthMode(): # pragma no cover (Function useful only when recording new tests, not used during automated tests)

	return GithubIntegration(auth=self.__requester._Requester__auth).get_app()
	return GithubIntegration(auth=self.__requester.auth).get_app()

Add support for get_app() with App authentication #2549

Add support for get_app() with App authentication #2549

Conversation

chantra commented Jun 9, 2023

chantra commented Jun 9, 2023

codecov-commenter commented Jun 9, 2023 • edited

Codecov Report

chantra commented Jun 9, 2023

chantra commented Jun 12, 2023

chantra commented Jun 12, 2023

EnricoMi commented Jun 13, 2023

EnricoMi 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

chantra commented Jun 13, 2023

chantra commented Jun 13, 2023

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

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

chantra commented Jun 16, 2023

chantra commented Jun 16, 2023

EnricoMi left a comment

Choose a reason for hiding this comment

EnricoMi commented Jun 16, 2023

chantra commented Jun 16, 2023

codecov-commenter commented Jun 9, 2023 •

edited