896121 - [guest] Disable 'share' menus in guest mode

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

We do not want a guest to be able to share data using the owner's share apps accounts. We should disable the menus. We should even add a fail-safe in the core sharing code to make sure it doesn't slip through.

https://wiki.mozilla.org/Mobile/Projects/Guest_mode

Comment 1

[Wesley Johnston (:wesj)]

Attachment: patch

Again, depends on the patch in bug 871863....

Comment 2

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

Comment on attachment 779479 [details] [diff] [review]
patch

I don't think this handles the other share menus, right?

Comment 3

[Wesley Johnston (:wesj)]

Attachment: Patch

This disables the share menus everywhere.

We don't actually support disabling contextmenu items (that aren't html5), so this adds the ability to return a string "disabled" from your selector (rather than the normal true or false). I like adding the ability, and I like showing these disabled rather than just hiding them if we're in guest mode, but it does feel a bit like a hack.

Comment 4

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

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

You said you would re-do this patch to be less complex and just hide menus. Other feedback:

* Since we have "docShell.usePrivateBrowsing" and we use "isPrivate" in browser.js a lot, let use BrowserApp.isGuest
* In BrowserCLH, let's drop the "mode" and just use "guest". That includes the passed param, rename "guest-mode" to "guest".

If we used the long form more often (usePrivateBrowsing) I would suggest using isGuestBrowsing.

Comment 5

[Wesley Johnston (:wesj)]

Attachment: Patch 1/2

This is the basic feature. Disables sharing and saving. For JS context menu items, the items are hidden entirely. The native UI items are just disabled.

Comment 6

[Wesley Johnston (:wesj)]

Attachment: Patch 2/2

This adds the disabled piece back for js context menu items.

Comment 7

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

Comment on attachment 784838 [details] [diff] [review]
Patch 1/2

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

>         menu.findItem(R.id.open_new_tab).setVisible(!isPrivate);
>         menu.findItem(R.id.open_private_tab).setVisible(isPrivate);
>+        menu.findItem(R.id.share).setEnabled(GeckoProfile.get(mContext).inGuestMode());

I think you want (!GeckoProfile.get(mContext).inGuestMode());

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js
>     NativeWindow.contextmenus.add(Strings.browser.GetStringFromName("contextmenu.shareLink"),
>-      NativeWindow.contextmenus.linkShareableContext,
>+      NativeWindow.contextmenus._disableInGuestMode(NativeWindow.contextmenus.linkShareableContext),

I wonder how complicated this will become when we have 2 or 3 more boolean states

>+    _disableInGuestMode: function _disableInGuestMode(selector) {

_disableInGuestMode -> disableInGuest or disableInGuestBrowsing

>+      return {
>+        matches: function _disableInGuestModeMatches(aElement, aX, aY) {
>+          let result = selector.matches(aElement, aX, aY);
>+          if (result && BrowserApp.isGuest)

Maybe you should do the isGuest check first and return early

>diff --git a/mobile/android/components/BrowserCLH.js b/mobile/android/components/BrowserCLH.js

>+      guest = aCmdLine.handleFlag("guest-mode", false);


Where is this passed to Gecko? Also "guest-mode" -> "guest"

Comment 8

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

I checked in with Ian about showing diabled "Share" menus or just hiding "Share" menus:

<mfinkle> ibarlow, for guest browsing, do we want the show disabled "Share" menus everywhere
          or just hide them?
          we typically don't show disabled context menus
<ibarlow> mfinkle: probably hide them yeah

So when you update Part 1, remember to hide the Share menus, not just disable. We need Part 2 at this time.

Comment 9

[Wesley Johnston (:wesj)]

(In reply to Mark Finkle (:mfinkle) from comment #7)
> I think you want (!GeckoProfile.get(mContext).inGuestMode());
Doh!

> Maybe you should do the isGuest check first and return early

Good idea. This was just fallout from my disable idea...

> Where is this passed to Gecko? Also "guest-mode" -> "guest"
This is the same arg from here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java#1816

I'll update those as well....

Comment 10

[Wesley Johnston (:wesj)]

Attachment: Patch

Updated to hide the menu items. Had to move the GuestProfile check in GeckoProfile.get because the Awesomescreen is not a GeckoApp.

Comment 11

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

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

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

>+    private static final String GUEST_MODE_ARGUMENT = "--guest";

GUEST_MODE_ARGUMENT -> GUEST_BROWSING_ARG

>         // Disable save as PDF for about:home and xul pages
>         saveAsPDF.setEnabled(!(tab.getURL().equals("about:home") ||
>                                tab.getContentType().equals("application/vnd.mozilla.xul+xml")));
>+        saveAsPDF.setVisible(!GeckoProfile.get(this).inGuestMode());

Do we want to turn off Save as PDF ?

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

>-      NativeWindow.contextmenus.linkShareableContext,
>+      NativeWindow.contextmenus._disableInGuestMode(NativeWindow.contextmenus.linkShareableContext),

_disableInGuestMode -> disableInGuest or disableInGuestBrowsing

>diff --git a/mobile/android/components/BrowserCLH.js b/mobile/android/components/BrowserCLH.js

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

You are assuming that the "guest" arg is passed to GeckoApp every time Fennec starts up. This only happens the first time you switch to a guest session. If there is an OOM or the user swipes close via recent apps, and then starts again, we are still in Guest Browsing, but the "guest" arg will not be sent.

I think you could add "guest" here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoThread.java#104

Can't r+ without making sure we handle all the "starting in guest session" cases.

Comment 12

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

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

Not needed just yet

Comment 13

[Wesley Johnston (:wesj)]

Attachment: Patch v3

Good catch. This passes the guest argument in GeckoThread.

I also realized I made a last minute mistake in the session restore patch. I moved the getGuestSession check from GeckoProfile.get outside of the if (context instanceof GeckoApp) check for some real reason, but that means that we aren't hitting the guest section part anymore. I can't really think of any time when we're not in a GeckoApp that we care about guest mode, so this new one does:

if we're in GeckoApp
  if we have cached session, return it
  if we're in BrowserApp and in guest mode, return it (don't do this for webapps!)
  return the default session for this app
else return GeckoProfile's default.

Comment 14

[Wesley Johnston (:wesj)]

Also, I left in hiding "Save as PDF", but I'm not sure if we want it or not. It doesn't really affect "accounts", so maybe we do.

Comment 15

[Ian Barlow (:ibarlow)]

(In reply to Wesley Johnston (:wesj) from comment #14)
> Also, I left in hiding "Save as PDF", but I'm not sure if we want it or not.
> It doesn't really affect "accounts", so maybe we do.

We can probably keep it. Taking it out would mean we would probably want to disallow saving images and so on as well, which I'm not sure we want to get into.

Comment 16

[Wesley Johnston (:wesj)]

Attachment: Patch 0/1

I ran into another problem while updating this. Namely, on the awesomescreen calling GeckoProfile.get(context) uses a non-GeckoApp context (until tomorrow whent this activity dies). Since we want to uplift this, this just makes the profile manager more aware of AwesomeBar and that we can be in guestmode while the awesomebar is open, but not for WebApps.

As a longer term fix, I've got a WIP I'm gonna throw up in Bug 902060.

Comment 17

[Wesley Johnston (:wesj)]

Attachment: Patch 1/1

This is the real patch again, with the "Save" menu items re-added.

Comment 18

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

Comment on attachment 792387 [details] [diff] [review]
Patch 0/1

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

>     public static GeckoProfile get(Context context) {
>         if (context instanceof GeckoApp) {

>+        if (context instanceof BrowserApp || context instanceof AwesomeBar) {

>+        if (context instanceof GeckoApp) {

This looks a little odd. Did you want the first and last to be GeckoApp checks? In general, I am OK with this even though I dislike it. I'm OK because I know we are working to remove these ugly dependencies.

Comment 19

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

Comment on attachment 792389 [details] [diff] [review]
Patch 1/1

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

>     public static GeckoProfile get(Context context) {
>         if (context instanceof GeckoApp) {
>             // Check for a cached profile on this context already
>             // TODO: We should not be caching profile information on the Activity context
>             if (((GeckoApp)context).mProfile != null) {
>                 return ((GeckoApp)context).mProfile;
>             }
>-
>-            GeckoProfile guest = GeckoProfile.getGuestProfile(context);
>-            // if the guest profile exists and is locked, return it
>-            if (guest != null && guest.locked()) {
>-                return guest;
>+            if (context instanceof BrowserApp) {
>+                GeckoProfile guest = GeckoProfile.getGuestProfile(context);
>+                // if the guest profile exists and is locked, return it
>+                if (guest != null && guest.locked()) {
>+                    return guest;
>+                }
>             }
>-
>             // Otherwise, get the default profile for the Activity
>             return get(context, ((GeckoApp)context).getDefaultProfileName());
>         }

This looks different than part 0/1. Do you need to rebase this patch?

Will r+ after we chat about the GeckoProfile.get questions

Comment 20

[Wesley Johnston (:wesj)]

Attachment: Aurora awesomebar patch

This is the idea we talked about a bit. I kinda like the other patch better to be honest, but this ships the "guestmode" flag over to the awesomebar activity in the intent.

Two more patches coming to do this same thing with the new awesomebar, and to handle the rest of our share endpoints.

Comment 21

[Wesley Johnston (:wesj)]

Attachment: Patch for BrowserApp and JS

Comment 22

[Wesley Johnston (:wesj)]

Attachment: Patch for nightly awesomebar

Comment 23

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

(In reply to Wesley Johnston (:wesj) from comment #20)
> Created attachment 794307 [details] [diff] [review]
> Aurora awesomebar patch
> 
> This is the idea we talked about a bit. I kinda like the other patch better
> to be honest, but this ships the "guestmode" flag over to the awesomebar
> activity in the intent.

I like this one. Less noise in GeckoProfile and less use of a singleton in the awesomebar.

Comment 24

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

Comment on attachment 794307 [details] [diff] [review]
Aurora awesomebar patch

># HG changeset patch
># Parent 2d1848f275f663d3e3ffb60ec3a7ae94ad283dc0
># User Wes Johnston <[e-mail]>
>
>diff --git a/mobile/android/base/AwesomeBar.java b/mobile/android/base/AwesomeBar.java
>--- a/mobile/android/base/AwesomeBar.java
>+++ b/mobile/android/base/AwesomeBar.java
>@@ -59,18 +59,21 @@ public class AwesomeBar extends GeckoAct
>     public static final String URL_KEY = "url";
>     public static final String TAB_KEY = "tab";
>     public static final String CURRENT_URL_KEY = "currenturl";
>     public static final String TARGET_KEY = "target";
>     public static final String SEARCH_KEY = "search";
>     public static final String TITLE_KEY = "title";
>     public static final String USER_ENTERED_KEY = "user_entered";
>     public static final String READING_LIST_KEY = "reading_list";
>+    public static final String GUEST_MODE_KEY = "guest_mode_enabled";

MODE? 1 MILLION YEARS DUNGEON!

"guest"

>+    public boolean inGuestMode = false;

isGuest

Comment 25

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/fx-team/rev/8870bca1edd3
https://hg.mozilla.org/integration/fx-team/rev/b46cafdfc8ba

Comment 26

[Wesley Johnston (:wesj)]

Comment on attachment 794314 [details] [diff] [review]
Patch for BrowserApp and JS

[Approval Request Comment]
Bug caused by (feature/regressing bug #): new feature
User impact if declined: Guest mode shows "Share" options directly linking into the device owners account
Testing completed (on m-c, etc.): Landed on mc 8/26
Risk to taking this patch (and alternatives if risky): Low risk. Guest mode isn't a normal way to run. Just removing some context menu items. Alternative would probably be to back the feature out or just ship without this polish in it for v1?
String or IDL/UUID changes made by this patch: None.

Comment 27

[Wesley Johnston (:wesj)]

Comment on attachment 794307 [details] [diff] [review]
Aurora awesomebar patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): new feature
User impact if declined: Guest mode shows "Share" options directly linking into the device owners account
Testing completed (on m-c, etc.): Because of huge architectural changes, this patch is specially made for Aurora and is completely different from the mc one. Tested locally on Aurora.
Risk to taking this patch (and alternatives if risky): Low risk. Guest mode isn't a normal way to run. Just removing some context menu items, sending a new extra to the awesomebar. Alternative would probably be to back the feature out or just ship without this polish in it for v1?
String or IDL/UUID changes made by this patch: None.

Comment 28

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/8870bca1edd3
https://hg.mozilla.org/mozilla-central/rev/b46cafdfc8ba

Comment 29

[Alex Keybl [:akeybl]]

(In reply to Wesley Johnston (:wesj) from comment #27)
> Risk to taking this patch (and alternatives if risky): Low risk. Guest mode
> isn't a normal way to run. Just removing some context menu items, sending a
> new extra to the awesomebar. Alternative would probably be to back the
> feature out or just ship without this polish in it for v1?

Not a critical issue (not worth tracking), but also a very simple patch. Wouldn't recommend backing out over this.

Comment 30

[Alex Keybl [:akeybl]]

Comment on attachment 794307 [details] [diff] [review]
Aurora awesomebar patch

Forgot to approve apparently.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0c619a15e9e2
https://hg.mozilla.org/releases/mozilla-aurora/rev/15344ea24f5b

Comment 32

[Teodora Vermesan (:TeoVermesan)]

Verified fixed on:
Build: Firefox for Android 25.0a2 (2013-09-12)
Device: LG Nexus 4
OS: Android 4.2.2