Bug 150968 – Implement authentication challenge handling when using NETWORK_SESSION

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

[Alex Christensen]

Implement authentication challenge handling when using NETWORK_SESSION

[Alex Christensen]

Created attachment 264918 [details]
Patch

[Alex Christensen]

Comment on attachment 264918 [details]
Patch

This is conceptually a step in the right direction, but I think I'll have to reimplement a lot of this.  I'll still have to sometimes save the callback, communicate with the UIProcess to get a credential, but I think I'm going to have to redo the AuthenticationManager.  It does a lot of coalescing, which I don't think I'll need to do because I need to implement URLSession:didReceiveChallenge:completionHandler: separate from URLSession:task:didReceiveChallenge:completionHandler: instead of using ProtectionSpaceAuthenticationSchemeServerTrustEvaluationRequested, and I'll also have to do deal with protection spaces in a different place than NetworkResourceLoader::canAuthenticateAgainstProtectionSpaceAsync according to https://developer.apple.com/library/ios/documentation/Cocoa/Conceptual/URLLoadingSystem/Articles/AuthenticationChallenges.html

This first patch was just so I don't lose my first attempt before reverting everything and starting over.

[Alex Christensen]

Created attachment 265231 [details]
Patch

[Chris Dumez]

Comment on attachment 265231 [details]
Patch

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

> Source/WebKit2/NetworkProcess/NetworkLoad.cpp:167
> +        completionHandler(AuthenticationChallengeDisposition::RejectProtectionSpace, Credential());

I am pretty sure we want PerformDefaultHandling here. We basically want to let the underlying layer handle server trust in this case.

> Source/WebKit2/NetworkProcess/NetworkLoad.cpp:180
> +        completionHandler(AuthenticationChallengeDisposition::UseCredential, WebCore::Credential());

redundant WebCore::

> Source/WebKit2/Shared/Authentication/AuthenticationManager.cpp:206
> +        if (auto completionHandler = m_challengeCompletionHandlers.take(challengeID)) {

I would move the new code out of the if(!coreClient) scope and do this beforehand.

> Source/WebKit2/Shared/Authentication/AuthenticationManager.cpp:235
> +        if (auto completionHandler = m_challengeCompletionHandlers.take(challengeID)) {

I would move this before the if(!coreClient)

> Source/WebKit2/Shared/Authentication/AuthenticationManager.cpp:263
> +#if USE(NETWORK_SESSION)

I would move this before the if(!coreClient)

> Source/WebKit2/Shared/Authentication/AuthenticationManager.cpp:293
> +        if (auto completionHandler = m_challengeCompletionHandlers.take(challengeID)) {

I would move this before the if(!coreClient)

> Source/WebKit2/Shared/Authentication/AuthenticationManager.cpp:322
> +        if (auto completionHandler = m_challengeCompletionHandlers.take(challengeID)) {

I would move this before the if(!coreClient)

> Source/WebKit2/Shared/Authentication/AuthenticationManager.h:99
> +    HashMap<uint64_t, ChallengeCompletionHandler> m_challengeCompletionHandlers;

I think I would add a new struct wrapper to reduce the amount of #ifdefing:
struct AuthenticationRequest {
    WebCore::AuthenticationChallenge challenge;
#if USE(NETWORK_SESSION)
    ChallengeCompletionHandler completionHandler;
#endif
}

[Alex Christensen]

Other comments are good, and I will address.
(In reply to comment #4)
> > Source/WebKit2/Shared/Authentication/AuthenticationManager.h:99
> > +    HashMap<uint64_t, ChallengeCompletionHandler> m_challengeCompletionHandlers;
> 
> I think I would add a new struct wrapper to reduce the amount of #ifdefing:
> struct AuthenticationRequest {
>     WebCore::AuthenticationChallenge challenge;
> #if USE(NETWORK_SESSION)
>     ChallengeCompletionHandler completionHandler;
> #endif
> }
I think this would be great, except right now we have media loading code that makes authentication challenges without completion handlers in the WebProcess.  Once this changes, I think this change would be good.  And also getting rid of the addChallengeToChallengeMap that doesn't have a callback.

[Alex Christensen]

Created attachment 265238 [details]
Patch

[Alex Christensen]

Created attachment 265253 [details]
Patch

[Antti Koivisto]

Comment on attachment 265253 [details]
Patch

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

> Source/WebKit2/NetworkProcess/cocoa/NetworkSessionCocoa.mm:115
> +            auto completionHandlerCopy = Block_copy(completionHandler);
> +            client->didReceiveChallenge(challenge, [completionHandlerCopy](WebKit::AuthenticationChallengeDisposition disposition, const WebCore::Credential& credential)

Why the Block_copy?

[Alex Christensen]

http://trac.webkit.org/changeset/192287

[Alex Christensen]

(In reply to comment #8)
> Comment on attachment 265253 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=265253&action=review
> 
> > Source/WebKit2/NetworkProcess/cocoa/NetworkSessionCocoa.mm:115
> > +            auto completionHandlerCopy = Block_copy(completionHandler);
> > +            client->didReceiveChallenge(challenge, [completionHandlerCopy](WebKit::AuthenticationChallengeDisposition disposition, const WebCore::Credential& credential)
> 
> Why the Block_copy?
Blocks are by default on the stack in non-automatic-reference-counted code, so we can't store the std::function that wraps it and call it once it goes out of scope.  With Block_copy, it copies it to the heap so we can use it later.