Adding support for authenticated copresence calls

components/copresence/copresence_manager_impl.cc

Index: components/copresence/copresence_manager_impl.cc
diff --git a/components/copresence/copresence_manager_impl.cc b/components/copresence/copresence_manager_impl.cc
index 342ed1999a184cb59788612f951bb25175595bc7..da05c9b63e12901f2cb2908fd18679cdf95ace5d 100644
--- a/components/copresence/copresence_manager_impl.cc
+++ b/components/copresence/copresence_manager_impl.cc
@@ -11,38 +11,57 @@
 
 namespace copresence {
 
-PendingRequest::PendingRequest(const copresence::ReportRequest& report,
-                               const std::string app_id,
+PendingRequest::PendingRequest(const ReportRequest& report,
+                               const std::string& app_id,
+                               const std::string& auth_token,
                                const StatusCallback& callback)
-    : report(report), app_id(app_id), callback(callback) {
-}
+    : report(new ReportRequest(report)),
+      app_id(app_id),
+      auth_token(auth_token),
+      callback(callback) {}
 
-PendingRequest::~PendingRequest() {
-}
+PendingRequest::~PendingRequest() {}
 
 // Public methods
 
-CopresenceManagerImpl::~CopresenceManagerImpl() {}
+CopresenceManagerImpl::~CopresenceManagerImpl() {
+  whispernet_init_callback_.Cancel();
+}
 
 // Returns false if any operations were malformed.
 void CopresenceManagerImpl::ExecuteReportRequest(
-    ReportRequest request,
+    const ReportRequest& request,
     const std::string& app_id,
     const StatusCallback& callback) {
-  // Don't take on any more requests. We can't execute them since init failed.
+  // If initialization has failed, reject all requests.
   if (init_failed_) {
     callback.Run(FAIL);
     return;
   }
 
-  DCHECK(rpc_handler_.get());
+  // Check if we are initialized enough to execute this request.
+  // If we haven't seen this auth token yet, we need to register for it.
+  DCHECK(rpc_handler_);
+  if (!rpc_handler_->IsRegisteredForToken(delegate_->GetAuthToken())) {
+    pending_init_operations_++;
+    rpc_handler_->RegisterForToken(

[rkc, 2014/10/28 04:12:45]

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.

[Charlie, 2014/10/28 22:51:51]

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.

+        delegate_->GetAuthToken(),
+        // The manager owns the RpcHandler, so this callback cannot outlive us.
+        base::Bind(&CopresenceManagerImpl::InitStepComplete,
+                   base::Unretained(this),
+                   "Device registration"));

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

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.

[Charlie, 2014/10/28 22:51:51]

Done.

+  }
+
+  // Execute the request if possible, or queue it
+  // if initialization is still in progress.
   if (pending_init_operations_) {
-    pending_requests_queue_.push_back(
-        PendingRequest(request, app_id, callback));
+    pending_requests_queue_.push_back(make_scoped_ptr(new PendingRequest(
+        request, app_id, delegate_->GetAuthToken(), callback)));
   } else {
     rpc_handler_->SendReportRequest(
-        make_scoped_ptr(new copresence::ReportRequest(request)),
+        make_scoped_ptr(new ReportRequest(request)),
         app_id,
+        delegate_->GetAuthToken(),
         callback);
   }
 }
@@ -52,7 +71,8 @@ void CopresenceManagerImpl::ExecuteReportRequest(
 CopresenceManagerImpl::CopresenceManagerImpl(CopresenceDelegate* delegate)
     : init_failed_(false),
       pending_init_operations_(0),
-      delegate_(delegate) {
+      delegate_(delegate),
+      rpc_handler_(new RpcHandler(delegate)) {
   DCHECK(delegate);
 }
 
@@ -64,14 +84,15 @@ void CopresenceManagerImpl::CompleteInitialization() {
   if (!init_failed_)
     rpc_handler_->ConnectToWhispernet();
 
-  for (PendingRequest& request : pending_requests_queue_) {
+  for (scoped_ptr<PendingRequest>& request : pending_requests_queue_) {
     if (init_failed_) {
-      request.callback.Run(FAIL);
+      request->callback.Run(FAIL);
     } else {
       rpc_handler_->SendReportRequest(
-          make_scoped_ptr(new copresence::ReportRequest(request.report)),
-          request.app_id,
-          request.callback);
+          request->report.Pass(),
+          request->app_id,
+          request->auth_token,
+          request->callback);
     }
   }
   pending_requests_queue_.clear();
@@ -82,11 +103,32 @@ void CopresenceManagerImpl::InitStepComplete(
   if (!success) {
     LOG(ERROR) << step << " failed!";
     init_failed_ = true;
+    // TODO(ckehoe): Retry for registration failures. But maybe not here.
   }
 
-  DVLOG(3) << "Init step: " << step << " complete.";
+  DVLOG(3) << step << " complete.";
+  DCHECK(pending_init_operations_ > 0);
   pending_init_operations_--;
   CompleteInitialization();
 }
 
+// static
+scoped_ptr<CopresenceManager> CopresenceManager::Create(
+    CopresenceDelegate* delegate) {
+  CopresenceManagerImpl* manager = new CopresenceManagerImpl(delegate);
+
+  manager->pending_init_operations_++;

[rkc, 2014/10/28 04:12:45]

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);"

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

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

+  manager->whispernet_init_callback_.Reset(
+      base::Bind(&CopresenceManagerImpl::InitStepComplete,
+                 // This callback will be canceled on manager's destruction,
+                 // so unretained is safe to use here.
+                 base::Unretained(manager),
+                 "Whispernet proxy initialization"));
+  DCHECK(delegate->GetWhispernetClient());
+  delegate->GetWhispernetClient()->Initialize(
+      manager->whispernet_init_callback_.callback());
+
+  return make_scoped_ptr<CopresenceManager>(manager);
+}
+
 }  // namespace copresence