Disallow repeated PushManager.subscribes with different sender ids

content/browser/push_messaging/push_messaging_message_filter.cc

Index: content/browser/push_messaging/push_messaging_message_filter.cc
diff --git a/content/browser/push_messaging/push_messaging_message_filter.cc b/content/browser/push_messaging/push_messaging_message_filter.cc
index fd768eb7ff6aca39eade34157eab148b28d50fd8..008d9aa724fb1c759dac3feb12d5feabfc6b9b78 100644
--- a/content/browser/push_messaging/push_messaging_message_filter.cc
+++ b/content/browser/push_messaging/push_messaging_message_filter.cc
@@ -284,23 +284,27 @@ void PushMessagingMessageFilter::OnSubscribe(
 
   service_worker_context_->GetRegistrationUserData(
       data.service_worker_registration_id,
-      {kPushRegistrationIdServiceWorkerKey},
+      {kPushRegistrationIdServiceWorkerKey, kPushSenderIdServiceWorkerKey},
       base::Bind(&PushMessagingMessageFilter::DidCheckForExistingRegistration,
                  weak_factory_io_to_io_.GetWeakPtr(), data));
 }
 
 void PushMessagingMessageFilter::DidCheckForExistingRegistration(
     const RegisterData& data,
-    const std::vector<std::string>& push_registration_id,
+    const std::vector<std::string>& push_registration_id_and_sender_id,
     ServiceWorkerStatusCode service_worker_status) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   if (service_worker_status == SERVICE_WORKER_OK) {
-    // TODO(johnme): Check that stored sender ID equals data.options.sender_info
-    // and throw an exception if they don't match.
-    DCHECK_EQ(1u, push_registration_id.size());
+    DCHECK_EQ(2u, push_registration_id_and_sender_id.size());
+    auto& push_registration_id = push_registration_id_and_sender_id[0];
+    auto& sender_id = push_registration_id_and_sender_id[1];

[harkness, 2016/10/25 10:11:22]

These can be const.

[awdf, 2016/10/25 13:18:18]

Done.

+    if (data.options.sender_info != sender_id) {

[johnme, 2016/10/25 12:33:31]

data.options.sender_info might be empty here, and if it is empty we interpret
that as "keep using the stored sender ID" (see below where we lookup
kPushSenderIdServiceWorkerKey). You hit this case if you register from a
document with a manifest containing gcm_sender_id, then later register from a
Service Worker, since SWs can't have an associated manifest so we cache the
document manifest's gcm_sender_id in that case.

tl;dr: I think this check should be:
    if (!data.options.sender_info.empty() &&
        data.options.sender_info != sender_id)

Hopefully this was already causing a test to fail (either in
PushMessagingBrowserTest or layout tests); if not we might want to add one.

(it might be nice to distinguish between omitting the applicationServerKey and
explicitly setting the applicationServerKey to the empty string, and still throw
in the latter case, but that's a bit of an edge case, and awkward to implement
since std::string doesn't have a null state, so probably fine to just leave a
comment documenting our quirky behavior in that edge case)

[awdf, 2016/10/25 13:18:18]

If data.options.sender_info is empty here though, won't it necessarily be
different from the retrieved sender_id, given that one was retrieved? (We only
get to this point if both kPushSenderIdServiceWorkerKey and
kPushRegistrationIdServiceWorkerKey are successfully retrieved). I can see that
we might want to be explicit about checking it's not empty but I want to
understand why you think it should cause test failures right now. Might the
retrieved stored sender id be empty too? In which case where would gcm_sender_id
get retrieved to use instead?

No push_messaging/ layout tests or tests in PushMessagingBrowserTest are failing
right now. 

[johnme, 2016/10/25 13:28:25]

PushMessagingBrowserTest.SubscribeWorker and
PushMessagingBrowserTest.SubscribeWorkerUsingManifest cover the case I'm talking
about, where a subscribe call from a SW with neither manifest nor
applicationServerKey should succeed even though data.options.sender_info is
empty because we cached the last sender ID they successfully subscribed with.
Unfortunately those tests are disabled on Linux because of unexplained flakiness
;)

The caching is a bit of a hack, but back before applicationServerKey when we
only supported manifests, it was the only way to enable subscribing from SWs,
and we shouldn't break it unnecessarily.

[awdf, 2016/10/25 13:50:36]

Oh, sorry, got confused, I now understand you're saying the sender id is 
*allowed* to be different if it's now empty but we had a valid one before.
(Think I was ignoring the '!' when reading your tl;dr before!)

I'll fix it and see if I can add a layout test for that scenario.

[harkness, 2016/10/25 14:25:20]

There is already a browser test for that, but it's one of the ones that is
currently disabled for being flaky.   :(

+      SendSubscriptionError(data, PUSH_REGISTRATION_STATUS_SENDER_ID_MISMATCH);
+      return;
+    }
     auto callback = base::Bind(
         &PushMessagingMessageFilter::DidGetEncryptionKeys,
-        weak_factory_io_to_io_.GetWeakPtr(), data, push_registration_id[0]);
+        weak_factory_io_to_io_.GetWeakPtr(), data, push_registration_id);
 
     BrowserThread::PostTask(
         BrowserThread::UI, FROM_HERE,