Adding support for authenticated copresence calls

Adding support for authenticated copresence calls. I've added a temporary call to the API to pass in an OAuth token to use with requests. Eventually we need to scope the token better - at least by app.

For privacy, we now register one device ID per account (auth token), including the null (anonymous) account.

Other changes:
- API keys are now scoped by app
- copresence_manager.cc deleted
- Minor performance/stability fixes
- Cleanup in rpc_handler_unittest.cc

BUG=423517
Committed: https://crrev.com/153e9af7a65fddeb5807427f681df8916fbfdffb
Cr-Commit-Position: refs/heads/master@{#302002}

[Charlie, 2014-10-21 20:51:03]

ckehoe@chromium.org changed reviewers:
+ rkc@chromium.org

[Charlie, 2014-10-21 20:51:04]

Still trying to figure out how to test this. But let me know how the code looks.

[Charlie, 2014-10-21 20:55:13]

ckehoe@chromium.org changed reviewers:
+ kalman@chromium.org

[Charlie, 2014-10-21 20:55:14]

+Ben for IDL review.

This is a temporary solution to allow per-app authenticated copresence calls.
See also crbug/423517. I can forward you the tangled PM/privacy thread if you're
interested.

[Charlie, 2014-10-21 21:01:41]

ckehoe@chromium.org changed reviewers:
+ isherman@chromium.org

[Charlie, 2014-10-21 21:01:43]

+Ilya for histograms review

[Ilya Sherman, 2014-10-21 21:03:28]

histograms lgtm

[not at google - send to devlin, 2014-10-21 22:07:34]

lgtm

[rkc, 2014-10-21 22:48:38]

https://codereview.chromium.org/671573003/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/copresence/copresence_api.cc (right):

https://codereview.chromium.org/671573003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/copresence/copresence_api.cc:131: auto key =
api_keys_by_app_.find(app_id);
const auto& ?

https://codereview.chromium.org/671573003/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/copresence/copresence_api.h (right):

https://codereview.chromium.org/671573003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/copresence/copresence_api.h:80:
std::map<std::string, std::string> api_keys_by_app_;
Instead of three different maps, all indexing by app_id, have one map of app_id
<-> AppCopresenceData (or whatever you want to call it). AppCopresenceData can
contain the API key, auth token and the subscription ID's belonging to an app.

https://codereview.chromium.org/671573003/diff/40001/components/copresence/rp...
File components/copresence/rpc/rpc_handler.cc (right):

https://codereview.chromium.org/671573003/diff/40001/components/copresence/rp...
components/copresence/rpc/rpc_handler.cc:182: "",
std::string(), here and below.

[Charlie, 2014-10-21 23:19:45]

https://codereview.chromium.org/671573003/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/copresence/copresence_api.cc (right):

https://codereview.chromium.org/671573003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/copresence/copresence_api.cc:131: auto key =
api_keys_by_app_.find(app_id);
On 2014/10/21 22:48:38, Rahul Chaturvedi wrote:
> const auto& ?

Done.

https://codereview.chromium.org/671573003/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/copresence/copresence_api.h (right):

https://codereview.chromium.org/671573003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/copresence/copresence_api.h:80:
std::map<std::string, std::string> api_keys_by_app_;
On 2014/10/21 22:48:38, Rahul Chaturvedi wrote:
> Instead of three different maps, all indexing by app_id, have one map of
app_id
> <-> AppCopresenceData (or whatever you want to call it). AppCopresenceData can
> contain the API key, auth token and the subscription ID's belonging to an app.

Putting the subscriptions IDs in doesn't actually work, because the index is the
wrong direction. We get messages back from the server tagged with their
subscription IDs. From that, we need to know which app to send each message to.
If we iterate through all the apps, it defeats the purpose of the map.

If you still want the other two maps collapsed, we can do that. But I don't
think it would be a lot cleaner.

https://codereview.chromium.org/671573003/diff/40001/components/copresence/rp...
File components/copresence/rpc/rpc_handler.cc (right):

https://codereview.chromium.org/671573003/diff/40001/components/copresence/rp...
components/copresence/rpc/rpc_handler.cc:182: "",
On 2014/10/21 22:48:38, Rahul Chaturvedi wrote:
> std::string(), here and below.

Done.

[rkc, 2014-10-22 19:21:13]

lgtm

At this time the two maps are alright, but if we do have more app specific data,
we should definitely look into collapsing the maps. Consider adding a TODO or
NOTE there.

[Charlie, 2014-10-22 19:28:50]

On 2014/10/22 19:21:13, Rahul Chaturvedi wrote:
> lgtm
> 
> At this time the two maps are alright, but if we do have more app specific
data,
> we should definitely look into collapsing the maps. Consider adding a TODO or
> NOTE there.

Done.

[Charlie, 2014-10-24 23:43:12]

Ok I added separate device registration for authenticated calls, as the server
requires (and privacy probably will too). PTAL.

[rkc, 2014-10-28 04:12:46]

https://codereview.chromium.org/671573003/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/api/copresence/copresence_api.cc (right):

https://codereview.chromium.org/671573003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/copresence/copresence_api.cc:34: //
CopresenceService implementation
.

https://codereview.chromium.org/671573003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/copresence/copresence_api.cc:185: //
CopresenceSetApiKeyFunction implementation
.

https://codereview.chromium.org/671573003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/copresence/copresence_api.cc:205:
->set_auth_token(params->token);
Add a TODO here with a link to a bug tracking that we should change this to be
handled correctly for multiple apps + unauthenticated.

https://codereview.chromium.org/671573003/diff/100001/components/copresence/c...
File components/copresence/copresence_manager_impl.cc (right):

https://codereview.chromium.org/671573003/diff/100001/components/copresence/c...
components/copresence/copresence_manager_impl.cc:47:
rpc_handler_->RegisterForToken(
A cleaner design would be that we have each RegisterDevice request as its own
object, with an attached queue of ReportRequests. The main issue with this
design is that now all execute requests will have to wait for all the register
calls that are active to finish first. In practice, this isn't a real issue but
the design feels, clunky. When we touch this code again, we should consider
moving to a design in which we have a pending queue per register device; which
will be bound to that particular register device request.

Considering that we may have RegisterDevice rolled into report request anyway,
the point would become moot and we'd have to redesign this entire class then
anyway.

https://codereview.chromium.org/671573003/diff/100001/components/copresence/c...
components/copresence/copresence_manager_impl.cc:52: "Device registration"));
Adding the last few characters of the Auth token to this description to help
disambiguate between different tokens. Do not however include the full token -
we shouldn't leak information that sensitive to logs. Last 2 or 4 characters
should be fine though.

https://codereview.chromium.org/671573003/diff/100001/components/copresence/c...
components/copresence/copresence_manager_impl.cc:120:
manager->pending_init_operations_++;
Creating a derived class and directly accessing its internals completely breaks
all encapsulation - this might as well be an extern Create function than in the
parent class.

Since we aren't creating the CopresenceManagerImpl anywhere except here, there
really remains no reason to not have all of this code in the
CopresenceManagerImpl's constructor itself. It'll bring this Create function
down to a clean "return new CopresenceManagerImpl(delegate);"

https://codereview.chromium.org/671573003/diff/100001/components/copresence/c...
File components/copresence/copresence_manager_impl.h (right):

https://codereview.chromium.org/671573003/diff/100001/components/copresence/c...
components/copresence/copresence_manager_impl.h:58:
std::vector<scoped_ptr<PendingRequest>> pending_requests_queue_;
Don't use scoped_ptrs in std C++ containers; they move around and make multiple
copies of their members. Usually if you need a vector of pointers that you want
scoped, use ScopedVector. Looking at your use case though, you can't use a
ScopedVector either (since the scoped vector owns all the pointers till it is
destructed). It may be best just to leave this as it is.

[Charlie, 2014-10-28 22:51:52]

https://codereview.chromium.org/671573003/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/api/copresence/copresence_api.cc (right):

https://codereview.chromium.org/671573003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/copresence/copresence_api.cc:34: //
CopresenceService implementation
On 2014/10/28 04:12:45, Rahul Chaturvedi wrote:
> .

Done.

https://codereview.chromium.org/671573003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/copresence/copresence_api.cc:185: //
CopresenceSetApiKeyFunction implementation
On 2014/10/28 04:12:45, Rahul Chaturvedi wrote:
> .

Done.

https://codereview.chromium.org/671573003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/copresence/copresence_api.cc:205:
->set_auth_token(params->token);
On 2014/10/28 04:12:45, Rahul Chaturvedi wrote:
> Add a TODO here with a link to a bug tracking that we should change this to be
> handled correctly for multiple apps + unauthenticated.

Done.

https://codereview.chromium.org/671573003/diff/100001/components/copresence/c...
File components/copresence/copresence_manager_impl.cc (right):

https://codereview.chromium.org/671573003/diff/100001/components/copresence/c...
components/copresence/copresence_manager_impl.cc:47:
rpc_handler_->RegisterForToken(
On 2014/10/28 04:12:45, Rahul Chaturvedi wrote:
> A cleaner design would be that we have each RegisterDevice request as its own
> object, with an attached queue of ReportRequests. The main issue with this
> design is that now all execute requests will have to wait for all the register
> calls that are active to finish first. In practice, this isn't a real issue
but
> the design feels, clunky. When we touch this code again, we should consider
> moving to a design in which we have a pending queue per register device; which
> will be bound to that particular register device request.
> 
> Considering that we may have RegisterDevice rolled into report request anyway,
> the point would become moot and we'd have to redesign this entire class then
> anyway.

Yes. Added a TODO.

Rolling the RegisterDevice in with the first Report doesn't fully solve the
issue, because we could still get a second Report before the first (batched)
call has completed.

https://codereview.chromium.org/671573003/diff/100001/components/copresence/c...
components/copresence/copresence_manager_impl.cc:52: "Device registration"));
On 2014/10/28 04:12:46, Rahul Chaturvedi wrote:
> Adding the last few characters of the Auth token to this description to help
> disambiguate between different tokens. Do not however include the full token -
> we shouldn't leak information that sensitive to logs. Last 2 or 4 characters
> should be fine though.
> 

Done.

https://codereview.chromium.org/671573003/diff/100001/components/copresence/c...
components/copresence/copresence_manager_impl.cc:120:
manager->pending_init_operations_++;
On 2014/10/28 04:12:45, Rahul Chaturvedi wrote:
> Creating a derived class and directly accessing its internals completely
breaks
> all encapsulation - this might as well be an extern Create function than in
the
> parent class.
> 
> Since we aren't creating the CopresenceManagerImpl anywhere except here, there
> really remains no reason to not have all of this code in the
> CopresenceManagerImpl's constructor itself. It'll bring this Create function
> down to a clean "return new CopresenceManagerImpl(delegate);"

Yes, this has become simple enough that it can probably be in the constructor.
Done.

https://codereview.chromium.org/671573003/diff/100001/components/copresence/c...
File components/copresence/copresence_manager_impl.h (right):

https://codereview.chromium.org/671573003/diff/100001/components/copresence/c...
components/copresence/copresence_manager_impl.h:58:
std::vector<scoped_ptr<PendingRequest>> pending_requests_queue_;
On 2014/10/28 04:12:46, Rahul Chaturvedi wrote:
> Don't use scoped_ptrs in std C++ containers; they move around and make
multiple
> copies of their members. Usually if you need a vector of pointers that you
want
> scoped, use ScopedVector. Looking at your use case though, you can't use a
> ScopedVector either (since the scoped vector owns all the pointers till it is
> destructed). It may be best just to leave this as it is.

Actually the ScopedVector works just fine. We don't want ownership of the
PendingRequest pointers, only their contents.

[rkc, 2014-10-29 18:44:34]

lgtm

[Charlie, 2014-10-29 18:58:21]

The CQ bit was checked by ckehoe@chromium.org

[commit-bot: I haz the power, 2014-10-29 18:59:35]

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

[commit-bot: I haz the power, 2014-10-29 19:04:03]

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

[commit-bot: I haz the power, 2014-10-29 19:04:04]

Try jobs failed on following builders:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/75329)
  android_arm64_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel/...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Charlie, 2014-10-29 20:47:51]

The CQ bit was checked by ckehoe@chromium.org

[commit-bot: I haz the power, 2014-10-29 20:49:04]

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

[Charlie, 2014-10-29 20:56:57]

The CQ bit was checked by ckehoe@chromium.org

[commit-bot: I haz the power, 2014-10-29 20:58:14]

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

[Charlie, 2014-10-29 23:35:20]

The CQ bit was checked by ckehoe@chromium.org

[commit-bot: I haz the power, 2014-10-29 23:37:44]

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

[commit-bot: I haz the power, 2014-10-30 01:10:29]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2014-10-30 01:11:22]

Patchset 13 (id:??) landed as
https://crrev.com/153e9af7a65fddeb5807427f681df8916fbfdffb
Cr-Commit-Position: refs/heads/master@{#302002}