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

Pass secrets to approved workflow jobs #258

Merged
merged 10 commits into from
Dec 2, 2023
Merged

Conversation

zimeg
Copy link
Member

@zimeg zimeg commented Nov 17, 2023

Summary

This PR attempts to pass secrets to integration tests for PRs opened from a fork. Fixes #251.

PRs not opened by a COLLABORATOR will initially fail because secrets "are not passed to workflows that are triggered by a pull request from a fork" but checking out the head.sha on a re-run is an apparent workaround for this and workflows can be re-run by a maintainer!

Notes

  • Other options for author_associations include MEMBER which might work better for the check in access_check. I think a re-run will pass this check in any case though 🤔

Requirements

@zimeg zimeg added semver:patch tests github_actions Pull requests that update GitHub Actions code labels Nov 17, 2023
@zimeg zimeg added this to the 1.x milestone Nov 17, 2023
@zimeg zimeg self-assigned this Nov 17, 2023
@zimeg
Copy link
Member Author

zimeg commented Nov 17, 2023

Good to know that MEMBER is different from COLLABORATOR! I'll try a re-run to see if that works 👀

Edit: Darn no luck with the re-run... Going to update the check to MEMBER now! 🙏

failed workflow run

@zimeg
Copy link
Member Author

zimeg commented Nov 17, 2023

Hmm... MEMBER passes the access check (unsure if this would be true for re-runs against other PRs) but secrets still aren't shared 🔐

@zimeg
Copy link
Member Author

zimeg commented Nov 17, 2023

Trouble with secret passing might be a limitation of the pull_request event entirely! There's an option to use pull_request_target instead which can access secrets but comes with many warnings around security. I think it's alright to use this event with caution and the "Require approval for outside collaborators" setting for Actions, but open to discussion!

Copy link
Member

@seratch seratch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally, we wan to run tests for PRs but this workaround looks good to me at this time

@zimeg zimeg marked this pull request as ready for review November 18, 2023 01:57
@zimeg
Copy link
Member Author

zimeg commented Nov 18, 2023

@seratch There are a few new changes so want to check that this looks good before merging.

The tests aren't running on this PR, but I believe this is because pull_request was removed and the updated pull_request_target doesn't yet exist on main.

Using pull_request_target for future PRs should start this workflow with necessary secrets and the github.event.pull_request.head.sha in actions/checkout@v4 will use the changes in the PR when building and testing changes.

runs-on: ubuntu-latest
steps:
- name: Check user permissions
if: ${{ github.event_name == 'pull_request' && github.event.pull_request.author_association != 'MEMBER' }}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pull_request_target still uses the event.pull_request values from what I can tell: https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#pull_request_target

Copy link
Member

@seratch seratch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zimeg Sorry for missing the additional changes here. All of them look good to me too. You can merge this PR tomorrow (or early next week).

@zimeg
Copy link
Member Author

zimeg commented Dec 2, 2023

@seratch No worries at all! Thank you for another review 🙌

@zimeg zimeg merged commit 197d9be into slackapi:main Dec 2, 2023
1 check passed
@zimeg zimeg deleted the access-checks branch December 2, 2023 00:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
github_actions Pull requests that update GitHub Actions code semver:patch tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Integration tests always fail for pull requests from a fork
2 participants