Signin/OAuth: Create an AccessTokenFetcher helper class

components/signin/core/browser/access_token_fetcher.cc

+// 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"
+
+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();
+    return;
+  }
+
+  // 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();
+    return;
+  }
+
+  // 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/27 10:38:58]

Please add a DCHECK(!access_token_request_) to catch cases when we call
StartAccessTokenRequest multiple times without waiting for previous calls to
end.

[Marc Treib, 2017/01/27 11:16:55]

Done.

+  DCHECK(token_service_->RefreshTokenIsAvailable(
+      signin_manager_->GetAuthenticatedAccountId()));
+  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_);
+  DCHECK(signin_manager_->IsAuthenticated());
+  waiting_for_sign_in_ = false;
+  signin_manager_->RemoveObserver(this);
+
+  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(
+      GoogleServiceAuthError(GoogleServiceAuthError::INVALID_GAIA_CREDENTIALS),

[msarda, 2017/01/27 10:38:58]

I suppose here we should pass the error |error| to the callback (and not
INVALID_GAIA_CREDENTIALS).

[Marc Treib, 2017/01/27 11:16:55]

D'oh. Done.

+      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(

[msarda, 2017/01/27 10:38:58]

I think calling here the callback with INVALID_GAIA_CRENDENTIALS is not correct.
If there is a refresh token for the authenticated account id, then
OnRefreshTokenAvailable(authenticated_account_id) was already called. This means
that a  StartAccessTokenRequest was fired, but it may not have returned with an
access token. Is at this point  OnRefreshTokensLoaded is called, then the
callback will be called with INVALID_GAIA_CREDENTIALS which is incorrect.

I see 2 options here:
1. As long as the tokens were not loaded, we ignore the calls to
OnRefreshTokenAvailable. We only call StartAccessTokenRequest when the tokens
are loaded if there is refresh token available for the authenticated account id.
or
2. OnRefreshTokensLoaded we call the callback with INVALID_GAIA_CREDENTIALS only
if there is no refresh token available for the authenticated account id.

[Marc Treib, 2017/01/27 11:16:55]

But if OnRefreshTokenAvailable(authenticated_account_id) was already called,
then we have unregistered ourselves as an observer already, so we should never
get the OnRefreshTokensLoaded call. I've added a
"DCHECK(!access_token_request_)" to make sure.
IIRC, there isn't any way to know if the tokens were already loaded earlier, so
we don't know if we should wait for that call or not.
Per the explanation above, I think this is what already happens.

[msarda, 2017/01/27 12:35:37]

I missed that part where the this object removes itself from the list of
observers of the token service. Thank you for explaining that.

+      GoogleServiceAuthError(GoogleServiceAuthError::INVALID_GAIA_CREDENTIALS),
+      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(GoogleServiceAuthError::AuthErrorNone(),
+                           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_));
+
+  // 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.
+  // However, a request may also get cancelled for legitimate reasons, e.g.
+  // because the user signed out. In those cases, there's no point in retrying,
+  // so only retry if there (still) is a valid refresh token.
+  // TODO(treib): Should we check error.IsTransientError() instead of explicit

[msarda, 2017/01/27 10:38:58]

I think we're defending against a corner case when requests are cancelled. Thus
I would say that we should not use error.IsTransientError() here. Consider
removing this TODO.

[Marc Treib, 2017/01/27 11:16:55]

A comment in google_service_auth_error.cc says "These are failures that are
likely to succeed if tried again". That sounds like a thing we might want to
retry here - I definitely want to avoid a situations where clients of this class
have to retry. All that ugliness should be handled in here.

OTOH, it feels wrong to add handling code for edge cases that we're not even
sure exist. Hence the TODO as a trade-off. WDYT?

[msarda, 2017/01/27 12:35:37]

Do you plan to do something towards fixing this TODO? If so, what? If not, then
I'd say it should be removed (we have already too many TODOs in the code).

[Marc Treib, 2017/01/27 13:16:38]

That's a good point - it's a TODO without concrete plans to do anything about
it. While have plenty of precedent for that, let's try not to add more :)

But I do want to document that I'm not confident we're doing the right thing
here. It seems reasonable to retry on a transient error, and I definitely want
to keep any retrying in here, and not leave it to clients who will get it wrong.
Would it be better to leave the comment in, just without the TODO tag?

+  // checking for REQUEST_CANCELED?
+  if (!access_token_retried_ &&
+      error.state() == GoogleServiceAuthError::State::REQUEST_CANCELED &&
+      token_service_->RefreshTokenIsAvailable(

[msarda, 2017/01/27 10:38:58]

I was thinking that here we may want to always retry when the existing requests
were cancelled, but there is a refresh token is still available. Why should we
only retry once?

I think that would simplify the code a bit.

[Marc Treib, 2017/01/27 11:16:55]

An unbounded retry loop is a recipe for DDOSing ourselves. I'm speaking from
experience here ;)

IMO if we retry multiple times, then we have to add backoff, which would make
the code more complicated. I'd say, as long as we don't know of any situation
that require multiple retries, we should stay with this.

[msarda, 2017/01/27 12:35:37]

Ok, I am convinced now that your approach is the right one - we should avoid
DDOsing GAIA :)

+          signin_manager_->GetAuthenticatedAccountId())) {
+    access_token_retried_ = true;
+    StartAccessTokenRequest();
+    return;
+  }
+
+  std::move(callback_).Run(error, std::string());
+}