CREDENTIAL: Rework the integration with Fetch (1/2)

CREDENTIAL: Introduce an "attached credential" to requests.

The spec has been updated after some discussion with Mozilla about the
interaction between PasswordCredential objects and Service Workers.
Details are at [1], the TL;DR is that we'll now pass PasswordCredential
objects in as the RequestInit dictionary 'credentials' member. This will
produce a Request object whose visible 'credentials' attribute is
'password', and which carries a hidden PasswordCredential around for
serialization when we hit the network.

The current patch does the first part of the work, refactoring the
integration between modules/credentialmanager and modules/fetch. It
stops when the WebURLRequest is handed over to //content for processing.
Currently, this means that the credentials are visible in the Request
body that's presented to Service Workers. This isn't where we want to
end up, but it maintains the status quo while presenting developers with
the new interface, so it seems like a good place for this first patch to
stop.

[1]: https://w3c.github.io/webappsec-credential-management/#fetch-integration

BUG=599597
Committed: https://crrev.com/5dc4a153d1ddc9b7083427fe2705cd1199795038
Cr-Commit-Position: refs/heads/master@{#385390}

[Mike West, 2016-03-31 19:45:38]

mkwst@chromium.org changed reviewers:
+ horo@chromium.org, philipj@opera.com, tyoshino@chromium.org

[Mike West, 2016-03-31 19:45:38]

Hello, horo@, philipj@, and tyoshino@! Y'all had excellent comments on
https://codereview.chromium.org/1446963002 way back when. I hope you have some
time to give me some feedback on this refactoring based on some spec changes.

Thanks!

-mike

[Mike West, 2016-04-01 11:49:01]

Second piece up at https://codereview.chromium.org/1847383003/#ps1.

[Mike West, 2016-04-01 12:25:47]

Description was changed from

==========
CREDENTIAL: Rework the integration with Fetch.

The spec has been updated after some discussion with Mozilla about the
interaction between PasswordCredential objects and Service Workers.
Details are at [1], the TL;DR is that we'll now pass PasswordCredential
objects in as the RequestInit dictionary 'credentials' member. This will
produce a Request object whose visible 'credentials' attribute is
'password', and which carries a hidden PasswordCredential around for
serialization when we hit the network.

The current patch does the first part of the work, refactoring the
integration between modules/credentialmanager and modules/fetch. It
stops when the WebURLRequest is handed over to //content for processing.
Currently, this means that the credentials are visible in the Request
body that's presented to Service Workers. This isn't where we want to
end up, but it maintains the status quo while presenting developers with
the new interface, so it seems like a good place for this first patch to
stop.

[1]: https://w3c.github.io/webappsec-credential-management/#fetch-integration

BUG=599597
==========

to

==========
CREDENTIAL: Introduce an "attached credential" to requests.

The spec has been updated after some discussion with Mozilla about the
interaction between PasswordCredential objects and Service Workers.
Details are at [1], the TL;DR is that we'll now pass PasswordCredential
objects in as the RequestInit dictionary 'credentials' member. This will
produce a Request object whose visible 'credentials' attribute is
'password', and which carries a hidden PasswordCredential around for
serialization when we hit the network.

The current patch does the first part of the work, refactoring the
integration between modules/credentialmanager and modules/fetch. It
stops when the WebURLRequest is handed over to //content for processing.
Currently, this means that the credentials are visible in the Request
body that's presented to Service Workers. This isn't where we want to
end up, but it maintains the status quo while presenting developers with
the new interface, so it seems like a good place for this first patch to
stop.

[1]: https://w3c.github.io/webappsec-credential-management/#fetch-integration

BUG=599597
==========

[Mike West, 2016-04-01 12:28:01]

mkwst@chromium.org changed reviewers:
+ jochen@chromium.org

[Mike West, 2016-04-01 12:28:01]

jochen@: Can you stamp the //content changes? They're small (3 fairly
uninteresting lines).

[jochen (gone - plz use gerrit), 2016-04-01 12:42:46]

lgtm

[horo, 2016-04-04 05:26:00]

https://codereview.chromium.org/1844053003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/credentialmanager/passwordcredential-fetch.html
(right):

https://codereview.chromium.org/1844053003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/credentialmanager/passwordcredential-fetch.html:39:

Please test req.clone().

var c = new PasswordCredential({
  id: 'id',
  password: 'pencil',
  name: 'name',
  iconURL: 'https://example.com/icon.png'
});

var req = new Request(
    "./resources/echo-post.php",
    { credentials: c, method: "POST" });
    
fetch(req.clone())
  .then(function(r) {return r.json();})
  .then(function(j) {
      assert_equals(j.username, 'id');
      assert_equals(j.password, 'pencil');
      return fetch(req);
    })
  .then(function(r) {return r.json();})
  .then(function(j) {
      assert_equals(j.username, 'id');
      assert_equals(j.password, 'pencil');
    });

https://codereview.chromium.org/1844053003/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/fetch/Request.cpp (right):

https://codereview.chromium.org/1844053003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/fetch/Request.cpp:226:
request->setRedirect(WebURLRequest::FetchRedirectModeManual);
Please add layout tests to check the manual redirect works well.

1. The server returns a redirect response to same origin.
2. The server returns a redirect response to other origin.

In SW patch, https://codereview.chromium.org/1847383003/

1. e.respondWith(Response.redirect("/same_origin_url"))
2. e.respondWith(Response.redirect("//other_origin_url"))
3. e.respondWith(fetch("/same_origin_url_to_redirect_to_same_origin"));
4. e.respondWith(fetch("/same_origin_url_to_redirect_to_other_origin"));
5. e.respondWith(fetch("//other_origin_url_to_redirect_to_same_origin"));
6. e.respondWith(fetch("//other_origin_url_to_redirect_to_other_origin"));

https://codereview.chromium.org/1844053003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/network/ResourceRequest.cpp (right):

https://codereview.chromium.org/1844053003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/network/ResourceRequest.cpp:339: bool
ResourceRequest::compare(const ResourceRequest& a, const ResourceRequest& b)
There is no code which compares ResourceRequests.
We can remove ResourceRequest::compare() and operator==(const ResourceRequest&
a, const ResourceRequest& b) and operator!=(ResourceRequest& a, const
ResourceRequest& b).

[Mike West, 2016-04-04 08:04:20]

Thank you! I think this patchset addresses your concerns.

https://codereview.chromium.org/1844053003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/credentialmanager/passwordcredential-fetch.html
(right):

https://codereview.chromium.org/1844053003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/credentialmanager/passwordcredential-fetch.html:39:

On 2016/04/04 at 05:26:00, horo wrote:
> Please test req.clone().
> 
> var c = new PasswordCredential({
>   id: 'id',
>   password: 'pencil',
>   name: 'name',
>   iconURL: 'https://example.com/icon.png'
> });
> 
> var req = new Request(
>     "./resources/echo-post.php",
>     { credentials: c, method: "POST" });
>     
> fetch(req.clone())
>   .then(function(r) {return r.json();})
>   .then(function(j) {
>       assert_equals(j.username, 'id');
>       assert_equals(j.password, 'pencil');
>       return fetch(req);
>     })
>   .then(function(r) {return r.json();})
>   .then(function(j) {
>       assert_equals(j.username, 'id');
>       assert_equals(j.password, 'pencil');
>     });

Done, thanks!

https://codereview.chromium.org/1844053003/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/fetch/Request.cpp (right):

https://codereview.chromium.org/1844053003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/fetch/Request.cpp:226:
request->setRedirect(WebURLRequest::FetchRedirectModeManual);
On 2016/04/04 at 05:26:00, horo wrote:
> Please add layout tests to check the manual redirect works well.
> 
> 1. The server returns a redirect response to same origin.
> 2. The server returns a redirect response to other origin.

Done. I also drove-by a small fix to set the opaque-redirect response's URL,
which wasn't being set in the current code (as the Fetch spec notes, it's safe
to do so, because we don't actually follow the redirect, so the URL will be the
value the page actually requested).

https://codereview.chromium.org/1844053003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/network/ResourceRequest.cpp (right):

https://codereview.chromium.org/1844053003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/network/ResourceRequest.cpp:339: bool
ResourceRequest::compare(const ResourceRequest& a, const ResourceRequest& b)
On 2016/04/04 at 05:26:00, horo wrote:
> There is no code which compares ResourceRequests.
> We can remove ResourceRequest::compare() and operator==(const ResourceRequest&
a, const ResourceRequest& b) and operator!=(ResourceRequest& a, const
ResourceRequest& b).

I've added a TODO, and I'll take a look in a separate patch.

[horo, 2016-04-04 08:54:04]

https://codereview.chromium.org/1844053003/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/credentialmanager/passwordcredential-fetch.html
(right):

https://codereview.chromium.org/1844053003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/credentialmanager/passwordcredential-fetch.html:20:
clone.text().then(t => assert_equals(t, ""));
You have to return the promise.
return clone.text().then(t => assert_equals(t, ""));

https://codereview.chromium.org/1844053003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/credentialmanager/passwordcredential-fetch.html:44:
fetch(clone)
return fetch(clone)

I don't know why, but this test is falling.
I think fetch(new Request('./resources/echo-post.php', { credentials: c,
method:"POST" })) doesn't send the credential.

[Mike West, 2016-04-04 10:11:14]

Patchset #3 (id:40001) has been deleted

[Mike West, 2016-04-04 10:12:12]

https://codereview.chromium.org/1844053003/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/credentialmanager/passwordcredential-fetch.html
(right):

https://codereview.chromium.org/1844053003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/credentialmanager/passwordcredential-fetch.html:20:
clone.text().then(t => assert_equals(t, ""));
On 2016/04/04 at 08:54:04, horo wrote:
> You have to return the promise.
> return clone.text().then(t => assert_equals(t, ""));

Fixed, thank you.

https://codereview.chromium.org/1844053003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/credentialmanager/passwordcredential-fetch.html:44:
fetch(clone)
On 2016/04/04 at 08:54:04, horo wrote:
> return fetch(clone)
> 
> I don't know why, but this test is falling.
> I think fetch(new Request('./resources/echo-post.php', { credentials: c,
method:"POST" })) doesn't send the credential.

I split this into two tests, one with `fetch(new Request)` and one with a cloned
request. Both pass locally... let's see if the bots agree.

[Mike West, 2016-04-04 12:59:04]

On 2016/04/04 at 10:12:12, Mike West (OOO through 31st) wrote:
>
https://codereview.chromium.org/1844053003/diff/20001/third_party/WebKit/Layo...
> File
third_party/WebKit/LayoutTests/http/tests/credentialmanager/passwordcredential-fetch.html
(right):
> 
>
https://codereview.chromium.org/1844053003/diff/20001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/http/tests/credentialmanager/passwordcredential-fetch.html:20:
clone.text().then(t => assert_equals(t, ""));
> On 2016/04/04 at 08:54:04, horo wrote:
> > You have to return the promise.
> > return clone.text().then(t => assert_equals(t, ""));
> 
> Fixed, thank you.
> 
>
https://codereview.chromium.org/1844053003/diff/20001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/http/tests/credentialmanager/passwordcredential-fetch.html:44:
fetch(clone)
> On 2016/04/04 at 08:54:04, horo wrote:
> > return fetch(clone)
> > 
> > I don't know why, but this test is falling.
> > I think fetch(new Request('./resources/echo-post.php', { credentials: c,
method:"POST" })) doesn't send the credential.
> 
> I split this into two tests, one with `fetch(new Request)` and one with a
cloned request. Both pass locally... let's see if the bots agree.

These pass locally, and now on the bots as well. But now
`passwordcredential-redirect.html` is failing on linux (but not Mac or Windows).
It, of course, passes locally. Wonderful.

[horo, 2016-04-04 14:13:04]

https://codereview.chromium.org/1844053003/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/credentialmanager/passwordcredential-fetch.html
(right):

https://codereview.chromium.org/1844053003/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/credentialmanager/passwordcredential-fetch.html:44:
fetch(clone)
You have to return the promise.
Line 44 must be "return fetch(clone)"

[Mike West, 2016-04-05 04:16:09]

On 2016/04/04 at 14:13:04, horo wrote:
>
https://codereview.chromium.org/1844053003/diff/80001/third_party/WebKit/Layo...
> File
third_party/WebKit/LayoutTests/http/tests/credentialmanager/passwordcredential-fetch.html
(right):
> 
>
https://codereview.chromium.org/1844053003/diff/80001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/http/tests/credentialmanager/passwordcredential-fetch.html:44:
fetch(clone)
> You have to return the promise.
> Line 44 must be "return fetch(clone)"

Yup. I'll fix that. That isn't the test that's failing on linux for no reason
that I can discern, however. I don't understand what's going on there... I'll
keep poking at it today.

Does the patch look good to you other than the failing bots? I'd like to get
this into M51 with a little time to bake, so it would be great if I could land
it once I figure out the bot issue. :)

[horo, 2016-04-05 07:24:36]

What should happen when credentials is overridden?
var c = new PasswordCredential({id: 'id', password: 'pass'});
var r1 = new Request("./resources/echo-post.php", { credentials: c, method:
"POST" });
var r2 = new Request(r1, { credentials: "same-origin" });
Does fetch(r2) contain the credentials?

https://codereview.chromium.org/1844053003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/fetch/FetchRequestData.h (right):

https://codereview.chromium.org/1844053003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/fetch/FetchRequestData.h:113:
Member<BodyStreamBuffer> m_attachedCredentialBuffer;
BodyStreamBuffer is designed to be readable only once.
For example this code throws an error.

var r1 = new Request("./resources/echo-post.php", {method: 'POST', body:
'hello'});
var r2 = new Request(r1);
fetch(r1)  // This throws TypeError.

What is the expected behavior of this code?

var c = new PasswordCredential({id: 'id', password: 'pass'});
var r1 = new Request("./resources/echo-post.php", { credentials: c, method:
"POST" });
var r2 = new Request(r1);
fetch(r1)  // Does this fetch contains the credentials?

If fetch(r1) must contain the credentials, you should not use BodyStreamBuffer
for credentials.
You can have the credentials as a EncodedFormData.


If you want to use a BodyStreamBuffer for the credentials in FetchRequestData,
you have to treat it same as the body.

From FetchRequestData::cloneExceptBody() remove:
> request->m_attachedCredentialBuffer = m_attachedCredentialBuffer;

In FetchRequestData::clone():
> if (m_attachedCredentialBuffer) {
>     OwnPtr<FetchDataConsumerHandle> dest1, dest2;
>     DataConsumerTee::create(executionContext,
m_attachedCredentialBuffer->releaseHandle(executionContext), &dest1, &dest2);
>     m_attachedCredentialBuffer = new BodyStreamBuffer(dest1.release());
>     request->m_attachedCredentialBuffer = new
BodyStreamBuffer(dest2.release());
> }

In FetchRequestData::pass():
> if (m_attachedCredentialBuffer) {
>     request->m_attachedCredentialBuffer = m_attachedCredentialBuffer;
>     m_attachedCredentialBuffer = new
BodyStreamBuffer(createFetchDataConsumerHandleFromWebHandle(createDoneDataConsumerHandle()));
>     m_attachedCredentialBuffer->stream()->setIsDisturbed();
> }


In Request::createRequestWithRequestOrString():
> if (inputRequest && inputRequest->attachedCredentialBuffer()) {
>    
r->m_request->setAttachedCredentialBuffer(inputRequest->attachedCredentialBuffer());
>     inputRequest->m_request->setAttachedCredentialBuffer(new
BodyStreamBuffer(createFetchDataConsumerHandleFromWebHandle(createDoneDataConsumerHandle())));
>     inputRequest->attachedCredentialBuffer()->stream()->setIsDisturbed();
> }

https://codereview.chromium.org/1844053003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/fetch/Request.cpp (right):

https://codereview.chromium.org/1844053003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/fetch/Request.cpp:369: 
I think you have to move attachedCredentialBuffer here.

    if (inputRequest && inputRequest->attachedCredentialBuffer()) {
       
r->m_request->setAttachedCredentialBuffer(inputRequest->attachedCredentialBuffer());
        inputRequest->m_request->setAttachedCredentialBuffer(new
BodyStreamBuffer(createFetchDataConsumerHandleFromWebHandle(createDoneDataConsumerHandle())));
        inputRequest->attachedCredentialBuffer()->stream()->setIsDisturbed();
    }

[horo, 2016-04-05 07:25:40]

horo@chromium.org changed reviewers:
+ yhirano@chromium.org

[horo, 2016-04-05 07:25:41]

+yhirano@
as a Stream master :)

[Mike West, 2016-04-05 10:20:56]

Thank you, horo@. This is a lot more complicated than I thought, and I
appreciate your thorough review. :)

I hadn't considered copying the request, but I think it makes sense to treat
this as an re-readable object rather than a read-once buffer. That matches the
way that I think about the feature (`PasswordCredential` object attached to a
`Request`), and seems straightforward to implement.

I've switched the implementation to store a `RefPtr<EncodedFormData>` rather
than a buffer, and added tests to verify the behavior of copied/cloned
`Request`s.

https://codereview.chromium.org/1844053003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/fetch/Request.cpp (right):

https://codereview.chromium.org/1844053003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/fetch/Request.cpp:369: 
On 2016/04/05 at 07:24:35, horo wrote:
> I think you have to move attachedCredentialBuffer here.
> 
>     if (inputRequest && inputRequest->attachedCredentialBuffer()) {
>        
r->m_request->setAttachedCredentialBuffer(inputRequest->attachedCredentialBuffer());
>         inputRequest->m_request->setAttachedCredentialBuffer(new
BodyStreamBuffer(createFetchDataConsumerHandleFromWebHandle(createDoneDataConsumerHandle())));
>         inputRequest->attachedCredentialBuffer()->stream()->setIsDisturbed();
>     }

Took care of this in `createCopyOfFetchRequestDataForFetch` (which is simplified
by switching to an EncodedFormData, as you suggested).

[Mike West, 2016-04-05 13:03:20]

On 2016/04/05 at 04:16:09, Mike West (OOO through 31st) wrote:
> Does the patch look good to you other than the failing bots? I'd like to get
this into M51 with a little time to bake, so it would be great if I could land
it once I figure out the bot issue. :)

After _WAY_ too long, it turns out that PHP on the trybots doesn't support
`http_response_code()` yet. *facepalm* I should have realized this, since I
fought with https://crbug.com/596425 just ~2 weeks ago. Bleh.

[horo, 2016-04-06 03:39:48]

https://codereview.chromium.org/1844053003/diff/140001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/credentialmanager/passwordcredential-fetch.html
(right):

https://codereview.chromium.org/1844053003/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/credentialmanager/passwordcredential-fetch.html:19:
r.text().then(t => assert_equals(t, ""));
Line 19 20  must be:
    return Promise.all([
      r.text().then(t => assert_equals(t, "")),
      clone.text().then(t => assert_equals(t, ""))
    ])

https://codereview.chromium.org/1844053003/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/credentialmanager/passwordcredential-fetch.html:50:
.then(_ => {
Line 50-57:
        .then(_ => fetch(r2))
        .then(resp => resp.json())
        .then(j => {
            assert_equals(j.username, 'id');
            assert_equals(j.password, 'pencil');
        });

https://codereview.chromium.org/1844053003/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/credentialmanager/passwordcredential-fetch.html:69:
.then(_ => {
ditto.

https://codereview.chromium.org/1844053003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/FetchRequestData.h (right):

https://codereview.chromium.org/1844053003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/FetchRequestData.h:61: void
setCredentials(WebURLRequest::FetchCredentialsMode credentials) { m_credentials
= credentials; }
|m_attachedCredential| must be cleared when |credentials| is not
FetchCredentialsModePassword.

Otherwise this test fails.

promise_test(_ => {
    var r1 = new Request('./resources/echo-post.php', { credentials: c, method:
'POST' });
    return fetch(new Request(r1, { credentials: 'same-origin' }))
        .then(resp => resp.text())
        .then(t => assert_equals(t, ""));
}, "Override credentials");

[Mike West, 2016-04-06 05:43:13]

Thanks!

https://codereview.chromium.org/1844053003/diff/140001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/credentialmanager/passwordcredential-fetch.html
(right):

https://codereview.chromium.org/1844053003/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/credentialmanager/passwordcredential-fetch.html:19:
r.text().then(t => assert_equals(t, ""));
On 2016/04/06 at 03:39:48, horo wrote:
> Line 19 20  must be:
>     return Promise.all([
>       r.text().then(t => assert_equals(t, "")),
>       clone.text().then(t => assert_equals(t, ""))
>     ])

Thanks!

https://codereview.chromium.org/1844053003/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/credentialmanager/passwordcredential-fetch.html:50:
.then(_ => {
On 2016/04/06 at 03:39:48, horo wrote:
> Line 50-57:
>         .then(_ => fetch(r2))
>         .then(resp => resp.json())
>         .then(j => {
>             assert_equals(j.username, 'id');
>             assert_equals(j.password, 'pencil');
>         });

Done.

https://codereview.chromium.org/1844053003/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/credentialmanager/passwordcredential-fetch.html:69:
.then(_ => {
On 2016/04/06 at 03:39:48, horo wrote:
> ditto.

Done.

https://codereview.chromium.org/1844053003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/FetchRequestData.h (right):

https://codereview.chromium.org/1844053003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/FetchRequestData.h:61: void
setCredentials(WebURLRequest::FetchCredentialsMode credentials) { m_credentials
= credentials; }
On 2016/04/06 at 03:39:48, horo wrote:
> |m_attachedCredential| must be cleared when |credentials| is not
FetchCredentialsModePassword.
> 
> Otherwise this test fails.
> 
> promise_test(_ => {
>     var r1 = new Request('./resources/echo-post.php', { credentials: c,
method: 'POST' });
>     return fetch(new Request(r1, { credentials: 'same-origin' }))
>         .then(resp => resp.text())
>         .then(t => assert_equals(t, ""));
> }, "Override credentials");

Good call. Added this test, and cleared the attached credentials.

[horo, 2016-04-06 06:17:47]

lgtm

[Mike West, 2016-04-06 06:54:28]

The CQ bit was checked by mkwst@chromium.org

[Mike West, 2016-04-06 06:54:28]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/1844053003/#ps160001
(title: "clear attachedcredentials")

[commit-bot: I haz the power, 2016-04-06 06:54:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1844053003/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1844053003/160001

[commit-bot: I haz the power, 2016-04-06 07:07:55]

Committed patchset #8 (id:160001)

[commit-bot: I haz the power, 2016-04-06 07:08:58]

Description was changed from

==========
CREDENTIAL: Introduce an "attached credential" to requests.

The spec has been updated after some discussion with Mozilla about the
interaction between PasswordCredential objects and Service Workers.
Details are at [1], the TL;DR is that we'll now pass PasswordCredential
objects in as the RequestInit dictionary 'credentials' member. This will
produce a Request object whose visible 'credentials' attribute is
'password', and which carries a hidden PasswordCredential around for
serialization when we hit the network.

The current patch does the first part of the work, refactoring the
integration between modules/credentialmanager and modules/fetch. It
stops when the WebURLRequest is handed over to //content for processing.
Currently, this means that the credentials are visible in the Request
body that's presented to Service Workers. This isn't where we want to
end up, but it maintains the status quo while presenting developers with
the new interface, so it seems like a good place for this first patch to
stop.

[1]: https://w3c.github.io/webappsec-credential-management/#fetch-integration

BUG=599597
==========

to

==========
CREDENTIAL: Introduce an "attached credential" to requests.

The spec has been updated after some discussion with Mozilla about the
interaction between PasswordCredential objects and Service Workers.
Details are at [1], the TL;DR is that we'll now pass PasswordCredential
objects in as the RequestInit dictionary 'credentials' member. This will
produce a Request object whose visible 'credentials' attribute is
'password', and which carries a hidden PasswordCredential around for
serialization when we hit the network.

The current patch does the first part of the work, refactoring the
integration between modules/credentialmanager and modules/fetch. It
stops when the WebURLRequest is handed over to //content for processing.
Currently, this means that the credentials are visible in the Request
body that's presented to Service Workers. This isn't where we want to
end up, but it maintains the status quo while presenting developers with
the new interface, so it seems like a good place for this first patch to
stop.

[1]: https://w3c.github.io/webappsec-credential-management/#fetch-integration

BUG=599597

Committed: https://crrev.com/5dc4a153d1ddc9b7083427fe2705cd1199795038
Cr-Commit-Position: refs/heads/master@{#385390}
==========

[commit-bot: I haz the power, 2016-04-06 07:09:00]

Patchset 8 (id:??) landed as
https://crrev.com/5dc4a153d1ddc9b7083427fe2705cd1199795038
Cr-Commit-Position: refs/heads/master@{#385390}

[tyoshino (SeeGerritForStatus), 2016-04-07 15:09:55]

I'll try to take a look this and the 2/2 tomorrow. Sorry for delay.

[tyoshino (SeeGerritForStatus), 2016-04-12 16:25:28]

lgtm