Bug 196520 - [ews-build] Cancel build and similar operations should have authentication
Summary: [ews-build] Cancel build and similar operations should have authentication
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: 2019-04-02 17:44 PDT by Aakash Jain
Modified: 2019-04-03 20:58 PDT (History)
7 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
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>