Bug 219608 - [ews] commit-queue should use only the first email from contributors.json to validate committers and reviewers
Summary: [ews] commit-queue should use only the first email from contributors.json to ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Aakash Jain
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-12-07 12:07 PST by Aakash Jain
Modified: 2020-12-08 07:19 PST (History)
7 users (show)

See Also:


Attachments
Patch (1.52 KB, patch)
2020-12-07 13:24 PST, Aakash Jain
no flags Details | Formatted Diff | Diff
Patch (2.41 KB, patch)
2020-12-08 06:39 PST, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch for landing (2.41 KB, patch)
2020-12-08 06:48 PST, Jonathan Bedard
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Aakash Jain 2020-12-07 12:07:40 PST
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.
Comment 1 Radar WebKit Bug Importer 2020-12-07 12:25:27 PST
<rdar://problem/72058809>
Comment 2 Aakash Jain 2020-12-07 13:24:44 PST
Created attachment 415574 [details]
Patch
Comment 3 Alexey Proskuryakov 2020-12-07 13:26:31 PST
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?
Comment 4 Alexey Proskuryakov 2020-12-07 13:27:57 PST
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.
Comment 5 Aakash Jain 2020-12-07 13:29:57 PST
(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.
Comment 6 Aakash Jain 2020-12-07 13:31:32 PST
(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.
Comment 7 Jonathan Bedard 2020-12-07 14:40:40 PST
Far as I can tell, only two contributors would have been broken by this:

Antoine Quint graouts@webkit.org
Justin Michaud justin@justinmichaud.com

feel like we should just fix those two cases.
Comment 8 Jonathan Bedard 2020-12-08 06:39:38 PST
Created attachment 415637 [details]
Patch
Comment 9 Aakash Jain 2020-12-08 06:44:16 PST
Comment on attachment 415637 [details]
Patch

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

> Tools/ChangeLog:1
> +2020-12-08  Aakash Jain  <aakash_jain@apple.com>

This might need to be updated.
Comment 10 Jonathan Bedard 2020-12-08 06:48:38 PST
Created attachment 415638 [details]
Patch for landing
Comment 11 EWS 2020-12-08 07:19:23 PST
Committed r270538: <https://trac.webkit.org/changeset/270538>

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