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.
Created attachment 366562 [details] Patch
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?
Created attachment 366598 [details] Patch
> 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 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.
Agree with Lucas, we probably want to check if there is no username and password and fail.
Created attachment 366677 [details] Patch
> I think if we fail to find an environment variable, we should probably not continue. Done.
Comment on attachment 366677 [details] Patch Clearing flags on attachment: 366677 Committed r243854: <https://trac.webkit.org/changeset/243854>
All reviewed patches have been landed. Closing bug.
<rdar://problem/49590783>