Closed Bug 896121 Opened 6 years ago Closed 6 years ago

[guest] Disable 'share' menus in guest mode

Categories

(Firefox for Android :: General, defect)

x86_64
Linux
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 26
Tracking Status
firefox24 --- unaffected
firefox25 --- verified
firefox26 --- verified

People

(Reporter: mfinkle, Assigned: wesj)

References

Details

Attachments

(3 files, 8 obsolete files)

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
Attached patch patch (obsolete) — Splinter Review
Again, depends on the patch in bug 871863....
Attachment #779479 - Flags: review?(mark.finkle)
Comment on attachment 779479 [details] [diff] [review]
patch

I don't think this handles the other share menus, right?
Attachment #779479 - Flags: review?(mark.finkle) → review-
Attached patch Patch (obsolete) — Splinter Review
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.
Attachment #779479 - Attachment is obsolete: true
Attachment #783550 - Flags: review?(mark.finkle)
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.
Attached patch Patch 1/2 (obsolete) — Splinter Review
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.
Attachment #783550 - Attachment is obsolete: true
Attachment #783550 - Flags: review?(mark.finkle)
Attachment #784838 - Flags: review?(mark.finkle)
Attached patch Patch 2/2 (obsolete) — Splinter Review
This adds the disabled piece back for js context menu items.
Attachment #784839 - Flags: review?(mark.finkle)
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"
Attachment #784838 - Flags: review?(mark.finkle) → review-
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.
(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....
Attached patch Patch (obsolete) — Splinter Review
Updated to hide the menu items. Had to move the GuestProfile check in GeckoProfile.get because the Awesomescreen is not a GeckoApp.
Attachment #784838 - Attachment is obsolete: true
Attachment #785096 - Flags: review?(mark.finkle)
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.
Attachment #785096 - Flags: review?(mark.finkle) → review-
Comment on attachment 784839 [details] [diff] [review]
Patch 2/2

Not needed just yet
Attachment #784839 - Flags: review?(mark.finkle)
Attached patch Patch v3 (obsolete) — Splinter Review
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.
Attachment #784839 - Attachment is obsolete: true
Attachment #785096 - Attachment is obsolete: true
Attachment #786421 - Flags: review?(mark.finkle)
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.
Flags: needinfo?(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.
Flags: needinfo?(ibarlow)
Attached patch Patch 0/1 (obsolete) — Splinter Review
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.
Attachment #786421 - Attachment is obsolete: true
Attachment #786421 - Flags: review?(mark.finkle)
Attachment #792387 - Flags: review?(mark.finkle)
Attached patch Patch 1/1 (obsolete) — Splinter Review
This is the real patch again, with the "Save" menu items re-added.
Attachment #792389 - Flags: review?(mark.finkle)
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.
Attachment #792387 - Flags: review?(mark.finkle) → feedback+
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
Attachment #792389 - Flags: review?(mark.finkle) → feedback+
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.
Attachment #792387 - Attachment is obsolete: true
Attachment #794307 - Flags: review?(mark.finkle)
Attachment #792389 - Attachment is obsolete: true
Attachment #794314 - Flags: review?(mark.finkle)
Attachment #794315 - Flags: review?(mark.finkle)
Attachment #794315 - Flags: review?(mark.finkle) → review+
(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 on attachment 794307 [details] [diff] [review]
Aurora awesomebar patch

># HG changeset patch
># Parent 2d1848f275f663d3e3ffb60ec3a7ae94ad283dc0
># User Wes Johnston <wjohnston@mozilla.com>
>
>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
Attachment #794307 - Flags: review?(mark.finkle) → review+
Attachment #794314 - Flags: review?(mark.finkle) → review+
tracking-fennec: --- → ?
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.
Attachment #794314 - Flags: approval-mozilla-aurora?
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.
Attachment #794307 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/8870bca1edd3
https://hg.mozilla.org/mozilla-central/rev/b46cafdfc8ba
Assignee: nobody → wjohnston
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
Status: RESOLVED → VERIFIED
(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 on attachment 794307 [details] [diff] [review]
Aurora awesomebar patch

Forgot to approve apparently.
Attachment #794307 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #794314 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified fixed on:
Build: Firefox for Android 25.0a2 (2013-09-12)
Device: LG Nexus 4
OS: Android 4.2.2
tracking-fennec: ? → ---
You need to log in before you can comment on or make changes to this bug.