Make Web Push use InstanceID tokens instead of GCM registrations

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 812f00bf8961dcd47428250ea620cf5ae0290fa1..9c176d14014780ee27a71cd11042fb8d74282670 100644
--- a/chrome/browser/push_messaging/push_messaging_app_identifier.cc
+++ b/chrome/browser/push_messaging/push_messaging_app_identifier.cc
@@ -19,11 +19,13 @@
 #include "components/prefs/scoped_user_pref_update.h"
 
 const char kPushMessagingAppIdentifierPrefix[] = "wp:";
+const char kInstanceIDGuidSuffix[] = "-V2";
 
 namespace {
 
 // sizeof is strlen + 1 since it's null-terminated.
 const size_t kPrefixLength = sizeof(kPushMessagingAppIdentifierPrefix) - 1;
+const size_t kGuidSuffixLength = sizeof(kInstanceIDGuidSuffix) - 1;
 
 const char kSeparator = '#';    // Ok as only the origin of the url is used.
 const size_t kGuidLength = 36;  // "%08X-%04X-%04X-%04X-%012llX"
@@ -63,12 +65,21 @@ void PushMessagingAppIdentifier::RegisterProfilePrefs(
 }
 
 // static
+bool PushMessagingAppIdentifier::UseInstanceID(const std::string& app_id) {
+  return app_id.length() > kPrefixLength + kGuidLength &&
+         kInstanceIDGuidSuffix == app_id.substr(app_id.size() - 3);
+}
+
+// static
 PushMessagingAppIdentifier PushMessagingAppIdentifier::Generate(
     const GURL& origin,
     int64_t service_worker_registration_id) {
   // Use uppercase GUID for consistency with GUIDs Push has already sent to GCM.
   // Also allows detecting case mangling; see code commented "crbug.com/461867".
   std::string guid = base::ToUpperASCII(base::GenerateGUID());
+  // All new push subscriptions are Instance ID tokens.
+  guid.replace(guid.size() - kGuidSuffixLength, kGuidSuffixLength,
+               kInstanceIDGuidSuffix);
   CHECK(!guid.empty());
   std::string app_id =
       kPushMessagingAppIdentifierPrefix + origin.spec() + kSeparator + guid;
@@ -204,6 +215,7 @@ void PushMessagingAppIdentifier::DCheckValid() const {
   // "wp:"
   DCHECK_EQ(kPushMessagingAppIdentifierPrefix,
             app_id_.substr(0, kPrefixLength));
+
   // Optional (origin.spec() + '#')
   if (app_id_.size() != kPrefixLength + kGuidLength) {
     const size_t suffix_length = 1 /* kSeparator */ + kGuidLength;
@@ -214,6 +226,16 @@ void PushMessagingAppIdentifier::DCheckValid() const {
     DCHECK_EQ(std::string(1, kSeparator),
               app_id_.substr(app_id_.size() - suffix_length, 1));
   }
-  // GUID
-  DCHECK(base::IsValidGUID(app_id_.substr(app_id_.size() - kGuidLength)));
+
+  // GUID, optionally with last few chars replaced with kInstanceIDGuidSuffix.
+  DCHECK(base::IsValidGUID(
+      app_id_.substr(app_id_.size() - kGuidLength,
+                     kGuidLength - kGuidSuffixLength) +
+      // It's ok for it to end with kInstanceIDGuidSuffix; if so, pass
+      // arbitrary hex of the same length so that IsValidGUID passes.
+      (kInstanceIDGuidSuffix ==
+               app_id_.substr(app_id_.size() - kGuidSuffixLength)
+           ? std::string(kGuidSuffixLength, 'C')  // arbitrary hex
+           : app_id_.substr(app_id_.size() - kGuidSuffixLength)  // real suffix
+       )));

[Peter Beverloo, 2016/06/02 15:53:17]

This is a ten line DCHECK containing significant logic, the complication of this
is way too high.

Let's instead separate this out in a series of variables living on the stack.
How about the following?

===============
  // App identifiers created for InstanceID-based subscriptions have a suffix
  // allowing them to be identified as such, whilst breaking the GUID syntax.
  std::string guid = app_id_.substr(app_id_.size() - kGuidLength);
  bool is_instance_id =
      base::EndsWith(guid, kInstanceIDGuidSuffix, base::CompareCase::SENSITIVE);

  if (is_instance_id) {
    DCHECK(!base::IsValidGUID(guid));

    // Replace the suffix with valid GUID characters in order to be able to
    // validate the rest of the string.
    guid = guid.replace(guid.size() - kInstanceIDGuidSuffix,
                        kInstanceIDGuidSuffix, kInstanceIDGuidSuffix, 'C');
  }

  DCHECK(base::IsValidGUID(guid));
===============

That's incredibly much easier to read.

If you're worried about compiling that code in non-debug builds, let's stick the
entire body of this method in an "#if DCHECK_IS_ON()" block.

[johnme, 2016/06/07 14:16:42]

Done.

 }