search

taskcluster / taskcluster-rfcs

Taskcluster team planning
Mozilla Public License 2.0
11 stars 19 forks source link

Author Association Roles for Github PRs #173

Open ahal opened 2 years ago

ahal commented 2 years ago

I was brainstorming alternative ways to solve the mobile org's contributor woes (other than RFC 168), and came up with this idea.

Github Pull Request events have an author_association field which can be one of COLLABORATOR, CONTRIBUTOR, FIRST_TIMER, FIRST_TIME_CONTRIBUTOR, MEMBER, NONE or OWNER. This means that TC GIthub can tell whether the PR is coming from a trusted source or not. TC Github can then assume a different role based on this fact. E.g, <repo>:pull-request if the PR is trusted, and <repo>:pull-request-untrusted if it is not. This logic would live somewhere around here.

If we want to preserve backwards compatibility, I think we'd need to create a new pullRequest policy (e.g mixed), and the above behaviour would only happen when it is used. If we don't care about preserving backwards compatibiliy, this could be the new behaviour of the public policy (though pulling this off feels like it would be hard).

The following should be considered out of scope for any potential RFCs here (though it may warrant a releng-rfc), but writing it out as an example of how this feature can be useful:

In ci-config we can grant sensitive scopes to the trusted role, and disallow them in the untrusted role. The last step is that projects will need to ensure they don't run any "sensitive" tasks for untrusted PRs (otherwise they'd get a scope expression failure). Since we pass the full Github event into .taskcluster.yml as context, this can be accomplished by creating a new "author_association" parameter, as well as a new target_tasks filter that uses it.

Once we have all of this, projects will be able to run "tasks" for PRs opened by the public, and sensitive tasks for PRs opened by collaborators.

ahal commented 2 years ago

It occurs to me that the author_association field probably isn't needed as taskcluster-github already has logic to determine whether a PR is opened by a collaborator or not. I believe we may just need to compute this a bit earlier so we can pass it down to the intree function (which is where the roles are assumed).

djmitche commented 2 years ago

Using GitHub's determination will probably significantly decrease the attack surface, though - especially if introducing a new policy, this might be a good idea.

Dustin

On Tue, May 31, 2022, 10:21 AM Andrew Halberstadt @.***> wrote:

It occurs to me that the author_association field probably isn't needed as taskcluster-github already has logic to determine whether a PR is opened by a collaborator or not. I believe we may just need to do compute this a bit earlier so we can pass it down to the intree function (which is where the scopes are assumed).

— Reply to this email directly, view it on GitHub https://github.com/taskcluster/taskcluster-rfcs/issues/173#issuecomment-1142202317, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAHAAIH3KA2U7YDCFEWWKTVMYN7BANCNFSM5XLQWHEA . You are receiving this because you are subscribed to this thread.Message ID: @.***>

ahal commented 2 years ago

Good idea. The TC Github check also considers where the PR is coming from, so we'd just have to make sure we don't solely rely on author_association, but use it in addition to that check.

ahal commented 2 years ago

The current implementation is using this function from octokit/rest to determine collaborator status: https://octokit.github.io/rest.js/v18#repos-check-collaborator

It looks like it's already doing the right thing, and is likely handling edge cases that wouldn't immediately be obvious if we were building custom logic on top of author_association.

djmitche commented 2 years ago

Ah, I apologize -- my information was outdated!