Push API: Fix stored sender ID being out of sync with registration ID

Push API: Fix stored sender ID being out of sync with registration ID

Until now we stored the sender ID into the Service Worker database
whenever an attempt was made at registering. This meant that a
successful subscribe followed by an unsuccessful subscribe with a
different sender ID would leave the SW database in an inconsistent
state: we'd be storing the old successful registration ID, but the new
unsuccessful sender ID!

This is mainly a problem when unsubscribing from GCM, which requires the
correct sender ID on Android.

This patch fixes that, so that henceforth whenever a registration ID is
stored, the sender ID is guaranteed to be the one used when subscribing.

Depends on https://codereview.chromium.org/1945753002

BUG=608831
Committed: https://crrev.com/8112686d4a56ef5200014a9ffa294529d071debc
Cr-Commit-Position: refs/heads/master@{#393041}

[johnme, 2016-05-05 10:56:54]

johnme@chromium.org changed reviewers:
+ harkness@chromium.org

[harkness, 2016-05-09 06:59:47]

https://codereview.chromium.org/1944863002/diff/20001/content/browser/push_me...
File content/browser/push_messaging/push_messaging_message_filter.cc (right):

https://codereview.chromium.org/1944863002/diff/20001/content/browser/push_me...
content/browser/push_messaging/push_messaging_message_filter.cc:266:
data.render_frame_id = render_frame_id;
At this point, the only difference between OnSubscribeFromDocument and
OnSubscribeFromWorker is setting the render frame. I'd suggest renaming
OnSubscribeFromWorker to just OnSubscribe, then remove everything in this
function except the render_frame_id, then call OnSubscribe.

That will just make it clearer that the flows are now almost completely unified,
and will call out the remaining difference.

https://codereview.chromium.org/1944863002/diff/20001/content/browser/push_me...
content/browser/push_messaging/push_messaging_message_filter.cc:293:
!service_worker_registration->active_version()) {
Previously, subscribing from a Document had the active version check, and
subscribing from a Worker didn't. So if you called subscribe from a worker and
expected to pull the sender_info from the previous version's subscription it
would have worked before, but doesn't appear to work now.

Is that an expected behavior change? Or was the previous SubscribeFromWorker
flow too loose?

[johnme, 2016-05-09 13:52:33]

Addressed review comments - thanks!

https://codereview.chromium.org/1944863002/diff/20001/content/browser/push_me...
File content/browser/push_messaging/push_messaging_message_filter.cc (right):

https://codereview.chromium.org/1944863002/diff/20001/content/browser/push_me...
content/browser/push_messaging/push_messaging_message_filter.cc:266:
data.render_frame_id = render_frame_id;
On 2016/05/09 06:59:46, harkness wrote:
> At this point, the only difference between OnSubscribeFromDocument and
> OnSubscribeFromWorker is setting the render frame. I'd suggest renaming
> OnSubscribeFromWorker to just OnSubscribe, then remove everything in this
> function except the render_frame_id, then call OnSubscribe.
> 
> That will just make it clearer that the flows are now almost completely
unified,
> and will call out the remaining difference.

Better yet, I've merged the IPC messages (and also gotten rid of
CheckForExistingRegistration) :-D

(I didn't merge the reply IPCs since the routing is useful there, and they don't
add much complexity).

https://codereview.chromium.org/1944863002/diff/20001/content/browser/push_me...
content/browser/push_messaging/push_messaging_message_filter.cc:293:
!service_worker_registration->active_version()) {
On 2016/05/09 06:59:46, harkness wrote:
> Previously, subscribing from a Document had the active version check, and
> subscribing from a Worker didn't. So if you called subscribe from a worker and
> expected to pull the sender_info from the previous version's subscription it
> would have worked before, but doesn't appear to work now.
> 
> Is that an expected behavior change? Or was the previous SubscribeFromWorker
> flow too loose?

I discussed this with mvanouwerkerk (the author of the active version check),
who confirmed that we should probably be doing it here too, it just happens to
be almost always true anyway since this IPC is coming from JS running in the SW.

[harkness, 2016-05-09 15:37:24]

LGTM


On 2016/05/09 13:52:33, johnme wrote:
> Addressed review comments - thanks!
> 
>
https://codereview.chromium.org/1944863002/diff/20001/content/browser/push_me...
> File content/browser/push_messaging/push_messaging_message_filter.cc (right):
> 
>
https://codereview.chromium.org/1944863002/diff/20001/content/browser/push_me...
> content/browser/push_messaging/push_messaging_message_filter.cc:266:
> data.render_frame_id = render_frame_id;
> On 2016/05/09 06:59:46, harkness wrote:
> > At this point, the only difference between OnSubscribeFromDocument and
> > OnSubscribeFromWorker is setting the render frame. I'd suggest renaming
> > OnSubscribeFromWorker to just OnSubscribe, then remove everything in this
> > function except the render_frame_id, then call OnSubscribe.
> > 
> > That will just make it clearer that the flows are now almost completely
> unified,
> > and will call out the remaining difference.
> 
> Better yet, I've merged the IPC messages (and also gotten rid of
> CheckForExistingRegistration) :-D
> 
> (I didn't merge the reply IPCs since the routing is useful there, and they
don't
> add much complexity).
> 
>
https://codereview.chromium.org/1944863002/diff/20001/content/browser/push_me...
> content/browser/push_messaging/push_messaging_message_filter.cc:293:
> !service_worker_registration->active_version()) {
> On 2016/05/09 06:59:46, harkness wrote:
> > Previously, subscribing from a Document had the active version check, and
> > subscribing from a Worker didn't. So if you called subscribe from a worker
and
> > expected to pull the sender_info from the previous version's subscription it
> > would have worked before, but doesn't appear to work now.
> > 
> > Is that an expected behavior change? Or was the previous SubscribeFromWorker
> > flow too loose?
> 
> I discussed this with mvanouwerkerk (the author of the active version check),
> who confirmed that we should probably be doing it here too, it just happens to
> be almost always true anyway since this IPC is coming from JS running in the
SW.

[Peter Beverloo, 2016-05-09 15:48:40]

peter@chromium.org changed reviewers:
+ peter@chromium.org

[Peter Beverloo, 2016-05-09 15:48:41]

rs lgtm, please run the try-bots

https://codereview.chromium.org/1944863002/diff/40001/content/browser/push_me...
File content/browser/push_messaging/push_messaging_message_filter.cc (right):

https://codereview.chromium.org/1944863002/diff/40001/content/browser/push_me...
content/browser/push_messaging/push_messaging_message_filter.cc:39: // sender ID
must be the one used to subscribe. Unfortunately, this isn't always
Question: should we start calling this "authorized entity" with VAPID in mind?

https://codereview.chromium.org/1944863002/diff/40001/content/browser/push_me...
content/browser/push_messaging/push_messaging_message_filter.cc:290: if
(service_worker_status == SERVICE_WORKER_OK) {
We should abort here if |data.options.sender_info| is different from the stored
sender Id. Doing so might allow us to merge this method with
DidGetSenderIdFromStorage, too.

Since we don't do that today (but probably want to for M52), mind adding a TODO?
:)

[johnme, 2016-05-10 12:29:08]

https://codereview.chromium.org/1944863002/diff/40001/content/browser/push_me...
File content/browser/push_messaging/push_messaging_message_filter.cc (right):

https://codereview.chromium.org/1944863002/diff/40001/content/browser/push_me...
content/browser/push_messaging/push_messaging_message_filter.cc:39: // sender ID
must be the one used to subscribe. Unfortunately, this isn't always
On 2016/05/09 15:48:41, Peter Beverloo wrote:
> Question: should we start calling this "authorized entity" with VAPID in mind?

Perhaps, we should discuss that. But we use sender ID so consistently (except
when dealing with Instance IDs where we match the terminology of the IID API),
that it's probably clearer to continue to do so until we do a wider search and
replace.

https://codereview.chromium.org/1944863002/diff/40001/content/browser/push_me...
content/browser/push_messaging/push_messaging_message_filter.cc:290: if
(service_worker_status == SERVICE_WORKER_OK) {
On 2016/05/09 15:48:41, Peter Beverloo wrote:
> We should abort here if |data.options.sender_info| is different from the
stored
> sender Id. Doing so might allow us to merge this method with
> DidGetSenderIdFromStorage, too.
> 
> Since we don't do that today (but probably want to for M52), mind adding a
TODO?
> :)

Done.

[johnme, 2016-05-10 12:29:13]

The CQ bit was checked by johnme@chromium.org

[johnme, 2016-05-10 12:29:13]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org,
harkness@chromium.org
Link to the patchset: https://codereview.chromium.org/1944863002/#ps60001
(title: "Add TODO")

[commit-bot: I haz the power, 2016-05-10 12:29:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1944863002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1944863002/60001

[commit-bot: I haz the power, 2016-05-10 12:36:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-10 12:36:13]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[johnme, 2016-05-10 12:52:09]

johnme@chromium.org changed reviewers:
+ mkwst@chromium.org

[johnme, 2016-05-10 12:52:09]

mkwst@chromium.org: Please review changes in
content/common/push_messaging_messages.h

[johnme, 2016-05-11 15:03:28]

johnme@chromium.org changed reviewers:
+ avi@chromium.org
- mkwst@chromium.org

[johnme, 2016-05-11 15:03:29]

avi@chromium.org: Please review changes in
content/common/push_messaging_messages.h

(since mkwst@ seems to be travelling)

[Peter Beverloo, 2016-05-11 15:04:54]

On 2016/05/11 15:03:29, johnme wrote:
> mailto:avi@chromium.org: Please review changes in
> content/common/push_messaging_messages.h
> 
> (since mkwst@ seems to be travelling)

You need a security reviewer for that file, John.

[johnme, 2016-05-11 15:08:11]

johnme@chromium.org changed reviewers:
+ palmer@chromium.org
- avi@chromium.org

[johnme, 2016-05-11 15:08:12]

palmer@chromium.org: Please review changes in
content/common/push_messaging_messages.h

peter wrote:
> You need a security reviewer for that file, John.

Oops.

[palmer, 2016-05-11 18:36:52]

lgtm

[johnme, 2016-05-11 19:09:40]

The CQ bit was checked by johnme@chromium.org

[commit-bot: I haz the power, 2016-05-11 19:10:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1944863002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1944863002/60001

[commit-bot: I haz the power, 2016-05-11 20:16:05]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-05-11 20:18:27]

Description was changed from

==========
Push API: Fix stored sender ID being out of sync with registration ID

Until now we stored the sender ID into the Service Worker database
whenever an attempt was made at registering. This meant that a
successful subscribe followed by an unsuccessful subscribe with a
different sender ID would leave the SW database in an inconsistent
state: we'd be storing the old successful registration ID, but the new
unsuccessful sender ID!

This is mainly a problem when unsubscribing from GCM, which requires the
correct sender ID on Android.

This patch fixes that, so that henceforth whenever a registration ID is
stored, the sender ID is guaranteed to be the one used when subscribing.

Depends on https://codereview.chromium.org/1945753002

BUG=608831
==========

to

==========
Push API: Fix stored sender ID being out of sync with registration ID

Until now we stored the sender ID into the Service Worker database
whenever an attempt was made at registering. This meant that a
successful subscribe followed by an unsuccessful subscribe with a
different sender ID would leave the SW database in an inconsistent
state: we'd be storing the old successful registration ID, but the new
unsuccessful sender ID!

This is mainly a problem when unsubscribing from GCM, which requires the
correct sender ID on Android.

This patch fixes that, so that henceforth whenever a registration ID is
stored, the sender ID is guaranteed to be the one used when subscribing.

Depends on https://codereview.chromium.org/1945753002

BUG=608831

Committed: https://crrev.com/8112686d4a56ef5200014a9ffa294529d071debc
Cr-Commit-Position: refs/heads/master@{#393041}
==========

[commit-bot: I haz the power, 2016-05-11 20:18:29]

Patchset 4 (id:??) landed as
https://crrev.com/8112686d4a56ef5200014a9ffa294529d071debc
Cr-Commit-Position: refs/heads/master@{#393041}