Security: Make grafana org users to be able to publish grafana/prometheus package - #209
Security: Make grafana org users to be able to publish grafana/prometheus package#209itsmylife wants to merge 10 commits into
Conversation
| if [[ "$PERMISSION" != "admin" ]]; then | ||
| echo "::error::Only repository admins can run this workflow. Your permission level: ${PERMISSION}" | ||
| exit 1 | ||
| check_permission() { |
There was a problem hiding this comment.
this broadens the set of accounts that can publish to npm.
is the tradeoff that we reduce the use of admin accounts?
There was a problem hiding this comment.
Yes. Less admins we have less issues potentially we might get. We can make the whole group as admin but I don't think it's a good idea. Or maybe that should be the idea we'd follow. I'm open suggestions.
There was a problem hiding this comment.
For instance github-datasource as all group members as admin https://github.com/grafana/github-datasource/settings/access
| local permission | ||
| permission=$(gh api "repos/${REPO}/collaborators/${actor}/permission" --jq '.permission') | ||
|
|
||
| if [[ "$permission" != "admin" && "$permission" != "write" ]]; then |
There was a problem hiding this comment.
Both dispatching a workflow_dispatch run and re-running a workflow already require write access to the repo, so after the admin-to-write downgrade this gate no longer excludes anyone who could reach it. If a stronger control than GitHub's built-in write requirement is wanted, it might be worth putting required reviewers or a deployment policy on the npm-publish environment instead, since that is enforced at the job that actually holds the OIDC token.
There was a problem hiding this comment.
Good points and those are already in place.
|
|
||
| check_permission "$ACTOR" | ||
| if [[ "$TRIGGERING_ACTOR" != "$ACTOR" ]]; then | ||
| check_permission "$TRIGGERING_ACTOR" |
There was a problem hiding this comment.
Consider noting that this only covers reruns of the whole workflow. If someone re-runs just the failed publish job, this job does not re-execute, so the new triggering_actor is never checked and only GitHub's write requirement for reruns covers that path.
| # Prevent overlapping publish runs from racing on the same package version. | ||
| concurrency: | ||
| group: npm-publish-grafana-prometheus | ||
| cancel-in-progress: false |
There was a problem hiding this comment.
nit: even with cancel-in-progress: false, GitHub keeps only the newest pending run in a concurrency group, so if two releases are dispatched while one is publishing, the older queued run is silently cancelled. Probably acceptable here, but worth knowing when releases are dispatched in quick succession.
There was a problem hiding this comment.
in general there will be one person releasing the packages so I think it's fine to keep it.
| publish: | ||
| name: Publish @grafana/prometheus | ||
| needs: build | ||
| if: needs.build.outputs.changed == 'true' && inputs['dry-run'] != true |
There was a problem hiding this comment.
nit: the changed == 'true' half is already enforced by the "Abort if version not newer" step failing the build job, so this is defence in depth rather than load-bearing. Fine to keep, just flagging it so a future reader does not assume it is the only guard.
4 participants
Summary
writeoradminaccess to run the npm publish workflow.