search

dcoapp / app

GitHub App that enforces the Developer Certificate of Origin (DCO) on Pull Requests
https://github.com/apps/dco
ISC License
299 stars 75 forks source link

Check for valid emails #34

Closed caniszczyk closed 6 years ago

caniszczyk commented 6 years ago

We had a case where someone signed off a commit with an invalid email address, it would be nice if the bot caught this: https://github.com/envoyproxy/envoy/pull/1690/commits/981375f5cf9b36144d295bfa15807fe7d30f3d59

Shouldn't the bot be checking against a valid email associated with the GitHub id?

caniszczyk commented 6 years ago

cc: @bkeepers

bkeepers commented 6 years ago

Interesting. At the moment it only checks that the signoff matches the commit author.

Is it specified anywhere that this needs to be a valid email address? I mean, it seems pretty obvious that it should be, but it's interesting that the kernel docs make a point that it should use your real name, but doesn't say anything about the email address. It's probably assumed that the commit author should include a real email.

caniszczyk commented 6 years ago

@bkeepers the best answer I got is from Linux Foundation legal counsel is that a real email address is expected, otherwise it's expected to reject the PR. Also notable that the GitHub UI flags this as an invalid email (see attached).

screen shot 2017-09-22 at 8 31 20 am

Ideally the email in the Signed-Off-By would match one of the registered/verified emails associated with a person's GitHub profile.

bkeepers commented 6 years ago

Cool. Thanks for the info. I think it makes sense to add strict email checking to this app.

Ideally the email in the Signed-Off-By would match one of the registered/verified emails associated with a person's GitHub profile.

What if I get a patch sent to a mailing list from a user without a GitHub account and want to contribute it upstream to a project on GitHub? I don't know if that's even legit with the DCO or how common of a use case it is. I just want to make sure we don't add restrictions that don't make sense.

Second issue: there's not an API to look up GitHub users by email (for privacy reasons), and getting the emails associated with a user account will require them to OAuth against the app (for each organization that it is installed on) and give permission for reading emails. Will that add too much friction?

What about this:

  1. First ship an update that checks for a valid-ish looking email address by format
  2. Add a configuration option that is disabled by default to require that the email is verified on GitHub from the user submitting the pull request.
caniszczyk commented 6 years ago

re #1, that is a great step forward and will catch 99.999% of issues!

re #2: sounds good but I'd have that as low priority now

The use case you mentioned is legit and happens in LMKL land.

Thanks for the super quick response on this, we're using probot more on LF-related repos so appreciate the support here :)

hiimbex commented 6 years ago

First ship an update that checks for a valid-ish looking email address by format

Simple fix would probably be to use this npm package to check email format for us: https://www.npmjs.com/package/email-validator 👍

mattklein123 commented 6 years ago

I'm definitely in favor of option (2) also and would be happy to turn that on for Envoy and require that people use verified emails. (Makes sense to make this opt-in).

caniszczyk commented 6 years ago

closing this since there is basic support here: https://github.com/probot/dco/pull/35