Bug 196520

Summary: [ews-build] Cancel build and similar operations should have authentication
Product: WebKit Reporter: Aakash Jain <aakash_jain>
Component: Tools / TestsAssignee: Aakash Jain <aakash_jain>
Status: RESOLVED FIXED    
Severity: Normal CC: aakash_jain, ap, commit-queue, dewei_zhu, kocsen_chung, lforschler, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Aakash Jain 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.
Comment 1 Aakash Jain 2019-04-02 17:48:49 PDT
Created attachment 366562 [details]
Patch
Comment 2 dewei_zhu 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?
Comment 3 Aakash Jain 2019-04-03 06:57:03 PDT
Created attachment 366598 [details]
Patch
Comment 4 Aakash Jain 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.
Comment 5 Lucas Forschler 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.
Comment 6 Kocsen Chung 2019-04-03 13:22:17 PDT
Agree with Lucas, we probably want to check if there is no username and password and fail.
Comment 7 Aakash Jain 2019-04-03 17:34:19 PDT
Created attachment 366677 [details]
Patch
Comment 8 Aakash Jain 2019-04-03 17:35:55 PDT
> I think if we fail to find an environment variable, we should probably not continue.
Done.
Comment 9 WebKit Commit Bot 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>
Comment 10 WebKit Commit Bot 2019-04-03 20:57:18 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Radar WebKit Bug Importer 2019-04-03 20:58:23 PDT
<rdar://problem/49590783>