Bug 65330 – Remove auth-related functions from SubresourceLoaderClient

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Nate Chapin]

It appears that getShouldUseCredentialStorage(), didReceiveAuthenticationChallenge() and receivedCancellation() don't actually do much (in the various SubresourceLoaderClients).  The code would probably be simpler if we pushed the little bit of logic in the clients down into SubresourceLoader and got rid of these callbacks.

It would also make efforts like https://bugs.webkit.org/show_bug.cgi?id=61225 easier by reducing the number of callbacks that need to be rewired.

Patch shortly.

[Nate Chapin]

Created attachment 102275 [details]
patch

All the LayoutTests still pass, and I've done a little bit of manual browsing with the patch (I plan on doing some more).

If there's something in particular I should be testing manually, suggestions are appreciated.

[Adam Barth]

This looks like a good change.  I'm going to give other folks a chance to comment though.

[Alexey Proskuryakov]

I'm on vacation now, but would like to take a close look next week.

I vaguely recall that getShouldUseCredentialStorage() in particular is supposed to be filtered down to WebKit client.

[Nate Chapin]

(In reply to comment #3)
> I'm on vacation now, but would like to take a close look next week.
> 
> I vaguely recall that getShouldUseCredentialStorage() in particular is supposed to be filtered down to WebKit client.

If I changed any of the calls back to the WebKit client, it's a bug. :)

This should only change calls that are made to DocumentThreadableLoader (and therefore the ThreadableLoaderClients) and IconLoader.

[Alexey Proskuryakov]

Comment on attachment 102275 [details]
patch

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

I like the direction of this patch. I didn't review it in a lot of detail yet, because I had some general comments that I think you may want to address first.

FrameLoaderClient calls are made from ResourceLoader, which is why changing SubresourceLoader shouldn't affect them indeed - we just need to make sure that we fall through to ResourceLoader in all the same cases.

> Source/WebCore/loader/ResourceLoader.cpp:524
> +    if (!m_request.allowCookies())
> +        return false;

This is worth at least a comment explaining why we tie cookies and HTTP credentials together like this.

> Source/WebCore/loader/ResourceLoader.cpp:559
> +    didReceiveResponse(challenge.failureResponse());

In XMLHttpRequest case at least, this will change when InspectorInstrumentation is notified. Is that an improvement?

For MainResourceLoader, this changes a lot of code that runs in this case. Why is that OK? The changes that went to ResourceLoader particularly worry me, because the patch is supposed to only affect subresources.

Perhaps this would be easier to review if there were three patches, one for each SubresourceLoaderClient method that's being removed. It's rather difficult for me to follow every code path, given that this patch actually changes what code is being run.

> Source/WebCore/loader/SubresourceLoader.cpp:254
> +        // Only these platforms provide a way to continue without credentials, and users shouldn't be prompted
> +        // for credentials on a cross-origin request. Fall through and cancel for other platfroms.

Typo: platfroms.

> Source/WebCore/loader/SubresourceLoader.cpp:261
> +    case AuthenticateNever:
> +        cancel();

It's a mystery why we'd try to continue without credentials in AuthenticateSameOrigin, but not in AuthenticateNever.

Another subtlety is when it's OK to use stored session credentials, and when it's OK to ask a user for credentials. These are separate questions, but the AuthenticateWhen naming scheme doesn't differentiate between the two.

I think that for this change to really be an improvement, SubresourceLoader options should be direct and straightforward, without traps like these.

> Source/WebCore/loader/SubresourceLoader.cpp:264
> +    default:
> +        ASSERT_NOT_REACHED();

Is there a specific reason to add this default clause? It prevents the compiler from generating an error if some cases are not handled.

[Nate Chapin]

r/SubresourceLoader.cpp:261
> > +    case AuthenticateNever:
> > +        cancel();
> 
> It's a mystery why we'd try to continue without credentials in AuthenticateSameOrigin, but not in AuthenticateNever.
> 
> Another subtlety is when it's OK to use stored session credentials, and when it's OK to ask a user for credentials. These are separate questions, but the AuthenticateWhen naming scheme doesn't differentiate between the two.

If I'm reading this correctly, it appears ResourceHandle calls ResourceLoader::shouldUseCredentialStorage() and uses credential storage first, and only calls ResourceLoader::didReceiveAuthenticationChallenge() if stored credentials are no longer an option.

[Alexey Proskuryakov]

That suggest that better names could be something like NeverAskUserForCredentials instead of AuthenticateNever.

[Nate Chapin]

(In reply to comment #7)
> That suggest that better names could be something like NeverAskUserForCredentials instead of AuthenticateNever.

Good point, I'll rename that stuff.

I think I'll follow your other suggestion and split this into a separate patch for each function, too.

[Nate Chapin]

Created attachment 102970 [details]
Remove getShouldUseCredentialStorage()

I gave my best shot at explaining the logic for the allowCookies() check in ResourceLoader::shouldUseCredentialStorage(), let me know if I said something stupid :)

[Nate Chapin]

Created attachment 102985 [details]
Remove didReceiveAuthenticationChallenge()

[Nate Chapin]

Created attachment 102990 [details]
Remove didReceiveAuthenticationChallenge()

Oops, uploaded the same patch twice.

[Nate Chapin]

Created attachment 103077 [details]
Remove receivedCancellation()

After some more studying, it appears receivedCancellation() doesn't actually do anything in any SubresourceLoaderClient.  Keep in mind that it's always followed by a didFail() because ResourceLoader::receivedCancellation() cancels the load.  didFail() and receivedCancellation() do the same thing in both WorkerScriptLoader and Notification.  In XMLHttpRequest, receivedCancellation() just sets m_response, and didFail() clears m_response before using it in any way.

[Alexey Proskuryakov]

Comment on attachment 103077 [details]
Remove receivedCancellation()

Let's start with this patch, as it seems to be the simplest one. It is subtle however, because not every browser UI provides a way for the user to cancel an authentication dialog. However, clients may pick either behavior (cancel request altogether or continue without credentials and use whatever data comes with 401 response).

Can you explain in ChangeLog why it's OK to make the change for each of the modified files? I'm particularly unsure about XMLHttpRequest - who will set m_response if didReceiveAuthenticationCancellation does not? Or how will WorkerScriptLoader know that it's in error state?

[Nate Chapin]

(In reply to comment #13)
> (From update of attachment 103077 [details])
> Let's start with this patch, as it seems to be the simplest one. It is subtle however, because not every browser UI provides a way for the user to cancel an authentication dialog. However, clients may pick either behavior (cancel request altogether or continue without credentials and use whatever data comes with 401 response).
> 
> Can you explain in ChangeLog why it's OK to make the change for each of the modified files? I'm particularly unsure about XMLHttpRequest - who will set m_response if didReceiveAuthenticationCancellation does not? Or how will WorkerScriptLoader know that it's in error state?

Will do.  I'll essentially be rewording comment #12. :)

[Alexey Proskuryakov]

> I'll essentially be rewording comment #12. :)

I did overlook that comment indeed. Let's get it into ChangeLog though.

[Nate Chapin]

Created attachment 103316 [details]
Remove receivedCancellation() - with better ChangeLog

[Alexey Proskuryakov]

Comment on attachment 103316 [details]
Remove receivedCancellation() - with better ChangeLog

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

> Source/WebCore/ChangeLog:9
> +        Remove receivedCancellation() from SubresourceLoaderClient.
> +        It's a no-op because it's always immediately followed by a
> +        canceallation (ResourceLoader::receivedCancellation() calls
> +        cancel(), which leads to calling SubresourceLoaderClinet::didFail()).
> +        For WorkerScriptLoader and Notification, receivedCancellation()
> +        and didFail() are identical. For XMLHttpRequest, receivedCancellation()
> +        sets m_response, which didFail() clears before it can be used.

Please don't put explanations into ChangeLog like this. There is an automatically generated list of files below. Simply listing modified files and functions is not very helpful, and the reason for the script to add it is that contributors will use it as a starting point.

> Source/WebCore/xml/XMLHttpRequest.cpp:-1054
> -void XMLHttpRequest::didReceiveAuthenticationCancellation(unsigned long, const ResourceResponse& failureResponse)
> -{
> -    m_response = failureResponse;
> -}

This code was added in <http://trac.webkit.org/changeset/24862> without a regression test. But it does seem fine to remove it now - WebKit (at least ports that I checked) no longer does a cancellation when the user cancels an authentication sheet. It now asks WebKit to continue without credentials.

[Nate Chapin]

Comment on attachment 103316 [details]
Remove receivedCancellation() - with better ChangeLog

receivedCancellation() removal landed: http://trac.webkit.org/changeset/92691

[Alexey Proskuryakov]

Would you be willing to annotate ChangeLogs of the remaining patches with equally good explanations?

[Nate Chapin]

(In reply to comment #19)
> Would you be willing to annotate ChangeLogs of the remaining patches with equally good explanations?

Sure.  Will remove r? from the existing patches in the interim.

[Nate Chapin]

Created attachment 104190 [details]
Remove getShouldUseCredentialStorage() - with more ChangeLog

Hopefully the changelog + comment in ResourceLoader.cpp covers this patch sufficiently.

[WebKit Review Bot]

Attachment 104190 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Nate Chapin]

Created attachment 104198 [details]
Remove didReceiveAuthenticationChallenge() - more ChangeLog

[Alexey Proskuryakov]

Comment on attachment 104198 [details]
Remove didReceiveAuthenticationChallenge() - more ChangeLog

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.

[Nate Chapin]

> 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.

I'm sort of inclined to punt on this issue until we get around to dealing with CORS redirects, but I'm going to think about it some more before uploading a new patch.

> 
> > Source/WebCore/ChangeLog:6
> > +        Note that this only
> 
> Please, more, more ChangeLog!

I'll do my best :)

> 
> > 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.

Hmm.....good point.  ClientCredentialPolicy?

> 
> > 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.

Good catch, that should be originalRequest() I think.

[Nate Chapin]

Created attachment 104727 [details]
Remove didReceiveAuthenticationChallenge() #3

Hopefully this is enough ChangeLog....not sure how I managed to leave that sentence fragment in the previous version.

[Alexey Proskuryakov]

Comment on attachment 104190 [details]
Remove getShouldUseCredentialStorage() - with more ChangeLog

I'm feeling conflicted about this patch:
1. When looking at ResourceLoader code, it's unnatural to expect that some policy choices are overridden by a subclass. So, getting rid of that is good.
2. The weird check for m_request.allowCookies() is indeed weird. We had very clear and straightforward code in the client.

I'm going to say r+, but could the real answer be to move the client call from SubresourceLoaderClient to ResourceLoaderClient, clarifying control flow in this way?

[Nate Chapin]

(In reply to comment #27)
> (From update of attachment 104190 [details])
> I'm feeling conflicted about this patch:
> 1. When looking at ResourceLoader code, it's unnatural to expect that some policy choices are overridden by a subclass. So, getting rid of that is good.
> 2. The weird check for m_request.allowCookies() is indeed weird. We had very clear and straightforward code in the client.
> 
> I'm going to say r+, but could the real answer be to move the client call from SubresourceLoaderClient to ResourceLoaderClient, clarifying control flow in this way?

We could plumb the relevant variable in ThreadableLoaderOptions (m_allowCredentials) down to SubresourceLoader?  Not sure if that's better or worse.

[Alexey Proskuryakov]

I think that this would be more straightforward than second-guessing from ResourceRequest's idea of whether to send cookies.

[Nate Chapin]

(In reply to comment #29)
> I think that this would be more straightforward than second-guessing from ResourceRequest's idea of whether to send cookies.

I revived https://bugs.webkit.org/show_bug.cgi?id=63301 in the hope of making passing these sorts of parameters more sane than a long string of bools.

[Nate Chapin]

Created attachment 105410 [details]
Remove getShouldUseCredentialStorage() without the allowCookies() check

[Alexey Proskuryakov]

Comment on attachment 105410 [details]
Remove getShouldUseCredentialStorage() without the allowCookies() check

Nice!

[WebKit Review Bot]

Comment on attachment 105410 [details]
Remove getShouldUseCredentialStorage() without the allowCookies() check

Clearing flags on attachment: 105410

Committed r93923: <http://trac.webkit.org/changeset/93923>

[Alexey Proskuryakov]

Comment on attachment 104727 [details]
Remove didReceiveAuthenticationChallenge() #3

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

Some comments for discussion below.

> Source/WebCore/ChangeLog:21
> +        (WebCore::SubresourceLoader::didReceiveAuthenticationChallenge): Handle the authentication

The good thing about two previous patches was that SubresourceLoader override was completely removed, making the logic much easier to follow. This is not the case here, so I'm not sure if the change is beneficial.

> Source/WebCore/loader/cache/CachedResourceRequest.cpp:128
> +        request.get(), resourceRequest, AlwaysAskClientForCredentials, priority, securityCheck, sendResourceLoadCallbacks);

I wonder how important it is to show credentials dialog when a cross-origin subresource or subframe wants it. It's certainly confusing and dangerous.

Definitely not something to change in this patch.

> Source/WebCore/loader/icon/IconLoader.cpp:-131
> -    m_resourceLoader->cancel();

This patch changes behavior for IconLoader in a way that's not mentioned in ChangeLog.

We use to always cancel such requests, but now we attempt to continue without credentials.

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

I would add a comment explaining that receivedRequestToContinueWithoutCredential will normally clear the challenge. Or it might be even better to put the other case in #else, so that no one could get confused as to whether we do didFail() after receivedRequestToContinueWithoutCredential().

There is a comment above the part I quoted, but it's fairly indirect.

Also, we used to have a cancel() call after didFail() in DocumentThreadableLoader::didReceiveAuthenticationChallenge, and IconLoader also performed a cancel(). Why is this OK to change?

> Source/WebCore/loader/DocumentThreadableLoader.cpp:346
> +        m_loader = resourceLoadScheduler()->scheduleSubresourceLoad(m_document->frame(), this, request, OnlyAskClientForCredentialsForSameOrigin, ResourceLoadPriorityMedium, securityCheck, sendLoadCallbacks,

It would be good to have a FIXME comment, explaining how OnlyAskClientForCredentialsForSameOrigin is not a proper policy. But it may be difficult to write - I certainly cannot do that without consulting relevant specs.

[Nate Chapin]

> The good thing about two previous patches was that SubresourceLoader override was completely removed, making the logic much easier to follow. This is not the case here, so I'm not sure if the change is beneficial.

I was thinking about re-writing this patch to pass the policy via ResourceLoaderOptions, which would enable us to move all the login into ResourceLoader very easily.

I'd need to figure out how to handle the similar between the StoredCredential policy and the new ClientCredentialPolicy, but it seems like it would still be cleaner.

[Nate Chapin]

Created attachment 105654 [details]
Remove didReceiveAuthenticationChallenge() #4

[WebKit Review Bot]

Attachment 105654 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Nate Chapin]

Created attachment 105664 [details]
Remove didReceiveAuthenticationChallenge() - minus style issue

[Eric Seidel (no email)]

Comment on attachment 103316 [details]
Remove receivedCancellation() - with better ChangeLog

Cleared Alexey Proskuryakov's review+ from obsolete attachment 103316 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Eric Seidel (no email)]

Comment on attachment 104190 [details]
Remove getShouldUseCredentialStorage() - with more ChangeLog

Cleared Alexey Proskuryakov's review+ from obsolete attachment 104190 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Alexey Proskuryakov]

Comment on attachment 105664 [details]
Remove didReceiveAuthenticationChallenge() - minus style issue

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

> Source/WebCore/loader/ResourceLoaderOptions.h:66
> +    { }

These braces should be on separate lines now.

> Source/WebCore/loader/ResourceLoaderOptions.h:71
> +    ClientCrossOriginCredentialPolicy crossOriginCredentialPolicy; // Whether we will ask the client for credentials (if we allow credentials at all).

This seems rather confusing. The options are AllowClientCrossOriginCredentials and BlockClientCrossOriginCredentials - I cannot easily connect those to whether we will ask the client.

[Nate Chapin]

Created attachment 108242 [details]
Remove didReceiveAuthenticationChallenge() - rename enum values

Addressed ap's comments on previous patch.

The new enum names are:
AskClientForCrossOriginCredentials
DoNotAskClientForCrossOriginCredentials

Hopefully that's clearer.

[Alexey Proskuryakov]

Comment on attachment 108242 [details]
Remove didReceiveAuthenticationChallenge() - rename enum values

r=me. I didn't review very closely this time, but looks like all comments were addressed.

[Nate Chapin]

Comment on attachment 108242 [details]
Remove didReceiveAuthenticationChallenge() - rename enum values

Thanks!

[WebKit Review Bot]

Comment on attachment 108242 [details]
Remove didReceiveAuthenticationChallenge() - rename enum values

Clearing flags on attachment: 108242

Committed r95768: <http://trac.webkit.org/changeset/95768>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.