Bug 196870 – Add a WebsiteDataStore delegate to handle AuthenticationChallenge that do not come from pages

• Component: Service Workers
• Status: RESOLVED FIXED

[youenn fablet]

Add a WebsiteDataStore delegate to handle AuthenticationChallenge that do not come from pages

[youenn fablet]

Created attachment 367337 [details]
Patch

[youenn fablet]

@wilander, this should take care of your issue with HTTPS ping loads.

[John Wilander]

Comment on attachment 367337 [details]
Patch

Nice. Could you add a Ping layout test with testRunner.setAllowsAnySSLCertificate(true) and the ping endpoint as https? Should be easy.

[youenn fablet]

Created attachment 367347 [details]
Patch

[youenn fablet]

(In reply to John Wilander from comment #3)
> Comment on attachment 367337 [details]
> Patch
> 
> Nice. Could you add a Ping layout test with
> testRunner.setAllowsAnySSLCertificate(true) and the ping endpoint as https?
> Should be easy.

Yep, probably need to terminate network process and then do a ping.

[youenn fablet]

Created attachment 367351 [details]
Patch

[Alex Christensen]

Comment on attachment 367351 [details]
Patch

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

I think this needs an API test that verifies this delegate is being called under the correct circumstances and the credentials are used as expected.

> Source/WebKit/NetworkProcess/PingLoad.cpp:135
> -        completionHandler(AuthenticationChallengeDisposition::PerformDefaultHandling, { });
> +        m_networkLoadChecker->networkProcess().authenticationManager().didReceiveAuthenticationChallenge(m_parameters.sessionID, 0, 0, challenge, WTFMove(completionHandler));

We almost certainly don't want ping loads to handle authentication challenges.  Then there would be random popups asking for credentials.

> Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStore.mm:82
> +        case NSURLSessionAuthChallengeUseCredential:

This is duplicate code with NavigationState.mm

> LayoutTests/ChangeLog:8
> +        * http/wpt/beacon/cors/crossorigin-arraybufferview-no-preflight.html:

Wouldn't this changed test have passed before because we performed default behavior?  Now we're just performing default behavior in a different place, so this test does not verify the change works.

[Alex Christensen]

Challenge.mm has a growing number of similar authentication unit tests.

[youenn fablet]

(In reply to Alex Christensen from comment #7)
> Comment on attachment 367351 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=367351&action=review
> 
> I think this needs an API test that verifies this delegate is being called
> under the correct circumstances and the credentials are used as expected.

I'll add one.
 
> > Source/WebKit/NetworkProcess/PingLoad.cpp:135
> > -        completionHandler(AuthenticationChallengeDisposition::PerformDefaultHandling, { });
> > +        m_networkLoadChecker->networkProcess().authenticationManager().didReceiveAuthenticationChallenge(m_parameters.sessionID, 0, 0, challenge, WTFMove(completionHandler));
> 
> We almost certainly don't want ping loads to handle authentication
> challenges.  Then there would be random popups asking for credentials.

This is limited to server trust evaluation, there is no need to ask user for credentials.
There might be a need for prompt to ask whether user trusts the site or not, but this is really an application decision.
Overall, the design I see is that this additional delegate would only be about asking whether the given certificate is already granted, not whether user is granting it.

Also, ping loads are no different from service worker fetches in that respect.
In the future, ping loads might just be regular fetch until their context is destroyed.
See https://bugs.webkit.org/show_bug.cgi?id=196807

> > LayoutTests/ChangeLog:8
> > +        * http/wpt/beacon/cors/crossorigin-arraybufferview-no-preflight.html:
> 
> Wouldn't this changed test have passed before because we performed default
> behavior?  Now we're just performing default behavior in a different place,
> so this test does not verify the change works.

Before the patch, the test would be broken.
By killing the network process, we are sure there is no connection.
The ping triggers a new connection, thus a server trust evaluation.
The default behavior will be done in network process and will cancel the creation of the connection.
After the patch, we go to UIProcess and ask WTR whether to proceed with the load.

[youenn fablet]

Created attachment 368012 [details]
Patch

[youenn fablet]

Created attachment 368033 [details]
Patch

[Alex Christensen]

Comment on attachment 368033 [details]
Patch

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

> Source/WebKit/NetworkProcess/PingLoad.cpp:151
> +        m_networkLoadChecker->networkProcess().authenticationManager().didReceiveAuthenticationChallenge(m_parameters.sessionID, 0, 0, challenge, WTFMove(completionHandler));

This needs rebasing.  The 0's are now empty identifiers.  We might need to make them nullopt's.

> Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStore.mm:54
> +        , m_hasAuthenticationChallengeSelector([m_delegate.get() respondsToSelector:@selector(didReceiveAuthenticationChallenge: completionHandler:)])

extra space

[youenn fablet]

Created attachment 376999 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 376999 [details]
Patch for landing

Clearing flags on attachment: 376999

Committed r249001: <https://trac.webkit.org/changeset/249001>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/54593556>

[Truitt Savell]

It looks like the changes in https://trac.webkit.org/changeset/249001/webkit

caused this test to fail constantly: imported/w3c/web-platform-tests/service-workers/service-worker/websocket-in-service-worker.https.html

it was a flakey failure before. The diff makes it look like this is just a rebasline, can you confirm? 

History:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=imported%2Fw3c%2Fweb-platform-tests%2Fservice-workers%2Fservice-worker%2Fwebsocket-in-service-worker.https.html

Diff:
--- /Volumes/Data/slave/highsierra-release-tests-wk2/build/layout-test-results/imported/w3c/web-platform-tests/service-workers/service-worker/websocket-in-service-worker.https-expected.txt
+++ /Volumes/Data/slave/highsierra-release-tests-wk2/build/layout-test-results/imported/w3c/web-platform-tests/service-workers/service-worker/websocket-in-service-worker.https-actual.txt
@@ -1,3 +1,3 @@
 
-FAIL Verify WebSockets can be created in a Service Worker assert_equals: expected "PASS" but got "FAIL: Got an error event"
+PASS Verify WebSockets can be created in a Service Worker

[Ryan Haddad]

Reverted r249001 for reason:

Caused one layout test to fail on all configurations and another to time out on Catalina / iOS 13.

Committed r249063: <https://trac.webkit.org/changeset/249063>

[youenn fablet]

Created attachment 377238 [details]
Relanding

[youenn fablet]

(In reply to Ryan Haddad from comment #18)
> Reverted r249001 for reason:
> 
> Caused one layout test to fail on all configurations and another to time out
> on Catalina / iOS 13.

Layout test was going from FAIL to PASS.
The other test was updated to cope with the more accurate handling of window.testRunner.setAllowsAnySSLCertificate

[WebKit Commit Bot]

Comment on attachment 377238 [details]
Relanding

Rejecting attachment 377238 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 377238, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in LayoutTests/imported/w3c/ChangeLog contains OOPS!.

Full output: https://webkit-queues.webkit.org/results/12968665

[youenn fablet]

Created attachment 377241 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 377241 [details]
Patch for landing

Clearing flags on attachment: 377241

Committed r249096: <https://trac.webkit.org/changeset/249096>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.