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"
+#include "base/threading/thread_task_runner_handle.h"
+#include "components/signin/core/browser/signin_manager_base.h"
+#include "google_apis/gaia/google_service_auth_error.h"
+
+AccessTokenFetcher::AccessTokenFetcher(
+    const std::string& oauth_consumer_name,
+    const SigninManagerBase* signin_manager,
+    OAuth2TokenService* token_service,
+    const OAuth2TokenService::ScopeSet& scopes,
+    const TokenCallback& callback)
+    : OAuth2TokenService::Consumer(oauth_consumer_name),
+      signin_manager_(signin_manager),
+      token_service_(token_service),
+      scopes_(scopes),
+      callback_(callback),
+      weak_ptr_factory_(this) {
+  Start();
+}
+
+AccessTokenFetcher::~AccessTokenFetcher() {
+  if (waiting_for_refresh_token_) {
+    token_service_->RemoveObserver(this);
+  }
+}
+
+void AccessTokenFetcher::Start() {
+  if (signin_manager_->IsAuthenticated() &&
+      token_service_->RefreshTokenIsAvailable(
+          signin_manager_->GetAuthenticatedAccountId())) {
+    // Already have refresh token: Get the access token directly.
+    StartAccessTokenRequest();
+  } else if (signin_manager_->IsAuthenticated() ||
+             signin_manager_->AuthInProgress()) {
+    // Currently signing in, or signed in but refresh token isn't there yet:
+    // Wait for auth to finish (to get the refresh token), then get the access
+    // token.
+    // TODO(treib): In the AuthInProgress case, should we wait for
+    // GoogleSigninSucceeded/Failed (from SigninManagerBase::Observer) first?

[msarda, 2017/01/18 21:55:19]

I agree this is a tricky question: currently the refresh token is updated after
the authenticated account id is set - this means that GoogleSigninSucceeded is
always fired before a OnRefreshTokenAvailable. So at the current state listening
for GoogleSigninSucceded may not be needed.

However, if the sign-in fails, then the OnRefreshTokenAvailable will not be
called, which in turn means that you will not call the callback if you are not
listening for GoogleSigninFailed.

So I would say you would probably need to wait for GoogleSigninSucceeded/Failed
as well.

[Marc Treib, 2017/01/19 15:03:55]

Done.
However, I'm still not super confident that this reliably handles all cases now.
I guess the only "safe" option would be to also add a timeout to the whole
thing... not sure that's worth the hassle.

[msarda, 2017/01/23 17:08:09]

I think using a timeout is not a good API.

This comment made me think that maybe we need a way to cancel the request (in
the current implementation that is actually handled by the constructor as it
unregisters from the token service, but the callback is not called in that
case). We need to be explicit about this (explain this in the comments on the
header file).

[Marc Treib, 2017/01/24 10:17:32]

Not calling the callback after we're destroyed is on purpose: The Fetcher *is*
the request, you cancel by destroying the fetcher. I find that convenient for
clients: They can have the Fetcher as a member (and bind the callback to a
method), and if they're destroyed, they don't have to worry about canceling.

The comment on the ctor in the header already mentions that the callback won't
be called after destroying the instance. Any suggestions on how to make that
clearer?

+    DCHECK(!waiting_for_refresh_token_);
+    waiting_for_refresh_token_ = true;
+    token_service_->AddObserver(this);
+  } else {
+    // Not signed in, no access token. Make sure not to run the callback
+    // synchronously, and not to run it if we get deleted in the meantime.
+    base::ThreadTaskRunnerHandle::Get()->PostTask(

[msarda, 2017/01/18 21:55:19]

It is not clear to me how you would use this API: would you expect the caller to
create an AccessTokenFetcher when the user is not authenticated (even when auth
is not even in progress?)? I mean in that case (which is a synchronous call by
the way that always ends in error), wouldn't it be easy to say the caller should
never create an AccessTokenFetcher if the profiles is not authenticated and
remove this if branch?

[Marc Treib, 2017/01/19 15:03:55]

There are some (future) clients of this class that can work in authenticated or
non-authenticated mode (but prefer authenticated of course). IMO it's convenient
for them to just say "get me an access token", and see if one comes back, rather
than checking IsAuthenticated/AuthInProgress/whatever else manually.
I could probably be convinced otherwise though :)

+        FROM_HERE, base::Bind(&AccessTokenFetcher::NotSignedInCallback,
+                              weak_ptr_factory_.GetWeakPtr()));
+  }
+}
+
+void AccessTokenFetcher::NotSignedInCallback() {
+  callback_.Run(std::string());

[msarda, 2017/01/18 21:55:19]

We should to pass an error to the callback  here as otherwise it will be
impossible to figure out on the caller what went wrong when requesting an access
token.

[msarda, 2017/01/18 21:55:19]

We should use a callback that may be called only once (see
https://cs.chromium.org/chromium/src/base/callback_forward.h?rcl=0&l=36 )

[Marc Treib, 2017/01/19 15:03:55]

I actually did use OnceCallback in a previous iteration. The issue is that it's
very new and not widely supported yet, e.g. all the PostTask methods only take
RepeatingCallbacks.
If it turns out that we can get away without PostTask etc, then I'm happy to
switch back to OnceCallback.

[Marc Treib, 2017/01/19 15:03:55]

So far, none of the callers I'm aware of care about what exactly went wrong, and
I don't see what they would do about it anyway (log an error message
somewhere?). So I went the lazy route :)
But maybe it's the nicer API anyway to return some sort of error code?

[msarda, 2017/01/23 17:08:09]

I think the current API is fine for discussion, but we need to add support for
errors before this CL can be landed.

+}
+
+void AccessTokenFetcher::OnRefreshTokenAvailable(
+    const std::string& account_id) {
+  DCHECK(waiting_for_refresh_token_);

[msarda, 2017/01/18 21:55:19]

Just to make sure what I said in my previous comment stays true: please add a
DCHECK(signin_manager_->IsAuthenticated()) here.

[Marc Treib, 2017/01/19 15:03:55]

Done.

+
+  // Only react on tokens for the account the user has signed in with.
+  if (account_id != signin_manager_->GetAuthenticatedAccountId()) {
+    return;
+  }
+

[msarda, 2017/01/18 21:55:18]

Nit: I would set waiting_for_refresh_token_ before calling
token_service_->RemoveObserver().

[Marc Treib, 2017/01/19 15:03:55]

Done.

+  token_service_->RemoveObserver(this);
+  waiting_for_refresh_token_ = false;
+  StartAccessTokenRequest();
+}
+
+void AccessTokenFetcher::OnRefreshTokensLoaded() {
+  DCHECK(waiting_for_refresh_token_);
+
+  // 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.
+  token_service_->RemoveObserver(this);
+  waiting_for_refresh_token_ = false;
+  callback_.Run(std::string());
+}
+
+void AccessTokenFetcher::StartAccessTokenRequest() {
+  access_token_request_ = token_service_->StartRequest(
+      signin_manager_->GetAuthenticatedAccountId(), scopes_, this);
+}
+
+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_));
+
+  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) {
+    // The request can get reset by loading the refresh token (happens during
+    // startup) - try one more time.
+    access_token_retried_ = true;
+    StartAccessTokenRequest();

[msarda, 2017/01/18 21:55:19]

It is not clear to me yet why we need to retry. I have not seen a similar
usecase of the access token API.

I think the fact that we need to always retry on a legitimate error response a
bad practice and we should fix the API to avoid such hacks in the code.

[Marc Treib, 2017/01/19 15:03:55]

We had a case on Android where sometimes during startup, the first request would
get canceled. IIRC, it had something to do with the refresh token getting
reloaded.
Agreed it's a hack, but at the time, nobody could figure out a nicer solution.
+jrkcal, do you remember more details?

[jkrcal, 2017/01/19 15:48:45]

I agree it is ugly. Reasoning for the retry:
https://groups.google.com/a/google.com/d/msg/zine-eng/TqMf2u1Pn9Y/1EelPl3XAQAJ

[msarda, 2017/01/23 17:08:09]

Thank you for the pointer. I think we should not do the retry for all fetchers
on all platforms, just because we have a limited use case on Android.

I think this should at least be scoped to Android OS, with a comment and a link
to this thread.

[Marc Treib, 2017/01/24 10:17:32]

The thread is not public unfortunately, so we can't link to it from Chromium.
But I've tried to copy the relevant parts into a comment here.
It sounds to me like one of the cases where a retry is required can happen on
non-Android too (credentials updated), so I haven't put in "#ifdef ANDROID"s.

+    return;
+  }
+
+  DLOG(WARNING) << "Unable to get token: " << error.ToString();
+  callback_.Run(std::string());

[msarda, 2017/01/18 21:55:18]

We need an API that would allow us to pass the error to the caller - the caller
should decide whether it needs to retry, use backoff or something else.

[Marc Treib, 2017/01/24 10:17:32]

I'd actually prefer if any retrying were handled within this class, so that
clients don't have to worry about that.
In the past, there were multiple instances where clients implemented retrying
badly and caused issues. (I know because I wrote one of those bad clients :D)

Passing an error might still make sense, but retry is IMO not a valid reason for
it.

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

I agree with this - we should hide the nasty things from the clients of this
class.

+}