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

PR previews: allow building PRs from project members only #10886

Open
stsewd opened this issue Nov 2, 2023 · 6 comments
Open

PR previews: allow building PRs from project members only #10886

stsewd opened this issue Nov 2, 2023 · 6 comments
Labels
Feature New feature Needed: design decision A core team decision is required

Comments

@stsewd
Copy link
Member

stsewd commented Nov 2, 2023

What's the problem this feature will solve?

Currently, we allow building PRs from any user. This may not be desirable for some projects.

Describe the solution you'd like

Have an option to disable building PRs from external contributors. We should be able to use GitHub's API to query if the PR was from an external contributor or not, o maybe just check if the PR was from a fork.

Alternative solutions

None

@stsewd stsewd added the Needed: design decision A core team decision is required label Nov 6, 2023
@humitos humitos added the Feature New feature label Nov 7, 2023
@stsewd
Copy link
Member Author

stsewd commented Jan 31, 2024

Taking some inspiration from circleci, they allow you to restrict building from forked PRs, and to allow you to choose if you wish to pass env vars to forked PRs.

Screenshot 2024-01-31 at 17-28-10 Advanced Settings - readthedocs org

With that in mind, I propose we change introduce a new option for PR builds, with the existing options we will have:

  • Build pull requests for this project
  • Build from forked pull requests (maybe group this with the above into one select option)
  • Privacy level of pull requests

And change the meaning of the privacy level of env vars from Expose this environment variable in PR builds? to Expose this environment variable in builds from forked pull requests?. This may be a breaking change I guess, depends on how users are using env vars, I'd say it should be safe to change them, but if we are in doubt, we can introduce a select option, something like Expose to all builds/Don't expose to builds from pull requests/Don't expose to builds from forked pull requests

@stsewd
Copy link
Member Author

stsewd commented Jan 31, 2024

And, both GitHub and GitLab provide a way to know if the PR is from a fork, so we are fine with that part. I was also thinking of an additional protection, check if the user who opened the PR is a member of the project, this is since GitHub used to allow external users to create PRs using an existing branch (I just tested this, and looks like GitHub no longer allows this, yay!), I'm still missing testing if that works on GitLab.

@stsewd
Copy link
Member Author

stsewd commented Feb 1, 2024

After testing GitLab, they also don't allow opening PRs for users outside the project using existing branches. The checks should be really simple now!

GitHub

is_fork = self.data.get('pull_request', {}).get('head', {}).get('repo', {}).get('fork', True)

GitLab

project_id = self.data.get('object_attributes', {}).get('target_project_id')
source_project_id = self.data.get('object_attributes', {}).get('source_project_id')
is_fork = project_id != source_project_id

@humitos
Copy link
Member

humitos commented Feb 12, 2024

I don't think we are saving those attributes in our db, right? So, we may need to update our RemoteRepository model to save them and re-sync all the data before implementing this feature, right?

@stsewd
Copy link
Member Author

stsewd commented Feb 12, 2024

I don't think we are saving those attributes in our db, right? So, we may need to update our RemoteRepository model to save them and re-sync all the data before implementing this feature, right?

We receive this data when processing the webhook, there is no need to have that in the DB.

@stevepiercy
Copy link
Contributor

In case this is useful, in one of our projects we limit running tests on pull requests from the repository only, not forks, with the following GitHub Action:

https://github.com/plone/volto/blob/f03e4b7c66be9c1c70680f2ae127b31e52bd0056/.github/workflows/acceptance.yml#L2-L5

on: [push, pull_request]
jobs:
  core:
    if: github.event_name != 'pull_request' || github.event.pull_request.head.repo.full_name != github.event.pull_request.base.repo.full_name

We also restrict write access to core organization team members.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New feature Needed: design decision A core team decision is required
Projects
Status: Planned
Development

No branches or pull requests

3 participants