Adding the AccessTokenFetcher and Environment class to the app remoting test

remoting/test/access_token_fetcher.cc

Index: remoting/test/access_token_fetcher.cc
diff --git a/remoting/test/access_token_fetcher.cc b/remoting/test/access_token_fetcher.cc
new file mode 100644
index 0000000000000000000000000000000000000000..ba99c42fb7ef878e0951489f2d780643fa08dbf0
--- /dev/null
+++ b/remoting/test/access_token_fetcher.cc
@@ -0,0 +1,256 @@
+// 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.
+
+#include "remoting/test/access_token_fetcher.h"
+
+#include "base/bind.h"
+#include "base/logging.h"
+#include "base/message_loop/message_loop.h"
+#include "base/thread_task_runner_handle.h"
+#include "google_apis/gaia/gaia_constants.h"
+#include "google_apis/google_api_keys.h"
+#include "net/url_request/url_fetcher.h"
+#include "remoting/base/url_request_context_getter.h"
+
+namespace {
+const int kMaxGetTokensRetries = 3;
+const char kOauthRedirectUrl[] = "https://chromoting-oauth.talkgadget."
+    "google.com/talkgadget/oauth/chrome-remote-desktop/dev";
+}
+
+namespace remoting {
+namespace test {
+
+using net::URLRequestContextGetter;

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

nit: We rarely use using, and it seems strange to use it here to save only 5
chars per usage; is it really used that much in this file to be worth this? It's
also confusing because it means that the use of net::URLRequestContextGetter is
unqualified, while use of the remoting:: version is qualified...

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

Done.

+
+GaiaOAuthClientAdapter::GaiaOAuthClientAdapter(
+    URLRequestContextGetter* context_getter) {
+  auth_client_.reset(new gaia::GaiaOAuthClient(context_getter));
+}
+
+GaiaOAuthClientAdapter::~GaiaOAuthClientAdapter() {
+}

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

nit: Run git cl format on this; I think it'll put the {} on one line.

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

Done.

+
+void GaiaOAuthClientAdapter::GetTokensFromAuthCode(
+    const gaia::OAuthClientInfo& oauth_client_info,
+    const std::string& auth_code,
+    int max_retries,
+    gaia::GaiaOAuthClient::Delegate* delegate) {
+  DCHECK(auth_client_.get());

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

You create |auth_client_| in the ctor, so no need to DCHECK it here - client
can't get here without calling the ctor!

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

Done.

+  auth_client_->GetTokensFromAuthCode(
+      oauth_client_info,
+      auth_code,
+      max_retries,
+      delegate);
+}
+
+void GaiaOAuthClientAdapter::RefreshToken(
+    const gaia::OAuthClientInfo& oauth_client_info,
+    const std::string& refresh_token,
+    const std::vector<std::string>& scopes,
+    int max_retries,
+    gaia::GaiaOAuthClient::Delegate* delegate) {
+  DCHECK(auth_client_.get());

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

As above

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

Done.

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

Done.

+  auth_client_->RefreshToken(
+      oauth_client_info,
+      refresh_token,
+      scopes,
+      max_retries,
+      delegate);
+}
+
+void GaiaOAuthClientAdapter::GetTokenInfo(
+    const std::string& oauth_access_token,
+    int max_retries,
+    gaia::GaiaOAuthClient::Delegate* delegate) {
+  DCHECK(auth_client_.get());

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

As above

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

Done.

+  auth_client_->GetTokenInfo(
+      oauth_access_token,
+      max_retries,
+      delegate);
+}
+
+AccessTokenFetcher::AccessTokenFetcher() {
+  oauth_client_info_ = {
+      google_apis::GetOAuth2ClientID(google_apis::CLIENT_REMOTING),
+      google_apis::GetOAuth2ClientSecret(google_apis::CLIENT_REMOTING),
+      kOauthRedirectUrl
+  };
+}
+
+AccessTokenFetcher::~AccessTokenFetcher() {
+}
+
+void AccessTokenFetcher::GetAccessTokenFromAuthCode(
+    const std::string& auth_code,
+    const AccessTokenCallback& callback) {
+  DVLOG(2) << "Calling GetTokensFromAuthCode to exchange auth_code for token";

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

You're logging this log before actually making the call; it'll be logged even if
you early-exit - consider moving it down to just before the call

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

Done.

+
+  if (!access_token_callback_.is_null()) {
+    LOG(ERROR) << "Access token request is already in progress";
+    return;
+  }
+
+  DCHECK(access_token_.empty());

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

You DCHECK that |access_token_| is empty here but in
GetAccessTokenFromRefreshToken you clear it instead - why?

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

I had assumed that this function would only be called once and that a second
call would be an error.  I'm updating the logic though as there isn't a business
reason why someone couldn't call this twice with different auth codes.

+  DCHECK(refresh_token_.empty());
+  access_token_callback_ = callback;
+
+  // Every call which uses a GaiaOAuthClient requires a new object.

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

Not sure what this means; are you saying that you need a fresh GaiaOAuthClient
for each request?

If so then I'd suggest changing RefreshGaiaOAuthClientInstance to
scoped_ptr<GaiaOAuthClient> CreateGaiaOAuthClient() and DCHECKing at the start
of this method that |auth_client_| is null. You'd then reset() the
|auth_client_| member in each of the response handler methods.

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

Updated the comment to be more clear.  I don't want to pass a scoped_ptr as the
scope of the auth_client_ object is longer than the function call.

+  RefreshGaiaOAuthClientInstance();
+  auth_client_->GetTokensFromAuthCode(
+      oauth_client_info_,
+      auth_code,
+      kMaxGetTokensRetries,
+      this);  // GaiaOAuthClient::Delegate* delegate
+}
+
+void AccessTokenFetcher::GetAccessTokenFromRefreshToken(
+    const std::string& refresh_token,
+    const AccessTokenCallback& callback) {
+  DVLOG(2) << "Calling RefreshToken to generate a new access token";
+
+  if (!access_token_callback_.is_null()) {
+    LOG(ERROR) << "Access token request is already in progress";
+    return;
+  }
+
+  access_token_.clear();

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

See comments above re DCHECKing vs clear()ing.

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

Done.

+  refresh_token_ = refresh_token;
+  access_token_callback_ = callback;
+
+  // Populate a vector with the required permissions for app remoting.
+  std::vector<std::string> scopes;
+  scopes.push_back(
+      "https://www.googleapis.com/auth/appremoting.runapplication");
+  scopes.push_back("https://www.googleapis.com/auth/googletalk");
+  scopes.push_back("https://www.googleapis.com/auth/userinfo.email");
+  scopes.push_back("https://docs.google.com/feeds");
+  scopes.push_back("https://www.googleapis.com/auth/drive");

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

Is there a way that you can initialize the vector<string> from a literal array,
so you can move these up to the anonymous namespace at the top, alongside the
other constants?

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

Done.

+
+  // Every call which uses a GaiaOAuthClient requires a new object.
+  RefreshGaiaOAuthClientInstance();
+  auth_client_->RefreshToken(
+      oauth_client_info_,
+      refresh_token_,
+      scopes,
+      kMaxGetTokensRetries,
+      this);  // GaiaOAuthClient::Delegate* delegate
+}
+
+void AccessTokenFetcher::RefreshGaiaOAuthClientInstance() {
+  scoped_refptr<remoting::URLRequestContextGetter> request_context_getter;
+  request_context_getter = new remoting::URLRequestContextGetter(
+      base::ThreadTaskRunnerHandle::Get(),   // network_runner
+      base::ThreadTaskRunnerHandle::Get());  // file_runner
+
+  auth_client_.reset(new GaiaOAuthClientAdapter(request_context_getter.get()));
+}
+
+void AccessTokenFetcher::OnGetTokensResponse(
+    const std::string& refresh_token,
+    const std::string& access_token,
+    int expires_in_seconds) {
+  DVLOG(1) << "AccessTokenFetcher::OnGetTokensResponse() Called";
+  DVLOG(1) << "--refresh_token: " << refresh_token;
+  DVLOG(1) << "--access_token: " << access_token;
+  DVLOG(1) << "--expires_in_seconds: " << expires_in_seconds;
+
+  refresh_token_ = refresh_token;
+  access_token_ = access_token;
+
+  ValidateAccessToken();
+}
+
+void AccessTokenFetcher::OnRefreshTokenResponse(
+    const std::string& access_token,
+    int expires_in_seconds) {
+  DVLOG(1) << "AccessTokenFetcher::OnRefreshTokenResponse() Called";
+  DVLOG(1) << "--access_token: " << access_token;
+  DVLOG(1) << "--expires_in_seconds: " << expires_in_seconds;
+
+  access_token_ = access_token;
+
+  ValidateAccessToken();
+}
+
+void AccessTokenFetcher::OnGetUserEmailResponse(
+  const std::string& user_email) {
+  // This callback should not be called as we do not request the user's email.
+  NOTREACHED();
+}
+
+void AccessTokenFetcher::OnGetUserIdResponse(
+  const std::string& user_id) {
+  // This callback should not be called as we do not request the user's id.
+  NOTREACHED();
+}
+
+void AccessTokenFetcher::OnGetUserInfoResponse(
+    scoped_ptr<base::DictionaryValue> user_info) {
+  // This callback should not be called as we do not request user info.
+  NOTREACHED();
+}
+
+void AccessTokenFetcher::OnGetTokenInfoResponse(
+    scoped_ptr<base::DictionaryValue> token_info) {
+  DVLOG(1) << "AccessTokenFetcher::OnGetTokenInfoResponse() Called";
+
+  std::string error_string;
+  std::string error_description;
+
+  // Check to see if the token_info we received had any errors,
+  // otherwise we will assume that it is valid for our purposes.
+  if (token_info->HasKey("error")) {
+    token_info->GetString("error", &error_string);
+    token_info->GetString("error_description", &error_description);
+
+    LOG(ERROR) << "OnGetTokenInfoResponse returned an error. " << ", "
+               << "error: " << error_string << ", "
+               << "description: " << error_description;
+    access_token_.clear();
+    refresh_token_.clear();
+  } else {
+    DVLOG(1) << "Access Token has been validated";
+  }
+
+  DCHECK(!access_token_callback_.is_null());

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

There's no point DCHECKing this here; the Run() would crash if the callback is
null - you want to DCHECK at the point where this operation actually starts, or
at the point where the caller supplies the callback.

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

Done.

+  access_token_callback_.Run(access_token_, refresh_token_);
+  access_token_callback_.Reset();
+}
+
+void AccessTokenFetcher::OnOAuthError() {
+  LOG(ERROR) << "AccessTokenFetcher::OnOAuthError() Called";
+
+  access_token_.clear();
+  refresh_token_.clear();
+
+  DCHECK(!access_token_callback_.is_null());
+  access_token_callback_.Run(access_token_, refresh_token_);
+  access_token_callback_.Reset();
+}
+
+void AccessTokenFetcher::OnNetworkError(int response_code) {
+  LOG(ERROR) << "AccessTokenFetcher::OnNetworkError() Called";
+  LOG(ERROR) << "response code: " << response_code;
+
+  access_token_.clear();
+  refresh_token_.clear();
+
+  DCHECK(!access_token_callback_.is_null());

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

See above re DCHECK

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

Done.

+  access_token_callback_.Run(access_token_, refresh_token_);
+  access_token_callback_.Reset();
+}
+
+void AccessTokenFetcher::ValidateAccessToken() {
+  DVLOG(2) << "Calling GetTokenInfo to validate access token";
+
+  RefreshGaiaOAuthClientInstance();
+  auth_client_->GetTokenInfo(
+      access_token_,
+      kMaxGetTokensRetries,
+      this);  // GaiaOAuthClient::Delegate* delegate
+}
+
+}  // namespace test
+}  // namespace remoting