G-services settings v3 implementation

google_apis/gcm/engine/gservices_settings.cc

Index: google_apis/gcm/engine/gservices_settings.cc
diff --git a/google_apis/gcm/engine/gservices_settings.cc b/google_apis/gcm/engine/gservices_settings.cc
index 55cfa391392373f3326e0a8d34842a4095f1e3c7..ac070fbd3433f2f9013c50708acf4acf1e1e9bb9 100644
--- a/google_apis/gcm/engine/gservices_settings.cc
+++ b/google_apis/gcm/engine/gservices_settings.cc
@@ -5,6 +5,7 @@
 #include "google_apis/gcm/engine/gservices_settings.h"
 
 #include "base/bind.h"
+#include "base/sha1.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/stringprintf.h"
 
@@ -28,12 +29,23 @@ const int kDefaultMCSMainSecurePort = 5228;
 const int kDefaultMCSFallbackSecurePort = 443;
 const char kDefaultRegistrationURL[] =
     "https://android.clients.google.com/c2dm/register3";
+const char kDeleteSettingPrefix[] = "delete_";
+const char kDigestVersionPrefix[] = "1-";
 const char kMCSEnpointTemplate[] = "https://%s:%d";
 
 std::string MakeMCSEndpoint(const std::string& mcs_hostname, int port) {
   return base::StringPrintf(kMCSEnpointTemplate, mcs_hostname.c_str(), port);
 }
 
+GURL DefaultMainMCSEndpoint() {
+  return GURL(MakeMCSEndpoint(kDefaultMCSHostname, kDefaultMCSMainSecurePort));
+}
+
+GURL DefaultFallbackMCSEndpoint() {
+  return GURL(
+      MakeMCSEndpoint(kDefaultMCSHostname, kDefaultMCSFallbackSecurePort));
+}
+
 }  // namespace
 
 namespace gcm {
@@ -48,151 +60,281 @@ const GURL GServicesSettings::DefaultCheckinURL() {
   return GURL(kDefaultCheckinURL);
 }
 
-GServicesSettings::GServicesSettings()
-    : checkin_interval_(base::TimeDelta::FromSeconds(kDefaultCheckinInterval)),
-      checkin_url_(kDefaultCheckinURL),
-      mcs_main_endpoint_(MakeMCSEndpoint(kDefaultMCSHostname,
-                                         kDefaultMCSMainSecurePort)),
-      mcs_fallback_endpoint_(MakeMCSEndpoint(kDefaultMCSHostname,
-                                             kDefaultMCSFallbackSecurePort)),
-      registration_url_(kDefaultRegistrationURL),
-      weak_ptr_factory_(this) {
+// static
+std::string GServicesSettings::CalculateDigest(const SettingsMap& settings) {
+  char hash[base::kSHA1Length];

[jianli, 2014/05/13 21:41:52]

Why not defining this as unsigned char array?

[fgorski, 2014/05/13 23:53:28]

Done.

+  std::vector<char> data;

[jianli, 2014/05/13 21:41:52]

It seems to be easier to use std::string, like:
  std::string data;
  for (...) {
    data += iter->first;
    data += '\0';
    data += iter->second;
    data += '\0';
  }

[fgorski, 2014/05/13 23:53:28]

Done.

+  for (SettingsMap::const_iterator iter = settings.begin();
+       iter != settings.end();
+       ++iter) {
+    data.insert(data.end(), iter->first.begin(), iter->first.end());
+    data.push_back((char)0);

[jianli, 2014/05/13 21:41:52]

nit: '\0' to replace c type conversion

[fgorski, 2014/05/13 23:53:28]

Done.

+    data.insert(data.end(), iter->second.begin(), iter->second.end());
+    data.push_back((char)0);
+  }
+  base::SHA1HashBytes(reinterpret_cast<const unsigned char*>(&data[0]),
+                      data.size(),
+                      reinterpret_cast<unsigned char*>(hash));
+  return kDigestVersionPrefix + base::HexEncode(hash, base::kSHA1Length);

[jianli, 2014/05/13 21:41:52]

Just to confirm that this is not base64 encoded, right?

[fgorski, 2014/05/13 23:53:28]

HEX is expected.

+}
+
+GServicesSettings::GServicesSettings() : weak_ptr_factory_(this) {
+  settings_[kCheckinIntervalKey] = base::Int64ToString(kDefaultCheckinInterval);
+  settings_[kCheckinURLKey] = kDefaultCheckinURL;
+  settings_[kMCSHostnameKey] = kDefaultMCSHostname;
+  settings_[kMCSSecurePortKey] = base::IntToString(kDefaultMCSMainSecurePort);
+  settings_[kRegistrationURLKey] = kDefaultRegistrationURL;
+  digest_ = CalculateDigest(settings_);
 }
 
 GServicesSettings::~GServicesSettings() {}
 
 bool GServicesSettings::UpdateFromCheckinResponse(
-  const checkin_proto::AndroidCheckinResponse& checkin_response) {
-  if (!checkin_response.has_digest() ||
-      checkin_response.digest() == digest_) {
-    // There are no changes as digest is the same or no settings provided.
+    const checkin_proto::AndroidCheckinResponse& checkin_response) {
+  if (!checkin_response.has_settings_diff()) {
+    LOG(ERROR) << "Field settings_diff not set in response.";
     return false;
   }
 
-  std::map<std::string, std::string> settings;
+  if (!checkin_response.has_digest()) {
+    LOG(ERROR) << "Field digest not set in response.";
+    return false;
+  }
+
+  bool settings_diff = checkin_response.settings_diff();
+  SettingsMap new_settings;
+  // Only reuse the existing settings, if we are given a settings difference.
+  if (settings_diff)
+    new_settings = GetSettingsMap();
+
   for (int i = 0; i < checkin_response.setting_size(); ++i) {
     std::string name = checkin_response.setting(i).name();
     std::string value = checkin_response.setting(i).value();
-    settings[name] = value;
+    if (settings_diff && name.find(kDeleteSettingPrefix) == 0) {

[jianli, 2014/05/13 21:41:52]

Consider adding the check for empty name.

[fgorski, 2014/05/13 23:53:28]

Done.

+      std::string setting_to_delete =
+          name.substr(arraysize(kDeleteSettingPrefix));
+      new_settings.erase(setting_to_delete);
+    } else {
+      new_settings[name] = value;
+    }
   }
 
-  // Only update the settings in store and digest, if the settings actually
-  // passed the verificaiton in update settings.
-  if (UpdateSettings(settings)) {
-    digest_ = checkin_response.digest();
-    return true;
+  if (!VerifySettings(new_settings))
+    return false;
+
+  std::string digest = CalculateDigest(new_settings);
+  if (digest != checkin_response.digest()) {
+    LOG(ERROR) << "Calculated digest does not match the original digest.";

[jianli, 2014/05/13 21:41:52]

nit: combine all 3 logs into one, like:
  LOG(ERROR) << "Digest mismatches: " << checkin_response.digest() << " != " <<
digest;

[fgorski, 2014/05/13 23:53:28]

Done.

+    LOG(ERROR) << "Expected G-Services settings digest: "
+               << checkin_response.digest();
+    LOG(ERROR) << "Calculated G-services settings digest: " << digest;
+    return false;
   }
 
-  return false;
+  if (digest == digest_)
+    return false;
+
+  settings_ = new_settings;

[jianli, 2014/05/13 21:41:52]

nit: use swap for efficiency

[fgorski, 2014/05/13 23:53:28]

Done.

+  digest_ = digest;
+  return true;
 }
 
 void GServicesSettings::UpdateFromLoadResult(
     const GCMStore::LoadResult& load_result) {
-  if (UpdateSettings(load_result.gservices_settings))
+  if (VerifySettings(load_result.gservices_settings) &&
+      VerifyDigest(load_result.gservices_settings,
+                   load_result.gservices_digest)) {
+    settings_ = load_result.gservices_settings;
     digest_ = load_result.gservices_digest;
+  }
+}
+
+GServicesSettings::SettingsMap GServicesSettings::GetSettingsMap() const {

[jianli, 2014/05/13 21:41:52]

This can be defined as getter now.

[fgorski, 2014/05/13 23:53:28]

Done.

+  return settings_;
+}
+
+base::TimeDelta GServicesSettings::checkin_interval() const {
+  int64 checkin_interval = kMinimumCheckinInterval;
+  SettingsMap::const_iterator iter = settings_.find(kCheckinIntervalKey);
+  if (iter == settings_.end() ||
+      !base::StringToInt64(iter->second, &checkin_interval)) {
+    checkin_interval = kDefaultCheckinInterval;
+  }
+
+  if (checkin_interval < kMinimumCheckinInterval)
+    checkin_interval = kMinimumCheckinInterval;
+
+  return base::TimeDelta::FromSeconds(checkin_interval);
+}
+
+GURL GServicesSettings::checkin_url() const {
+  SettingsMap::const_iterator iter = settings_.find(kCheckinURLKey);
+  if (iter == settings_.end() || iter->second.empty())
+    return GURL(kDefaultCheckinURL);
+  return GURL(iter->second);
+}
+
+GURL GServicesSettings::mcs_main_endpoint() const {
+  SettingsMap::const_iterator iter = settings_.find(kMCSHostnameKey);
+  if (iter == settings_.end() || iter->second.empty())
+    return DefaultMainMCSEndpoint();
+
+  std::string mcs_hostname = iter->second;
+
+  iter = settings_.find(kMCSSecurePortKey);
+  int mcs_secure_port = kDefaultMCSMainSecurePort;
+  if (iter == settings_.end() || iter->second.empty() ||
+      !base::StringToInt(iter->second, &mcs_secure_port))
+    return DefaultMainMCSEndpoint();
+
+  GURL mcs_endpoint(MakeMCSEndpoint(mcs_hostname, mcs_secure_port));
+
+  if (!mcs_endpoint.is_valid())
+    return DefaultMainMCSEndpoint();
+
+  return mcs_endpoint;
+}
+
+GURL GServicesSettings::mcs_fallback_endpoint() const {
+  SettingsMap::const_iterator iter = settings_.find(kMCSHostnameKey);
+  if (iter == settings_.end() || iter->second.empty())
+    return DefaultFallbackMCSEndpoint();
+
+  GURL mcs_endpoint(
+      MakeMCSEndpoint(iter->second, kDefaultMCSFallbackSecurePort));
+
+  if (!mcs_endpoint.is_valid())
+    return DefaultFallbackMCSEndpoint();
+
+  return mcs_endpoint;
 }
 
-std::map<std::string, std::string> GServicesSettings::GetSettingsMap() const {
-  std::map<std::string, std::string> settings;
-  settings[kCheckinIntervalKey] =
-      base::Int64ToString(checkin_interval_.InSeconds());
-  settings[kCheckinURLKey] = checkin_url_.spec();
-  settings[kMCSHostnameKey] = mcs_main_endpoint_.host();
-  settings[kMCSSecurePortKey] = mcs_main_endpoint_.port();
-  settings[kRegistrationURLKey] = registration_url_.spec();
-  return settings;
+GURL GServicesSettings::registration_url() const {
+  SettingsMap::const_iterator iter = settings_.find(kRegistrationURLKey);
+  if (iter == settings_.end() || iter->second.empty())
+    return GURL(kDefaultRegistrationURL);
+  return GURL(iter->second);
 }
 
-bool GServicesSettings::UpdateSettings(
-    const std::map<std::string, std::string>& settings) {
-  int64 new_checkin_interval = kMinimumCheckinInterval;
-  std::map<std::string, std::string>::const_iterator iter =
-      settings.find(kCheckinIntervalKey);
+bool GServicesSettings::VerifyCheckinInterval(
+    const SettingsMap& settings) const {
+  SettingsMap::const_iterator iter = settings.find(kCheckinIntervalKey);
   if (iter == settings.end()) {
     LOG(ERROR) << "Setting not found: " << kCheckinIntervalKey;
     return false;
   }
-  if (!base::StringToInt64(iter->second, &new_checkin_interval)) {
+  int64 checkin_interval = kMinimumCheckinInterval;
+  if (!base::StringToInt64(iter->second, &checkin_interval)) {
     LOG(ERROR) << "Failed to parse checkin interval: " << iter->second;
     return false;
   }
-  if (new_checkin_interval < kMinimumCheckinInterval) {
-    LOG(ERROR) << "Checkin interval: " << new_checkin_interval
-               << " is less than allowed minimum: " << kMinimumCheckinInterval;
-    new_checkin_interval = kMinimumCheckinInterval;
-  }
-  if (new_checkin_interval == std::numeric_limits<int64>::max()) {
-    LOG(ERROR) << "Checkin interval is too big: " << new_checkin_interval;
+  if (checkin_interval == std::numeric_limits<int64>::max()) {
+    LOG(ERROR) << "Checkin interval is too big: " << checkin_interval;
     return false;
   }
+  if (checkin_interval < kMinimumCheckinInterval) {
+    // This is emited as an error, but is not mean to fail verification.
+    LOG(ERROR) << "Checkin interval: " << checkin_interval
+               << " is less than allowed minimum: " << kMinimumCheckinInterval;
+  }
 
-  std::string new_mcs_hostname;
-  iter = settings.find(kMCSHostnameKey);
+  return true;
+}
+
+bool GServicesSettings::VerifyMCSEndpoint(const SettingsMap& settings) const {
+  SettingsMap::const_iterator iter = settings.find(kMCSHostnameKey);
+  std::string mcs_hostname;
   if (iter == settings.end()) {
     LOG(ERROR) << "Setting not found: " << kMCSHostnameKey;
     return false;
   }
-  new_mcs_hostname = iter->second;
-  if (new_mcs_hostname.empty()) {
+  mcs_hostname = iter->second;
+  if (iter->second.empty()) {
     LOG(ERROR) << "Empty MCS hostname provided.";
     return false;
   }
 
-  int new_mcs_secure_port = -1;
+  int mcs_secure_port = -1;
   iter = settings.find(kMCSSecurePortKey);
   if (iter == settings.end()) {
     LOG(ERROR) << "Setting not found: " << kMCSSecurePortKey;
     return false;
   }
-  if (!base::StringToInt(iter->second, &new_mcs_secure_port)) {
+  if (!base::StringToInt(iter->second, &mcs_secure_port)) {
     LOG(ERROR) << "Failed to parse MCS secure port: " << iter->second;
     return false;
   }
-  if (new_mcs_secure_port < 0 || 65535 < new_mcs_secure_port) {
-    LOG(ERROR) << "Incorrect port value: " << new_mcs_secure_port;
+  if (mcs_secure_port < 0 || 65535 < mcs_secure_port) {
+    LOG(ERROR) << "Incorrect port value: " << mcs_secure_port;
     return false;
   }
 
-  GURL new_mcs_main_endpoint =
-      GURL(MakeMCSEndpoint(new_mcs_hostname, new_mcs_secure_port));
-  GURL new_mcs_fallback_endpoint =
-      GURL(MakeMCSEndpoint(new_mcs_hostname, kDefaultMCSFallbackSecurePort));
-  if (!new_mcs_main_endpoint.is_valid() ||
-      !new_mcs_fallback_endpoint.is_valid())
+  GURL mcs_main_endpoint(MakeMCSEndpoint(mcs_hostname, mcs_secure_port));
+  if (!mcs_main_endpoint.is_valid()) {
+    LOG(ERROR) << "Invalid main MCS endpoint: "
+               << mcs_main_endpoint.possibly_invalid_spec();
+    return false;
+  }
+  GURL mcs_fallback_endpoint(
+      MakeMCSEndpoint(mcs_hostname, kDefaultMCSFallbackSecurePort));
+  if (!mcs_fallback_endpoint.is_valid()) {
+    LOG(ERROR) << "Invalid fallback MCS endpoint: "
+               << mcs_fallback_endpoint.possibly_invalid_spec();
     return false;
+  }
+
+  return true;
+}
 
-  GURL new_checkin_url;
-  iter = settings.find(kCheckinURLKey);
+bool GServicesSettings::VerifyCheckinURL(const SettingsMap& settings) const {

[jianli, 2014/05/13 21:41:52]

It seems that this could be defined as static method. Or even better, it can put
into the anonymouse namespace.

[fgorski, 2014/05/13 23:53:28]

Done.

+  SettingsMap::const_iterator iter = settings.find(kCheckinURLKey);
   if (iter == settings.end()) {
     LOG(ERROR) << "Setting not found: " << kCheckinURLKey;
     return false;
   }
-  new_checkin_url = GURL(iter->second);
-  if (!new_checkin_url.is_valid()) {
+  GURL checkin_url(iter->second);
+  if (!checkin_url.is_valid()) {
     LOG(ERROR) << "Invalid checkin URL provided: "
-               << new_checkin_url.possibly_invalid_spec();
+               << checkin_url.possibly_invalid_spec();
     return false;
   }
 
-  GURL new_registration_url;
-  iter = settings.find(kRegistrationURLKey);
+  return true;
+}
+
+bool GServicesSettings::VerifyRegistrationURL(
+    const SettingsMap& settings) const {
+  SettingsMap::const_iterator iter = settings.find(kRegistrationURLKey);
   if (iter == settings.end()) {
     LOG(ERROR) << "Setting not found: " << kRegistrationURLKey;
     return false;
   }
-  new_registration_url = GURL(iter->second);
-  if (!new_registration_url.is_valid()) {
+  GURL registration_url(iter->second);
+  if (!registration_url.is_valid()) {
     LOG(ERROR) << "Invalid registration URL provided: "
-               << new_registration_url.possibly_invalid_spec();
+               << registration_url.possibly_invalid_spec();
+    return false;
+  }
+
+  return true;
+}
+
+bool GServicesSettings::VerifySettings(const SettingsMap& settings) const {
+  return VerifyCheckinInterval(settings) && VerifyMCSEndpoint(settings) &&
+         VerifyCheckinURL(settings) && VerifyRegistrationURL(settings);
+}
+
+bool GServicesSettings::VerifyDigest(const SettingsMap& settings,
+                                     const std::string& expected_digest) const {
+  std::string calculated_digest = CalculateDigest(settings);
+  if (calculated_digest != expected_digest) {
+    LOG(ERROR) << "Calculated digest does not match the original digest.";
+    LOG(ERROR) << "Expected G-Services settings digest: " << expected_digest;
+    LOG(ERROR) << "Calculated G-services settings digest: "
+               << calculated_digest;
     return false;
   }
 
-  // We only update the settings once all of them are correct.
-  checkin_interval_ = base::TimeDelta::FromSeconds(new_checkin_interval);
-  mcs_main_endpoint_ = new_mcs_main_endpoint;
-  mcs_fallback_endpoint_ = new_mcs_fallback_endpoint;
-  checkin_url_ = new_checkin_url;
-  registration_url_ = new_registration_url;
   return true;
 }