Bug 160292 - [Fetch API] Activate credentials mode
Summary: [Fetch API] Activate credentials mode
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords:
Depends on:
Blocks: 151937
  Show dependency treegraph
 
Reported: 2016-07-28 06:25 PDT by youenn fablet
Modified: 2016-07-29 07:13 PDT (History)
3 users (show)

See Also:


Attachments
Patch (41.22 KB, patch)
2016-07-28 06:43 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (41.29 KB, patch)
2016-07-29 06:44 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2016-07-28 06:25:03 PDT
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.
Comment 1 youenn fablet 2016-07-28 06:43:11 PDT
Created attachment 284779 [details]
Patch
Comment 2 Alex Christensen 2016-07-28 10:28:54 PDT
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?
Comment 3 youenn fablet 2016-07-29 06:26:15 PDT
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.
Comment 4 youenn fablet 2016-07-29 06:44:00 PDT
Created attachment 284862 [details]
Patch for landing
Comment 5 WebKit Commit Bot 2016-07-29 07:13:18 PDT
Comment on attachment 284862 [details]
Patch for landing

Clearing flags on attachment: 284862

Committed r203900: <http://trac.webkit.org/changeset/203900>
Comment 6 WebKit Commit Bot 2016-07-29 07:13:22 PDT
All reviewed patches have been landed.  Closing bug.