Cannot install packaged apps from reviewer details page on Android

RESOLVED FIXED in Firefox 32

Status

()

Firefox for Android
Web Apps
P1
major
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: TheOne, Assigned: myk)

Tracking

Trunk
Firefox 32
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [WebRuntime] [A4A], URL)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 years ago
Note: This is about android only, Firefox OS works fine.

STR:
1. Go to url above.
2. Click the install button.

Observed results:
- Installing takes longer than usual.
- After a while, instead of "Launch", it says "Install" again. I click it again, I get an error that the app is already installed.
- The app is not in the android app list nor on a home screen.
- I see it in about:apps though, but with a broken icon.
- I cannot launch it from there.

Expected results:
App install and launch should succeed normally.
(Reporter)

Comment 1

5 years ago
Suddenly it's working now...
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → WORKSFORME
(Reporter)

Comment 2

5 years ago
Reopening as I just hit this again with https://marketplace.firefox.com/reviewers/apps/review/leo
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
Summary: Cannot launch apps from reviewer details page on Android → Cannot launch packaged apps from reviewer details page on Android
Priority: -- → P3
(In reply to Christopher Van Wiemeersch [:cvan] from comment #3)
> Does navigator.mozApps.mgmt.launch work in Android yet?
> https://developer.mozilla.org/en-US/docs/Web/Apps/JavaScript_API/navigator.
> mozApps.mgmt.launch

fwiw, installing packaged apps from the consumer pages work on Android (Aurora at least).
Lisa and I were talking about this yesterday and speculated it may be a signing issue.  (i.e. we install reviewer certs onto the devices... we don't on Android, and don't know how we would)
Summary: Cannot launch packaged apps from reviewer details page on Android → Cannot install packaged apps from reviewer details page on Android

Updated

5 years ago
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 896059
Now I understand the problem here. Disregard the dupe.

I do not have any knowledge of how we can push reviewer certs to a FxAndroid device right now to allow us to test packaged apps during an app review process. As such, this is a FxAndroid bug.
Status: RESOLVED → REOPENED
Component: Reviewer Tools → Web Apps
Product: Marketplace → Firefox for Android
QA Contact: aaron.train
Resolution: DUPLICATE → ---
Version: 1.5 → Trunk

Updated

5 years ago
Priority: P3 → --
Putting as P1 because this will prevent app reviewers from being able to review packaged apps on FxAndroid.
Priority: -- → P1
Whiteboard: [A4A]
Bill, I'm trying to get some further understanding of the problem underlying this bug.  Is it effectively blocked on bug 896620?  Or is it a different issue?
Flags: needinfo?(bwalker)
(In reply to Andrew Williamson [:eviljeff] from comment #9)
> Bill, I'm trying to get some further understanding of the problem underlying
> this bug.  Is it effectively blocked on bug 896620?  Or is it a different
> issue?

Different issue -- As far as I know, bug 896620 pertains only to desktop web runtime, I believe we have solved this problem on Android for now.
Flags: needinfo?(bwalker)
(In reply to Bill Walker [:bwalker] [@wfwalker] from comment #10)
> (In reply to Andrew Williamson [:eviljeff] from comment #9)
> > Bill, I'm trying to get some further understanding of the problem underlying
> > this bug.  Is it effectively blocked on bug 896620?  Or is it a different
> > issue?
> 
> Different issue -- As far as I know, bug 896620 pertains only to desktop web
> runtime, I believe we have solved this problem on Android for now.

Just to clarify, you meant that the equivalent issue in bug 896620 for Android was fixed, not that *this* bug is fixed?  Because its not afaik.
Will bug 896620 fix this? I'm unclear of what the verdict was above.
Will bug 896620 fix this? I'm unclear of what the verdict was above.
(Assignee)

Comment 14

4 years ago
(In reply to Christopher Van Wiemeersch [:cvan] from comment #13)
> Will bug 896620 fix this? I'm unclear of what the verdict was above.

No, I don't think so.  That bug is specific to desktop, while this bug is for Android; and that bug is about the production certificate, while this bug is about enabling installation of apps signed with the review certificate.
(Assignee)

Comment 15

4 years ago
Any chance I can get "reviewer" privileges on marketplace-dev.allizom.org so I can diagnose the problem?  I used to have them, but now when I go to <https://marketplace-dev.allizom.org/reviewers/> I'm told "Oops! Not allowed. You tried to do something that you weren't allowed to."

My account's email address is myk@mozilla.org.
Assignee: nobody → myk
Status: REOPENED → ASSIGNED
(In reply to Myk Melez [:myk] [@mykmelez] from comment #15)
> Any chance I can get "reviewer" privileges on marketplace-dev.allizom.org so
> I can diagnose the problem?  I used to have them, but now when I go to
> <https://marketplace-dev.allizom.org/reviewers/> I'm told "Oops! Not
> allowed. You tried to do something that you weren't allowed to."

Done.
(Assignee)

Comment 17

4 years ago
The first problem is that the APK Factory service doesn't have access to the mini-manifest (nor, presumably, the package) for the app.  In order for Fennec to download and install the APK, the APK Factory has to be able to access those resources.  But they seem to require a session identifier that is presumably stored in a cookie.

cvan: can the Marketplace allow the APK Factory service to access those resources?

Regardless, after we fix that problem, I suspect we'll run into something like an INVALID_SIGNATURE error on webapp install because Fennec doesn't have the certificate with which the reviewer package was signed.  To fix that, we need to find a way to enable reviewers to install that certificate.  And Brian Smith and I just discussed doing that on desktop via an addon that installs the certificate.  Presumably we could do the same thing on Android.
Flags: needinfo?(cvan)
(Assignee)

Comment 18

4 years ago
(In reply to Myk Melez [:myk] [@mykmelez] from comment #17)
> The first problem is that the APK Factory service doesn't have access to the
> mini-manifest (nor, presumably, the package) for the app.

To be precise: the client's request to download the mini-manifest results in a "bad request" error:

 22627                  Gecko  I  _downloadApk for https://marketplace-dev.allizom.org/reviewers/481517/1513782/mini-manifest
 22627                  Gecko  I  downloading APK from http://dapk.net/application.apk?manifestUrl=https%3A%2F%2Fmarketplace-dev.allizom.org%2Freviewers%2F481517%2F1513782%2Fmini-manifest
 22627                  Gecko  I  downloading APK to /storage/emulated/0/Download/httpsmarketplacedevallizomorgreviewers4815171513782minimanifest.apk
 22627                  Gecko  I  [22627] WARNING: blocked access to response header: file /Users/myk/Mozilla/gecko-dev/content/base/src/nsXMLHttpRequest.cpp, line 1202
 22627                  Gecko  I  [22627] WARNING: blocked access to response header: file /Users/myk/Mozilla/gecko-dev/content/base/src/nsXMLHttpRequest.cpp, line 1202
 22627                  Gecko  I  [22627] WARNING: blocked access to response header: file /Users/myk/Mozilla/gecko-dev/content/base/src/nsXMLHttpRequest.cpp, line 1202
 22627                  Gecko  I  --DOMWINDOW == 17 (0x91c2e1a0) [pid = 22627] [serial = 25] [outer = 0x888423c0] [url = about:blank]
 22627                  Gecko  I  [22627] WARNING: NS_ENSURE_TRUE(!(aRv.Failed())) failed: file /Users/myk/Mozilla/gecko-dev/content/base/src/nsXMLHttpRequest.cpp, line 3637
 22627                  Gecko  I  WebManagerWorker onreadystatechange: 2
 22627                  Gecko  I  WebManagerWorker onreadystatechange: 3
 22627                  Gecko  I  WebManagerWorker onprogress: received 31 bytes
 22627                  Gecko  I  WebManagerWorker onprogress: wrote 31 bytes
 22627                  Gecko  I  WebManagerWorker onreadystatechange: 4
 22627                  Gecko  I  error downloading APK: 400 - Bad Request
 22627                  Gecko  I  error downloading APK: 400 - Bad Request
 22627           GeckoConsole  E  Error code: 400 - Bad Request

And wgetting the mini-manifest returns an HTML page requesting that the user sign in.  So it seems very much like Marketplace is requiring a session in order to access that file.
I'm not entirely familiar with the APK stuff. I'll let Kumar/Andy answer that.
Flags: needinfo?(kumar.mcmillan)
Flags: needinfo?(cvan)
Flags: needinfo?(amckay)
(In reply to Myk Melez [:myk] [@mykmelez] from comment #17)
> Regardless, after we fix that problem, I suspect we'll run into something
> like an INVALID_SIGNATURE error on webapp install because Fennec doesn't
> have the certificate with which the reviewer package was signed.  To fix
> that, we need to find a way to enable reviewers to install that certificate.
> And Brian Smith and I just discussed doing that on desktop via an addon that
> installs the certificate.  Presumably we could do the same thing on Android.

Yeah I wrote an add-on that did that last year:
https://addons.mozilla.org/en-US/android/addon/mkt-app-reviewer-certs/
- it needs a little polish though.
(In reply to Myk Melez [:myk] [@mykmelez] from comment #18)
> (In reply to Myk Melez [:myk] [@mykmelez] from comment #17)
> > The first problem is that the APK Factory service doesn't have access to the
> > mini-manifest (nor, presumably, the package) for the app.
> 
> To be precise: the client's request to download the mini-manifest results in
> a "bad request" error:

Yep, the view to a mini manifest requires reviewer authentication: https://github.com/mozilla/zamboni/blob/master/mkt/reviewers/views.py#L795

I'm not sure why. Andy, do you know? Maybe we need to protect a non-public app from being exposed while under review? If that's the case, it might be a tough fix. We'd have to pass around some sort of reviewer token in lieu of sending a session cookie.

Maybe our reviewer instance (bug 963676) will solve this? The reviewer instance could maybe get some privileged API access to view mini manifests.
Flags: needinfo?(kumar.mcmillan)
As you've found the view is protected by a permission ("Apps:Review") which requires a user session to associate this with.

If I remember correctly, the main reason for setting it up this way was to only allow reviewers to install non-public reviewer signed packaged apps.

Once you get the mini manifest the path to the actual signed zip is also protected by the same permission:
https://github.com/mozilla/zamboni/blob/master/mkt/reviewers/views.py#L838
(In reply to Rob Hudson [:robhudson] from comment #22)
> As you've found the view is protected by a permission ("Apps:Review") which
> requires a user session to associate this with.
> 
> If I remember correctly, the main reason for setting it up this way was to
> only allow reviewers to install non-public reviewer signed packaged apps.
> 
> Once you get the mini manifest the path to the actual signed zip is also
> protected by the same permission:
> https://github.com/mozilla/zamboni/blob/master/mkt/reviewers/views.py#L838

Blocking the zip is the important part, so we could probably make the mini-manifest available if that helps (I suspect it doesn't).

Comment 24

4 years ago
Even if we provided some sort of auth to grab that package, would we then be in a scenario where a un-reviewed priveleged app is now available on the APK Factory CDN without any sort of security?
Flags: needinfo?(amckay)
(In reply to Andy McKay [:andym] from comment #24)
> Even if we provided some sort of auth to grab that package, would we then be
> in a scenario where a un-reviewed priveleged app is now available on the APK
> Factory CDN without any sort of security?

Yeah we shouldn't do that both from a possible malware point of view and also that we shouldn't be making a developers code available for download until they've chosen to publish it.

Comment 26

4 years ago
If the APK factory respected cache-headers, we could send a no cache header back to the service when it asks. This still requires the auth issue to be fixed however.

Comment 27

4 years ago
We could run the reviewer APK factory behind the marketplace, use the marketplace as a proxy doing the auth for the reviewer APK factory queue. That still won't stop the auth problem between the APK factory and the marketplace. I think the only way around this is to add authentication to the APK factory.
Blocks: 958329
Created attachment 8375327 [details] [diff] [review]
WIP-map-install-path-to-cert.patch
Attachment #8375327 - Flags: feedback?(fabrice)
Comment on attachment 8375327 [details] [diff] [review]
WIP-map-install-path-to-cert.patch

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

::: dom/apps/src/Webapps.jsm
@@ +3026,5 @@
>  
>      }).bind(this));
>    },
>  
> +  _openPackage: function(aZipFile, aApp, aInstallPath, aIsLocalFileInstall) {

I need aInstallPath to be the path component of the URL of the page that installed the app. In particular, I need to be able to distinguish between https://marketplace.firefox.com/reviewers/<whatever> and https://marketplace.firefox.com/<anything-else>.

@@ +3109,2 @@
>      aCertDb.openSignedAppFileAsync(
>         Ci.nsIX509CertDB.AppMarketplaceProdPublicRoot, aZipFile,

s/Ci.nsIX509CertDB.AppMarketplaceProdPublicRoot/root/
(Assignee)

Comment 30

4 years ago
Note: Andy et al. are working out the issues with requesting the mini-manifest over in Marketplace bug 972642.
Comment on attachment 8375327 [details] [diff] [review]
WIP-map-install-path-to-cert.patch

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

Ok, so you need to get the installPath from the message sent at https://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.js#214
That means adding the installURL directly in https://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.js#133 and using it in the .jsm
Attachment #8375327 - Flags: feedback?(fabrice) → feedback+
(In reply to Fabrice Desré [:fabrice] from comment #31)
> Comment on attachment 8375327 [details] [diff] [review]
> WIP-map-install-path-to-cert.patch
> 
> Review of attachment 8375327 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Ok, so you need to get the installPath from the message sent at
> https://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.js#214
> That means adding the installURL directly in
> https://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.js#133
> and using it in the .jsm

We'd need the install URL for updates too. Can we use the manifest URL? We're serving the mini-manifest from the Marketplace, so using it should work too.
May I have a reviewer account to test the patch?
(In reply to Marco Castelluccio [:marco] from comment #33)
> May I have a reviewer account to test the patch?

access granted on marketplace-dev
Created attachment 8383012 [details] [diff] [review]
Patch

I haven't tested the patch yet, but it should be correct.
Is using the manifest URL instead of the install URL reasonable?
Attachment #8383012 - Flags: feedback?(fabrice)
Attachment #8383012 - Flags: feedback?(brian)
Comment on attachment 8383012 [details] [diff] [review]
Patch

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

::: dom/apps/src/Webapps.jsm
@@ +3062,5 @@
> +        root = uri.path.startsWith("/reviewers/")
> +             ? Ci.nsIX509CertDB.AppMarketplaceDevReviewersRoot
> +             : Ci.nsIX509CertDB.AppMarketplaceDevPublicRoot;
> +        break;
> +    

nit: whitespace

@@ +3064,5 @@
> +             : Ci.nsIX509CertDB.AppMarketplaceDevPublicRoot;
> +        break;
> +    
> +      default:
> +        throw "INSTALL_FROM_DENIED";

That's different from the current behavior. Why not just default root to Ci.nsIX509CertDB.AppMarketplaceProdPublicRoot and let openSignedAppFileAsync() fail gracefully?
Attachment #8383012 - Flags: feedback?(fabrice) → feedback-
(In reply to Fabrice Desré [:fabrice] from comment #36)
> @@ +3064,5 @@
> > +             : Ci.nsIX509CertDB.AppMarketplaceDevPublicRoot;
> > +        break;
> > +    
> > +      default:
> > +        throw "INSTALL_FROM_DENIED";
> 
> That's different from the current behavior. Why not just default root to
> Ci.nsIX509CertDB.AppMarketplaceProdPublicRoot and let
> openSignedAppFileAsync() fail gracefully?

Do you mean trying to open with the AppMarketplaceProdPublicRoot cert and, if it fails, try with the other certificates?
No, I just mean keep the current behavior (which always uses the AppMarketplaceProdPublicRoot cert), and override when you hit the marketplace urls. With your patch we don't fail in the same way when trying to install a signed app from another cert right?
Comment on attachment 8383012 [details] [diff] [review]
Patch

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

::: dom/apps/src/Webapps.jsm
@@ +2999,5 @@
>          aApp.downloadAvailable = false;
>          throw "CERTDB_ERROR";
>        }
>  
> +      let [result, zipReader] = yield this._openSignedPackage(aApp.manifestURL,

Does the manifest URL actually require authentication?

It would be better, theoretically, to pass in the URL of the page that actually called the installPackage() function.

See also Jonas's email on the dev-webapps list. He probably should look at this too.

Personally, I think that this "/reviewers/" thing I suggested is just a hack to get what we've currently deployed working, so it doesn't have to be perfect because we know it isn't the long-term solution.

The best long-term solution is probably to extend the "installs_allowed_from" mechanism so that it can support paths. Then, instead of signing not-yet-reviewed apps using a different root, instead we would make sure that installs_allowed_from=https://marketplace.firefox.com/reviewers. Then it would be up to marketplace.firefox.com to refuse access (require auth) to stuff under /reviewers Not only would this be simpler, but this is something more likely to go through standardization.

However, I think that we don't need to do the long-term thing right now, especially considering we have legacy stuff to be concerned with. So, I think this patch is reasonable, though, like I said above, it would be better to use the URL of the page that called installPackage instead of the manifest URL.

@@ +3064,5 @@
> +             : Ci.nsIX509CertDB.AppMarketplaceDevPublicRoot;
> +        break;
> +    
> +      default:
> +        throw "INSTALL_FROM_DENIED";

What Marco is doing here seems logical to me. I don't understand why we'd want to trust the marketplace cert for any other domain. In fact, I believe we DON'T want to trust it for any other domain, because we don't want other websites to distribute marketplace-signed apps, because they may be distributing old versions of apps that have been blacklisted for security reasons or which are out of date and missing security fixes.
Attachment #8383012 - Flags: feedback?(brian) → feedback+
(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #39)
> Does the manifest URL actually require authentication?
> 
> It would be better, theoretically, to pass in the URL of the page that
> actually called the installPackage() function.
> 
> See also Jonas's email on the dev-webapps list. He probably should look at
> this too.

The problem is that we don't store the entire install URL, but only its origin, so during updates we can't use it.
To address the problem Jonas was referring to, we could use the install origin to choose between prod and dev and the path from the manifest URL to choose between reviewers and public:

  let root;
  switch (installOrigin) {
    case "https://marketplace.firefox.com":
      root = manifestURLPath.startsWith("/reviewers/")
           ? Ci.nsIX509CertDB.AppMarketplaceProdReviewersRoot
           : Ci.nsIX509CertDB.AppMarketplaceProdPublicRoot;
      break;
 
    case "https://marketplace-dev.allizom.org":
      root = manifestURLPath.startsWith("/reviewers/")
           ? Ci.nsIX509CertDB.AppMarketplaceDevReviewersRoot
           : Ci.nsIX509CertDB.AppMarketplaceDevPublicRoot;
      break;

    default:
      throw "INSTALL_FROM_DENIED";
  }

This way third party stores can't install apps signed by the marketplace.
Flags: needinfo?(jonas)
Flags: needinfo?(brian)
If we need th full install url to do things properly, we should just store that.
(In reply to Fabrice Desré [:fabrice] from comment #41)
> If we need th full install url to do things properly, we should just store
> that.

If the install url is just an hack that will probably get ditched in the future, maybe it's better to avoid storing this information that we will not need anymore (and we don't have it for apps that are already installed, so how would we update them?)
It's not clear what is being asked of me here, so I'll just give some thoughts and hope that it covers it.

I talked with Brian the other day about this patch.

Right now it is possible for 3rd party stores to "hotlink" to privileged apps that live in the mozilla marketplace. I think that's a good thing. Keep in mind that these apps will still be updated whenever the mozilla marketplace pushes an update. Including that they'll be updated to a "this app is blocked" version if we push such an update.

We can debate if we should remove this capability. Brian had some concerns about it. But I think we should have that debate as a separate bug as it's a fairly big deal.


Right now we do *not* allow privileged apps to be hosted on other origins than the mozilla marketplace ones. I think that's how we should keep it until we've properly figured out a security model to support 3rd party marketplaces. Hardcoding this limitation in the code seems fine.


I think it's an ok idea that we use the reviewer cert whenever the path starts with "/reviewers/", like the code in comment 40 does. I don't have a better suggestion.

However I think we should *also* require that a pref has been set that enables "reviewer mode". If that pref has not been set then we should always reject review-only apps.
Flags: needinfo?(jonas)
Clearing needinfo. Jonas already summarized the conversation.
Flags: needinfo?(brian)
(In reply to Jonas Sicking (:sicking) from comment #43)
> However I think we should *also* require that a pref has been set that
> enables "reviewer mode". If that pref has not been set then we should always
> reject review-only apps.

how would the pref be set?  Whatever solution we go with it has to be possible to use it on non-rooted devices (i.e. doesn't rely on building images or pushing files with adb) to be useful to our reviewer community.
You should be able to set prefs through adb. I believe that bsmith had some script that helped the reviewer do that.
(In reply to Jonas Sicking (:sicking) from comment #46)
> You should be able to set prefs through adb. I believe that bsmith had some
> script that helped the reviewer do that.

The only script that we know of from smith to help reviewers uses adb to push the certs and the prefs.js file and it doesn't work on non-rooted devices.  bsmith - do you have a different one?
(see last comment, my needinfo didn't work)
Flags: needinfo?(brian)
Clearing needinfo. Andrew, please check your email in two minutes.
Flags: needinfo?(brian)
I'd need the public certificates that are used on marketplace-dev.
If possible, I'd like to access the reviewers pages on the prod marketplace too (otherwise I can't test the patch).
(In reply to Andrew Williamson [:eviljeff] from comment #47)
> (In reply to Jonas Sicking (:sicking) from comment #46)
> > You should be able to set prefs through adb. I believe that bsmith had some
> > script that helped the reviewer do that.
> 
> The only script that we know of from smith to help reviewers uses adb to
> push the certs and the prefs.js file and it doesn't work on non-rooted
> devices.  bsmith - do you have a different one?

okay, bsmith can't help.  If someone can confirm that prefs can be set in some way on a *non-rooted* device then we're good.  If not then we're back to our current position with scripts and the inability for our volunteers to actually use consumer devices.
We could possibly use the 'am start' command - http://developer.android.com/tools/help/adb.html#am.

We may to hand craft an activity which we can instigate via this method.
Could you help with comment 50 and comment 51?
Flags: needinfo?(awilliamson)
(In reply to Marco Castelluccio [:marco] from comment #54)
> Could you help with comment 50 and comment 51?

no and done.
Flags: needinfo?(awilliamson)
(Assignee)

Updated

4 years ago
Whiteboard: [A4A] → [WebRuntime] [A4A]
It was my understanding that Andrew's certificate installation add-on in Comment 20 worked, or at least worked at one point.  But I'm currently testing on Firefox 29 (Aurora, I don't have any updates available yet for Beta), and every time I try to install a packaged app from the reviewer tools I get "Error code: 400 - Bad Request" in the log.
(In reply to Lisa Brewster [:adora] from comment #56)
> It was my understanding that Andrew's certificate installation add-on in
> Comment 20 worked, or at least worked at one point.  But I'm currently
> testing on Firefox 29 (Aurora, I don't have any updates available yet for
> Beta), and every time I try to install a packaged app from the reviewer
> tools I get "Error code: 400 - Bad Request" in the log.

it used to work - but that was before Android started doing APK wrapping.  I don't think the patch to allow reviewer signed apps to actually work on Android has even landed on central yet (that's what Marco is trying to test).
(In reply to Andrew Williamson [:eviljeff] from comment #57)
> (In reply to Lisa Brewster [:adora] from comment #56)
> > It was my understanding that Andrew's certificate installation add-on in
> > Comment 20 worked, or at least worked at one point.  But I'm currently
> > testing on Firefox 29 (Aurora, I don't have any updates available yet for
> > Beta), and every time I try to install a packaged app from the reviewer
> > tools I get "Error code: 400 - Bad Request" in the log.
> 
> it used to work - but that was before Android started doing APK wrapping.  I
> don't think the patch to allow reviewer signed apps to actually work on
> Android has even landed on central yet (that's what Marco is trying to test).

fwiw, this has just worked for me on Aurora
What steps are you using?  I've tried on multiple devices, installed the add-on and checked all the boxes, and am still getting the same error when I try to install any packaged app from reviewer tools.
Regarding setting prefs, try reaching out to dhylands or fabrice. Either might be able to help.

Comment 61

4 years ago
I am experiencing the same problem as Lisa. I tried both Firefox Aurora and Nightly. :-(
Which APK factory are you using?

Steps to determine
1) about:config
2) search for apk
3) Note the value of 'browser.webapps.apkFactoryUrl'

There was an issue with stage-release until 2:20pm today.

Also, which app are you installing?
Created attachment 8395220 [details] [diff] [review]
Patch v2

Added dom.mozApps.use_reviewer_certs pref.
Attachment #8375327 - Attachment is obsolete: true
Attachment #8383012 - Attachment is obsolete: true
Attachment #8395220 - Flags: review?(fabrice)
(In reply to Austin King [:ozten] from comment #62)
> Which APK factory are you using?

https://controller.apk.firefox.com/application.apk

> Also, which app are you installing?

https://marketplace.firefox.com/reviewers/apps/review/adoras-packaged-app-1, but I've tried others, too.  I re-tested and I'm still not able to install today.
I was going to try to repro using curl against stage and production.

However, that is an app in review. I'll need some help to get the manifest url.
Flags: needinfo?
(In reply to Austin King [:ozten] from comment #65)
> I'll need some help to get the manifest url.

https://marketplace.firefox.com/reviewers/adoras-packaged-app-1/1533428/mini-manifest

Send me your Marketplace email address and I can also give you access to the reviewer pages.
In the Marketplace server logs I'm seeing:

    z.reviewers:INFO Token provided for app:495338 but was not valid

This is suggesting that the token, which is a single use token, has already been used and is no longer valid. We don't have logs for the happy flow but I could add them and try to get them on prod soon which would allow us to see if, perhaps, the request is happening more than once?
Flags: needinfo?
Comment on attachment 8395220 [details] [diff] [review]
Patch v2

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

We need tests before landing.
Attachment #8395220 - Flags: review?(fabrice) → feedback+
I'm not sure how to test this, as it would involve installing apps from the marketplace.
Can't we get sample apps signed by the marketplace and install them?
(In reply to Fabrice Desré [:fabrice] from comment #70)
> Can't we get sample apps signed by the marketplace and install them?

Ok, we can't test the install origin stuff though.
I guess we need to wait for bug 880043, that is adding signed apps tests.
Flags: needinfo?(awilliamson)
Mochitest uses proxy configuration which makes it possible to make files loaded from the mochitest server be loaded from any domain that we want to.

http://mxr.mozilla.org/mozilla-central/source/build/pgo/server-locations.txt
(In reply to Marco Castelluccio [:marco] from comment #71)
> (In reply to Fabrice Desré [:fabrice] from comment #70)
> > Can't we get sample apps signed by the marketplace and install them?
> 
> Ok, we can't test the install origin stuff though.
> I guess we need to wait for bug 880043, that is adding signed apps tests.

I don't know what I'm being needinfo'd for
Flags: needinfo?(awilliamson)
(In reply to Andrew Williamson [:eviljeff] from comment #73)
> (In reply to Marco Castelluccio [:marco] from comment #71)
> > (In reply to Fabrice Desré [:fabrice] from comment #70)
> > > Can't we get sample apps signed by the marketplace and install them?
> > 
> > Ok, we can't test the install origin stuff though.
> > I guess we need to wait for bug 880043, that is adding signed apps tests.
> 
> I don't know what I'm being needinfo'd for

Oops, sorry.
For completeness, I believe robhudson fixed something on the marketplace side Tuesday for 
https://marketplace.firefox.com/reviewers/adoras-packaged-app-1/1533428/mini-manifest

We tested this in stage and had it working, as I recall.
I think we're mixing two problems here, so I've filed bug 989806.
(Assignee)

Comment 77

4 years ago
(In reply to Marco Castelluccio [:marco] from comment #76)
> I think we're mixing two problems here, so I've filed bug 989806.

I suspect bug 989806 is actually the only remaining issue, so I built a debug build with that patch applied and tested installation on stage, where it failed at "package open/read error: INVALID_SIGNATURE".

But that patch doesn't include certs for stage, only dev and production, so the failure is expected.  Once I get access to dev (currently blocked by bug 990265), or if someone can give me access to production, I'll test there.
(In reply to Myk Melez [:myk] [@mykmelez] from comment #77)
> if someone can give me access to production, I'll test there.

Done!
(Assignee)

Comment 79

4 years ago
Ok, I tested on production with the first packaged app in the queue, and I see "package open/read error: INVALID_SIGNATURE" there as well, so bug 989806 is apparently *not* the only remaining issue.  I'll keep investigating.
Duplicate of this bug: 996725
I've hit this bug with 2 test apks I've created via the CLI.

https://ozten.com/random/apk-factory/test.apk
https://ozten.com/random/apk-factory/fb_test.apk

myk - can you take a peak at these and let me know if there is anything obviously wrong with them?

Aside: I'm working on bug#962879, which is how I hit this.
Flags: needinfo?(myk)
I've written the wrong bug number in the patch, this changeset is for bug 989806.
Depends on: 989806
(Assignee)

Comment 85

4 years ago
I just built Fennec from the Central branch with the latest patch on bug 989806, set the dom.mozApps.user_reviewer_certs pref to true, and successfully installed the first five packaged apps that are marked as Firefox for Android-compatible in the marketplace.firefox.com "apps" queue.

So I think bug 989806 is actually sufficient to resolve this bug, at least now.  Perhaps other necessary fixes have landed since the last time I tested, or maybe I tested incorrectly.

I did see one case in which Marketplace told me that the app failed to install.  logcat revealed that the APK Factory service response was only two bytes long.  But reloading the Marketplace page and reattempting the install succeeded in retrieving and installing a valid APK.
(Assignee)

Comment 86

4 years ago
(In reply to Austin King [:ozten] from comment #81)
> myk - can you take a peak at these and let me know if there is anything
> obviously wrong with them?

I was able to "adb install", launch, and run test.apk on a build that has the fix for bug 989806 applied, although logcat reported "[JavaScript Error: "example.com:443 uses an invalid security certificate." (which is a different issue).  Subsequent launches stalled on a white screen, but that too seems like a different issue.

I was also able to install fb_test.apk after uninstalling test.apk, although that app doesn't appear to have the same name, and I couldn't figure out its name via "aapt dump", which complained "ERROR getting 'android:name' attribute", so I wasn't able to test launching and running it.
Flags: needinfo?(myk)
No longer blocks: 972201
(Assignee)

Comment 87

4 years ago
Now that blocking bug 989806 has been fixed, this should similarly be fixed, and installing apps from the reviewer details page should work on today's nightly build of Fennec.

Note that you need to use about:config to create the boolean pref dom.mozApps.use_reviewer_certs and set it to true in order to enable the functionality implemented in bug 989806.
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago4 years ago
Resolution: --- → FIXED
Did this get merged to central?
(Assignee)

Comment 89

4 years ago
This bug was fixed by blocking bug 989806, which merged to central on Tuesday, April 29 at 20:39:44 UTC, according to the push log <https://hg.mozilla.org/mozilla-central/pushloghtml?changeset=8234dac0ea59>.
Target Milestone: --- → Firefox 32
You need to log in before you can comment on or make changes to this bug.