1058150 - Use a restricted profile for guest mode

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Wesley Johnston (:wesj)]

We've got a bunch of Guest mode checks in Fennec right now that would probably be better served by unifying with the restricted profile code (i.e. disabling downloads/addons/sharing/etc in guest mode).

As a first step, we can just make these things disabled if you're in a restricted profile. If we want later, we can come back and add ways to opt in or out.

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

We have nsIParentalControlsService which can be used in the code to limit functionality based on Android restricted profiles. We can extend this impl to also look for a Guest Session and treat it as restricted too. 

We would add a call to see if we are in a Guest Session here:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/parentalcontrols/nsParentalControlsServiceAndroid.cpp#17

Comment 2

[Wesley Johnston (:wesj)]

Attachment: Patch

This switches our current guest mode checks to all use a basic ParentalControls.parentalControlsEnabled (argh, that naming!). It forces them enabled by adding some checks in GeckoAppShell, one of which returns some (fake) restrictions for guest mode. Those aren't used anywhere right now, so we don't really need 'em (but I also fixed the test for this to use the new JNI.jsm implementation). Just added for "completeness".

With this we should be able to remove our old download check, but I noticed that the nsIParentalControls implementation throws from that method, so I expect it doesn't work. Updating these individual callers to use more specific filters is a separate bug anyway.

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

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

In general I like this direction. You went a little further than I imagined, but it's good.

>diff --git a/mobile/android/base/GeckoAppShell.java b/mobile/android/base/GeckoAppShell.java

>     public static boolean isUserRestricted() {
>+        // Guest mode is supported in all Android versions
>+        if (getGeckoInterface().getProfile().inGuestMode()) {
>+            return true;
>+        }
>+

I'm not sure I like that we lose the ability to tell if the user is restricted because of Android or because of Guest. I was expecting a separate JNI wrapper to return the Guest boolean. I liked having the distinction available here, but mixing the two in nsIParentalControlsService. We could change this method to return an Enum instead. Or we could add a separate JNI wrapper. Thoughts?

>     public static String getUserRestrictions() {
>+        // Guest mode is supported in all Android versions
>+        if (getGeckoInterface().getProfile().inGuestMode()) {
>+            // TODO: Guest mode should enforce all known restrictions by default
>+            return "{ " +
>+              "\"no_install_apps\": true, " +
>+              "\"no_download_files\": true, " +
>+              "\"no_install_extensions\": true, " +
>+              "\"no_share\": true, " +
>+              "\"no_browse_files\": true, " +
>+            "}";
>+        }

I like this. Did you make these up or do some of these come from Android?

We might want to merge these with the Android restrictions too since I could be running a Guest session in a Restricted profile. But I guess if Guest is always more restrictive, it doesn't matter.

>diff --git a/mobile/android/base/tests/testRestrictedProfiles.js b/mobile/android/base/tests/testRestrictedProfiles.js

>-/*
>+
> // NOTE: JNI.jsm has no way to call a string method yet
> add_task(function test_getUserRestrictions() {

Hot dog! The new JNI.jsm has landed. You can remove the comment too.

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

I love that you are trying to remove the inGuest stuff. However, ...

>     NativeWindow.contextmenus.add({
>       label: Strings.browser.GetStringFromName("contextmenu.shareLink"),
>       order: NativeWindow.contextmenus.DEFAULT_HTML5_ORDER - 1, // Show above HTML5 menu items
>-      selector: NativeWindow.contextmenus._disableInGuest(NativeWindow.contextmenus.linkShareableContext),
>+      selector: NativeWindow.contextmenus._disableInRestrictedProfiles(NativeWindow.contextmenus.linkShareableContext),

Sharing should be allowed in Android Restricted profiles. This is the kid's data. As long as they have other apps installed, we should allow sharing.

Maybe we need to add something to nsIParentalControlsService? Maybe nsIParentalControlsService.isRestricted(String name) ?

That way we could use the restriction you have above, "no_share", and add:
NativeWindow.contextmenus._allowShare(NativeWindow.contextmenus.linkShareableContext)

>     NativeWindow.contextmenus.add({
>       label: Strings.browser.GetStringFromName("contextmenu.shareEmailAddress"),
>       order: NativeWindow.contextmenus.DEFAULT_HTML5_ORDER - 1,
>-      selector: NativeWindow.contextmenus._disableInGuest(NativeWindow.contextmenus.emailLinkContext),
>+      selector: NativeWindow.contextmenus._disableInRestrictedProfiles(NativeWindow.contextmenus.emailLinkContext),

Same

>     NativeWindow.contextmenus.add({
>       label: Strings.browser.GetStringFromName("contextmenu.sharePhoneNumber"),
>       order: NativeWindow.contextmenus.DEFAULT_HTML5_ORDER - 1,
>-      selector: NativeWindow.contextmenus._disableInGuest(NativeWindow.contextmenus.phoneNumberLinkContext),
>+      selector: NativeWindow.contextmenus._disableInRestrictedProfiles(NativeWindow.contextmenus.phoneNumberLinkContext),

Same

>     NativeWindow.contextmenus.add(Strings.browser.GetStringFromName("contextmenu.addToContacts"),
>-      NativeWindow.contextmenus._disableInGuest(NativeWindow.contextmenus.emailLinkContext),
>+      NativeWindow.contextmenus._disableInRestrictedProfiles(NativeWindow.contextmenus.emailLinkContext),

This should be allowed in Restricted profiles too. Maybe "no_contacts" ?

>     NativeWindow.contextmenus.add(Strings.browser.GetStringFromName("contextmenu.addToContacts"),
>-      NativeWindow.contextmenus._disableInGuest(NativeWindow.contextmenus.phoneNumberLinkContext),
>+      NativeWindow.contextmenus._disableInRestrictedProfiles(NativeWindow.contextmenus.phoneNumberLinkContext),

Same

>     NativeWindow.contextmenus.add(Strings.browser.GetStringFromName("contextmenu.bookmarkLink"),
>-      NativeWindow.contextmenus._disableInGuest(NativeWindow.contextmenus.linkBookmarkableContext),
>+      NativeWindow.contextmenus._disableInRestrictedProfiles(NativeWindow.contextmenus.linkBookmarkableContext),

Maybe "no_bookmarks"

>     NativeWindow.contextmenus.add({
>       label: Strings.browser.GetStringFromName("contextmenu.shareMedia"),
>       order: NativeWindow.contextmenus.DEFAULT_HTML5_ORDER - 1,
>-      selector: NativeWindow.contextmenus._disableInGuest(NativeWindow.contextmenus.SelectorContext("video")),
>+      selector: NativeWindow.contextmenus._disableInRestrictedProfiles(NativeWindow.contextmenus.SelectorContext("video")),

"no_share" again

>     NativeWindow.contextmenus.add({
>       label: Strings.browser.GetStringFromName("contextmenu.shareImage"),
>-      selector: NativeWindow.contextmenus._disableInGuest(NativeWindow.contextmenus.imageSaveableContext),
>+      selector: NativeWindow.contextmenus._disableInRestrictedProfiles(NativeWindow.contextmenus.imageSaveableContext),

"no_share"

>     NativeWindow.contextmenus.add(Strings.browser.GetStringFromName("contextmenu.setImageAs"),
>-      NativeWindow.contextmenus._disableInGuest(NativeWindow.contextmenus.imageSaveableContext),
>+      NativeWindow.contextmenus._disableInRestrictedProfiles(NativeWindow.contextmenus.imageSaveableContext),

"no_download_files" might be enough here

>-    _disableInGuest: function _disableInGuest(selector) {
>+    _disableInRestrictedProfiles: function _disableInRestrictedProfiles(selector) {

Either convert this to _isRestricted("no_XXX", selector) or create a _allowShare(selector) and _allowBookmark(selector) and ...

>-    // In guest sessions, we refuse to let you open any file urls.
>-    if (BrowserApp.isGuest) {
>+    // In restricted profiles, we refuse to let you open any file urls.
>+    if (ParentalControls.parentalControlsEnabled) {
>       let bannedSchemes = ["file", "chrome", "resource", "jar", "wyciwyg"];

If we add the nsIParentControlsService.isRestricted, you could pass in "no_download_files" or/and we could use that to impl the current nsIParentalControlsService method for downloading files.

>diff --git a/mobile/android/chrome/content/downloads.js b/mobile/android/chrome/content/downloads.js

>       case Ci.nsIDownloadManager.DOWNLOAD_QUEUED: {
>-        if (BrowserApp.isGuest) {
>+        if (ParentalControls.parentalControlsEnabled) {

Same

>           NativeWindow.toast.show(Strings.browser.GetStringFromName("downloads.disabledInGuest"), "long");

Does this string need to change if it's an Android Restricted profile? Maybe less "Guest" centric, more generic?

>-    try {
>-      guest = aCmdLine.handleFlag("guest", false);
>-    } catch (e) { /* Optional */ }

Does this mean we can stop adding the -guest flag to the command line in Java?

>diff --git a/mobile/android/modules/WebappManager.jsm b/mobile/android/modules/WebappManager.jsm

>   _installApk: function(aMessage, aMessageManager) { return Task.spawn((function*() {
>-    if (this.inGuestSession()) {
>+    if (ParentalControls.parentalControlsEnabled) {

Same thing about using "no_install_apps" restriction, if we add the method.

r-/f+ let's figure out how we want to move ahead

Comment 4

[Wesley Johnston (:wesj)]

(In reply to Mark Finkle (:mfinkle) from comment #3)
> nsIParentalControlsService. We could change this method to return an Enum
> instead. Or we could add a separate JNI wrapper. Thoughts?

My preference is what I did here. I don't understand why Gecko would need to know about GuestMode. If we need to know, my instinct is that that data should go in nsIToolkitProfile:

int profile.type = nsIToolkitProfile::GUEST_TYPE;

> I like this. Did you make these up or do some of these come from Android?
I think the install_apps one comes from Android. The rest I made up to match their style. Being able to use the Android setting seems neat for apps though :)

> Sharing should be allowed in Android Restricted profiles. This is the kid's
> data. As long as they have other apps installed, we should allow sharing.
> Maybe we need to add something to nsIParentalControlsService? Maybe
> nsIParentalControlsService.isRestricted(String name) ?

I was trying to keep this simple as possible at first. Blocking everything in every restricted profile and then adding it back as we want. My preference here would be to add a method onto nsIParentalControls like you said. Something like:

parentalControls.isAllowed(int action, [optional] nsIURI uri);

i.e. I think it would be good to have at least an nsIURI parameter here. Maybe nsISupports? That gives us the freedom to black/whitelist certain actions/uris. i.e. I can imagine a flow like:

if (!parentalControls.isAllowed(LOAD, uri)) {
  // show blocked page/dialog with a "request access" button
  button.onClick = function() {
    parentalControls.sendRequestOverride(LOAD, uri);
    // show a "request sent" page/dialog
  }
}

but seems further down the road (and requires updating the Windows code). If we add this "isAllowed" method, would it replace the already in place "blockFileDownloadsEnabled" attribute (who named this stuff?) How much cleanup/change to this interface do you want to undertake in this bug? In other bugs?

> Does this mean we can stop adding the -guest flag to the command line in
> Java?
Yeah. I left it... because. Will yank.

Comment 5

[Wesley Johnston (:wesj)]

Attachment: Patch 2/2

This adds changes to restricted profile to support an isAllowed(int, uri) method and uses it through Fennec (in both JS and Java code). It doesn't implement isAllowed for other platforms. I also moved the Java code for this into a RestrictedProfiles.java file (because I hate cluttering up GeckoAppShell with more crap).

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8480362 [details] [diff] [review]
Patch 2/2


>+++ b/mobile/android/base/RestrictedProfiles.java

>+    private static String geckoActionToRestrction(int action) {

typo: geckoActionToRestrction -> geckoActionToRestriction

>+    private static Bundle getRestrctions() {

typo: getRestrctions -> getRestrictions

>+    public static boolean isAllowed(int action, String url) {
>+        // ALl actions are blocked in Guest mode

typo: // All

>\ No newline at end of file

Add one

>diff --git a/mobile/android/base/tests/testRestrictedProfiles.js b/mobile/android/base/tests/testRestrictedProfiles.js

>+  const short DOWNLOAD = 1;
>+  const short INSTALL_EXTENSION = 2;
>+  const short INSTALL_APP = 3;
>+  const short VISIT_FILE_URLS = 4;
>+  const short SHARE = 5;
>+  const short BOOKMARK = 6;

What are these doing?

>-    var geckoAppShell = JNI.LoadClass(jenv, "org.mozilla.gecko.GeckoAppShell", {
>+    var geckoAppShell = JNI.LoadClass(jenv, "org.mozilla.gecko.RestrictedProfile", {

Change "geckoAppShell" variable to "profile" ?

>diff --git a/mobile/android/chrome/content/SelectionHandler.js b/mobile/android/chrome/content/SelectionHandler.js

>         matches: function() {
>+          if (!ParentalControls.isAllowed(ParentalControls.SHARE)) {

I assume ParentalControls is being pulled in from browser.js ?

>diff --git a/toolkit/components/parentalcontrols/nsIParentalControlsService.idl b/toolkit/components/parentalcontrols/nsIParentalControlsService.idl

You update the IDL

>diff --git a/toolkit/components/parentalcontrols/nsParentalControlsServiceAndroid.cpp b/toolkit/components/parentalcontrols/nsParentalControlsServiceAndroid.cpp

You only updated the Android Impl. I think we need to add stubs to the other Impls, right? And yes, I am fine with adding stubbed out Impls.

r+ with the nits and the stubs addressed.

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

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

Patch 2/2 adds the changes needed to get an r+ here too.

Comment 8

[Wesley Johnston (:wesj)]

to try: https://tbpl.mozilla.org/?tree=Try&rev=71479403b9d3

Comment 9

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/fx-team/rev/43e6d3d43b50

Comment 10

[Wesley Johnston (:wesj)]

Grr.. Lost my implementation for other platforms along the way. Landed a follow up:
https://hg.mozilla.org/integration/fx-team/rev/f3a47630d9a6

Comment 11

[Wesley Johnston (:wesj)]

Backout:
https://hg.mozilla.org/integration/fx-team/rev/642407825292

Relanded:
https://hg.mozilla.org/integration/fx-team/rev/bf52063ed95f

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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