Add Ability to Scope Install Access Tokens to a List of Repositories #2892

bakosa · 2024-02-03T02:32:20Z

Description

As you can see here in the GitHub Docs you can now pass in ether a list of repos via the repositories or repository_ids body parameters when you create an app installation access token.

The problem that this is trying to solve is we need to generate least privileged tokens scoped to only specific repositories and specific permissions.

This PR adds new token_repositories and token_repository_ids arguments to the AppInstallationAuth class and uses them when invoking the get_access_token method to get a scoped app install access token.

Things to Note

I tired to follow the same pattern that is being used for the permissions body parameter. Instead of defaulting to {} when nothing is passed in, None is preferred here since that will translate to null which drive the expected outcome (tested this). Also, you can pass a combination of both repositories and repository_ids in because 🦄

Related open issues

#2807

…s to get install access token

…ries

codecov-commenter · 2024-02-03T02:34:09Z

Codecov Report

Attention: Patch coverage is 95.00000% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 96.74%. Comparing base (e4106e0) to head (f8ed209).

Files	Patch %	Lines
github/Auth.py	85.71%	1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2892      +/-   ##
==========================================
- Coverage   96.74%   96.74%   -0.01%     
==========================================
  Files         147      147              
  Lines       14978    14993      +15     
==========================================
+ Hits        14491    14505      +14     
- Misses        487      488       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

bakosa · 2024-02-06T14:46:14Z

Is there anything I can do to help get this merged in 😃

mchieco · 2024-02-13T13:27:43Z

Any updates on this issue/PR? Was looking around for this functionality and if we could get this merged in that would be great so I don't have to fork for this feature

github/AppAuthentication.py

github/GithubIntegration.py

bakosa · 2024-02-23T18:47:37Z

This should be good to go now!

EnricoMi · 2024-03-02T11:50:38Z

github/Auth.py

 token_permissions: Optional[Dict[str, str]] = None,
 requester: Optional[Requester] = None,
+ token_repositories: Optional[List[Union[str, int]]] = None,


Though this method is public, the requester argument is internal in a sense that there should be no user code calling this method with requester given. Hence, it is safe (and preferable) to add token_repositories before requester:

Suggested change

token_permissions: Optional[Dict[str, str]] = None,

requester: Optional[Requester] = None,

token_repositories: Optional[List[Union[str, int]]] = None,

token_permissions: Optional[Dict[str, str]] = None,

token_repositories: Optional[List[Union[str, int]]] = None,

*,

requester: Optional[Requester] = None,

Adding the * makes the requester argument mandatory named, which avoids breaking user code in the future when adding an argument before it.

This requires code that provides requester to add requester=... throughout the project.

I was wondering the same thing when I went to add the new argument. This makes sense! thanks for the deeper explanation.

github/Auth.py

github/GithubIntegration.py

EnricoMi · 2024-03-02T11:55:51Z

github/GithubIntegration.py

+ repository_names = []
+ repository_ids = []
+ for r in repositories:
+ print(r, type(r))


this debug line should be removed

Wow, that is just embarrassing 🤦‍♂️

EnricoMi · 2024-03-02T11:58:52Z

github/GithubIntegration.py

+ repository_names = []
+ repository_ids = []
+ for r in repositories:
+ print(r, type(r))
+ 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


What about list comprehension?

Suggested change

repository_names = []

repository_ids = []

for r in repositories:

print(r, type(r))

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

if repository_names:

body["repositories"] = [r for r in repositories if isinstance(r, str)]

if repository_ids:

body["repository_ids"] = [r for r in repositories if isinstance(r, int)]

I did not originally use list comprehension in favor of only iterating over repositories once to type check. Also, need to null check before adding to the body.

However, my latest commit swaps it over to using list comprehension, I suppose it is still more readable!

EnricoMi · 2024-03-03T10:11:28Z

Note that #2912 will add some Requester-related assertions to github.Auth that might conflict with your changes.

bakosa · 2024-03-04T13:00:27Z

Note that #2912 will add some Requester-related assertions to github.Auth that might conflict with your changes.

Sounds good, hopefully this PR is now ready! Happy to handle any conflicts if #2912 gets in first. Curious the reasoning behind all the runtime type checking?

EnricoMi · 2024-03-08T19:47:07Z

Merge, happy conflict chasing!

bakosa · 2024-03-08T21:08:06Z

Resolved 🚀

EnricoMi

Minor changes, LGTM overall!

github/GithubIntegration.py

tests/GithubIntegration.py

EnricoMi · 2024-03-09T09:54:18Z

I have added the new argument to the examples docs: 80407cc

bakosa · 2024-03-09T15:59:46Z

github/GithubIntegration.py

 ) -> 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>`
 """
 if permissions is None:
 permissions = {}

+ if repositories is not None and not (isinstance(repositories, list) and all(isinstance(r, (str, int)) for r in repositories)):


The unit test fails here because in Python, booleans are subclass of integers 🙃

We could ether be explicit about the type check (exclude subclasses..)

Suggested change

if repositories is not None and not (isinstance(repositories, list) and all(isinstance(r, (str, int)) for r in repositories)):

if repositories is not None and not (isinstance(repositories, list) and all(type(r) in (str, int) for r in repositories)):

Or change the unit test. Not sure why someone would pass in a subclassed object or str or int into the list of repositories but I will leave it up to you!

Let's stick to isinstance as we use it everywhere while it is not accurate, as you pointed out. Can you fix the unittest by simply using object()?

Makes sense, updated the test.

…ting access token

bakosa added 2 commits February 2, 2024 16:38

Adding abbility to pass the repositories or repository_ids body param…

1353fd9

…s to get install access token

Updating contracts of AppAuthentication to handle passing in reposito…

7c0ec62

…ries

bakosa changed the title ~~Add Ability to Scope Innstall Access Tokens to a List of Repositories~~ Add Ability to Scope Install Access Tokens to a List of Repositories Feb 3, 2024

Don't default repos and repo ids to empty list, instead pass None

7126212

bakosa force-pushed the main branch from 5d7ac3c to 7126212 Compare February 3, 2024 02:36

bakosa added 2 commits February 13, 2024 08:39

Merge branch 'main' into main

bc82108

Merge branch 'main' into main

54f181e

EnricoMi requested changes Feb 22, 2024

View reviewed changes

github/AppAuthentication.py Outdated Show resolved Hide resolved

github/GithubIntegration.py Outdated Show resolved Hide resolved

bakosa added 2 commits February 23, 2024 07:06

Merge branch 'main' of https://github.com/PyGithub/PyGithub

3db8d37

unifying token repo names and repo IDs 🦄

5930adc

bakosa force-pushed the main branch from 7132963 to 5930adc Compare February 23, 2024 18:45

EnricoMi requested changes Mar 2, 2024

View reviewed changes

bakosa added 3 commits March 4, 2024 07:09

Merge branch 'main' of https://github.com/PyGithub/PyGithub

cda2795

putting before on internal App installation auth method

2144866

filter token repositories using list comprehension

e94c09b

pulling latest from main

9b6a640

Add token repositories to auth example docs

80407cc

EnricoMi requested changes Mar 9, 2024

View reviewed changes

github/GithubIntegration.py Outdated Show resolved Hide resolved

github/GithubIntegration.py Show resolved Hide resolved

github/GithubIntegration.py Outdated Show resolved Hide resolved

tests/GithubIntegration.py Show resolved Hide resolved

Apply suggestions from code review

29679aa

bakosa commented Mar 9, 2024

View reviewed changes

Using base object class to test invalid repositories data when reques…

c81293c

…ting access token

Merge branch 'main' into main

f8ed209

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Ability to Scope Install Access Tokens to a List of Repositories #2892

Add Ability to Scope Install Access Tokens to a List of Repositories #2892

bakosa commented Feb 3, 2024 •

edited

codecov-commenter commented Feb 3, 2024 •

edited

bakosa commented Feb 6, 2024

mchieco commented Feb 13, 2024

bakosa commented Feb 23, 2024

EnricoMi Mar 2, 2024

bakosa Mar 4, 2024

EnricoMi Mar 2, 2024

bakosa Mar 4, 2024

EnricoMi Mar 2, 2024

bakosa Mar 4, 2024 •

edited

EnricoMi commented Mar 3, 2024

bakosa commented Mar 4, 2024

EnricoMi commented Mar 8, 2024

bakosa commented Mar 8, 2024

EnricoMi left a comment

EnricoMi commented Mar 9, 2024

bakosa Mar 9, 2024 •

edited

EnricoMi Mar 9, 2024

bakosa Mar 10, 2024

	if repositories is not None and not (isinstance(repositories, list) and all(isinstance(r, (str, int)) for r in repositories)):
	if repositories is not None and not (isinstance(repositories, list) and all(type(r) in (str, int) for r in repositories)):

Add Ability to Scope Install Access Tokens to a List of Repositories #2892

Are you sure you want to change the base?

Add Ability to Scope Install Access Tokens to a List of Repositories #2892

Conversation

bakosa commented Feb 3, 2024 • edited

Description

Things to Note

Related open issues

codecov-commenter commented Feb 3, 2024 • edited

Codecov Report

bakosa commented Feb 6, 2024

mchieco commented Feb 13, 2024

bakosa commented Feb 23, 2024

EnricoMi Mar 2, 2024

Choose a reason for hiding this comment

bakosa Mar 4, 2024

Choose a reason for hiding this comment

EnricoMi Mar 2, 2024

Choose a reason for hiding this comment

bakosa Mar 4, 2024

Choose a reason for hiding this comment

EnricoMi Mar 2, 2024

Choose a reason for hiding this comment

bakosa Mar 4, 2024 • edited

Choose a reason for hiding this comment

EnricoMi commented Mar 3, 2024

bakosa commented Mar 4, 2024

EnricoMi commented Mar 8, 2024

bakosa commented Mar 8, 2024

EnricoMi left a comment

Choose a reason for hiding this comment

EnricoMi commented Mar 9, 2024

bakosa Mar 9, 2024 • edited

Choose a reason for hiding this comment

EnricoMi Mar 9, 2024

Choose a reason for hiding this comment

bakosa Mar 10, 2024

Choose a reason for hiding this comment

bakosa commented Feb 3, 2024 •

edited

codecov-commenter commented Feb 3, 2024 •

edited

bakosa Mar 4, 2024 •

edited

bakosa Mar 9, 2024 •

edited