Remove associated keying material when unregistering from GCM

Remove associated keying material when unregistering from GCM

This removes the keying material associated with a GCM registration from
the GCM key store when a registration has been unregistered.

BUG=486040
Committed: https://crrev.com/a4795ae834e15eee3d208cac776859dc3927fe45
Cr-Commit-Position: refs/heads/master@{#375855}

[Peter Beverloo, 2016-02-16 18:05:52]

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

[Peter Beverloo, 2016-02-16 18:05:52]

+johnme, PTAL

[johnme, 2016-02-16 18:36:13]

lgtm

https://codereview.chromium.org/1701973003/diff/20001/components/gcm_driver/g...
File components/gcm_driver/gcm_driver.h (right):

https://codereview.chromium.org/1701973003/diff/20001/components/gcm_driver/g...
components/gcm_driver/gcm_driver.h:272: // information associated with the
|app_id|.
It's not clear from this description/method name that it ultimately calls
UnregisterFinished.

https://codereview.chromium.org/1701973003/diff/20001/components/gcm_driver/g...
File components/gcm_driver/gcm_driver_desktop_unittest.cc (right):

https://codereview.chromium.org/1701973003/diff/20001/components/gcm_driver/g...
components/gcm_driver/gcm_driver_desktop_unittest.cc:350: if
(!async_operation_completed_callback_.is_null())
FWIW, this can never be null, since you never reset
async_operation_completed_callback_. But I guess no harm in being defensive.

https://codereview.chromium.org/1701973003/diff/20001/components/gcm_driver/g...
components/gcm_driver/gcm_driver_desktop_unittest.cc:712: // The GCMKeyStore
eagerly creates new keying material for registrations that
It seems odd (and a potential security foot-gun for us) that you can generate
and persist encryption info without having a valid registration?

[Peter Beverloo, 2016-02-16 21:50:50]

https://codereview.chromium.org/1701973003/diff/20001/components/gcm_driver/g...
File components/gcm_driver/gcm_driver.h (right):

https://codereview.chromium.org/1701973003/diff/20001/components/gcm_driver/g...
components/gcm_driver/gcm_driver.h:272: // information associated with the
|app_id|.
On 2016/02/16 18:36:13, johnme wrote:
> It's not clear from this description/method name that it ultimately calls
> UnregisterFinished.

Done.

https://codereview.chromium.org/1701973003/diff/20001/components/gcm_driver/g...
File components/gcm_driver/gcm_driver_desktop_unittest.cc (right):

https://codereview.chromium.org/1701973003/diff/20001/components/gcm_driver/g...
components/gcm_driver/gcm_driver_desktop_unittest.cc:350: if
(!async_operation_completed_callback_.is_null())
On 2016/02/16 18:36:13, johnme wrote:
> FWIW, this can never be null, since you never reset
> async_operation_completed_callback_. But I guess no harm in being defensive.

Just being consistent with the rest of the file - we can clean up a bunch of
stuff in here in due course.

https://codereview.chromium.org/1701973003/diff/20001/components/gcm_driver/g...
components/gcm_driver/gcm_driver_desktop_unittest.cc:712: // The GCMKeyStore
eagerly creates new keying material for registrations that
On 2016/02/16 18:36:13, johnme wrote:
> It seems odd (and a potential security foot-gun for us) that you can generate
> and persist encryption info without having a valid registration?

There are two requirements that led to this -

  (1) The encryption code has to be shared between Android and desktop. While
desktop has a good database already (GCMStore down in the engine), Play Services
will do this for us on Android, but doesn't allow us to query state. As such, we
don't know, unless we want to do duplicated bookkeeping.

  (2) In order to give subscriptions that currently don't have encryption info
associated with them an upgrade path, we eagerly create this on request.

[Peter Beverloo, 2016-02-16 21:53:16]

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

[Peter Beverloo, 2016-02-16 21:53:17]

+zea for desktop perspective

This means we jump to the blocking task runner after an unregistration has been
processed by the GCMClient (and below) to remove encryption information.

I'm fairly sure that this is OK, but would appreciate a quick look.

[Nicolas Zea, 2016-02-16 22:59:22]

lgtm

https://codereview.chromium.org/1701973003/diff/40001/components/gcm_driver/g...
File components/gcm_driver/gcm_driver.h (right):

https://codereview.chromium.org/1701973003/diff/40001/components/gcm_driver/g...
components/gcm_driver/gcm_driver.h:273: void
UnregisterRemoveEncryptionInfo(const std::string& app_id,
nit: maybe rename to RemoveEncryptionInfoAfterUnregister?

[Peter Beverloo, 2016-02-17 10:20:24]

https://codereview.chromium.org/1701973003/diff/40001/components/gcm_driver/g...
File components/gcm_driver/gcm_driver.h (right):

https://codereview.chromium.org/1701973003/diff/40001/components/gcm_driver/g...
components/gcm_driver/gcm_driver.h:273: void
UnregisterRemoveEncryptionInfo(const std::string& app_id,
On 2016/02/16 22:59:22, Nicolas Zea wrote:
> nit: maybe rename to RemoveEncryptionInfoAfterUnregister?

Done.

[Peter Beverloo, 2016-02-17 10:20:29]

The CQ bit was checked by peter@chromium.org

[Peter Beverloo, 2016-02-17 10:20:29]

The patchset sent to the CQ was uploaded after l-g-t-m from johnme@chromium.org,
zea@chromium.org
Link to the patchset: https://codereview.chromium.org/1701973003/#ps60001
(title: " ")

[commit-bot: I haz the power, 2016-02-17 10:20:34]

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

[commit-bot: I haz the power, 2016-02-17 11:35:35]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-02-17 11:37:02]

Description was changed from

==========
Remove associated keying material when unregistering from GCM

This removes the keying material associated with a GCM registration from
the GCM key store when a registration has been unregistered.

BUG=486040
==========

to

==========
Remove associated keying material when unregistering from GCM

This removes the keying material associated with a GCM registration from
the GCM key store when a registration has been unregistered.

BUG=486040

Committed: https://crrev.com/a4795ae834e15eee3d208cac776859dc3927fe45
Cr-Commit-Position: refs/heads/master@{#375855}
==========

[commit-bot: I haz the power, 2016-02-17 11:37:02]

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