Expose passwords to JavaScript in Credential Manager API

Expose passwords to JavaScript in Credential Manager API

This change implements the proposed change to the Credential Manager API
to directly expose passwords to JavaScript. It also deprecates the
existing PasswordCredential attributes and the ability to attach a
Credential to fetch.

See the corresponding GitHub issue and pull request for more details:
- https://github.com/w3c/webappsec-credential-management/issues/75
- https://github.com/w3c/webappsec-credential-management/pull/76

- Intent to Deprecate: Custom fetch for Credential Manager API:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/WPckvg8JO0U

- Intent to Implement and Ship: The password property of PasswordCredential:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/nT51eE7fWn4

BUG=718416
Review-Url: https://codereview.chromium.org/2852423002
Cr-Commit-Position: refs/heads/master@{#473576}
Committed: https://chromium.googlesource.com/chromium/src/+/0ee82f7e38627cd7382166e219f4ec72631c816a

[jdoerrie, 2017-05-02 15:54:43]

The CQ bit was checked by jdoerrie@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-02 15:55:12]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-02 16:18:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-02 16:18:51]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jdoerrie, 2017-05-03 09:13:06]

The CQ bit was checked by jdoerrie@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-03 09:13:21]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-03 10:31:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-03 10:31:08]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[jdoerrie, 2017-05-03 14:16:38]

The CQ bit was checked by jdoerrie@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-03 14:16:58]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-03 15:40:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-03 15:40:34]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jdoerrie, 2017-05-03 15:53:07]

The CQ bit was checked by jdoerrie@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-03 15:53:33]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-03 18:03:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-03 18:04:00]

Dry run: This issue passed the CQ dry run.

[jdoerrie, 2017-05-04 08:34:17]

Description was changed from

==========
First Attempt

BUG=
==========

to

==========
Expose passwords to JavaScript in Credential Manager API

This change implements the proposed change to the Credential Manager API to
directly expose passwords to JavaScript. It also deprecates the existing
PasswordCredential attributes and the ability to attach a Credential to fetch.

See the corresponding GitHub issue and pull request for more details:
- https://github.com/w3c/webappsec-credential-management/issues/75
- https://github.com/w3c/webappsec-credential-management/pull/76

BUG=
==========

[jdoerrie, 2017-05-04 08:35:17]

Description was changed from

==========
Expose passwords to JavaScript in Credential Manager API

This change implements the proposed change to the Credential Manager API to
directly expose passwords to JavaScript. It also deprecates the existing
PasswordCredential attributes and the ability to attach a Credential to fetch.

See the corresponding GitHub issue and pull request for more details:
- https://github.com/w3c/webappsec-credential-management/issues/75
- https://github.com/w3c/webappsec-credential-management/pull/76

BUG=
==========

to

==========
Expose passwords to JavaScript in Credential Manager API

This change implements the proposed change to the Credential Manager API
to directly expose passwords to JavaScript. It also deprecates the
existing PasswordCredential attributes and the ability to attach a
Credential to fetch.

See the corresponding GitHub issue and pull request for more details:
- https://github.com/w3c/webappsec-credential-management/issues/75
- https://github.com/w3c/webappsec-credential-management/pull/76

BUG=
==========

[jdoerrie, 2017-05-04 08:41:32]

jdoerrie@chromium.org changed reviewers:
+ mkwst@chromium.org, vasilii@chromium.org

[jdoerrie, 2017-05-04 08:41:34]

Hi Vasilii and Mike, please consider this a first draft of the upcoming change.
At this point I have no intentions to land this, given that the official intent
to ship is still missing. I am mostly interested in your opinion whether I am
heading in the right direction and what else would need to be done.

To me it seems this change can be done without touching any of the
implementation of CM API in //components/password_manager, but I would like
Vasilii to confirm this.

[Mike West, 2017-05-04 11:13:33]

Thanks for putting this together! It looks pretty reasonable...

Please add some layout test coverage for the new `password` attribute. You
should be able to adjust the existing `PasswordCredential` tests pretty easily.

https://codereview.chromium.org/2852423002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/Deprecation.cpp (right):

https://codereview.chromium.org/2852423002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/Deprecation.cpp:434: return
willBeRemoved("fetch.RequestInit.RequestCredentials.password", M62,
We'll want a friendlier deprecation message than this produces, pointing
developers to the new `.password` attribute. It's also going to require some
rework for developers who have invested in the `fetch()` workflow, so it's
probably worth talking to folks about putting together a short migration article
to which we can point developers, showing them a drop-in replacement for the
`fetch()` flow. That is, instead of:

```
c.idName = "id";
c.passwordName = "password";
c.additionalData = formData;
fetch(url, { credentials: c, method: "POST" });
```

they'd want something like:

```
formData.set("id", c.id);
formData.set("password", c.password);
fetch(url, { credentials: "include", body: formData, method: "POST" });
```

https://codereview.chromium.org/2852423002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/credentialmanager/PasswordCredential.h
(right):

https://codereview.chromium.org/2852423002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/credentialmanager/PasswordCredential.h:48:
void setPassword(const String&);
I don't think we need this; it's a readonly attribute, isn't it?

https://codereview.chromium.org/2852423002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/credentialmanager/PasswordCredential.idl
(right):

https://codereview.chromium.org/2852423002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/credentialmanager/PasswordCredential.idl:19:
attribute DOMString password;
`readonly`

https://codereview.chromium.org/2852423002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/fetch/FetchManager.cpp (right):

https://codereview.chromium.org/2852423002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/fetch/FetchManager.cpp:732:
UseCounter::kCredentialManagerCustomFetch);
I think it would be better to put this counter in the `RequestInit` constructor
instead. That would cover cases in which a developer constructed a `Request`
object separately from the call to `fetch()`. Since that will end up throwing in
the future, it's important to notify folks at the point we'd end up throwing.

[jdoerrie, 2017-05-04 13:57:59]

Description was changed from

==========
Expose passwords to JavaScript in Credential Manager API

This change implements the proposed change to the Credential Manager API
to directly expose passwords to JavaScript. It also deprecates the
existing PasswordCredential attributes and the ability to attach a
Credential to fetch.

See the corresponding GitHub issue and pull request for more details:
- https://github.com/w3c/webappsec-credential-management/issues/75
- https://github.com/w3c/webappsec-credential-management/pull/76

BUG=
==========

to

==========
Expose passwords to JavaScript in Credential Manager API

This change implements the proposed change to the Credential Manager API
to directly expose passwords to JavaScript. It also deprecates the
existing PasswordCredential attributes and the ability to attach a
Credential to fetch.

See the corresponding GitHub issue and pull request for more details:
- https://github.com/w3c/webappsec-credential-management/issues/75
- https://github.com/w3c/webappsec-credential-management/pull/76

BUG=718416
==========

[jdoerrie, 2017-05-04 14:22:49]

The CQ bit was checked by jdoerrie@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-04 14:23:07]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jdoerrie, 2017-05-04 14:24:11]

Thanks, Mike!

https://codereview.chromium.org/2852423002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/Deprecation.cpp (right):

https://codereview.chromium.org/2852423002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/Deprecation.cpp:434: return
willBeRemoved("fetch.RequestInit.RequestCredentials.password", M62,
On 2017/05/04 11:13:33, Mike West wrote:
> We'll want a friendlier deprecation message than this produces, pointing
> developers to the new `.password` attribute. It's also going to require some
> rework for developers who have invested in the `fetch()` workflow, so it's
> probably worth talking to folks about putting together a short migration
article
> to which we can point developers, showing them a drop-in replacement for the
> `fetch()` flow. That is, instead of:
> 
> ```
> c.idName = "id";
> c.passwordName = "password";
> c.additionalData = formData;
> fetch(url, { credentials: c, method: "POST" });
> ```
> 
> they'd want something like:
> 
> ```
> formData.set("id", c.id);
> formData.set("password", c.password);
> fetch(url, { credentials: "include", body: formData, method: "POST" });
> ```

Acknowledged.

https://codereview.chromium.org/2852423002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/credentialmanager/PasswordCredential.h
(right):

https://codereview.chromium.org/2852423002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/credentialmanager/PasswordCredential.h:48:
void setPassword(const String&);
On 2017/05/04 11:13:33, Mike West wrote:
> I don't think we need this; it's a readonly attribute, isn't it? 

I'm not quite sure, in the PR it's not:
https://github.com/w3c/webappsec-credential-management/pull/76/files#diff-117...
I implemented your comment regarding DOMString, but I didn't know that it's
supposed to be readonly. I suppose that makes sense, though.

https://codereview.chromium.org/2852423002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/credentialmanager/PasswordCredential.idl
(right):

https://codereview.chromium.org/2852423002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/credentialmanager/PasswordCredential.idl:19:
attribute DOMString password;
On 2017/05/04 11:13:33, Mike West wrote:
> `readonly`

See comment above.

https://codereview.chromium.org/2852423002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/fetch/FetchManager.cpp (right):

https://codereview.chromium.org/2852423002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/fetch/FetchManager.cpp:732:
UseCounter::kCredentialManagerCustomFetch);
On 2017/05/04 11:13:33, Mike West wrote:
> I think it would be better to put this counter in the `RequestInit`
constructor
> instead. That would cover cases in which a developer constructed a `Request`
> object separately from the call to `fetch()`. Since that will end up throwing
in
> the future, it's important to notify folks at the point we'd end up throwing.

Done.

[commit-bot: I haz the power, 2017-05-04 14:26:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-04 14:26:12]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[jdoerrie, 2017-05-04 14:56:55]

The CQ bit was checked by jdoerrie@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-04 14:57:08]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-04 17:00:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-04 17:00:54]

Dry run: This issue passed the CQ dry run.

[Mike West, 2017-05-05 11:35:10]

Thanks!

Would you mind updating
//LayoutTests/external/wpt/credential-management/idl.https.html as well?

https://codereview.chromium.org/2852423002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/credentialmanager/PasswordCredential.h
(right):

https://codereview.chromium.org/2852423002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/credentialmanager/PasswordCredential.h:48:
void setPassword(const String&);
On 2017/05/04 at 14:24:10, jdoerrie wrote:
> On 2017/05/04 11:13:33, Mike West wrote:
> > I don't think we need this; it's a readonly attribute, isn't it? 
> 
> I'm not quite sure, in the PR it's not:
https://github.com/w3c/webappsec-credential-management/pull/76/files#diff-117...
I implemented your comment regarding DOMString, but I didn't know that it's
supposed to be readonly. I suppose that makes sense, though.

I think the patch is incorrect, and added a comment to that effect. :)

[vasilii, 2017-05-05 15:36:06]

Provisionally LGTM

[jdoerrie, 2017-05-05 16:03:25]

The CQ bit was checked by jdoerrie@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-05 16:04:00]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-05 17:55:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-05 17:55:27]

Dry run: This issue passed the CQ dry run.

[Mike West, 2017-05-08 11:01:57]

Two more small comments. :)

Will you send out an intent to ship?

https://codereview.chromium.org/2852423002/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/external/wpt/credential-management/idl.https.html
(right):

https://codereview.chromium.org/2852423002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/credential-management/idl.https.html:17:
dictionary SiteBoundCredentialData : CredentialData {
We should have removed the `SiteBound` stuff in the other patch. Would you mind
spinning off a new patch to clean up this test?

https://codereview.chromium.org/2852423002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/credential-management/idl.https.html:68:
attribute CredentialBodyType? additionalData;
Please drop these three attributes from this test, as it will be upstreamed and
should reflect the state of the spec and not the state of Chrome.

[jdoerrie, 2017-05-08 14:03:17]

Thanks again, Mike. I will send out the intent to ship soon, but wanted to see
if there are any concerns in the blink-api-owners-discuss@ thread.

However, given that no-one raised any real objections yet, I suppose I could go
ahead and start the I2S, right?

https://codereview.chromium.org/2852423002/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/external/wpt/credential-management/idl.https.html
(right):

https://codereview.chromium.org/2852423002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/credential-management/idl.https.html:17:
dictionary SiteBoundCredentialData : CredentialData {
On 2017/05/08 11:01:57, Mike West wrote:
> We should have removed the `SiteBound` stuff in the other patch. Would you
mind
> spinning off a new patch to clean up this test?

Will do. I had another related question: In
third_party/WebKit/Source/modules/credentialmanager we use
LocallyStoredCredentialData.idl which links to the (dead)
https://w3c.github.io/webappsec/specs/credentialmanagement/#dictdef-locallyst....
Was this also part of the spec at some point and is now obsolete? It doesn't
appear in this file, so my current assumption is that it's mainly an
implementation detail at this point. Is this correct? If yes, should we keep it
or also delete it?

https://codereview.chromium.org/2852423002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/credential-management/idl.https.html:68:
attribute CredentialBodyType? additionalData;
On 2017/05/08 11:01:57, Mike West wrote:
> Please drop these three attributes from this test, as it will be upstreamed
and
> should reflect the state of the spec and not the state of Chrome.

Will do this as well. Currently I am getting a presubmit error after I removed
the attributes and the appropriate lines from the *-expected.txt file: 

** Presubmit ERRORS **
* The following files are passing testharness results without console error
messages, they should be removed:

//third_party/WebKit/LayoutTests/external/wpt/credential-management/idl.https-expected.txt
ERROR: found passing testharness results without console error messages.

From the looks of it it's complaining that there is no line with FAIL in that
file anymore.  The corresponding script
(https://codesearch.chromium.org/chromium/src/third_party/WebKit/Tools/Scripts...)
states the following:

"""Check if a LayoutTest expected file is a passing testharness result.

The intent of this script is to identify expected files that are passing
testharness.js results. Those files are not needed because the test
infrastructure will read the output of testharness.js tests if there is no
expected files."""

Is it fine to simply go ahead and remove the *-expected.txt file? I don't fully
understand this test framework yet, but removing the expectation file seems
wrong to me. What do you think?

[jdoerrie, 2017-05-08 15:21:07]

https://codereview.chromium.org/2852423002/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/external/wpt/credential-management/idl.https.html
(right):

https://codereview.chromium.org/2852423002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/credential-management/idl.https.html:17:
dictionary SiteBoundCredentialData : CredentialData {
On 2017/05/08 14:03:17, jdoerrie (slow this week) wrote:
> On 2017/05/08 11:01:57, Mike West wrote:
> > We should have removed the `SiteBound` stuff in the other patch. Would you
> mind
> > spinning off a new patch to clean up this test?
> 
> Will do. I had another related question: In
> third_party/WebKit/Source/modules/credentialmanager we use
> LocallyStoredCredentialData.idl which links to the (dead)
>
https://w3c.github.io/webappsec/specs/credentialmanagement/#dictdef-locallyst....
> Was this also part of the spec at some point and is now obsolete? It doesn't
> appear in this file, so my current assumption is that it's mainly an
> implementation detail at this point. Is this correct? If yes, should we keep
it
> or also delete it?

Nvm, I also deleted LocallyStoredCredentialData as part of the
SiteBoundCredentialData cleanup: https://codereview.chromium.org/2867643004/ I
will send that CL out for review by you when the bots are happy with it.

[jdoerrie, 2017-05-09 15:48:05]

Description was changed from

==========
Expose passwords to JavaScript in Credential Manager API

This change implements the proposed change to the Credential Manager API
to directly expose passwords to JavaScript. It also deprecates the
existing PasswordCredential attributes and the ability to attach a
Credential to fetch.

See the corresponding GitHub issue and pull request for more details:
- https://github.com/w3c/webappsec-credential-management/issues/75
- https://github.com/w3c/webappsec-credential-management/pull/76

BUG=718416
==========

to

==========
Expose passwords to JavaScript in Credential Manager API

This change implements the proposed change to the Credential Manager API
to directly expose passwords to JavaScript. It also deprecates the
existing PasswordCredential attributes and the ability to attach a
Credential to fetch.

See the corresponding GitHub issue and pull request for more details:
- https://github.com/w3c/webappsec-credential-management/issues/75
- https://github.com/w3c/webappsec-credential-management/pull/76

- Intent to Deprecate: Custom fetch for Credential Manager API:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/WPckvg8JO0U

- Intent to Implement and Ship: The password property of PasswordCredential:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/nT51eE7fWn4

BUG=718416
==========

[jdoerrie, 2017-05-09 15:48:05]

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

[jdoerrie, 2017-05-09 15:50:02]

I added the corresponding Intent threads to the CL description and added jochen@
as an API OWNER reviewer.

[jdoerrie, 2017-05-15 14:49:57]

The CQ bit was checked by jdoerrie@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-15 14:50:15]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-15 15:52:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-15 15:52:32]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Mike West, 2017-05-16 09:30:45]

https://codereview.chromium.org/2852423002/diff/140001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/platform/mac/virtual/stable/webexposed/global-interface-listing-expected.txt
(right):

https://codereview.chromium.org/2852423002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/platform/mac/virtual/stable/webexposed/global-interface-listing-expected.txt:3820:
getter password
The `//virtual/stable` tests won't change results until you drop the runtime
flag (or set it to "stable").

https://codereview.chromium.org/2852423002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/credentialmanager/PasswordCredential.cpp
(right):

https://codereview.chromium.org/2852423002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/credentialmanager/PasswordCredential.cpp:137:
return RuntimeEnabledFeatures::passwordCredentialPasswordEnabled()
I think you can safely change this to:

```
return
static_cast<PlatformPasswordCredential*>(platform_credential_.Get())->Password();
```

as the guard you put into the IDL file will prevent it from being called from
the web if it's not meant to be available.

(That is, you can just rename and move the existing `Password()` method)

https://codereview.chromium.org/2852423002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/credentialmanager/PasswordCredential.cpp:157:
params->append(passwordName(), password());
I don't think this is going to work, as the current code will prevent the
password from being sent out via `fetch()` unless the runtime flag is set. Doing
the change above should make this work again.

https://codereview.chromium.org/2852423002/diff/140001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/credentialmanager/PasswordCredentialTest.cpp
(right):

https://codereview.chromium.org/2852423002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/credentialmanager/PasswordCredentialTest.cpp:107:
if (RuntimeEnabledFeatures::passwordCredentialPasswordEnabled())
I'd drop these `if`s, as per the discussion above.

[jdoerrie (Google - do not use), 2017-05-16 10:08:00]

jdoerrie@google.com changed reviewers:
+ jdoerrie@google.com

[jdoerrie (Google - do not use), 2017-05-16 10:08:01]

Thanks, Mike! I left another comment where I would like to have your insight :)

https://codereview.chromium.org/2852423002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/credentialmanager/PasswordCredential.idl
(right):

https://codereview.chromium.org/2852423002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/credentialmanager/PasswordCredential.idl:18:
[DeprecateAs=CredentialManagerAdditionalData] attribute CredentialBodyType?
additionalData;
Ideally the deprecation messages would only appear when the experimental feature
is enabled, but I couldn't figure out how to do it in the IDL. Simply writing
[DeprecateAs=CredentialManagerAdditionalData,RuntimeEnabled=PasswordCredentialPassword]
wouldn't work, because then the whole method is only available in the
experimental context.

I thought about adding a conditional Deprecation message to the getter and
setter (similarly to the change in RequestInit.cpp), but I don't know how get
hold of the context then which I need to pass to the CountDeprecation message.

What is your opinion here?

[jdoerrie, 2017-05-17 09:50:20]

jdoerrie@chromium.org changed reviewers:
- jdoerrie@google.com

[jdoerrie, 2017-05-17 14:14:08]

The CQ bit was checked by jdoerrie@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-17 14:14:23]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-17 14:17:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-17 14:17:24]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[jdoerrie, 2017-05-17 14:56:56]

The CQ bit was checked by jdoerrie@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-17 14:57:29]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jdoerrie, 2017-05-17 15:39:22]

https://codereview.chromium.org/2852423002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/credentialmanager/PasswordCredential.cpp
(right):

https://codereview.chromium.org/2852423002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/credentialmanager/PasswordCredential.cpp:137:
return RuntimeEnabledFeatures::passwordCredentialPasswordEnabled()
On 2017/05/16 09:30:45, Mike West wrote:
> I think you can safely change this to:
> 
> ```
> return
>
static_cast<PlatformPasswordCredential*>(platform_credential_.Get())->Password();
> ```
> 
> as the guard you put into the IDL file will prevent it from being called from
> the web if it's not meant to be available.
> 
> (That is, you can just rename and move the existing `Password()` method)

Done.

https://codereview.chromium.org/2852423002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/credentialmanager/PasswordCredential.cpp:157:
params->append(passwordName(), password());
On 2017/05/16 09:30:45, Mike West wrote:
> I don't think this is going to work, as the current code will prevent the
> password from being sent out via `fetch()` unless the runtime flag is set.
Doing
> the change above should make this work again.

Acknowledged.

https://codereview.chromium.org/2852423002/diff/140001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/credentialmanager/PasswordCredentialTest.cpp
(right):

https://codereview.chromium.org/2852423002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/credentialmanager/PasswordCredentialTest.cpp:107:
if (RuntimeEnabledFeatures::passwordCredentialPasswordEnabled())
On 2017/05/16 09:30:45, Mike West wrote:
> I'd drop these `if`s, as per the discussion above.

Done.

[commit-bot: I haz the power, 2017-05-17 17:13:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-17 17:13:32]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jdoerrie, 2017-05-19 09:24:21]

The CQ bit was checked by jdoerrie@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-19 09:24:45]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-19 09:27:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-19 09:27:43]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Mike West, 2017-05-19 09:47:49]

One nit about the developer-facing messaging. We need to do better than the
current error messages. :)

https://codereview.chromium.org/2852423002/diff/220001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/credentialmanager/passwordcredential-fetch-expected.txt
(right):

https://codereview.chromium.org/2852423002/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/credentialmanager/passwordcredential-fetch-expected.txt:1:
CONSOLE WARNING: line 43: fetch.RequestInit.RequestCredentials.password is
deprecated and will be removed in M62, around October 2017. See
https://www.chromestatus.com/features/5689327799500800 for more details.
This is a fairly meaningless error message. Can we change it to something like
"Passing 'PasswordCredential' objects into 'fetch(..., { credentials: ... })' is
deprecated, and will be removed in M62, around October 2017. See
https://chromium.org/useful/document/on/the/wiki for migration suggestions." and
put up a wiki page detailing the migration?

https://codereview.chromium.org/2852423002/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/credentialmanager/passwordcredential-fetch-expected.txt:4:
CONSOLE WARNING: line 158: PasswordCredential.additionalData is deprecated and
will be removed in M62, around October 2017. See
https://www.chromestatus.com/features/5689327799500800 for more details.
Ditto for these.

[jdoerrie, 2017-05-19 09:49:00]

The CQ bit was checked by jdoerrie@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-19 09:49:22]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-19 09:52:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-19 09:52:35]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[jochen (gone - plz use gerrit), 2017-05-19 13:52:21]

API lgtm, please wait for Mike to approve as well

[jdoerrie, 2017-05-22 11:29:12]

The CQ bit was checked by jdoerrie@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-22 11:29:23]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jdoerrie, 2017-05-22 11:35:32]

https://codereview.chromium.org/2852423002/diff/220001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/credentialmanager/passwordcredential-fetch-expected.txt
(right):

https://codereview.chromium.org/2852423002/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/credentialmanager/passwordcredential-fetch-expected.txt:1:
CONSOLE WARNING: line 43: fetch.RequestInit.RequestCredentials.password is
deprecated and will be removed in M62, around October 2017. See
https://www.chromestatus.com/features/5689327799500800 for more details.
On 2017/05/19 09:47:49, Mike West wrote:
> This is a fairly meaningless error message. Can we change it to something like
> "Passing 'PasswordCredential' objects into 'fetch(..., { credentials: ... })'
is
> deprecated, and will be removed in M62, around October 2017. See
> https://chromium.org/useful/document/on/the/wiki for migration suggestions."
and
> put up a wiki page detailing the migration?

Done, I put up a guide at
https://www.chromium.org/developers/recent-changes-credential-management-api.

Shortly afterwards I got an email stating the guide is better placed at
https://developers.google.com/web/updates/.

That is a good point, but I don't think we should block this CL on a decision
where the guide should go, given that it is very easy to change this console
message later.

https://codereview.chromium.org/2852423002/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/credentialmanager/passwordcredential-fetch-expected.txt:4:
CONSOLE WARNING: line 158: PasswordCredential.additionalData is deprecated and
will be removed in M62, around October 2017. See
https://www.chromestatus.com/features/5689327799500800 for more details.
On 2017/05/19 09:47:49, Mike West wrote:
> Ditto for these.

Done.

https://codereview.chromium.org/2852423002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/Deprecation.cpp (right):

https://codereview.chromium.org/2852423002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/Deprecation.cpp:419: case
UseCounter::kCredentialManagerCustomFetch:
Should we just collapse this into one enum value to avoid showing the exact same
deprecation message up to 4 times?

[Mike West, 2017-05-22 14:37:13]

LGTM. If there's a better spot for the migration suggestions, we can move it
later. I agree that that doesn't need to block this CL.

Ditto for collapsing the deprecation warnings. I'm not convinced it's a great
idea to jumble them all together, but I'm totally willing to be convinced that
I'm wrong (in a separate CL). :)

[commit-bot: I haz the power, 2017-05-22 14:40:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-22 14:40:35]

Dry run: This issue passed the CQ dry run.

[jdoerrie, 2017-05-22 14:41:18]

The CQ bit was checked by jdoerrie@chromium.org

[jdoerrie, 2017-05-22 14:41:18]

The patchset sent to the CQ was uploaded after l-g-t-m from
vasilii@chromium.org, jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/2852423002/#ps260001
(title: "Console Message")

[commit-bot: I haz the power, 2017-05-22 14:41:37]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-22 14:47:35]

CQ is committing da patch.

Bot data: {"patchset_id": 260001, "attempt_start_ts": 1495464078156700,
"parent_rev": "875c54258b74113d3f22411fe14c8a24d363f6c2", "commit_rev":
"0ee82f7e38627cd7382166e219f4ec72631c816a"}

[commit-bot: I haz the power, 2017-05-22 14:47:51]

Description was changed from

==========
Expose passwords to JavaScript in Credential Manager API

This change implements the proposed change to the Credential Manager API
to directly expose passwords to JavaScript. It also deprecates the
existing PasswordCredential attributes and the ability to attach a
Credential to fetch.

See the corresponding GitHub issue and pull request for more details:
- https://github.com/w3c/webappsec-credential-management/issues/75
- https://github.com/w3c/webappsec-credential-management/pull/76

- Intent to Deprecate: Custom fetch for Credential Manager API:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/WPckvg8JO0U

- Intent to Implement and Ship: The password property of PasswordCredential:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/nT51eE7fWn4

BUG=718416
==========

to

==========
Expose passwords to JavaScript in Credential Manager API

This change implements the proposed change to the Credential Manager API
to directly expose passwords to JavaScript. It also deprecates the
existing PasswordCredential attributes and the ability to attach a
Credential to fetch.

See the corresponding GitHub issue and pull request for more details:
- https://github.com/w3c/webappsec-credential-management/issues/75
- https://github.com/w3c/webappsec-credential-management/pull/76

- Intent to Deprecate: Custom fetch for Credential Manager API:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/WPckvg8JO0U

- Intent to Implement and Ship: The password property of PasswordCredential:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/nT51eE7fWn4

BUG=718416

Review-Url: https://codereview.chromium.org/2852423002
Cr-Commit-Position: refs/heads/master@{#473576}
Committed:
https://chromium.googlesource.com/chromium/src/+/0ee82f7e38627cd7382166e219f4...
==========

[commit-bot: I haz the power, 2017-05-22 14:47:53]

Committed patchset #14 (id:260001) as
https://chromium.googlesource.com/chromium/src/+/0ee82f7e38627cd7382166e219f4...