-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Add Ability to Scope Install Access Tokens to a List of Repositories #2892
base: main
Are you sure you want to change the base?
Changes from 7 commits
1353fd9
7c0ec62
7126212
bc82108
54f181e
3db8d37
5930adc
cda2795
2144866
e94c09b
9b6a640
80407cc
29679aa
c81293c
f8ed209
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -174,10 +174,13 @@ def __exit__(self, exc_type: Any, exc_val: Any, exc_tb: Any) -> None: | |||||||||||||||||||||||||||||||||||
self.close() | ||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||
def get_github_for_installation( | ||||||||||||||||||||||||||||||||||||
self, installation_id: int, token_permissions: dict[str, str] | None = None | ||||||||||||||||||||||||||||||||||||
self, | ||||||||||||||||||||||||||||||||||||
installation_id: int, | ||||||||||||||||||||||||||||||||||||
token_permissions: dict[str, str] | None = None, | ||||||||||||||||||||||||||||||||||||
token_repositories: list[str | int] | None = None, | ||||||||||||||||||||||||||||||||||||
) -> github.Github: | ||||||||||||||||||||||||||||||||||||
# The installation has to authenticate as an installation, not an app | ||||||||||||||||||||||||||||||||||||
auth = self.auth.get_installation_auth(installation_id, token_permissions, self.__requester) | ||||||||||||||||||||||||||||||||||||
auth = self.auth.get_installation_auth(installation_id, token_permissions, self.__requester, token_repositories) | ||||||||||||||||||||||||||||||||||||
bakosa marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||||
return github.Github(**self.__requester.withAuth(auth).kwargs) | ||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||
def _get_headers(self) -> dict[str, str]: | ||||||||||||||||||||||||||||||||||||
|
@@ -212,18 +215,41 @@ def create_jwt(self, expiration: int | None = None) -> str: | |||||||||||||||||||||||||||||||||||
return self.auth.create_jwt(expiration) | ||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||
def get_access_token( | ||||||||||||||||||||||||||||||||||||
self, installation_id: int, permissions: dict[str, str] | None = None | ||||||||||||||||||||||||||||||||||||
self, | ||||||||||||||||||||||||||||||||||||
installation_id: int, | ||||||||||||||||||||||||||||||||||||
permissions: dict[str, str] | None = None, | ||||||||||||||||||||||||||||||||||||
repositories: list[str | int] | None = None, | ||||||||||||||||||||||||||||||||||||
) -> InstallationAuthorization: | ||||||||||||||||||||||||||||||||||||
""" | ||||||||||||||||||||||||||||||||||||
:calls: `POST /app/installations/{installation_id}/access_tokens <https://docs.github.com/en/rest/apps/apps#create-an-installation-access-token-for-an-app>` | ||||||||||||||||||||||||||||||||||||
""" | ||||||||||||||||||||||||||||||||||||
body: dict[str, Any] = {} | ||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||
EnricoMi marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||||
if permissions is None: | ||||||||||||||||||||||||||||||||||||
permissions = {} | ||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||
if repositories is not None and not isinstance(repositories, list): | ||||||||||||||||||||||||||||||||||||
EnricoMi marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||||
raise GithubException(status=400, data={"message": "Invalid repositories"}, headers=None) | ||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||
if not isinstance(permissions, dict): | ||||||||||||||||||||||||||||||||||||
raise GithubException(status=400, data={"message": "Invalid permissions"}, headers=None) | ||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||
body = {"permissions": permissions} | ||||||||||||||||||||||||||||||||||||
body["permissions"] = permissions | ||||||||||||||||||||||||||||||||||||
EnricoMi marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||||
if repositories: | ||||||||||||||||||||||||||||||||||||
repository_names = [] | ||||||||||||||||||||||||||||||||||||
repository_ids = [] | ||||||||||||||||||||||||||||||||||||
for r in repositories: | ||||||||||||||||||||||||||||||||||||
print(r, type(r)) | ||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this debug line should be removed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wow, that is just embarrassing 🤦♂️ |
||||||||||||||||||||||||||||||||||||
if isinstance(r, str): | ||||||||||||||||||||||||||||||||||||
repository_names.append(r) | ||||||||||||||||||||||||||||||||||||
elif isinstance(r, int): | ||||||||||||||||||||||||||||||||||||
repository_ids.append(r) | ||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||
if repository_names: | ||||||||||||||||||||||||||||||||||||
body["repositories"] = repository_names | ||||||||||||||||||||||||||||||||||||
if repository_ids: | ||||||||||||||||||||||||||||||||||||
body["repository_ids"] = repository_ids | ||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What about list comprehension?
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I did not originally use list comprehension in favor of only iterating over However, my latest commit swaps it over to using list comprehension, I suppose it is still more readable! |
||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||
headers, response = self.__requester.requestJsonAndCheck( | ||||||||||||||||||||||||||||||||||||
"POST", | ||||||||||||||||||||||||||||||||||||
f"/app/installations/{installation_id}/access_tokens", | ||||||||||||||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
https | ||
POST | ||
api.github.com | ||
None | ||
/app/installations/30614487/access_tokens | ||
{'Authorization': 'Bearer jwt_removed', 'Accept': 'application/vnd.github.machine-man-preview+json', 'User-Agent': 'PyGithub/Python', 'Content-Type': 'application/json'} | ||
{"permissions": {}, "repositories": ["invalid-repo"]} | ||
422 | ||
[('status', '422 UNPROCESSABLE ENTITY'), ('server', 'Github.com'), ('date', 'Mon, 24 Oct 2022 23:11:45 GMT'), ('content-type', 'application/json; charset=utf-8'), ('connection', 'keep-alive'), ('content-length', '1962'), ('etag', 'W/"b11a1c9caabe35f1de0a13e597a3022d27d2bff0694c2ccb5a65edc3b4d18837"'), ('cache-control', 'public, max-age=60, s-maxage=60'), ('vary', 'Accept'), ('x-github-media-type', 'github.v3; format=json'), ('access-control-expose-headers', 'ETag, Link, Location, Retry-After, X-GitHub-OTP, X-RateLimit-Limit, X-RateLimit-Remaining, X-RateLimit-Used, X-RateLimit-Resource, X-RateLimit-Reset, X-OAuth-Scopes, X-Accepted-OAuth-Scopes, X-Poll-Interval, X-GitHub-Media-Type, X-GitHub-SSO, X-GitHub-Request-Id, Deprecation, Sunset'), ('access-control-allow-origin', '*'), ('strict-transport-security', 'max-age=31536000; includeSubdomains; preload'), ('x-frame-options', 'deny'), ('x-content-type-options', 'nosniff'), ('x-xss-protection', '0'), ('referrer-policy', 'origin-when-cross-origin, strict-origin-when-cross-origin'), ('x-github-request-id', "E475:53DD:8B7A89E:11E38A79:63571BB0"), ('vary', 'Accept-Encoding, Accept, X-Requested-With'), ('content-security-policy', "default-src 'none'")] | ||
{"message": "There is at least one repository that does not exist or is not accessible to the parent installation.", "documentation_url": "https://docs.github.com/rest/reference/apps#create-an-installation-access-token-for-an-app"} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
https | ||
POST | ||
api.github.com | ||
None | ||
/app/installations/30614487/access_tokens | ||
{'Authorization': 'Bearer jwt_removed', 'Accept': 'application/vnd.github.machine-man-preview+json', 'User-Agent': 'PyGithub/Python', 'Content-Type': 'application/json'} | ||
{"permissions": {}, "repository_ids": [123456, 7891011]} | ||
422 | ||
[('status', '422 UNPROCESSABLE ENTITY'), ('server', 'Github.com'), ('date', 'Mon, 24 Oct 2022 23:11:45 GMT'), ('content-type', 'application/json; charset=utf-8'), ('connection', 'keep-alive'), ('content-length', '1962'), ('etag', 'W/"b11a1c9caabe35f1de0a13e597a3022d27d2bff0694c2ccb5a65edc3b4d18837"'), ('cache-control', 'public, max-age=60, s-maxage=60'), ('vary', 'Accept'), ('x-github-media-type', 'github.v3; format=json'), ('access-control-expose-headers', 'ETag, Link, Location, Retry-After, X-GitHub-OTP, X-RateLimit-Limit, X-RateLimit-Remaining, X-RateLimit-Used, X-RateLimit-Resource, X-RateLimit-Reset, X-OAuth-Scopes, X-Accepted-OAuth-Scopes, X-Poll-Interval, X-GitHub-Media-Type, X-GitHub-SSO, X-GitHub-Request-Id, Deprecation, Sunset'), ('access-control-allow-origin', '*'), ('strict-transport-security', 'max-age=31536000; includeSubdomains; preload'), ('x-frame-options', 'deny'), ('x-content-type-options', 'nosniff'), ('x-xss-protection', '0'), ('referrer-policy', 'origin-when-cross-origin, strict-origin-when-cross-origin'), ('x-github-request-id', "E475:53DD:8B7A89E:11E38A79:63571BB0"), ('vary', 'Accept-Encoding, Accept, X-Requested-With'), ('content-security-policy', "default-src 'none'")] | ||
{"message": "There is at least one repository that does not exist or is not accessible to the parent installation.", "documentation_url": "https://docs.github.com/rest/reference/apps#create-an-installation-access-token-for-an-app"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Though this method is public, the
requester
argument is internal in a sense that there should be no user code calling this method withrequester
given. Hence, it is safe (and preferable) to addtoken_repositories
beforerequester
:Adding the
*
makes therequester
argument mandatory named, which avoids breaking user code in the future when adding an argument before it.This requires code that provides
requester
to addrequester=...
throughout the project.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was wondering the same thing when I went to add the new argument. This makes sense! thanks for the deeper explanation.