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

Clarify How the permissions are calculated for a workflow job #32398

Open
1 task done
jsoref opened this issue Apr 5, 2024 · 11 comments
Open
1 task done

Clarify How the permissions are calculated for a workflow job #32398

jsoref opened this issue Apr 5, 2024 · 11 comments
Labels
actions This issue or pull request should be reviewed by the docs actions team content This issue or pull request belongs to the Docs Content team help wanted Anyone is welcome to open a pull request to fix this issue SME reviewed An SME has reviewed this issue/PR

Comments

@jsoref
Copy link
Contributor

jsoref commented Apr 5, 2024

Code of Conduct

What article on docs.github.com is affected?

https://docs.github.com/en/actions/security-guides/automatic-token-authentication#how-the-permissions-are-calculated-for-a-workflow-job

What part(s) of the article would you like to see updated?

Finally,
if the workflow was triggered by a pull request from a forked repository,
and the Send write tokens to workflows from pull requests setting is not selected,
the permissions are adjusted to change any write permissions to read only.

Should be changed to say clarify that if the workflow was triggered by a pull request and the job event is pull_request_target then write permissions will not be changed to read only.

I'm still recovering from a concussion, but here's my first attempt at fixing this text:

Finally,
if the workflow was triggered for the pull_request event (and not the pull_request_target event) by a pull request from a forked repository,
and the Send write tokens to workflows from pull requests setting is not selected,
the permissions are adjusted to change any write permissions to read only.

Additional information

No response

@jsoref jsoref added the content This issue or pull request belongs to the Docs Content team label Apr 5, 2024
@github-actions github-actions bot added the triage Do not begin working on this issue until triaged by the team label Apr 5, 2024
@nguyenalex836 nguyenalex836 added actions This issue or pull request should be reviewed by the docs actions team waiting for review Issue/PR is waiting for a writer's review and removed triage Do not begin working on this issue until triaged by the team labels Apr 5, 2024
@nguyenalex836
Copy link
Contributor

@jsoref Sorry to hear about the concussion 💛 I'll get this triaged for review ✨

@jc-clark jc-clark added the needs SME This proposal needs review from a subject matter expert label May 30, 2024
Copy link
Contributor

Thanks for opening an issue! We've triaged this issue for technical review by a subject matter expert 👀

@nguyenalex836
Copy link
Contributor

@jsoref Hello! 👋 Our engineering team reviewed, and agreed with your proposed clarification. They also wanted to mention the following -

The suggested text is not quite right though - the read-only behavior applies to all pull request events triggered on a fork PR except pull_request_target

So it’s not correct to say it only applies to pull_request events, it would apply to pull_request_review and the other PR/issue related events

With that adjustment in mind, you or anyone else is welcome to open a PR with this update ✨

CC @jc-clark just for visibility 💛

@nguyenalex836 nguyenalex836 added help wanted Anyone is welcome to open a pull request to fix this issue SME reviewed An SME has reviewed this issue/PR and removed waiting for review Issue/PR is waiting for a writer's review needs SME This proposal needs review from a subject matter expert labels Jun 14, 2024
@Vikranth3140
Copy link

Hello @nguyenalex836,

I have read through the conversation and the suggestions from the engineering team. Please review the proposed update for the documentation to clarify the behavior of workflow job permissions. The read-only adjustment applies to all pull request-related events triggered on a fork PR except for the pull_request_target event. This ensures that users understand the specific conditions under which write permissions remain unchanged.

Updated section:

Finally, if the workflow was triggered by a pull request from a forked repository, and the Send write tokens to workflows from pull requests setting is not selected, the permissions are adjusted to change any write permissions to read-only, except for the pull_request_target event. This applies to all pull request-related events, including pull_request_review.

Is this good to go?

@jsoref
Copy link
Contributor Author

jsoref commented Jun 18, 2024

@Vikranth3140: the pull_request_review bit was a correction to my mentioning pull_request. I think it's probably best to omit both.

You probably want to include:

For more information, see "[AUTOTITLE](/actions/using-workflows/events-that-trigger-workflows#pull_request_target)."

(That text appears in places like https://docs.github.com/en/actions/security-guides/automatic-token-authentication#permissions-for-the-github_token -- see https://github.com/search?q=repo%3Agithub%2Fdocs%20see%20%22%5BAUTOTITLE%5D(%2Factions%2Fusing-workflows%2Fevents-that-trigger-workflows).%22&type=code)

@Vikranth3140
Copy link

Vikranth3140 commented Jun 18, 2024

Hello @jsoref,

Thank you for the feedback. I understand the need to clarify the read-only behaviour without specifying pull_request or pull_request_review. I will update the documentation to reflect that the read-only adjustment applies to all pull request-related events triggered on a fork PR except for the pull_request_target event.

Additionally, I will include a reference link for further information:

Finally, if the workflow was triggered by a pull request from a forked repository, and the Send write tokens to workflows from pull requests setting is not selected, the permissions are adjusted to change any write permissions to read-only, except for the pull_request_target event.

For more information, see "AUTOTITLE."

Is this approach good to go?

@jsoref
Copy link
Contributor Author

jsoref commented Jun 18, 2024

The order of the logic doesn't really work for me. It's convoluted.

I wouldn't use except there.

I suspect I'd want "isn't a pull_request_target and the send... Isn't checked".

I don't have time to think about it further today. Maybe someone else will before I get back...

@nguyenalex836
Copy link
Contributor

@Vikranth3140 Hello! 👋 Thank you for opening a PR for this issue! Our team will provide feedback on your proposed changes in the PR you opened 💛 #33566

@jsoref Thank you for providing feedback on the proposed changes ✨

@Kon-pov9

This comment was marked as spam.

@Kon-pov9

This comment was marked as spam.

@github-actions github-actions bot added the stale There is no recent activity on this issue or pull request label Nov 5, 2024
@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Nov 12, 2024
@jsoref
Copy link
Contributor Author

jsoref commented Nov 12, 2024

@nguyenalex836

@nguyenalex836 nguyenalex836 reopened this Nov 12, 2024
@github-actions github-actions bot added the triage Do not begin working on this issue until triaged by the team label Nov 12, 2024
@nguyenalex836 nguyenalex836 removed triage Do not begin working on this issue until triaged by the team stale There is no recent activity on this issue or pull request labels Nov 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
actions This issue or pull request should be reviewed by the docs actions team content This issue or pull request belongs to the Docs Content team help wanted Anyone is welcome to open a pull request to fix this issue SME reviewed An SME has reviewed this issue/PR
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants