744867 - Installation failure due to installs_allowed_from returns INVALID_MANIFEST

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

[Ian Bicking (:ianbicking)]

If you install an app from an origin that isn't allowed due to installs_allowed_from, the error is INVALID_MANIFEST, which doesn't seem right and is definitely confusing.  PERMISSION_DENIED might be better, or a new error just for this case.

The problem is in Webapps.js, WebappsRegistry.checkManifest (or how that function is being used).

Comment 1

[Jason Smith [:jsmith]]

This sounds like a mozapps API bug, not a bug in the front-end UI.

Comment 2

[Ian Bicking (:ianbicking)]

Attachment: Checks installs_allowed_from separately and has a separate error for that case

Patch attached.  We don't have tests in the tree, but I have a manual test here: http://app1.ianbicking.org/?manifest=manifest_bad_installs_allowed_from.webapp

This adds a new error, INSTALL_FROM_DENIED

Because we've encountered this specific error several times in testing, and each time it confuses people, I think it is best to have a distinct error for just this case.

Comment 3

[[:fabrice] Fabrice Desré]

Comment on attachment 617583 [details]
Checks installs_allowed_from separately and has a separate error for that case

>diff -r 85abf12e5c83 dom/base/Webapps.js
>--- a/dom/base/Webapps.js	Mon Apr 23 07:11:19 2012 -0700
>+++ b/dom/base/Webapps.js	Mon Apr 23 14:04:08 2012 -0500
>@@ -35,25 +35,28 @@
>    * only the name property is mandatory
>    */
>   checkManifest: function(aManifest, aInstallOrigin) {
>-    // TODO : check for install_allowed_from
>     if (aManifest.name == undefined)
>       return false;
>+    return true;
>+  },
> 
>-    if (aManifest.installs_allowed_from) {
>-      ok = false;
>-      aManifest.installs_allowed_from.forEach(function(aOrigin) {
>-        if (aOrigin == "*" || aOrigin == aInstallOrigin)
>-          ok = true;
>-      });
>-      return ok;
>+  checkInstallAllowed: function checkInstallAllowed(manifest, installOrigin) {

use aManifest and aInstallOrigin here.

>+    if (! manifest.installs_allowed_from) {
>+      return true;
>     }
>+    for (let i=0; i<manifest.installs_allowed_from.length; i++) {

nit: spaces around < . Also, why not use forEach ?

>+      let origin = manifest.installs_allowed_from[i];
>+      if (origin == "*" || origin == installOrigin) {
>     return true;
>+      }
>+    }
>+    return false;
>   },
> 

r=me with nits addressed

Comment 4

[Ian Bicking (:ianbicking)]

Attachment: Checks installs_allowed_from separately and has a separate error for that case (v2)

New patch with nits addressed.  I didn't use forEach because that required a flag to handle the return, so using an index is less total code.

Comment 5

[[:fabrice] Fabrice Desré]

Comment on attachment 617600 [details] [diff] [review]
Checks installs_allowed_from separately and has a separate error for that case (v2)

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

::: dom/base/Webapps.js
@@ +42,3 @@
>  
> +  checkInstallAllowed: function checkInstallAllowed(aManifest, aInstallOrigin) {
> +    if (! aManifest.installs_allowed_from) {

Nit: !aManifest.

@@ +46,2 @@
>      }
> +    for (let i=0; i < aManifest.installs_allowed_from.length; i++) {

Nit: i = 0

And what about:
installs_allowed_from.forEach(aOrigin) {
  if (aOrigin == "*" || aOrigin == aInstallOrigin) {
    return true;
  }
}

return false;

Comment 6

[Ian Bicking (:ianbicking)]

Attachment: Checks installs_allowed_from separately and has a separate error for that case (v3)

whitespace fixed

Comment 7

[[:fabrice] Fabrice Desré]

Comment on attachment 617607 [details] [diff] [review]
Checks installs_allowed_from separately and has a separate error for that case (v3)

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

::: dom/base/Webapps.js
@@ +50,1 @@
>      return true;

Nit: indentation

Comment 8

[Ian Bicking (:ianbicking)]

Attachment: Checks installs_allowed_from separately and has a separate error for that case (v4)

Fix indentation (an unfortunate side effect from diff -w)

Comment 9

[Matthew N. [:MattN]]

Attachment: Rebase ianb's patch (v5)

This patch had r+ but never got checked-in :(

I ran into this issue and found this bug so decided to rebase.  There has been a decent amount of change in this code so I'm requesting re-review.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=3e107bbcf247

Comment 10

[[:fabrice] Fabrice Desré]

Comment on attachment 682855 [details] [diff] [review]
Rebase ianb's patch (v5)

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

Thanks for reviving that Matthew! Can you add a test also before we land? They are in dom/tests/mochitests/webapps

Comment 11

[Matthew N. [:MattN]]

Attachment: Added test for non-packaged installation (v6)

I added tests for the basic install case but didn't see where the other cases are tested and I don't know enough about packaged apps to create one from scratch.

Before attaching v5 I searched for references to INVALID_MANIFEST and didn't see any cases that needed to be updated so I don't expect things to break since this patch just changed the error name for this case.

Comment 12

[[:fabrice] Fabrice Desré]

Comment on attachment 682935 [details] [diff] [review]
Added test for non-packaged installation (v6)

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

r=me
Can you ask for uplift to aurora 18?

Comment 13

[Matthew N. [:MattN]]

Comment on attachment 682935 [details] [diff] [review]
Added test for non-packaged installation (v6)

Pushed: https://hg.mozilla.org/integration/mozilla-inbound/rev/4fddb9923ef0

[Approval Request Comment]
Bug caused by (feature/regressing bug #): More prevalent confusion caused by bug 799563
User impact if declined: Developers will think their manifest is invalid when it is not.
Testing completed (on m-c, etc.): Just pushed to inbound
Risk to taking this patch (and alternatives if risky): Low risk IMO because the only difference is what error is thrown.
String or UUID changes made by this patch: None

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4fddb9923ef0

Comment 15

[Alex Keybl [:akeybl]]

Comment on attachment 682935 [details] [diff] [review]
Added test for non-packaged installation (v6)

[Triage Comment]
Low risk fix in support of web apps (with no chance of regression outside of apps).

If this is landed on mozilla-aurora before ~11AM PT tomorrow, this will make the merge from Aurora 18 to Beta 18. If landed 11AM-5PM PT tomorrow on mozilla-beta, it will make the first FF18 Beta. If landed after that, it will end up in the second FF18 beta.

Comment 16

[Matthew N. [:MattN]]

Thanks everyone for the quick turnaround.

https://hg.mozilla.org/releases/mozilla-aurora/rev/13365a46ce7d