Allow developers to revoke all permissions apps in the settings

RESOLVED FIXED in Firefox OS v2.1

Status

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

People

(Reporter: freddyb, Assigned: freddyb)

Tracking

({dev-doc-complete})

unspecified
2.1 S3 (29aug)
dev-doc-complete
Dependency tree / graph

Firefox Tracking Flags

(b2g-v2.1 fixed)

Details

Attachments

(1 attachment)

It's a valuable feature for developers and power users alike to revoke a wider set of permissions in the "Apps permissions" settings. It's arguably also a bigger footgun.

I propose we allow this given a specific checkbox is checked in the Developer settings.
Arthur suggested we loop in UX.
Jenny, do you think we need new UX, given this is a developer feature?
Flags: needinfo?(jelee)
I have been looking into the code lines in the settings app (especially apps/settings/js/panels/app_permissions_detail/app_permissions_detail.js#L74 and apps/settings/resources/permissions_table.json), but I'm not entirely sure where the filtering happens.

There is some filtering based on isExplicit, but this isn't what were looking for, right?

Comment 3

4 years ago
(In reply to Frederik Braun [:freddyb] from comment #1)
> Arthur suggested we loop in UX.
> Jenny, do you think we need new UX, given this is a developer feature?

I think it's fine to go ahead and add this checkbox in Developer section. Tks!
Flags: needinfo?(jelee)
Keywords: dev-doc-needed
Created attachment 8470003 [details] [review]
pull request (first attempt)

This is a first prototype, to show how I'd solve this.
I'm especially unsure about how to notify the panel of a settings change and whether to refresh it. 
(See https://github.com/mozfreddyb/gaia/compare/dev-permissions?expand=1#diff-e1e59a664daa46d3ffd67195db21934aL39).

Feedback appreciated.
Attachment #8470003 - Flags: feedback?(gduan)
Comment on attachment 8470003 [details] [review]
pull request (first attempt)

Hi Freddy, I would suggest to add the third parameter `verbose` to `showAppDetails` so that you could determine whether to show the verbose app permissions or not every time you display the panel.

Note that 'onInit' is called only once before the first call to `onBeforeShow`, so there is only one `permissionDetailModule` created and being used until the app is closed.
Attachment #8470003 - Flags: feedback?(gduan)
Arthur, can you take another look?
For certified apps I'm running into a lot of 'Failed to set the ... permission.' warnings.
I think there are two ways to handle this:
First, don't allow developers to revoke implicit permissions for the certified(=built in) apps.
Second, we should expose a warning that the permission change didn't work out. Maybe tint the select-element with a reddish background?

I removed the previous commit, see https://github.com/mozilla-b2g/gaia/pull/22672
Flags: needinfo?(arthur.chen)
pauljt argues that we should only do this for privileged apps. I agree that side-loaded certified apps are not a big issue, since developers who side-load certified apps have the possibility to audit and amend permissions before installing them.
The patch looks good to me. For the implicit permissions for the certified apps which option is better for your use case? Or we simply hide the implicit permissions?
Flags: needinfo?(arthur.chen)
Comment on attachment 8470003 [details] [review]
pull request (first attempt)

I have excluded certified apps and rebased again. This should work.
Arthur: Can you do a last review?

Try run through github hooks at https://tbpl.mozilla.org/?rev=bee3e24557c3c6f52eaca5c7cdb4aa7443bdd81c&tree=Gaia-Try
Attachment #8470003 - Flags: review?(arthur.chen)
We may want to sort the new l10n properties differently and hint that those strings are (for now?) only seen by developers which activate a specific setting.
Comment on attachment 8470003 [details] [review]
pull request (first attempt)

Thanks for the patch! Please check my comments in github.

As for the sorting of the newly added string ids, I flag l10n team for their inputs.
Attachment #8470003 - Flags: review?(arthur.chen) → feedback?(community)
Comment on attachment 8470003 [details] [review]
pull request (first attempt)

:l10n is an alias for the community of localizers, it shouldn't be used for actionable items like needinfo, feedback, or review.

See
https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_best_practices#Bugzilla_and_l10n
Attachment #8470003 - Flags: feedback?(community)
I left a couple of comments on Github, but they're actually about copy, not localizability.

I don't see anything that requires l10n feedback here: as long as you add the IDs in the .properties file, they're not duplicated, and they have comments in case the string has unclear variables, you're good to go ;-)

In case you have doubts feel free to NI me or :pike directly.
Comment on attachment 8470003 [details] [review]
pull request (first attempt)

can you take another look, arthur?
Attachment #8470003 - Flags: review?(arthur.chen)
Comment on attachment 8470003 [details] [review]
pull request (first attempt)

r=me with the nits addressed, thanks!
Attachment #8470003 - Flags: review?(arthur.chen) → review+
greenish on try (red Gij because of other code that has been fixed but in the meantime and isn't part of this PR): https://hg.mozilla.org/integration/gaia-try/rev/656b83c379f5
Keywords: checkin-needed
Master: https://github.com/mozilla-b2g/gaia/commit/86babec46643b0e5a23c213c0f96286b71b7c350
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-b2g-v2.1: --- → fixed
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S3 (29aug)
Chris, can you review if this clears the dev-doc-needed flag?
https://developer.mozilla.org/en-US/Firefox_OS/Debugging/Developer_settings$compare?to=657477&from=652847


A longer write-up of this feature has been posted on my personal blog: https://frederik-braun.com/revoking-permissions-on-firefox-os.html
Flags: needinfo?(cmills)
Thanks for the addition! I have modified the text slightly, to add a bit more explanation. I just tested the feature on my Flame, and it makes sense to me. Is this ok?
Flags: needinfo?(cmills)
Keywords: dev-doc-needed → dev-doc-complete
Thanks Chris. Having an example makes sense!

Geolocation is already always present and defaults to "Prompt" for all apps (even certified). I will change this to "Schedule Alarms" and add a note that some apps may not be able to cope with the permission change and might show unexpected behavior (which can be fixed by changing the permission to it's original value or re-installing the app)
(In reply to Frederik Braun [:freddyb] from comment #20)
> Thanks Chris. Having an example makes sense!
> 
> Geolocation is already always present and defaults to "Prompt" for all apps
> (even certified). I will change this to "Schedule Alarms" and add a note
> that some apps may not be able to cope with the permission change and might
> show unexpected behavior (which can be fixed by changing the permission to
> it's original value or re-installing the app)

Ah, cool - that example makes more sense, thanks!
You need to log in before you can comment on or make changes to this bug.