1125290 - KidFox: Restricted profiles - Hide private browsing support and all related UI

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Profile Handling, defect)

Comment 1

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: 1125290-private-browsing-WIP.patch

WIP patch based on the changes in bug 1187260.

This patch:
* Hides "New Private Tab" from the menu
* Hides the "Normal / Private tabs" button from the tab switcher
* Hide the "New Private Tab" option from the tab switcher menu
* Hides setting: "Customize > Open links in private browsing"
* Hides setting: "Privacy > Tracking protection"

Comment 2

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: 1125290-private-browsing.patch

Final patch for review.

Comment 3

[Sebastian Kaspari (:sebastian; :pocmo)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff2c293295d8

Comment 4

[:Margaret Leibovic]

Comment on attachment 8639876 [details] [diff] [review]
1125290-private-browsing.patch

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

::: mobile/android/base/BrowserApp.java
@@ +3357,5 @@
>          final boolean toolsVisible = RestrictedProfiles.isAllowed(this, Restriction.DISALLOW_TOOLS_MENU);
>          MenuUtils.safeSetVisible(aMenu, R.id.tools, toolsVisible);
>  
> +        final boolean privateTabVisible = RestrictedProfiles.isAllowed(this, Restriction.DISALLOW_PRIVATE_BROWSING);
> +        MenuUtils.safeSetVisible(aMenu, R.id.new_private_tab, privateTabVisible);

We should also make sure we disable context menus, e.g.:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#693

Comment 5

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8639876 [details] [diff] [review]
1125290-private-browsing.patch

(In reply to :Margaret Leibovic from comment #4)
> We should also make sure we disable context menus, e.g.:
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/
> browser.js#693

Good point! :) I'll update the patch.

Comment 6

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: 1125290-private-browsing-v2.patch

This patch additional hides:
* "Open Link in Private Tab" from link context menus
* "Open in private tab" from Top Sites context menu
* "Open in private tab" from HomeFragment context menus (Bookmarks, Readling List, History, Recent tabs)

Comment 7

[:Margaret Leibovic]

Comment on attachment 8640449 [details] [diff] [review]
1125290-private-browsing-v2.patch

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

This looks fine to me, but my main fear is that there's something we're missing.

I just realized there's also a link to open a new private tab in about:privatebrowsing. We should probably add a check to just prevent being able to load that page altogether. But maybe we should add a sanity check case somewhere in our tab creation logic to throw an error if we're trying to open a private tab in a restricted profile, to at least prevent it from ever happening.

I'm giving this an r+, but please add a check to prevent loading about:privatebrowsing, and consider a follow-up to add an assertion closer to the core of private tabs.

::: mobile/android/base/restrictions/Restriction.java
@@ +11,5 @@
>  
>  /**
>   * This is a list of things we can restrict you from doing. Some of these are reflected in Android UserManager constants.
>   * Others are specific to us.
> + * These constants should be in sync with the ones from toolkit/components/parentalcontrols/nsIParentalControlsService.idl

This comment tripped me up before! Thanks for fixing it :)

::: mobile/android/base/tabs/TabsPanel.java
@@ +4,5 @@
>   * You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  package org.mozilla.gecko.tabs;
>  
> +import org.mozilla.gecko.*;

We generally avoid global imports. Maybe your IDE this this automatically? You should fix this before landing.

::: toolkit/components/parentalcontrols/nsIParentalControlsService.idl
@@ +29,5 @@
>    const short REMOTE_DEBUGGING = 10; // Remote debugging
>    const short IMPORT_SETTINGS = 11; // Importing settings from other apps
>    const short TOOLS_MENU = 12; // Hide tools menu entry
>    const short REPORT_SITE_ISSUE = 13; // Hide "Report Site Issue" menu entry
> +  const short PRIVATE_BROWSING = 16; // Disallow usage of private browsing

We should update the uuid for this interface.

Comment 8

[Allison Naaktgeboren :ally]

Comment on attachment 8639876 [details] [diff] [review]
1125290-private-browsing.patch

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

Margaret already beat me to the big bit of feedback. We need to cover the context menu in js :) 

This is one we have to be most through in testing.

Comment 9

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to :Margaret Leibovic from comment #7)
> I just realized there's also a link to open a new private tab in
> about:privatebrowsing. We should probably add a check to just prevent being
> able to load that page altogether. But maybe we should add a sanity check
> case somewhere in our tab creation logic to throw an error if we're trying
> to open a private tab in a restricted profile, to at least prevent it from
> ever happening.

Oh, I didn't know about about:privatebrowsing. Will take care of that. My biggest fear is that new features or components will not have these checks in the future. I guess I'm going to write to the mailing list to get people aware of that and to source some ideas how we can prevent this to break in the future.


(In reply to :Margaret Leibovic from comment #7)
> I'm giving this an r+, but please add a check to prevent loading
> about:privatebrowsing, and consider a follow-up to add an assertion closer
> to the core of private tabs.

Alright. Will update the patch and file a follow-up.


(In reply to :Margaret Leibovic from comment #7)
> > +import org.mozilla.gecko.*;
> 
> We generally avoid global imports. Maybe your IDE this this automatically?
> You should fix this before landing.

Argh, Intellij! I fixed a couple of these in different patches but it keeps doing that.


(In reply to :Margaret Leibovic from comment #7)
> We should update the uuid for this interface.

Oh yeah, I remember that there's a push-hook preventing you from pushing this without updating the uuid. Will take care of that too.

Comment 10

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: 1125290-private-browsing-v3.patch

Updated patch addressing the review comments.

Comment 11

[Sebastian Kaspari (:sebastian; :pocmo)]

url:        https://hg.mozilla.org/integration/fx-team/rev/8bdfb76f8a081d570dd6cbbb4f3e73bbe38ee204
changeset:  8bdfb76f8a081d570dd6cbbb4f3e73bbe38ee204
user:       Sebastian Kaspari <[e-mail]>
date:       Thu Jul 30 19:55:18 2015 +0200
description:
Bug 1125290 - Hide private browsing support and all related UI. r=margaret

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8bdfb76f8a08

Comment 13

[Catalin Suciu [:csuciu]]

When the 'owner' disables 'Private Browsing', the following options are hidden in restricted profiles:
- "New Private Tab" from the menu
- "Normal / Private tabs" button from the tab switcher
- "New Private Tab" option from the tab switcher menu
- "Customize > Open links in private browsing"
- "Privacy > Tracking protection"

Verifying as fixed.