Add List fine-grained personal access tokens with access to organization resources API #3215

arymoraes · 2024-07-20T21:55:15Z

Fixes: #3213.

codecov · 2024-07-20T23:10:01Z

Codecov Report

Attention: Patch coverage is 92.59259% with 2 lines in your changes missing coverage. Please review.

Project coverage is 92.92%. Comparing base (2b8c7fa) to head (af4b0ab).
Report is 85 commits behind head on master.

Files	Patch %	Lines
github/orgs_personal_access_tokens.go	92.59%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3215      +/-   ##
==========================================
- Coverage   97.72%   92.92%   -4.80%     
==========================================
  Files         153      171      +18     
  Lines       13390    11609    -1781     
==========================================
- Hits        13085    10788    -2297     
- Misses        215      727     +512     
- Partials       90       94       +4

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

gmlewis

Thank you, @arymoraes!

gmlewis · 2024-07-20T23:27:54Z

github/orgs_personal_access_tokens.go

+// GitHub API docs: https://docs.github.com/rest/orgs/personal-access-tokens#list-fine-grained-personal-access-tokens-with-access-to-organization-resources
+//
+//meta:operation GET /orgs/{org}/personal-access-tokens
+func (s *OrganizationsService) ListFineGrainedPersonalAccessTokens(ctx context.Context, org string, opts *ListOptions) ([]*PersonalAccessToken, *Response, error) {


This endpoint can take a whole bunch more options:

sort stringThe property by which to sort the results.Default: created_atValue: created_at -- direction stringThe direction to sort the results by.Default: descCan be one of: asc, desc owner arrayA list of owner usernames to use to filter the results. repository stringThe name of the repository to use to filter the results. permission stringThe permission to use to filter the results. last_used_before stringOnly show fine-grained personal access tokens used before the given time. This is a timestamp in ISO 8601 format: YYYY-MM-DDTHH:MM:SSZ. last_used_after stringOnly show fine-grained personal access tokens used after the given time. This is a timestamp in ISO 8601 format: YYYY-MM-DDTHH:MM:SSZ. sort string The property by which to sort the results. Default: created_at Value: created_at direction string The direction to sort the results by. Default: desc Can be one of: asc, desc owner array A list of owner usernames to use to filter the results. repository string The name of the repository to use to filter the results. permission string The permission to use to filter the results. last_used_before string Only show fine-grained personal access tokens used before the given time. This is a timestamp in [ISO 8601](https://en.wikipedia.org/wiki/ISO_8601) format: YYYY-MM-DDTHH:MM:SSZ. last_used_after string Only show fine-grained personal access tokens used after the given time. This is a timestamp in [ISO 8601](https://en.wikipedia.org/wiki/ISO_8601) format: YYYY-MM-DDTHH:MM:SSZ.

So let's please create a new type ListFineGrainedPATOptions struct { ... ListOptions } that can take all these parameter, and make sure to add omitempty to each one of them since they are optional.

Good point and sorry for missing that. I added that on my latest commit.

One thing though, using the owner array as

Owner []string `url:"owner,omitempty,comma"`

will cause the API to return 422 status code, as it expects the []owner=... format, so I had to create the helper function below.

I tried to find an example on the codebase and I found IssueListOptions, but this one seems to work as that endpoint expects the orgs/{ORG}/issues?labels=bug%2Cfeature format

@arymoraes - I'm looking all over the GitHub v3 API documentation and can't find []owner=... anywhere.
Where did you find this? It seems incredibly strange syntax to me for a query parameter.

I also found it very strange. I did not find it on the docs, but by trial and error (and by remembering seeing this kind of issue once).
If you run a CURL with:

curl -L \ -H "Accept: application/vnd.github+json" \ -H "Authorization: {TOKEN}" \ -H "X-GitHub-Api-Version: 2022-11-28" \ https://api.github.com/orgs/{ORG}/personal-access-tokens?owner={username}

It will give you a 422 status code error:

{ "message": "Invalid request.\n\nInvalid input: `\"{username}\"` is not of type `array`.", "documentation_url": "https://docs.github.com/rest/orgs/personal-access-tokens#list-fine-grained-personal-access-tokens-with-access-to-organization-resources", "status": "422" }

However:

curl -L \ -H "Accept: application/vnd.github+json" \ -H "Authorization: Bearer {TOKEN}" \ -H "X-GitHub-Api-Version: 2022-11-28" \ https://api.github.com/orgs/{ORG}/personal-access-tokens?owner[]={username}

Is valid and will indeed return you a list of tokens filtered by that owner

OK, wow, that is bizarre... so I was hoping that your unit test would show how multiple owners would appear in the resulting URL, but it doesn't... do they show up as comma-separated values?

Honestly, it gets even weirder if you want multiple owners.

personal-access-tokens?owner[]=owner1,owner2

doesn't work, it'll just return an empty array of tokens (maybe if thinks the owner name is owner1,owner2?

You have to do

personal-access-tokens?owner[]=owner1&owner[]=owner2

To be able to filter tokens from any of those two owners. This is the one that is generated with addListFineGrainedPATOptions.

I am finding this really weird, but this is the way it works even with curl, so I'm assuming this is indeed the way that GitHub API wants us to send the query param. I tried to search for others with the same question, but could not. I could take another look though.

I should probably improve the comment for addListFineGrainedPATOptions to add the multiple owners scenario, and maybe also test the URL explicitly on the unit test?

Wow, that's amazing... thank you for the details, @arymoraes !

Maybe could you please just add a comment explaining all this weirdness would be extremely helpful for anyone else that comes across this and tries to use it. Thank you!

Done!

I added the comments to the addListFineGrainedPATOptions helper function declaration, as well as inside addListFineGrainedPATOptions when we call addListFineGrainedPATOptions, so whoever runs into that code knows why we are doing this.

…ssTokens

gmlewis · 2024-07-21T16:51:50Z

github/orgs_personal_access_tokens_test.go

+		ListOptions: ListOptions{Page: 2, PerPage: 2},
+		Sort:        "created_at",
+		Direction:   "desc",
+		Owner:       []string{"octocat"},


Please modify your test case to list multiple owners in order to demonstrate how this code behaves for this case.

…AccessTokens test case

…edPersonalAccessTokens

gmlewis

Thank you, @arymoraes !
LGTM.

Awaiting second LGTM+Approval from any other contributor to this repo before merging.

AbhishekAg · 2024-07-24T07:01:50Z

@arymoraes @gmlewis Could you please check who will be the next reviewer?

gmlewis · 2024-07-25T18:41:17Z

@arymoraes @gmlewis Could you please check who will be the next reviewer?

In this repo, we depend upon volunteers to review. Since you are another contributor to this repo, you are welcome to review this PR and give your feedback. If you approve, please LGTM+Approve and we can move forward with this PR. Thank you.

AbhishekAg

LGTM

gmlewis · 2024-07-27T20:57:41Z

Thank you, @AbhishekAg !
Merging.

feat: Add ListFineGrainedPersonalAccessTokens API

04f9a42

gmlewis requested changes Jul 20, 2024

View reviewed changes

feat: added ListFineGrainedPATOptions for ListFineGrainedPersonalAcce…

2a70832

…ssTokens

gmlewis reviewed Jul 21, 2024

View reviewed changes

arymoraes added 2 commits July 21, 2024 11:40

feat: Added multiple owners as query params on ListFineGranedPersonal…

e4b84aa

…AccessTokens test case

feat: better comments explaining the owner parameter in ListFineGrain…

af4b0ab

…edPersonalAccessTokens

gmlewis added the NeedsReview PR is awaiting a review before merging. label Jul 22, 2024

gmlewis approved these changes Jul 22, 2024

View reviewed changes

AbhishekAg approved these changes Jul 27, 2024

View reviewed changes

gmlewis removed the NeedsReview PR is awaiting a review before merging. label Jul 27, 2024

gmlewis merged commit 7f7f194 into google:master Jul 27, 2024
5 of 7 checks passed

gmlewis mentioned this pull request Aug 21, 2024

Bump version of go-github to v64.0.0 #3240

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add List fine-grained personal access tokens with access to organization resources API #3215

Add List fine-grained personal access tokens with access to organization resources API #3215

arymoraes commented Jul 20, 2024 •

edited by gmlewis

Loading

codecov bot commented Jul 20, 2024 •

edited

Loading

gmlewis left a comment

gmlewis Jul 20, 2024

arymoraes Jul 21, 2024

gmlewis Jul 21, 2024

arymoraes Jul 22, 2024 •

edited

Loading

gmlewis Jul 22, 2024

arymoraes Jul 22, 2024

gmlewis Jul 22, 2024

arymoraes Jul 22, 2024

gmlewis Jul 21, 2024

arymoraes Jul 21, 2024

gmlewis left a comment

AbhishekAg commented Jul 24, 2024

gmlewis commented Jul 25, 2024

AbhishekAg left a comment

gmlewis commented Jul 27, 2024

Add List fine-grained personal access tokens with access to organization resources API #3215

Add List fine-grained personal access tokens with access to organization resources API #3215

Conversation

arymoraes commented Jul 20, 2024 • edited by gmlewis Loading

codecov bot commented Jul 20, 2024 • edited Loading

Codecov Report

gmlewis 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

arymoraes Jul 22, 2024 • edited Loading

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

gmlewis left a comment

Choose a reason for hiding this comment

AbhishekAg commented Jul 24, 2024

gmlewis commented Jul 25, 2024

AbhishekAg left a comment

Choose a reason for hiding this comment

gmlewis commented Jul 27, 2024

arymoraes commented Jul 20, 2024 •

edited by gmlewis

Loading

codecov bot commented Jul 20, 2024 •

edited

Loading

arymoraes Jul 22, 2024 •

edited

Loading