Blimp: add support for SSL connections.

blimp/client/session/assignment_source.h

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef BLIMP_CLIENT_SESSION_ASSIGNMENT_SOURCE_H_
 #define BLIMP_CLIENT_SESSION_ASSIGNMENT_SOURCE_H_
 
 #include <string>
 
 #include "base/callback.h"
+#include "base/memory/weak_ptr.h"
 #include "blimp/client/blimp_client_export.h"
 #include "net/base/ip_endpoint.h"
 #include "net/url_request/url_fetcher_delegate.h"
 
 namespace base {
+class FilePath;
 class SingleThreadTaskRunner;
+class Value;
 }
 
 namespace net {
 class URLFetcher;
 class URLRequestContextGetter;
+class X509Certificate;
 }
 
 namespace blimp {
 namespace client {
 
 // TODO(kmarshall): Take values from configuration data.
 const char kDummyClientToken[] = "MyVoiceIsMyPassport";
 
 // Potential assigner URLs.
 const char kDefaultAssignerURL[] =
     "https://blimp-pa.googleapis.com/v1/assignment";
 
 // An Assignment contains the configuration data needed for a client
 // to connect to the engine.
 struct BLIMP_CLIENT_EXPORT Assignment {
   enum TransportProtocol {
     UNKNOWN = 0,
     SSL = 1,
     TCP = 2,
     QUIC = 3,

[Wez, 2016/02/26 18:32:09]

We don't have a QUIC transport, so let's remove this.

[Kevin M, 2016/02/26 19:57:23]

Done.

   };
 
   Assignment();
   ~Assignment();
 
-  TransportProtocol transport_protocol;
-  net::IPEndPoint ip_endpoint;
-  std::string client_token;
-  std::string certificate;
-  std::string certificate_fingerprint;
-
   // Returns true if the net::IPEndPoint has an unspecified IP, port, or
   // transport protocol.
   bool is_null() const;

[Ryan Sleevi, 2016/02/25 22:16:25]

DESIGN NIT: I would have suggested this be called "IsValid()" (with the inverted
logic); null on a concrete object just feels like it 'reads' weird.

[Wez, 2016/02/26 18:32:09]

nit: is_valid() seems a more appropriate name for this - having one of those
fields unspecified does not seem conceptually null-like.

[Kevin M, 2016/02/26 19:57:23]

Done.

+
+  // The transport protocol used to contact the engine.

[Wez, 2016/02/26 18:32:09]

nit: This is a somewhat tautologous and ambiguous comment - at a first glance I
can't tell whether you mean it identifies the _type_ of transport protocol
(which seems to be the case) or it is the actual implementation of a transport
protocol.

Suggest: Specifies the transport to use to connect to the engine.

[Kevin M, 2016/02/26 19:57:23]

Done.

+  TransportProtocol transport_protocol;
+
+  // The IP address of the engine.

[Wez, 2016/02/26 18:32:09]

nit: This comment also seems tautologous, especially if you rename ip_endpoint
to something more descriptive, e.g. engine_endpoint

Suggest: Specifies the IP address & port on which to reach the engine.

[Kevin M, 2016/02/26 19:57:23]

Done.

+  net::IPEndPoint ip_endpoint;
+
+  // The token used to authenticate the client to the engine.

[Wez, 2016/02/26 18:32:09]

Suggest simply: Used to authenticate to the specified engine.

[Kevin M, 2016/02/26 19:57:23]

Done.

+  std::string client_token;

[Wez, 2016/02/26 18:32:09]

Also suggest renaming this to auth_token.

[Kevin M, 2016/02/26 19:57:23]

That would make this inconsistent with the Assigner API's nomenclature.

[Wez, 2016/03/01 00:23:55]

Yes, but we're already inconsistent with the names of the other fields
(engine_endpoint vs host_and_port, cert vs certificate), so I'm not sure that's
a concern.

[Kevin M, 2016/03/01 18:23:16]

It's not just the Assigner response; "client_token" is used all over the
codebase. I'd have to touch a lot of files to incorporate this change and keep
it consistent across all of blimp/'s source files, which would dilute the
reviewing focus of this CL. I'd prefer to do that in its own CL.

Filed bug 591074.

+
+  // The expected certificate of the engine. The peer certificate must match
+  // |cert| exactly, otherwise the connection will be dropped.

[Wez, 2016/02/26 18:32:09]

The suggestions I've given above switch the sense of the comments to describe
how the field is intended to be used, rather than just describing what it
contains (although the two are often pretty similar!) - I'd suggest doing the
same here, e.g. "Specifies the X509 certificate that the engine must report.
..."

[Kevin M, 2016/02/26 19:57:23]

Done.

+  scoped_refptr<net::X509Certificate> cert;
 };
 
 // AssignmentSource provides functionality to find out how a client should
 // connect to an engine.
 class BLIMP_CLIENT_EXPORT AssignmentSource : public net::URLFetcherDelegate {
  public:
   // A Java counterpart will be generated for this enum.
   // GENERATED_JAVA_ENUM_PACKAGE: org.chromium.blimp.assignment
   enum Result {
     RESULT_UNKNOWN = 0,
     RESULT_OK = 1,
     RESULT_BAD_REQUEST = 2,
     RESULT_BAD_RESPONSE = 3,
     RESULT_INVALID_PROTOCOL_VERSION = 4,
     RESULT_EXPIRED_ACCESS_TOKEN = 5,
     RESULT_USER_INVALID = 6,
     RESULT_OUT_OF_VMS = 7,
     RESULT_SERVER_ERROR = 8,
     RESULT_SERVER_INTERRUPTED = 9,
-    RESULT_NETWORK_FAILURE = 10
+    RESULT_NETWORK_FAILURE = 10,
+    RESULT_INVALID_CERT = 11,
   };
 
   typedef base::Callback<void(AssignmentSource::Result, const Assignment&)>
       AssignmentCallback;
 
-  // The |main_task_runner| should be the task runner for the UI thread because
-  // this will in some cases be used to trigger user interaction on the UI
-  // thread.
-  AssignmentSource(
-      const scoped_refptr<base::SingleThreadTaskRunner>& main_task_runner,
-      const scoped_refptr<base::SingleThreadTaskRunner>& io_task_runner);
+  explicit AssignmentSource(
+      scoped_refptr<base::SingleThreadTaskRunner> io_task_runner);

[Wez, 2016/02/26 18:32:09]

nit: Document why the |io_task_runner| is needed, e.g. to use for File I/O? Or
URL fetching, etc?

[Kevin M, 2016/02/26 19:57:23]

Done.

   ~AssignmentSource() override;
 
   // Retrieves a valid assignment for the client and posts the result to the
   // given callback.  |client_auth_token| is the OAuth2 access token to send to
   // the assigner when requesting an assignment.  If this is called before a
   // previous call has completed, the old callback will be called with
   // RESULT_SERVER_INTERRUPTED and no Assignment.
   void GetAssignment(const std::string& client_auth_token,
                      const AssignmentCallback& callback);
 
+  // Called when GetCustomAssignment() has completed.

[Wez, 2016/02/26 18:32:09]

GetCustomAssignment() isn't declared here, so it doesn't make sense to refer to
it in documenting a public API.

Why is this API, which seems to be a completion-handler function really, public?

What is a "custom assignment"?

[Kevin M, 2016/02/26 19:57:23]

Doesn't need to be public. "custom assignment" means "command line parsed
assignment".

Changed the name and visibility.

+  // Uses |custom_assignment| if provided; queries the Assigner for one
+  // otherwise.
+  void OnGetCustomAssignmentDone(const std::string& client_auth_token,
+                                 Assignment custom_assignment);
+
   // net::URLFetcherDelegate implementation:
   void OnURLFetchComplete(const net::URLFetcher* source) override;
 
  private:
   void ParseAssignerResponse();
-
-  scoped_refptr<base::SingleThreadTaskRunner> main_task_runner_;
-
-  scoped_refptr<net::URLRequestContextGetter> url_request_context_;
-  scoped_ptr<net::URLFetcher> url_fetcher_;
+  void AssignerJSONParseOK(scoped_ptr<base::Value> json);
+  void AssignerJSONParseError(const std::string& error);

[Wez, 2016/02/26 18:32:09]

These names don't scan - do you mean OnJsonParsed() and OnJsonParseError(),
perhaps?

 
   // This callback is set during a call to GetAssignment() and is cleared after
   // the request has completed (whether it be a success or failure).

[Wez, 2016/02/26 18:32:09]

nit: (And I realise this was already here) This comment describes _what_ you're
doing, but not _why_ - the only reason I can see offhand is to so that we can
DCHECK that GetAssignment isn't called twice?

[Kevin M, 2016/02/26 19:57:23]

Mostly because URLFetcher takes a delegate object for completion, not a
base::Callback, so this is how we stash the callback for later invocation.

Added the "why".

   AssignmentCallback callback_;
 
+  scoped_refptr<base::SingleThreadTaskRunner> io_task_runner_;
+  scoped_refptr<net::URLRequestContextGetter> url_request_context_;
+  scoped_ptr<net::URLFetcher> url_fetcher_;
+  base::WeakPtrFactory<AssignmentSource> weak_factory_;

[Wez, 2016/02/26 18:32:09]

nit: Suggest separating the weak_factory_ from the rest by a blank-line since it
is Special. :P

[Kevin M, 2016/02/26 19:57:23]

Done.

+
   DISALLOW_COPY_AND_ASSIGN(AssignmentSource);
 };
 
 }  // namespace client
 }  // namespace blimp
 
 #endif  // BLIMP_CLIENT_SESSION_ASSIGNMENT_SOURCE_H_