804611 - Add a way to grant/deny getUserMedia permissions persistently

Status: RESOLVED FIXED

(Firefox :: Site Permissions, defect, P2)

Description

[Anant Narayanan [:anant]]

We had recently decided that it should be possible for the user to say that they don't want a particular site to ask for camera access, which should be persisted. This could be surfaced as a "Don't ask me again" option in the PopupNotification.

Boriss will give us details on how a user should be able to revoke this later, we'll likely do something simple for v1.

Comment 1

[Anant Narayanan [:anant]]

Attachment: Add support to deny camera access - v1

This patch does three things:
- Add a "Never share with this site" option to the consent dialog for getUserMedia.
- If the never share option is chosen, persist the choice in nsIPermissionManager.
- Add an entry to about:permissions so a user can revert their earlier decision.

Note that we do not have an "always allow" option, like Geolocation & IndexedDB. I think this is sufficient for us to be able to enable gUM in FF 19 (pending all other blockers of course).

Comment 2

[Dão Gottwald [:dao]]

Comment on attachment 678329 [details] [diff] [review]
Add support to deny camera access - v1

># HG changeset patch
># Parent 35ba50a6a97e3df90392394bc177fa6e2941ce90
>
>diff --git a/browser/components/preferences/aboutPermissions.js b/browser/components/preferences/aboutPermissions.js
>--- a/browser/components/preferences/aboutPermissions.js
>+++ b/browser/components/preferences/aboutPermissions.js

about:permissions is unfinished hidden, users generally don't know about it. The real permissions UI is in Page Info > Permissions.

>+  set getUserMedia(aValue) {
>+    let value = (aValue != this.DENY);
>+    Services.prefs.getBoolPref("media.navigator.enabled", value);

You probably meant to call setBoolPref here. Why would this code modify media.navigator.enabled, though?

Comment 3

[Dão Gottwald [:dao]]

(In reply to Dão Gottwald [:dao] from comment #2)
> about:permissions is unfinished hidden

unfinished *and* hidden

Comment 4

[Anant Narayanan [:anant]]

Attachment: Add support to deny camera access - v2

Thanks for the pointer to Page Info, I didn't know that even existed.

I think we should still retain the about:permission UI for future use, but in this patch I added support for changing it from page info as well.

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 678341 [details] [diff] [review]
Add support to deny camera access - v2

Review of attachment 678341 [details] [diff] [review]:
-----------------------------------------------------------------

From my non-UI perspective, r+

Comment 6

[Dão Gottwald [:dao]]

Comment on attachment 678341 [details] [diff] [review]
Add support to deny camera access - v2

The "allow" setting appears to be broken. I'm still asked for approval as if "always ask" was checked.

Also, unchecking "always ask" in Page Info's Permission tab doesn't enable the "allow" and "block" radio buttons like it should.

Comment 7

[Dão Gottwald [:dao]]

(In reply to Dão Gottwald [:dao] from comment #6)
> Comment on attachment 678341 [details] [diff] [review]
> Add support to deny camera access - v2
> 
> The "allow" setting appears to be broken. I'm still asked for approval as if
> "always ask" was checked.

Note that I had only one camera available. I guess the prompt is unavoidable if you have multiple devices to choose from, but then the "allow" setting doesn't make sense in the first place.

Comment 8

[Jason Smith [:jsmith]]

Btw - Anant is currently no longer an employee, so someone else may need to pick this up to finish this off. Dao - Can you help out here?

Comment 9

[Maire Reavy [:mreavy]]

We're not supporting persistent permissions/refusals in version 1.  So this is non-blocking.

Comment 10

[Florian Quèze [:florian]]

Attachment: Patch

This patch adds a "Remember this decision" checkbox in the device permission panel.

Microphone and Camera permissions are saved separately, so that if the user checked the "remember" checkbox when granting access to the microphone to a webpage; if later the page asks for both the microphone and camera, the prompt is shown again.

Revoking permissions is possible from:
- The Permissions tab of the Tool->Page Info dialog. (Note: these new permissions are at the bottom of the list because the list is sorted alphabetically, and "Use ..." is sorted last. If we want them to not be at the bottom we will need to change the wording of the permissions).
- about:permissions (not discoverable for now)
- Clicking the identity/padlock button in the URL bar: this panel lists only permissions that have non default values. But it's possible to set the permissions back to "Always ask" or "Deny".
- The "Stop Sharing" button I'm adding in bug 885796: if the user stops sharing with a web page but had granted persistent permissions, we forget the permissions and will prompt again next time.

I pushed this to try at https://tbpl.mozilla.org/?tree=Try&rev=ee226f4cd584 to simplify the UI review.

Comment 11

[Dão Gottwald [:dao]]

Comment on attachment 8346180 [details] [diff] [review]
Patch

>+  let perms = Services.perms;
>+  let micPerm = perms.testExactPermission(uri, "microphone");
>+  if (micPerm == perms.PROMPT_ACTION)
>+    micPerm = perms.UNKNOWN_ACTION;
>+  let camPerm = perms.testExactPermission(uri, "camera");
>+  if (camPerm == perms.PROMPT_ACTION)
>+    camPerm = perms.UNKNOWN_ACTION;
>+
>+  if ((!audioDevices.length || micPerm) && (!videoDevices.length || camPerm)) {
>+    // All permissions we were about to request already have a saved value.
>+    let allowedDevices = Cc["@mozilla.org/supports-array;1"]
>+                           .createInstance(Ci.nsISupportsArray);
>+    if (videoDevices.length && camPerm == perms.ALLOW_ACTION)
>+      allowedDevices.AppendElement(videoDevices[0]);
>+    if (audioDevices.length && micPerm == perms.ALLOW_ACTION)
>+      allowedDevices.AppendElement(audioDevices[0]);
>+
>+    if (allowedDevices.Count() == 0)
>+      denyRequest(aCallID);
>+    else
>+      Services.obs.notifyObservers(allowedDevices, "getUserMedia:response:allow", aCallID);

AFAIK this logic is usually not in UI code but in Gecko, i.e. Gecko should read those permissions and either deny the request, allow it or call the UI code.

Comment 12

[Florian Quèze [:florian]]

Attachment: Patch v2

Updated patch:
- removes bitrot caused by bug 723951
- based on the new patch in bug 885796
- addresses comment 11: the code granting or denying the request automatically based on saved permissions is now in MediaManager.cpp.

Requesting feedback from jesup on the MediaManager.cpp changes only.

Next I'll work on creating tests for this UI. I plan to do this by adding a preference that would cause MediaManager.cpp to check permissions when requesting fake streams.

Comment 13

[Florian Quèze [:florian]]

Attachment: Patch v3

- Found and fixed small mistakes in my changes to webrtcUI.jsm and MediaManager.cpp
- Now based on top of attachment 8357799 [details] [diff] [review] from bug 885796.

Comment 14

[Florian Quèze [:florian]]

Attachment: Test WIP

This doesn't test the "remember this decision" checkbox yet, but I'm in the process of writing tests for the WebRTC UI.

Comment 15

[Florian Quèze [:florian]]

Attachment: Patch v4, tests still WIP

There are now tests for the WebRTC UI for these cases:
test audio+video
test audio only
test video only
test audio+video, user disables video, only audio is shared
test audio+video, user disables audio, only video is shared
test audio+video, user disables both, nothing is shared
test audio+video, user clicks 'Don't Share', nothing is shared
test stop sharing

I still need to add tests for the "Remember this decision" checkbox.

Comment 16

[Florian Quèze [:florian]]

Attachment: Patch v5, tests included

Requesting review from dao for the browser/ part and jesup for the dom/media part as you reviewed attachment 678341 [details] [diff] [review] before. If you are too busy right now please suggest other reviews.

Note: the tests require the fix for bug 961802 to be applied (otherwise there's a large memory leak), and include a workaround for bug 962719.

Comment 18

[jory.graham]

I'd love to also see this on the Android client.

Comment 19

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8363866 [details] [diff] [review]
Patch v5, tests included

Review of attachment 8363866 [details] [diff] [review]:
-----------------------------------------------------------------

r- for now.  Very close to r+; may switch after discussion

::: dom/media/MediaManager.cpp
@@ +1090,5 @@
>      }
>      NS_DispatchToMainThread(new DeviceSuccessCallbackRunnable(mSuccess, mError,
>                                                                final.forget()));
> +    if (mConstraints.mFake)
> +      delete backend;

GetBackend() returns an ownership always?  or only in the fake case?  Seems like this is a bit risky API-wise.  Perhaps is should return an already_AddRefed ptr (which might have other refs, or not in the fake case)

@@ +1425,5 @@
> +    // Check if this site has persistent permissions.
> +    nsresult rv;
> +    nsCOMPtr<nsIPermissionManager> permManager =
> +      do_GetService(NS_PERMISSIONMANAGER_CONTRACTID, &rv);
> +    NS_ENSURE_SUCCESS(rv, rv);

Per m.d.platform, I believe we're trying to move away from NS_ENSURE_* (finally)

Comment 20

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8363866 [details] [diff] [review]
Patch v5, tests included

Review of attachment 8363866 [details] [diff] [review]:
-----------------------------------------------------------------

After discussing IRC, either comment it or convert backend to a RefPtr and remove the delete.

::: dom/media/MediaManager.cpp
@@ +1090,5 @@
>      }
>      NS_DispatchToMainThread(new DeviceSuccessCallbackRunnable(mSuccess, mError,
>                                                                final.forget()));
> +    if (mConstraints.mFake)
> +      delete backend;

GetBackend() returns an ownership always?  or only in the fake case?  Seems like this is a bit risky API-wise.  Perhaps is should return an already_AddRefed ptr (which might have other refs, or not in the fake case)

@@ +1425,5 @@
> +    // Check if this site has persistent permissions.
> +    nsresult rv;
> +    nsCOMPtr<nsIPermissionManager> permManager =
> +      do_GetService(NS_PERMISSIONMANAGER_CONTRACTID, &rv);
> +    NS_ENSURE_SUCCESS(rv, rv);

Per m.d.platform, I believe we're trying to move away from NS_ENSURE_* (finally)

Comment 21

[Florian Quèze [:florian]]

Comment on attachment 8363866 [details] [diff] [review]
Patch v5, tests included

The tests are green on try: https://tbpl.mozilla.org/?tree=Try&rev=605970db34d2

Jesup suggested I request the review from dolske; adding another request flag.
I'll update the patch tomorrow to address comment 20, but that won't change the browser/ part of the patch.

Comment 22

[Florian Quèze [:florian]]

Attachment: Patch v6

The nsRefPtr approach didn't work, so I just added a comment before the delete line. Also removed a slight touch of bitrot.

Comment 23

[Gregg Lind (Fx Strategy and Insights - Shield - Heartbeat )]

(clarification:  this patch seems like the flow is always "ask at least once?".  No pre-emptive, or white listing (say for addons)).

Comment 24

[Florian Quèze [:florian]]

(In reply to Gregg Lind (User Research - Test Pilot) from comment #23)
> (clarification:  this patch seems like the flow is always "ask at least
> once?".  No pre-emptive, or white listing (say for addons)).

An explicit click of the user on the "Remember this decision" checkbox is required.

Comment 25

[Justin Dolske [:Dolske]]

Comment on attachment 8364548 [details] [diff] [review]
Patch v6

Review of attachment 8364548 [details] [diff] [review]:
-----------------------------------------------------------------

(I didn't really look at the MediaManager bits or tests)

::: browser/base/content/popup-notifications.inc
@@ +30,5 @@
> +      <popupnotificationcontent>
> +        <checkbox id="webRTC-remember-decision"
> +                  label="&getUserMedia.remember.label;"
> +                  accesskey="&getUserMedia.remember.accesskey;"/>
> +      </popupnotificationcontent>

Why is this a checkbox?

Every other permission prompt uses "always allow" and "never allow" secondary actions, so seems like we should be consistent with that. Especially because this prompt is already complex, due to the presence of the device-selection widget(s).

Also, what's supposed to happen when the user selects a non-default device, and then wants the browser to do this automatically? [Another strike against the checkbox, or at least the specific wording, because it could be misinterpreted as just remembering the selection of a non-default device, versus automatically granting access.]

A few thoughts:

1) I suppose the ideal solution would be to somehow store the device selection along with the "always" permission. (Might even be nice to do so independently of that, so that your choice is sticky even if you want it to ask every time.)

2) We could disable the "always" option when you select a non-default device. That avoids the problem, but at the cost of potentially-confusing UI (i.e., user may not make the connection between the disabling an non-default choice).

3) Ignore the problem (followup bug!), as I suspect the combination of non-defaultness and setting the always-permission is rather uncommon.

I'd welcome UX input, but would probably lean towards #2 or #3 in the short-term.

[Small aside: is there any way for the browser to change what media device content gets afterwards, or for content to request a different device? One downside of #3 is that if a user wants to use a non-default device but we automatically give access to the default device, it seems like the only way for the user to undo that requires figuring out how revoke the permission.]

::: browser/modules/webrtcUI.jsm
@@ +293,5 @@
> +          perms.testExactPermission(uri, "camera") == perms.ALLOW_ACTION)
> +        Services.perms.remove(uri.host, "camera");
> +      if (hasAudio.value &&
> +          perms.testExactPermission(uri, "microphone") == perms.ALLOW_ACTION)
> +        Services.perms.remove(uri.host, "microphone");

The mixing of |perms| and |Services.perms| is a little weird. Pick one?

Comment 26

[Florian Quèze [:florian]]

(In reply to Justin Dolske [:Dolske] from comment #25)

Thanks for the review.

> Why is this a checkbox?

It looks like we have 2 possible ways to handle this:

1. A "[] Remember this decision" checkbox, like in the current patch.
Issues:
- the user may be confused and think the remembered decision is the choice of device.
- Other permission prompts are different, according to comment 25.

2. Put "Always Share" and "Never Share" items in the list of secondary actions.
- Edge case: if the user selects "no video" and "no audio", then clicks the "Always Allow" secondary action, what should the behavior be? (The 'remember this decision' wording was purposefully chosen to apply both to the allow or deny case.)
- Issue: low discoverability.

Boriss, what should we do here?

> Also, what's supposed to happen when the user selects a non-default device,
> and then wants the browser to do this automatically?

I think remembering which device is used should be independent from remembering camera/microphone permissions. I don't think it makes sense to remember the device per-hostname, so I think the selection of the device would be better in the preferences UI. This would also solve the problem of changing the preferred device without having to revoke permissions first. So I prefer your option #3; and I agree it's likely an uncommon case.


> [Small aside: is there any way for the browser to change what media device
> content gets afterwards

No, unfortunately. I filed bug 880312 about this a while ago.

> or for content to request a different device?

On Mobile I think content can specify whether the front or back camera is wanted.

> One
> downside of #3 is that if a user wants to use a non-default device but we
> automatically give access to the default device, it seems like the only way
> for the user to undo that requires figuring out how revoke the permission.]

Yes, the user has to click the "Stop sharing" secondary action I added in bug 885796. That's the reason why I worked on the 'stop sharing' case before working on persistent permissions.

Comment 27

[Jennifer Morrow [:Boriss] (UX)]

(In reply to Florian Quèze [:florian] [:flo] from comment #26)
> (In reply to Justin Dolske [:Dolske] from comment #25)
> > Why is this a checkbox?
> 
> It looks like we have 2 possible ways to handle this:
> 
> 1. A "[] Remember this decision" checkbox, like in the current patch.
> Issues:
> - the user may be confused and think the remembered decision is the choice
> of device.
> - Other permission prompts are different, according to comment 25.
> 
> 2. Put "Always Share" and "Never Share" items in the list of secondary
> actions.
> - Edge case: if the user selects "no video" and "no audio", then clicks the
> "Always Allow" secondary action, what should the behavior be? (The 'remember
> this decision' wording was purposefully chosen to apply both to the allow or
> deny case.)
> - Issue: low discoverability.

Good points all round, and this is a tricky one.  Yes, consistency-wise, Always Allow (as Dolske notes in Comment 25) is what we do elsewhere in Firefox.  However, there's one compelling reason this might be special-cased: applications that need to call again and again.  

The reason "Always Allow" was demoted in the original design to the dropdown menu is that it was thought to be rare: that a website would be granted access and then be able to function properly.  Few sites need to continually ask about your geolocation, for instance, because your needy SO is hopefully not a website.  And yes, discoverability is an issue in the dropdown - deliberately, because granting constant permission is (and perhaps should be) a rare action.

For sites and applications with background calling available-but-not-constant, however, this begins to break down.  It means a dialog at the beginning of every single call, making apps that use getusermedia in this way (like Talkilla) virtually unusable.  It also hinders developers from seeing getusermedia in Firefox as a viable vehicle for their app-ish functionality.

So, I'm proposing a checkbox as the measure to correct this.  The cons: #1 Inconsistency across Firefox and #2 Cluttering the UI.  #2 is unfortunate, but this UI is becoming so burdensome (and is so ill-suited to some applications smaller than a website) that we should undoubtedly do a separate redesign on it in particular (with media preview, hopefully).  #1 sucks, but I think is outweighed by the pro: the ability to use background calling applications without a dialog each and every time.

What would be even better?  A way to differentiate a website that just needs access once from one that needs it constantly.  I'm skeptical that this is possible on the non-Mozilla side, because if given the chance to self-differentiate I'm sure all sites would want the "always on" option.  Florian, if you know of a sensible way to distinguish between the two on our side, this would be ideal.

> > Also, what's supposed to happen when the user selects a non-default device,
> > and then wants the browser to do this automatically?

The best solution is likely actually solving this via bug 880312.  The problem is larger than this bug, because even changing device after having given access regularly can't be done right now.

For the very short term, bug 885796 allows users to stop sharing entirely, which means users can see they're sharing the wrong device - even if they've done so "Always," and revoke that access.

As for language - "Remember this decision" really means "Remember this decision for this... thing you're on."  We can't really use the word "site," because calling *applications* are not only possible but the more pressing need on this bug.  The ideal would be more like "Remember this decision for [foo]," where foo is something sensible to user to describe what they're granting access to.  Florian, can we extract some useful string here, like "amazon.com" or "Firefox messenger" or "Firefox Instant Share?"  If not, we may have to settle for something like "Remember this decision for this application" and hope users will associate it with sites and non-site.

Comment 28

[Florian Quèze [:florian]]

(In reply to Jennifer Morrow [:Boriss] (Firefox UX) from comment #27)

> What would be even better?  A way to differentiate a website that just needs
> access once from one that needs it constantly.  I'm skeptical that this is
> possible on the non-Mozilla side, because if given the chance to
> self-differentiate I'm sure all sites would want the "always on" option. 
> Florian, if you know of a sensible way to distinguish between the two on our
> side, this would be ideal.

The only way I can think of is to:
1. not show the checkbox the first time
2. If the user grants permissions to use the device, remember this automatically.
3. If the same website asks for permissions again, show the checkbox (which we could even think about checking by default).

I don't think this qualifies as a 'sensible way' because storing automatically the info that the user granted permissions to that website has bad privacy implications.


> As for language - "Remember this decision" really means "Remember this
> decision for this... thing you're on."
[...]
> Florian, can we extract some useful string here, like
> "amazon.com" or "Firefox messenger" or "Firefox Instant Share?"

We can extract the hostname; the prompt ("Would you like to share your <device> with <hostname>?") already contains it.
I don't think this is a great idea though, because:
- We would be duplicating this information
- The hostname may be long, and look ugly near a checkbox. More generally, I think the longer the label, the less likely it is to be read, and people would probably stop reading after the word "Remember".

Comment 29

[Florian Quèze [:florian]]

Attachment: Patch v7

(In reply to Justin Dolske [:Dolske] from comment #25)

> (I didn't really look at the MediaManager bits or tests)

The MediaManager changes have already been r+'ed by jesup on attachment 8363866 [details] [diff] [review].

The tests do need review though.


> Why is this a checkbox?

My understanding of comment 27 is that we do want a checkbox here.


> The mixing of |perms| and |Services.perms| is a little weird. Pick one?

Fixed, I picked |perms|.

Comment 30

[Justin Dolske [:Dolske]]

Sorry for the delay here. :(

(In reply to Florian Quèze [:florian] [:flo] from comment #26)

> > One
> > downside of #3 is that if a user wants to use a non-default device but we
> > automatically give access to the default device, it seems like the only way
> > for the user to undo that requires figuring out how revoke the permission.]
> 
> Yes, the user has to click the "Stop sharing" secondary action I added in
> bug 885796. That's the reason why I worked on the 'stop sharing' case before
> working on persistent permissions.

Hmm. I was specifically thinking about the other normal (but rarely encountered) ways the user has to revoke permissions (site identity doorhanger, page info dialog, about:permissions). All of which are... not exactly great UI. But it's what we have, and so gUM doesn't _need_ to tackle that problem.

It didn't quite connect for me that this patch adds revoking the permission in the "stop sharing" widget. I guess that's an improvement... Although if we expect users will commonly use that widget, it could be annoying to have it revoke the "always" permission. I suppose either way is ok for now, since AIUI the primary intent (?) of it is just to provide an obvious off-switch for crappy pages that don't provide any similar UI. But if we feel (or find) that users prefer to use it as a more general-purpose control, it probably shouldn't clear the permission.


(In reply to Jennifer Morrow [:Boriss] (Firefox UX) from comment #27)

> However, there's one compelling reason this might be
> special-cased: applications that need to call again and again.  

I don't agree with this argument. I'd expect that this permission will be encountered in the same way that other similar permissions are -- the vast majority of sites won't trigger it at all, you'll occasionally use it on a some sites as a one-off thing, and only on a tiny handful of sites (< 5?) will a user use it enough that the "Always" permission becomes a useful thing.

Geolocation is a prime example of this -- e.g. you'll see it now and then for something like store locator, but only on a few rare sites (like Google Maps or similar) do you want to enable it permanently.

Every permission will always have some use-case when "Always" is clearly something the user should enable to make their life easier. But I don't believe we should design for that unless it's a fairly common use case. We've only done that so far for plugins (Click-to-play), and that interaction still feel weird/wonky.

I think it would be entirely fair to revisit discoverability of Always/Never for doorhangers in general -- maybe they should all move to a checkbox, or have a "so you've clicked this 5 times, do this automatically from now on?" hint. But it's not clear to me that the case for gUM is strong enough to break with convention.

[Aside: if we did move to a checkbox, I suspect it would be a usability win to have the checkbox serve only for the "Always Allow" permission, and keep "Never Allow" as a secondary menu action. I suspect that's more common, and I find the "if this thing is checked automatically do whatever action is associated with the button I'm going to click whatever that means" wording rather awkward.]

> The reason "Always Allow" was demoted in the original design to the dropdown
> menu is that it was thought to be rare

Hmmm, that was part of it, but it was also to keep the prompts clear and simple. The icon + single main action was to make it easy to recognize and click (low cognitive load), and avoid the "uh oh, what's this about" generic wall of text that you had to read to decide which button to click. That's why I noted in comment 25 that this prompt is already a bit cluttered (dropdowns for cam and mic), and a checkbox is making that worse. And that's also why I'm harping on consistency; if every prompt had a "[ ] Always blah blah blah" checkbox at the bottom, that's easy to ignore. Having it there as a special case is a speed bump.


> For sites and applications with background calling
> available-but-not-constant, however, this begins to break down.  It means a
> dialog at the beginning of every single call, making apps that use
> getusermedia in this way (like Talkilla) virtually unusable.  It also
> hinders developers from seeing getusermedia in Firefox as a viable vehicle
> for their app-ish functionality.

To be clear, using the secondary action Always/Never makes this prompt no worse than all the other permission prompts we have.


> What would be even better?  A way to differentiate a website that just needs
> access once from one that needs it constantly.  I'm skeptical that this is
> possible on the non-Mozilla side, because if given the chance to
> self-differentiate I'm sure all sites would want the "always on" option. 
> Florian, if you know of a sensible way to distinguish between the two on our
> side, this would be ideal.

This is should be technically feasible, but probably doesn't make sense. We could make the permission persist until you close the tab / close the browser (I think we already do this with some perms?), but leads towards some hard questions, especially with something as privacy-sensitive as a webcam. EG, if you try out CaptionMyWebcamKitten.com for a one-time selfie but then leave the tab open... Will you be surprised if hours later it's able to silently start taking more pictures? Tying things to a "session" (until browser close) gets weird given the vast differences between a desktop user who keeps their browser running for many days, someone who opens and closes it many times in a day, and a mobile user where there's just app switching and no clear notion of quitting an app.


> As for language - "Remember this decision" really means "Remember this
> decision for this... thing you're on."  We can't really use the word "site,"
> because calling *applications* are not only possible but the more pressing
> need on this bug.

We use "site" (or a hostname/URL) everywhere else, because that's exactly what's getting permission. I'm not sure what you mean by "application". It's come up before for SocialAPI, but this UI doesn't apply for that, and AFAIK how to give sidebar pages the ability to prompt for permissions is an unsolved problem. Add-ons are also a separate issue, and are generally responsible for their own permission prompts, if they even want to have one.

Comment 31

[Florian Quèze [:florian]]

(In reply to Justin Dolske [:Dolske] from comment #30)

Justin, I'm disappointed by this reply. I was expecting a code review, not a longer UX debate with Boriss who ui-r+'ed the change more than 2 months ago, then read your concerns in comment 25 and confirmed in comment 27 that the checkbox in the patch was what she wanted.

The UI debate here is about a very tiny detail of the patch. The parts that are really important are:
- adding a way for users to not see the permission prompt every single time a webapp uses a device.
- adding tests for all of the existing webrtc UI (in bug 885796 you insisted to have them).

This patch is now blocking the further webrtc ui improvement I plan (or was planning) to do (eg. bug 876041, bug 970413), and likely also bug 947522. And I think it's now bitrotted by bug 949907 that got fixed yesterday; disappointing for a patch that has been waiting since 2014-01-23.

Can we please move the remaining UI discussions to a follow-up, and move forward?

Comment 32

[Justin Dolske [:Dolske]]

As noted in my previous comment, I very much disagree with the rationale for the checkbox. You can resubmit the patch with the small change to use the conventional UI, or escalate to Madhava to make a final UI decision.

Comment 33

[Florian Quèze [:florian]]

(In reply to Justin Dolske [:Dolske] from comment #32)

Can you please review the rest of the patch (especially the tests) anyway?

Comment 34

[Jennifer Morrow [:Boriss] (UX)]

We all agree that we need better UI for doorhangers in general, which should be a Firefox priority.  Particularly so for apps.  And, the checkbox is a UI "sloppy" stopgap, no doubt.

Given the shift that the webRTC team has taken this past week which means a Mozilla-published communication service is unlikely to ship in the next year, the priority of this particular feature is reduced.  The pain of cluttered UI for all gUM dialogs starts to become worse than the benefit of having the checkbox for those (now) few use cases.  

For now, let's add "Always" to the video permission dropdown.  
For the longterm, let's fix this dialog to make more sense for persistent permissions, such as Dolske's idea for a prompt on 2nd or Nth show of a dialog.

Florian, you're so right that this needs to be addressed, but given the recent shifts I hope you'll agree that the desperate need for it to be fixed *NOW* is absent.

Comment 35

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Jennifer Morrow [:Boriss] (Firefox UX) from comment #34)
> We all agree that we need better UI for doorhangers in general, which should
> be a Firefox priority.  Particularly so for apps.  And, the checkbox is a UI
> "sloppy" stopgap, no doubt.

Perhaps so (likely so) - but it's what we have to work with for now.

> Given the shift that the webRTC team has taken this past week which means a
> Mozilla-published communication service is unlikely to ship in the next
> year, the priority of this particular feature is reduced.

I'd say the opposite - I think the pressure is on to complete this and create a product.  Also there's considerable external pressure to provide persistent permissions (which are the default on Chrome (for https) and in fact impossible not to give there (IIRC)).

> The pain of cluttered UI for all gUM dialogs starts to become worse than the benefit of
> having the checkbox for those (now) few use cases.  
> 
> For now, let's add "Always" to the video permission dropdown.  

I don't think that's possible (or possible currently): that has to select a camera, so you can't add "always" as selection - or you have to have "foo" and "foo (always)" for every items.  Or you have to remove device selection from the UI like Chrome does (which can cause problems) and punt it to the application by enumerating input devices.  Which is possible, but we also don't support enumeration by apps or constraints to select using them yet.

You could also add another dropdown that defaults to "Just this time" and (when possible (https and maybe request from the site)) allows "Allow this site until revoked" (wordsmith as needed).  Yes, it's a glorified checkbox but it allows more wordy descriptions.  Just an idea.

> For the longterm, let's fix this dialog to make more sense for persistent
> permissions, such as Dolske's idea for a prompt on 2nd or Nth show of a
> dialog.

Note that communication services/sites/apps may then try to walk users through hitting it "enough" times to make it go away; not a great experience especially compared to Chrome (though Chrome's model has it's own separate issues re: security/etc).  That said: it's certainly an option.  The critical piece for persistent permissions was a reasonable way to view/revoke them, which we didn't have.  "privileged"/"trusted" apps or built-in ones may be able to bypass a number of security concerns (or revocation concerns) - we talked on occasion that "installing" apps might be linked to allowing persistent permissions.

> Florian, you're so right that this needs to be addressed, but given the
> recent shifts I hope you'll agree that the desperate need for it to be fixed
> *NOW* is absent.

I think if anything it's the opposite - resolving this is going to be an important point.  And as I mention, the pressure for persistent permissions is growing as people move to real apps using webrtc/getUserMedia and away from tech demos.

Comment 36

[Florian Quèze [:florian]]

(In reply to Randell Jesup [:jesup] from comment #35)

> > For now, let's add "Always" to the video permission dropdown.  
> 
> I don't think that's possible (or possible currently): that has to select a
> camera, so you can't add "always" as selection

The dropdown Boriss is talking about here is the one that contains "Share Selected Devices" and "Don't Share". Not the dropdown that lists the video devices.

Comment 37

[Maire Reavy [:mreavy]]

I agree with Randell about the urgency.  As the engineering manager for this work, I think it's critical to resolve this issue ASAP.  We've been talking about adding persistent permissions since Oct 2012.  Chrome has had persistent permissions for a while.  It is one of the most asked for features from our users & developers.  It's time to make a decision and land a solution.

Comment 38

[Madhava Enros [:madhava]]

Thanks, Boriss, for clarifying.

It sounds to me like we have a case we want to support (here for WebRTC but also for other similar cases in the future) for which our current UI model presents no perfect options. The checkbox is one possible solution, but further complicates what we have -- Dolske is right to point out that complication, and it's amplified but the (real!) problem we have with a hundred special cases, added in urgency, aggregating to real problems.

It sounds to me like Boriss' suggestion c34 is essentially what Dolske was also suggesting as a sound iterim UI that works but that we should revisit more holistically soon.

It also looks to me, though I'm not sure, that Randell's concern in c35 was a misunderstanding (Florian clarifies in c36). Can we go with that one?

Comment 39

[Madhava Enros [:madhava]]

To be clear - can we go with the suggestion in c34?

Comment 40

[Florian Quèze [:florian]]

(In reply to Madhava Enros [:madhava] from comment #38)

> It sounds to me like Boriss' suggestion c34 is essentially what Dolske was
> also suggesting

That's also the way I understood it. But I think that "let's add "Always" to the video permission dropdown" in comment 34 is an over-simplification.

To replace the "Remember this decision" checkbox of the current patch, we need to add 2 menu items:
- Always Share
- Never Share

And the edge case where the user selects "No Video" and "No Audio" in the device drop downs but then clicks "Always Share" is unfortunate. (Should this be handled as a click on "Never Share"?)

Comment 41

[Jennifer Morrow [:Boriss] (UX)]

(In reply to Florian Quèze [:florian] [:flo] from comment #40)
> (In reply to Madhava Enros [:madhava] from comment #38)
> 
> > It sounds to me like Boriss' suggestion c34 is essentially what Dolske was
> > also suggesting

Pretty much.  Not perfect by any means, but stopgaps fairly enough.

> That's also the way I understood it. But I think that "let's add "Always" to
> the video permission dropdown" in comment 34 is an over-simplification.
> 
> To replace the "Remember this decision" checkbox of the current patch, we
> need to add 2 menu items:
> - Always Share
> - Never Share
> 
> And the edge case where the user selects "No Video" and "No Audio" in the
> device drop downs but then clicks "Always Share" is unfortunate. (Should
> this be handled as a click on "Never Share"?)

Yes, exactly right.

Comment 42

[Justin Dolske [:Dolske]]

(In reply to Randell Jesup [:jesup] from comment #35)

> Also there's considerable external pressure to provide
> persistent permissions (which are the default on Chrome (for https) and in
> fact impossible not to give there (IIRC)).

That is... surprising. I just confirmed with https://people.mozilla.org/~dolske/tmp/chrome-gum-test/chrome-gum.html (which I had to copy over from https://shinydemos.com/3d-color-histogram/ since I couldn't find an existing SSL-hosted demo). Looks like geolocation works the same way -- grant it once on https://google.com/maps, and it persists! Wow.

We (Firefox) don't make this kind of distinction anywhere else with permissions, so that would be another inconsistent case. My first impression is that this isn't even the right permission model, _especially_ for cam/mic access. I should think the primary concern is with the site's basic ability to access recording devices, and the security of the network connection isn't terribly relevant... If a user notices their camera light unexpectedly turn on, will their first concern be "who's doing this" or "is this being transmitted over SSL"? [Of course, in our brave new NSA world, this actually should be a concern. And we might want to consider things like ignoring "always" permissions on non-SSL, or even requiring SSL for certain permissions. But not in this bug! :-)]

I think a Chrome-like permission model would likely be a tough sell to do in Firefox -- we don't play as fast and loose with privacy as Chrome does. That said, Chrome's UI does some things to help mitigate this. Specifically, they show a notification widget for both camera and geolocation access (and presumably all permissions?), which provides (1) a pretty clear indication when something is active, (2) an explanation of what's going on, (3) immediate controls to stop it, and (4) access to general UI to manage other exceptions. Firefox is generally lacking these (although, ironically, our cam/mic UI is the only thing to provide #1/#3).

But the point here is that we don't have this today. And while we should absolutely improve our permissions model/UX, doing it as a one-off isn't the right way to go about it IMO.


(In reply to Maire Reavy [:mreavy] from comment #37)
> I agree with Randell about the urgency.  As the engineering manager for this
> work, I think it's critical to resolve this issue ASAP.  We've been talking
> about adding persistent permissions since Oct 2012.

I'd point out that the original patch in this bug only added a "Never Share" option, and the r- on it was due to other brokenness. That patch was never updated, and there was basically no other activity on this bug for an entire year. The discussion really only started a few weeks ago.

Comment 43

[Justin Dolske [:Dolske]]

(In reply to Florian Quèze [:florian] [:flo] from comment #40)

> To replace the "Remember this decision" checkbox of the current patch, we
> need to add 2 menu items:
> - Always Share
> - Never Share
> 
> And the edge case where the user selects "No Video" and "No Audio" in the
> device drop downs but then clicks "Always Share" is unfortunate. (Should
> this be handled as a click on "Never Share"?)

WFM.

Comment 44

[Justin Dolske [:Dolske]]

Attachment: Chrome's post-allow indicator for camera

Just for future reference, here's the indicator Chrome shows after granting camera access, and the prompt shown upon clicking it.

Comment 45

[Justin Dolske [:Dolske]]

Attachment: Chrome's post-allow indicator for geolocation

...and for geolocation.

Comment 46

[Florian Quèze [:florian]]

Attachment: Patch v8

Comment 47

[Randell Jesup [:jesup] (needinfo me)]

Starting by responding to Madhava:
> To be clear - can we go with the suggestion in c34?
and Florian expanding on it:
> To replace the "Remember this decision" checkbox of the current patch, we need to add 2 menu items:
> - Always Share
> - Never Share

Yes, with two provisos: first we need to have a way to revoke persistent "Always" permission (and strongly preferred not to have to share your camera/mic in order to get to the UI to revoke!) and second, we need to limit Always to https-loaded sites, and not just whether we give the option, but also whether we apply a previous Always in case they get MITMed and/or switch from https to http.  Without this I think privacy/security review will be very hard.

With that out of the way, I'll respond to another comment:

(In reply to Justin Dolske [:Dolske] from comment #42)
> (In reply to Randell Jesup [:jesup] from comment #35)
> 
> > Also there's considerable external pressure to provide
> > persistent permissions (which are the default on Chrome (for https) and in
> > fact impossible not to give there (IIRC)).
> 
> That is... surprising. I just confirmed with
> https://people.mozilla.org/~dolske/tmp/chrome-gum-test/chrome-gum.html
> (which I had to copy over from https://shinydemos.com/3d-color-histogram/
> since I couldn't find an existing SSL-hosted demo). Looks like geolocation
> works the same way -- grant it once on https://google.com/maps, and it
> persists! Wow.
> 
> We (Firefox) don't make this kind of distinction anywhere else with
> permissions, so that would be another inconsistent case. 

Right.  And I was indicating what they do, not suggesting that model; we've been critical of it.  It's not illogical; but there are impacts of doing it their way.

> My first impression
> is that this isn't even the right permission model, _especially_ for cam/mic
> access. I should think the primary concern is with the site's basic ability
> to access recording devices, and the security of the network connection
> isn't terribly relevant... 

It's a security issue that no http-loaded site be allowed to get persistent permissions since it can be trivially MITM'd.  (Argument was made that no site with *any* HTML/JS loaded over http be allowed to get persistent permission; I forget how that one ended up.  From a sec point-of-view it makes sense.)  I suggested early on that persistent permission be tied to some sort of explicit trust from the user (such as installing an HTML app).  Many cases where persistent permission is wanted are replacements for native OS apps like Skype or WebEx (and installing those gives FAR more permission than we give in the browser with persistent permissions).

> If a user notices their camera light unexpectedly
> turn on, will their first concern be "who's doing this" or "is this being
> transmitted over SSL"? [Of course, in our brave new NSA world, this actually
> should be a concern. And we might want to consider things like ignoring
> "always" permissions on non-SSL, or even requiring SSL for certain
> permissions. But not in this bug! :-)]

Security/privacy review will need to be done on this; and this will be an issue.  See draft-ietf-rtcweb-security-06 4.1.4 for the threat model; it gets far worse when the attacker can spoof a site that has persistent permissions.  This is why Chrome only allows this on https pages.

> 
> I think a Chrome-like permission model would likely be a tough sell to do in
> Firefox -- we don't play as fast and loose with privacy as Chrome does. That
> said, Chrome's UI does some things to help mitigate this. Specifically, they
> show a notification widget for both camera and geolocation access (and
> presumably all permissions?), which provides (1) a pretty clear indication
> when something is active, (2) an explanation of what's going on, (3)
> immediate controls to stop it, and (4) access to general UI to manage other
> exceptions. Firefox is generally lacking these (although, ironically, our
> cam/mic UI is the only thing to provide #1/#3).
> 
> But the point here is that we don't have this today. And while we should
> absolutely improve our permissions model/UX, doing it as a one-off isn't the
> right way to go about it IMO.

I have no problem with re-doing it once a framework exists; but we need a solution for now.

> (In reply to Maire Reavy [:mreavy] from comment #37)
> > I agree with Randell about the urgency.  As the engineering manager for this
> > work, I think it's critical to resolve this issue ASAP.  We've been talking
> > about adding persistent permissions since Oct 2012.
> 
> I'd point out that the original patch in this bug only added a "Never Share"
> option, and the r- on it was due to other brokenness. That patch was never
> updated, and there was basically no other activity on this bug for an entire
> year. The discussion really only started a few weeks ago.

We punted on this until Florian picked it up (since we could do non-product-ish sites/demos without it), due to lack of resources (especially on the UI front, and because of what we were told would be needed).  Florian picked it up because he was working on a project that needed this functionality (but may externals have been clamoring for it, especially commercial-focused and "real user" services).  (Kudos to florian for pushing this forward.)

Comment 48

[Florian Quèze [:florian]]

(In reply to Randell Jesup [:jesup] from comment #47)

> Yes, with two provisos: first we need to have a way to revoke persistent
> "Always" permission (and strongly preferred not to have to share your
> camera/mic in order to get to the UI to revoke!)

See comment 10 for the list of ways to revoke persistent permissions offered by this patch.

> and second, we need to
> limit Always to https-loaded sites, and not just whether we give the option,
> but also whether we apply a previous Always in case they get MITMed and/or
> switch from https to http.  Without this I think privacy/security review
> will be very hard.

We just discussed this on IRC and agreed that this is something important we want to do, but that it shouldn't block the current patch from landing on inbound. We could then do a privacy/security review, and address the comments from that review at the latest before this feature goes to beta, and preferably before it goes to aurora.

Comment 49

[Justin Dolske [:Dolske]]

Comment on attachment 8379885 [details] [diff] [review]
Patch v8

Review of attachment 8379885 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/modules/webrtcUI.jsm
@@ +234,5 @@
> +          }
> +          if (allowCamera !== undefined) {
> +            perms.add(uri, "camera",
> +                      allowCamera ? perms.ALLOW_ACTION : perms.DENY_ACTION);
> +          }

I think it's generally undesirable to have tri-state booleans (true/false/undefined). I almost commented on this last time, but didn't have a worthwhile improvement... And now I just read this again, trying to remember why it's weird.

So, how about a brief comment like "only remember decision if a device was available"?

I suppose you could also fold it into the code above, ala:

  if (videoDevices.length) {
    let videoDeviceIndex = ...
    allowCamera = videoDeviceIndex != "-1";
    if (allowCamera)
      allowedDevices.AppendElement(videoDevices[videoDeviceIndex]);
    if (aRemember)
      perms.add(uri, "camera" ...)
  }

Comment 50

[Florian Quèze [:florian]]

Attachment: Patch v9

(In reply to Justin Dolske [:Dolske] from comment #49)

> I suppose you could also fold it into the code above

That's cleaner, thanks!

Comment 51

[Florian Quèze [:florian]]

(In reply to Randell Jesup [:jesup] from comment #20)

> After discussing IRC, either comment it or convert backend to a RefPtr and
> remove the delete.

Using a RefPtr is now required, as MediaEngine is now ref counted. Not doing this caused consistent test failures on try (https://tbpl.mozilla.org/?tree=Try&rev=0c376c56e317) on all debug builds.

Comment 52

[Florian Quèze [:florian]]

Attachment: Patch v10 (as checked in)

The only difference in this new patch is using a RefPtr, as discussed in comments 20 and 51.

I pushed it to try (https://tbpl.mozilla.org/?tree=Try&rev=3cc3ccdcf339) and the results were mostly green. There was one failure on Linux x64 Debug that seemed related (TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/general/browser_get_user_media.js | Unexpected Exception: TypeError: PopupNotifications.panel.firstChild is null) but I haven't seen it again after retriggering 'bc' 13 times. It may be just that this new test is exposing an existent panel issue that was already the cause of other intermittent panel bugs.

https://hg.mozilla.org/integration/fx-team/rev/2c5c8a682efc

Comment 53

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2c5c8a682efc

Comment 54

[Florian Quèze [:florian]]

The tests added in this bug have been failing intermittently on Mac opt and Linux debug since they landed. I've been dealing with this in bug 976544. After renaming the test to run it before the fullscreen test (that we suspect of messing up focus (bug 987115), we no longer hit the failure on Mac opt. I've just disabled the test on Linux Debug (and only there) to 'fix' the remaining failure.

Comment 55

[Pomax]

just to pitch in on permissions, only yes/no/forever/never misses out on the life of a url option. So if we're offering options, could I request a "yes/no" option, with three possible lifespans: this activation instance, the life of this page (effectively: until you close the tab), and forever?

UI wise that'd probably mean two separate options, one to select yes/no, one that's defaulted to whatever makes the most sense (and that's probably actually 'the life of this page').

Comment 56

[Florian Quèze [:florian]]

(In reply to Mike "Pomax" Kamermans [:pomax] from comment #55)

You are commenting in a bug that's already resolved. If you want to request improvements, please file a new bug, thanks.

Comment 58

[:Gavin Sharp [email: [e-mail]]]

Disabled in firefox 30 in bug 1000253.

Comment 59

[FI]

Does firefox for android supports "always allow" option for getUserMedia?

I didn't find that option so far (tested on HTTPS site).