Parse and add manifest version to the Omaha Response object.
This CL adds the version of the update we're updating to to the Omaha
response object and removes the use of DisplayVersion. DisplayVersion is
from the Omaha Protocol v1, and v2 and has never been used or relied
upon in the update engine.
Protocol is described here:
https://code.google.com/p/omaha/wiki/ServerProtocol
Manifest version should always describe the version of the application we are
updating to as is described by the manifest.
BUG=chromium:252527
TEST=Unittests + devserver image_to_live test.
Change-Id: I9afaa8af3c21813f19d88abf437f35b024d31985
Reviewed-on: https://gerrit.chromium.org/gerrit/59498
Reviewed-by: Chris Sosa <sosa@chromium.org>
Tested-by: Chris Sosa <sosa@chromium.org>
Commit-Queue: Chris Sosa <sosa@chromium.org>
diff --git a/omaha_request_action.cc b/omaha_request_action.cc
index 15d3188..6a6f87a 100644
--- a/omaha_request_action.cc
+++ b/omaha_request_action.cc
@@ -34,7 +34,7 @@
// List of custom pair tags that we interpret in the Omaha Response:
static const char* kTagDeadline = "deadline";
static const char* kTagDisablePayloadBackoff = "DisablePayloadBackoff";
-static const char* kTagDisplayVersion = "DisplayVersion";
+static const char* kTagVersion = "version";
// Deprecated: "IsDelta"
static const char* kTagIsDeltaPayload = "IsDeltaPayload";
static const char* kTagMaxFailureCountPerUrl = "MaxFailureCountPerUrl";
@@ -614,22 +614,48 @@
bool OmahaRequestAction::ParseParams(xmlDoc* doc,
OmahaResponse* output_object,
ScopedActionCompleter* completer) {
- // Get the action node where parameters are present.
+ // XPath location for response elements we care about.
+ static const char* kManifestNodeXPath("/response/app/updatecheck/manifest");\
static const char* kActionNodeXPath(
- "/response/app/updatecheck/manifest/actions/action");
+ "/response/app/updatecheck/manifest/actions/action");
+ // Get the manifest node where version is present.
scoped_ptr_malloc<xmlXPathObject, ScopedPtrXmlXPathObjectFree>
- xpath_nodeset(GetNodeSet(doc, ConstXMLStr(kActionNodeXPath)));
- if (!xpath_nodeset.get()) {
+ xpath_manifest_nodeset(GetNodeSet(doc, ConstXMLStr(kManifestNodeXPath)));
+ if (!xpath_manifest_nodeset.get()) {
completer->set_code(kErrorCodeOmahaResponseInvalid);
return false;
}
- xmlNodeSet* nodeset = xpath_nodeset->nodesetval;
- CHECK(nodeset) << "XPath missing " << kActionNodeXPath;
+ // Grab the only matching node there should be from the xpath.
+ xmlNodeSet* nodeset = xpath_manifest_nodeset->nodesetval;
+ CHECK(nodeset) << "XPath missing " << kManifestNodeXPath;
+ CHECK_GE(nodeset->nodeNr, 1);
+ xmlNode* manifest_node = nodeset->nodeTab[0];
- // We only care about the action that has event "postinall", because this is
+ // Set the version.
+ output_object->version = XmlGetProperty(manifest_node, kTagVersion);
+ if (output_object->version.empty()) {
+ LOG(ERROR) << "Omaha Response has manifest version";
+ completer->set_code(kErrorCodeOmahaResponseInvalid);
+ return false;
+ }
+
+ LOG(INFO) << "Received omaha response to update to version "
+ << output_object->version;
+
+ // Grab the action nodes.
+ scoped_ptr_malloc<xmlXPathObject, ScopedPtrXmlXPathObjectFree>
+ xpath_action_nodeset(GetNodeSet(doc, ConstXMLStr(kActionNodeXPath)));
+ if (!xpath_action_nodeset.get()) {
+ completer->set_code(kErrorCodeOmahaResponseInvalid);
+ return false;
+ }
+
+ // We only care about the action that has event "postinstall", because this is
// where Omaha puts all the generic name/value pairs in the rule.
+ nodeset = xpath_action_nodeset->nodesetval;
+ CHECK(nodeset) << "XPath missing " << kActionNodeXPath;
LOG(INFO) << "Found " << nodeset->nodeNr
<< " action(s). Processing the postinstall action.";
@@ -658,8 +684,6 @@
}
// Get the optional properties one by one.
- output_object->display_version =
- XmlGetProperty(pie_action_node, kTagDisplayVersion);
output_object->more_info_url = XmlGetProperty(pie_action_node, kTagMoreInfo);
output_object->metadata_size =
ParseInt(XmlGetProperty(pie_action_node, kTagMetadataSize));
diff --git a/omaha_request_action_unittest.cc b/omaha_request_action_unittest.cc
index de5f49f..6051f01 100644
--- a/omaha_request_action_unittest.cc
+++ b/omaha_request_action_unittest.cc
@@ -70,7 +70,7 @@
}
string GetUpdateResponse2(const string& app_id,
- const string& display_version,
+ const string& version,
const string& more_info_url,
const string& prompt,
const string& codebase,
@@ -87,12 +87,11 @@
"<app appid=\"" + app_id + "\" status=\"ok\">"
"<ping status=\"ok\"/><updatecheck status=\"ok\">"
"<urls><url codebase=\"" + codebase + "\"/></urls>"
- "<manifest version=\"" + display_version + "\">"
+ "<manifest version=\"" + version + "\">"
"<packages><package hash=\"not-used\" name=\"" + filename + "\" "
"size=\"" + size + "\"/></packages>"
"<actions><action event=\"postinstall\" "
- "DisplayVersion=\"" + display_version + "\" "
- "ChromeOSVersion=\"" + display_version + "\" "
+ "ChromeOSVersion=\"" + version + "\" "
"MoreInfo=\"" + more_info_url + "\" Prompt=\"" + prompt + "\" "
"IsDelta=\"true\" "
"IsDeltaPayload=\"true\" "
@@ -106,7 +105,7 @@
}
string GetUpdateResponse(const string& app_id,
- const string& display_version,
+ const string& version,
const string& more_info_url,
const string& prompt,
const string& codebase,
@@ -116,7 +115,7 @@
const string& size,
const string& deadline) {
return GetUpdateResponse2(app_id,
- display_version,
+ version,
more_info_url,
prompt,
codebase,
@@ -309,7 +308,7 @@
NULL));
EXPECT_TRUE(response.update_exists);
EXPECT_TRUE(response.update_exists);
- EXPECT_EQ("1.2.3.4", response.display_version);
+ EXPECT_EQ("1.2.3.4", response.version);
EXPECT_EQ("http://code/base/file.signed", response.payload_urls[0]);
EXPECT_EQ("http://more/info", response.more_info_url);
EXPECT_EQ("HASH1234=", response.hash);
@@ -733,11 +732,10 @@
"<app appid=\"xyz\" status=\"ok\">"
"<updatecheck status=\"ok\">"
"<urls><url codebase=\"http://missing/field/test/\"/></urls>"
- "<manifest version=\"1.0.0.0\">"
+ "<manifest version=\"10.2.3.4\">"
"<packages><package hash=\"not-used\" name=\"f\" "
"size=\"587\"/></packages>"
"<actions><action event=\"postinstall\" "
- "DisplayVersion=\"10.2.3.4\" "
"ChromeOSVersion=\"10.2.3.4\" "
"Prompt=\"false\" "
"IsDelta=\"true\" "
@@ -757,7 +755,7 @@
&response,
NULL));
EXPECT_TRUE(response.update_exists);
- EXPECT_EQ("10.2.3.4", response.display_version);
+ EXPECT_EQ("10.2.3.4", response.version);
EXPECT_EQ("http://missing/field/test/f", response.payload_urls[0]);
EXPECT_EQ("", response.more_info_url);
EXPECT_EQ("lkq34j5345", response.hash);
diff --git a/omaha_response.h b/omaha_response.h
index db39868..85cec5a 100644
--- a/omaha_response.h
+++ b/omaha_response.h
@@ -35,7 +35,7 @@
int poll_interval;
// These are only valid if update_exists is true:
- std::string display_version;
+ std::string version;
// The ordered list of URLs in the Omaha response. Each item is a complete
// URL (i.e. in terms of Omaha XML, each value is a urlBase + packageName)
diff --git a/omaha_response_handler_action_unittest.cc b/omaha_response_handler_action_unittest.cc
index ae8fccb..3516cd6 100644
--- a/omaha_response_handler_action_unittest.cc
+++ b/omaha_response_handler_action_unittest.cc
@@ -112,7 +112,7 @@
{
OmahaResponse in;
in.update_exists = true;
- in.display_version = "a.b.c.d";
+ in.version = "a.b.c.d";
in.payload_urls.push_back("http://foo/the_update_a.b.c.d.tgz");
in.more_info_url = "http://more/info";
in.hash = "HASH+";
@@ -138,7 +138,7 @@
{
OmahaResponse in;
in.update_exists = true;
- in.display_version = "a.b.c.d";
+ in.version = "a.b.c.d";
in.payload_urls.push_back("http://foo/the_update_a.b.c.d.tgz");
in.more_info_url = "http://more/info";
in.hash = "HASHj+";
@@ -157,7 +157,7 @@
{
OmahaResponse in;
in.update_exists = true;
- in.display_version = "a.b.c.d";
+ in.version = "a.b.c.d";
in.payload_urls.push_back(kLongName);
in.more_info_url = "http://more/info";
in.hash = "HASHj+";
@@ -190,7 +190,7 @@
TEST_F(OmahaResponseHandlerActionTest, HashChecksForHttpTest) {
OmahaResponse in;
in.update_exists = true;
- in.display_version = "a.b.c.d";
+ in.version = "a.b.c.d";
in.payload_urls.push_back("http://test.should/need/hash.checks.signed");
in.more_info_url = "http://more/info";
in.hash = "HASHj+";
@@ -205,7 +205,7 @@
TEST_F(OmahaResponseHandlerActionTest, HashChecksForHttpsTest) {
OmahaResponse in;
in.update_exists = true;
- in.display_version = "a.b.c.d";
+ in.version = "a.b.c.d";
in.payload_urls.push_back("https://test.should.not/need/hash.checks.signed");
in.more_info_url = "http://more/info";
in.hash = "HASHj+";
@@ -220,7 +220,7 @@
TEST_F(OmahaResponseHandlerActionTest, HashChecksForBothHttpAndHttpsTest) {
OmahaResponse in;
in.update_exists = true;
- in.display_version = "a.b.c.d";
+ in.version = "a.b.c.d";
in.payload_urls.push_back("http://test.should.still/need/hash.checks");
in.payload_urls.push_back("https://test.should.still/need/hash.checks");
in.more_info_url = "http://more/info";
@@ -236,7 +236,7 @@
TEST_F(OmahaResponseHandlerActionTest, ChangeToMoreStableChannelTest) {
OmahaResponse in;
in.update_exists = true;
- in.display_version = "a.b.c.d";
+ in.version = "a.b.c.d";
in.payload_urls.push_back("https://MoreStableChannelTest");
in.more_info_url = "http://more/info";
in.hash = "HASHjk";
@@ -273,7 +273,7 @@
TEST_F(OmahaResponseHandlerActionTest, ChangeToLessStableChannelTest) {
OmahaResponse in;
in.update_exists = true;
- in.display_version = "a.b.c.d";
+ in.version = "a.b.c.d";
in.payload_urls.push_back("https://LessStableChannelTest");
in.more_info_url = "http://more/info";
in.hash = "HASHjk";