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

Support full GitHub app authentication #1986

Merged
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
885a522
Support full GitHub app authentication
dblanchette Jun 30, 2021
a719746
Fix main class type
dblanchette Jul 7, 2021
c39a183
Merge master and fix conflict
dblanchette Oct 15, 2021
d8b20e1
Remove unused imports
dblanchette Oct 15, 2021
30a11a0
Merge branch 'master' into feature/github-app-authentication
dblanchette Nov 3, 2021
c15de72
Code review changes
dblanchette Jan 11, 2022
f8a071b
Fix type annotation
dblanchette Jan 11, 2022
e957467
Remove default values in type hints
dblanchette Jan 14, 2022
add6683
Remove default values in .pyi (again)
dblanchette Jan 17, 2022
3ba6e76
Merge remote-tracking branch 'origin/master' into feature/github-app-…
dblanchette Jan 17, 2022
8df28e9
Merge branch 'master' into feature/github-app-authentication
dblanchette Oct 13, 2022
a461e0e
black
dblanchette Oct 13, 2022
94ca7be
Refactor GithubIntegration class and add test case for app authentica…
ammarmallik Oct 31, 2022
8e47431
Modify existing testcases for GithubIntegration as per the framework …
ammarmallik Nov 1, 2022
a9c2f81
Provide installation ID for creating the access token instead of gett…
ammarmallik Nov 2, 2022
b88f36f
Add test case for getting app installation
ammarmallik Nov 3, 2022
8534fe9
Add optional permissions support for installation access token
ammarmallik Nov 4, 2022
d502f27
Merge pull request #28 from ammarmallik/feature/github-app-authentica…
dblanchette Nov 29, 2022
350911e
Merge branch 'master' into feature/github-app-authentication
dblanchette Nov 29, 2022
15f1c0c
flake8 not on gitlab anymore?
dblanchette Nov 29, 2022
99e6b3a
Merge branch 'master' into feature/github-app-authentication
dblanchette Dec 19, 2022
ceb8d7d
Add lock around app authentication
dblanchette Jan 16, 2023
df38766
Remove unrelated fixes
dblanchette Jan 23, 2023
8899a9e
A tab
dblanchette Jan 23, 2023
38fb3cd
Keep compatibility for importing GithubIntegration from MainClass
dblanchette Jan 30, 2023
2c106c6
Group app authentication parameters in a class
dblanchette Jan 31, 2023
ce199bf
Merge branch 'master' into feature/github-app-authentication
dblanchette Jan 31, 2023
a50aecd
Remove useless exception handling
dblanchette Jan 31, 2023
0e245d0
Add isinstance checks in github/AppAuthentication.py
dblanchette Feb 1, 2023
7d61945
Code review changes
dblanchette Feb 1, 2023
a7ca0cb
Merge remote-tracking branch 'coveord/feature/github-app-authenticati…
dblanchette Feb 1, 2023
7dd9fa9
Fix an assert
dblanchette Feb 1, 2023
7e6d5b7
black
dblanchette Feb 1, 2023
148c14f
Remove useless check
dblanchette Feb 1, 2023
3e16166
Fix an assert and a docstring
dblanchette Feb 2, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 4 additions & 4 deletions github/ApplicationOAuth.pyi
Expand Up @@ -13,12 +13,12 @@ class ApplicationOAuth(NonCompletableGithubObject):
def client_secret(self) -> str: ...
def get_login_url(
self,
redirect_uri: Optional[str],
state: Optional[str],
login: Optional[str]
redirect_uri: Optional[str]=None,
dblanchette marked this conversation as resolved.
Show resolved Hide resolved
state: Optional[str]=None,
login: Optional[str]=None
) -> str: ...
def get_access_token(
self,
code: str,
state: Optional[str]
state: Optional[str]=None
) -> AccessToken: ...
4 changes: 3 additions & 1 deletion github/AuthenticatedUser.pyi
@@ -1,5 +1,5 @@
from datetime import datetime
from typing import Any, Dict, List, Optional, Union, NamedTuple
from typing import Any, Dict, List, NamedTuple, Union

from github.Authorization import Authorization
from github.Event import Event
Expand All @@ -20,6 +20,7 @@ from github.Repository import Repository
from github.Team import Team
from github.UserKey import UserKey


class EmailData(NamedTuple):
email: str
primary: bool
Expand Down Expand Up @@ -85,6 +86,7 @@ class AuthenticatedUser(CompletableGithubObject):
allow_squash_merge: Union[bool, _NotSetType] = ...,
allow_merge_commit: Union[bool, _NotSetType] = ...,
allow_rebase_merge: Union[bool, _NotSetType] = ...,
delete_branch_on_merge: Union[bool, _NotSetType] = ...,
) -> Repository: ...
@property
def created_at(self) -> datetime: ...
Expand Down
2 changes: 0 additions & 2 deletions github/Branch.pyi
Expand Up @@ -42,8 +42,6 @@ class Branch(NonCompletableGithubObject):
strict: Union[_NotSetType, bool] = ...,
contexts: Union[List[str], _NotSetType] = ...,
) -> None: ...
def edit_team_push_restrictions(self, *teams: str) -> None: ...
def edit_user_push_restrictions(self, *users: str) -> None: ...
def get_admin_enforcement(self) -> bool: ...
def get_protection(self) -> BranchProtection: ...
def get_required_pull_request_reviews(self) -> RequiredPullRequestReviews: ...
Expand Down
8 changes: 6 additions & 2 deletions github/CheckSuite.pyi
@@ -1,5 +1,5 @@
from datetime import datetime
from typing import Any, Dict, List
from typing import Any, Dict, List, Union

from github.CheckRun import CheckRun
from github.GitCommit import GitCommit
Expand All @@ -8,6 +8,7 @@ from github.GithubObject import CompletableGithubObject
from github.PaginatedList import PaginatedList
from github.PullRequest import PullRequest
from github.Repository import Repository
from github.GithubObject import _NotSetType, NotSet

class CheckSuite(CompletableGithubObject):
def __repr__(self) -> str: ...
Expand Down Expand Up @@ -47,5 +48,8 @@ class CheckSuite(CompletableGithubObject):
def url(self) -> str: ...
def rerequest(self) -> bool: ...
def get_check_runs(
self, check_name: str, status: str, filter: str
self,
check_name: Union[str, _NotSetType]=NotSet,
dblanchette marked this conversation as resolved.
Show resolved Hide resolved
status: Union[str, _NotSetType]=NotSet,
filter: Union[str, _NotSetType]=NotSet,
) -> PaginatedList[CheckRun]: ...
6 changes: 3 additions & 3 deletions github/Commit.pyi
Expand Up @@ -8,7 +8,7 @@ from github.CommitStats import CommitStats
from github.CommitStatus import CommitStatus
from github.File import File
from github.GitCommit import GitCommit
from github.GithubObject import CompletableGithubObject, _NotSetType
from github.GithubObject import CompletableGithubObject, _NotSetType, NotSet
from github.NamedUser import NamedUser
from github.PaginatedList import PaginatedList
from github.PullRequest import PullRequest
Expand Down Expand Up @@ -45,8 +45,8 @@ class Commit(CompletableGithubObject):
def files(self) -> List[File]: ...
def get_check_suites(
self,
app_id: Union[_NotSetType, int],
check_name: Union[_NotSetType, str],
app_id: Union[_NotSetType, int]=NotSet,
check_name: Union[_NotSetType, str]=NotSet,
dblanchette marked this conversation as resolved.
Show resolved Hide resolved
) -> PaginatedList[CheckSuite]: ...
def get_combined_status(self) -> CommitCombinedStatus: ...
def get_comments(self) -> PaginatedList[CommitComment]: ...
Expand Down
8 changes: 8 additions & 0 deletions github/Consts.py
Expand Up @@ -128,3 +128,11 @@

# https://developer.github.com/changes/2019-12-03-internal-visibility-changes/
repoVisibilityPreview = "application/vnd.github.nebula-preview+json"

DEFAULT_BASE_URL = "https://api.github.com"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

DEFAULT_STATUS_URL = "https://status.github.com"
# As of 2018-05-17, Github imposes a 10s limit for completion of API requests.
# Thus, the timeout should be slightly > 10s to account for network/front-end
# latency.
DEFAULT_TIMEOUT = 15
DEFAULT_PER_PAGE = 30
2 changes: 1 addition & 1 deletion github/GithubException.py
Expand Up @@ -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):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

super().__init__()
self.__status = status
self.__data = data
Expand Down
140 changes: 140 additions & 0 deletions github/GithubIntegration.py
@@ -0,0 +1,140 @@
import time
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


import deprecated
import jwt
import requests

import github.Installation
import github.InstallationAuthorization
import github.PaginatedList
import github.Requester

from . import Consts, GithubException


class GithubIntegration:
"""
Main class to obtain tokens for a GitHub integration.
"""

def __init__(self, integration_id, private_key, base_url=Consts.DEFAULT_BASE_URL):
"""
:param base_url: string
:param integration_id: int
:param private_key: string
"""
self.base_url = base_url
self.integration_id = integration_id
self.private_key = private_key
assert isinstance(base_url, str), base_url

def create_jwt(self, expiration=60):
"""
Creates a signed JWT, valid for 60 seconds by default.
The expiration can be extended beyond this, to a maximum of 600 seconds.

:param expiration: int
:return string:
"""
now = int(time.time())
dblanchette marked this conversation as resolved.
Show resolved Hide resolved
payload = {"iat": now, "exp": now + expiration, "iss": self.integration_id}
encrypted = jwt.encode(payload, key=self.private_key, algorithm="RS256")

if isinstance(encrypted, bytes):
encrypted = encrypted.decode("utf-8")

return encrypted

def get_access_token(self, installation_id, user_id=None):
"""
Get an access token for the given installation id.
POSTs https://api.github.com/app/installations/<installation_id>/access_tokens
:param user_id: int
:param installation_id: int
:return: :class:`github.InstallationAuthorization.InstallationAuthorization`
"""
body = {}
if user_id:
body = {"user_id": user_id}
response = requests.post(
f"{self.base_url}/app/installations/{installation_id}/access_tokens",
headers={
"Authorization": f"Bearer {self.create_jwt()}",
"Accept": Consts.mediaTypeIntegrationPreview,
"User-Agent": "PyGithub/Python",
},
json=body,
)

if response.status_code == 201:
return github.InstallationAuthorization.InstallationAuthorization(
requester=None, # not required, this is a NonCompletableGithubObject
headers={}, # not required, this is a NonCompletableGithubObject
attributes=response.json(),
completed=True,
)
elif response.status_code == 403:
raise GithubException.BadCredentialsException(
status=response.status_code, data=response.text
)
elif response.status_code == 404:
raise GithubException.UnknownObjectException(
status=response.status_code, data=response.text
)
raise GithubException.GithubException(
status=response.status_code, data=response.text
)

@deprecated.deprecated("Use get_repo_installation")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

def get_installation(self, owner, repo):
"""
:calls: `GET /repos/{owner}/{repo}/installation <https://docs.github.com/en/rest/reference/apps#get-a-repository-installation-for-the-authenticated-app>`_
:param owner: str
:param repo: str
:rtype: :class:`github.Installation.Installation`
"""
headers = {
"Authorization": f"Bearer {self.create_jwt()}",
"Accept": Consts.mediaTypeIntegrationPreview,
"User-Agent": "PyGithub/Python",
}

response = requests.get(
f"{self.base_url}/repos/{owner}/{repo}/installation",
headers=headers,
)
response_dict = response.json()
return github.Installation.Installation(None, headers, response_dict, True)

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

def get_installations(self):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

"""
:calls: GET /app/installations <https://docs.github.com/en/rest/reference/apps#list-installations-for-the-authenticated-app>
:rtype: :class:`github.PaginatedList.PaginatedList[github.Installation.Installation]`
"""
return github.PaginatedList.PaginatedList(
contentClass=github.Installation.Installation,
requester=github.Requester.Requester(
login_or_token=None,
password=None,
jwt=self.create_jwt(),
app_id=None,
app_private_key=None,
base_url=Consts.DEFAULT_BASE_URL,
dblanchette marked this conversation as resolved.
Show resolved Hide resolved
timeout=Consts.DEFAULT_TIMEOUT,
user_agent="PyGithub/Python",
per_page=Consts.DEFAULT_PER_PAGE,
verify=True,
retry=None,
pool_size=None,
),
firstUrl="/app/installations",
firstParams=None,
headers={
"Authorization": f"Bearer {self.create_jwt()}",
"User-Agent": "PyGithub/Python",
},
list_item="installations",
)
17 changes: 17 additions & 0 deletions github/GithubIntegration.pyi
@@ -0,0 +1,17 @@
from typing import Union, Optional

from github.Installation import Installation
from github.InstallationAuthorization import InstallationAuthorization
from github.PaginatedList import PaginatedList


class GithubIntegration:
def __init__(
self, integration_id: Union[int, str], private_key: str, base_url: str = ...
) -> None: ...
def create_jwt(self, expiration: int = ...) -> str: ...
def get_access_token(
self, installation_id: int, user_id: Optional[int] = ...
) -> InstallationAuthorization: ...
def get_installation(self, owner: str, repo: str) -> Installation: ...
def get_installations(self) -> PaginatedList[Installation]: ...
9 changes: 3 additions & 6 deletions github/GithubObject.pyi
@@ -1,11 +1,6 @@
from typing import Any, Callable, Dict, List, Optional, Type, Union

from github.Commit import Commit
from github.GistFile import GistFile
from github.GitRelease import GitRelease
from github.NamedUser import NamedUser
from github.Organization import Organization
from github.PullRequestReview import PullRequestReview
from github.Requester import Requester

class GithubObject:
Expand Down Expand Up @@ -108,7 +103,7 @@ class CompletableGithubObject(GithubObject):
def _completeIfNotSet(
self, value: Union[_ValuedAttribute, _BadAttribute, _NotSetType]
) -> None: ...
def update(self) -> bool: ...
def update(self, additional_headers: Optional[Dict[str, str]]=None) -> bool: ...

class _BadAttribute:
def __init__(
Expand All @@ -120,5 +115,7 @@ class _BadAttribute:
class _NotSetType:
def __repr__(self) -> str: ...

NotSet: _NotSetType

class _ValuedAttribute:
def __init__(self, value: Any) -> None: ...