Add support for GCM subtypes to desktop Instance ID implementation

components/gcm_driver/gcm_client_impl.cc

Index: components/gcm_driver/gcm_client_impl.cc
diff --git a/components/gcm_driver/gcm_client_impl.cc b/components/gcm_driver/gcm_client_impl.cc
index e5c238dad562ce68743bff49b7c935a4732c05d6..1fb948c66e83de157a8f428c7554a23c9dede962 100644
--- a/components/gcm_driver/gcm_client_impl.cc
+++ b/components/gcm_driver/gcm_client_impl.cc
@@ -86,6 +86,7 @@ const char kMessageTypeDeletedMessagesKey[] = "deleted_messages";
 const char kMessageTypeKey[] = "message_type";
 const char kMessageTypeSendErrorKey[] = "send_error";
 const char kSendErrorMessageIdKey[] = "google.message_id";
+const char kSubtypeKey[] = "subtype";
 const char kSendMessageFromValue[] = "gcm@chrome.com";
 const int64_t kDefaultUserSerialNumber = 0LL;
 const int kDestroyGCMStoreDelayMS = 5 * 60 * 1000;  // 5 minutes.
@@ -238,6 +239,21 @@ void RecordResetStoreErrorToUMA(ResetStoreError error) {
   UMA_HISTOGRAM_ENUMERATION("GCM.ResetStore", error, RESET_STORE_ERROR_COUNT);
 }
 
+// Helper method for DCHECKs that prevent requesting the same token both with
+// and without subtypes.
+bool InstanceIDsDifferOnlyInUseSubtype(
+    const RegistrationInfo* registration_info_a,
+    const RegistrationInfo* registration_info_b) {
+  const InstanceIDTokenInfo* token_a =
+      InstanceIDTokenInfo::FromRegistrationInfo(registration_info_a);
+  const InstanceIDTokenInfo* token_b =
+      InstanceIDTokenInfo::FromRegistrationInfo(registration_info_b);
+  return token_a && token_b && token_a->use_subtype != token_b->use_subtype &&
+         token_a->app_id == token_b->app_id &&
+         token_a->authorized_entity == token_b->authorized_entity &&
+         token_a->scope == token_b->scope;

[Peter Beverloo, 2016/07/28 12:34:10]

nit: Could we logically separate these checks for readability?

====
  if (!token_a || !token_b)
    return false;

  if (token_a->app_id != token_b->app_id ||
      token_a->authorized_entity != token_b->authorized_entity ||
      token_a->scope != token_b->scope) {
    return false;
  }

  return token_a->use_subtype != token_b->use_subtype;
====

[johnme, 2016/08/04 17:47:14]

Done.

+}
+
 }  // namespace
 
 GCMInternalsBuilder::GCMInternalsBuilder() {}
@@ -850,6 +866,11 @@ void GCMClientImpl::Register(
     const linked_ptr<RegistrationInfo>& registration_info) {
   DCHECK_EQ(state_, READY);
 
+  // Registrations should never pass as an app_id the special category used
+  // internally when registering with a subtype. See security note in
+  // GCMClientImpl::HandleIncomingMessage.
+  CHECK_NE(registration_info->app_id, chrome_build_info_.category_for_subtypes);
+
   // Find and use the cached registration ID.
   RegistrationInfoMap::const_iterator registrations_iter =
       registrations_.find(registration_info);
@@ -870,6 +891,11 @@ void GCMClientImpl::Register(
               cached_gcm_registration_info->sender_ids) {
         matched = false;
       }
+    } else {
+      DCHECK(!InstanceIDsDifferOnlyInUseSubtype(
+          registration_info.get(), registrations_iter->first.get()))
+          << "Registering the same Instance ID token both with and without "
+             "subtypes is not supported";
     }
 
     if (matched) {
@@ -916,10 +942,13 @@ void GCMClientImpl::Register(
                        instance_id_token_info->scope;
   }
 
+  bool use_subtype =
+      instance_id_token_info && instance_id_token_info->use_subtype;
+
   RegistrationRequest::RequestInfo request_info(
-      device_checkin_info_.android_id,
-      device_checkin_info_.secret,
-      registration_info->app_id);
+      device_checkin_info_.android_id, device_checkin_info_.secret,
+      registration_info->app_id, use_subtype,
+      chrome_build_info_.category_for_subtypes);
 
   std::unique_ptr<RegistrationRequest> registration_request(
       new RegistrationRequest(
@@ -1002,6 +1031,7 @@ void GCMClientImpl::Unregister(
       NOTREACHED();
       return;
     }
+
     request_handler.reset(new InstanceIDDeleteTokenRequestHandler(
         instance_id_iter->second.first,
         instance_id_token_info->authorized_entity,
@@ -1049,6 +1079,8 @@ void GCMClientImpl::Unregister(
       delegate_->OnUnregisterFinished(registration_info, INVALID_PARAMETER);
       return;
     }
+    DCHECK(!InstanceIDsDifferOnlyInUseSubtype(registration_info.get(),
+                                              iter->first.get()));
     registrations_.erase(iter);
 
     gcm_store_->RemoveRegistration(
@@ -1057,10 +1089,13 @@ void GCMClientImpl::Unregister(
                   weak_ptr_factory_.GetWeakPtr()));
   }
 
+  bool use_subtype =
+      instance_id_token_info && instance_id_token_info->use_subtype;
+
   UnregistrationRequest::RequestInfo request_info(
-      device_checkin_info_.android_id,
-      device_checkin_info_.secret,
-      registration_info->app_id);
+      device_checkin_info_.android_id, device_checkin_info_.secret,
+      registration_info->app_id, use_subtype,
+      chrome_build_info_.category_for_subtypes);
 
   std::unique_ptr<UnregistrationRequest> unregistration_request(
       new UnregistrationRequest(
@@ -1260,49 +1295,76 @@ void GCMClientImpl::HandleIncomingMessage(const gcm::MCSMessage& message) {
           message.GetProtobuf());
   DCHECK_EQ(data_message_stanza.device_user_id(), kDefaultUserSerialNumber);
 
-  // Copying all the data from the stanza to a MessageData object. When present,
-  // keys like kMessageTypeKey or kSendErrorMessageIdKey will be filtered out
-  // later.
+  // Copy all the data from the stanza to a MessageData object. When present,
+  // keys like kSubtypeKey, kMessageTypeKey or kSendErrorMessageIdKey will be
+  // filtered out later.
   MessageData message_data;
   for (int i = 0; i < data_message_stanza.app_data_size(); ++i) {
     std::string key = data_message_stanza.app_data(i).key();
     message_data[key] = data_message_stanza.app_data(i).value();
   }
 
+  std::string subtype;
+  auto subtype_iter = message_data.find(kSubtypeKey);
+  if (subtype_iter != message_data.end()) {
+    subtype = subtype_iter->second;
+    message_data.erase(subtype_iter);
+  }
+
+  // SECURITY NOTE: Subtypes that we receive from GCM *cannot* be trusted for
+  // registrations without a subtype (as the sender can pass any subtype they
+  // want). They can however be trusted for registrations that are known to have
+  // a subtype (as GCM overwrites anything passed by the sender).
+  //
+  // TODO(johnme): Get FCM to send the subtype as a trusted google.subtype key.

[Peter Beverloo, 2016/07/28 12:34:10]

This is rather passive-aggressive :-) What about:

  // TODO(johnme): Remove this limitation when GCM sends us the subtype in a
  // field that's guaranteed to be trusted. crbug.com/123456

[johnme, 2016/08/04 17:47:14]

I clicked on that crbug link! Done.

+  //
+  // So a given Chrome profile always passes a fixed string called
+  // |category_for_subtypes| (of the form "com.chrome.stable.macosx") as the
+  // category when registering with a subtype, and incoming subtypes are only
+  // trusted for that category.
+  //
+  // (On Android, all registrations made by Chrome on behalf of third-party
+  // apps/extensions/websites have always had a subtype, so we don't need such a
+  // check, indeed we cannot, since category is fixed to the true package name).

[Peter Beverloo, 2016/07/28 12:34:10]

nit: avoid use of "we" (3x) in comments... All of these can easily be rephrased,
making the comments shorter too.

[johnme, 2016/08/04 17:47:14]

Removed we's. I can't find ways to shorten these comments without losing clarity
though.

+  bool subtype_is_trusted = data_message_stanza.category() ==
+                            chrome_build_info_.category_for_subtypes;
+  bool use_subtype = subtype_is_trusted && !subtype.empty();
+  std::string app_id = use_subtype ? subtype : data_message_stanza.category();
+
   MessageType message_type = DATA_MESSAGE;
-  MessageData::iterator iter = message_data.find(kMessageTypeKey);
-  if (iter != message_data.end()) {
-    message_type = DecodeMessageType(iter->second);
-    message_data.erase(iter);
+  MessageData::iterator type_iter = message_data.find(kMessageTypeKey);
+  if (type_iter != message_data.end()) {
+    message_type = DecodeMessageType(type_iter->second);
+    message_data.erase(type_iter);
   }
 
   switch (message_type) {
     case DATA_MESSAGE:
-      HandleIncomingDataMessage(data_message_stanza, message_data);
+      HandleIncomingDataMessage(app_id, use_subtype, data_message_stanza,
+                                message_data);
       break;
     case DELETED_MESSAGES:
-      recorder_.RecordDataMessageReceived(data_message_stanza.category(),
-                                          data_message_stanza.from(),
-                                          data_message_stanza.ByteSize(),
-                                          true,
+      recorder_.RecordDataMessageReceived(app_id, data_message_stanza.from(),
+                                          data_message_stanza.ByteSize(), true,
                                           GCMStatsRecorder::DELETED_MESSAGES);
-      delegate_->OnMessagesDeleted(data_message_stanza.category());
+      delegate_->OnMessagesDeleted(app_id);
       break;
     case SEND_ERROR:
-      HandleIncomingSendError(data_message_stanza, message_data);
+      HandleIncomingSendError(app_id, data_message_stanza, message_data);
       break;
     case UNKNOWN:
     default:  // Treat default the same as UNKNOWN.
       DVLOG(1) << "Unknown message_type received. Message ignored. "
-               << "App ID: " << data_message_stanza.category() << ".";
+               << "App ID: " << app_id << ".";
       break;
   }
 }
 
 void GCMClientImpl::HandleIncomingDataMessage(
+    const std::string& app_id,
+    bool use_subtype,
     const mcs_proto::DataMessageStanza& data_message_stanza,
     MessageData& message_data) {
-  std::string app_id = data_message_stanza.category();
   std::string sender = data_message_stanza.from();
 
   // Drop the message when the app is not registered for the sender of the
@@ -1329,15 +1391,23 @@ void GCMClientImpl::HandleIncomingDataMessage(
 
   // Then, find among all InstanceID registrations.
   if (!registered) {
-    std::unique_ptr<InstanceIDTokenInfo> instance_id_token(
-        new InstanceIDTokenInfo);
+    linked_ptr<InstanceIDTokenInfo> instance_id_token(new InstanceIDTokenInfo);
     instance_id_token->app_id = app_id;
+    instance_id_token->use_subtype = use_subtype;
     instance_id_token->authorized_entity = sender;
     instance_id_token->scope = kGCMScope;
-    auto instance_id_token_iter = registrations_.find(
-        make_linked_ptr<RegistrationInfo>(instance_id_token.release()));
-    if (instance_id_token_iter != registrations_.end())
-      registered = true;
+
+    auto instance_id_token_iter = registrations_.find(instance_id_token);
+    if (instance_id_token_iter != registrations_.end()) {
+      if (InstanceIDsDifferOnlyInUseSubtype(
+              instance_id_token.get(), instance_id_token_iter->first.get())) {
+        DLOG(ERROR) << "Incoming GCM message for " << app_id
+                    << " had use_subtype = " << use_subtype
+                    << " which conflicts with stored registration";
+      } else {
+        registered = true;
+      }
+    }
   }
 
   recorder_.RecordDataMessageReceived(app_id, sender,
@@ -1357,6 +1427,7 @@ void GCMClientImpl::HandleIncomingDataMessage(
 }
 
 void GCMClientImpl::HandleIncomingSendError(
+    const std::string& app_id,
     const mcs_proto::DataMessageStanza& data_message_stanza,
     MessageData& message_data) {
   SendErrorDetails send_error_details;
@@ -1370,12 +1441,9 @@ void GCMClientImpl::HandleIncomingSendError(
     send_error_details.additional_data.erase(iter);
   }
 
-  recorder_.RecordIncomingSendError(
-      data_message_stanza.category(),
-      data_message_stanza.to(),
-      data_message_stanza.id());
-  delegate_->OnMessageSendError(data_message_stanza.category(),
-                                send_error_details);
+  recorder_.RecordIncomingSendError(app_id, data_message_stanza.to(),
+                                    data_message_stanza.id());
+  delegate_->OnMessageSendError(app_id, send_error_details);
 }
 
 bool GCMClientImpl::HasStandaloneRegisteredApp() const {