Bug 219608 – [ews] commit-queue should use only the first email from contributors.json to validate committers and reviewers

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Aakash Jain]

commit-queue should use only the first email from contributors.json to validate committers and reviewers.

Currently contributors.json can have multiple email addresses for any user. However, allowing the committer and reviewer permissions to all those emails isn't very secure. Additional email addresses might be old emails, which can have expired domains that someone could register and become a committer illegitimately.

[Radar WebKit Bug Importer]

<rdar://problem/72058809>

[Aakash Jain]

Created attachment 415574 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 415574 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=415574&action=review

> Tools/ChangeLog:9
> +        (ValidateCommiterAndReviewer.load_contributors): Use only first email for validating commiters and reviewers.

Did you verify that this won't break CQ for active contributors at least?

[Alexey Proskuryakov]

Comment on attachment 415574 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=415574&action=review

> Tools/CISupport/ews-build/steps.py:760
> +                bugzilla_email = emails[0].lower()  # We're assuming the first email is a valid bugzilla email

I think that this is more like requiring than assuming. Also, valid is probably not the best word here, as it's unclear to me hoe a Bugzilla email can be invalid. Lastly, please put a period at the end of the sentence.

[Aakash Jain]

(In reply to Alexey Proskuryakov from comment #3)
> Did you verify that this won't break CQ for active contributors at least?
Haven't verified yet if all the active contributors have their primary Bugzilla email as the first email in contributors.json (in case of multiple emails). We should do that verification before landing this patch.

[Aakash Jain]

(In reply to Alexey Proskuryakov from comment #4)
> I think that this is more like requiring than assuming. Also, valid is probably not the best word here, as it's unclear to me how a Bugzilla email can be invalid.
Yeah, I simply copied this line from https://trac.webkit.org/browser/webkit/trunk/Tools/Scripts/webkitpy/common/config/committers.py#L71, will re-word it.

[Jonathan Bedard]

Far as I can tell, only two contributors would have been broken by this:

Antoine Quint [e-mail]
Justin Michaud [e-mail]

feel like we should just fix those two cases.

[Jonathan Bedard]

Created attachment 415637 [details]
Patch

[Aakash Jain]

Comment on attachment 415637 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=415637&action=review

> Tools/ChangeLog:1
> [phone]  Aakash Jain  <[e-mail]>

This might need to be updated.

[Jonathan Bedard]

Created attachment 415638 [details]
Patch for landing

[EWS]

Committed r270538: <https://trac.webkit.org/changeset/270538>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 415638 [details].