search

canonical / has-signed-canonical-cla

1 stars 16 forks source link

index.js: refactor checks to allow permitted emails without github accounts #19

Open anonymouse64 opened 2 years ago

anonymouse64 commented 2 years ago

This allows a few cases that should be allowed:

  1. A person whose email address should be allowed creates a commit, but submits the PR from an account that was not configured to also have that email associated with the GitHub account. For example this could be a situation where someone does GitHub stuff on the side and not as part of their main job, but then has to submit a PR for their job to GitHub but hasn't associated the GitHub account to their work email.

  2. Someone is submitting the PR on behalf of an allowed contributor who has a permitted email. This could be the case where someone else wrote the code and has an email address which permits them, but then the give the git commit to a colleague to submit instead. The original contributor may not have a GitHub account for example.

  3. Finally, a project could have migrated from elsewhere to GitHub and a previous contributor sent in a commit/patch which was accepted before the project migrated and the project has turned on allowing existing contributors, so this email address should be permitted, even though that contributor may not have a GitHub account.

anonymouse64 commented 2 years ago

An example PR which is failing but should be allowed is here: https://github.com/snapcore/snapd/pull/10907

anonymouse64 commented 2 years ago

@seb128 sorry for not getting back to this, but you are correct in that this is lowering the standards somewhat but I think this is appropriate because the standard we are lowering right now is:

I think this is an unreasonably high standard and relaxing this check allows for all 3 of the use cases I mentioned above.

I personally don't think that it is fair on balance to complicate the CLA process for contributors in any of those 3 use cases just to have a check against someone maliciously submitting code which passes the CLA - they can already maliciously pass the CLA check on a PR by making their email address on the commit end in "@canonical.com" for someone who has linked their github account with their @canonical.com email address (or one of the other statically, always allowed emails which will bypass the check) anyways AFAICT. To do this, they just need to have a github account and then set their commit email to "someone@canonical.com" and then push it, github will show the commit as coming from that canonical user. See i.e. https://news.ycombinator.com/item?id=10005577 for example of this sort of attack

anonymouse64 commented 2 years ago

cc @mvo5

seb128 commented 2 years ago

Sorry, I sort of ended up in the reviewer position here because I've the needed acl to accept PRs but I've not been really involved before in the CLA checker and I don't feel I'm in position on redefining our standards.

I see you Cced mvo there. @mvo5 if you want to be added to the collaborators for the project let me know, also if you want to vouch for the change I can press the merge button for you.

beliaev-maksim commented 6 months ago

@seb128 -1 on this PR

from legal perspective we record GitHub accounts, LP accounts and emails. If that is not in the database, that legally does not exist and should not accepted