[webkit-reviews] review granted: [Bug 31844] SocketStreamHandleCFNet should support CONNECT proxy credentials : [Attachment 43796] proposed patch

[bugzilla-daemon at webkit.org]

Brady Eidson <beidson at apple.com> has granted Alexey Proskuryakov
<ap at webkit.org>'s request for review:
Bug 31844: SocketStreamHandleCFNet should support CONNECT proxy credentials
https://bugs.webkit.org/show_bug.cgi?id=31844

Attachment 43796: proposed patch
https://bugs.webkit.org/attachment.cgi?id=43796&action=review

------- Additional Comments from Brady Eidson <beidson at apple.com>
r+ with comments

> +void SocketStreamHandle::addCONNECTCredentials(CFHTTPMessageRef
proxyResponse)
> +{
> +    // Creating a temporary request to make CFNetwork apply credentials to
it. Unfortunately, this cannot work with NTLM authentication.
> +    RetainPtr<CFHTTPMessageRef> dummyRequest(AdoptCF,
CFHTTPMessageCreateRequest(0, CFSTR("GET"), m_httpsURL.get(),
kCFHTTPVersion1_1));
> +    RetainPtr<CFHTTPAuthenticationRef> authentication(AdoptCF,
CFHTTPAuthenticationCreateFromResponse(0, proxyResponse));
> +
> +    if
(!CFHTTPAuthenticationRequiresUserNameAndPassword(authentication.get())) {
> +	   // That's all we can offer...
> +	   m_client->didFail(this, SocketStreamError()); // FIXME: Provide a
sensible error.
> +	   return;
> +    }

Suggest re-ordering the above to create the authentication object, check if it
needs username/password, do the early return if it doesn't, then create the
dummy request.

Plus, how are we going to track getting a sensible error for this(x1)?

> +
> +    int port = 0;
> +    CFNumberGetValue(m_proxyPort.get(), kCFNumberIntType, &port);
> +    RetainPtr<CFStringRef> methodCF(AdoptCF,
CFHTTPAuthenticationCopyMethod(authentication.get()));
> +    RetainPtr<CFStringRef> realmCF(AdoptCF,
CFHTTPAuthenticationCopyRealm(authentication.get()));
> +    ProtectionSpace protectionSpace(String(m_proxyHost.get()), port,
ProtectionSpaceProxyHTTPS, String(realmCF.get()),
authenticationSchemeFromAuthenticationMethod(methodCF.get()));
> +    String login;
> +    String password;
> +    if (!m_sentStoredCredentials &&
getStoredCONNECTProxyCredentials(protectionSpace, login, password)) {
> +	   // Try to get sotred credentials, if we haven't tried those already.


Typo on this comment.  Plus, I think you mean to say "Try to *use* stored
credentials..."?

> +	   RetainPtr<CFStringRef> loginCF(AdoptCF, login.createCFString());
> +	   RetainPtr<CFStringRef> passwordCF(AdoptCF,
password.createCFString());
> +	   Boolean appliedCredentials =
CFHTTPMessageApplyCredentials(dummyRequest.get(), authentication.get(),
loginCF.get(), passwordCF.get(), 0);
> +	   ASSERT_UNUSED(appliedCredentials, appliedCredentials);
> +
> +	   RetainPtr<CFStringRef> proxyAuthorizationString(AdoptCF,
CFHTTPMessageCopyHeaderFieldValue(dummyRequest.get(),
CFSTR("Proxy-Authorization")));
> +
> +	   if (!proxyAuthorizationString) {
> +	       // Fails e.g. for NTLM auth.
> +	       m_client->didFail(this, SocketStreamError()); // FIXME: Provide
a sensible error.
> +	       return;
> +	   }

How are we going to track getting a sensible error for this (x2)?

> +
> +	   // Setting the authorization results in a new connection attempt.
> +	   wkSetCONNECTProxyAuthorizationForStream(m_readStream.get(),
proxyAuthorizationString.get());
> +	   m_sentStoredCredentials = true;
> +	   return;
> +    }
> +
> +    // FIXME: Ask the client if credentials could not be found.

How are we going to track asking the client?

> +
> +    m_client->didFail(this, SocketStreamError()); // FIXME: Provide a
sensible error.

How are we going to track getting a sensible error for this(x3)?