Skip to content

Commit

Permalink
Add support for get_app() with App authentication (#2549)
Browse files Browse the repository at this point in the history
Moves `Github.get_app()` called without `slug` parameter into `GithubIntegration`, because it needs an `AppAuth`,
and we collect all such endpoints there. Calling `Github.get_app()` without `slug` parameter is now deprecated.

Replaces `datetime.utcnow()` with `datetime.now(timezone.utc)`, as it is deprecated in Python 3.12.
  • Loading branch information
chantra committed Jun 16, 2023
1 parent 6a21761 commit 6d4b6d1
Show file tree
Hide file tree
Showing 13 changed files with 137 additions and 53 deletions.
4 changes: 2 additions & 2 deletions github/AccessToken.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
# #
################################################################################

from datetime import datetime, timedelta
from datetime import datetime, timedelta, timezone

import github.GithubObject

Expand Down Expand Up @@ -128,7 +128,7 @@ def _initAttributes(self):
self._refresh_expires_in = github.GithubObject.NotSet

def _useAttributes(self, attributes):
self._created = datetime.utcnow()
self._created = datetime.now(timezone.utc)
if "access_token" in attributes: # pragma no branch
self._token = self._makeStringAttribute(attributes["access_token"])
if "token_type" in attributes: # pragma no branch
Expand Down
10 changes: 6 additions & 4 deletions github/Auth.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
import abc
import base64
import time
from datetime import datetime, timedelta
from datetime import datetime, timedelta, timezone
from typing import Dict, Optional, Union

import jwt
Expand Down Expand Up @@ -307,7 +307,7 @@ def _is_expired(self) -> bool:
self.__installation_authorization.expires_at
- TOKEN_REFRESH_THRESHOLD_TIMEDELTA
)
return token_expires_at < datetime.utcnow()
return token_expires_at < datetime.now(timezone.utc)

def _get_installation_authorization(self) -> InstallationAuthorization:
assert (
Expand Down Expand Up @@ -413,7 +413,9 @@ def withRequester(self, requester: Requester) -> "AppUserAuth":

@property
def _is_expired(self) -> bool:
return self._expires_at is not None and self._expires_at < datetime.utcnow()
return self._expires_at is not None and self._expires_at < datetime.now(
timezone.utc
)

def _refresh(self):
if self._refresh_token is None:
Expand All @@ -422,7 +424,7 @@ def _refresh(self):
)
if (
self._refresh_expires_at is not None
and self._refresh_expires_at < datetime.utcnow()
and self._refresh_expires_at < datetime.now(timezone.utc)
):
raise RuntimeError(
"Cannot refresh expired token because refresh token also expired"
Expand Down
4 changes: 3 additions & 1 deletion github/AuthenticatedUser.py
Original file line number Diff line number Diff line change
Expand Up @@ -1229,7 +1229,9 @@ def has_in_watched(self, watched):
)
return status == 200

def mark_notifications_as_read(self, last_read_at=datetime.datetime.utcnow()):
def mark_notifications_as_read(
self, last_read_at=datetime.datetime.now(datetime.timezone.utc)
):
"""
:calls: `PUT /notifications <https://docs.github.com/en/rest/reference/activity#notifications>`_
:param last_read_at: datetime
Expand Down
19 changes: 18 additions & 1 deletion github/GithubIntegration.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

from github import Consts
from github.Auth import AppAuth
from github.GithubApp import GithubApp
from github.GithubException import GithubException
from github.Installation import Installation
from github.InstallationAuthorization import InstallationAuthorization
Expand Down Expand Up @@ -71,7 +72,10 @@ def __init__(
jwt_algorithm=jwt_algorithm,
)

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

self.auth = auth

self.__requester = Requester(
Expand Down Expand Up @@ -213,3 +217,16 @@ def get_app_installation(self, installation_id):
:rtype: :class:`github.Installation.Installation`
"""
return self._get_installed_app(url=f"/app/installations/{installation_id}")

def get_app(self):
"""
:calls: `GET /app <https://docs.github.com/en/rest/reference/apps#get-the-authenticated-app>`_
:rtype: :class:`github.GithubApp.GithubApp`
"""

headers, data = self.__requester.requestJsonAndCheck(
"GET", "/app", headers=self._get_headers()
)
return GithubApp(
requester=self.__requester, headers=headers, attributes=data, completed=True
)
8 changes: 6 additions & 2 deletions github/MainClass.py
Original file line number Diff line number Diff line change
Expand Up @@ -808,8 +808,12 @@ def get_app(self, slug=github.GithubObject.NotSet):
if slug is github.GithubObject.NotSet:
# with no slug given, calling /app returns the authenticated app,
# including the actual /apps/{slug}
headers, data = self.__requester.requestJsonAndCheck("GET", "/app")
return GithubApp.GithubApp(self.__requester, headers, data, completed=True)
warnings.warn(
"Argument slug is mandatory, calling this method without the slug argument is deprecated, please use "
"github.GithubIntegration(auth=github.Auth.AppAuth(...)).get_app() instead",
category=DeprecationWarning,
)
return GithubIntegration(auth=self.__requester.auth).get_app()
else:
# with a slug given, we can lazily load the GithubApp
return GithubApp.GithubApp(
Expand Down
4 changes: 3 additions & 1 deletion github/Repository.py
Original file line number Diff line number Diff line change
Expand Up @@ -3702,7 +3702,9 @@ def get_notifications(
params,
)

def mark_notifications_as_read(self, last_read_at=datetime.datetime.utcnow()):
def mark_notifications_as_read(
self, last_read_at=datetime.datetime.now(datetime.timezone.utc)
):
"""
:calls: `PUT /repos/{owner}/{repo}/notifications <https://docs.github.com/en/rest/reference/activity#notifications>`_
:param last_read_at: datetime
Expand Down
25 changes: 16 additions & 9 deletions tests/ApplicationOAuth.py
Original file line number Diff line number Diff line change
Expand Up @@ -83,8 +83,10 @@ def testGetAccessToken(self):

def testGetAccessTokenWithExpiry(self):
with mock.patch("github.AccessToken.datetime") as dt:
dt.utcnow = mock.Mock(
return_value=datetime.datetime(2023, 6, 7, 12, 0, 0, 123)
dt.now = mock.Mock(
return_value=datetime.datetime(
2023, 6, 7, 12, 0, 0, 123, tzinfo=datetime.timezone.utc
)
)
access_token = self.app.get_access_token(
"oauth_code_removed", state="state_removed"
Expand All @@ -100,13 +102,14 @@ def testGetAccessTokenWithExpiry(self):
self.assertEqual(access_token.scope, "")
self.assertEqual(access_token.expires_in, 28800)
self.assertEqual(
access_token.expires_at, datetime.datetime(2023, 6, 7, 20, 0, 0, 123)
access_token.expires_at,
datetime.datetime(2023, 6, 7, 20, 0, 0, 123, tzinfo=datetime.timezone.utc),
)
self.assertEqual(access_token.refresh_token, "refresh_token_removed")
self.assertEqual(access_token.refresh_expires_in, 15811200)
self.assertEqual(
access_token.refresh_expires_at,
datetime.datetime(2023, 12, 7, 12, 0, 0, 123),
datetime.datetime(2023, 12, 7, 12, 0, 0, 123, tzinfo=datetime.timezone.utc),
)

def testRefreshAccessToken(self):
Expand All @@ -115,8 +118,10 @@ def testRefreshAccessToken(self):
)

with mock.patch("github.AccessToken.datetime") as dt:
dt.utcnow = mock.Mock(
return_value=datetime.datetime(2023, 6, 7, 12, 0, 0, 123)
dt.now = mock.Mock(
return_value=datetime.datetime(
2023, 6, 7, 12, 0, 0, 123, tzinfo=datetime.timezone.utc
)
)
refreshed = self.app.refresh_access_token(access_token.refresh_token)

Expand All @@ -133,17 +138,19 @@ def testRefreshAccessToken(self):
self.assertEqual(refreshed.type, "bearer")
self.assertEqual(refreshed.scope, "")
self.assertEqual(
refreshed.created, datetime.datetime(2023, 6, 7, 12, 0, 0, 123)
refreshed.created,
datetime.datetime(2023, 6, 7, 12, 0, 0, 123, tzinfo=datetime.timezone.utc),
)
self.assertEqual(refreshed.expires_in, 28800)
self.assertEqual(
refreshed.expires_at, datetime.datetime(2023, 6, 7, 20, 0, 0, 123)
refreshed.expires_at,
datetime.datetime(2023, 6, 7, 20, 0, 0, 123, tzinfo=datetime.timezone.utc),
)
self.assertEqual(refreshed.refresh_token, "another_refresh_token_removed")
self.assertEqual(refreshed.refresh_expires_in, 15811200)
self.assertEqual(
refreshed.refresh_expires_at,
datetime.datetime(2023, 12, 7, 12, 0, 0, 123),
datetime.datetime(2023, 12, 7, 12, 0, 0, 123, tzinfo=datetime.timezone.utc),
)

def testGetAccessTokenBadCode(self):
Expand Down
37 changes: 18 additions & 19 deletions tests/Authentication.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@
# #
################################################################################
import datetime
import warnings
from unittest import mock

import jwt
Expand All @@ -42,16 +41,6 @@ def testNoAuthentication(self):
g = github.Github()
self.assertEqual(g.get_user("jacquev6").name, "Vincent Jacques")

def assertWarning(self, warning, expected):
self.assertWarnings(warning, expected)

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 testBasicAuthentication(self):
with self.assertWarns(DeprecationWarning) as warning:
g = github.Github(self.login.login, self.login.password)
Expand Down Expand Up @@ -124,25 +113,33 @@ def testAppUserAuthentication(self):
g = github.Github()
app = g.get_oauth_application(client_id, client_secret)
with mock.patch("github.AccessToken.datetime") as dt:
dt.utcnow = mock.Mock(
return_value=datetime.datetime(2023, 6, 7, 12, 0, 0, 123)
dt.now = mock.Mock(
return_value=datetime.datetime(
2023, 6, 7, 12, 0, 0, 123, tzinfo=datetime.timezone.utc
)
)
token = app.refresh_access_token(refresh_token)
self.assertEqual(token.token, "fresh access token")
self.assertEqual(token.type, "bearer")
self.assertEqual(token.scope, "")
self.assertEqual(token.expires_in, 28800)
self.assertEqual(token.expires_at, datetime.datetime(2023, 6, 7, 20, 0, 0, 123))
self.assertEqual(
token.expires_at,
datetime.datetime(2023, 6, 7, 20, 0, 0, 123, tzinfo=datetime.timezone.utc),
)
self.assertEqual(token.refresh_token, "fresh refresh token")
self.assertEqual(token.refresh_expires_in, 15811200)
self.assertEqual(
token.refresh_expires_at, datetime.datetime(2023, 12, 7, 12, 0, 0, 123)
token.refresh_expires_at,
datetime.datetime(2023, 12, 7, 12, 0, 0, 123, tzinfo=datetime.timezone.utc),
)

auth = app.get_app_user_auth(token)
with mock.patch("github.Auth.datetime") as dt:
dt.utcnow = mock.Mock(
return_value=datetime.datetime(2023, 6, 7, 20, 0, 0, 123)
dt.now = mock.Mock(
return_value=datetime.datetime(
2023, 6, 7, 20, 0, 0, 123, tzinfo=datetime.timezone.utc
)
)
self.assertEqual(auth._is_expired, False)
self.assertEqual(auth.token, "fresh access token")
Expand All @@ -151,8 +148,10 @@ def testAppUserAuthentication(self):

# expire auth token
with mock.patch("github.Auth.datetime") as dt:
dt.utcnow = mock.Mock(
return_value=datetime.datetime(2023, 6, 7, 20, 0, 1, 123)
dt.now = mock.Mock(
return_value=datetime.datetime(
2023, 6, 7, 20, 0, 1, 123, tzinfo=datetime.timezone.utc
)
)
self.assertEqual(auth._is_expired, True)
self.assertEqual(auth.token, "another access token")
Expand Down
16 changes: 16 additions & 0 deletions tests/Framework.py
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
import os
import traceback
import unittest
import warnings

import httpretty # type: ignore
from requests.structures import CaseInsensitiveDict
Expand Down Expand Up @@ -328,6 +329,21 @@ def tearDown(self):
self.__closeReplayFileIfNeeded()
github.Requester.Requester.resetConnectionClasses()

def assertWarning(self, warning, expected):
self.assertWarnings(warning, expected)

def assertWarnings(self, warning, *expecteds):
self.assertEqual(len(warning.warnings), len(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)

def __openFile(self, mode):
for (_, _, functionName, _) in traceback.extract_stack():
if (
Expand Down
22 changes: 20 additions & 2 deletions tests/GithubApp.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,10 @@

from datetime import datetime

import github

from . import Framework
from .GithubIntegration import APP_ID, PRIVATE_KEY


class GithubApp(Framework.TestCase):
Expand Down Expand Up @@ -99,8 +102,23 @@ def testGetPublicApp(self):
self.assertEqual(app.url, "/apps/github-actions")

def testGetAuthenticatedApp(self):
# For this to work correctly in record mode, this test must be run with --auth_with_jwt
app = self.g.get_app()
auth = github.Auth.AppAuth(APP_ID, PRIVATE_KEY)
g = github.Github(auth=auth)

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(
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.assertEqual(app.created_at, datetime(2020, 8, 1, 17, 23, 46))
self.assertEqual(app.description, "Sample App to test PyGithub")
self.assertListEqual(
Expand Down
29 changes: 18 additions & 11 deletions tests/GithubIntegration.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import time # NOQA
import warnings

import requests # NOQA

Expand Down Expand Up @@ -42,16 +41,6 @@ def setUp(self):
self.repo_installation_id = 30614431
self.user_installation_id = 30614431

def assertWarning(self, warning, expected):
self.assertWarnings(warning, expected)

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 testDeprecatedAppAuth(self):
# Replay data copied from testGetInstallations to test authentication only
with self.assertWarns(DeprecationWarning) as warning:
Expand All @@ -67,6 +56,16 @@ def testDeprecatedAppAuth(self):
"instead",
)

def testRequiredAppAuth(self):
# GithubIntegration requires AppAuth authentication.
for auth in [self.oauth_token, self.jwt, self.login]:
with self.assertRaises(AssertionError) as r:
github.GithubIntegration(auth=auth)
self.assertEqual(
str(r.exception),
f"GithubIntegration requires github.Auth.AppAuth authentication, not {type(auth)}",
)

def testAppAuth(self):
# Replay data copied from testDeprecatedAppAuth to test parity
auth = github.Auth.AppAuth(APP_ID, PRIVATE_KEY)
Expand Down Expand Up @@ -227,3 +226,11 @@ def testGetAccessTokenWithInvalidData(self):
)

self.assertEqual(raisedexp.exception.status, 400)

def testGetApp(self):
auth = github.Auth.AppAuth(APP_ID, PRIVATE_KEY)
github_integration = github.GithubIntegration(auth=auth)
app = github_integration.get_app()

self.assertEqual(app.name, "PyGithubTest")
self.assertEqual(app.url, "/apps/pygithubtest")

0 comments on commit 6d4b6d1

Please sign in to comment.