Co-authored PR suggestions

[bmt-tock]

I've noticed that suggestions on PRs result in a commit that includes a co-author. This co-author then ends up as a contributor. Github required reviews don't apply any restriction on reviews to users who have contributed in this way. However policy bot does.

From reading the code, it looks like as-is there is no way to differentiate between an external contributor who pushed a change to the branch and suggestions through the UI.

Would it be acceptable to include some kind of option that doesn't ban based on co-authorship -- basically don't consider commits with multiple contributors in the approver banning code. If this sounds like a plausible path forward, I can put together a PR and we can bikeshed what to call this option :)

My goal is to create a set of review requirements that is like required github required reviewers but with exceptions. This is the only blocker right now.

[bluekeyes]

I think this makes sense and I've encountered the same problem before. I could see implementing this either as an allowance (e.g. allow_coauthors), which would allow approval from contributors who co-authored a commit, or as a suggestion-specific property (e.g. ignore_suggestions), which would remove the users who provided the suggestion from the contributor list or ignore the commit completely for computing contributors.

One thing to consider is how GitHub represents multi-author commits in the API. If you can create something that looks like a suggestion by adding a Co-authored-by line to a regular commit message, would that allow someone to abuse this property to bypass approval? If so, it may be desirable to also check the commit was created by the GitHub UI.

[bmt-tock]

Good call on the potential bypass. I will do some testing and try to put together a PR. It will probably be a couple weeks before I can get time for it.

Thanks!

On Wed, Jun 15, 2022, 11:31 PM Billy Keyes @.***> wrote:

I think this makes sense and I've encountered the same problem before. I could see implementing this either as an allowance (e.g. allow_coauthors), which would allow approval from contributors who co-authored a commit, or as a suggestion-specific property (e.g. ignore_suggestions), which would remove the users who provided the suggestion from the contributor list or ignore the commit completely for computing contributors.

One thing to consider is how GitHub represents multi-author commits in the API. If you can create something that looks like a suggestion by adding a Co-authored-by line to a regular commit message, would that allow someone to abuse this property to bypass approval? If so, it may be desirable to also check the commit was created by the GitHub UI.

— Reply to this email directly, view it on GitHub https://github.com/palantir/policy-bot/issues/439#issuecomment-1157220224, or unsubscribe https://github.com/notifications/unsubscribe-auth/ATEQT7HEPNYCAD6JHNQ6C2LVPKUYTANCNFSM5YZFJYPA . You are receiving this because you authored the thread.Message ID: @.***>

[bmt-tock]

After digging into this a bit today, it looks like co-authors aren't showing up in the github api responses at all and I was able to pass policies even though the approver was a co-author on one commit. I could have sworn when we tested this a few weeks ago co-authors couldn't approve, but it looks like it must have been something else. For now, I'm going to close this issue since it looks like things are already working as intended.