Introduce CryptAuthClient, a class capable of performing all CryptAuth APIs.

components/proximity_auth/cryptauth/cryptauth_client.h

+// Copyright 2014 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 COMPONENTS_PROXIMITY_AUTH_CRYPT_AUTH_CLIENT_H
+#define COMPONENTS_PROXIMITY_AUTH_CRYPT_AUTH_CLIENT_H
+
+#include "base/callback.h"
+#include "base/macros.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/memory/weak_ptr.h"
+#include "components/proximity_auth/cryptauth/proto/cryptauth_api.pb.h"

[Ilya Sherman, 2014/11/18 22:30:44]

nit: Can you forward-declare the necessary classes, rather than #including the
header?  That would allow us not to make the the proto target a public dep,
which makes the build easier to configure correctly.

[Tim Song, 2014/12/03 01:18:24]

You will still have to include cryptauth_api.pb.h for the response messages, so
the proto target will need to be a public dep.

+#include "net/url_request/url_request_context_getter.h"
+#include "url/gurl.h"
+
+class OAuth2TokenService;
+
+namespace proximity_auth {
+
+class CryptAuthAccessTokenFetcher;
+class CryptAuthApiCallFlow;
+
+// Use CryptAuthClient to make API requests to the CryptAuth service, which
+// manages cryptographic credentials (ie. public keys) for a user's devices.
+// At most one request can be processed concurrently; the error callback will
+// be invoked if you make a request when there is another pending. Create
+// another CryptAuthClient if you want to make two requests at the same time.
+class CryptAuthClient {
+ public:
+  typedef base::Callback<void(const std::string&)> ErrorCallback;
+
+  CryptAuthClient(net::URLRequestContextGetter* url_request_context,

[Ilya Sherman, 2014/11/18 22:30:44]

It looks like this is stored in a scoped_refptr below, so please pass it as one
as well.

[Tim Song, 2014/12/03 01:18:24]

Done.

+                  CryptAuthAccessTokenFetcher* access_token_fetcher);

[Ilya Sherman, 2014/11/18 22:30:44]

nit: Please document, including lifetime expectations.

[Tim Song, 2014/12/03 01:18:24]

Done.

+  virtual ~CryptAuthClient();
+
+  // The GetMyDevices API request returns a list of devices that the user has
+  // registered with CryptAuth.
+  typedef base::Callback<void(const cryptauth::GetMyDevicesResponse&)>
+      GetMyDevicesCallback;
+  void GetMyDevices(bool allow_stale_read,
+                    GetMyDevicesCallback callback,
+                    ErrorCallback error_callback);

[Ilya Sherman, 2014/11/18 22:30:44]

nit: Please pass callbacks by const-reference.  (Applies throughout.)

[Tim Song, 2014/12/03 01:18:24]

Done.

+
+  // The FindEligibleUnlockDevices API request returns a list of devices
+  // eligible to be an "unlock key", that is a device capable of unlocking other
+  // devices.
+  // The |bluetooth_address| field should contain the Bluetooth address of the
+  // local device, which is sent to the eligible devices so they can connect to
+  // the local device.

[Ilya Sherman, 2014/11/18 22:30:44]

I wonder if it would be better to just document the calls in the .proto file,
and refer readers of this file to that one.  Having the documentation in just
one place seems like it would lead to a lower likelihood of stale or conflicting
docs.

[Tim Song, 2014/12/03 01:18:24]

I would prefer the documentation to be here as the proto file is pulled from
google3 and keeping that in sync is problematic by itself. Plus, users will
probably look at this file first rather than the proto.

+  typedef base::Callback<void(
+      const cryptauth::FindEligibleUnlockDevicesResponse&)>
+      FindEligibleUnlockDevicesCallback;
+  void FindEligibleUnlockDevices(const std::string& bluetooth_address,
+                                 FindEligibleUnlockDevicesCallback callback,
+                                 ErrorCallback error_callback);
+
+  // The SendDeviceSyncTickle API request tells the server to "tickle" all
+  // the user's registered devices, so they can sync the latest device state.
+  typedef base::Callback<void(const cryptauth::SendDeviceSyncTickleResponse&)>
+      SendDeviceSyncTickleCallback;
+  void SendDeviceSyncTickle(SendDeviceSyncTickleCallback callback,
+                            ErrorCallback error_callback);
+
+  // The ToggleEasyUnlock API request tells the server to designate devices as
+  // an "unlock key" that can unlock other devices. The |turn_on| argument is
+  // used to enable or disable the device given by its |public_key|.  If
+  // |apply_to_all| is true, then |public_key| will be ignored, and |turn_on|
+  // will be applied to all eligible unlock keys.
+  typedef base::Callback<void(const cryptauth::ToggleEasyUnlockResponse&)>
+      ToggleEasyUnlockCallback;
+  void ToggleEasyUnlock(bool turn_on,
+                        bool apply_to_all,
+                        const std::string public_key,

[Ilya Sherman, 2014/11/18 22:30:44]

nit: Please pass strings by const-reference too (also applies throughout).

[Ilya Sherman, 2014/11/18 22:30:44]

Since we already reference cryptauth::ToggleEasyUnlockResponse in the callback,
it might be cleaner to simply pass a cryptauth::ToggleEasyUnlockRequest object
to this method, rather than repeating the fields.  Ditto for all of the other
wrapper methods.

[Tim Song, 2014/12/03 01:18:24]

Done.

[Tim Song, 2014/12/03 01:18:24]

Done.

+                        ToggleEasyUnlockCallback callback,
+                        ErrorCallback error_callback);
+
+  // The SetupEnrollment API request is the first of a two step process to
+  // enroll the device with CryptAuth. See FinishEnrollment for the last step of
+  // enrollment.
+  // You most likely will not use this API directly. Look instead at
+  // CryptAuthEnroller, which handles the entire enrollment operation.
+  typedef base::Callback<void(const cryptauth::SetupEnrollmentResponse&)>
+      SetupEnrollmentCallback;
+  void SetupEnrollment(const std::string application_id,
+                       const std::vector<std::string>& supported_protocols,
+                       SetupEnrollmentCallback callback,
+                       ErrorCallback error_callback);
+
+  // The FinishEnrollment API request is the last of a two step process to
+  // enroll the device with CryptAuth. See for the SetupEnrollment for the
+  // first step of enrollment.
+  // You most likely will not use this API directly. Look instead at
+  // CryptAuthEnroller, which handles the entire enrollment operation.
+  typedef base::Callback<void(const cryptauth::FinishEnrollmentResponse&)>
+      FinishEnrollmentCallback;
+  void FinishEnrollment(const std::string& enrollment_session_id,
+                        const std::string& enrollment_message,
+                        const std::string& device_ephermeral_key,
+                        FinishEnrollmentCallback callback,
+                        ErrorCallback error_callback);
+
+ protected:
+  // Creates a CryptAuthApiCallFlow object and takes ownership of it.
+  // Exposed for testing.
+  virtual CryptAuthApiCallFlow* CreateFlow(GURL request_url);

[Ilya Sherman, 2014/11/18 22:30:44]

nit: Please pass by const-reference.

[Tim Song, 2014/12/03 01:18:24]

Done.

+
+ private:
+  // Starts a call to the API given by |request_path|, with the templated
+  // request and response types. The client first fetches the access token and
+  // then makes the HTTP request.
+  template <class RequestProto, class ResponseProto>
+  void MakeApiCall(const std::string& request_path,
+                   const RequestProto& request_proto,
+                   base::Callback<void(const ResponseProto&)> response_callback,
+                   ErrorCallback error_callback);

[Ilya Sherman, 2014/11/18 22:30:44]

Expanding on a comment above: Why not just expose this method publicly, and
remove all of the specialized methods?

[Tim Song, 2014/12/03 01:18:24]

I prefer making all the possible API calls explicitly defined here, so the user
doesn't need to know the URL path for each API call. Another advantage is that
we can easily add tracing, logs, or metrics for each API calls.

+
+  // Called when the access token is obtained so the API request can be made.
+  template <class ResponseProto>
+  void OnAccessTokenFetched(
+      std::string serialized_request,
+      base::Callback<void(const ResponseProto&)> response_callback,
+      const std::string& access_token);
+
+  // Called with CryptAuthApiCallFlow completes successfully to deserialize and
+  // return the result.
+  template <class ResponseProto>
+  void OnFlowSuccess(base::Callback<void(const ResponseProto&)> result_callback,
+                     const std::string& serialized_response);
+  // Called when the current API call fails at any step.
+  void OnApiCallFailed(const std::string& error_message);
+
+  // The context for network requests.
+  scoped_refptr<net::URLRequestContextGetter> url_request_context_;

[Ilya Sherman, 2014/11/18 22:30:44]

nit: Please leave blank lines between documented variables and methods. 
(Applies above and below as well.)

[Tim Song, 2014/12/03 01:18:24]

Done.

+  // Fetchs the access token authorizing the API calls.
+  CryptAuthAccessTokenFetcher* access_token_fetcher_;

[Ilya Sherman, 2014/11/18 22:30:44]

nit: Please document lifetime expectations.  Alternately, would it be
appropriate for this class to own the token fetcher, and store it in a
scoped_ptr?

[Tim Song, 2014/12/03 01:18:24]

I haven't implemented the access token fetching yet, but I was originally
thinking that it could be reused among multiple clients.

However, thinking about it some more that might make ownership kind of tricky as
there is no good place to put it. I'll just make it a scoped_ptr for now.

+
+  // Handles the current API call.
+  scoped_ptr<CryptAuthApiCallFlow> flow_;
+  // URL path of the current request.
+  std::string request_path_;
+  // Called when the current request fails.
+  ErrorCallback error_callback_;
+
+  base::WeakPtrFactory<CryptAuthClient> weak_ptr_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(CryptAuthClient);
+};
+
+}  // namespace proximity_auth
+
+#endif  // COMPONENTS_PROXIMITY_AUTH_CRYPT_AUTH_CLIENT_H