Bug 196520

Summary:

[ews-build] Cancel build and similar operations should have authentication

Product:

WebKit

Reporter:

Aakash Jain <aakash_jain>

Component:

Tools / Tests

Assignee:

Aakash Jain <aakash_jain>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

aakash_jain, ap, commit-queue, dewei_zhu, kocsen_chung, lforschler, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

Other

Hardware:

Unspecified

OS:

Unspecified

See Also:

https://bugs.webkit.org/show_bug.cgi?id=221052

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none

Aakash Jain

Reported 2019-04-02 17:44:36 PDT

Currently there is no authentication on ews-build webpages. Anyone can press 'cancel' button and cancel all the running builds and all the pending buildrequests. Similarly anyone can pause/shutdown the workers. These operations should not be allowed without authentication.

Attachments
Patch (1.71 KB, patch) 2019-04-02 17:48 PDT, Aakash Jain	no flags	Details Formatted Diff Diff
Patch (1.71 KB, patch) 2019-04-03 06:57 PDT, Aakash Jain	no flags	Details Formatted Diff Diff
Patch (1.92 KB, patch) 2019-04-03 17:34 PDT, Aakash Jain	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Aakash Jain

Comment 1 2019-04-02 17:48:49 PDT

Created attachment 366562 [details] Patch

dewei_zhu

Comment 2 2019-04-02 18:38:23 PDT

Comment on attachment 366562 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=366562&action=review > Tools/BuildSlaveSupport/ews-build/master.cfg:4 > +from buildbot.plugins import * I would prefer to import exactly the models those will be used in this file so that it would be much easier to locate certain module / function. > Tools/BuildSlaveSupport/ews-build/master.cfg:22 > +admin_username = os.getenv('EWS_ADMIN_USERNAME') > +admin_password = os.getenv('EWS_ADMIN_PASSWORD') Does it work when neither environment is set? What happens if only either of them is set?

Aakash Jain

Comment 3 2019-04-03 06:57:03 PDT

Created attachment 366598 [details] Patch

Aakash Jain

Comment 4 2019-04-03 07:00:42 PDT

> I would prefer to import exactly the models those will be used in this file so that it would be much easier to locate certain module / function. Done. > Does it work when neither environment is set? Yes, in that case it will work without authentication. 'if admin_username and admin_password:' condition wouldn't be met and we won't set any authentication, this might be useful for local testing. > What happens if only either of them is set? Same as above, we won't configure the authentication since the 'if' condition won't be met.

Lucas Forschler

Comment 5 2019-04-03 13:01:28 PDT

Comment on attachment 366598 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=366598&action=review > Tools/BuildSlaveSupport/ews-build/master.cfg:32 > + I'm concerned that if we have no environment variable set, the default state of the server is to not require auth... I think if we fail to find an environment variable, we should probably not continue.

Kocsen Chung

Comment 6 2019-04-03 13:22:17 PDT

Agree with Lucas, we probably want to check if there is no username and password and fail.

Aakash Jain

Comment 7 2019-04-03 17:34:19 PDT

Created attachment 366677 [details] Patch

Aakash Jain

Comment 8 2019-04-03 17:35:55 PDT

> I think if we fail to find an environment variable, we should probably not continue. Done.

WebKit Commit Bot

Comment 9 2019-04-03 20:57:16 PDT

Comment on attachment 366677 [details] Patch Clearing flags on attachment: 366677 Committed r243854: <https://trac.webkit.org/changeset/243854>

WebKit Commit Bot

Comment 10 2019-04-03 20:57:18 PDT

All reviewed patches have been landed. Closing bug.

Radar WebKit Bug Importer

Comment 11 2019-04-03 20:58:23 PDT

<rdar://problem/49590783>

Note You need to log in before you can comment on or make changes to this bug.