Push API: Include origin in generated app_ids

chrome/browser/push_messaging/push_messaging_app_identifier.cc

Index: chrome/browser/push_messaging/push_messaging_app_identifier.cc
diff --git a/chrome/browser/push_messaging/push_messaging_app_identifier.cc b/chrome/browser/push_messaging/push_messaging_app_identifier.cc
index feb069a006cdea7024aaf665d1c6bc2b3091624a..1c5b00f2b3afc032066e429dc71e47f93577d533 100644
--- a/chrome/browser/push_messaging/push_messaging_app_identifier.cc
+++ b/chrome/browser/push_messaging/push_messaging_app_identifier.cc
@@ -4,6 +4,8 @@
 
 #include "chrome/browser/push_messaging/push_messaging_app_identifier.h"
 
+#include <string.h>
+
 #include "base/guid.h"
 #include "base/logging.h"
 #include "base/prefs/pref_service.h"
@@ -17,7 +19,40 @@
 #include "components/pref_registry/pref_registry_syncable.h"
 
 namespace {
+
 const char kSeparator = '#';  // Ok as only the origin of the url is used.
+const int kGuidLength = 36;  // "%08X-%04X-%04X-%04X-%012llX"
+
+bool IsLegacyAppId(const std::string& app_id) {
+  return app_id.size() == strlen(kPushMessagingAppIdentifierPrefix)

[Peter Beverloo, 2015/05/13 13:04:14]

I'd slightly prefer to use IsValidGUID for this.

bool IsLegacyAppId(const std::string& app_id) {
  return base::IsValidGUID(
      app_id.substr(strlen(kPushMessagingAppIdentifierPrefix)));
}

[johnme, 2015/05/13 16:18:09]

How about:

  if (app_id.size() == strlen(kPushMessagingAppIdentifierPrefix)
                     + kGuidLength) {
    DCHECK(base::IsValidGUID(
        app_id.substr(strlen(kPushMessagingAppIdentifierPrefix))));
    return true;
  }
  return false;

? Happy to go with yours if you'd rather.

+                        + kGuidLength;
+}
+
+GURL GetOriginFromModernAppId(const std::string& app_id) {

[Peter Beverloo, 2015/05/13 13:04:14]

s/Modern//. The omission from "Legacy" in the name implies that it's current.

[johnme, 2015/05/13 16:18:09]

Done.

+  const size_t prefix_len = strlen(kPushMessagingAppIdentifierPrefix);
+  const size_t suffix_len = 1 /* kSeparator */ + kGuidLength;
+  CHECK(app_id.size() > prefix_len + suffix_len);
+  return GURL(app_id.substr(prefix_len,
+                            app_id.size() - prefix_len - suffix_len));
+}
+
+bool GetOriginAndSWRFromLegacyMapValue(

[Peter Beverloo, 2015/05/13 13:04:14]

nit: While the variable in PushMessagingAppIdentifier::Get() is called
map_value, that makes no sense in scope of this function. Could we call it
PrefKey or similar?

[johnme, 2015/05/13 16:18:09]

Done.

+    const std::string& map_value, GURL* origin,
+    int64_t* service_worker_registration_id) {
+  // map_value is origin + kSeparator + service_worker_registration_id
+  std::vector<std::string> parts;
+  base::SplitString(map_value, kSeparator, &parts);
+  if (parts.size() != 2)
+    return false;
+
+  if (!base::StringToInt64(parts[1], service_worker_registration_id))
+    return false;
+
+  *origin = GURL(parts[0]);
+
+  return true;
+}
+
 }  // namespace
 
 const char kPushMessagingAppIdentifierPrefix[] = "wp:";
@@ -34,7 +69,8 @@ PushMessagingAppIdentifier PushMessagingAppIdentifier::Generate(
 {
   std::string guid = base::GenerateGUID();
   CHECK(!guid.empty());
-  std::string app_id = kPushMessagingAppIdentifierPrefix + guid;
+  std::string app_id = kPushMessagingAppIdentifierPrefix + origin.spec()

[Peter Beverloo, 2015/05/13 13:04:14]

How certain are we that |origin|==|origin.GetOrigin()| here?

[Peter Beverloo, 2015/05/13 13:04:14]

You could consider using base::StringPrintF here, which is how several other
features handle generation of such strings.

  std::string app_id = base::StringPrintf(
      "%s%s%c%s",
      kPushMessagingAppIdentifierPrefix,
      origin.spec(),
      kSeparator,
      guid);

[johnme, 2015/05/13 16:18:09]

Seems more legible to use + (even if minorly less efficient).

[johnme, 2015/05/13 16:18:09]

DCheckValid enforces this if DCHECKs are on. Happy to always use
origin.GetOrigin() though if you'd rather I take no changes when DCHECKs are
off? WDYT?

+                     + kSeparator + guid;
 
   PushMessagingAppIdentifier app_identifier(app_id, origin,
                                             service_worker_registration_id);
@@ -45,34 +81,31 @@ PushMessagingAppIdentifier PushMessagingAppIdentifier::Generate(
 // static
 PushMessagingAppIdentifier PushMessagingAppIdentifier::Get(
     Profile* profile, const std::string& app_id) {
-  // Workaround crbug.com/461867 in GCM where it converts subtypes to lowercase.
-  // TODO(johnme): Remove this when obsolete
-  const size_t prefix_len = strlen(kPushMessagingAppIdentifierPrefix);
-  if (app_id.size() < prefix_len)
-    return PushMessagingAppIdentifier();
-  std::string uppercase_app_id =
-      app_id.substr(0, prefix_len) +
-      StringToUpperASCII(app_id.substr(prefix_len, std::string::npos));
-
   const base::DictionaryValue* map =
       profile->GetPrefs()->GetDictionary(prefs::kPushMessagingAppIdentifierMap);
 
-  std::string origin_and_sw_id;
-  if (!map->GetStringWithoutPathExpansion(uppercase_app_id, &origin_and_sw_id))
+  std::string map_value;
+  if (!map->GetStringWithoutPathExpansion(app_id, &map_value))
     return PushMessagingAppIdentifier();
 
-  std::vector<std::string> parts;
-  base::SplitString(origin_and_sw_id, kSeparator, &parts);
-  if (parts.size() != 2)
-    return PushMessagingAppIdentifier();
-
-  GURL origin = GURL(parts[0]);
-
+  GURL origin;
   int64_t service_worker_registration_id;
-  if (!base::StringToInt64(parts[1], &service_worker_registration_id))
-    return PushMessagingAppIdentifier();
+  if (IsLegacyAppId(app_id)) {
+    if (!GetOriginAndSWRFromLegacyMapValue(map_value, &origin,
+                                           &service_worker_registration_id)) {
+      NOTREACHED();
+      return PushMessagingAppIdentifier();
+    }
+  } else {
+    origin = GetOriginFromModernAppId(app_id);
+
+    if (!base::StringToInt64(map_value, &service_worker_registration_id)) {
+      NOTREACHED();
+      return PushMessagingAppIdentifier();
+    }
+  }
 
-  PushMessagingAppIdentifier app_identifier(uppercase_app_id, origin,
+  PushMessagingAppIdentifier app_identifier(app_id, origin,
                                             service_worker_registration_id);
   app_identifier.DCheckValid();
   return app_identifier;
@@ -83,15 +116,22 @@ PushMessagingAppIdentifier PushMessagingAppIdentifier::Get(
     Profile* profile, const GURL& origin,
     int64_t service_worker_registration_id)
 {
-  base::StringValue origin_and_sw_id = base::StringValue(origin.spec() +
-      kSeparator + base::Int64ToString(service_worker_registration_id));
+  const base::StringValue modern_swr_id_only = base::StringValue(
+      base::Int64ToString(service_worker_registration_id));
+  const base::StringValue legacy_origin_and_swr_id = base::StringValue(
+      origin.spec() + kSeparator + modern_swr_id_only.GetString());
 
   const base::DictionaryValue* map =
       profile->GetPrefs()->GetDictionary(prefs::kPushMessagingAppIdentifierMap);
   for (auto it = base::DictionaryValue::Iterator(*map); !it.IsAtEnd();
        it.Advance()) {
-    if (it.value().Equals(&origin_and_sw_id))
+    if (IsLegacyAppId(it.key())) {
+      if (it.value().Equals(&legacy_origin_and_swr_id))
+        return Get(profile, it.key());
+    } else if (GetOriginFromModernAppId(it.key()) == origin &&
+               it.value().Equals(&modern_swr_id_only)) {
       return Get(profile, it.key());
+    }
   }
   return PushMessagingAppIdentifier();
 }
@@ -140,11 +180,10 @@ void PushMessagingAppIdentifier::PersistToPrefs(Profile* profile) const {
   PushMessagingAppIdentifier old = Get(profile, origin_,
                                        service_worker_registration_id_);
   if (!old.is_null())
-    map->RemoveWithoutPathExpansion(old.app_id_, nullptr);
+    map->RemoveWithoutPathExpansion(old.app_id_, nullptr /* out_value */);
 
-  std::string origin_and_sw_id = origin_.spec() + kSeparator +
-      base::Int64ToString(service_worker_registration_id_);
-  map->SetStringWithoutPathExpansion(app_id_, origin_and_sw_id);
+  map->SetStringWithoutPathExpansion(
+      app_id_, base::Int64ToString(service_worker_registration_id_));
 }
 
 void PushMessagingAppIdentifier::DeleteFromPrefs(Profile* profile) const {
@@ -153,7 +192,7 @@ void PushMessagingAppIdentifier::DeleteFromPrefs(Profile* profile) const {
   DictionaryPrefUpdate update(profile->GetPrefs(),
                               prefs::kPushMessagingAppIdentifierMap);
   base::DictionaryValue* map = update.Get();
-  map->RemoveWithoutPathExpansion(app_id_, nullptr);
+  map->RemoveWithoutPathExpansion(app_id_, nullptr /* out_value */);
 }
 
 void PushMessagingAppIdentifier::DCheckValid() const {
@@ -162,5 +201,11 @@ void PushMessagingAppIdentifier::DCheckValid() const {
   DCHECK(origin_.is_valid());
   DCHECK_EQ(origin_.GetOrigin(), origin_);
   DCHECK_EQ(app_id_.substr(0, prefix_len), kPushMessagingAppIdentifierPrefix);
-  DCHECK(base::IsValidGUID(app_id_.substr(prefix_len, std::string::npos)));
+  if (!IsLegacyAppId(app_id_)) {
+    DCHECK_EQ(origin_, GetOriginFromModernAppId(app_id_));
+    DCHECK_EQ(std::string(1, kSeparator),
+              app_id_.substr(app_id_.size() - kGuidLength - 1, 1));
+  }
+  DCHECK(base::IsValidGUID(app_id_.substr(app_id_.size() - kGuidLength,
+                                          std::string::npos)));

[Peter Beverloo, 2015/05/13 13:04:14]

std::string::npos is the default argument value, you can omit it.

[johnme, 2015/05/13 16:18:09]

Done.

 }