1299783 - The camera indicator translates between "Allow temporarily" and "Allow"

Status: VERIFIED FIXED

(Firefox :: Site Permissions, defect, P1)

Description

[Adrian Florinescu [:aflorinescu]]

[Affected versions]:
FF Nightly 51.0a1


[Affected platforms]:
All


[Steps to reproduce]:

1. Open two tabs and in each go to https://people.mozilla.org/~fqueze2/webrtc/ .
2. In Tab 1 click on video and share selected device and check the permission label in Control Center.
3. In Tab 2 click on video and share selected device and check the permission label in Control Center.
4. Switch to Tab1 and check permission text.
5. In Tab 2 open Control Center and click x closing the camera share then check the permission label in Control Center for tab1.



[Expected result]:
For each tab, the device permissions should state that Video is Allowed Temporarily.

[Actual result]:
Step2: The permission text states(tab1): Allow Temporarily.
Step3: The permission text states(tab2): Allow.
Step4: The permission text states(tab1): Allow.
Step5: The permission text states(tab1): Allow Temporarily.

[Note1]:
I think that the standard permission status label in use currently  is a bit confusing. In my point of view it it should be like:
Allow temporarily : Allow Once 
Always            : Always Allow

[Note2]:
I don't think this is a regression, the behavior for version 48 is somehow similar. The permission status label keeps translating between Continue sharing and Allow depending on how many tabs you open.

Comment 1

[Florian Quèze [:florian]]

Our sharing indicator shouldn't be confused -> P1.

Comment 3

[Florian Quèze [:florian]]

This is caused by the temporary permissions set by the code added in bug 1177242.

When there's already an active stream from the same origin, we don't go through the C++ code that removes the permission after use. It seems to be a bug of the implementation, but I also have doubts about the approach itself:
- it seems to be an abuse of the permission manager.
- it sets the "camera" permission even if the user is sharing a screen rather than a camera (as illustrated by bug 1300055).
- I don't think it protects the user as much as it could, as (unless I got confused) an attacker using a compromised child process with a one-time permission to access the camera could get a stream of the screen.

I wonder if we could find a more robust solution where webrtcUI.jsm would send the unanonimized device ids added in bug 1284683 + the origin to the media manager service in the chrome process, in addition to the message already sent to the child process when the user grants access.

Comment 4

[Florian Quèze [:florian]]

This is a regression in 47, but it became more user visible with bug 1206246 in Firefox 50.

Comment 5

[Florian Quèze [:florian]]

Should we go for a quick hack to hide the problem (eg. rename the temporary permission set for that purpose to a name that isn't exposed in the UI) or is there any chance of a real fix being developed in the relatively near future?

Comment 6

[Gian-Carlo Pascutto [:gcp]]

I'm not sure how a "real fix" should look, but renaming the temporary permission sounds fine to me, even better if specifies camera vs screen vs window which fixes (2) and (3). Fixing up CamerasParent to make the same distinction shouldn't be hard.

Reimplementing the permission manager in MediaManager sounds overkill for now.

Comment 7

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Florian Quèze [:florian] [:flo] from comment #3)
> When there's already an active stream from the same origin, we don't go
> through the C++ code that removes the permission after use.

Does that match the symptom in comment 0? The problem seems to appear as early as step 3 (before closing anything):

    Step3: The permission text states(tab2): Allow.

I.e. sharing camera in two same-page tabs should not elevate permission from "Allow Temporarily" to "Allow".

Re [Note 1] in comment 0: Renaming in the UX is probably a separate issue. FWIW I think "Allow Temporarily" is appropriate given the direction we're going with permission in bug 1270572.

Comment 8

[Adrian Florinescu [:aflorinescu]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #7)

> I.e. sharing camera in two same-page tabs should not elevate permission from
> "Allow Temporarily" to "Allow".

The permission is not elevated, just the status label gets changed to Allow - behind, the permission is still "Allow Temporarily".

Comment 9

[Florian Quèze [:florian]]

(In reply to Gian-Carlo Pascutto [:gcp] from comment #6)
> I'm not sure how a "real fix" should look, but renaming the temporary
> permission sounds fine to me, even better if specifies camera vs screen vs
> window which fixes (2) and (3). Fixing up CamerasParent to make the same
> distinction shouldn't be hard.

Is this something you expect to do soon?

Comment 10

[Gian-Carlo Pascutto [:gcp]]

No, not at all.

Comment 11

[Florian Quèze [:florian]]

Attachment: Patch v1

Not sure if 'MediaManagerVideo' is a reasonable name for the temporary permission, whatever string that isn't exposed in the UI should be fine.

Comment 12

[Gerry Chang [:gchang]]

Track for 50+/51+ as this is a regression for camera indicator.

Comment 13

[Florian Quèze [:florian]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=16138d433fdf

Comment 14

[Florian Quèze [:florian]]

https://hg.mozilla.org/integration/fx-team/rev/c1e0b205c39b8946dfb485c0ecee7a5bc640815c
Bug 1299783 - change the name of the temporary permission from 'camera' to something not exposed in the UI, r=gcp.

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c1e0b205c39b

Comment 16

[Gian-Carlo Pascutto [:gcp]]

This missed the Android case:
http://searchfox.org/mozilla-central/rev/8910ca900f826a9b714607fd23bfa1b37a191eca/mobile/android/chrome/content/WebrtcUI.js#169

Comment 17

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Florian, should we uplift this fix to Beta50 and Aurora51?

Comment 18

[Florian Quèze [:florian]]

(In reply to Ritu Kothari (:ritu) from comment #17)
> Hi Florian, should we uplift this fix to Beta50 and Aurora51?

I would like to, yes. But unfortunately this caused 2 regressions:
- bug 1304798, already fixed; we should uplift this fix at the same time.
- bug 1304975 (android only). Still unfixed.

Comment 19

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Ok. Thanks Florian. Let's uplift this and fixes from both bugs mentioned in comment 18 at the same time.

Comment 20

[Adrian Florinescu [:aflorinescu]]

Ovidiu, could you please verify this bug and the its non-android dependency Bug1304798.

Comment 21

[ovidiu boca[:Ovidiu]]

Build ID 	20161006030208

Tested on Mac OS X 10.10 with Firefox Nightly 52.0a1 (2016-10-06)

Seems like the original scenario from description is fixed. However if I change the scenario as follows: 

STR:
1. Open two tabs and in each go to https://people.mozilla.org/~fqueze2/webrtc/ .
2. In Tab 1 click on video and select "Always share" and check the permission label in Control Center.
3. In Tab 2 click on video and check the permission label in Control Center.
4. In Tab 2 open Control Center and click x closing the camera share then check the permission label in Control Center for tab1.

AR: 

2. Use the camera - Allow 
3. Use the camera - Allow 
4. Use the camera - Allow Temporarily  

Should I open a new bug for this scenario? Or is this something you want to fix in this one?

Comment 22

[Florian Quèze [:florian]]

(In reply to ovidiu boca[:Ovidiu] from comment #21)

> Seems like the original scenario from description is fixed. However if I
> change the scenario as follows: 
> 
> STR:
> 1. Open two tabs and in each go to
> https://people.mozilla.org/~fqueze2/webrtc/ .
> 2. In Tab 1 click on video and select "Always share" and check the
> permission label in Control Center.
> 3. In Tab 2 click on video and check the permission label in Control Center.
> 4. In Tab 2 open Control Center and click x closing the camera share then
> check the permission label in Control Center for tab1.
> 
> AR: 
> 
> 2. Use the camera - Allow 
> 3. Use the camera - Allow 
> 4. Use the camera - Allow Temporarily  
> 
> Should I open a new bug for this scenario? Or is this something you want to
> fix in this one?

This is a variation of bug 1297047, but I don't think we have a bug covering specifically the steps you gave, so feel free to file one.

Comment 23

[ovidiu boca[:Ovidiu]]

> This is a variation of bug 1297047, but I don't think we have a bug covering
> specifically the steps you gave, so feel free to file one.

Thanks Florian, I followed your instructions and I filled a new bug for scenario from comment 21:
bug 1308213

Comment 24

[Florian Quèze [:florian]]

Attachment: Combined patch for uplift

This is attachment 8792933 [details] [diff] [review] + the 2 regression fixes from bug 1304798 and bug 1304975.

Approval Request Comment
[Feature/regressing bug #]: This is a regression in 47 from bug 1177242, but it became more user visible with bug 1206246 in Firefox 50.
[User impact if declined]: Confusing "Allow" permission that doesn't match what the user set can appear in the control center.
[Describe test coverage new/current, TreeHerder]: QA verified the fixes. No automated test coverage specifically for this, but the webrtc UI is overall well covered.
[Risks and why]: Acceptable in my opinion, the changes are simple.
[String/UUID change made/needed]: none.

Comment 25

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8798464 [details] [diff] [review]
Combined patch for uplift

Fixes a recent regression (since 48), Aurora51+, Beta50+

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/27f8698b0106

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/3bf677e1bd15

Comment 28

[ovidiu boca[:Ovidiu]]

Tested on Firefox Developer Edition (2016-10-07), Build ID 20161007004004 and I can confirm the fix.

Tested on Firefox beta 50.0b5 and here I found some additional issues that block verifying the fix:

(1) First issue:

STR:
1. Go to https://people.mozilla.org/~fqueze2/webrtc/ 
2. Click on video and share selected device and check the permission label in Control Center.

AR: 
Under Permissions you don't have any status, you can't turn off the video only if you refresh the page.

(2) Second issue: 

STR:
1.Go to https://people.mozilla.org/~fqueze2/webrtc/ 
2.Click on video and choose "Always share"
3.Under "Permissions" close the "Use the camera"

AR:
Camera is not turned off.

Comment 29

[Florian Quèze [:florian]]

(In reply to ovidiu boca[:Ovidiu] from comment #28)
> Tested on Firefox Developer Edition (2016-10-07), Build ID 20161007004004
> and I can confirm the fix.
> 
> Tested on Firefox beta 50.0b5 and here I found some additional issues that
> block verifying the fix:
> 
> (1) First issue:
> 
> STR:
> 1. Go to https://people.mozilla.org/~fqueze2/webrtc/ 
> 2. Click on video and share selected device and check the permission label
> in Control Center.
> 
> AR: 
> Under Permissions you don't have any status, you can't turn off the video
> only if you refresh the page.

This is because bug 1206233 landed in Firefox 51. On Firefox 50 (and before), the camera sharing indicator is a separate icon. If you click it, you'll have a doorhanger with a button saying "Continue sharing" and a dropdown containing "Stop sharing".

> (2) Second issue: 
> 
> STR:
> 1.Go to https://people.mozilla.org/~fqueze2/webrtc/ 
> 2.Click on video and choose "Always share"
> 3.Under "Permissions" close the "Use the camera"
> 
> AR:
> Camera is not turned off.

In Firefox 50 you already have the 'X' button as this landed in bug 1203292 for 50, but the sharing indicator used to stop sharing is still a separate doorhanger.

So... I think the 2 issues you described are just part of the feature that weren't implemented yet in Firefox 50.

The 'Allow temporarily' state also didn't exist in 50. So I think with the fix, you should not see a "Use the camera" line at all in the control center on 50, and without the fix you should see the camera being allowed.

Comment 30

[ovidiu boca[:Ovidiu]]

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

> So... I think the 2 issues you described are just part of the feature that
> weren't implemented yet in Firefox 50.

I think you are right Florian the 2 scenarios are not implemented yet in FF50. Thanks for clearing this out.