Fix #364: Integrate EME with Feature Policy #432

raymeskhoury · 2017-10-04T05:43:30Z

This adds a section defining the feature name and default allowlist, as well as integration with the requestMediaKeySystemAccess algorithm.

Preview | Diff

This adds a section defining the feature name and default allowlist, as well as integration with the requestMediaKeySystemAccess algorithm.

raymeskhoury · 2017-10-04T05:49:19Z

@clelland we still need to fix the "allowed to use" monkey-patch to make it take a policy-controlled feature. Is that something you could do?

@ddorwin ptal - I'm sure I've violated some style conventions but help would be appreciated in how to do things right - I'm not very experienced with this stuff!

I'm also in the process of getting a W3C account linked...

raymeskhoury · 2017-10-04T05:51:28Z

encrypted-media-respec.html

@@ -657,9 +669,10 @@

            <ol class="method-algorithm">
              <!-- TODO: Convert all parameters to use <code>. -->
+              <li><p>Let <var>document</var> be the calling context's <a def-id="document-concept"></a>.</p></li>
+              <li><p>If <var>document</var> is not <a data-cite="!HTML#allowed-to-use">allowed to use</a> the <a data-cite="!FEATURE-POLICY#policy-controlled-feature">policy-controlled feature</a> named <a>encrypted-media</a>, return a promise rejected with a newly created <a def-id="NotSupportedError"></a>.</p></li>


This could be a SecurityError as recommended in https://github.com/WICG/feature-policy/blob/gh-pages/integration.md. I actually think SecurityError is more appropriate but there may be backward compatibility concerns.

I think that's probably fine. NotSupportedError is an expected result that needs to be handled, but nothing is going to work in this case anyway.

@xhwang-chromium, we'll need to update Chromium if this changes (along with the actual algorithm steps).

Sorry just to clarify: which do you prefer - NotSupportedError or SecurityError?

I am fine with SecurityError if that is what other specs are using.

web-platform-tests/wpt#7889 assumes SecurityError.

Updated link: https://github.com/w3c/webappsec-feature-policy/blob/master/integration.md#referencing-the-feature-policy-spec

raymeskhoury · 2017-10-09T04:10:35Z

@ddorwin ping :)

ddorwin

Thank you. Looks good overall. We'll also need to update index.html before landing.

I updated the PR title to refer to the issue. You'll need to make sure the eventual commit message matches that (or I can do the merge).

On the process side, EME is now a Recommendation, so it's not clear how we should handle such post-REC changes in the repo. This is not really a new feature, simply patches the algorithm to align with another spec, and further enhances the user security and privacy mitigations in this spec, but it is observable. /cc @plehegar

ddorwin · 2017-10-10T04:34:45Z

encrypted-media-respec.html

@@ -657,9 +669,10 @@

            <ol class="method-algorithm">
              <!-- TODO: Convert all parameters to use <code>. -->
+              <li><p>Let <var>document</var> be the calling context's <a def-id="document-concept"></a>.</p></li>
+              <li><p>If <var>document</var> is not <a data-cite="!HTML#allowed-to-use">allowed to use</a> the <a data-cite="!FEATURE-POLICY#policy-controlled-feature">policy-controlled feature</a> named <a>encrypted-media</a>, return a promise rejected with a newly created <a def-id="NotSupportedError"></a>.</p></li>


I think that's probably fine. NotSupportedError is an expected result that needs to be handled, but nothing is going to work in this case anyway.

@xhwang-chromium, we'll need to update Chromium if this changes (along with the actual algorithm steps).

ddorwin · 2017-10-10T04:37:17Z

encrypted-media-respec.html

@@ -620,6 +620,18 @@
      </dl>
    </section>

+    <section>


This appears to be a new top-level section, which seems a bit odd. Also, it is specific to a single method, which is defined in the next top-level section. Should it be part of that? How do other APIs handle this?

I wasn't sure where to put it some I'm open to suggestions. I don't think there is any standard place right now but @clelland may have thoughts. I'll move it to the next section.

raymeskhoury · 2017-10-11T02:13:15Z

Thanks - please let me know how to update index.html.

I'm happy for you to do the merge if it's easier.

ddorwin · 2017-10-12T01:06:39Z

See https://github.com/w3c/encrypted-media/blob/master/TEAM.md#updating-indexhtml for how to update index.html.

This changes the type of error returned to be a SecurityError, as discussed in w3c/encrypted-media#432 (review). This also makes 3 other small changes: 1) The deprecation warning is only shown when the feature is not enabled. 2) A separate console message is logged when a request has been rejected due to a Feature Policy in effect. 3) A UseCounter also counts the number of times that a request is rejected due to Feature Policy. Bug: 689802 Change-Id: Ic067fea9df858ba5390f74073e67a350043fe8ce

raymeskhoury · 2017-10-24T06:28:45Z

Updated index.html. Note that it changed massively. I'm not sure if I did something wrong or if it is a new version of respec or something. But it changed just as much when I produced a snapshot from top-of-tree. Let me know if I'm missing something!

This changes the type of error returned to be a SecurityError, as discussed in w3c/encrypted-media#432 (review). This also makes 3 other small changes: 1) The deprecation warning is only shown when the feature is not enabled. 2) A separate console message is logged when a request has been rejected due to a Feature Policy in effect. 3) A UseCounter also counts the number of times that a request is rejected due to Feature Policy. Bug: 689802 Change-Id: Ic067fea9df858ba5390f74073e67a350043fe8ce Reviewed-on: https://chromium-review.googlesource.com/727319 Reviewed-by: Timothy Loh <timloh@chromium.org> Reviewed-by: Xiaohan Wang <xhwang@chromium.org> Reviewed-by: David Dorwin <ddorwin@chromium.org> Commit-Queue: Raymes Khoury <raymes@chromium.org> Cr-Commit-Position: refs/heads/master@{#511051}

plehegar · 2017-10-24T13:09:30Z

(to answer David's question)

plehegar · 2017-10-24T13:45:19Z

Feature Policy is still being developed so isn't yet suitable as a normative reference in a W3C Recommendation, so we wouldn't be able to push a new Edited Recommendation with it.
Once it becomes more stable, it's only adding a restriction so, from an IPR perspective, that's fine (cc @wseltzer ). As long as the change gets properly reviewed, we'd be good to go.

Malvoz · 2019-08-04T14:58:00Z

Friendly bump, other W3C recommendations such as https://www.w3.org/TR/payment-request/#feature-policy are now integrating this.

joeyparrish · 2019-11-18T23:17:27Z

Any objections to my merging this now (with an update to index.html)?

ddorwin · 2019-11-18T23:22:09Z

It looks like there is an open question of NotSupportedError vs SecurityError in the inline comments.

joeyparrish · 2019-11-18T23:34:44Z

It looks like there is an open question of NotSupportedError vs SecurityError in the inline comments.

The latest from that discussion was you saying you were fine with SecurityError and foolip saying that WPT expects it. So I assumed that was settled.

joeyparrish · 2019-11-18T23:37:58Z

JFYI, the "as defined by" part was being rejected by respec as it couldn't resolve the reference to "Feature Policy". I couldn't figure out the syntax to fix it, but I realized it was redundant, since the sentence already links to the definition in the Feature Policy spec. So with that removed and index.html regenerated, this merges cleanly.

I'll leave this open for another day to see if anybody has further comment. If not, I'll merge it. If I merge it and someone comes along later and says I missed something, we'll fix it.

Thanks!

ddorwin

It looks like there is an open question of NotSupportedError vs SecurityError in the inline comments.

The latest from that discussion was you saying you were fine with SecurityError and foolip saying that WPT expects it. So I assumed that was settled.

I had had hoped for a confirmation of "that is what other specs are using" and read the WPT comment as more of an observation than authoritative. :) That said, SecurityError appears to be the recommendation from the integration doc, so SGTM.

ddorwin · 2019-11-18T23:39:02Z

encrypted-media-respec.html

@@ -657,9 +669,10 @@

            <ol class="method-algorithm">
              <!-- TODO: Convert all parameters to use <code>. -->
+              <li><p>Let <var>document</var> be the calling context's <a def-id="document-concept"></a>.</p></li>
+              <li><p>If <var>document</var> is not <a data-cite="!HTML#allowed-to-use">allowed to use</a> the <a data-cite="!FEATURE-POLICY#policy-controlled-feature">policy-controlled feature</a> named <a>encrypted-media</a>, return a promise rejected with a newly created <a def-id="NotSupportedError"></a>.</p></li>


Updated link: https://github.com/w3c/webappsec-feature-policy/blob/master/integration.md#referencing-the-feature-policy-spec

joeyparrish · 2019-11-19T19:08:48Z

Updated link: https://github.com/w3c/webappsec-feature-policy/blob/master/integration.md#referencing-the-feature-policy-spec

Thanks! I've updated my local copy to reflect all of this, including the switch to SecurityError.

joeyparrish · 2019-11-19T19:17:10Z

Since my local diff against master is fairly small and easy to verify against the latest referencing guidelines, I'll go ahead and merge it now. I'd be happy to follow-up on any changes I missed, or review another PR if someone wants to make additional changes for this.

Thank you @raymeskhoury for your contribution!

Integrate EME with Feature Policy

4e96d44

This adds a section defining the feature name and default allowlist, as well as integration with the requestMediaKeySystemAccess algorithm.

raymeskhoury commented Oct 4, 2017

View reviewed changes

ddorwin changed the title ~~Integrate EME with Feature Policy~~ Fix #364: Integrate EME with Feature Policy Oct 10, 2017

ddorwin reviewed Oct 10, 2017

View reviewed changes

ddorwin mentioned this pull request Oct 10, 2017

Require explicit enabling of EME in nested contexts #364

Closed

Moved FP section

e71ecb0

chromium-wpt-export-bot mentioned this pull request Oct 18, 2017

Make EME return a SecurityError when disabled by Feature Policy web-platform-tests/wpt#7889

Merged

Update index.html

e4287db

plehegar self-assigned this Oct 24, 2017

plehegar removed their assignment Oct 24, 2017

clelland mentioned this pull request Nov 13, 2017

Require SecurityError to be used for EME w3c/webappsec-permissions-policy#92

Closed

joeyparrish self-assigned this Nov 18, 2019

joeyparrish added the feature request label Nov 18, 2019

ddorwin reviewed Nov 18, 2019

View reviewed changes

joeyparrish closed this in 5ba4b1b Nov 19, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #364: Integrate EME with Feature Policy #432

Fix #364: Integrate EME with Feature Policy #432

raymeskhoury commented Oct 4, 2017 •

edited by pr-preview bot

Loading

raymeskhoury commented Oct 4, 2017 •

edited

Loading

raymeskhoury Oct 4, 2017

ddorwin Oct 10, 2017

raymeskhoury Oct 11, 2017

ddorwin Oct 12, 2017

foolip Nov 23, 2017

ddorwin Nov 18, 2019

raymeskhoury commented Oct 9, 2017

ddorwin left a comment

ddorwin Oct 10, 2017

ddorwin Oct 10, 2017

raymeskhoury Oct 11, 2017

raymeskhoury commented Oct 11, 2017

ddorwin commented Oct 12, 2017

raymeskhoury commented Oct 24, 2017

plehegar commented Oct 24, 2017

plehegar commented Oct 24, 2017

Malvoz commented Aug 4, 2019

joeyparrish commented Nov 18, 2019

ddorwin commented Nov 18, 2019

joeyparrish commented Nov 18, 2019

joeyparrish commented Nov 18, 2019

ddorwin left a comment

ddorwin Nov 18, 2019

joeyparrish commented Nov 19, 2019

joeyparrish commented Nov 19, 2019

Fix #364: Integrate EME with Feature Policy #432

Fix #364: Integrate EME with Feature Policy #432

Conversation

raymeskhoury commented Oct 4, 2017 • edited by pr-preview bot Loading

raymeskhoury commented Oct 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raymeskhoury commented Oct 9, 2017

ddorwin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raymeskhoury commented Oct 11, 2017

ddorwin commented Oct 12, 2017

raymeskhoury commented Oct 24, 2017

plehegar commented Oct 24, 2017

plehegar commented Oct 24, 2017

Malvoz commented Aug 4, 2019

joeyparrish commented Nov 18, 2019

ddorwin commented Nov 18, 2019

joeyparrish commented Nov 18, 2019

joeyparrish commented Nov 18, 2019

ddorwin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joeyparrish commented Nov 19, 2019

joeyparrish commented Nov 19, 2019

raymeskhoury commented Oct 4, 2017 •

edited by pr-preview bot

Loading

raymeskhoury commented Oct 4, 2017 •

edited

Loading