Adding the AccessTokenFetcher and Environment class to the app remoting test

remoting/test/access_token_fetcher.h

+// Copyright 2015 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 REMOTING_TEST_ACCESS_TOKEN_FETCHER_H_
+#define REMOTING_TEST_ACCESS_TOKEN_FETCHER_H_
+
+#include "base/callback.h"
+#include "base/memory/ref_counted.h"
+#include "base/memory/scoped_ptr.h"
+#include "google_apis/gaia/gaia_oauth_client.h"
+
+namespace remoting {
+namespace test {
+
+typedef base::Callback<void(
+    const std::string& access_token,
+    const std::string& refresh_token)> AccessTokenCallback;
+
+// This class presents the same interface as the GaiaOAuthClient and wraps an
+// instance of that class internally.  The reason for using an adapter instead
+// of creating the client object directly is that the GaiaOAuthClient class
+// has non-virtual methods so it is difficult to abstract.
+// Using this adapter allows us to easily create a mock or fake class and test
+// the AccessTokenFetcher more efficiently.

[Wez, 2015/02/13 03:01:51]

Why do we need this class? Why not just make the GaiaOAuthClient itself have
virtual methods suitable for mocking?

[joedow, 2015/02/14 02:31:25]

Done.

+class GaiaOAuthClientAdapter {

[Wez, 2015/02/13 03:01:50]

nit: Move this class into it's own .h & .cc files?

[joedow, 2015/02/14 02:31:26]

Done.

+ public:
+  explicit GaiaOAuthClientAdapter(net::URLRequestContextGetter* context_getter);

[Wez, 2015/02/13 03:01:51]

const scoped_refptr<>&

[joedow, 2015/02/14 02:31:26]

Done.

+  virtual ~GaiaOAuthClientAdapter();
+
+  // Uses the |auth_code| passed in to retrieve an access token.

[Wez, 2015/02/13 03:01:51]

Since these three methods just mirror the GaiaOAuthClient API, just have a block
comment "These methods mirror the GaiaOAuthClient API"?

[joedow, 2015/02/14 02:31:26]

Done.

+  virtual void GetTokensFromAuthCode(
+      const gaia::OAuthClientInfo& oauth_client_info,
+      const std::string& auth_code,
+      int max_retries,
+      gaia::GaiaOAuthClient::Delegate* delegate);
+
+  // Uses the |refresh_token| passed in to retrieve an access token.
+  virtual void RefreshToken(

[Wez, 2015/02/13 03:01:51]

This seems a daft name for the API; why is it not
GetAccessTokenFromRefreshToken()?

[joedow, 2015/02/14 02:31:25]

Done.

+      const gaia::OAuthClientInfo& oauth_client_info,
+      const std::string& refresh_token,
+      const std::vector<std::string>& scopes,
+      int max_retries,
+      gaia::GaiaOAuthClient::Delegate* delegate);
+
+  // Calls the Gaia service to validate the |oauth_access_token| passed in.
+  virtual void GetTokenInfo(
+      const std::string& oauth_access_token,
+      int max_retries,
+      gaia::GaiaOAuthClient::Delegate* delegate);
+
+ private:
+  // Used to make token requests to Gaia service.  Uses a request_context_getter
+  // to make those requests on the current thread's task runner.

[Wez, 2015/02/13 03:01:51]

"Uses a ..." doesn't seem relevant here - the relevance of the context getter
should be documented on the ctor, where it's passed-in.

It's also the case that the thread that the context getter works on is an
implementation detail of that class, not a property of this one.

[joedow, 2015/02/14 02:31:25]

Done.

+  scoped_ptr<gaia::GaiaOAuthClient> auth_client_;
+
+  DISALLOW_COPY_AND_ASSIGN(GaiaOAuthClientAdapter);
+};
+
+// The AccessTokenFetcher class is used to retrieve an access token from either

[Wez, 2015/02/13 03:01:51]

You don't really need "The AccessTokenFetcher class is used to" - just start
with "Retrieves an access token..." - the fact this is a class comment is
implicit in its placement.

[joedow, 2015/02/14 02:31:25]

Done.

+// an authorization code or a refresh token.  If an auth code is provided,
+// a refresh token will be retrieved as well.  A new callback is supplied by the

[Wez, 2015/02/13 03:01:51]

What does the fetching of the refresh token actually mean to the caller? Surely
"if an auth code is provided" translates to "if GetAccessTokenFromAuthCode is
used", i.e. this particular detail belongs on that API, not on the class?

[joedow, 2015/02/14 02:31:25]

Done.  Removed the comment since the tokens returned is tied to the callback
signature rather than an aspect of this class.

+// client for each network call which will return a valid token on success or

[Wez, 2015/02/13 03:01:51]

What network calls?

[joedow, 2015/02/14 02:31:25]

Done.

+// an empty token on failure.  This callback will not be called back once the
+// AccessTokenFetcher instance has been destructed and it is intended to be safe
+// to destruct the AccessTokenFetcher instance within the callback if needed.

[Wez, 2015/02/13 03:01:51]

This could be expressed more succinctly, e.g. "Destroying the AccessTokenFetcher
while a request is outstanding will cancel the request. It is safe to delete the
fetcher from within a completion callback."

[joedow, 2015/02/14 02:31:26]

Done.

+// This class requires a valid IO MessageLoop on the thread it runs on as it
+// uses the current thread's task runner to make its network requests.

[Wez, 2015/02/13 03:01:51]

Again, this could be shorter, e.g. "Must be used from a thread running an IO
message loop."

[joedow, 2015/02/14 02:31:25]

Done.

+class AccessTokenFetcher : public gaia::GaiaOAuthClient::Delegate {
+ public:
+  AccessTokenFetcher();
+  ~AccessTokenFetcher() override;
+
+  // Retrieve an access token from a one time use authorization code.
+  // Method is marked virtual to allow for mocking.

[Wez, 2015/02/13 03:01:51]

The comments re marking virtual feel like they'd make more sense as class-level
comments explaining the virtual tags on the public & protected methods.

What code is going to call AccessTokenFetcher, such that it's worth mocking it?
(Sorry; it's been a while so I'm sure I've lost some context...)

[joedow, 2015/02/14 02:31:25]

Done.

+  virtual void GetAccessTokenFromAuthCode(const std::string& auth_code,
+      const AccessTokenCallback& callback);
+
+  // Retrieve an access token from a refresh token.
+  // Method is marked virtual to allow for mocking.
+  virtual void GetAccessTokenFromRefreshToken(const std::string& refresh_token,
+      const AccessTokenCallback& callback);
+
+ protected:
+  // Updates |auth_client_| with a new GaiaOAuthClient instance.
+  virtual void RefreshGaiaOAuthClientInstance();

[Wez, 2015/02/13 03:01:51]

Why is this also virtual? For mocking?

"Refresh" in the name implies that the client may be re-used across calls, but I
don't think that's what you actually mean here?

[joedow, 2015/02/14 02:31:25]

Done.

+
+  // Used to make token requests to GAIA.  Uses a request_context_getter
+  // to make those requests on the current thread's task runner.

[Wez, 2015/02/13 03:01:51]

As above.

[joedow, 2015/02/14 02:31:25]

Done.

+  scoped_ptr<GaiaOAuthClientAdapter> auth_client_;
+
+ private:
+  // gaia::GaiaOAuthClient::Delegate Interface.
+  void OnGetTokensResponse(
+      const std::string& refresh_token,
+      const std::string& access_token,
+      int expires_in_seconds) override;
+  void OnRefreshTokenResponse(
+      const std::string& access_token,
+      int expires_in_seconds) override;
+  void OnGetUserEmailResponse(const std::string& user_email) override;
+  void OnGetUserIdResponse(const std::string& user_id) override;
+  void OnGetUserInfoResponse(
+      scoped_ptr<base::DictionaryValue> user_info) override;
+  void OnGetTokenInfoResponse(
+      scoped_ptr<base::DictionaryValue> token_info) override;
+  void OnOAuthError() override;
+  void OnNetworkError(int response_code) override;
+
+  // Validates the current access token and either returns it via the success
+  // callback or clears the token and calls the failure callback.
+  void ValidateAccessToken();

[Wez, 2015/02/13 03:01:51]

ValidateAccessTokenAndNotifyCallback, or simply NotifyCallback?

[joedow, 2015/02/14 02:31:26]

Cleaned up the comment since the function doesn't actually call the callback,
the GaiaOAuthClient::Delegate methods call the callback.

+
+  // Caller-supplied callback used to return valid tokens on success or an
+  // empty access token if a failure occurred.  The refresh token may or may not
+  // be empty on failure.

[Wez, 2015/02/13 03:01:51]

Why would the refresh token not be empty on failure?

[joedow, 2015/02/14 02:31:26]

Comment was out of date, it is empty now.

+  AccessTokenCallback access_token_callback_;
+
+  // The access token retrieved based on the |refresh_token_|.
+  std::string access_token_;

[Wez, 2015/02/13 03:01:51]

Why do you cache the access token in this class if you never return the same
access token between consecutive calls?

[joedow, 2015/02/14 02:31:26]

I make a validate token call after I receive the access token (which is
suggested in the OAuth doc) and the data returned from the validate call does
not include the original access token.  So I squirrel away the token to return
to the caller after the validate call succeeds.

[Wez, 2015/02/19 22:00:22]

OK; it wasn't clear why you needed to store it rather than Bind()ing it into a
completion callback for the validate operation. I think it'd make sense to
clarify why we need this member, e.g.

// Used to store the access-token while the GetTokenInfo request to verify it is
in-progress.

+
+  // Refresh token supplied by the caller or retrieved from a caller-supplied

[Wez, 2015/02/13 03:01:50]

nit: No need for "Refresh token"

[joedow, 2015/02/14 02:31:26]

Done.

+  // auth token.
+  std::string refresh_token_;
+
+  // Object which holds the client id, secret, and redirect url used to make

[Wez, 2015/02/13 03:01:50]

nit: No need for "Object which"

[joedow, 2015/02/14 02:31:25]

Done.

+  // the Gaia service request.
+  gaia::OAuthClientInfo oauth_client_info_;
+
+  DISALLOW_COPY_AND_ASSIGN(AccessTokenFetcher);
+};
+
+}  // namespace test
+}  // namespace remoting
+
+#endif  // REMOTING_TEST_ACCESS_TOKEN_FETCHER_H_