Introduce the PushSubscription.curve25519dh attribute (2/3)

Introduce the PushSubscription.curve25519dh attribute (2/3)

This CL provides plumbing for sending the curve25519dh attribute to Blink
in the appropriate places. Hooking it up with the GCM Driver will be
done in a later patch.

This CL is part of a three-sided patch:
  [1] https://codereview.chromium.org/1222903007
  [2] This CL.
  [3] https://codereview.chromium.org/1237673002

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

[Peter Beverloo, 2015-07-08 20:25:52]

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

[Peter Beverloo, 2015-07-08 20:25:53]

+johnme

The amount of plumbing that's required here makes me feel real bad. How can we
simplify the hell out of this?

[johnme, 2015-07-09 16:02:53]

lgtm with nits

https://codereview.chromium.org/1210283005/diff/1/chrome/browser/push_messagi...
File chrome/browser/push_messaging/push_messaging_service_impl.cc (right):

https://codereview.chromium.org/1210283005/diff/1/chrome/browser/push_messagi...
chrome/browser/push_messaging/push_messaging_service_impl.cc:419: void
PushMessagingServiceImpl::GetPublicEncryptionKey(
Nit: Please move this to a new section of the .cc file called
"GetPublicEncryptionKey method", just above "// Unsubscribe methods" (rather
than in the middle of the subscribe methods - GetPermissionStatus is only here
by historical accident).

https://codereview.chromium.org/1210283005/diff/1/chrome/browser/push_messagi...
chrome/browser/push_messaging/push_messaging_service_impl.cc:488:
SubscribeEnd(callback, subscription_id, curve25519dh, status);
Nit: it feels slightly inconsistent that you still use SubscribeEnd for the
error cases here, now that there is SubscribeEndWithError. But I don't feel
strongly.

https://codereview.chromium.org/1210283005/diff/1/content/browser/push_messag...
File content/browser/push_messaging/push_messaging_message_filter.cc (right):

https://codereview.chromium.org/1210283005/diff/1/content/browser/push_messag...
content/browser/push_messaging/push_messaging_message_filter.cc:69: void
ForwardPublicEncryptionKeysToIOThreadProxy(
It's a bit unusual that Core::GetPublicEncryptionKeyOnUI gets called on UI
thread, but runs its callback on IO thread.

I've looked at the two callbacks (DidGetEncryptionKeys &
DidGetRegistrationKeys), and it seems both of them could actually be implemented
on the UI thread almost as easily (you can replace Send with the Core::Send
proxy; SendRegister{Success,Error} can be PostTask'd to; finally
RecordGetRegistrationStatus is safe enough to call from UI thread - just change
the DCHECK_CURRENTLY_ON).

Then you could remove ForwardPublicEncryptionKeysToIOThreadProxy. Alternatively,
if you prefer to keep the current threading, then document it clearly (rename
Core::GetPublicEncryptionKeyOnUI's callback parameter to io_thread_callback, and
add a comment in the method declaration).

https://codereview.chromium.org/1210283005/diff/1/content/browser/push_messag...
content/browser/push_messaging/push_messaging_message_filter.cc:112: void
GetPublicEncryptionKeyOnUI(
Please move this to the "Public helper methods on UI thread" section, since it's
used both for the Register flow and the GetRegistration flow.

https://codereview.chromium.org/1210283005/diff/1/content/browser/push_messag...
content/browser/push_messaging/push_messaging_message_filter.cc:364: void
PushMessagingMessageFilter::Core::GetPublicEncryptionKeyOnUI(
Similarly, please move this to the "Helper methods on both IO and UI threads,
merged from PushMessagingMessageFilter and Core." section at the bottom of this
file.

https://codereview.chromium.org/1210283005/diff/1/content/browser/push_messag...
content/browser/push_messaging/push_messaging_message_filter.cc:861: void
PushMessagingMessageFilter::DidGetRegistrationKeys(
Please add a DCHECK_CURRENTLY_ON.

https://codereview.chromium.org/1210283005/diff/1/content/common/push_messagi...
File content/common/push_messaging_messages.h (right):

https://codereview.chromium.org/1210283005/diff/1/content/common/push_messagi...
content/common/push_messaging_messages.h:98: GURL /* origin */,
It seems problematic to let the untrusted renderer provide the origin directly.

Other handlers typically use
service_worker_context_->GetLiveRegistration(service_worker_registration_id)->pattern().GetOrigin()
to get the origin from the browser-process, though that's not much better (as it
seems easy to enumerate all possible service worker registration ids), though
perhaps it at least limits it to origins which have a SW (which in this case
isn't a particularly useful limitation).

Is there some way we can check if the origin makes sense for the render process?
I guess this is blocked on https://crbug.com/109792 :-(

https://codereview.chromium.org/1210283005/diff/1/content/shell/browser/layou...
File content/shell/browser/layout_test/layout_test_push_messaging_service.cc
(right):

https://codereview.chromium.org/1210283005/diff/1/content/shell/browser/layou...
content/shell/browser/layout_test/layout_test_push_messaging_service.cc:91:
kTestCurve25519dh, kTestCurve25519dh + arraysize(kTestCurve25519dh));
Nit: #include "base/macros.h"

https://codereview.chromium.org/1210283005/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1210283005/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:65365: +  <int value="11" label="Unable
to retrieve the public key"/>
Nit: Might want a more generic string to cover other possible errors, like
failing to persist the private key to GCMDriver's store?

[Peter Beverloo, 2015-07-09 18:54:17]

https://codereview.chromium.org/1210283005/diff/1/chrome/browser/push_messagi...
File chrome/browser/push_messaging/push_messaging_service_impl.cc (right):

https://codereview.chromium.org/1210283005/diff/1/chrome/browser/push_messagi...
chrome/browser/push_messaging/push_messaging_service_impl.cc:419: void
PushMessagingServiceImpl::GetPublicEncryptionKey(
On 2015/07/09 16:02:53, johnme wrote:
> Nit: Please move this to a new section of the .cc file called
> "GetPublicEncryptionKey method", just above "// Unsubscribe methods" (rather
> than in the middle of the subscribe methods - GetPermissionStatus is only here
> by historical accident).

Done.

https://codereview.chromium.org/1210283005/diff/1/chrome/browser/push_messagi...
chrome/browser/push_messaging/push_messaging_service_impl.cc:488:
SubscribeEnd(callback, subscription_id, curve25519dh, status);
On 2015/07/09 16:02:53, johnme wrote:
> Nit: it feels slightly inconsistent that you still use SubscribeEnd for the
> error cases here, now that there is SubscribeEndWithError. But I don't feel
> strongly.

The alternative would be to return from the gcm::GCMClient::SUCCESS case and
duplicate the DecreasePushSubscriptionCount call. I'd rather keep that logic
isolated.

I did move the curve25519dh definition and the TODO to clarify this.

https://codereview.chromium.org/1210283005/diff/1/content/browser/push_messag...
File content/browser/push_messaging/push_messaging_message_filter.cc (right):

https://codereview.chromium.org/1210283005/diff/1/content/browser/push_messag...
content/browser/push_messaging/push_messaging_message_filter.cc:69: void
ForwardPublicEncryptionKeysToIOThreadProxy(
On 2015/07/09 16:02:53, johnme wrote:
> It's a bit unusual that Core::GetPublicEncryptionKeyOnUI gets called on UI
> thread, but runs its callback on IO thread.
> 
> I've looked at the two callbacks (DidGetEncryptionKeys &
> DidGetRegistrationKeys), and it seems both of them could actually be
implemented
> on the UI thread almost as easily (you can replace Send with the Core::Send
> proxy; SendRegister{Success,Error} can be PostTask'd to; finally
> RecordGetRegistrationStatus is safe enough to call from UI thread - just
change
> the DCHECK_CURRENTLY_ON).
> 
> Then you could remove ForwardPublicEncryptionKeysToIOThreadProxy.
Alternatively,
> if you prefer to keep the current threading, then document it clearly (rename
> Core::GetPublicEncryptionKeyOnUI's callback parameter to io_thread_callback,
and
> add a comment in the method declaration).

All the Record*Status have DCHECKS to be on the IO thread, so
DidGetRegistrationKeys would become messier w/ another PostTask.

I've updated the argument names and comment.

https://codereview.chromium.org/1210283005/diff/1/content/browser/push_messag...
content/browser/push_messaging/push_messaging_message_filter.cc:112: void
GetPublicEncryptionKeyOnUI(
On 2015/07/09 16:02:53, johnme wrote:
> Please move this to the "Public helper methods on UI thread" section, since
it's
> used both for the Register flow and the GetRegistration flow.

Done.

https://codereview.chromium.org/1210283005/diff/1/content/browser/push_messag...
content/browser/push_messaging/push_messaging_message_filter.cc:364: void
PushMessagingMessageFilter::Core::GetPublicEncryptionKeyOnUI(
On 2015/07/09 16:02:53, johnme wrote:
> Similarly, please move this to the "Helper methods on both IO and UI threads,
> merged from PushMessagingMessageFilter and Core." section at the bottom of
this
> file.

Done.

https://codereview.chromium.org/1210283005/diff/1/content/browser/push_messag...
content/browser/push_messaging/push_messaging_message_filter.cc:861: void
PushMessagingMessageFilter::DidGetRegistrationKeys(
On 2015/07/09 16:02:53, johnme wrote:
> Please add a DCHECK_CURRENTLY_ON.

Done.

https://codereview.chromium.org/1210283005/diff/1/content/common/push_messagi...
File content/common/push_messaging_messages.h (right):

https://codereview.chromium.org/1210283005/diff/1/content/common/push_messagi...
content/common/push_messaging_messages.h:98: GURL /* origin */,
On 2015/07/09 16:02:53, johnme wrote:
> It seems problematic to let the untrusted renderer provide the origin
directly.
> 
> Other handlers typically use
>
service_worker_context_->GetLiveRegistration(service_worker_registration_id)->pattern().GetOrigin()
> to get the origin from the browser-process, though that's not much better (as
it
> seems easy to enumerate all possible service worker registration ids), though
> perhaps it at least limits it to origins which have a SW (which in this case
> isn't a particularly useful limitation).

I don't buy that argument. Passing the origin would allow us to check that the
origin matches the SW registration id, making iterating over possible SW
registration ids *much* harder. Right now we'd return the registration if it
exists, regardless of origin.

That said, keeping this all in PushMessagingMessageFilter and keeping the IPCs
consistent is good. I've moved it there.

https://codereview.chromium.org/1210283005/diff/1/content/shell/browser/layou...
File content/shell/browser/layout_test/layout_test_push_messaging_service.cc
(right):

https://codereview.chromium.org/1210283005/diff/1/content/shell/browser/layou...
content/shell/browser/layout_test/layout_test_push_messaging_service.cc:91:
kTestCurve25519dh, kTestCurve25519dh + arraysize(kTestCurve25519dh));
On 2015/07/09 16:02:53, johnme wrote:
> Nit: #include "base/macros.h"

Done.

https://codereview.chromium.org/1210283005/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1210283005/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:65365: +  <int value="11" label="Unable
to retrieve the public key"/>
On 2015/07/09 16:02:53, johnme wrote:
> Nit: Might want a more generic string to cover other possible errors, like
> failing to persist the private key to GCMDriver's store?

I think we'll want separate UMA for the GCMDriver store. Since Push will be the
first consumer, all data will be ours.

[Peter Beverloo, 2015-07-09 18:55:10]

peter@chromium.org changed reviewers:
+ avi@chromium.org, isherman@chromium.org, mkwst@chromium.org

[Peter Beverloo, 2015-07-09 18:55:11]

mkwst: Please review IPC & layout tests
isherman: Please review UMA
avi: Please review //content/public

[Avi (use Gerrit), 2015-07-09 19:11:14]

On 2015/07/09 18:55:11, Peter Beverloo wrote:
> mkwst: Please review IPC & layout tests
> isherman: Please review UMA
> avi: Please review //content/public

"// A push service-agnostic interface that the Push API uses"

The plausibility of calling this content code "service-agnostic" takes a dive if
we're locking it down to a specific crypto spec used by our code.

[Peter Beverloo, 2015-07-09 19:16:34]

On 2015/07/09 19:11:14, Avi wrote:
> On 2015/07/09 18:55:11, Peter Beverloo wrote:
> > mkwst: Please review IPC & layout tests
> > isherman: Please review UMA
> > avi: Please review //content/public
> 
> "// A push service-agnostic interface that the Push API uses"
> 
> The plausibility of calling this content code "service-agnostic" takes a dive
if
> we're locking it down to a specific crypto spec used by our code.

This is in fact in process of being standardized - I'm flying to IETF 93 Prague
in two weeks to, hopefully, finalize the details :-).

See the following IETF draft, W3C Push API proposal (by Mozilla) and blink-dev
Intent to Implement:

https://tools.ietf.org/html/draft-thomson-webpush-encryption-01
https://github.com/w3c/push-api/pull/130

https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/_LQX_cjAATA

Additionally, if the service does not require encryption (which any conformant
implementation will - including Mozilla and us) then it could just return an
empty vector. Decryption will be done well above this layer (in
//components/gcm_driver if you're interested).

[Mike West, 2015-07-10 08:09:33]

IPC LGTM.

[Avi (use Gerrit), 2015-07-10 15:46:17]

Well, then, LGTM

[Peter Beverloo, 2015-07-13 12:59:03]

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

[Peter Beverloo, 2015-07-13 12:59:03]

+asvitkine for UMA

[Alexei Svitkine (slow), 2015-07-13 14:56:28]

histograms lgtm

[Peter Beverloo, 2015-07-14 15:13:13]

The CQ bit was checked by peter@chromium.org

[Peter Beverloo, 2015-07-14 15:13:13]

The patchset sent to the CQ was uploaded after l-g-t-m from johnme@chromium.org,
mkwst@chromium.org, avi@chromium.org, asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/1210283005/#ps80001
(title: "android fix 2")

[commit-bot: I haz the power, 2015-07-14 15:13:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1210283005/80001

[commit-bot: I haz the power, 2015-07-14 16:27:43]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-07-14 16:28:52]

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