semantic-release / github

:octocat: semantic-release plugin to publish a GitHub release and comment on released Pull Requests/Issues
https://www.npmjs.com/package/@semantic-release/github
MIT License
419 stars 128 forks source link

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

Open jedwards1211 opened 3 months ago

jedwards1211 commented 3 months ago

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 commented 3 months ago

@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 commented 3 months ago

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 commented 3 months ago

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

gr2m commented 3 months ago

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 commented 3 months ago

Thanks! I'll look into that next week

jedwards1211 commented 3 months ago

@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 commented 3 months ago

@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 commented 3 months ago

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 commented 3 months ago

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

gr2m commented 3 months ago

@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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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)