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

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Aakash Jain]

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.

[Aakash Jain]

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?

[Aakash Jain]

Created attachment 366598 [details]
Patch

[Aakash Jain]

> 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 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]

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

[Aakash Jain]

Created attachment 366677 [details]
Patch

[Aakash Jain]

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

[WebKit Commit Bot]

Comment on attachment 366677 [details]
Patch

Clearing flags on attachment: 366677

Committed r243854: <https://trac.webkit.org/changeset/243854>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/49590783>