Bug 189668 – Clean up AuthenticationChallengeProxy

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alex Christensen]

Clean up AuthenticationChallengeProxy

[Alex Christensen]

Created attachment 349905 [details]
Patch

[Alex Christensen]

Created attachment 349911 [details]
Patch

[EWS Watchlist]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Alex Christensen]

Created attachment 349917 [details]
Patch

[Alex Christensen]

Created attachment 349932 [details]
Patch

[Alex Christensen]

Created attachment 349934 [details]
Patch

[EWS Watchlist]

Comment on attachment 349934 [details]
Patch

Attachment 349934 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9248186

New failing tests:
accessibility/smart-invert-reference.html

[EWS Watchlist]

Created attachment 349954 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[Alex Christensen]

test failure unrelated

[youenn fablet]

Comment on attachment 349934 [details]
Patch

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

> Source/WebKit/UIProcess/ServiceWorkerProcessProxy.cpp:101
> +        challenge->listener().useCredential(WebCredential::create(credential).ptr());

Preexisting code but it seems a bit weird to wrap the WebCore credential into a WebCredential one.
Could the listener directly take a WebCore::Credential*?
If it exposes this API, I guess WebCredential makes sense. Maybe it could also directly take a WebCore::credential overload.

> Source/WebKit/UIProcess/Authentication/AuthenticationChallengeProxy.cpp:45
> +AuthenticationChallengeProxy::AuthenticationChallengeProxy(WebCore::AuthenticationChallenge&& authenticationChallenge, uint64_t challengeID, IPC::Connection* connection, WeakPtr<SecKeyProxyStore>&& secKeyProxyStore)

A null connection does not make a lot of sense probably, could be a Connection& probably.
Since it is refed by the lambda, it could be made a Ref<>&&.

> Source/WebKit/UIProcess/Authentication/AuthenticationChallengeProxy.cpp:47
> +    , m_listener(AuthenticationDecisionListener::create([challengeID, connection = makeRefPtr(connection), secKeyProxyStore = WTFMove(secKeyProxyStore)](AuthenticationChallengeDisposition disposition, WebCredential* credential) {

I am not sure how readable that is to make a big lambda in the constructor.
Here it seems to make sense to take a WebCore credential.

> Source/WebKit/UIProcess/Authentication/AuthenticationChallengeProxy.cpp:69
> +            }

Previously in the code, if m_secKeyProxyStore is null, we were sending ContinueWithoutCredentialForChallenge.
Now we are sending UseCredentialForChallenge even though we might not send the client certificate.
Is there no change of behavior? Should we assert that secKeyProxyStore is not null if having ProtectionSpaceAuthenticationSchemeClientCertificateRequested?

> Source/WebKit/UIProcess/Authentication/AuthenticationDecisionListener.cpp:52
> +    if (m_completionHandler)

Can we have normal cases where we would call useCredential() and m_completionHandler be null?
Should we assert?

> Source/WebKit/UIProcess/Authentication/AuthenticationDecisionListener.cpp:53
> +        m_completionHandler(AuthenticationChallengeDisposition::UseCredential, credential);

By going with the completion handler, we end-up with an unneeded switch in the completion handler, slight loss of efficiency here.

It might be slightly clearer to have a ContinueWithoutCredential in case credential is nullptr, or maybe surface a continueWithoutCredential so that we would pass a Credential& here.

> Source/WebKit/UIProcess/Authentication/AuthenticationDecisionListener.cpp:59
> +        m_completionHandler(AuthenticationChallengeDisposition::Cancel, nullptr);

With Expected<Credential*, Action>, we would not need this nullptr, here and below.
But we probably more often end up without credential than with credentials.

> Source/WebKit/UIProcess/Authentication/AuthenticationDecisionListener.h:31
>  #include <wtf/RefPtr.h>

Is RefPtr.h include needed?

> Source/WebKit/UIProcess/Authentication/cocoa/AuthenticationChallengeProxyCocoa.mm:39
> +void AuthenticationChallengeProxy::sendClientCertificateCredentialOverXpc(IPC::Connection* connection, SecKeyProxyStore& secKeyProxyStore, uint64_t challengeID, const WebCore::Credential& credential)

Connection& probably.

[Alex Christensen]

Comment on attachment 349934 [details]
Patch

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

Thanks for the great review.  I'll make this patch even better with this feedback.

>> Source/WebKit/UIProcess/Authentication/AuthenticationChallengeProxy.cpp:69
>> +            }
> 
> Previously in the code, if m_secKeyProxyStore is null, we were sending ContinueWithoutCredentialForChallenge.
> Now we are sending UseCredentialForChallenge even though we might not send the client certificate.
> Is there no change of behavior? Should we assert that secKeyProxyStore is not null if having ProtectionSpaceAuthenticationSchemeClientCertificateRequested?

A SecKeyProxyStore has a lifetime that is tied to the WebsiteDataStore.  If there's no SecKeyProxyStore it means we are stopping the entire session, so it doesn't matter what we do because everything is in the process of being cancelled.  In this case, using a credential without a SecKeyProxyStore will just cause a different failure right before cancelling.  I think this is significantly more elegant than having to capture whether the protection space is a client certificate request and having complex logic here.

>> Source/WebKit/UIProcess/Authentication/AuthenticationDecisionListener.cpp:53
>> +        m_completionHandler(AuthenticationChallengeDisposition::UseCredential, credential);
> 
> By going with the completion handler, we end-up with an unneeded switch in the completion handler, slight loss of efficiency here.
> 
> It might be slightly clearer to have a ContinueWithoutCredential in case credential is nullptr, or maybe surface a continueWithoutCredential so that we would pass a Credential& here.

I plan to get rid of ContinueWithoutCredential and replace it with UseCredentialForChallenge with no credential.  This matches the NSURLSession API, and it removes an unnecessary duplicate code path.

[Alex Christensen]

Created attachment 350034 [details]
Patch

[Alex Christensen]

Created attachment 350037 [details]
Patch

[Alex Christensen]

Created attachment 350039 [details]
Patch

[Alex Christensen]

http://trac.webkit.org/r236153

[Radar WebKit Bug Importer]

<rdar://problem/44574166>

[Jiewen Tan]

Have done a manual test on tlstestwebkit.org. Doesn't seem to break anything.

[youenn fablet]

This patch seems to break Safari with WebKit nightly builds:
- Open a new tab and try loading google.com by clicking an icon: will hang
- Open google.com through "Open on new tab" and it will succeed.

[youenn fablet]

(In reply to youenn fablet from comment #18)
> This patch seems to break Safari with WebKit nightly builds:
> - Open a new tab and try loading google.com by clicking an icon: will hang
> - Open google.com through "Open on new tab" and it will succeed.

Tested on High Sierra

[youenn fablet]

(In reply to youenn fablet from comment #19)
> (In reply to youenn fablet from comment #18)
> > This patch seems to break Safari with WebKit nightly builds:
> > - Open a new tab and try loading google.com by clicking an icon: will hang
> > - Open google.com through "Open on new tab" and it will succeed.
> 
> Tested on High Sierra

Filed https://bugs.webkit.org/show_bug.cgi?id=190202