Update CM API IDL to use SecureContext

Update CM API IDL to use SecureContext

This change implements recent changes to the Credential Manager API
specification that introduce the |SecureContext| extended attribute on
its interfaces. Furthermore it removes
|RuntimeEnabled=CredentialManager| from the interfaces following the
discussion in http://crrev.com/2832813002.

Intent to implement and ship:
https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/-E-O9LgsPfM/ExJ7hiY1BQAJ

BUG=715077
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2844453002
Cr-Commit-Position: refs/heads/master@{#468596}
Committed: https://chromium.googlesource.com/chromium/src/+/44ca0605b598508b94a2cf0ccb4d37b8023ef3fb

[jdoerrie, 2017-04-25 11:01:16]

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

[commit-bot: I haz the power, 2017-04-25 11:01:48]

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-04-25 12:19:28]

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

[commit-bot: I haz the power, 2017-04-25 12:19:29]

Dry run: Try jobs failed on following builders:
  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-04-25 12:41:23]

Description was changed from

==========
Update CM API idl to reflect spec changes

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

to

==========
Update CM API to use SecureContext

This change implements recent changes to the Credential Manager API
specification that introduce the |SecureContext| extended attribute on its
interfaces. Furthermore it removes |RuntimeEnabled=CredentialManager| from the
interfaces following the discussion in http://crrev.com/2832813002.

BUG=715077
==========

[jdoerrie, 2017-04-25 12:41:24]

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

[jdoerrie, 2017-04-25 12:42:07]

Description was changed from

==========
Update CM API to use SecureContext

This change implements recent changes to the Credential Manager API
specification that introduce the |SecureContext| extended attribute on its
interfaces. Furthermore it removes |RuntimeEnabled=CredentialManager| from the
interfaces following the discussion in http://crrev.com/2832813002.

BUG=715077
==========

to

==========
Update CM API IDL to use SecureContext

This change implements recent changes to the Credential Manager API
specification that introduce the |SecureContext| extended attribute on
its interfaces. Furthermore it removes
|RuntimeEnabled=CredentialManager| from the interfaces following the
discussion in http://crrev.com/2832813002.

BUG=715077
==========

[Mike West, 2017-04-25 18:39:45]

Thanks! I left some feedback inline. Also, would you mind linking to battre@'s
intent thread in the CL description?
https://groups.google.com/a/chromium.org/d/msg/blink-dev/-E-O9LgsPfM/ExJ7hiY1...

https://codereview.chromium.org/2844453002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/credentialmanager/CredentialUserData.idl
(right):

https://codereview.chromium.org/2844453002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/credentialmanager/CredentialUserData.idl:7:
[NoInterfaceObject, SecureContext]
This is actually a spec bug; `SecureContext` doesn't have any meaning here
because of `NoInterfaceObject`. Let's drop it. I'll drop it from the spec.

https://codereview.chromium.org/2844453002/diff/1/third_party/WebKit/Source/m...
File
third_party/WebKit/Source/modules/credentialmanager/NavigatorCredentials.idl
(right):

https://codereview.chromium.org/2844453002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/credentialmanager/NavigatorCredentials.idl:7:
readonly attribute CredentialsContainer credentials;
Please put the attributes on the attribute, not the interface. That is:

```
[SecureContext, SameObject] readonly attribute ...;
```

[jdoerrie, 2017-04-26 11:00:18]

Description was changed from

==========
Update CM API IDL to use SecureContext

This change implements recent changes to the Credential Manager API
specification that introduce the |SecureContext| extended attribute on
its interfaces. Furthermore it removes
|RuntimeEnabled=CredentialManager| from the interfaces following the
discussion in http://crrev.com/2832813002.

BUG=715077
==========

to

==========
Update CM API IDL to use SecureContext

This change implements recent changes to the Credential Manager API
specification that introduce the |SecureContext| extended attribute on
its interfaces. Furthermore it removes
|RuntimeEnabled=CredentialManager| from the interfaces following the
discussion in http://crrev.com/2832813002.

Intent to implement and ship:
https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/-E-O9LgsPfM/Ex...

BUG=715077
==========

[jdoerrie, 2017-04-26 12:09:43]

Description was changed from

==========
Update CM API IDL to use SecureContext

This change implements recent changes to the Credential Manager API
specification that introduce the |SecureContext| extended attribute on
its interfaces. Furthermore it removes
|RuntimeEnabled=CredentialManager| from the interfaces following the
discussion in http://crrev.com/2832813002.

Intent to implement and ship:
https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/-E-O9LgsPfM/Ex...

BUG=715077
==========

to

==========
Update CM API IDL to use SecureContext

This change implements recent changes to the Credential Manager API
specification that introduce the |SecureContext| extended attribute on
its interfaces. Furthermore it removes
|RuntimeEnabled=CredentialManager| from the interfaces following the
discussion in http://crrev.com/2832813002.

Intent to implement and ship:
https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/-E-O9LgsPfM/Ex...

BUG=715077
==========

[jdoerrie, 2017-04-26 12:20:35]

Dear all, please review. This CL causes the
http/tests/credentialmanager/credentialscontainer-frame-errors.html test to
reliably time out
(https://codesearch.chromium.org/chromium/src/third_party/WebKit/LayoutTests/h...),
which puzzles me. When trying to debug this locally I only encounter a
net::ERR_NAME_NOT_RESOLVED:

"example.test’s server DNS address could not be found."

This test passes locally when commenting out
|async_test(create_iframe_test("http://example.test:8000"), "Cross-origin IFrame
throws.");|

Any insights to what is going on here?

https://codereview.chromium.org/2844453002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/credentialmanager/CredentialUserData.idl
(right):

https://codereview.chromium.org/2844453002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/credentialmanager/CredentialUserData.idl:7:
[NoInterfaceObject, SecureContext]
On 2017/04/25 18:39:45, Mike West wrote:
> This is actually a spec bug; `SecureContext` doesn't have any meaning here
> because of `NoInterfaceObject`. Let's drop it. I'll drop it from the spec.

Done.

https://codereview.chromium.org/2844453002/diff/1/third_party/WebKit/Source/m...
File
third_party/WebKit/Source/modules/credentialmanager/NavigatorCredentials.idl
(right):

https://codereview.chromium.org/2844453002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/credentialmanager/NavigatorCredentials.idl:7:
readonly attribute CredentialsContainer credentials;
On 2017/04/25 18:39:45, Mike West wrote:
> Please put the attributes on the attribute, not the interface. That is:
> 
> ```
> [SecureContext, SameObject] readonly attribute ...;
> ```

Done.

https://codereview.chromium.org/2844453002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/WebRuntimeFeatures.cpp (left):

https://codereview.chromium.org/2844453002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebRuntimeFeatures.cpp:302:
RuntimeEnabledFeatures::setCredentialManagerEnabled(enable);
Dropping the entry in RuntimeEnabledFeatures.json5 resulted in
setCredentialManagerEnabled not being generated anymore, which is why I had to
remove this method and also the corresponding line in runtime_features.cc.

Looking at Mike's CL enabling CM API by default also other files were changed:
http://crrev.com/1720403003
That CL's description states that the "feature flag can be removed after ~a
release", which did not seem to happen yet. Should I go ahead and remove this
flag also in this CL?

[vasilii, 2017-04-26 17:44:09]

Probably it can't understand if example.test:8000 is secure origin or not. I
think the test should be rewritten.

https://codereview.chromium.org/2844453002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/WebRuntimeFeatures.cpp (left):

https://codereview.chromium.org/2844453002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebRuntimeFeatures.cpp:302:
RuntimeEnabledFeatures::setCredentialManagerEnabled(enable);
On 2017/04/26 12:20:35, jdoerrie wrote:
> Dropping the entry in RuntimeEnabledFeatures.json5 resulted in
> setCredentialManagerEnabled not being generated anymore, which is why I had to
> remove this method and also the corresponding line in runtime_features.cc.
> 
> Looking at Mike's CL enabling CM API by default also other files were changed:
> http://crrev.com/1720403003
> That CL's description states that the "feature flag can be removed after ~a
> release", which did not seem to happen yet. Should I go ahead and remove this
> flag also in this CL?

yes.

[Mike West, 2017-04-27 11:18:29]

On 2017/04/26 at 12:20:35, jdoerrie wrote:
> Dear all, please review. This CL causes the
http/tests/credentialmanager/credentialscontainer-frame-errors.html test to
reliably time out
(https://codesearch.chromium.org/chromium/src/third_party/WebKit/LayoutTests/h...),
which puzzles me. When trying to debug this locally I only encounter a
net::ERR_NAME_NOT_RESOLVED:
> 
> "example.test’s server DNS address could not be found."

This relies on `content_shell --run-layout-test`'s DNS resolver, which resolves
`*.test` to the loopback address. If you want it to work in a non-layout-test
mode, you could add `example.test` to your `/etc/hosts` file.
 
> This test passes locally when commenting out
> |async_test(create_iframe_test("http://example.test:8000"), "Cross-origin
IFrame throws.");|
> 
> Any insights to what is going on here?

You'll need to change the test to load from `https://example.test:8443`, as
`http://example.test:8000` is not a secure context, so `navigator.credentials`
no longer exists, so `iframed-credentialscontainer.html` throws an error rather
than posting a message back to the parent frame.

[jdoerrie, 2017-04-27 13:27:37]

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

[commit-bot: I haz the power, 2017-04-27 13:27:54]

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-04-27 15:10:47]

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

[commit-bot: I haz the power, 2017-04-27 15:10:48]

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

[jdoerrie, 2017-04-27 16:23:08]

Description was changed from

==========
Update CM API IDL to use SecureContext

This change implements recent changes to the Credential Manager API
specification that introduce the |SecureContext| extended attribute on
its interfaces. Furthermore it removes
|RuntimeEnabled=CredentialManager| from the interfaces following the
discussion in http://crrev.com/2832813002.

Intent to implement and ship:
https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/-E-O9LgsPfM/Ex...

BUG=715077
==========

to

==========
Update CM API IDL to use SecureContext

This change implements recent changes to the Credential Manager API
specification that introduce the |SecureContext| extended attribute on
its interfaces. Furthermore it removes
|RuntimeEnabled=CredentialManager| from the interfaces following the
discussion in http://crrev.com/2832813002.

Intent to implement and ship:
https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/-E-O9LgsPfM/Ex...

BUG=715077
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[jdoerrie, 2017-04-27 16:23:10]

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

[commit-bot: I haz the power, 2017-04-27 16:23:46]

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-04-27 16:28:06]

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

[commit-bot: I haz the power, 2017-04-27 16:28:07]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[jdoerrie, 2017-04-27 16:38:17]

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

[commit-bot: I haz the power, 2017-04-27 16:38:57]

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-04-27 17:02:24]

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

[commit-bot: I haz the power, 2017-04-27 17:02:25]

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

[jdoerrie, 2017-04-28 06:29:34]

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

[commit-bot: I haz the power, 2017-04-28 06:29:47]

Dry run: CQ is trying da patch.

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

[Mike West, 2017-04-28 07:34:48]

third_party/WebKit LGTM, thanks!

[commit-bot: I haz the power, 2017-04-28 07:39:35]

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

[commit-bot: I haz the power, 2017-04-28 07:39:36]

Dry run: This issue passed the CQ dry run.

[jdoerrie, 2017-04-28 08:49:54]

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

[jdoerrie, 2017-04-28 08:49:55]

Thanks, Mike!

stevenjb@chromium.org: Please review changes in
- chrome/browser/resources/options/password_manager.html
- chrome/browser/resources/options/password_manager.js
- chrome/browser/ui/webui/options/password_manager_handler.cc

jochen@chromium.org: Please review the remaining files in //chrome and
//content.

[stevenjb, 2017-04-28 15:44:51]

It looks like the credentials_enable_autosignin pref is not hidden in the new MD
Settings UI?

https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/passwo...

It might be worth double checking that, but this lgtm.

[jochen (gone - plz use gerrit), 2017-05-02 09:16:13]

lgtm

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

Thank you for the LGTMs!

stevenjb@chromium.org: You are right, in the new MD settings the auto-signin box
is always shown, which is the desired behavior.

[jdoerrie, 2017-05-02 09:23:25]

The CQ bit was checked by jdoerrie@chromium.org

[commit-bot: I haz the power, 2017-05-02 09:23:44]

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 11:19:38]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1493717005614130,
"parent_rev": "37b22b3ee932500caa379e06b0a170cc164c8d03", "commit_rev":
"44ca0605b598508b94a2cf0ccb4d37b8023ef3fb"}

[commit-bot: I haz the power, 2017-05-02 11:19:48]

Description was changed from

==========
Update CM API IDL to use SecureContext

This change implements recent changes to the Credential Manager API
specification that introduce the |SecureContext| extended attribute on
its interfaces. Furthermore it removes
|RuntimeEnabled=CredentialManager| from the interfaces following the
discussion in http://crrev.com/2832813002.

Intent to implement and ship:
https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/-E-O9LgsPfM/Ex...

BUG=715077
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Update CM API IDL to use SecureContext

This change implements recent changes to the Credential Manager API
specification that introduce the |SecureContext| extended attribute on
its interfaces. Furthermore it removes
|RuntimeEnabled=CredentialManager| from the interfaces following the
discussion in http://crrev.com/2832813002.

Intent to implement and ship:
https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/-E-O9LgsPfM/Ex...

BUG=715077
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2844453002
Cr-Commit-Position: refs/heads/master@{#468596}
Committed:
https://chromium.googlesource.com/chromium/src/+/44ca0605b598508b94a2cf0ccb4d...
==========

[commit-bot: I haz the power, 2017-05-02 11:19:50]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/44ca0605b598508b94a2cf0ccb4d...