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

Potentially breaking change introduced in #897 - clarification needed #914

Closed
dominykas opened this issue Sep 5, 2024 · 6 comments · Fixed by #915
Closed

Potentially breaking change introduced in #897 - clarification needed #914

dominykas opened this issue Sep 5, 2024 · 6 comments · Fixed by #915
Labels

Comments

@dominykas
Copy link

dominykas commented Sep 5, 2024

#897 introduces a requirement for the token to be associated with a user that has maintain permission.

That was not necessary in the past, and should probably be considered a breaking change?

Moreover, we've never had issues with just the push permissions - not sure if there was any specific rationale around it? This was picked up in the review, and noted as "probably can't write issues, releases and pull requests" - but I'm not sure that's the case - the standard push permissions gives most of that access?

I'm also not 100% certain how that maps to the concept of roles and actual permissions in Github - it is possible to set things up in such a way that roles have different permissions, so quite possibly the naive maintain check is not enough and needs explicit checks for specific API access?

When the user does not have the maintain permission, it then performs the check to HEAD /installation/repositories - however that is only necessary when running as an app. If semantic-release is running inside e.g. Jenkins - then this check does not make sense, as generally semantic-release does not need to "List repositories accessible to the app installation" anyways to perform a release?

@travi
Copy link
Member

travi commented Sep 5, 2024

@babblebey @jedwards1211 let's get this change reverted and regroup to consider if there is a safer way to accomplish this goal

@jedwards1211
Copy link
Contributor

oof, sorry about that, sure wish the GitHub API just gave us a method to check that the token has the exact permissions we need

@travi travi closed this as completed in #915 Sep 5, 2024
Copy link

github-actions bot commented Sep 5, 2024

🎉 This issue has been resolved in version 10.3.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

@jedwards1211
Copy link
Contributor

@travi @babblebey it should work if we keep everything in the PR except the check for maintain permission. I was looking back through the GitHub docs and I realized my mistake. The table of repository role permissions lists:

  • Read
  • Triage
  • Write
  • Maintain
  • Admin

I got confused because the Get a repository method lists:

  • pull
  • triage
  • push
  • maintain
  • admin

I mistakenly assumed push was merely permission to push, not one of the roles in the first list, but now I realize, push must correspond to Write and pull must correspond to Read...argh. And as OP mentions, Write AKA push does have permission to write issues and releases according to the table.

Do you want me to make a new PR with everything but the maintain check?

@gr2m I wish the Get a repository API method had explicit documentation about the permissions subfields.

@jedwards1211
Copy link
Contributor

jedwards1211 commented Sep 5, 2024

@dominykas I just reread this more carefully:

it is possible to set things up in such a way that roles have different permissions

From what I can see, there are just a few limits that can be configured for repository roles in an organization -- it doesn't look like it's possible to remove permission to push commits, edit issues, releases, or pull requests from the push role. If you have seen documentation that contradicts this please let me know.

@dominykas
Copy link
Author

If you have seen documentation that contradicts this please let me know.

I'll admit that I haven't looked into this in detail - I just know that it's possible to make some changes there.

Thanks for looking into this!

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

Successfully merging a pull request may close this issue.

3 participants