880043 - Mochitest: Install / Launch / Uninstall signed packaged app

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

[[e-mail] [:onecyrenus]]

1) Install a signed packaged app
2) Launch packaged app
3) Uninstall signed packaged app

Comment 1

[Antonio Manuel Amaya Calvo (:amac)]

Taking this. 
Besides installing, launching and uninstalling (and I think launch might be  already covered by bug 821589), we need tests for updating signed packaged apps, and for installing, uninstalling and updating signed packaged apps with a custom origin set.

Comment 2

[Antonio Manuel Amaya Calvo (:amac)]

Added it to the 1.2 systems backlog, as technical debt.

Comment 3

[Jason Smith [:jsmith]]

(In reply to Antonio Manuel Amaya Calvo from comment #2)
> Added it to the 1.2 systems backlog, as technical debt.

Actually this falls under systems front end group I think. App Install falls under that group.

Comment 4

[[e-mail] [:onecyrenus]]

So this is a gecko based test.

Comment 5

[Jason Smith [:jsmith]]

(In reply to [e-mail]  [:onecyrenus] from comment #4)
> So this is a gecko based test.

Correct, although the Systems Front End team apparently is handling this functionality (the team naming being used is not entirely in alignment with gecko vs. gaia).

Comment 6

[Antonio Manuel Amaya Calvo (:amac)]

Untaking this since I'm on vacation for a couple of weeks and so I won't work on this right now.

Comment 7

[[e-mail] [:onecyrenus]]

Attachment: signed_apps.diff

Signed Packaged app install test suite. 
There is one more scenario to add here which is where we create a very large ids.json file.

Comment 8

[[e-mail] [:onecyrenus]]

This should be working on both desktop and b2g. The caveat seems to be on b2g that we need to remove all logins pwmgr.removeAllLogins();, this wasn't required on desktop, but was required on b2g.  
Somehow on b2g it seems that a login / pwd is required for the mochi.test domain ? Unsure how this is coming to be.

Comment 9

[[e-mail] [:onecyrenus]]

Attachment: signed_apps.diff

Attached is a final patch.

Comment 10

[[:fabrice] Fabrice Desré]

Comment on attachment 815133 [details] [diff] [review]
signed_apps.diff

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

Myk, do you have time taking a look at this?

Comment 11

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

(In reply to Fabrice Desré [:fabrice] from comment #10)
> Myk, do you have time taking a look at this?

Yes, I'll review this shortly.

Comment 12

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

Comment on attachment 815133 [details] [diff] [review]
signed_apps.diff

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

I haven't reviewed all the code yet, but here's a partial review that describes some useful and necessary changes:


This needs to add chromeScript.js and test_signed_pkg_install.html to the lists of support files and tests, respectively, in mochitest.ini.  Otherwise the tests won't be built and run.

Also, chromeScript.js is a poor name for that JavaScript file, since it describes how the file does something but not what the file does.  There are only two consumers of SpecialPowers.loadChromeScript in the codebase, and they load the files debugger-protocol-helper.js and bug578096LoadChromeScript.js.  In this case, the sole function of the file is to add a cert to the application's cert database, so I would call this file add-cert.js.

::: dom/apps/tests/chromeScript.js
@@ +1,1 @@
> +if (typeof(Ci) == 'undefined') {

typeof <https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/typeof> is an operator, not a function; so its operand usually doesn't need to (and shouldn't) be parenthesized.

@@ +7,5 @@
> +
> +addMessageListener("addCert", function (message) {
> +  var aTrust = message.trust;
> +  var aName = message.name;
> +  var aUrl = message.url;

Nit: drop the "a" prefixes and use destructuring assignment <http://fitzgeraldnick.com/weblog/50/> to define these variables in the parameter list:

  addMessageListener("addCert", function ({ trust, name, url }) {

@@ +18,5 @@
> +  var bstream = Cc["@mozilla.org/binaryinputstream;1"]
> +                .createInstance(Ci.nsIBinaryInputStream);
> +  bstream.setInputStream(stream);
> +  var size = 0;
> +  var file_data = "";

Nit: use camelCase for this variable name, i.e. call it fileData.

@@ +19,5 @@
> +                .createInstance(Ci.nsIBinaryInputStream);
> +  bstream.setInputStream(stream);
> +  var size = 0;
> +  var file_data = "";
> +  while(size = bstream.available()) {

Nit: put a space between "while" and its opening parenthesis.

@@ +20,5 @@
> +  bstream.setInputStream(stream);
> +  var size = 0;
> +  var file_data = "";
> +  while(size = bstream.available()) {
> +    dump("size = " +size);

Nit: put a space after the plus sign and concatenate a newline to the end of all dump() output, i.e.:

  dump("size = " + size + "\n");

@@ +26,5 @@
> +  }
> +  var cert = file_data;
> +  var certdb = Cc["@mozilla.org/security/x509certdb;1"].getService(Ci.nsIX509CertDB);
> +  var pwmgr = Cc["@mozilla.org/login-manager;1"].
> +                       getService(Ci.nsILoginManager);

Nit: the formatting of Cc[].getService/createInstance() expressions in this file is both internally and externally inconsistent.  Wrap lines consistently at 80 characters, and employ one of these two conventions when you have to wrap a statement containing such an expression:

  var pwmgr = Cc["@mozilla.org/login-manager;1"].
              getService(Ci.nsILoginManager);

  var pwmgr = Cc["@mozilla.org/login-manager;1"]
                .getService(Ci.nsILoginManager);

@@ +34,5 @@
> +  //todo(false, "Warning: wasn't expecting logins to be present.");
> +    pwmgr.removeAllLogins();
> +    dump("removed all logins");
> +  }
> +  certdb.addCert(cert, aTrust, aName);

In my testing, the tests hang on this line with:

 0:06.00 TEST-PASS | unknown test url | this is a test
 0:06.05 size = 581System JS : ERROR http://mochi.test:8888/tests/dom/apps/tests/chromeScript.js:38 - NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIX509CertDB.addCert]

@@ +35,5 @@
> +    pwmgr.removeAllLogins();
> +    dump("removed all logins");
> +  }
> +  certdb.addCert(cert, aTrust, aName);
> +  for(var i=0; i < logins.length; i++) {

Nit: put a space between "for" and its opening parenthesis.

::: dom/apps/tests/signed/Makefile.in
@@ +5,5 @@
> +DEPTH	 = @DEPTH@
> +topsrcdir	 = @top_srcdir@
> +srcdir	= @srcdir@
> +VPATH	 = @srcdir@
> +relativesrcdir	= @relativesrcdir@

Nit: use spaces instead of tabs to indent the values.

::: dom/apps/tests/test_signed_pkg_install.html
@@ +1,4 @@
> +<!DOCTYPE html>
> +<html>
> +<!--
> +https://bugzilla.mozilla.org/show_bug.cgi?id={821589}

Nit: It's misleading to reference a bug number here, since it suggests that this test verifies the fix for a specific defect; whereas it actually implements a set of general functional tests for a feature, and the bug is just a task tracking mechanism.

However, if you do keep this reference to the bug, then the number should not be enclosed in curly brackets (neither here nor elsewhere), so the link actually works, and for consistency with other test pages (and the output generated by testing/mochitest/gen_template.pl).

(I know that test_packaged_app_install.html and test_packaged_app_update.html also use curly brackets, which should also get fixed, although that can happen in a different bug.)

Comment 13

[[e-mail] [:onecyrenus]]

Thanks myk, working on a new patch

Comment 14

[[e-mail] [:onecyrenus]]

Attachment: signed_apps.diff

Fixes for the previously noted items. Also have a run going on tbpl, will add a reviewer once i see the status. 

https://tbpl.mozilla.org/?tree=Try&rev=ec37d8efc769

Comment 15

[[e-mail] [:onecyrenus]]

https://tbpl.mozilla.org/?tree=Try&rev=ec434504bc8d, running through the try server again. Had some small nits to fix.

Comment 16

[[e-mail] [:onecyrenus]]

Attachment: signed_apps.diff

Comment 17

[[e-mail] [:onecyrenus]]

updated the patch, do you have an eta on review ?

Comment 18

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

Comment on attachment 8343248 [details] [diff] [review]
signed_apps.diff

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

Thanks for the updated patch!  I did a more thorough review this time and identified a number of issues.  Many are nits, but some are substantive issues that include bugs as well as problems that will make it harder for someone to enhance these tests in the future.

Regarding the binary certificate file and ZIP archives, how would I go about regenerating them?  And where is the source for the code in the ZIP archives located?

At the very least, these binary files must be accompanied by instructions for regenerating them, so someone can change the tests in the future.  Even better would be to also add the source code for the ZIP archives (and any source file used to generate the certificate) to the patch, so the binaries would be accompanied by the source used to generate them.  And ideally the binaries would be generated dynamically at build time, although I know that's significantly more work, and it's ok to commit the binaries for now given their small size and the unlikelihood that they will change much over time.

(Note that some of the binaries have 0755 permissions; as files, they should all be 0644.)

::: dom/apps/tests/chromeAddCert.js
@@ +2,5 @@
> +  var Ci = Components.interfaces;
> +}
> +if (typeof Cc == 'undefined') {
> +  var Cc = Components.classes;
> +}

Why are these declarations conditional?  The SpecialPowers API evaluates the script in a sandbox, and these variables shouldn't already be defined in it.

There's only one other script that gets loaded by SpecialPowers.loadChromeScript and defines Cc/Ci/Cu: toolkit/devtools/apps/tests/debugger-protocol-helper.js.  And it defines them as constants unconditionally.  So this code should be able to do the same.

@@ +5,5 @@
> +  var Cc = Components.classes;
> +}
> +
> +addMessageListener("addCert", function (message) {
> +  var {trust, name, url} = message;

Nit: as a script being evaluated in a JavaScript 1.8 sandbox, this script should use *let* to declare variables.

Also note that you can use destructuring assignment directly inside the parameter list to simplify assigning variables to the properties of the *message* argument:

  addMessageListener("addCert", function({ trust, name, url }) {

@@ +13,5 @@
> +  var channel = ioserv.newChannel(url, 0, null);
> +  var stream = channel.open();
> +
> +  var bstream = Cc["@mozilla.org/binaryinputstream;1"]
> +                .createInstance(Ci.nsIBinaryInputStream);

Nit: later code appends dot operators to the ends of the first lines of multi-line declarations like these two.  The code should use the same style consistently, preferably the one it uses later in the file, which is more conventional.  So these should be:

  var ioserv = Cc["@mozilla.org/network/io-service;1"].
               getService(Ci.nsIIOService);
  …
  var bstream = Cc["@mozilla.org/binaryinputstream;1"].
                createInstance(Ci.nsIBinaryInputStream);

@@ +18,5 @@
> +  bstream.setInputStream(stream);
> +  var size = 0;
> +  var cert = "";
> +  while( size = bstream.available() ) {
> +    dump("size = " + size + "\n");

Contextless console output like this dump statement is difficult to understand for a reviewer or another engineer.  There are a variety of ways to add context, and you can go overboard, but a little goes a long way, so you don't have to add much to make a statement understandable.

In this case, I would simply prepend the name of the function, "addCert", and "bstream.available", since that'll make it clear what is being reported:

  dump("addCert: bstream.available size = " + size + "\n");

However, for each such statement, it's also worth asking whether the output is actually useful on an ongoing basis or just during the initial development of the patch.  If the latter, then we should remove it from the patch, since it'll just make the code more complex and the console log harder to read.

@@ +28,5 @@
> +              getService(Ci.nsILoginManager);
> +  // Check that initial state has no logins
> +  var logins = pwmgr.getAllLogins();
> +  if (logins.length) {
> +  //todo(false, "Warning: wasn't expecting logins to be present.");

This line seems like it can't possibly work if uncommented, since this is the chrome script, not a test file, and "todo" is not defined in this context.  So this line should be either a line of code that *would* work in this file or a non-code comment about logins being unexpected.

But it isn't clear why logins aren't expected nor why we have to remove and readd them all.  Couldn't another test have added some logins that it didn't subsequently remove?  And why does it readding logins matter?  At the very least, this code needs some commentary explaining why it's doing this.

@@ +36,5 @@
> +  certdb.addCert(cert, trust, name);
> +  for (var i=0; i < logins.length; i++) {
> +    pwmgr.addLogin(logins[i]);
> +  }
> +  sendAsyncMessage("addCertCompleted", {status: "completed"});

The code that listens for addCertCompleted ignores its *message* parameter, so there's no point passing an argument to that parameter here, and this call should simply leave it out.

::: dom/apps/tests/mochitest.ini
@@ +10,5 @@
>    file_packaged_app.template.webapp
> +  chromeAddCert.js
> +  signed_app.sjs
> +  signed_app_template.webapp
> +  signed/*

Nit: this list of support files is currently in alphabetical order, and we should keep it in that order.  Also, we should retain the blank line that separates it from the list of test files.

@@ +15,5 @@
>  [test_app_update.html]
>  [test_bug_795164.html]
>  [test_packaged_app_common.js]
>  [test_packaged_app_install.html]
> +[test_signed_pkg_install.html]

Nit: this list of test files is also in alphabetical order and should remain so.

::: dom/apps/tests/signed/Makefile.in
@@ +8,5 @@
> +     unknown_issuer_app_1.zip \
> +     unsigned_app_1.zip \
> +     unsigned_app_2.zip \
> +     valid_app_1.zip \
> +     valid_app_2.zip \

Conclude this list with:

    $(NULL)

Also, nit: use a conventional two- or four-space intent here instead of five spaces.

::: dom/apps/tests/signed/moz.build
@@ +2,5 @@
> +# vim: set filetype=python:
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +#

Nit: make the sixth line be a blank line rather than a comment line.

::: dom/apps/tests/signed_app.sjs
@@ +26,5 @@
> +  var prevVersion = Number(getState("version"));
> +
> +  if (version != prevVersion) {
> +    setState("version", version);
> +  }

query.version is a string, as is getState("version"); but *1* is an integer, as is Number(getState("version")).  And setState("version", 1) will throw an exception ("Error: non-string value passed on line 2945 in httpd.js"), because setState requires its second argument to be a string; while |version != prevVersion| may do type conversion, although it isn't immediately obvious if it'll do it or which value it'll convert.

So this code needs to be more careful about the types of the values it uses.  Since it only tests whether the two version values are unequal, not whether one is greater or less than the other; and since it always uses them in a string context (to create an etag and to inject into the *version* field of the manifest, which has a string value); it should simply always treat version values as strings, setting *version* to the string value "1" if the query doesn't include a *version* parameter, and setting prevVersion directly to the return value of getState("version").

@@ +31,5 @@
> +  var packageName = app + "_app_" +
> +                      version + ".zip";
> +  setState("packageName", packageName);
> +  var packagePath = "/" + gBasePath + "signed/" +
> +                      packageName;

Nit: don't wrap lines when it isn't necessary, as with these two varibale declarations, which could be:

  var packageName = app + "_app_" + version + ".zip";
  …
  var packagePath = "/" + gBasePath + "signed/" + packageName;

@@ +46,5 @@
> +  response.setHeader("Etag", etag, false);
> +
> +  // Serve the mini-manifest corresponding to the requested app version.
> +  var template = gBasePath + gMiniManifestTemplate;
> +  //dump(template);

Nit: remove this line (or uncomment it and add some context to it).

@@ +52,5 @@
> +                     "application/x-web-app-manifest+json", false);
> +  var manifest = makeResource(template, version, packagePath, packageSize,
> +                              gAppName, gDevName, gDevUrl);
> +  response.write(manifest);
> +  return;

Nit: this is the end of the function, so there's no point in returning explicitly (unless there's actually a value to return).

@@ +59,5 @@
> +function getQuery(request) {
> +  var query = {};
> +  request.queryString.split('&').forEach(function (val) {
> +    var [name, value] = val.split('=');
> +    query[name] = unescape(value);

*unescape* is deprecated <https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/unescape>.  Use *decodeURIComponent* instead <https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/decodeURIComponent>.

Nit: for robustness, decode the name as well.

@@ +65,5 @@
> +  return query;
> +}
> +
> +function getEtag(request, version) {
> +  return request.queryString.replace(/&/g, '-').replace(/=/g, '-') +

Nit: use a character set <https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/RegExp?redirectlocale=en-US&redirectslug=JavaScript%2FReference%2FGlobal_Objects%2FRegExp#character-sets> to replace both characters with a single regexp, i.e.:

  return request.queryString.replace(/[&=]/g, '-') …

@@ +76,5 @@
> +}
> +
> +// File and resources helpers
> +
> +function readSize(path, fromTmp) {

readSize has only a single caller, which passes `false` to the fromTmp parameter, so it doesn't seem to need that parameter.

@@ +85,5 @@
> +  var fstream = Cc["@mozilla.org/network/file-input-stream;1"]
> +                .createInstance(Ci.nsIFileInputStream);
> +  var split = path.split("/");
> +  var size = 0;
> +  for(var i = 0; i < split.length; ++i) {

Nit: space between *for* and opening parenthesis here and elsewhere.

@@ +91,5 @@
> +  }
> +  fstream.init(file, -1, 0, 0);
> +  size = fstream.available();
> +  fstream.close();
> +  return size;

After creating the nsIFile, you should be able to get its size from its fileSize property; you shouldn't have to read the whole thing.

@@ +95,5 @@
> +  return size;
> +}
> +
> +function readFile(path, fromTmp) {
> +  var dir = fromTmp ? "TmpD" : "CurWorkD";

As with readSize, there's only one caller, and it passes only one value to fromTmp, so this function doesn't need that parameter.

::: dom/apps/tests/test_packaged_app_common.js
@@ +225,1 @@
>      set gApp(aValue) { gApp = aValue; },

Nit: we should group the gSJSPath and gApp getters/setters together instead of sticking the gSJSPath setter in the middle of the gApp getter/setter (and far away from the gSJSPath getter).

::: dom/apps/tests/test_signed_pkg_install.html
@@ +24,5 @@
> +"use strict";
> +
> +var Ci = SpecialPowers.Ci;
> +const Cc = SpecialPowers.Cc;
> +var Cu = SpecialPowers.Cu;

Why are two of these variables while the third is a const?  They should all be able to be consts.

@@ +26,5 @@
> +var Ci = SpecialPowers.Ci;
> +const Cc = SpecialPowers.Cc;
> +var Cu = SpecialPowers.Cu;
> +
> +var launchableValue = undefined;

Nit: don't set a variable to the *undefined* value when declaring it; simply declare it without setting it to a value:

  var launchableValue;

@@ +44,5 @@
> +  navigator.mozApps.mgmt.oninstall = function(evt) {
> +    ok(true, "Got oninstall event");
> +    gApp = evt.application;
> +    gApp.ondownloaderror = function() {
> +      if(gApp.downloadError.name == aExpectedError) {

Nit: add space between *if* and opening parenthesis.

@@ +50,5 @@
> +        PackagedTestHelper.next();
> +      } else {
> +        ok(false, "Download fails but for an unexpected reason " + gApp.downloadError.name);
> +        PackagedTestHelper.finish();
> +      }

There are two related problems here:
* test logic that could be part of a test assertion is instead outside of it, so the test harness doesn't capture and display a failure consistently;
* the meanings of the messages in the two test assertions are reversed: in one, the message says what is expected to happen and did; while in the other it says what is not expected to happen and did.

Test assertion messages should describe what the tests expect to happen, so it's always clear what the test expected in the case of a failure.

In this case, the test expects a particular download error, so it should simply assert that the error it observes is that error:

    gApp.ondownloaderror = function() {
      is(gApp.downloadError.name, aExpectedError,
         "Download fails with expected error: " + aExpectedError);
      …
    }

Then, when the test fails, you can rely on the harness to log the message and the discrepancy between the two values in a way that is consistent with other tests in the tree.

Deciding whether to continue to the next() test or finish() is then a separate concern that the code should determine after making the test assertion.  Although, in general, it's better to assume the test will pass and let the test harness decide what to do when it fails:

    gApp.ondownloaderror = function() {
      is(gApp.downloadError.name, aExpectedError,
         "Download fails with expected error: " + aExpectedError);
      PackagedTestHelper.next();
    }

@@ +77,5 @@
> +    aApp.onprogress = null;
> +    PackagedTestHelper.next();
> +  };
> +  req.onerror = function(evt) {
> +    ok(false, "Got unexpected " + evt.target.error.name);

This message should be something like "app uninstallation should succeed (failed with …)" so it describes what the test expects to happen.

@@ +88,5 @@
> +    // Set up
> +    ok(true, "this is a test");
> +    gSignedAppOriginsStr = SpecialPowers.getCharPref("dom.mozApps.signed_apps_installable_from");
> +    var signedAppOriginsStr = gSignedAppOriginsStr.concat("," + gInstallOrigin.slice(0, -1));
> +    SpecialPowers.pushPrefEnv({'set': [['dom.mozApps.signed_apps_installable_from', signedAppOriginsStr]]}, function () {

Nit: be consistent about whether or not to put a space between the *function* keyword and its parameter list.

@@ +94,5 @@
> +      var script = SpecialPowers.loadChromeScript(url);
> +      script.addMessageListener("addCertCompleted", function (message) {
> +        launchableValue = SpecialPowers.setAllAppsLaunchable(true);
> +        SpecialPowers.addPermission("webapps-manage", true, document);
> +        ok(true, "Set up");

Nit: this is an informational message rather than a test assertion; use SimpleTest's *info* function for such messages (here and below).

@@ +119,5 @@
> +    navigator.mozApps.mgmt.oninstall = function(evt) {
> +      ok(true, "Got oninstall event");
> +      gApp = evt.application;
> +      gApp.ondownloaderror = function() {
> +        ok(false, "Download error " + gApp.downloadError.name);

Make this message describe what the test expects to happen.

@@ +135,5 @@
> +          downloading: false,
> +          readyToApplyDownload: false,
> +        };
> +        PackagedTestHelper.checkAppState(gApp, 1, expected,
> +                                     true, false, PackagedTestHelper.next);

Nit: indent the second line of the parameter list with the first one.

@@ +138,5 @@
> +        PackagedTestHelper.checkAppState(gApp, 1, expected,
> +                                     true, false, PackagedTestHelper.next);
> +      };
> +    };
> +    console.log(miniManifestURL);

Nit: provide context for this log message (f.e. "mini manifest URL: …") or remove it.

@@ +141,5 @@
> +    };
> +    console.log(miniManifestURL);
> +    var request = navigator.mozApps.installPackage(miniManifestURL);
> +    request.onerror = function(evt) {
> +      ok(false, "Application was not able to be installed " + JSON.stringify(evt));

Make this message describe what the test expects to happen.

@@ +169,5 @@
> +    // Scenario: This is where an unexpected store is signing packages and attempting to install
> +    SpecialPowers.pushPrefEnv({'set': [['dom.mozApps.signed_apps_installable_from', gSignedAppOriginsStr]]}, function () {
> +      var miniManifestURL = gSJS + "?" +
> +                          "app=valid&" +
> +                          "version=1";

Nit: fix the indentation of the second and third lines.

@@ +171,5 @@
> +      var miniManifestURL = gSJS + "?" +
> +                          "app=valid&" +
> +                          "version=1";
> +      checkAppOnInstallError(miniManifestURL, "INSTALL_FROM_DENIED");
> +   });

Nit: indent this line one more space.

@@ +174,5 @@
> +      checkAppOnInstallError(miniManifestURL, "INSTALL_FROM_DENIED");
> +   });
> +  },
> +  function() {
> +    ok(true, "all done!\n");

Nit: ok -> info, remove the spurious newline.

Comment 19

[Antonio Manuel Amaya Calvo (:amac)]

Are you still working on this, David? If your workload doesn't let you close this, I volunteer myself to help ;)

Comment 20

[[e-mail] [:onecyrenus]]

Antonio, no problem please take this off my plate :) .. It works as is, it just needs to be re formatted so it can be checked in. 

I can walk you through how i created all the signed apps as well. It is not entirely linear.

Comment 21

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: Part 1. Installation tests, with auxiliary files

This is basically Dave's patch, with the last review comments addressed (except the providing scripts to regenerate the binary test files comment).

I'll upload another patch with the scripts to regenerate the test files.

Comment 22

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: Part 2. Binary test files generator

The script included on this patch generates direct replacements for the files included on the P1 patch (both for the CA certificate and the test applications).

The execution is quite straightforward, just cd /dom/apps/tests/signed/gentestfiles and execute create_test_files.sh

Comment 23

[Antonio Manuel Amaya Calvo (:amac)]

Comment on attachment 8362876 [details] [diff] [review]
Part 1. Installation tests, with auxiliary files

Cancelling review because I'm going to add the test for bug 960601 here also (since it belongs here and it should be a very small increase on this patch).

Comment 24

[Antonio Manuel Amaya Calvo (:amac)]

Comment on attachment 8362914 [details] [diff] [review]
Part 2. Binary test files generator

Cancelling review here to add the file needed to test bug 960601. Will resubmit once that's done.

Comment 25

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: Part 1. Installation tests, with auxiliary files

This is Dave's patch, adressing the previous review comments. It also includes a new test case, to test the installation of signed apps that include a origin (to avoid repeats of bug 960601). As such, that test *fails* (correctly) if the patch from bug 960601 isn't applied.

The scripts to regenerate the binary files in this patch are included on Part 2. The files on this patch have been generated using that scripts.

Comment 26

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: Part 2. Binary test files generator

This script generates the test case files. The binary files (zips) included on the patch are raw files (no signature) and are included in binary form just to avoid repackaging them on the generating script.

To regenerate the test case files, just cd to dom/apps/tests/signed/gentestfiles and execute create_test_files.sh there.

Comment 27

[Antonio Manuel Amaya Calvo (:amac)]

Try run at: 
https://tbpl.mozilla.org/?tree=Try&rev=e9cc593d8df8 
(including the proposed patch for bug 960601)

Comment 28

[[e-mail] [:onecyrenus]]

Comment on attachment 8363068 [details] [diff] [review]
Part 1. Installation tests, with auxiliary files

seems reasonable + passes.

Comment 29

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

Comment on attachment 8363068 [details] [diff] [review]
Part 1. Installation tests, with auxiliary files

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

Huge improvement, thanks amac!

I have a bunch of nits, but no substantial issues, so r=myk.

Note that the mochitest.ini change conflicts with a recent change, although it's trivial to resolve the conflict:

error: patch failed: dom/apps/tests/mochitest.ini:1
error: dom/apps/tests/mochitest.ini: patch does not apply

::: dom/apps/tests/chromeAddCert.js
@@ +1,4 @@
> +const Ci = Components.interfaces;
> +const Cc = Components.classes;
> +
> +var DEBUG_ADD_CERT=1;

Nit: if this is actually a var, then it shouldn't use ALL_CAPS style.  But it looks like it isn't modified after initialization and thus can be const.  So the style is fine, but the declaration should use const.

Also, nit: add a space to each side of assignment operator (=).

Erm, but are these messages intended to be enabled by default?  If so, why not simply dump them unconditionally?

@@ +3,5 @@
> +
> +var DEBUG_ADD_CERT=1;
> +
> +addMessageListener("addCert", function ({trust, name, url}) {
> +

Nit: remove this line of extraneous whitespace (but it wouldn't hurt to add such lines periodically throughout this long function body to break it up into logical blocks).

@@ +14,5 @@
> +                  createInstance(Ci.nsIBinaryInputStream);
> +  bstream.setInputStream(stream);
> +  let size = 0;
> +  let cert = "";
> +  while( size = bstream.available() ) {

Nits:

* remove extraneous whitespace from around the conditional expression for consistency with the style of other conditional expressions in this file;
* add a space between "while" and the opening parenthesis;
* assignments within conditional expressions trigger warnings in linters, hinters, and SpiderMonkey's javascript.options.strict mode, so suppress the warning (in some contexts, anyway) by wrapping the expression in parentheses, i.e.:

  while ((size = bstream.available())) {

::: dom/apps/tests/mochitest.ini
@@ +18,5 @@
>  [test_packaged_app_common.js]
>  [test_packaged_app_install.html]
>  [test_packaged_app_update.html]
> +[test_receipt_operations.html]
> +[test_signed_pkg_install.html]

Thanks for alphabetizing these!  Note that a new one, test_install_receipts.html, has been stuck to the end of the list.  Perhaps it's worth adding a comment asking folks to keep the list alphabetized.  Or maybe we reviewers just need to keep an eye out.

::: dom/apps/tests/signed/Makefile.in
@@ +3,5 @@
> +# You can obtain one at http://mozilla.org/MPL/2.0/.$
> +
> +MOCHITEST_FILES= \
> +    corrupt_app_1.zip \
> +    moz.build \

I don't think you need to (nor should) include moz.build in the list of test files.  It's a build configuration script that the test runner shouldn't need to access in order to run the tests.

::: dom/apps/tests/signed_app.sjs
@@ +56,5 @@
> +function getQuery(request) {
> +  var query = {};
> +  request.queryString.split('&').forEach(function (val) {
> +    var [name, value] = val.split('=');
> +    query[decodeURIComponent(name)] = decodeURIComponent(value);

Good idea to use decodeURIComponent on the name as well!  Query parameter names don't usually have encoded characters, but they can have them, so decoding them improves the robustness of this generic method.

@@ +75,5 @@
> +
> +function readSize(path) {
> +  var dir = "CurWorkD";
> +  var file = Cc["@mozilla.org/file/directory_service;1"].
> +               getService(Ci.nsIProperties).get(dir, Ci.nsILocalFile);

Nit: since *dir* is only used once, it might as well be a literal string in the get() call.  Alternately, since readFile also references it, make it a global const CUR_WORK_DIR.

@@ +77,5 @@
> +  var dir = "CurWorkD";
> +  var file = Cc["@mozilla.org/file/directory_service;1"].
> +               getService(Ci.nsIProperties).get(dir, Ci.nsILocalFile);
> +  var split = path.split("/");
> +  var size = 0;

Nit: this variable is never used and should be removed.

::: dom/apps/tests/test_packaged_app_common.js
@@ +181,5 @@
>           "Check readyToApplyDownload");
>      }
> +    if (typeof aExpectedApp.origin !== "undefined") {
> +      is(aApp.origin, aExpectedApp.origin,
> +         "Check origin");

Nit: like the downloadSize assertion two assertions above it, this assertion can fit on a single line.

::: dom/apps/tests/test_signed_pkg_install.html
@@ +46,5 @@
> +    gApp = evt.application;
> +    gApp.ondownloaderror = function() {
> +      is(gApp.downloadError.name, aExpectedError,
> +        "Download fails with expected error: " + aExpectedError);
> +      if(gApp.downloadError.name != aExpectedError) {

Nit: space between "if" and opening parenthesis.

@@ +50,5 @@
> +      if(gApp.downloadError.name != aExpectedError) {
> +        PackagedTestHelper.finish();
> +      } else {
> +        PackagedTestHelper.next();
> +      }

Nit: here and below, it would be better to expect that the test will pass and let the test harness decide what to do when it fails instead of explicitly calling PackagedTestHelper.finish() on test failure.

@@ +118,5 @@
> +    navigator.mozApps.mgmt.oninstall = function(evt) {
> +      info("Got oninstall event");
> +      gApp = evt.application;
> +      gApp.ondownloaderror = function() {
> +        ok(false, "Download should succeed (got error: " + 

Nit: remove trailing whitespace.

@@ +175,5 @@
> +    navigator.mozApps.mgmt.oninstall = function(evt) {
> +      info("Got oninstall event");
> +      gApp = evt.application;
> +      gApp.ondownloaderror = function() {
> +        ok(false, "Download should succeed (got error: " + 

Nit: remove trailing whitespace.

@@ +209,5 @@
> +  },
> +  function() {
> +    info("== TEST == Install app from an invalid source");
> +    // Scenario: This is where an unexpected store is signing packages and
> +    // attempting to install. Please note that after this test you cannot 

Nit: remove trailing whitespace.

Comment 30

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

Comment on attachment 8363072 [details] [diff] [review]
Part 2. Binary test files generator

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

It's difficult to review this without instructions, but I gather that I need NSS, which I installed via brew on my Mac (and manually add to my path).  I still get errors when running the shell script, though, due to a crash in Python:

01-27 14:14 > DYLD_LIBRARY_PATH=$DYLD_LIBRARY_PATH:/usr/local/Cellar/nss/3.14.1/lib PATH=$PATH:/usr/local/Cellar/nss/3.14.1/bin ./create_test_files.sh 
Warning! The directories ./signingDB and ./testApps will be erased!
Do you want to proceed anyway?
1) Yes
2) No
#? 1
head: illegal option -- -
usage: head [-n lines | -c bytes] [file ...]


Generating key.  This may take a few moments...

…

./create_test_files.sh: line 72:  5114 Segmentation fault: 11  python `dirname $0`/sign_b2g_app.py -d $db -f $passwordfile -k ${6} -i ${2} -o ${3} -S ${4} -V ${5}
./create_test_files.sh: line 72:  5117 Segmentation fault: 11  python `dirname $0`/sign_b2g_app.py -d $db -f $passwordfile -k ${6} -i ${2} -o ${3} -S ${4} -V ${5}
  End-of-central-directory signature not found.  Either this file is not
  a zipfile, or it constitutes one disk of a multi-part archive.  In the
  latter case the central directory and zipfile comment will be found on
  the last disk(s) of this archive.
unzip:  cannot find zipfile directory in one of ../valid_app_1.zip or
        ../valid_app_1.zip.zip, and cannot find ../valid_app_1.zip.ZIP, period.
./create_test_files.sh: line 72:  5127 Segmentation fault: 11  python `dirname $0`/sign_b2g_app.py -d $db -f $passwordfile -k ${6} -i ${2} -o ${3} -S ${4} -V ${5}
./create_test_files.sh: line 72:  5130 Segmentation fault: 11  python `dirname $0`/sign_b2g_app.py -d $db -f $passwordfile -k ${6} -i ${2} -o ${3} -S ${4} -V ${5}


Here's the stack trace:

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   libsystem_c.dylib             	0x00007fff8cb2e812 strlen + 18
1   libnss3.dylib                 	0x000000010ed960a0 PK11_DoPassword + 424
2   libnss3.dylib                 	0x000000010ed960a0 PK11_DoPassword + 424
3   libnss3.dylib                 	0x000000010ed97827 PK11_FindKeyByAnyCert + 137
4   libsmime3.dylib               	0x000000010eef026d sec_pkcs7_encoder_sig_and_certs + 352
5   libsmime3.dylib               	0x000000010eef06b3 SEC_PKCS7PrepareForEncode + 188
6   libsmime3.dylib               	0x000000010eef074d SEC_PKCS7Encode + 39
7   _ctypes.so                    	0x000000010ecf5007 ffi_call_unix64 + 79
8   _ctypes.so                    	0x000000010ecf5815 ffi_call + 813
9   _ctypes.so                    	0x000000010ecf0ffa _ctypes_callproc + 826
10  _ctypes.so                    	0x000000010eceb613 PyCFuncPtr_call + 907
11  org.python.python             	0x000000010e7c522b PyObject_Call + 101
12  org.python.python             	0x000000010e83f36c PyEval_EvalFrameEx + 15525
13  org.python.python             	0x000000010e83b59d PyEval_EvalCodeEx + 1638
14  org.python.python             	0x000000010e84198f fast_function + 282
15  org.python.python             	0x000000010e83e795 PyEval_EvalFrameEx + 12494
16  org.python.python             	0x000000010e84192b fast_function + 182
17  org.python.python             	0x000000010e83e795 PyEval_EvalFrameEx + 12494
18  org.python.python             	0x000000010e84192b fast_function + 182
19  org.python.python             	0x000000010e83e795 PyEval_EvalFrameEx + 12494
20  org.python.python             	0x000000010e84192b fast_function + 182
21  org.python.python             	0x000000010e83e795 PyEval_EvalFrameEx + 12494
22  org.python.python             	0x000000010e83b59d PyEval_EvalCodeEx + 1638
23  org.python.python             	0x000000010e83af31 PyEval_EvalCode + 54
24  org.python.python             	0x000000010e859e08 run_mod + 53
25  org.python.python             	0x000000010e859eaf PyRun_FileExFlags + 137
26  org.python.python             	0x000000010e8599fd PyRun_SimpleFileExFlags + 718
27  org.python.python             	0x000000010e86a56b Py_Main + 3039
28  libdyld.dylib                 	0x00007fff8e95f5fd start + 1


Does nss_ctypes depend on a certain version of NSS?


Also, it isn't clear what unsigned_app_origin.zip is for, as I don't see it being used anywhere.  And unsigned_app_1.zip presents us with the same issue we had with the test packages: it is a binary whose source is not readily available.  This patch should also include the source of that file, so it's possible to regenerate the file from changed source and commit the entire changeset in a way that provides a reasonable diff of the changeset in the repository log.

Comment 31

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

(In reply to Myk Melez [:myk] [@mykmelez] from comment #30)
> Also, it isn't clear what unsigned_app_origin.zip is for, as I don't see it
> being used anywhere.

Erm, never mind this comment, as I found the code that uses it.

Comment 32

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: Part 2. Binary test files generator, V2

This version should fix the crashes running the native code from Python on a Mac detected by Myk.

Also I added a README file to explain the prerequisites for the script to work.

Comment 33

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: Part 2. Binary test files generator, V3

Same as V2, but with the directories I forgot to add added now

Comment 34

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Myk Melez [:myk] [@mykmelez] from comment #30)
> Comment on attachment 8363072 [details] [diff] [review]
> 
> Does nss_ctypes depend on a certain version of NSS?

I assume it needs at least 3.4 or so, but the problem wasn't that. The problem is that on Linux Python doesn't require you to specify the return values of the native functions, while for Mac it does. The sad part is I already knew that... but used an old version of the signing script. Shame on me.

Thanks for detecting this! This version is tested on Linux (Ubuntu) and on MacOs.

Comment 35

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

Comment on attachment 8366536 [details] [diff] [review]
Part 2. Binary test files generator, V3

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

Thanks, amac, this works as expected!


Nit: lots of lines add "whitespace errors" (trailing whitespace):

01-28 02:12 > git apply ~/880043-gencert_2.patch
/Users/myk/880043-gencert_2.patch:24: trailing whitespace.
Run 
/Users/myk/880043-gencert_2.patch:590: trailing whitespace.
<!doctype html>
/Users/myk/880043-gencert_2.patch:591: trailing whitespace.
<html lang=en>
/Users/myk/880043-gencert_2.patch:592: trailing whitespace.
<head><meta charset=utf-8><title>Simple App</title></head>
/Users/myk/880043-gencert_2.patch:593: trailing whitespace.
<body><p>This is a Simple App.</body>
warning: squelched 11 whitespace errors
warning: 16 lines add whitespace errors.


Also, I just realized that there's an unsigned_app_1.zip file and an unsigned_app_2.zip file (which is the same size but differs according to binary diff) in dom/apps/tests/signed/ (per Part 1), but neither file appears to be used by tests.  If that's really the case, then we should remove them from the patch (and their entries from the Makefile).  Otherwise, if they are actually used by tests, then this generator should generate them in gentestfiles/testApps/ (or copy them from gentestfiles/, in the case of unsigned_app_1.zip).

Comment 36

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Myk Melez [:myk] [@mykmelez] from comment #35)
> Comment on attachment 8366536 [details] [diff] [review]
> Nit: lots of lines add "whitespace errors" (trailing whitespace):
> ...
> <body><p>This is a Simple App.</body>
> warning: squelched 11 whitespace errors
> warning: 16 lines add whitespace errors.

Hmm... I think this is because that files are in DOS mode (they have the DOS endlines). I'll remove the extra ^Ms.

> Also, I just realized that there's an unsigned_app_1.zip file and an
> unsigned_app_2.zip file (which is the same size but differs according to
> binary diff) in dom/apps/tests/signed/ (per Part 1), but neither file
> appears to be used by tests.  If that's really the case, then we should
> remove them from the patch (and their entries from the Makefile). 
> Otherwise, if they are actually used by tests, then this generator should
> generate them in gentestfiles/testApps/ (or copy them from gentestfiles/, in
> the case of unsigned_app_1.zip).

I didn't regenerate/copy those files because they're raw zips (so regenerating them is just unzipping/modifying/rezipping them). In any case, you're right that for completions sake they should be generated too. I'll change that too.

Since those two changes are minor, if you don't mind I'll wait until Brian reviews this before uploading a new version.

Comment 37

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

(In reply to Antonio Manuel Amaya Calvo (:amac) from comment #36)
> I didn't regenerate/copy those files because they're raw zips (so
> regenerating them is just unzipping/modifying/rezipping them). In any case,
> you're right that for completions sake they should be generated too. I'll
> change that too.

Ah, and I see now where they're used.  signed_app.sjs constructs packageName, i.e. the name of the ZIP archive, from the "app" and "version" query parameters.


> Since those two changes are minor, if you don't mind I'll wait until Brian
> reviews this before uploading a new version.

No worries, that's fine!

Comment 38

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8366536 [details] [diff] [review]
Part 2. Binary test files generator, V3

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

r- because nss_ctypes.py and sign_b2g_app.py seem to be copy/pasted from security/manager/ssl/tests/unit/test_signed_apps. We should not have two copies of this stuff that we have to keep in sync. If PSM is not a good place for such scripts to live then let's put them in some other, common, place.

Please ask me to re-review once the duplication issue has been taken care of.

Comment 39

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: Part 2. Binary test files generator, V4

I changed the shell script to use the .py on the security/manager directory, and changed that script to work when invoked from anywhere else. For the rest, is the same patch than V3.

Comment 40

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: Part 1. Installation tests, with auxiliary files, V2

r=myk

This addresses all the nits except for

@@ +50,5 @@
> +      if(gApp.downloadError.name != aExpectedError) {
> +        PackagedTestHelper.finish();
> +      } else {
> +        PackagedTestHelper.next();
> +      }

Nit: here and below, it would be better to expect that the test will pass and let the test harness decide what to do when it fails instead of explicitly calling PackagedTestHelper.finish() on test failure.

Due to the way the test is written, either next or finish has to be called explicitly. Otherwise, the test would finish by timeout and that would delay the rest of the tests.

Comment 41

[[:fabrice] Fabrice Desré]

(In reply to Antonio Manuel Amaya Calvo (:amac) from comment #40)

> Due to the way the test is written, either next or finish has to be called
> explicitly. Otherwise, the test would finish by timeout and that would delay
> the rest of the tests.

But are we getting a test failure (gApp.downloadError.name != aExpectedError) ? I think we want, right? I agree that failing fast is better that letting the test timeout, which is mostly a "this test has an intermittent failure" situation that is unhandled.

Comment 42

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Fabrice Desré [:fabrice] from comment #41)
> (In reply to Antonio Manuel Amaya Calvo (:amac) from comment #40)
> 
> > Due to the way the test is written, either next or finish has to be called
> > explicitly. Otherwise, the test would finish by timeout and that would delay
> > the rest of the tests.
> 
> But are we getting a test failure (gApp.downloadError.name !=
> aExpectedError) ? I think we want, right? I agree that failing fast is
> better that letting the test timeout, which is mostly a "this test has an
> intermittent failure" situation that is unhandled.

No, usually the test is working (so it calls next()). In case it failed some day, it should call finish and... well, finish immediately :).

Comment 43

[[:fabrice] Fabrice Desré]

Sure, but is this kind of finish() and error or a success overall for the test? Should we care about that?

Comment 44

[Antonio Manuel Amaya Calvo (:amac)]

Oh, sorry, I misunderstood the question. It's a failure and there's an is() test before that if. The if is just to finish the test immediately after the is.

Comment 45

[[:fabrice] Fabrice Desré]

(In reply to Antonio Manuel Amaya Calvo (:amac) from comment #44)
> Oh, sorry, I misunderstood the question. It's a failure and there's an is()
> test before that if. The if is just to finish the test immediately after the
> is.

Ok!

Comment 46

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Antonio, do we really need to generate new certificates for these tests? Can we not use the same certificates that are used for the xpcshell tests? The reason I ask is that in bug 896620 we're almost definitely going to be changing how the reviewer cert mechanism works and how the automated-test cert mechanism works, and it would be much simpler if there was only one trusted root certificate used for both mochitest and xpcshell.

More generally, can't we just re-use the data files that are in the xpcshell tests (test_signed_apps.js and test_signed_apps-marketplace.js), expanding that set if necessary?

Comment 47

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8368559 [details] [diff] [review]
Part 2. Binary test files generator, V4

Please re-request review after answering the NEEDINFO.

Comment 48

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #46)
> Antonio, do we really need to generate new certificates for these tests? Can
> we not use the same certificates that are used for the xpcshell tests? The
> reason I ask is that in bug 896620 we're almost definitely going to be
> changing how the reviewer cert mechanism works and how the automated-test
> cert mechanism works, and it would be much simpler if there was only one
> trusted root certificate used for both mochitest and xpcshell.
> 
> More generally, can't we just re-use the data files that are in the xpcshell
> tests (test_signed_apps.js and test_signed_apps-marketplace.js), expanding
> that set if necessary?

Hmm yes and no. The idea behind this script set was to allow regenerating the signed files and I can't do that with the preexisting files (nor can I add more files) because there's no private key anywhere that I can see on the repo. That's also why I have to regenerate the certs and private keys (because they're not included). 

In any case, I think that the files used for the xpcshell tests are a subset of the files used for the mochitests. I'm thinking that I can: 

* Move the scripts to the xpcshell part so they're all together.
* Change them so they generate the files used for both tests.
* Change the instructions to tell where should the generated files be copied if they're regenerated.
* Regenerate a first set for all the files and add that to the test patch (part 1). I will have to generate a new test CA here because I don't have the one used to generate the current files.
* Modify security/manager/ssl/tests/unit/test_signed_apps/trusted_ca1.der so it's the same cert as dom/apps/tests/signed/trusted_ca1.der (I can also try loading the .der from the security/manager/... directory on dom/apps/tests/chromeAddCert.js but I don't know if at runtime that directory is accessible from the mochitest one).

Then when the signature checking is changed, the certificate will have to be added just at one place (assuming the addition survives the change from one type of test to the other, or is done at tests setup time).

Would that be enough?

Comment 49

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Antonio Manuel Amaya Calvo (:amac) from comment #48)
> Hmm yes and no. The idea behind this script set was to allow regenerating
> the signed files and I can't do that with the preexisting files (nor can I
> add more files) because there's no private key anywhere that I can see on
> the repo. That's also why I have to regenerate the certs and private keys
> (because they're not included). 

Every time we generate new zip files, we need to generate a new certificate and keypair. The private key for the certificate is intentionally not checked in so, because that reduces the risk of accidentally trusting the xpcshell certificate,sSince nobody can get the private key except the person that regenerated the cert and the test apps.

> * Move the scripts to the xpcshell part so they're all together.
> * Change them so they generate the files used for both tests.
> * Change the instructions to tell where should the generated files be copied
> if they're regenerated.
> * Regenerate a first set for all the files and add that to the test patch
> (part 1). I will have to generate a new test CA here because I don't have
> the one used to generate the current files.
> * Modify security/manager/ssl/tests/unit/test_signed_apps/trusted_ca1.der so
> it's the same cert as dom/apps/tests/signed/trusted_ca1.der (I can also try
> loading the .der from the security/manager/... directory on
> dom/apps/tests/chromeAddCert.js but I don't know if at runtime that
> directory is accessible from the mochitest one).
> 
> Then when the signature checking is changed, the certificate will have to be
> added just at one place (assuming the addition survives the change from one
> type of test to the other, or is done at tests setup time).
> 
> Would that be enough?

Yes, this sounds good. It would be great if we could avoid having two copies of the certificate, but if we need to have two copies, it isn't the end of the world.

Comment 50

[Antonio Manuel Amaya Calvo (:amac)]

I was doing this today, and I've seen that bug 896620 has landed. Well, I actually have seen that after the tests started failing. The way bug 896820 has implemented the certificate pinning makes finishing this bug impossible without touching at least Webapps.jsm again.

[1] defines a pinned certificate, Ci.nsIX509CertDB.AppMarketplaceProdPublicRoot as the one and only valid certificate for app signing. That is a constant defined at the IDL. And the actual value for the certificate is generated at build time at [2], and set at [3] and cannot be configured at runtime. That basically means that as it is I can't think of a way to generate a mochitest for mozApps.installPackage for a signed app (other than checking in a set of apps signed with the real marketplace cert, which would be easy but kinda defeats the 'add a way to regenerate the binary files' part).

Note that the XPC test at security/manager/ssl/tests/unit/test_signed_apps.js works because it is *not* testing the installation of apps, just the signature (and it's passing a different certificate for the check). 

[1] http://hg.mozilla.org/mozilla-central/diff/8cdaaf3da9f8/dom/apps/src/Webapps.jsm
[2] http://hg.mozilla.org/mozilla-central/diff/8cdaaf3da9f8/security/apps/Makefile.in
[3] http://hg.mozilla.org/mozilla-central/annotate/e3daaa4c73dd/security/apps/AppTrustDomain.cpp#l40

Comment 51

[[:fabrice] Fabrice Desré]

I'll let Brian answer that.

Comment 52

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Antonio Manuel Amaya Calvo (:amac) from comment #50)
> [1] defines a pinned certificate,
> Ci.nsIX509CertDB.AppMarketplaceProdPublicRoot as the one and only valid
> certificate for app signing.

See also the patch I attached to bug 889744.

It also defines Ci.nsIX509CertDB.AppXPCShellRoot. The idea is that for any kind of testing, we need to make the test use AppXPCShellRoot instead of AppMarketplaceProdPublicRoot. In particular, for this bug we need to figure out a way to let Webapps.jsm know that it is being tested so it can use Ci.nsIX509CertDB.AppXPCShellRoot instead of Ci.nsIX509CertDB.AppMarketplaceProdPublicRoot.

> That basically means that as it
> is I can't think of a way to generate a mochitest for mozApps.installPackage
> for a signed app (other than checking in a set of apps signed with the real
> marketplace cert, which would be easy but kinda defeats the 'add a way to
> regenerate the binary files' part).

I suggest:

1. Update the script for generating the test apps in security/manager/ssl/tests/unit/test_signed_apps to generate the additional test apps that you need for mochitests.
2. Run the generation script. This will cause a new xpcshell cert, a new xpcshell keypair, and new signed test apps to be generated.
3. Rebuild Gecko. This will cause the hard-coded xpcshell cert in AppTrustDomain to be updated to the new xpcshell cert generated in step #2.
4. Modify Webapps.jsm so that it uses Ci.nsIX509CertDB.AppXPCShellRoot instead of 
Ci.nsIX509CertDB.AppMarketplaceProdPublicRoot when running these mochitests. (Extend SpecialPowers or whatever so that you can communicate the fact that we're running the mochitests to Webapps.jsm. Fabrice or others can help with this; I don't know how it is done.)


> Note that the XPC test at
> security/manager/ssl/tests/unit/test_signed_apps.js works because it is
> *not* testing the installation of apps, just the signature (and it's passing
> a different certificate for the check). 

Right. We need to follow the same pattern for the mochitests.

Sorry that I did not get this communicated to you. I had assumed that you would see the thread on dev-webapps about this, but I should have also reached out to you individually. Sorry about any miscommunication.

Comment 53

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #52)
> (In reply to Antonio Manuel Amaya Calvo (:amac) from comment #50)
> > [1] defines a pinned certificate,
> > Ci.nsIX509CertDB.AppMarketplaceProdPublicRoot as the one and only valid
> > certificate for app signing.
> 
> See also the patch I attached to bug 889744.
> 
> It also defines Ci.nsIX509CertDB.AppXPCShellRoot. The idea is that for any
> kind of testing, we need to make the test use AppXPCShellRoot instead of
> AppMarketplaceProdPublicRoot. In particular, for this bug we need to figure
> out a way to let Webapps.jsm know that it is being tested so it can use
> Ci.nsIX509CertDB.AppXPCShellRoot instead of
> Ci.nsIX509CertDB.AppMarketplaceProdPublicRoot.
> 
> > That basically means that as it
> > is I can't think of a way to generate a mochitest for mozApps.installPackage
> > for a signed app (other than checking in a set of apps signed with the real
> > marketplace cert, which would be easy but kinda defeats the 'add a way to
> > regenerate the binary files' part).
> 
> I suggest:
> 
> 1. Update the script for generating the test apps in
> security/manager/ssl/tests/unit/test_signed_apps to generate the additional
> test apps that you need for mochitests.
> 2. Run the generation script. This will cause a new xpcshell cert, a new
> xpcshell keypair, and new signed test apps to be generated.
> 3. Rebuild Gecko. This will cause the hard-coded xpcshell cert in
> AppTrustDomain to be updated to the new xpcshell cert generated in step #2.
I had already done all of this yesterday before my last comment (I only realized the verification had changed when that didn't work :))


> 4. Modify Webapps.jsm so that it uses Ci.nsIX509CertDB.AppXPCShellRoot
> instead of 
> Ci.nsIX509CertDB.AppMarketplaceProdPublicRoot when running these mochitests.
> (Extend SpecialPowers or whatever so that you can communicate the fact that
> we're running the mochitests to Webapps.jsm. Fabrice or others can help with
> this; I don't know how it is done.)

That's what I was talking about on my last comment when I said that I couldn't figure how to do this without modifying Webapps.jsm again. And that's why I set the ni for Fabrice :). I know how to change that, but I would prefer not having to do it. Changing the code just so the tests can run is kinda ugly :P. 

Anyway, if there's no other way and Fabrice doesn't oppose, I'll do just that. Fabrice, WDYT?

> 
> 
> > Note that the XPC test at
> > security/manager/ssl/tests/unit/test_signed_apps.js works because it is
> > *not* testing the installation of apps, just the signature (and it's passing
> > a different certificate for the check). 
> 
> Right. We need to follow the same pattern for the mochitests.
> 
> Sorry that I did not get this communicated to you. I had assumed that you
> would see the thread on dev-webapps about this, but I should have also
> reached out to you individually. Sorry about any miscommunication.

Ah no worries :).

Comment 54

[[:fabrice] Fabrice Desré]

Yeah, I don't like test-specific codepaths either but the pandora box is already open anyway in Webapps.jsm. I think it's more important to get these tests running.

Comment 55

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Fabrice Desré [:fabrice] from comment #54)
> Yeah, I don't like test-specific codepaths either but the pandora box is
> already open anyway in Webapps.jsm. I think it's more important to get these
> tests running.

Ok, I'll try and do this tomorrow then :)

Comment 56

[Antonio Manuel Amaya Calvo (:amac)]

Just a quick update. I've done as I said on my previous comment already... but now the test isn't working because the ondownloadsuccess event isn't being fired for the app-with-an-origin test. I'm trying to find out why that happens (I don't think it's related with the changing the trust anchor at runtime since the normal app installation test passes successfully, but you never know :)).

Comment 57

[Antonio Manuel Amaya Calvo (:amac)]

And that's bug 978089 now.

Comment 58

[Carmen Jimenez Cabezas]

Attachment: Part 2: Generate test certificate and apps, v5

This version creates the CA .der only on one place, and generates the test app files for both the standalone signature XPC tests and for the app installation tests.

The trick to allow changing the trust anchor at run time for the app installation is on part 1. This test only generates the files (and includes a initial set of files).

Comment 59

[Antonio Manuel Amaya Calvo (:amac)]

Sorry about that, the last comment was actually me. I should remember to set the auto log off here :)

Comment 60

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: Part 1: Tests and Webapps change to allow then to run, v3

This adds the changes needed to run the tests with the new signature checking. It's mostly the reviewed patches with the following changes:

* Changed some test messages from ok(true to info(
* Added a default timeout so we don't have to wait 5 minutes when a message/event is lost (which hopefully will stop happening some time soon ;)).
* Added a way to change the default trust anchor from the test code.

Comment 61

[Antonio Manuel Amaya Calvo (:amac)]

Try run at https://tbpl.mozilla.org/?tree=Try&rev=c14f8c124384

Comment 62

[Antonio Manuel Amaya Calvo (:amac)]

Test run failed with
100 INFO TEST-UNEXPECTED-FAIL | /tests/dom/apps/tests/test_signed_pkg_install.html | Timeout! Probably waiting on a app installation event

Which actually was expected ;) It's caused by 978089.

Comment 63

[Antonio Manuel Amaya Calvo (:amac)]

New try run at https://tbpl.mozilla.org/?tree=Try&rev=855d399932ad

Comment 64

[[:fabrice] Fabrice Desré]

Comment on attachment 8383871 [details] [diff] [review]
Part 1: Tests and Webapps change to allow then to run, v3

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

Thanks Antonio. I want to take a look at the new version. The tests looks fine to me but I'd like someone else to also take a look. bsmith or ferjm?

::: dom/apps/src/StoreTrustAnchor.jsm
@@ +16,5 @@
> +                          "AppMarketplaceDevPublicRoot",
> +                          "AppMarketplaceDevReviewersRoot",
> +                          "AppXPCShellRoot"];
> +
> +const NUM_TRUSTED_ROOTS = APP_TRUSTED_ROOTS.length;

no need for a global const.

@@ +30,5 @@
> +      let found = false;
> +      let i = 0;
> +      while (!found && (i < NUM_TRUSTED_ROOTS)) {
> +        found = (Ci.nsIX509CertDB[APP_TRUSTED_ROOTS[i++]] === aIndex);
> +      };

that's very c like ;) Please switch to Array.some() instead.

::: dom/apps/src/moz.build
@@ +34,5 @@
>  
>  EXTRA_PP_JS_MODULES += [
>      'AppsUtils.jsm',
>      'OperatorApps.jsm',
> +    'StoreTrustAnchor.jsm',

no need to preprocess this jsm

Comment 65

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: Part 1. Installation tests, with auxiliary files, v4

Including the last review comments.

Can you take a look to the tests as per Fabrice request on the last review, Fernando?

Comment 66

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8385463 [details] [diff] [review]
Part 1. Installation tests, with auxiliary files, v4

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

LGTM! Thanks Antonio. Only a few style nits on non-test files.

::: dom/apps/src/StoreTrustAnchor.jsm
@@ +22,5 @@
> +  get index() {
> +    return this._index;
> +  },
> +  set index(aIndex) {
> +      // aIndex should be one of the

nit: alignment of the whole block (2 spaces to the left).

@@ +23,5 @@
> +    return this._index;
> +  },
> +  set index(aIndex) {
> +      // aIndex should be one of the
> +      // ci.nsIX509CertDB AppTrustedRoot defined values

nit: "Ci"

@@ +24,5 @@
> +  },
> +  set index(aIndex) {
> +      // aIndex should be one of the
> +      // ci.nsIX509CertDB AppTrustedRoot defined values
> +      let found = APP_TRUSTED_ROOTS.some(function(trustRoot) {

nit: the new trend is to use '=>'

((trustRoot) => {...})

::: dom/apps/tests/signed_app.sjs
@@ +9,5 @@
> +
> +const CUR_WORK_DIR = "CurWorkD";
> +
> +
> +Cu.import("resource://gre/modules/FileUtils.jsm");

Is this used?

Comment 67

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8385463 [details] [diff] [review]
Part 1. Installation tests, with auxiliary files, v4

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

::: dom/apps/src/moz.build
@@ +34,5 @@
>  
>  EXTRA_PP_JS_MODULES += [
>      'AppsUtils.jsm',
>      'OperatorApps.jsm',
> +    'StoreTrustAnchor.jsm',

You still need to address Fabrice's feedback from comment 64

Comment 68

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Fernando Jiménez Moreno [:ferjm] from comment #67)
> Comment on attachment 8385463 [details] [diff] [review]
> Part 1. Installation tests, with auxiliary files, v4
> 
> Review of attachment 8385463 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/apps/src/moz.build
> @@ +34,5 @@
> >  
> >  EXTRA_PP_JS_MODULES += [
> >      'AppsUtils.jsm',
> >      'OperatorApps.jsm',
> > +    'StoreTrustAnchor.jsm',
> 
> You still need to address Fabrice's feedback from comment 64

Agh! I *did* change that. Then I noticed I had an old version of the patch on this computer, and redownloaded the latest version from bugzilla and apparently forgot to remake the change.

I'll upload a new version with your nits (and this!) later today.

Comment 69

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: Part 1: Tests, webapps change and additional files, V5

I'm not obsoleting V4 still in case you're already reviewing it, Fabrice. This is the version r+ed by ferjm, with his requested changed (and I hope with the moz.build change you requested).

Comment 70

[Antonio Manuel Amaya Calvo (:amac)]

Brian, do you want me to assign the review to somebody else? I would like to finish this before more patches land that change Webapps again :)

Comment 71

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Antonio Manuel Amaya Calvo (:amac) from comment #70)
> Brian, do you want me to assign the review to somebody else? I would like to
> finish this before more patches land that change Webapps again :)

Yes, please assign the review to somebody else. I sent you email.

Comment 72

[Antonio Manuel Amaya Calvo (:amac)]

Fabrice, do you know who can review the certificate generation patch?

Comment 73

[Antonio Manuel Amaya Calvo (:amac)]

Comment on attachment 8383843 [details] [diff] [review]
Part 2: Generate test certificate and apps, v5

Hi Ryan. Since Brian cannot review this, and you reviewed the original version, can you please review this?

Comment 74

[Antonio Manuel Amaya Calvo (:amac)]

Thanks for the review, Camilo.

Comment 75

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: Part 1: Tests, webapps change and additional files, V5, rebased

r=fabrice

This is the reviewed version, rebased to apply cleanly.

Try run at https://tbpl.mozilla.org/?tree=Try&rev=09232f6f1a4c

Comment 76

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: Part 2: Generate test certificate and apps, v5

r=cviecco
Rebased, and fixed commit message

Comment 77

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: 8396218: Part 1: Tests, webapps change and additional files, V5, rebased

Again, Makefile.in has been phased out between the original review and now.

New try run at: https://tbpl.mozilla.org/?tree=Try&rev=94f8795edbe7

Comment 78

[Antonio Manuel Amaya Calvo (:amac)]

Comment on attachment 8396482 [details] [diff] [review]
8396218: Part 1: Tests, webapps change and additional files, V5, rebased

r=fabrice for this patch

Comment 79

[Marco Castelluccio [:marco]]

Could you lazily load the "StoreTrustAnchor.jsm" module? (we're trying to avoid loading modules at startup if they aren't needed at startup, see bug 986493)

Comment 80

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: Part 1: Tests, webapps change and additional files, V6, rebased

r=fabrice

Also lazy loads the StoreTrustAnchor.jsm, as requested on comment 79. Try run at
https://tbpl.mozilla.org/?tree=Try&rev=f0becab95de1

Comment 81

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: Part 2: Generate test certificate and apps, v6

r=cviecco

Try run at https://tbpl.mozilla.org/?tree=Try&rev=f0becab95de1

Comment 82

[Antonio Manuel Amaya Calvo (:amac)]

Try run looks finally good, requesting check-in.

Please note that part 1 must land before part 2 (since part 2 overwrites some binary files created in part 1).

Comment 83

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ed98f4616956
https://hg.mozilla.org/integration/mozilla-inbound/rev/891233755657

Comment 84

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ed98f4616956
https://hg.mozilla.org/mozilla-central/rev/891233755657

Comment 85

[Marco Castelluccio [:marco]]

The launchableValue isn't being restored after the test. I don't think it is really important, but we should fix it sooner or later as it could possibly harm other tests.