Summary: | Implement authentication challenge handling when using NETWORK_SESSION | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alex Christensen <achristensen> | ||||||||||
Component: | New Bugs | Assignee: | Alex Christensen <achristensen> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | cdumez | ||||||||||
Priority: | P2 | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Alex Christensen
2015-11-05 23:19:39 PST
Created attachment 264918 [details]
Patch
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. Created attachment 265231 [details]
Patch
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 } 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. Created attachment 265238 [details]
Patch
Created attachment 265253 [details]
Patch
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? (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. |