Closed
Bug 880043
Opened 12 years ago
Closed 11 years ago
Mochitest: Install / Launch / Uninstall signed packaged app
Categories
(Core Graveyard :: DOM: Apps, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla31
People
(Reporter: onecyrenus, Assigned: amac)
References
Details
Attachments
(2 files, 19 obsolete files)
53.17 KB,
patch
|
amac
:
review+
|
Details | Diff | Splinter Review |
63.78 KB,
patch
|
amac
:
review+
|
Details | Diff | Splinter Review |
1) Install a signed packaged app
2) Launch packaged app
3) Uninstall signed packaged app
Assignee | ||
Comment 1•12 years ago
|
||
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.
Assignee: nobody → amac
Assignee | ||
Comment 2•12 years ago
|
||
Added it to the 1.2 systems backlog, as technical debt.
Blocks: 896364
Comment 3•12 years ago
|
||
(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.
Reporter | ||
Comment 4•12 years ago
|
||
So this is a gecko based test.
Comment 5•12 years ago
|
||
(In reply to dclarke@mozilla.com [: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).
Assignee | ||
Comment 6•12 years ago
|
||
Untaking this since I'm on vacation for a couple of weeks and so I won't work on this right now.
Assignee: amac → nobody
Reporter | ||
Comment 7•11 years ago
|
||
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.
Attachment #813853 -
Flags: review?(fabrice)
Reporter | ||
Comment 8•11 years ago
|
||
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.
Reporter | ||
Comment 9•11 years ago
|
||
Attached is a final patch.
Attachment #813853 -
Attachment is obsolete: true
Attachment #813853 -
Flags: review?(fabrice)
Attachment #815133 -
Flags: review?(fabrice)
Comment 10•11 years ago
|
||
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?
Attachment #815133 -
Flags: review?(myk)
Comment 11•11 years ago
|
||
(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•11 years ago
|
||
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.)
Attachment #815133 -
Flags: review?(myk)
Attachment #815133 -
Flags: review?(fabrice)
Attachment #815133 -
Flags: review-
Reporter | ||
Comment 13•11 years ago
|
||
Thanks myk, working on a new patch
Reporter | ||
Comment 14•11 years ago
|
||
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
Attachment #815133 -
Attachment is obsolete: true
Reporter | ||
Comment 15•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=ec434504bc8d, running through the try server again. Had some small nits to fix.
Reporter | ||
Comment 16•11 years ago
|
||
Attachment #8343248 -
Flags: review?(myk)
Reporter | ||
Updated•11 years ago
|
Attachment #8335022 -
Attachment is obsolete: true
Reporter | ||
Updated•11 years ago
|
Assignee: nobody → dclarke
Reporter | ||
Comment 17•11 years ago
|
||
updated the patch, do you have an eta on review ?
Comment 18•11 years ago
|
||
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.
Attachment #8343248 -
Flags: review?(myk) → review-
Assignee | ||
Comment 19•11 years ago
|
||
Are you still working on this, David? If your workload doesn't let you close this, I volunteer myself to help ;)
Blocks: 960601
Updated•11 years ago
|
Flags: needinfo?(dclarke)
Reporter | ||
Comment 20•11 years ago
|
||
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.
Flags: needinfo?(dclarke)
Assignee | ||
Comment 21•11 years ago
|
||
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.
Assignee: dclarke → amac
Attachment #8343248 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8362876 -
Flags: review?(myk)
Assignee | ||
Comment 22•11 years ago
|
||
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
Attachment #8362914 -
Flags: review?(myk)
Attachment #8362914 -
Flags: review?(brian)
Assignee | ||
Comment 23•11 years ago
|
||
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).
Attachment #8362876 -
Flags: review?(myk)
Assignee | ||
Comment 24•11 years ago
|
||
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.
Attachment #8362914 -
Flags: review?(myk)
Attachment #8362914 -
Flags: review?(brian)
Assignee | ||
Comment 25•11 years ago
|
||
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.
Attachment #8362876 -
Attachment is obsolete: true
Attachment #8363068 -
Flags: review?(myk)
Attachment #8363068 -
Flags: feedback?(dclarke)
Assignee | ||
Comment 26•11 years ago
|
||
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.
Attachment #8362914 -
Attachment is obsolete: true
Attachment #8363072 -
Flags: review?(myk)
Attachment #8363072 -
Flags: review?(brian)
Assignee | ||
Updated•11 years ago
|
Assignee | ||
Comment 27•11 years ago
|
||
Try run at:
https://tbpl.mozilla.org/?tree=Try&rev=e9cc593d8df8
(including the proposed patch for bug 960601)
Reporter | ||
Comment 28•11 years ago
|
||
Comment on attachment 8363068 [details] [diff] [review]
Part 1. Installation tests, with auxiliary files
seems reasonable + passes.
Attachment #8363068 -
Flags: feedback?(dclarke) → feedback+
Comment 29•11 years ago
|
||
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.
Attachment #8363068 -
Flags: review?(myk) → review+
Comment 30•11 years ago
|
||
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.
Attachment #8363072 -
Flags: review?(myk) → review-
Comment 31•11 years ago
|
||
(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.
Assignee | ||
Comment 32•11 years ago
|
||
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.
Attachment #8363072 -
Attachment is obsolete: true
Attachment #8363072 -
Flags: review?(brian)
Attachment #8366533 -
Flags: review?(myk)
Attachment #8366533 -
Flags: review?(brian)
Assignee | ||
Comment 33•11 years ago
|
||
Same as V2, but with the directories I forgot to add added now
Attachment #8366533 -
Attachment is obsolete: true
Attachment #8366533 -
Flags: review?(myk)
Attachment #8366533 -
Flags: review?(brian)
Attachment #8366536 -
Flags: review?(myk)
Attachment #8366536 -
Flags: review?(brian)
Assignee | ||
Comment 34•11 years ago
|
||
(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•11 years ago
|
||
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).
Attachment #8366536 -
Flags: review?(myk) → review+
Assignee | ||
Comment 36•11 years ago
|
||
(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•11 years ago
|
||
(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•11 years ago
|
||
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.
Attachment #8366536 -
Flags: review?(brian) → review-
Assignee | ||
Comment 39•11 years ago
|
||
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.
Attachment #8366536 -
Attachment is obsolete: true
Attachment #8368559 -
Flags: review?(brian)
Assignee | ||
Comment 40•11 years ago
|
||
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.
Attachment #8363068 -
Attachment is obsolete: true
Attachment #8368602 -
Flags: review+
Comment 41•11 years ago
|
||
(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.
Assignee | ||
Comment 42•11 years ago
|
||
(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•11 years ago
|
||
Sure, but is this kind of finish() and error or a success overall for the test? Should we care about that?
Assignee | ||
Comment 44•11 years ago
|
||
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•11 years ago
|
||
(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•11 years ago
|
||
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?
Flags: needinfo?(amac)
Comment 47•11 years ago
|
||
Comment on attachment 8368559 [details] [diff] [review]
Part 2. Binary test files generator, V4
Please re-request review after answering the NEEDINFO.
Attachment #8368559 -
Flags: review?(brian)
Assignee | ||
Comment 48•11 years ago
|
||
(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?
Flags: needinfo?(amac)
Assignee | ||
Updated•11 years ago
|
Flags: needinfo?(brian)
Comment 49•11 years ago
|
||
(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.
Flags: needinfo?(brian)
Assignee | ||
Comment 50•11 years ago
|
||
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
Flags: needinfo?(fabrice)
Flags: needinfo?(brian)
Comment 52•11 years ago
|
||
(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.
Flags: needinfo?(brian)
Assignee | ||
Comment 53•11 years ago
|
||
(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 :).
Flags: needinfo?(fabrice)
Comment 54•11 years ago
|
||
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.
Flags: needinfo?(fabrice)
Assignee | ||
Comment 55•11 years ago
|
||
(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 :)
Assignee | ||
Comment 56•11 years ago
|
||
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 58•11 years ago
|
||
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).
Attachment #8368559 -
Attachment is obsolete: true
Attachment #8383843 -
Flags: review?(brian)
Assignee | ||
Comment 59•11 years ago
|
||
Sorry about that, the last comment was actually me. I should remember to set the auto log off here :)
Assignee | ||
Comment 60•11 years ago
|
||
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.
Attachment #8368602 -
Attachment is obsolete: true
Attachment #8383871 -
Flags: review?(fabrice)
Assignee | ||
Comment 61•11 years ago
|
||
Assignee | ||
Comment 62•11 years ago
|
||
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.
Assignee | ||
Comment 63•11 years ago
|
||
New try run at https://tbpl.mozilla.org/?tree=Try&rev=855d399932ad
Comment 64•11 years ago
|
||
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
Attachment #8383871 -
Flags: review?(fabrice) → review-
Assignee | ||
Comment 65•11 years ago
|
||
Including the last review comments.
Can you take a look to the tests as per Fabrice request on the last review, Fernando?
Attachment #8383871 -
Attachment is obsolete: true
Attachment #8385463 -
Flags: review?(ferjmoreno)
Attachment #8385463 -
Flags: review?(fabrice)
Comment 66•11 years ago
|
||
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?
Attachment #8385463 -
Flags: review?(ferjmoreno) → review+
Comment 67•11 years ago
|
||
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
Assignee | ||
Comment 68•11 years ago
|
||
(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.
Assignee | ||
Comment 69•11 years ago
|
||
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).
Attachment #8386699 -
Flags: review?(fabrice)
Updated•11 years ago
|
Attachment #8386699 -
Flags: review?(fabrice) → review+
Updated•11 years ago
|
Attachment #8385463 -
Flags: review?(fabrice)
Assignee | ||
Updated•11 years ago
|
Attachment #8385463 -
Attachment is obsolete: true
Assignee | ||
Comment 70•11 years ago
|
||
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 :)
Flags: needinfo?(brian)
Updated•11 years ago
|
Attachment #8383843 -
Flags: review?(brian)
Comment 71•11 years ago
|
||
(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.
Flags: needinfo?(brian)
Assignee | ||
Comment 72•11 years ago
|
||
Fabrice, do you know who can review the certificate generation patch?
Flags: needinfo?(fabrice)
Assignee | ||
Comment 73•11 years ago
|
||
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?
Attachment #8383843 -
Flags: review?(rtilder)
Flags: needinfo?(fabrice)
Updated•11 years ago
|
Attachment #8383843 -
Flags: review?(rtilder) → review?(dougt)
Updated•11 years ago
|
Attachment #8383843 -
Flags: review?(dougt) → review+
Assignee | ||
Comment 74•11 years ago
|
||
Thanks for the review, Camilo.
Assignee | ||
Comment 75•11 years ago
|
||
r=fabrice
This is the reviewed version, rebased to apply cleanly.
Try run at https://tbpl.mozilla.org/?tree=Try&rev=09232f6f1a4c
Attachment #8386699 -
Attachment is obsolete: true
Attachment #8396218 -
Flags: review+
Assignee | ||
Comment 76•11 years ago
|
||
r=cviecco
Rebased, and fixed commit message
Attachment #8383843 -
Attachment is obsolete: true
Attachment #8396221 -
Flags: review+
Assignee | ||
Comment 77•11 years ago
|
||
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
Attachment #8396218 -
Attachment is obsolete: true
Attachment #8396482 -
Flags: review+
Assignee | ||
Comment 78•11 years ago
|
||
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•11 years ago
|
||
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)
Assignee | ||
Comment 80•11 years ago
|
||
r=fabrice
Also lazy loads the StoreTrustAnchor.jsm, as requested on comment 79. Try run at
https://tbpl.mozilla.org/?tree=Try&rev=f0becab95de1
Attachment #8396482 -
Attachment is obsolete: true
Attachment #8397286 -
Flags: review+
Assignee | ||
Comment 81•11 years ago
|
||
r=cviecco
Try run at https://tbpl.mozilla.org/?tree=Try&rev=f0becab95de1
Attachment #8396221 -
Attachment is obsolete: true
Attachment #8397287 -
Flags: review+
Assignee | ||
Comment 82•11 years ago
|
||
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).
Keywords: checkin-needed
Comment 83•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed98f4616956
https://hg.mozilla.org/integration/mozilla-inbound/rev/891233755657
Flags: in-testsuite+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ed98f4616956
https://hg.mozilla.org/mozilla-central/rev/891233755657
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Comment 85•11 years ago
|
||
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.
Updated•7 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•