Add deprecation warnings for permission API usage from iframes

Add deprecation warnings for permission API usage from iframes

This adds deprecation warnings when the following APIs are used from cross
origin iframes:
-getCurrentPosition and watchPosition
-requestMIDIAccess
-requestMediaKeySystemAccess
-getUserMedia

A message is only logged if the feature would be disallowed by feature policy.

BUG=689802
Review-Url: https://codereview.chromium.org/2945223002
Cr-Commit-Position: refs/heads/master@{#485491}
Committed: https://chromium.googlesource.com/chromium/src/+/6a2e6fd73be0892689832e9af7feb26413db51ea

[raymes, 2017-06-21 05:00:41]

raymes@chromium.org changed reviewers:
+ iclelland@chromium.org

[Takashi Toyoshima, 2017-06-21 08:11:54]

webmidi lgtm

[iclelland, 2017-06-22 15:12:49]

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

https://codereview.chromium.org/2945223002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/Deprecation.cpp:462: return
iframePermissionsWarning("requestMediaKeySystemAccess", M62);
Should the interfaces in these messages be qualified with the object they apply
to? (I see that in a lot of the other deprecation warnings) --
"Geolocation.getCurrentPosition", for instance, rather than just
"getCurrentPosition"

https://codereview.chromium.org/2945223002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp
(right):

https://codereview.chromium.org/2945223002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp:330:
Deprecation::CountDeprecationCrossOriginIframe(
This appears to print a deprecation message (and count usage) on every use of
requestMediaKeySystemAccess -- is that correct? Can we restrict the count to
just cases where the method was actually called in a cross-origin context, and
was blocked?

By M61, I'd like to have also distinguish between the cases where it was
implicitly blocked because no positive-policy was present, and the case where it
was explicitly blocked by a header policy, but we don't have the logic in place
for that yet.

https://codereview.chromium.org/2945223002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webmidi/NavigatorWebMIDI.cpp (right):

https://codereview.chromium.org/2945223002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webmidi/NavigatorWebMIDI.cpp:35: #include
"core/dom/Document.h"
Whoa -- when did the strict ASCII ordering change? O < o, last time I checked.

https://codereview.chromium.org/2945223002/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/enums.xml (right):

https://codereview.chromium.org/2945223002/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/enums.xml:15497: +  <int value="2028"
label="EncryptedMediaDisabledIframe"/>
Can these be "<x>DisabledInCrossOriginIframe" for clarity?

[raymes, 2017-07-05 00:49:32]

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

[commit-bot: I haz the power, 2017-07-05 00:49:40]

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-07-05 00:51:50]

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

[commit-bot: I haz the power, 2017-07-05 00:51:51]

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_...)

[raymes, 2017-07-05 01:04:14]

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

[commit-bot: I haz the power, 2017-07-05 01:04:20]

Dry run: CQ is trying da patch.

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

[raymes, 2017-07-05 01:04:26]

iclelland: ptal :)

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

https://codereview.chromium.org/2945223002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/Deprecation.cpp:462: return
iframePermissionsWarning("requestMediaKeySystemAccess", M62);
On 2017/06/22 15:12:48, iclelland wrote:
> Should the interfaces in these messages be qualified with the object they
apply
> to? (I see that in a lot of the other deprecation warnings) --
> "Geolocation.getCurrentPosition", for instance, rather than just
> "getCurrentPosition"

Yeah I thought about that. Some things don't seem to though - e.g. the
InsecureOrigin messages which just use the function name. I'm happy to change if
it it's a better approach but I was just following the insecure origin example.
It also just felt easier to read without the fully qualified names and just as
easy to find out what it references.

https://codereview.chromium.org/2945223002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp
(right):

https://codereview.chromium.org/2945223002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp:330:
Deprecation::CountDeprecationCrossOriginIframe(
Updated to only count if:
-Disabled by FP, and
-In cross-origin iframe

Also added a comment about the ideal behavior

https://codereview.chromium.org/2945223002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webmidi/NavigatorWebMIDI.cpp (right):

https://codereview.chromium.org/2945223002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webmidi/NavigatorWebMIDI.cpp:35: #include
"core/dom/Document.h"
On 2017/06/22 15:12:48, iclelland wrote:
> Whoa -- when did the strict ASCII ordering change? O < o, last time I checked.

Fixed :) not sure what happened

https://codereview.chromium.org/2945223002/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/enums.xml (right):

https://codereview.chromium.org/2945223002/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/enums.xml:15497: +  <int value="2028"
label="EncryptedMediaDisabledIframe"/>
On 2017/06/22 15:12:48, iclelland wrote:
> Can these be "<x>DisabledInCrossOriginIframe" for clarity?

Done.

[raymes, 2017-07-05 01:05:12]

Description was changed from

==========
Add deprecation warnings for permission API usage from iframes

This adds deprecation warnings when the following APIs are used from cross
origin iframes:
-getCurrentPosition and watchPosition
-requestMIDIAccess
-requestMediaKeySystemAccess
-getUserMedia

BUG=689802
==========

to

==========
Add deprecation warnings for permission API usage from iframes

This adds deprecation warnings when the following APIs are used from cross
origin iframes:
-getCurrentPosition and watchPosition
-requestMIDIAccess
-requestMediaKeySystemAccess
-getUserMedia

A message is only logged if the feature would be disallowed by feature policy.

BUG=689802
==========

[commit-bot: I haz the power, 2017-07-05 03:24:11]

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

[commit-bot: I haz the power, 2017-07-05 03:24:12]

Dry run: This issue passed the CQ dry run.

[iclelland, 2017-07-05 04:41:22]

Thanks raymes -- this LGTM

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

https://codereview.chromium.org/2945223002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/Deprecation.cpp:462: return
iframePermissionsWarning("requestMediaKeySystemAccess", M62);
On 2017/07/05 01:04:25, raymes wrote:
> On 2017/06/22 15:12:48, iclelland wrote:
> > Should the interfaces in these messages be qualified with the object they
> apply
> > to? (I see that in a lot of the other deprecation warnings) --
> > "Geolocation.getCurrentPosition", for instance, rather than just
> > "getCurrentPosition"
> 
> Yeah I thought about that. Some things don't seem to though - e.g. the
> InsecureOrigin messages which just use the function name. I'm happy to change
if
> it it's a better approach but I was just following the insecure origin
example.
> It also just felt easier to read without the fully qualified names and just as
> easy to find out what it references.

Sounds good as long as there's no ambiguity there. (getUserMedia can refer to
two different APIs -- navigator and navigator.mediaDevices -- but as long as
they're controlled identically, by the same permissions, I think this is still
okay)

[raymes, 2017-07-05 05:09:54]

On 2017/07/05 04:41:22, iclelland wrote:
> Thanks raymes -- this LGTM
> 
>
https://codereview.chromium.org/2945223002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/frame/Deprecation.cpp (right):
> 
>
https://codereview.chromium.org/2945223002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/frame/Deprecation.cpp:462: return
> iframePermissionsWarning("requestMediaKeySystemAccess", M62);
> On 2017/07/05 01:04:25, raymes wrote:
> > On 2017/06/22 15:12:48, iclelland wrote:
> > > Should the interfaces in these messages be qualified with the object they
> > apply
> > > to? (I see that in a lot of the other deprecation warnings) --
> > > "Geolocation.getCurrentPosition", for instance, rather than just
> > > "getCurrentPosition"
> > 
> > Yeah I thought about that. Some things don't seem to though - e.g. the
> > InsecureOrigin messages which just use the function name. I'm happy to
change
> if
> > it it's a better approach but I was just following the insecure origin
> example.
> > It also just felt easier to read without the fully qualified names and just
as
> > easy to find out what it references.
> 
> Sounds good as long as there's no ambiguity there. (getUserMedia can refer to
> two different APIs -- navigator and navigator.mediaDevices -- but as long as
> they're controlled identically, by the same permissions, I think this is still
> okay)

Yep - they should both get the same restriction/message :)

[raymes, 2017-07-05 05:10:04]

raymes@chromium.org changed reviewers:
+ dcheng@chromium.org, ddorwin@chromium.org, guidou@chromium.org,
mcasas@chromium.org

[raymes, 2017-07-05 05:10:05]

+ddorwin for:
third_party/WebKit/Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp	

+guidou for 	third_party/WebKit/Source/modules/mediastream/UserMediaRequest.cpp

+mcasas for third_party/WebKit/Source/modules/geolocation/Geolocation.cpp

+dcheng for third_party/WebKit/Source/core/frame/Deprecation.cpp/h
third_party/WebKit/public/platform/web_feature.mojom

[Guido Urdaneta, 2017-07-05 10:34:03]

mediastream lgtm

[dcheng, 2017-07-05 20:18:02]

LGTM with comments addressed

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

https://codereview.chromium.org/2945223002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/Deprecation.cpp:74: String
iframePermissionsWarning(const char* function,
2 nits:
- please use UpperCamelCase for function names to match the new Blink style:
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/blink-...
- I would suggest naming this DeprecatedWillBeDisabledInCrossOriginSubframe.
It's a bit wordy, but describes the purpose exactly.

[ddorwin, 2017-07-05 20:29:20]

encryptedmedia/ LGTM. I left some comments in other files.

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

https://codereview.chromium.org/2945223002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/Deprecation.cpp:14: #include
"public/platform/WebFeaturePolicyFeature.h"
In this file, the URL is out of date. It should be
https://github.com/WICG/feature-policy/blob/gh-pages/features.md.

https://codereview.chromium.org/2945223002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/Deprecation.cpp:213: case
WebFeaturePolicyFeature::kEme:
nit: I suggest making this kEncryptedMedia as that term is more common in Blink
and clearer to those not familiar with the spec.

https://codereview.chromium.org/2945223002/diff/60001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/web_feature.mojom (right):

https://codereview.chromium.org/2945223002/diff/60001/third_party/WebKit/publ...
third_party/WebKit/public/platform/web_feature.mojom:1589:
kEncryptedMediaDisabledCrossOriginIframe = 2051,
"Disabled" here seems misleading. 
...CrossOriginIframeWithoutpermission?

[raymes, 2017-07-06 03:48:04]

Thanks all. 

Just waiting for mcasas@.

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

https://codereview.chromium.org/2945223002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/Deprecation.cpp:14: #include
"public/platform/WebFeaturePolicyFeature.h"
On 2017/07/05 20:29:20, ddorwin wrote:
> In this file, the URL is out of date. It should be
> https://github.com/WICG/feature-policy/blob/gh-pages/features.md.

Done.

https://codereview.chromium.org/2945223002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/Deprecation.cpp:74: String
iframePermissionsWarning(const char* function,
On 2017/07/05 20:18:02, dcheng wrote:
> 2 nits:
> - please use UpperCamelCase for function names to match the new Blink style:
>
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/blink-...
> - I would suggest naming this DeprecatedWillBeDisabledInCrossOriginSubframe.
> It's a bit wordy, but describes the purpose exactly.

Done.

https://codereview.chromium.org/2945223002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/Deprecation.cpp:213: case
WebFeaturePolicyFeature::kEme:
On 2017/07/05 20:29:20, ddorwin wrote:
> nit: I suggest making this kEncryptedMedia as that term is more common in
Blink
> and clearer to those not familiar with the spec.

That would be ok, but I'd prefer not to do it in the CL if we can.

https://codereview.chromium.org/2945223002/diff/60001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/web_feature.mojom (right):

https://codereview.chromium.org/2945223002/diff/60001/third_party/WebKit/publ...
third_party/WebKit/public/platform/web_feature.mojom:1589:
kEncryptedMediaDisabledCrossOriginIframe = 2051,
On 2017/07/05 20:29:20, ddorwin wrote:
> "Disabled" here seems misleading. 
> ...CrossOriginIframeWithoutpermission?

Hmm, that could imply user permission was denied though. I made it really
verbose but clear:
kEncryptedMediaDisabledInCrossOriginIframeDueToFeaturePolicy

[mcasas, 2017-07-06 04:49:34]

On 2017/07/06 03:48:04, raymes wrote:
> Thanks all. 
> 
> Just waiting for mcasas@.
> 
>
https://codereview.chromium.org/2945223002/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/frame/Deprecation.cpp (right):
> 
>
https://codereview.chromium.org/2945223002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/frame/Deprecation.cpp:14: #include
> "public/platform/WebFeaturePolicyFeature.h"
> On 2017/07/05 20:29:20, ddorwin wrote:
> > In this file, the URL is out of date. It should be
> > https://github.com/WICG/feature-policy/blob/gh-pages/features.md.
> 
> Done.
> 
>
https://codereview.chromium.org/2945223002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/frame/Deprecation.cpp:74: String
> iframePermissionsWarning(const char* function,
> On 2017/07/05 20:18:02, dcheng wrote:
> > 2 nits:
> > - please use UpperCamelCase for function names to match the new Blink style:
> >
>
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/blink-...
> > - I would suggest naming this DeprecatedWillBeDisabledInCrossOriginSubframe.
> > It's a bit wordy, but describes the purpose exactly.
> 
> Done.
> 
>
https://codereview.chromium.org/2945223002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/frame/Deprecation.cpp:213: case
> WebFeaturePolicyFeature::kEme:
> On 2017/07/05 20:29:20, ddorwin wrote:
> > nit: I suggest making this kEncryptedMedia as that term is more common in
> Blink
> > and clearer to those not familiar with the spec.
> 
> That would be ok, but I'd prefer not to do it in the CL if we can.
> 
>
https://codereview.chromium.org/2945223002/diff/60001/third_party/WebKit/publ...
> File third_party/WebKit/public/platform/web_feature.mojom (right):
> 
>
https://codereview.chromium.org/2945223002/diff/60001/third_party/WebKit/publ...
> third_party/WebKit/public/platform/web_feature.mojom:1589:
> kEncryptedMediaDisabledCrossOriginIframe = 2051,
> On 2017/07/05 20:29:20, ddorwin wrote:
> > "Disabled" here seems misleading. 
> > ...CrossOriginIframeWithoutpermission?
> 
> Hmm, that could imply user permission was denied though. I made it really
> verbose but clear:
> kEncryptedMediaDisabledInCrossOriginIframeDueToFeaturePolicy

geolocation lgtm, sorry for the delay

[ddorwin, 2017-07-06 23:28:22]

https://codereview.chromium.org/2945223002/diff/60001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/web_feature.mojom (right):

https://codereview.chromium.org/2945223002/diff/60001/third_party/WebKit/publ...
third_party/WebKit/public/platform/web_feature.mojom:1589:
kEncryptedMediaDisabledCrossOriginIframe = 2051,
On 2017/07/06 03:48:04, raymes wrote:
> On 2017/07/05 20:29:20, ddorwin wrote:
> > "Disabled" here seems misleading. 
> > ...CrossOriginIframeWithoutpermission?
> 
> Hmm, that could imply user permission was denied though. I made it really
> verbose but clear:
> kEncryptedMediaDisabledInCrossOriginIframeDueToFeaturePolicy

I was trying to find language that indicated the condition (no explicit
allowance for the iframe) without indicating what happened, since that will
change in a future release. In this case, "Disabled" could imply the future
behavior. I guess that's more future-proof if we intend to keep the stat for
reporting when it is really disallowed (no longer a deprecation counter but a
normal counter). I'm not sure how useful that will be, though.

[raymes, 2017-07-10 23:19:13]

https://codereview.chromium.org/2945223002/diff/60001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/web_feature.mojom (right):

https://codereview.chromium.org/2945223002/diff/60001/third_party/WebKit/publ...
third_party/WebKit/public/platform/web_feature.mojom:1589:
kEncryptedMediaDisabledCrossOriginIframe = 2051,
On 2017/07/06 23:28:21, ddorwin wrote:
> On 2017/07/06 03:48:04, raymes wrote:
> > On 2017/07/05 20:29:20, ddorwin wrote:
> > > "Disabled" here seems misleading. 
> > > ...CrossOriginIframeWithoutpermission?
> > 
> > Hmm, that could imply user permission was denied though. I made it really
> > verbose but clear:
> > kEncryptedMediaDisabledInCrossOriginIframeDueToFeaturePolicy
> 
> I was trying to find language that indicated the condition (no explicit
> allowance for the iframe) without indicating what happened, since that will
> change in a future release. In this case, "Disabled" could imply the future
> behavior. I guess that's more future-proof if we intend to keep the stat for
> reporting when it is really disallowed (no longer a deprecation counter but a
> normal counter). I'm not sure how useful that will be, though.

Ah I understand. Thanks for explaining. It's hard to phrase exactly because it
is still "disallowed by feature policy" but we're just not enforcing the feature
policy right now.

I've gone with DisallowedByFeaturePolicyInCrossOriginIframe which is hopefully
good enough :)

[raymes, 2017-07-10 23:27:33]

The CQ bit was checked by raymes@chromium.org

[raymes, 2017-07-10 23:27:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
toyoshim@chromium.org, guidou@chromium.org, dcheng@chromium.org,
iclelland@chromium.org, ddorwin@chromium.org, mcasas@chromium.org
Link to the patchset: https://codereview.chromium.org/2945223002/#ps140001
(title: ".")

[commit-bot: I haz the power, 2017-07-10 23:27:56]

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-07-11 01:55:48]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1499729253646490,
"parent_rev": "9497ff4773bba2f6638d9611b854c6bb20cb0652", "commit_rev":
"6a2e6fd73be0892689832e9af7feb26413db51ea"}

[commit-bot: I haz the power, 2017-07-11 01:56:03]

Description was changed from

==========
Add deprecation warnings for permission API usage from iframes

This adds deprecation warnings when the following APIs are used from cross
origin iframes:
-getCurrentPosition and watchPosition
-requestMIDIAccess
-requestMediaKeySystemAccess
-getUserMedia

A message is only logged if the feature would be disallowed by feature policy.

BUG=689802
==========

to

==========
Add deprecation warnings for permission API usage from iframes

This adds deprecation warnings when the following APIs are used from cross
origin iframes:
-getCurrentPosition and watchPosition
-requestMIDIAccess
-requestMediaKeySystemAccess
-getUserMedia

A message is only logged if the feature would be disallowed by feature policy.

BUG=689802

Review-Url: https://codereview.chromium.org/2945223002
Cr-Commit-Position: refs/heads/master@{#485491}
Committed:
https://chromium.googlesource.com/chromium/src/+/6a2e6fd73be0892689832e9af7fe...
==========

[commit-bot: I haz the power, 2017-07-11 01:56:05]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/6a2e6fd73be0892689832e9af7fe...