Closed Bug 1189414 Opened 4 years ago Closed 4 years ago

Restricted Profiles: Clean up missing and unneeded restrictions

Categories

(Firefox for Android :: Profile Handling, defect)

All
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 42
Tracking Status
firefox42 --- verified

People

(Reporter: sebastian, Assigned: sebastian)

References

Details

Attachments

(5 files)

There's some cleanup work around restrictions left that I would like to get in v1 if possible:

* nsIParentalControlsService.idl is not really in sync with Restriction.java

* We introduced a restriction DISALLOW_TOOLS_MENU but this doesn't really make sense. We are actually disabling some features that are shown in the tools menu so we should hide menu items based on this and not have a restriction just to hide a menu entry. This also makes more sense from a parent's point of view: They want to disable features and not hide menus, not knowing what's in there. And right now with hiding the tools menu we are hiding features that are not disabled (Downloads, Logins).

* In bug 1125313 we introduced a restriction to hide the "report site issue" menu item: DISALLOW_REPORT_SITE_ISSUE. This add-on is only in Nightly and having a settings to enable/disable this restriction doesn't make any sense. We can still hide this for restricted profiles in Nightly but it shouldn't be a configurable restriction.
Also:

* DISALLOW_INSTALL_APPS / no_install_apps is a restriction that is imposed by the system. So we shouldn't make it configurable because we can't enable this from our side.
Depends on: 1180653
I'd like to get these cleanup patches into v1. Especially because it removes some unnecessary restrictions from the admin's settings.
Blocks: kidfox-v1
Status: NEW → ASSIGNED
Comment on attachment 8642396 [details] [diff] [review]
1189414-part5-aboutpages.patch

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

nice. I'm not sure this is necessary per se, but it does make reading aboutPages.java easier in the end.
Attachment #8642396 - Flags: review?(ally) → review+
Attachment #8642376 - Flags: review?(ally) → review+
Comment on attachment 8642374 [details] [diff] [review]
1189414-part3-reportsiteissue.patch

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

::: toolkit/components/parentalcontrols/nsIParentalControlsService.idl
@@ +10,5 @@
>  interface nsIFile;
>  interface nsIInterfaceRequestor;
>  interface nsIArray;
>  
> +[scriptable, uuid(ed14d186-e902-4d41-86cb-8949fd7b53d7)]

So these 3 patches have changes to the nsIParentalControlsService.idl. I'd like to seem them all rolled into one patch to reduce churn on the idl and not rev the uuid unnecessarily.
Attachment #8642374 - Flags: review?(ally) → review+
Comment on attachment 8642372 [details] [diff] [review]
1189414-part2-tools.patch

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

::: mobile/android/base/BrowserApp.java
@@ +3377,5 @@
>          } else {
>              enterGuestMode.setVisible(true);
>          }
>  
> +        if (!RestrictedProfiles.isAllowed(this, Restriction.DISALLOW_GUEST_BROWSING)) {

Why did you decide to move this code down here?
Attachment #8642372 - Flags: review?(ally) → review+
Comment on attachment 8642370 [details] [diff] [review]
1189414-part1-synchronize.patch

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

see comment about combining patches with changes to the .idl file. :)
Attachment #8642370 - Flags: review?(ally) → review+
(In reply to Allison Naaktgeboren please NEEDINFO? :ally from comment #10)
> So these 3 patches have changes to the nsIParentalControlsService.idl. I'd
> like to seem them all rolled into one patch to reduce churn on the idl and
> not rev the uuid unnecessarily.

Okay. I'll collapse all patches here into one changeset.


(In reply to Allison Naaktgeboren please NEEDINFO? :ally from comment #11)
> Why did you decide to move this code down here?


I saw that some menu items can be disabled/hidden because of other reasons. So I just want to hide the item if the restriction is enabled and never show it based on that (because it might already be hidden because of other decisions).
url:        https://hg.mozilla.org/integration/fx-team/rev/7e98222202e4f9079cba2ccda07c624516d13a77
changeset:  7e98222202e4f9079cba2ccda07c624516d13a77
user:       Sebastian Kaspari <s.kaspari@gmail.com>
date:       Thu Aug 06 10:51:45 2015 +0200
description:
Bug 1189414 - Restricted Profiles: Clean up missing and unneeded restrictions. r=ally

* Synchronize nsIParentalControlsService.idl and Restriction.java
* Do not hide 'tools' menu but menu items of disabled features
* Hiding 'Report site issue' should not be configurable
* Restricted profiles: DISALLOW_INSTALL_APPS is a system restriction and should not be configurable
* RestrictedProfileConfiguration: Use AboutPages
https://hg.mozilla.org/mozilla-central/rev/7e98222202e4
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Verified as fixed on Nightly and Aurora: 
Tools menu cannot be hidden, only the menu items (Add-ons and guest browsing)
'Report site issue' was removed from the restriction options list
'Install Apps' was removed from the restriction options list
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.