Bug 127849 – WebKit Bot Watcher's Dashboard: Access restricted queue should only prompt for HTTP credentials once per page load

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Daniel Bates]

A queue, whose associated buildbot requires authentication, should only prompt for HTTP credentials at most once per page load to avoid inundating a person with HTTP authentication prompts (e.g. say, they aren't authorized to access the buildbot) whenever the queue is requested to update.

[Daniel Bates]

Created attachment 222720 [details]
Patch

[Daniel Bates]

Created attachment 222721 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 222721 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=222721&action=review

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Buildbot.js:40
> +    this.isAuthenticated = !this.needsAuthentication;

This is counter-intuitive. A host that doesn't even need authentication is marked as authenticated.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Buildbot.js:89
> +        request.open("HEAD", this.baseURL);

I don't understand why this is needed. Is it working around a Safari/WebKit bug where we don't coalesce authentication dialogs?

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueue.js:164
> +            // FIXME: We should consider defining an event, say UnauthorizedAccess, to notify listeners to update
> +            // their state when an authorization error occurs instead of repurposing the event IterationsAdded.

I agree that sending a fake IterationsAdded is not great. Is doing the right thing complicated?

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueueView.js:72
> +    _appendUnauthorizedLineView: function(queue)

Nice.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/StatusLineView.js:26
> -StatusLineView = function(message, status, label, repeatCount, url)
> +StatusLineView = function(message, status, label, repeatCount, urlOrFunction)

Would a javascript: URL work? If we can just pass such, that seems better to me than making the code more polymorphic.

[Daniel Bates]

Comment on attachment 222721 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=222721&action=review

>> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Buildbot.js:40
>> +    this.isAuthenticated = !this.needsAuthentication;
> 
> This is counter-intuitive. A host that doesn't even need authentication is marked as authenticated.

One idea to make this more intuitive is to rename isAuthenticated to authenticationStatus and define a set of values that this instance variable can take: Unauthenticated, Authenticated, and Invalid Credentials. Initially, authenticationStatus := Unauthenticated. So, a Buildbot that doesn't require authentication will have (needsAuthentication, authenticationStatus) = (false, Unauthenticated) and a Buildbot that requires authentication and hasn't prompted a person for credentials will have (needsAuthentication, authenticationStatus) = (true, Unauthenticated).

>> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Buildbot.js:89
>> +        request.open("HEAD", this.baseURL);
> 
> I don't understand why this is needed. Is it working around a Safari/WebKit bug where we don't coalesce authentication dialogs?

Yes, the primary reason for this function is to work around a bug where we don't coalesce authentication dialogs. I'll look to file a bug for this issue if one doesn't exist.

Although we only have two iOS Buildbot queues, this function may also prevent excessive network requests depending on how WebKit/Safari determines when to show an authentication. Specifically, this function ensures that we make exactly one HTTP request (per authentication attempt) before allowing all of the queues associated with an access-restricted Buildbot to independently update themselves.

>> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueue.js:164
>> +            // their state when an authorization error occurs instead of repurposing the event IterationsAdded.
> 
> I agree that sending a fake IterationsAdded is not great. Is doing the right thing complicated?

Will add event BuildbotQueue.Event.UnauthorizedAccess and dispatch it when an HTTP 401 error occurs.

>> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/StatusLineView.js:26
>> +StatusLineView = function(message, status, label, repeatCount, urlOrFunction)
> 
> Would a javascript: URL work? If we can just pass such, that seems better to me than making the code more polymorphic.

Yes, we can take advantage of the global array buildbots to tell the Buildbot we're interested in to update its queues. Will revert the changes to the StatusLineView constructor and remove the selector .status-line .bubble.can-be-clicked from file dashboard/Styles/StatusLineView.css (since we don't need to change the style of the cursor to give the appearance of a hyperlink as the status line will now be an actual hyperlink).

[Daniel Bates]

(In reply to comment #4)
> (From update of attachment 222721 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=222721&action=review
> 
> [...]
> >> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Buildbot.js:89
> >> +        request.open("HEAD", this.baseURL);
> > 
> > I don't understand why this is needed. Is it working around a Safari/WebKit bug where we don't coalesce authentication dialogs?
> 
> Yes, the primary reason for this function is to work around a bug where we don't coalesce authentication dialogs. I'll look to file a bug for this issue if one doesn't exist.

Filed bug #128006 to coalesce credential requests.

[Daniel Bates]

Created attachment 222848 [details]
Patch

Updated patch based on Alexey Proskuryakov's remarks.

It seems sufficient to have the browser prompt exactly once for each access-restricted queue on page load and expose a way to trigger an authentication dialog from the dashboard. This allows a person who doesn't have permission to view the status of the queue to view the dashboard without the annoyance of being prompted for credentials each time the queue update timer fires (every 45 seconds as of the time of writing).

For now, I am removing the logic to work around bug #128006 as the effects of this bug can be seen as an annoyance. That is, the effects don't significantly impact the functionality of the dashboard. Ideally we should fix bug #128006 so that we we only present a single authentication dialog for all access-restricted queues associated with the same Buildbot. We can consider adding back the work around should it turn out that it's either non-trivial to fix bug #128006 or its effects are seen as a significant annoyance.

[Daniel Bates]

Created attachment 222852 [details]
[Screenshot] Unauthorized status line

[Alexey Proskuryakov]

Comment on attachment 222848 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=222848&action=review

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Buildbot.js:109
> +        // FIXME: We may want to assert that the authentication status didn't change. When such an assertion fails
> +        // it indirectly indicates that we made an synchronous HTTP request, which we should avoid to ensure app
> +        // responsiveness.

Sounds easier to just add the assert, and not a FIXME :)

We definitely shouldn't be making sync requests.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueue.js:166
> +                // FIXME: Safari/WebKit should coallesce authentication requests with the same origin and authentication realm.
> +                // See <https://bugs.webkit.org/show_bug.cgi?id=128006>.

I usually use this format:

// FIXME (128006): Safari/WebKit should coallesce authentication requests with the same origin and authentication realm.

It would be helpful to add a comment about what the observable effect of this issue is in this particular case.

[Daniel Bates]

Comment on attachment 222848 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=222848&action=review

>> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Buildbot.js:109
>> +        // responsiveness.
> 
> Sounds easier to just add the assert, and not a FIXME :)
> 
> We definitely shouldn't be making sync requests.

Will add assert and remove the FIXME comment.

>> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueue.js:166
>> +                // See <https://bugs.webkit.org/show_bug.cgi?id=128006>.
> 
> I usually use this format:
> 
> // FIXME (128006): Safari/WebKit should coallesce authentication requests with the same origin and authentication realm.
> 
> It would be helpful to add a comment about what the observable effect of this issue is in this particular case.

Will update comment to read:

// FIXME (128006): Safari/WebKit should coallesce authentication requests with the same origin and authentication realm.
// In absence of the fix, Safari presents additional authentication dialogs regardless of whether an earlier authentication
// dialog was dismissed. As a way to ameliorate the user experience where a person authenticated successfully using an
// earlier authentication dialog and cancelled the authentication dialog associated with the load for this queue, we call
// ourself so that we can schedule another load, which should complete successfully now that we have credentials.

[Daniel Bates]

Committed r163213: <http://trac.webkit.org/changeset/163213>

[Daniel Bates]

(In reply to comment #10)
> Committed r163213: <http://trac.webkit.org/changeset/163213>

Changed equality operator to identity operator when comparing HTTP status code and committed this fix in <http://trac.webkit.org/changeset/163268>.