[webkit-reviews] review denied: [Bug 65330] Remove auth-related functions from SubresourceLoaderClient : [Attachment 104198] Remove didReceiveAuthenticationChallenge() - more ChangeLog

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Nate Chapin
<japhet at chromium.org>'s request for review:
Bug 65330: Remove auth-related functions from SubresourceLoaderClient
https://bugs.webkit.org/show_bug.cgi?id=65330

Attachment 104198: Remove didReceiveAuthenticationChallenge() - more ChangeLog
https://bugs.webkit.org/attachment.cgi?id=104198&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=104198&action=review


Thanks, the split patch is so much easier to review. Seems like this has a few
rough edges, but looks like a nice improvement overall.

One challenge is that existing cross origin policies are incorrect in that they
are too limiting. When a cross-origin XMLHttpRequest is redirected, we should
actually perform a new preflight, and not just bail out (not quite sure if
we're allowed to ask the user for credentials after the redirect though). We
might actually need to call into DocumentThreadableLoader, and in this sense,
this patch could be a step in a wrong direction. Perhaps it's OK to make this
step back to make existing code clearer, and think about implementing CORS
redirects when we come to that.

> Source/WebCore/ChangeLog:6
> +	   Note that this only

Please, more, more ChangeLog!

> Source/WebCore/loader/FrameLoaderTypes.h:127
> +    enum UserCredentialPolicy {

Despite what I said before, this is about asking the client, which will not
necessarily ask the user.

> Source/WebCore/loader/FrameLoaderTypes.h:130
> +	   AskUserForCredentialsAlways,
> +	   AskUserForCredentialsSameOrigin,
> +	   AskUserForCredentialsNever

The enum values would benefit from being better formed grammatically
(AskUserForCredentialsNever vs. NeverAskUserForCredentials).

For AskUserForCredentialsSameOrigin in particular, I can't tell what this
option does from looking at its name. Is it really
AskUserForCredentialsUnlessRedirectedCrossOrigin? Or if it's about cross-origin
requests, why can't we decide that upfront and pass AskUserForCredentialsNever?


> Source/WebCore/loader/SubresourceLoader.cpp:260
> +	   if
(m_frame->document()->securityOrigin()->canRequest(request().url()))

This check doesn't seem to match what we had before. We looked at
DocumentThreadableLoader::m_sameOriginRequest, which is set in constructor,
while request() changes with every redirect. IIRC DocumentThreadableLoader just
prevents cross origin redirects now (which is not correct according to the CORS
spec), but you don't want to rely on the fact that DocumentThreadableLoader is
the only client that passes AskUserForCredentialsSameOrigin.

> Source/WebCore/loader/SubresourceLoader.cpp:269
> +#if PLATFORM(MAC) || USE(CFNETWORK) || USE(CURL)
> +	   handle()->receivedRequestToContinueWithoutCredential(challenge);
> +#endif
> +	   if (handle()->hasAuthenticationChallenge())
> +	       cancel();

Old code also called didFail with blockedError. Why is that not needed?
ChangeLog doesn't say.