Signin/OAuth: Create an AccessTokenFetcher helper class

components/signin/core/browser/access_token_fetcher.cc

Index: components/signin/core/browser/access_token_fetcher.cc
diff --git a/components/signin/core/browser/access_token_fetcher.cc b/components/signin/core/browser/access_token_fetcher.cc
new file mode 100644
index 0000000000000000000000000000000000000000..b33471624e3ad37997230b85467c832f85a152f9
--- /dev/null
+++ b/components/signin/core/browser/access_token_fetcher.cc
@@ -0,0 +1,155 @@
+// Copyright 2017 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 "components/signin/core/browser/access_token_fetcher.h"
+
+#include <utility>
+
+#include "base/logging.h"
+#include "google_apis/gaia/google_service_auth_error.h"
+
+AccessTokenFetcher::AccessTokenFetcher(
+    const std::string& oauth_consumer_name,
+    SigninManagerBase* signin_manager,
+    OAuth2TokenService* token_service,
+    const OAuth2TokenService::ScopeSet& scopes,
+    TokenCallback callback)
+    : OAuth2TokenService::Consumer(oauth_consumer_name),
+      signin_manager_(signin_manager),
+      token_service_(token_service),
+      scopes_(scopes),
+      callback_(std::move(callback)),
+      waiting_for_sign_in_(false),
+      waiting_for_refresh_token_(false),
+      access_token_retried_(false) {
+  Start();
+}
+
+AccessTokenFetcher::~AccessTokenFetcher() {
+  if (waiting_for_sign_in_) {
+    signin_manager_->RemoveObserver(this);
+  }
+  if (waiting_for_refresh_token_) {
+    token_service_->RemoveObserver(this);
+  }
+}
+
+void AccessTokenFetcher::Start() {
+  if (signin_manager_->IsAuthenticated()) {
+    // Already signed in: Make sure we have a refresh token, then get the access
+    // token.
+    WaitForRefreshToken();
+  } else {
+    // Not signed in: Wait for a sign-in to complete (to get the refresh token),
+    // then get the access token.
+    DCHECK(!waiting_for_sign_in_);
+    waiting_for_sign_in_ = true;
+    signin_manager_->AddObserver(this);
+  }
+}
+
+void AccessTokenFetcher::WaitForRefreshToken() {
+  DCHECK(signin_manager_->IsAuthenticated());
+  DCHECK(!waiting_for_refresh_token_);
+
+  if (token_service_->RefreshTokenIsAvailable(
+          signin_manager_->GetAuthenticatedAccountId())) {
+    // Already have refresh token: Get the access token directly.
+    StartAccessTokenRequest();

[msarda, 2017/01/25 09:54:29]

Optional nit: Let's use an early return here and remove the else branch.

[Marc Treib, 2017/01/25 11:37:04]

Done. Also done similarly in Start() above.

+  } else {
+    // Signed in, but refresh token isn't there yet: Wait for the refresh
+    // token to be loaded, then get the access token.
+    waiting_for_refresh_token_ = true;
+    token_service_->AddObserver(this);
+  }
+}
+
+void AccessTokenFetcher::StartAccessTokenRequest() {

[msarda, 2017/01/25 09:54:29]

Let's add a DCHECK that this method is called only when the token service has a
refresh token for the authenticated account id. Let will allow us to catch cases
when this method is called in an unexpected state.
WDYT?

[Marc Treib, 2017/01/25 11:37:04]

Yup, sounds reasonable. Done.

+  access_token_request_ = token_service_->StartRequest(
+      signin_manager_->GetAuthenticatedAccountId(), scopes_, this);
+}
+
+void AccessTokenFetcher::GoogleSigninSucceeded(const std::string& account_id,
+                                               const std::string& username,
+                                               const std::string& password) {
+  DCHECK(waiting_for_sign_in_);
+  DCHECK(!waiting_for_refresh_token_);
+  waiting_for_sign_in_ = false;
+  signin_manager_->RemoveObserver(this);
+
+  DCHECK(signin_manager_->IsAuthenticated());

[msarda, 2017/01/25 09:54:29]

Nit: Move this DCHECK in the group of DCHECKs above.

[Marc Treib, 2017/01/25 11:37:04]

Done.

+
+  WaitForRefreshToken();
+}
+
+void AccessTokenFetcher::GoogleSigninFailed(
+    const GoogleServiceAuthError& error) {
+  DCHECK(waiting_for_sign_in_);
+  DCHECK(!waiting_for_refresh_token_);
+  waiting_for_sign_in_ = false;
+  signin_manager_->RemoveObserver(this);
+
+  std::move(callback_).Run(std::string());
+}
+
+void AccessTokenFetcher::OnRefreshTokenAvailable(
+    const std::string& account_id) {
+  DCHECK(waiting_for_refresh_token_);
+  DCHECK(!waiting_for_sign_in_);
+
+  // Only react on tokens for the account the user has signed in with.
+  if (account_id != signin_manager_->GetAuthenticatedAccountId()) {
+    return;
+  }
+
+  waiting_for_refresh_token_ = false;
+  token_service_->RemoveObserver(this);
+  StartAccessTokenRequest();
+}
+
+void AccessTokenFetcher::OnRefreshTokensLoaded() {
+  DCHECK(waiting_for_refresh_token_);
+  DCHECK(!waiting_for_sign_in_);
+
+  // All refresh tokens were loaded, but we didn't get one for the account we
+  // care about. We probably won't get one any time soon.
+  waiting_for_refresh_token_ = false;
+  token_service_->RemoveObserver(this);
+  std::move(callback_).Run(std::string());
+}
+
+void AccessTokenFetcher::OnGetTokenSuccess(
+    const OAuth2TokenService::Request* request,
+    const std::string& access_token,
+    const base::Time& expiration_time) {
+  DCHECK_EQ(request, access_token_request_.get());
+  std::unique_ptr<OAuth2TokenService::Request> request_deleter(
+      std::move(access_token_request_));
+
+  std::move(callback_).Run(access_token);
+}
+
+void AccessTokenFetcher::OnGetTokenFailure(
+    const OAuth2TokenService::Request* request,
+    const GoogleServiceAuthError& error) {
+  DCHECK_EQ(request, access_token_request_.get());
+  std::unique_ptr<OAuth2TokenService::Request> request_deleter(
+      std::move(access_token_request_));
+
+  if (!access_token_retried_ &&
+      error.state() == GoogleServiceAuthError::State::REQUEST_CANCELED) {

[msarda, 2017/01/25 09:54:29]

I think this case is a bit more tricky:
1. If the user signs out when we're waiting for an access token (the token was
revoked), then all the existing access token requests will be cancelled (see
https://cs.chromium.org/chromium/src/components/signin/core/browser/profile_o...).
In that case, I do not think it is a good idea to start a new access token
request.
2. If the user re authenticates (the OnRefreshTokenAvailable case), then all the
existing access token requests will be cancelled (this matches your comment). In
that case we would like to start a new access token request.

The problem is how to distinguish between the 2 cases. I would say that what we
could do here would be to start an access token request ONLY IF
RefreshTokenAvailable(authenticated acount id) return true (this basically
matches the case when the requests were cancelled after a
OnRefreshTokenAvailable method was called).

[Marc Treib, 2017/01/25 11:37:04]

Yup, that sounds very reasonable!
I guess it doesn't cause any big harm to retry if the refresh token isn't there
anymore, but let's try to keep the hacky workaround as contained as possible.
I've updated the comment to explain this, please sanity check :)

I've also added tests to make sure we don't retry in those cases.

+    // There is a special case for Android that RefreshTokenIsAvailable and
+    // StartRequest are called to pre-fetch the account image and name before
+    // sign-in. In that case, our ongoing access token request gets cancelled.
+    // Moreover, OnRefreshTokenAvailable might happen after startup when the
+    // credentials are changed/updated.
+    // To handle these cases, we retry a canceled request once.
+    access_token_retried_ = true;
+    StartAccessTokenRequest();
+    return;
+  }
+
+  DLOG(WARNING) << "Unable to get token: " << error.ToString();
+  std::move(callback_).Run(std::string());
+}