986085 - User-hand-holding to check the Unknown Sources checkbox.

Status: RESOLVED WONTFIX

(Firefox for Android Graveyard :: Web Apps (PWAs), defect, P1)

Description

[James Hugman [:jhugman] [@jhugman]]

Android, by default, disallows the installation of APKs from anywhere but the Play Store. This is "for security purposes". 

It does, however, stop sideloading and alternative APK stores. 

We need to:
 * detect when the user has not checked the checkbox. 
 * help the user through the process of checking it.

This is done by Android in Android 4.x.

Comment 1

[Myk Melez [:myk] [@mykmelez]]

(In reply to James Hugman [:jhugman] [@jhugman] from comment #0)
> It does, however, stop sideloading and alternative APK stores. 

Erm, presumably you meant that it does *not* stop sideloading and alternative APK stores.

Comment 2

[James Hugman [:jhugman] [@jhugman]]

> It does, however, stop sideloading and alternative APK stores.

The checkbox is a speedbump for those users who know about it. 

For users who don't know (most of them), then it does stop sideloading and alternate APK distribution mechanisms.

Comment 3

[James Hugman [:jhugman] [@jhugman]]

I have the building blocks of this ready. However, I'm unsure from a UX pov where to put it. 

My preference would be to do the check /before/ download of the APK, around about https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/webapp/EventListener.java#107 . 

At this point, we should be telling the user that there is a download available, but will only be usable if she takes appropriate steps. It could be argued that without the APK shouldn't be downloaded at all until the user confirms it.

Alternatively, put this right before the package is installed, e.g. near https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/webapp/EventListener.java#191 or one of its callers.

Given that there is an activity change, and a result to catch, then it is difficult to guage which would be easier.

Comment 4

[Myk Melez [:myk] [@mykmelez]]

I suspect it'll be a bit easier to do it before the APK is downloaded, although I don't think it'll be so much easier that it informs the decision either way.

I'm unsure which is user-friendlier, and there are multiple considerations, but the most significant of them seems to be: enabling unknown sources entails taking on some risk, and Android warns the user about that risk, so some users will decide not to follow through.  Thus it makes sense to frontload the work and prompt the user to do it when they initiate the install, before we've downloaded the APK.

Comment 5

[Wesley Johnston (:wesj)]

Attachment: Amazon

I think I agree with myk. I'd rather we do this quickly when the user first attempts to install something. Here's the Amazon store for reference, but we can try and ping UX to see if they have any ideas/time here.

Comment 6

[Ian Barlow (:ibarlow)]

I think this is a great idea. Sadly, I'm not sure when UX would have time to look at this at the moment, given our other priorities. We're pretty overloaded at the moment :(

Needinfo-ing Deb to put this on her radar, and see where this might fit into our roadmap.

Comment 7

[James Hugman [:jhugman] [@jhugman]]

Attachment: First patch for review

Comment 8

[Wesley Johnston (:wesj)]

Comment on attachment 8405458 [details] [diff] [review]
First patch for review

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

Looks pretty good. Hoping you can flip this over to use our new callbacks. Any reason not to?

::: mobile/android/base/webapp/EventListener.java
@@ +185,5 @@
> +        });
> +    }
> +
> +    private void preinstallCheckSuccess(JSONObject message) {
> +        GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("Webapps:Preinstall:OK", message.toString()));

The OK/KO message system is something we invented in Fennec and threw away as soon as we could :)

I think you could do this same work using the new callback object that gets passed to your handleMessage method above now though. You'll just have to pass it through here instead of the original message.

::: mobile/android/modules/WebappManager.jsm
@@ +110,5 @@
> +    sendMessageToJava({
> +       type: "Webapps:Preinstall",
> +       origin: origin,
> +       message: aMessage
> +    });

Then you can just attach your callback to this sendMessageToJava call. i.e.

sendMessageToJava(someJSON, function (success) {
}, function(err) {
});

Comment 9

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 8405458 [details] [diff] [review]
First patch for review

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

I spotted a few nits and a few more substantive issues, but this is headed in the right direction!

::: mobile/android/base/locales/en-US/android_strings.dtd
@@ +314,5 @@
>  <!ENTITY button_set "Set">
>  <!ENTITY button_clear "Clear">
>  
> +<!ENTITY button_settings "Settings">
> +<!ENTITY unknown_sources_dialog_title "Allow Unknown Sources">

Nit: Allow -> Enable (conventional when talking about the setting itself, f.e. see <http://developer.android.com/distribute/open.html#unknown-sources>)

@@ +315,5 @@
>  <!ENTITY button_clear "Clear">
>  
> +<!ENTITY button_settings "Settings">
> +<!ENTITY unknown_sources_dialog_title "Allow Unknown Sources">
> +<!ENTITY unknown_sources_dialog "You need to allow the <b>Unknown Sources</b> in your Android settings">

Nits:

* allow -> enable (conventional)
* remove the word "the" (they aren't specific unknown sources)
* <b>Unknown Sources</b> -> <b>Unknown sources</b> (consistent with capitalization of the setting
* your Android -> Android (possession is extraneous)
* settings -> Settings > Security (more specific)

Resulting in:

  You need to enable <b>Unknown sources</b> in Android Settings > Security

::: mobile/android/base/webapp/EventListener.java
@@ +119,5 @@
> +                        offerUnknownSourcesActivity(retMessage);
> +                    } else {
> +                        preinstallCheckSuccess(retMessage);
> +                    }
> +                } else {

This logic should live inside a "preinstall" method, so handleMessage's scope is limited to receiving and routing the message.

::: mobile/android/modules/WebappManager.jsm
@@ +75,5 @@
> +  _stashMessageManager: function (aMessage, aMessageManager) {
> +    let messageManagers = this._messageManagers;
> +    if (!messageManagers) {
> +      messageManagers = this._messageManagers = {};
> +    }

WebappManager is a singleton, so you should create this_messageManagers unconditionally when the module is loaded via a property declaration:

this.WebappManager = {
  …
  _messageManagers: {},
  …

And once you've done that, it isn't necessary to have a _stashMessageManager method, because stashing a message manager is a one-liner:

  this._messageManagers[aMessage.requestID] = Cu.getWeakReference(aMessageManager);

@@ +90,5 @@
> +      messageManager = {
> +        sendAsyncMessage: function (aType, aMessage) {
> +          debug(aType + " received by null messageManager: " + aMessage);
> +        }
> +      };

This is a great idea, since it'll provide informative debug messages when message managers go away (even expectedly).  But we should reuse a singleton message manager for this purpose:

this.WebappManager = {
  …
  _defaultMessageManager: function(aType, aMessage) {
    …
  },
  …

Then getting a stashed message manager can be a one-liner, and we don't need a method for it:

  let messageManager = this._messageManagers[aMessage.requestID].get() || this._defaultMessageManager;

@@ +100,5 @@
> +    this._stashMessageManager(aMessage, aMessageManager);
> +    this._checkInstallApk(aMessage);
> +  },
> +
> +  _checkInstallApk: function (aMessage) {

Why are _installApk and _checkInstallApk two separate methods?  The former is the only caller of the latter, and it calls it unconditionally; nor is either method particularly long; so it seems unnecessary to break up their functionality across two methods.

@@ +101,5 @@
> +    this._checkInstallApk(aMessage);
> +  },
> +
> +  _checkInstallApk: function (aMessage) {
> +    // This asks Android to examine the Unknown Sources checkbox, 

Nit: trailing space.

Comment 10

[Myk Melez [:myk] [@mykmelez]]

(In reply to Wesley Johnston (:wesj) from comment #8)

> I think you could do this same work using the new callback object that gets
> passed to your handleMessage method above now though. You'll just have to
> pass it through here instead of the original message.

Using the callback form of handleMessage means making EventListener implement NativeEventListener instead of GeckoEventListener, and there aren't yet any examples of that in the codebase.  Also, at least one branch of the logic posts a runnable to the UI thread to show a dialog; can/should we stash a final reference to the callback to reference it from the dialog callbacks?

An advantage of using the callback form of sendMessageToJava is that we wouldn't need to pass the message from WebappManager to EventListener and back, since WebappManager's callbacks would retain a reference to it.  But we don't need to use the callback form of handleMessage for that, as GeckoEventListener supports calling sendMessageToJava's callback(s) via EventListener.sendResponse/sendError.

Comment 11

[Wesley Johnston (:wesj)]

(In reply to Myk Melez [:myk] [@mykmelez] from comment #10)
> Using the callback form of handleMessage means making EventListener
> implement NativeEventListener instead of GeckoEventListener, and there
> aren't yet any examples of that in the codebase.  Also, at least one branch
> of the logic posts a runnable to the UI thread to show a dialog; can/should
> we stash a final reference to the callback to reference it from the dialog
> callbacks?

Yeah, you can just stache the callback or make it final. It retains a message ID, but not much else about the original.

> An advantage of using the callback form of sendMessageToJava is that we
> wouldn't need to pass the message from WebappManager to EventListener and
> back, since WebappManager's callbacks would retain a reference to it.  But
> we don't need to use the callback form of handleMessage for that, as
> GeckoEventListener supports calling sendMessageToJava's callback(s) via
> EventListener.sendResponse/sendError.

True. The big advantage of the other approach will be speed, which probably isn't a huge issue, but we'd like to deprecate all the slow paths at some point, so new code should use the new methods. In fact, if you're using Eclipse, you'll see the old methods are already marked as "deprecated" and Eclipse will yell at you for using 'em.

Comment 12

[Deb Richardson [:dria] (plz NEEDINFO)]

This is in the project funnel and will eventually get worked into the project roadmap from there.

Comment 13

[Myk Melez [:myk] [@mykmelez]]

Per bug 1235869, we're going to disable the Android web runtime, so we won't fix this bug in it.

(This is part of a bulk resolution of bugs in the Firefox for Android::Web Apps component, from which I attempted to exclude bugs that are not specific to the runtime, but it's possible that I included one accidentally.  If so, I'm sorry, and please reopen the bug!)