Closed Bug 1224453 Opened 9 years ago Closed 3 years ago

Revoking persistent camera+mic permissions from different-domain iframe is confusing/hard

Categories

(Firefox :: Site Permissions, defect, P2)

Product:
Firefox
Component:
Site Permissions
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 75

People

(Reporter: jib, Unassigned)

References

(Depends on 1 open bug, Blocks 2 open bugs)

Details

(Whiteboard: [fxprivacy][fixed by bug 1617219])

Attachments

(7 files)

Bug 1224453 - Part 1 - Display frame permissions in the control center.
59 bytes, text/x-review-board-request
Details
Screenshot
97.00 KB, image/png
Details
Bug 1224453 - Part 2 - Adjust WebRTC sharing state to enable per-frame display.
59 bytes, text/x-review-board-request
Details
Bug 1224453 - Part 3 - Change identity popup permission descriptions to match the UX spec.
59 bytes, text/x-review-board-request
Details
Spacing_1224453.png
84.56 KB, image/png
Details
Bug 1224453 - Part 4 - Add UI tests for frame permissions.
59 bytes, text/x-review-board-request
Details
Bug 1224453 - Part 5 - Add a test for WebRTC sharing indicators in frames.
59 bytes, text/x-review-board-request
Details 
STR:
1. Open https://jsfiddle.net/srn9db4h/
2. Hit [Start] and "Always Share".
3. Refresh the page.
4. Try to revoke the camera+mic access you added in (2).

Expected result: I can turn it off in the Page Info dropdown.
Actual result: That doesn't work because of Bug 1201972.

Workarounds:

A. Hit [Start] to turn on cam+mic again, then click camera icon and "Stop Sharing".
B. Right-click LR frame / This Frame / Show Only This Frame, then Page Info / Block. 
C: about:permissions (going away soon in bug 933917), and find jshell.net sub-domain.

B and C are hard and unintuitive.

A is insufficient, because a user may not have control over turning on the camera and mic (a spying malicious site that's been able to obtain persistent permission may only turn the camera or mic on when the browser is idle, turning it off as soon as the user moves the mouse).
See Also: → 933917
(In reply to Jan-Ivar Bruaroey [:jib] from comment #0)

> B. Right-click LR frame / This Frame / Show Only This Frame, then Page Info
> / Block. 

This workaround can be (slightly) simplified by clicking "View Frame Info" rather than "Show Only This Frame".

It's not enough anyway, as the iframe with persistent permissions may not be visible/clickable when the camera isn't in use.

For that web page, as a user I would expect to be able to revoke the permissions from the control center (doesn't work either right now). But I don't see how that could be implemented if the frame isn't even currently visible at the time the user thinks of revoking the permissions.

I'm starting to wonder if we shouldn't just refuse to honor persistent permissions for sub-frames, and prompt the user each time.
Bug 1225156 is related and will hopefully help some here.
See Also: → 1297041
No longer blocks: 1330559
See Also: → 1330559
Whiteboard: [fxprivacy] [triage]
Blocks: 1335155
Priority: -- → P2
Whiteboard: [fxprivacy] [triage] → [fxprivacy]
Assignee: nobody → jhofmann
Status: NEW → ASSIGNED
Comment on attachment 8844986 [details]
Bug 1224453 - Part 1 - Display frame permissions in the control center.

This is a first attempt at showing subframe permissions in the control center. I tried to keep it simple. I found an old mockup of this basic idea in the invision spec which looks a lot like what I implemented: https://mozilla.invisionapp.com/share/PC3Y0QSRK#/screens/96290112

If we want to go forward with the restrictions on iframe permissions outlined in bug 1335155 we need a way for the user to easily revert blocked permissions from iframes. At the same time it can be very useful to remove Allow permissions for origins you did not previously know were embedded on a page.

Florian, I'd love if you could take a look at this and give some feedback on the patch. I need to get general UX input on this so I don't think it's worth doing an actual round of review yet.

A big caveat is that I don't think it's technically viable to display the indicator on the (i) icon and/or the blocked permission icons for tabswitch performance reasons and because we would have to have some sort of listener for additional iframes being added asynchronously.

Philipp, can you find someone to take a look at this and find out

a) if there are any major issues with this UX-wise and
b) if someone from UX could go over this and make it look consistent with our design?

I'll attach a screenshot.
Flags: needinfo?(philipp)
Attachment #8844986 - Flags: feedback?(florian)
Comment on attachment 8844986 [details]
Bug 1224453 - Part 1 - Display frame permissions in the control center.

f+ as this seems to be going in the intended direction :-).

A few issues I noticed:
- the "You have not granted this site any special permission." text is weird when we are going to display permission for a frame on the next line.
- collecting the frames at the time the user opens the identity panel is a good start, but I think there are more cases we should display. Eg. if a subframe requests camera access and the user says 'no' with the checkbox checked, we are going to block it persistently on the subframe's domain. If the main page's error handler reacts by removing the subframe from the DOM, then there's no way to cancel the block.
Similarly, if the user granted persistent permission for a subframe and that subframe is removed from the DOM, the control center should still show it (see what the webrtc UI code does with _devicePermissionURIs to handle this when the user stops an ongoing share).
- somehow, when sharing the camera, "allowed temporarily" is shown for the main page. And if the user checked the checkbox before clicking the 'Allow' button, then the 'Allowed temporarily' string is still displayed for the main page, and 'Allowed' is displayed for the frame.
Attachment #8844986 - Flags: feedback?(florian) → feedback+
Thanks!

(In reply to Florian Quèze [:florian] [:flo] from comment #7)
> Comment on attachment 8844986 [details]
> Bug 1224453 - Display frame permissions in the control center.
> 
> f+ as this seems to be going in the intended direction :-).
> 
> A few issues I noticed:
> - the "You have not granted this site any special permission." text is weird
> when we are going to display permission for a frame on the next line.

Interesting, I left that there on purpose because I thought it was clear enough that it means that you have not granted _this_ site any special permissions, _but_ there are permissions for example.com. I guess that's something for UX to decide.

> - collecting the frames at the time the user opens the identity panel is a
> good start, but I think there are more cases we should display. Eg. if a
> subframe requests camera access and the user says 'no' with the checkbox
> checked, we are going to block it persistently on the subframe's domain. If
> the main page's error handler reacts by removing the subframe from the DOM,
> then there's no way to cancel the block.

I don't really see why we should consider that. If the site wants to enable the user to revert their decision they should really not remove the subframe, I don't think it's our responsibility to handle this.

> Similarly, if the user granted persistent permission for a subframe and that
> subframe is removed from the DOM, the control center should still show it
> (see what the webrtc UI code does with _devicePermissionURIs to handle this
> when the user stops an ongoing share).

Well for this case I might be lucky and this could be handled by the webrtcUI.jsm code already.

> - somehow, when sharing the camera, "allowed temporarily" is shown for the
> main page. And if the user checked the checkbox before clicking the 'Allow'
> button, then the 'Allowed temporarily' string is still displayed for the
> main page, and 'Allowed' is displayed for the frame.

Hm, yeah, it would indeed be better if the active sharing state was in the iframe.
(In reply to Johann Hofmann [:johannh] from comment #8)

> > A few issues I noticed:
> > - the "You have not granted this site any special permission." text is weird
> > when we are going to display permission for a frame on the next line.
> 
> Interesting, I left that there on purpose because I thought it was clear
> enough that it means that you have not granted _this_ site any special
> permissions, _but_ there are permissions for example.com.

My assumption is that it's clear enough for the 1% of users who understand what a frame is, and will be very confusing for everybody else.

> > Similarly, if the user granted persistent permission for a subframe and that
> > subframe is removed from the DOM, the control center should still show it
> > (see what the webrtc UI code does with _devicePermissionURIs to handle this
> > when the user stops an ongoing share).
> 
> Well for this case I might be lucky and this could be handled by the
> webrtcUI.jsm code already.

My assumption was that you wanted subframe permissions to work similarly for all permission types (which could let us remove the current webrtc special case).
> My assumption was that you wanted subframe permissions to work similarly for all permission types (which could let us remove the current webrtc special case).

Yes I generally like not making a difference between webrtc and other permissions (there are enough already) but in this case webrtc is the only permission that has an active sharing state.

I'll think about how to make this more elegant tomorrow :)
Jacqueline,

I think the main engineering effort for this is mostly done, but before starting to polish and write tests I'd like some input on this from UX.

This bug is implementing a part of the control center spec that was left out in the original MVP, displaying permissions for sub-frames on a page: https://mozilla.invisionapp.com/share/PC3Y0QSRK#/screens/96290112

The spec is a bit vague and my implementation makes some estimates about what would look good here. I would love if you could play around with it for a couple of minutes and give me an overall impression as well as some specific things you would like me to change in order for this to be shippable to users.

You can download a build to try it from the try push in comment 16, e.g. this is the link for OSX:
https://archive.mozilla.org/pub/firefox/try-builds/jhofmann@mozilla.com-13cc94cd6cee50c271ca8457b47501d689172d32/try-macosx64-debug/firefox-55.0a1.en-US.mac.dmg

You can try sub-frame permissions on http://johannh.me/frame-permission-site/

If this is all a bit confusing feel free to hit me up on IRC for more explanation. :)

Note that I will very soon devote the majority of my work time to the Photon project and would appreciate if we could try to get this important feature to users without a major UI rewrite. (Otherwise someone else would have to take over)

Thank you!!!
Flags: needinfo?(philipp) → needinfo?(jsavory)
Hi Johann, I’ve started with a couple things I’ve noticed:

- I seem to run into some problems with the permissions from the sub-frames being sorted into the “this site” category, should they always be appearing in the sub-frame section or is there a case where they aren’t?

- How are the different sub-frame sections organized? I would assume it would go in order that I enabled the permissions but it seems to have reorganized itself? (ex. If I have permissions from http://permission.site and http://people.mozilla.org)

- At first I was unsure which of the permissions the second title (“set by http://permisson.site”) was referring to, the above or below permissions. This might just be a spacing thing or we could change the text. I’ll play around with a couple things and let you know how we might solve this with minimal changes.

Let me know if anything I’ve written here either doesn’t make sense, or if you have questions about anything I’ve missed :)
Flags: needinfo?(jsavory)
(In reply to Jacqueline Savory [:jsavory] UX from comment #18)
> Hi Johann, I’ve started with a couple things I’ve noticed:
> 
> - I seem to run into some problems with the permissions from the sub-frames
> being sorted into the “this site” category, should they always be appearing
> in the sub-frame section or is there a case where they aren’t?
> 

Uh, yeah, they should always appear in the correct section. I'd appreciate STR for that :D

> - How are the different sub-frame sections organized? I would assume it
> would go in order that I enabled the permissions but it seems to have
> reorganized itself? (ex. If I have permissions from http://permission.site
> and http://people.mozilla.org)
> 

They're currently being organized by where they appear in the page source, but I could sort them alphabetically if you want some sort of order in there.

> - At first I was unsure which of the permissions the second title (“set by
> http://permisson.site”) was referring to, the above or below permissions.
> This might just be a spacing thing or we could change the text. I’ll play
> around with a couple things and let you know how we might solve this with
> minimal changes.

This is the sort of thing I was interested in, thanks! One thing to note is that visiting a site with more than one sub-frame with permissions should be extremely rare. (Even having a page that has a sub-frame with permissions at all should not be very common).
Attached image Spacing_1224453.png — 
Hey Johann, 

So I played around a bit and I think the easiest way to fix this will just be to change some spacing. 
Right now it looks like there is a larger gap underneath the “Set by this site.”, if we could instead use that size to be put underneath the last permission of each section and before the title of the next. Also, reduce the space of the gap to be the size that is in-between the “Set by https://permission.site.” I attached a screenshot that I edited to show what I mean in terms of the spacing, let me know if you have any questions about this. 

In terms of the permissions going in the wrong section, it seems that if I deny access to a permission is goes into the “this site” section and if I allow it then it goes into the appropriate “permission.site” section. Feel free to ping me if you have any questions about this, it seems a little inconsistent.
 
Thanks!
Florian, I'd appreciate a review on these patches, but I know this is neither your nor my main occupation at the moment, so please take your time and go over this whenever you have a moment to focus. It would be ideal if you could take a look at this before Austin, so that we can talk away any remaining issues in person.

Thanks!
Comment on attachment 8848569 [details]
Bug 1224453 - Part 2 - Adjust WebRTC sharing state to enable per-frame display.

https://reviewboard.mozilla.org/r/121474/#review221022

::: commit-message-c5c4f:8
(Diff revision 6)
> +The frame-specific display for web permissions in the control center should
> +also apply to active WebRTC streams. This commit adds an origin attribute
> +to the sharingState that is passed by ContentWebRTC and adapts the site
> +identity code to use it.
> +
> +I assume we can also get rid of the _devicePermissionURIs browser attribute,

I don't think so. Unless I'm misremembering something, _devicePermissionURIs was meant to also contain the URIs of frames that may no longer exist (and so which you can't access by recursively walking through the frame tree).

The problem here is:
- assume the user is browsing foo.com
- the page contains a bar.foo.com frame that requests device access, and manages to convince the user to grant persistent permission.
- as soon as the user grants permission, the bar.foo.com frame is removed from the foo.com page.

Now the user is left with no way to cancel that persistent permission for bar.foo.com.

Maybe we should show bar.foo.com in the doorhanger even if there's currently no frame for it, using the _devicePermissionURIs field for the control center too.

Or maybe we really need to prioritize fixing bug 1299577.
Attachment #8848569 - Flags: review?(florian)
Comment on attachment 8850950 [details]
Bug 1224453 - Part 3 - Change identity popup permission descriptions to match the UX spec.

https://reviewboard.mozilla.org/r/123458/#review221032

::: browser/locales/en-US/chrome/browser/browser.dtd:782
(Diff revision 5)
>  <!ENTITY identity.removeCertException.accesskey "R">
>  
>  <!ENTITY identity.moreInfoLinkText2 "More Information">
>  
>  <!ENTITY identity.permissions "Permissions">
> -<!ENTITY identity.permissionsEmpty "You have not granted this site any special permissions.">
> +<!ENTITY identity.permissionsDefault "Set to default">

"Set to default" is impossible to understand. Why are we changing this string?
Comment on attachment 8844986 [details]
Bug 1224453 - Part 1 - Display frame permissions in the control center.

https://reviewboard.mozilla.org/r/118236/#review227480

Clearing review flags until comment 43 is answered.
Attachment #8844986 - Flags: review?(florian)
Comment on attachment 8850950 [details]
Bug 1224453 - Part 3 - Change identity popup permission descriptions to match the UX spec.

https://reviewboard.mozilla.org/r/123458/#review227484
Attachment #8850950 - Flags: review?(florian)
Comment on attachment 8928976 [details]
Bug 1224453 - Part 4 - Add UI tests for frame permissions.

https://reviewboard.mozilla.org/r/200308/#review227486
Attachment #8928976 - Flags: review?(florian)
Comment on attachment 8928977 [details]
Bug 1224453 - Part 5 - Add a test for WebRTC sharing indicators in frames.

https://reviewboard.mozilla.org/r/200310/#review227488
Attachment #8928977 - Flags: review?(florian)

This will get resolved through feature policy, so I'm not planning to work on this anymore.

Assignee: jhofmann → nobody
Status: ASSIGNED → NEW

I assume this can be closed since Bug 1617219 enabled permission delegation in release? Johann, can you confirm?

Flags: needinfo?(jhofmann)

Discussed this with Johann on Matrix.

Status: NEW → RESOLVED
Closed: 3 years ago
Flags: needinfo?(jhofmann)
Resolution: --- → FIXED
Whiteboard: [fxprivacy] → [fxprivacy][fixed by bug 1617219]
Target Milestone: --- → Firefox 75
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: