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

ci: explicitly define permissions for each job and as the first property in each job #513

Merged
merged 1 commit into from
Dec 7, 2023

Conversation

G-Rath
Copy link
Contributor

@G-Rath G-Rath commented Nov 17, 2023

In my mind there's a number of advantages to this:

  • you can easily see all the permissions a job requires
  • the block always exists making it easier to pick up on when you need to add new permissions
    • because of this I've also put the permissions property as the first one in every job for consistency
  • we're leading by example, instead of doing something we're telling people not to do (using a global permission)

Note that our reusable workflows do still have a workflow-level permissions property - I've kept this because reusable workflows can only narrow permissions meaning the caller job should still always have a permissions block, and so in my mind for reusable workflows permissions is more "here's what you need to copy into the caller job" which would be more work if we were to have a reusable workflow with multiple jobs that had different permissions as you'd have to manually do the work of merging.

On the other hand they arguably muddy our "don't use global permissions" message so happy to refactor them to be per-job as well if people don't think it'll be a problem.

Also note that while we can and should configure the default permissions at the repository level, we still want to explicitly set the permissions in each workflow because we can't be sure the permissions are tightened at the repository level and this only costs us a couple of bytes in lines of code so there's no real downside 🤷

@G-Rath G-Rath force-pushed the lockdown-permissions branch from 4f687bd to d034c85 Compare November 30, 2023 22:51
@G-Rath G-Rath force-pushed the lockdown-permissions branch from d034c85 to e600a5f Compare November 30, 2023 22:53
@G-Rath G-Rath merged commit 3e546f7 into main Dec 7, 2023
22 checks passed
@G-Rath G-Rath deleted the lockdown-permissions branch December 7, 2023 21:52
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 this pull request may close these issues.

3 participants