945111 - [B2G getUserMedia] Show audio-capture and video-capture in the settings app

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Settings, defect)

Description

[Paul Theriault [:pauljt] (no longer reading bugmail)]

+++ This bug was initially created as a clone of Bug #853356 +++

We don't currently show 'audio-capture' or 'video-capture' under the App Permissions section of the settings app. We should change this, so that (at least for apps) that the user can see what permissions they have granted.

AFAICT In 1.2 there the user is prompted prior to every use of getUserMedia, and there is not way to remember permissions. If this is the case, then we probably don't need this for 1.2

But for later versions,  I understand the plan is to allow users to be able to remember this permission. If so, this permission needs to be a visible setting under App Permissions section of the Settings app.

The WebRTC permission should NOT be able to be remembered for web pages in the browser though, since we have no good way of allowing the user to view or manage this at the moment.

One final note: if we show this permission under settings, a user could set a regular web app to always have access to getUserMedia. Since we don't enforce SSL on web apps, this potentially increases the impact of a Man-in-the-middle scenario. However I think the benefits of allowing the user to remember the decision, and thus not getting prompt fatigue, outweigh the risks here. (especially when considered with other mitigation, like not allowing getusermedia from the background, and showing highly visible notifications, both of which I believe are already in place)

Comment 1

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Actually I have a feeling this bug may be invalid. When 853356 lands, it will make audio-capture and video-capture set to "PROMPT" for all app types in PermissionTable.jsm. Which means, iiuc, that it will automatically show up in the settings app, since the settings app uses this function to decide whether to display the permission or not[1]. See here in settings app [2].

[1] http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/PermissionsTable.jsm#459

[2] https://github.com/mozilla-b2g/gaia/blob/master/apps/settings/js/apps.js#L274

Comment 2

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Im going to leave this open assigned to me to test it though.

Comment 3

[Jason Smith [:jsmith]]

(In reply to Paul Theriault [:pauljt] from comment #2)
> Im going to leave this open assigned to me to test it though.

Really? We technically already have PROMPT_ACTION set for audio-capture permissions, so if that was true, we would see audio-capture in the settings app perms UI already.

Permission Table: http://hg.mozilla.org/mozilla-central/file/c93cfe704487/dom/apps/src/PermissionsTable.jsm#l300

Test App: http://mozilla.github.io/qa-testcase-data/webapi/webrtc/

Comment 4

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Yeh I mid-aired commenting the same thing as comment 3. As you say, we should see this already for audio-capture so more digging is needed to find out why this isn't shown.

Comment 5

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Oh I see why. The settings app has its own list of permissions [1] that isn't in sync with the table in PermissionsTable.jsm. Ich. I'm actually surprised we haven't hit more bugs but I guess we don't have many explicit permissions. Can we remove permissions_table.json and just use navigator.mozPermissionSettings instead? Or to ask a better question what is the meaning of "plain permissions" and why do we need that list (maybe its needed for other things that aren't captured in PermissionsTable.js ?


[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/settings/resources/permissions_table.json

Comment 6

[Stephanie Ouillon [:arroway] (needinfo me)]

I think you need permissions table in Gaia because the Permissions API doesn't allow you to list all existing permissions [1].
One move would be to add a getPermissionsTable() method to the Permissions interface (unless it exists somewhere but I didn't see it) and call it from the Settings app.
Would there be any reason not to do so?

[1] https://developer.mozilla.org/en-US/docs/Web/API/PermissionSettings

Comment 7

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Yeh ok makes sense. So as a simple fix, maybe its best just to update permissions_table.json for now, and solve the PermissionSettings API later? (being aware that 1.3 feature complete is only few days)

Comment 8

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Paul Theriault [:pauljt] from comment #5)
> Oh I see why. The settings app has its own list of permissions [1] that
> isn't in sync with the table in PermissionsTable.jsm. Ich. I'm actually
> surprised we haven't hit more bugs but I guess we don't have many explicit
> permissions. Can we remove permissions_table.json and just use
> navigator.mozPermissionSettings instead? Or to ask a better question what is
> the meaning of "plain permissions" and why do we need that list (maybe its
> needed for other things that aren't captured in PermissionsTable.js ?

plain_permissions are permissions that don't have sub-permissions (like in permissionname-write, permissionname-read...).

That table came originally from an script I made back in the day. My original idea was adding that script to the build process (in fact, it's done as a xulrunner js script mostly because of that), but that idea was shot down someplace in bug 821961.

The original commit for that script is on:

https://github.com/arcturus/gaia/commit/5c739688683c68a92cfab1a3ac00bb72c18f1b20


And TBH I don't remember why I didn't just add a getPermissionTable method to the settings API, as said in comment 6. The reason for that might be someplace in bug 812289 or bug 817034.

Comment 9

[Stephanie Ouillon [:arroway] (needinfo me)]

I updated permissions_table.json to match PermissionsTable.jsm:
https://github.com/mozilla-b2g/gaia/pull/14366

(I didn't find the video-capture permission, though)

Comment 10

[Stephanie Ouillon [:arroway] (needinfo me)]

Attachment: update_permission.patch

Comment 11

[Paul Theriault [:pauljt] (no longer reading bugmail)]

(In reply to Stéphanie Ouillon [:arroway] from comment #9)
> I updated permissions_table.json to match PermissionsTable.jsm:
> https://github.com/mozilla-b2g/gaia/pull/14366
> 
> (I didn't find the video-capture permission, though)

It 'video-capture' but it hasn't landed yet (see the patches in bug 853356).

Comment 12

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Specifically this one: https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=853356&attachment=827240

Comment 13

[Stephanie Ouillon [:arroway] (needinfo me)]

So should we have two separate patches: one now, and another one adding 'video-capture' when the code in gecko will have landed?

Comment 14

[Jason Smith [:jsmith]]

(In reply to Stéphanie Ouillon [:arroway] from comment #13)
> So should we have two separate patches: one now, and another one adding
> 'video-capture' when the code in gecko will have landed?

Technically video-capture support is coming very shortly in 1.3, so it's safe to land this on master. In the absolute worst case that we decide to move gUM camera to 1.4, we could just back out the video capture support from the patch later.

Comment 15

[Stephanie Ouillon [:arroway] (needinfo me)]

Attachment: update_permission_v2.patch

Ok, so I updated the patch to add the video-capture permission:

Pull request: https://github.com/mozilla-b2g/gaia/pull/14453

Comment 16

[Jason Smith [:jsmith]]

(In reply to Stéphanie Ouillon [:arroway] from comment #15)
> Created attachment 8343633 [details] [diff] [review]
> update_permission_v2.patch
> 
> Ok, so I updated the patch to add the video-capture permission:
> 
> Pull request: https://github.com/mozilla-b2g/gaia/pull/14453

Do you need a reviewer for this pull request?

Comment 17

[Stephanie Ouillon [:arroway] (needinfo me)]

It's a json file, I've just added permission names so I don't think there is need of a code review.

Comment 18

[Jason Smith [:jsmith]]

(In reply to Stéphanie Ouillon [:arroway] from comment #17)
> It's a json file, I've just added permission names so I don't think there is
> need of a code review.

Actually, a code review is required on every commit to the Mozilla codebase, even if it's dirt simple config file change, pref change, etc. So I'd flag kaze for review on your github pull request. It'll likely be a very quick review, however.

Comment 19

[Jason Smith [:jsmith]]

Switching this to be dependent on bug 853356, as bug 853356 can land without this implemented.

Note - the dependent code in bug 853356 has landed outside of the automated test, so this is safe to land now. It would be great if we could get this landed before Monday's merge.

Comment 20

[Fabien Cazenave [:kaze]]

Comment on attachment 8343633 [details] [diff] [review]
update_permission_v2.patch

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

LGTM

Comment 21

[Jason Smith [:jsmith]]

This is ready to land. John - Can you land this?

https://github.com/mozilla-b2g/gaia/pull/14453

Comment 22

[John Ford [:jhford] CET/CEST Berlin Time]

This is failing travis.  Are the failures explained?

Comment 23

[Jason Smith [:jsmith]]

(In reply to John Ford [:jhford] -- please use 'needinfo?' instead of a CC from comment #22)
> This is failing travis.  Are the failures explained?

Redirecting this to arroway to find out what the test failures are.

Comment 24

[John Ford [:jhford] CET/CEST Berlin Time]

Both failures seem to be related to DNT, though I might be misreading the logs.


================================

TEST-START test_settings_do_not_track.py

test_enable_do_not_track_via_settings_app (test_settings_do_not_track.TestSettingsDoNotTrack)

Enable do not track via the Settings app ... ERROR

======================================================================

ERROR: None

----------------------------------------------------------------------

Traceback (most recent call last):

File "/usr/local/lib/python2.7/dist-packages/marionette_client-0.6.2-py2.7.egg/marionette/marionette_test.py", line 143, in run

testMethod()

File "/home/travis/build/mozilla-b2g/gaia/tests/python/gaia-ui-tests/gaiatest/tests/functional/settings/test_settings_do_not_track.py", line 26, in test_enable_do_not_track_via_settings_app

do_not_track_settings.tap_disallow_tracking()

File "/home/travis/build/mozilla-b2g/gaia/tests/python/gaia-ui-tests/gaiatest/apps/settings/regions/do_not_track.py", line 22, in tap_disallow_tracking

el.tap()

File "/usr/local/lib/python2.7/dist-packages/marionette_client-0.6.2-py2.7.egg/marionette/marionette.py", line 83, in tap

return self.marionette._send_message('singleTap', 'ok', id=self.id, x=x, y=y)

File "/usr/local/lib/python2.7/dist-packages/marionette_client-0.6.2-py2.7.egg/marionette/marionette.py", line 577, in _send_message

self._handle_error(response)

File "/usr/local/lib/python2.7/dist-packages/marionette_client-0.6.2-py2.7.egg/marionette/marionette.py", line 604, in _handle_error

raise ElementNotVisibleException(message=message, status=status, stacktrace=stacktrace)

TEST-UNEXPECTED-FAIL | test_settings_do_not_track.py test_settings_do_not_track.TestSettingsDoNotTrack.test_enable_do_not_track_via_settings_app | ElementNotVisibleException: Element is not currently visible and may not be manipulated

----------------------------------------------------------------------

Ran 1 test in 10.350s

FAILED (errors=1)

TEST-START test_settings_wallpaper.py

test_change_wallpaper (test_settings_wallpaper.TestWallpaper) ... ok

----------------------------------------------------------------------

Ran 1 test in 23.849s

OK

Comment 25

[Stephanie Ouillon [:arroway] (needinfo me)]

I relaunched the tests after a rebase, Travis says everything is fine now.

Comment 26

[John Ford [:jhford] CET/CEST Berlin Time]

master: c17ce6040081294f8b6b3ed52decd6788da20dce

Comment 27

[Jason Smith [:jsmith]]

We need back this out - this doesn't work in the GRANT case per bug 978660, so there's no point in enabling the UI right now.

John - Can you back this out?

Comment 28

[John Ford [:jhford] CET/CEST Berlin Time]

Backed out with master: 6df4533f14ec2645bb13d8a803a5151583ca13a8

Comment 29

[Paul Theriault [:pauljt] (no longer reading bugmail)]

So I am not sure just hiding the UI is the answer here - all that does is exchange one bad UX for another. (In reply to Jason Smith [:jsmith] from comment #27)
> We need back this out - this doesn't work in the GRANT case per bug 978660,
> so there's no point in enabling the UI right now.
> 
> John - Can you back this out?

Really not sure that this was the right decision here. It might make sense to not have a broken UI in the grant case, but hiding this a) users can no longer forbid apps from asking for camera/microphone by setting the permission to DENY, and b) we are inconsistent with our UX that all user security decisions should be reversible. ( I know it doesn't currently matter for getUserMedia, since we prompt always, but I am still trying to understand why we still have this UX. I'm guess its a result of moved 1.4 deadline, but I would much rather see this fixed rather than hidden for 1.4.

Comment 30

[Jason Smith [:jsmith]]

(In reply to Paul Theriault [:pauljt] from comment #29)
> So I am not sure just hiding the UI is the answer here - all that does is
> exchange one bad UX for another.

This is an incomplete feature. I don't think it makes sense to expose a UI that does not actually work. This is certainly worthwhile fixing, but if we are going to turn a UI on, then it needs to actually work.

> (In reply to Jason Smith [:jsmith] from comment #27)
> > We need back this out - this doesn't work in the GRANT case per bug 978660,
> > so there's no point in enabling the UI right now.
> > 
> > John - Can you back this out?
> 
> Really not sure that this was the right decision here. It might make sense
> to not have a broken UI in the grant case, but hiding this a) users can no
> longer forbid apps from asking for camera/microphone by setting the
> permission to DENY, and b) we are inconsistent with our UX that all user
> security decisions should be reversible. ( I know it doesn't currently
> matter for getUserMedia, since we prompt always, but I am still trying to
> understand why we still have this UX. I'm guess its a result of moved 1.4
> deadline, but I would much rather see this fixed rather than hidden for 1.4.

[a] & [b] was technically never supported in any release we've shipped across multiple platforms that we've shipped gUM support on previously. On that regard, I don't think this is a blocker for the release, as past shipments of gUM never justified a need to have this requirement.

Comment 31

[Ivan Tsay (:ITsay)]

Fred will help to check the existing patch and re-land it after he finishes bug 938467 in 2.0.

Comment 32

[[e-mail]]

Attachment: pull request redirect to github

I'd like to land the patch with bug 938467 since these 2 issues should be bonded.

please help review the settings part 
apps/settings/resources/permissions_table.json

Comment 33

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8422172 [details] [review]
pull request redirect to github

Looks good to me for the settings part.

Comment 34

[Ivan Tsay (:ITsay)]

This feature is needed for gUM in Firefox OS 2.0.

Comment 35

[[e-mail]]

merged with bug 938467