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

RESOLVED FIXED in 2.0 S3 (6june)

Status

Firefox OS
Gaia::Settings
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: pauljt, Assigned: gasolin)

Tracking

(Blocks: 1 bug)

unspecified
2.0 S3 (6june)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(feature-b2g:2.0)

Details

(Whiteboard: [WebRTC][blocking-webrtc-][b2g-gum+][p=1]ft:loop)

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

4 years ago
+++ 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)

Updated

4 years ago
Blocks: 918876
(Reporter)

Comment 1

4 years ago
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
(Reporter)

Comment 2

4 years ago
Im going to leave this open assigned to me to test it though.
Assignee: nobody → ptheriault
(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/
(Reporter)

Comment 4

4 years ago
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.
Assignee: ptheriault → nobody
(Reporter)

Comment 5

4 years ago
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
Assignee: nobody → stephouillon
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
(Reporter)

Comment 7

4 years ago
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)
(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.
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)
Created attachment 8342432 [details] [diff] [review]
update_permission.patch
(Reporter)

Comment 11

4 years ago
(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).
(Reporter)

Comment 12

4 years ago
Specifically this one: https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=853356&attachment=827240
So should we have two separate patches: one now, and another one adding 'video-capture' when the code in gecko will have landed?
(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.
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
Attachment #8342432 - Attachment is obsolete: true
(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?
It's a json file, I've just added permission names so I don't think there is need of a code review.
(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.
Attachment #8343633 - Flags: review?(kaze)
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.
No longer blocks: 853356
Depends on: 853356
Comment on attachment 8343633 [details] [diff] [review]
update_permission_v2.patch

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

LGTM
Attachment #8343633 - Flags: review?(kaze) → review+
This is ready to land. John - Can you land this?

https://github.com/mozilla-b2g/gaia/pull/14453
Flags: needinfo?(jhford)
This is failing travis.  Are the failures explained?
Flags: needinfo?(jhford) → needinfo?(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.
Flags: needinfo?(jsmith) → needinfo?(stephouillon)
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
I relaunched the tests after a rebase, Travis says everything is fine now.
Flags: needinfo?(stephouillon)
Flags: needinfo?(jhford)
Keywords: checkin-needed
master: c17ce6040081294f8b6b3ed52decd6788da20dce
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Flags: needinfo?(jhford)
Keywords: checkin-needed
Resolution: --- → FIXED

Updated

4 years ago
Keywords: verifyme
QA Contact: jsmith

Updated

4 years ago
QA Contact: jsmith

Updated

4 years ago
Depends on: 978660
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?
Flags: needinfo?(jhford)
Keywords: verifyme
Backed out with master: 6df4533f14ec2645bb13d8a803a5151583ca13a8
Flags: needinfo?(jhford)

Updated

4 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Reporter)

Comment 29

4 years ago
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.
blocking-b2g: --- → 1.4?
(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.
blocking-b2g: 1.4? → ---

Updated

4 years ago
Blocks: 938467

Comment 31

4 years ago
Fred will help to check the existing patch and re-land it after he finishes bug 938467 in 2.0.
Assignee: stephouillon → gasolin
Whiteboard: [WebRTC][blocking-webrtc-][b2g-gum+] → [WebRTC][blocking-webrtc-][b2g-gum+][p=1]
Target Milestone: --- → 2.0 S2 (23may)
(Assignee)

Comment 32

4 years ago
Created attachment 8422172 [details] [review]
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
Attachment #8422172 - Flags: review?(arthur.chen)
Comment on attachment 8422172 [details] [review]
pull request redirect to github

Looks good to me for the settings part.
Attachment #8422172 - Flags: review?(arthur.chen) → review+

Comment 34

4 years ago
This feature is needed for gUM in Firefox OS 2.0.
feature-b2g: --- → 2.0

Updated

3 years ago
Whiteboard: [WebRTC][blocking-webrtc-][b2g-gum+][p=1] → [WebRTC][blocking-webrtc-][b2g-gum+][p=1]ft:loop

Updated

3 years ago
Target Milestone: 2.0 S2 (23may) → 2.0 S3 (6june)
(Assignee)

Comment 35

3 years ago
merged with bug 938467
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.