Added allowvr attribute to iframes and vrEnabled to Document

Added allowvr attribute to iframes

Given that the API is capable of taking over the screen on mobile, similar to
fullscreen, it's prudent to ensure that iframes aren't given access to the
feature unless explicitly granted permission by the embedding page.

Spec: https://w3c.github.io/webvr/#allowvr-attribute

BUG=389343

[bajones, 2016-09-12 23:16:27]

Description was changed from

==========
Added allowvr attribute to iframes

Given that the API is capable of taking over the screen on mobile, similar to
fullscreen, it's prudent to ensure that iframes aren't given access to the
feature unless explicitly granted permission by the embedding page.

BUG=389343
==========

to

==========
Added allowvr attribute to iframes

Given that the API is capable of taking over the screen on mobile, similar to
fullscreen, it's prudent to ensure that iframes aren't given access to the
feature unless explicitly granted permission by the embedding page.

Spec: https://w3c.github.io/webvr/#allowvr-attribute

BUG=389343
==========

[bajones, 2016-09-15 18:48:15]

bajones@chromium.org changed reviewers:
+ haraken@chromium.org, mkwst@chromium.org, piman@chromium.org

[bajones, 2016-09-15 18:48:16]

I need some feedback on the high-level design of this, not sure who the right
reviewers are for that so please feel free to make suggestions.

For everyone's general sanity it's probably a good idea to not allow random
iframes to access VR APIs, especially on mobile where presenting VR content
means taking over the screen. Thus we're speccing an "allowvr" attribute for
iframes and a corresponding "vrEnabled" attribute for the document, mirroring
the same mechanisms for fullscreen.

Two questions come out of that: I'm aware of the iframe sandbox attribute.
Should I be preferring to add restrictions like this to the sandbox properties
instead? (Having it as an iframe attribute directly does make it restricted by
default, which is nice, but I don't want to propagate a pattern if it's been
replaced by something better.)

Second, if we do go the allowvr/vrEnabled route I need to add the vrEnabled
attribute to the document. I'd prefer to do that in modules/vr but that would be
the first time the document object has had a partial interface outside of core/
and I recall that there were reasons to avoid that? Would it be preferable to
implement this one attribute in core rather than expose document to partial
interfaces in modules?

This is an area where the spec is still very flexible, so I'm more interested in
adhering to best practices than conforming to any exact behavior.

Thanks in advance for any feedback or suggestions for better reviewers!

[dcheng, 2016-09-15 18:59:37]

dcheng@chromium.org changed reviewers:
+ dcheng@chromium.org

[dcheng, 2016-09-15 18:59:38]

Have you seen the CLs around iframe permission delegation?
https://codereview.chromium.org/2011763006/

That seems like something that'd be handy here.

[bajones, 2016-09-15 19:05:06]

On 2016/09/15 18:59:38, dcheng wrote:
> Have you seen the CLs around iframe permission delegation?
> https://codereview.chromium.org/2011763006/
> 
> That seems like something that'd be handy here.

I have not yet, thanks for the pointer!

[Mike West, 2016-09-16 05:07:17]

Yeah, talk to ojan@ / raymes@ / igrigorik@ about this in the context of feature
policy and delegation. I'm not sure what the latest thinking is there, but it
seems like following along in their footsteps would be the right way to go.

[raymes, 2016-09-22 06:04:40]

raymes@chromium.org changed reviewers:
+ alexmos@chromium.org, raymes@chromium.org

[raymes, 2016-09-22 06:04:41]

Sorry for the long delay.

The high level approach looks good to me (follows the same as other attributes).
+alexmos who is the expert.

We are doing something similar for the experimental permission delegation which
will be superseded by the Feature Policy proposal which we're currently in the
process of implementing.

[alexmos, 2016-09-23 00:53:31]

The allowvr plumbing and replication bits look good.  It's mirroring how
allowfullscreen works almost exactly.  Thanks for adding the site-per-process
test.

My main question is could we do this as part of permission delegation that
others have mentioned, instead of another separate attribute?  That seems like a
good fit for this and would let you do <iframe permissions="vr">, and raymes@
already added all this plumbing for it.  I guess I don't know if it's ok for VR
to take a dependency on permission delegation at this stage - raymes@ can
probably comment more on this.

> Two questions come out of that: I'm aware of the iframe sandbox attribute.
> Should I be preferring to add restrictions like this to the sandbox properties
> instead? (Having it as an iframe attribute directly does make it restricted by
> default, which is nice, but I don't want to propagate a pattern if it's been
> replaced by something better.)

I don't think this is a good fit for iframe sandbox.  Sandboxed iframes block a
bunch of other things, and the attribute tokens enable them back in.  Some
related discussion on the distinction here:
https://github.com/noncombatant/permission-delegation-api/issues/21
https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/KoasIWagGsk/L-...

https://codereview.chromium.org/2331223002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/vr/DocumentVR.idl (right):

https://codereview.chromium.org/2331223002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/vr/DocumentVR.idl:5: //
https://w3c.github.io/webvr/#document-vrenabled-attribute
The hash in that link doesn't seem to work.  Is this what you wanted?
https://w3c.github.io/webvr/#allowvr-attribute

https://codereview.chromium.org/2331223002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/vr/NavigatorVR.cpp (right):

https://codereview.chromium.org/2331223002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/vr/NavigatorVR.cpp:136: void
NavigatorVR::fireVRDisplayPresentChange(VRDisplay* display)
The spec mentions that VR events should also not be fired without the allowvr
check.  Does this also need that check?

[bajones, 2016-09-23 17:33:19]

Thanks for everyone's feedback so far! At this point I've proposed using
permission delegation to the other implementing browser (Firefox, Microsoft) but
they've expressed some concerns over the spec. They intend to get back to me
after consulting with the appropriate people within their orgs, and I'll relay
their thoughts when I hear back from them.

[raymes, 2016-09-26 00:04:34]

On 2016/09/23 17:33:19, bajones wrote:
> Thanks for everyone's feedback so far! At this point I've proposed using
> permission delegation to the other implementing browser (Firefox, Microsoft)
but
> they've expressed some concerns over the spec. They intend to get back to me
> after consulting with the appropriate people within their orgs, and I'll relay
> their thoughts when I hear back from them.

A few notes which hopefully answer some questions:
-The current feature policy spec is very out of date. We have an explainer doc
that is more up to date:
https://docs.google.com/document/d/1k0Ua-ZWlM_PsFCFdLMa8kaVTo32PeNZ4G7FFHqpFx...
-We'd love to discuss this more with other browser vendors. Please keep
igrigorik@, ojan@ and myself in the loop.
-On the implementation side, the permission delegation work I've done will have
to change as Feature Policy has evolved. I'm looking at diving into that more
soon. But if we do end up adding a separate allowvr attribute I think it may be
better to follow the fullscreen approach for now.