Bug 160292 – [Fetch API] Activate credentials mode

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

[youenn fablet]

We should activate credentials mode for fetch API.
In this vein, we might want to retire progressively allowCredentials option, which should be replaced by a credential flag controlled by lower loading code.

[youenn fablet]

Created attachment 284779 [details]
Patch

[Alex Christensen]

Comment on attachment 284779 [details]
Patch

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

> Source/WebCore/loader/DocumentThreadableLoader.cpp:99
> +    m_options.setAllowCredentials((m_options.credentials == FetchOptions::Credentials::Include || (m_options.credentials == FetchOptions::Credentials::SameOrigin && m_sameOriginRequest)) ? AllowStoredCredentials : DoNotAllowStoredCredentials);

Why don't we do this where the ThreadableLoaderOptions is created?  It seems strange to create a ThreadableLoaderOptions, give it to the DocumentThreadableLoader, then compute the allowCredentials after we have copied it.

> Source/WebCore/loader/DocumentThreadableLoader.cpp:376
> +        newRequest.mutableResourceRequest().setAllowCookies(m_options.allowCredentials() == AllowStoredCredentials);

FYI, in the NSURLSession loader, if a request doesn't allow credentials, it uses a stateless NSURLSession that has no cookie storage or credential storage.

> Source/WebCore/loader/DocumentThreadableLoader.cpp:386
> +    // If credentials mode is omit, we should disable cookie sending.

omitted

> Source/WebCore/workers/WorkerScriptLoader.cpp:90
> +    // We should add an option to set credential mode.

FIXME?

[youenn fablet]

Thanks for the review.

(In reply to comment #2)
> Comment on attachment 284779 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=284779&action=review
> 
> > Source/WebCore/loader/DocumentThreadableLoader.cpp:99
> > +    m_options.setAllowCredentials((m_options.credentials == FetchOptions::Credentials::Include || (m_options.credentials == FetchOptions::Credentials::SameOrigin && m_sameOriginRequest)) ? AllowStoredCredentials : DoNotAllowStoredCredentials);
> 
> Why don't we do this where the ThreadableLoaderOptions is created?  It seems
> strange to create a ThreadableLoaderOptions, give it to the
> DocumentThreadableLoader, then compute the allowCredentials after we have
> copied it.

I think allowCredentials should be removed from ResourceLoaderOptions.
Loader clients should just think in terms of fetch parameters.
Ideally, allowCredentials should be replaced by a credentialFlag which would be set or not according fetch options.
Typically at ResourceLoader level.

The only potential issue is the case of DocumentThreadableLoader which stops ResourceLoader from redirecting in case of cross origin redirections.
Maybe that can be handled through fetch options.

> > Source/WebCore/loader/DocumentThreadableLoader.cpp:376
> > +        newRequest.mutableResourceRequest().setAllowCookies(m_options.allowCredentials() == AllowStoredCredentials);
> 
> FYI, in the NSURLSession loader, if a request doesn't allow credentials, it
> uses a stateless NSURLSession that has no cookie storage or credential
> storage.

That is a nice and consistent way of handling things.
Maybe this should be widespread to all ports.

Note though that CachedResourceLoader is checking whether allowCookies is set or not to evaluate whether resources can be reused or not.
That is one item, amongst others, that would call for a good rationalization of CachedResourceLoader.

> 
> > Source/WebCore/loader/DocumentThreadableLoader.cpp:386
> > +    // If credentials mode is omit, we should disable cookie sending.
> 
> omitted

I'll rewrite as: If credentials mode is 'Omit', we should...

> > Source/WebCore/workers/WorkerScriptLoader.cpp:90
> > +    // We should add an option to set credential mode.
> 
> FIXME?

Yes, I'll update accordingly.

[youenn fablet]

Created attachment 284862 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 284862 [details]
Patch for landing

Clearing flags on attachment: 284862

Committed r203900: <http://trac.webkit.org/changeset/203900>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.