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..ff8a937c86f2532b3cab953cd42b2e142686e54f 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,172 +29,316 @@ const int kDefaultMCSMainSecurePort = 5228;
 const int kDefaultMCSFallbackSecurePort = 443;
 const char kDefaultRegistrationURL[] =
     "https://android.clients.google.com/c2dm/register3";
+const char kDeleteSettingPrefix[] = "delete_";

[Nicolas Zea, 2014/05/14 23:00:33]

nit: comment what these are for?

[fgorski, 2014/05/16 20:48:47]

Done.

+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);
 }
 
-}  // namespace
-
-namespace gcm {
-
-// static
-const base::TimeDelta GServicesSettings::MinimumCheckinInterval() {
-  return base::TimeDelta::FromSeconds(kMinimumCheckinInterval);
-}
-
-// static
-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) {
-}
-
-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.
-    return false;
-  }
-
-  std::map<std::string, std::string> settings;
-  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;
-  }
-
-  // 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;
-  }
-
-  return false;
-}
-
-void GServicesSettings::UpdateFromLoadResult(
-    const GCMStore::LoadResult& load_result) {
-  if (UpdateSettings(load_result.gservices_settings))
-    digest_ = load_result.gservices_digest;
+GURL DefaultMainMCSEndpoint() {
+  return GURL(MakeMCSEndpoint(kDefaultMCSHostname, kDefaultMCSMainSecurePort));
 }
 
-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 DefaultFallbackMCSEndpoint() {
+  return GURL(
+      MakeMCSEndpoint(kDefaultMCSHostname, kDefaultMCSFallbackSecurePort));
 }
 
-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 =
+bool VerifyCheckinInterval(
+    const gcm::GServicesSettings::SettingsMap& settings) {
+  gcm::GServicesSettings::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 meant 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 VerifyMCSEndpoint(const gcm::GServicesSettings::SettingsMap& settings) {
+  gcm::GServicesSettings::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) {

[Nicolas Zea, 2014/05/14 23:00:33]

nit: mcs_secure_port > 65535 is more readable/consistent I think

[fgorski, 2014/05/16 20:48:47]

Done.

+    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 VerifyCheckinURL(const gcm::GServicesSettings::SettingsMap& settings) {
+  gcm::GServicesSettings::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()) {
-    LOG(ERROR) << "Invalid checkin URL provided: "
-               << new_checkin_url.possibly_invalid_spec();
+  GURL checkin_url(iter->second);
+  if (!checkin_url.is_valid()) {
+    LOG(ERROR) << "Invalid checkin URL provided: " << iter->second;
     return false;
   }
 
-  GURL new_registration_url;
-  iter = settings.find(kRegistrationURLKey);
+  return true;
+}
+
+bool VerifyRegistrationURL(
+    const gcm::GServicesSettings::SettingsMap& settings) {
+  gcm::GServicesSettings::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()) {
-    LOG(ERROR) << "Invalid registration URL provided: "
-               << new_registration_url.possibly_invalid_spec();
+  GURL registration_url(iter->second);
+  if (!registration_url.is_valid()) {
+    LOG(ERROR) << "Invalid registration URL provided: " << iter->second;
+    return false;
+  }
+
+  return true;
+}
+
+bool VerifySettings(const gcm::GServicesSettings::SettingsMap& settings) {
+  return VerifyCheckinInterval(settings) && VerifyMCSEndpoint(settings) &&
+         VerifyCheckinURL(settings) && VerifyRegistrationURL(settings);
+}
+
+bool VerifyDigest(const gcm::GServicesSettings::SettingsMap& settings,
+                  const std::string& expected_digest) {
+  std::string calculated_digest =
+      gcm::GServicesSettings::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;
+  }
+
+  return true;
+}
+
+}  // namespace
+
+namespace gcm {
+
+// static
+const base::TimeDelta GServicesSettings::MinimumCheckinInterval() {
+  return base::TimeDelta::FromSeconds(kMinimumCheckinInterval);
+}
+
+// static
+const GURL GServicesSettings::DefaultCheckinURL() {
+  return GURL(kDefaultCheckinURL);
+}
+
+// static
+std::string GServicesSettings::CalculateDigest(const SettingsMap& settings) {
+  unsigned char hash[base::kSHA1Length];
+  std::string data;
+  for (SettingsMap::const_iterator iter = settings.begin();
+       iter != settings.end();
+       ++iter) {
+    data += iter->first;
+    data += '\0';
+    data += iter->second;
+    data += '\0';
+  }
+  base::SHA1HashBytes(
+      reinterpret_cast<const unsigned char*>(&data[0]), data.size(), hash);
+  return kDigestVersionPrefix + base::HexEncode(hash, base::kSHA1Length);
+}
+
+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_settings_diff()) {
+    LOG(ERROR) << "Field settings_diff not set in response.";

[Nicolas Zea, 2014/05/14 23:00:33]

will settings_diff and digest always be set? (even on first time checkin
response) If it's valid for them to not be set, I'd rather just return without
LOG(ERROR).

In general, I'm a bit concerned about the amount of logging here. I count 35 log
statements, which would put it as the 2nd highest per file in the entire chrome
repo (and it's likely to grow as we consume more settings).

Could we cut down on them? I realize that GCMStoreImpl also has too many
LOG(ERRORS), which I should address, but I'd like to avoid adding so many here.
A generic "CheckinResponse returned invalid data error" would probably be good
enough, so that a developer could investigate further.

[fgorski, 2014/05/16 20:48:47]

Done. Replaced with DVLOG

+    return false;
+  }
+
+  if (!checkin_response.has_digest()) {
+    LOG(ERROR) << "Field digest not set in response.";
+    return false;
+  }
+
+  if (digest_ == checkin_response.digest())
+    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 = settings_map();
+
+  for (int i = 0; i < checkin_response.setting_size(); ++i) {
+    std::string name = checkin_response.setting(i).name();
+    if (name.empty())
+      return false;
+
+    if (settings_diff && name.find(kDeleteSettingPrefix) == 0) {
+      std::string setting_to_delete =
+          name.substr(arraysize(kDeleteSettingPrefix) - 1);
+      new_settings.erase(setting_to_delete);
+    } else {
+      new_settings[name] = checkin_response.setting(i).value();
+    }
+  }
+
+  if (!VerifySettings(new_settings))
+    return false;
+
+  std::string digest = CalculateDigest(new_settings);
+  if (digest != checkin_response.digest()) {
+    LOG(ERROR) << "G-services settings digest mismatch. "
+               << "Checkin response contains: " << checkin_response.digest()
+               << ". Calculated digest is: " << 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;
+  settings_.swap(new_settings);
+  digest_ = digest;
   return true;
 }
 
+void GServicesSettings::UpdateFromLoadResult(
+    const GCMStore::LoadResult& load_result) {
+  // No need to try to update settings when load_result is empty.
+  if (load_result.gservices_settings.empty())
+    return;
+  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;
+  }
+}
+
+base::TimeDelta GServicesSettings::GetCheckinInterval() 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::GetCheckinURL() const {
+  SettingsMap::const_iterator iter = settings_.find(kCheckinURLKey);
+  if (iter == settings_.end() || iter->second.empty())
+    return GURL(kDefaultCheckinURL);
+  return GURL(iter->second);
+}
+
+GURL GServicesSettings::GetMCSMainEndpoint() 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::GetMCSFallbackEndpoint() 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;
+}
+
+GURL GServicesSettings::GetRegistrationURL() const {
+  SettingsMap::const_iterator iter = settings_.find(kRegistrationURLKey);
+  if (iter == settings_.end() || iter->second.empty())
+    return GURL(kDefaultRegistrationURL);
+  return GURL(iter->second);
+}
+
 }  // namespace gcm