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

Feature request: precheck more permissions needed by other steps in verify step #895

Open
jedwards1211 opened this issue Aug 6, 2024 · 14 comments · Fixed by #897
Open

Comments

@jedwards1211
Copy link
Contributor

jedwards1211 commented Aug 6, 2024

Related to #738, it would be good to try to verify that we have as many of the necessary GitHub permissions as possible before running other steps. For example, publish can succeed but then adding issue comments can fail during the success step; then adding those comments manually is the only option. But if we could detect the lack of permissions and error out on the verify step, then the user can fix their token and rerun, and the issue comments will be created successfully after publish.

As I mentioned in #738:

What I've found out so far is:

Maybe we can check permission to update issues and releases by doing a no-op update on one (sending the title it already has in an update, etc), I will have to experiment. But we'd be able to avoid hacky workarounds if the GitHub API provided an explicit way to check if we have permissions to do a certain operation.

@jedwards1211
Copy link
Contributor Author

@gr2m

The github plugin does not use the git app, only semantic-release core does that. The github plugin exclusively interacts with APIs.

Can you help me understand the rationale for this? I understand it would be bad for @semantic-release/npm to do git operations, but in the case of @semantic-release/github, I'd think there's no harm if we can perform a guaranteed no-op with git, since they would have to be using git anyway on a GitHub project, right? Or are we trying to be cautious and avoid triggering prepush or other git hooks except when absolutely necessary?

@gr2m
Copy link
Member

gr2m commented Aug 9, 2024

Can you help me understand the rationale for this? I understand it would be bad for @semantic-release/npm to do git operations, but in the case of @semantic-release/github, I'd think there's no harm if we can perform a guaranteed no-op with git, since they would have to be using git anyway on a GitHub project, right?

The rationale is that it wasn't necessary so far. We can use git if we deem it necessary.

But I think we might be able to use GraphQL to check the necessary permissions, with a query like this

query ($owner: String!,$repo:String!) {
  repository(owner:$owner,name:$repo) {
    # Any of these should suffice: ADMIN, MAINTAIN, WRITE. Maybe TRIAGE, too, but need to verify if TRIAGE is sufficient to create a release
    # https://docs.github.com/en/graphql/reference/enums#repositorypermission
    viewerPermission
  }
}

We'd need to experiment if these match the permissions we actually need

@jedwards1211
Copy link
Contributor Author

Oh nice. Yeah definitely better to use that if we can

@gr2m
Copy link
Member

gr2m commented Aug 9, 2024

Just jotting down what the next steps would be as I don't know when I'll have the time. If anyone else would like to go through these tests that'd be great!

We need to verify the above query with both classic tokens and the new fine-grained tokens. And we need to test it in both a private and a public repository. We need to test if we can do the following

  1. Comment on issue or pull request
  2. Add label to issue or pull request
  3. Create a release

Classic tokens

  1. No scopes
  2. repo:public scope
  3. repo scope

Fine grained

  1. Permissions: content:write, issues:write, pull_requests:write

@jedwards1211
Copy link
Contributor Author

Thanks! I'll look into that next week

@jedwards1211
Copy link
Contributor Author

jedwards1211 commented Aug 12, 2024

@gr2m you mean public_repo, not repo:public, right? (Naming scopes like repo:public would have made a lot more sense though lol, the classic token scope name organization makes no sense)

@jedwards1211
Copy link
Contributor Author

jedwards1211 commented Aug 12, 2024

@gr2m viewerPermission doesn't seem to return the right value. Even with a classic token with no scopes, it's ADMIN for a public repo under my username. And also for a fine-grained PAT with content:read, pull_requests:read, issues:write, it's ADMIN.

jedwards1211/semantic-release-test-public classicNoScope ADMIN
jedwards1211/semantic-release-test-public classicPublicRepoScope ADMIN
jedwards1211/semantic-release-test-public classicRepoScope ADMIN
jedwards1211/semantic-release-test-public classicRepoStatus ADMIN
jedwards1211/semantic-release-test-public fineGrainedPublic ADMIN
jedwards1211/semantic-release-test-public fineGrainedFull ADMIN
jedwards1211/semantic-release-test-private classicNoScope GraphqlResponseError: Request failed due to following response errors:
 - Could not resolve to a Repository with the name 'jedwards1211/semantic-release-test-private'.
jedwards1211/semantic-release-test-private classicPublicRepoScope GraphqlResponseError: Request failed due to following response errors:
 - Could not resolve to a Repository with the name 'jedwards1211/semantic-release-test-private'.
jedwards1211/semantic-release-test-private classicRepoScope ADMIN
jedwards1211/semantic-release-test-private classicRepoStatus ADMIN
jedwards1211/semantic-release-test-private fineGrainedPublic GraphqlResponseError: Request failed due to following response errors:
 - Could not resolve to a Repository with the name 'jedwards1211/semantic-release-test-private'.
jedwards1211/semantic-release-test-private fineGrainedFull ADMIN

I double checked and the Classic PAT docs say

(no scope) | Grants read-only access to public information (including user profile info, repository info, and gists)

I submitted an issue to GitHub Support (ticket 2935830)

@jedwards1211
Copy link
Contributor Author

jedwards1211 commented Aug 12, 2024

Oh...according to the docs viewerPermission is

The users permission level on the repository. Will return null if authenticated as an GitHub App.

So it seems to get the permission of the user associated with the PAT, rather than the permission of the PAT itself, so this won't help us. I think this is equivalent to the REST route https://api.github.com/repos/OWNER/REPO/collaborators/USERNAME/permission.

@jedwards1211
Copy link
Contributor Author

According to this there is a way to check the permissions of a classic PAT but not a fine-grained PAT.

@gr2m
Copy link
Member

gr2m commented Aug 13, 2024

@gr2m you mean public_repo, not repo:public, right? (Naming scopes like repo:public would have made a lot more sense though lol, the classic token scope name organization makes no sense)

yes 🤣

According to this there is a way to check the permissions of a classic PAT but not a fine-grained PAT.

Yes. Maybe we just start out with this, it's better than nothing, and do a follow up issue to add checks for installation access tokens / fine-grained user tokens?

@jedwards1211
Copy link
Contributor Author

jedwards1211 commented Aug 13, 2024

Has the API team scheduled a ticket for a way to query fine-grained PAT permissions for like, within a year or so? If not then I still think it would be worth implementing checks via same-value writes now, and relying on those until the day we can query the permissions.

I mean, I guess I could start with a PR for classic PAT permission checks, and then make a separate one for same-value write checks

@gr2m
Copy link
Member

gr2m commented Aug 13, 2024

either sounds good to me because either is better than what we have today :) Checking with git push would at least verify we have contents:write which is what is needed to create a release. Commenting or labeling needs other permissions, but it's less problematic than not being able to create a release

@jedwards1211
Copy link
Contributor Author

Oh my gosh, I just realized...we can check permissions.maintain in the GET /repos/{owner}/{repo} response. It seems buggy for fine-grained PATs (all the permissions are true even if I only granted read-only perms), but, at least it wouldn't give us any false negatives where we error out saying the token doesn't have enough permissions.

@jedwards1211
Copy link
Contributor Author

Oh, argh. Again, permissions are the permissions of the user associated with the token, not the token itself. They should have named the field userPermissions, it's too easy to misinterpret what it means (and the API docs say nothing about what it means)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants