Use OAuth2 token for sync

chrome/browser/sync/profile_sync_service.cc

Index: chrome/browser/sync/profile_sync_service.cc
diff --git a/chrome/browser/sync/profile_sync_service.cc b/chrome/browser/sync/profile_sync_service.cc
index da473e4540cc2246f0b555ceeb573a97581237ae..c6ba85b50633bf7eebeb1442c2b1c8fe27fdeede 100644
--- a/chrome/browser/sync/profile_sync_service.cc
+++ b/chrome/browser/sync/profile_sync_service.cc
@@ -28,6 +28,8 @@
 #include "chrome/browser/net/chrome_cookie_notification_details.h"
 #include "chrome/browser/prefs/pref_service_syncable.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/signin/profile_oauth2_token_service.h"
+#include "chrome/browser/signin/profile_oauth2_token_service_factory.h"
 #include "chrome/browser/signin/signin_manager.h"
 #include "chrome/browser/signin/signin_manager_factory.h"
 #include "chrome/browser/signin/token_service.h"
@@ -104,24 +106,15 @@ const char* ProfileSyncService::kDevServerUrl =
 
 static const int kSyncClearDataTimeoutInSeconds = 60;  // 1 minute.
 
-static const char* kRelevantTokenServices[] = {
-    GaiaConstants::kSyncService
+static const char* kOAuth2Scopes[] = {

[tim (not reviewing), 2013/05/23 19:03:41]

These belong in google_apis / GaiaConstants.

[pavely, 2013/05/30 07:42:12]

Done.

[Andrew T Wilson (Slow), 2013/05/31 12:57:28]

That's too bad - I guess we need to move a little more functionality into
OAuth2TokenService then.

+  "https://www.googleapis.com/auth/chromesync",
+  "https://www.googleapis.com/auth/googletalk"

[Andrew T Wilson (Slow), 2013/05/31 12:57:28]

So, this means we *cannot* sync if the user has the talk service disabled since
we can't mint a token. I think that's probably OK, but wanted to make sure
nobody was shocked by this (I think in some past release, we would still be able
to sync but just wouldn't get notifications but this might not be the current
behavior any more).

[pavely, 2013/06/04 00:49:59]

Currently if notifications server returns auth error then SyncManager will
invalidate token for both sync and notifications so in this sense this is not a
regression.

 };
-static const int kRelevantTokenServicesCount =
-    arraysize(kRelevantTokenServices);
+
 
 static const char* kSyncUnrecoverableErrorHistogram =
     "Sync.UnrecoverableErrors";
 
-// Helper to check if the given token service is relevant for sync.
-static bool IsTokenServiceRelevant(const std::string& service) {
-  for (int i = 0; i < kRelevantTokenServicesCount; ++i) {
-    if (service == kRelevantTokenServices[i])
-      return true;
-  }
-  return false;
-}
-
 bool ShouldShowActionOnUI(
     const syncer::SyncProtocolError& error) {
   return (error.action != syncer::UNKNOWN_ACTION &&
@@ -154,7 +147,9 @@ ProfileSyncService::ProfileSyncService(ProfileSyncComponentsFactory* factory,
       failed_datatypes_handler_(this),
       configure_status_(DataTypeManager::UNKNOWN),
       setup_in_progress_(false),
-      invalidator_state_(syncer::DEFAULT_INVALIDATION_ERROR) {
+      invalidator_state_(syncer::DEFAULT_INVALIDATION_ERROR),
+      access_token_(),

[tim (not reviewing), 2013/05/23 19:03:41]

Remove these two initializers as they are not necessary and convention in
Chromium is only to have an initializer if necessary.

[pavely, 2013/05/30 07:42:12]

Done.

+      access_token_request_() {
   // By default, dev, canary, and unbranded Chromium users will go to the
   // development servers. Development servers have more features than standard
   // sync servers. Users with officially-branded Chrome stable and beta builds
@@ -190,7 +185,7 @@ bool ProfileSyncService::IsSyncTokenAvailable() {
   TokenService* token_service = TokenServiceFactory::GetForProfile(profile_);
   if (!token_service)
     return false;
-  return token_service->HasTokenForService(GaiaConstants::kSyncService);
+  return token_service->HasOAuthLoginToken();

[tim (not reviewing), 2013/05/23 19:03:41]

Is there a better name than just "OAuthLoginToken" for this function? 
"OAuth[Uber, Refresh]Token"?

[Andrew T Wilson (Slow), 2013/05/24 14:10:15]

I wonder if it's better to call OAuth2TokenService::RefreshTokenIsAvailable()
here instead of talking to TokenService directly. Ideally we would remove our
direct dependency on TokenService.

And if we can remove this function entirely, that'd be even niftier, in favor of
callers just calling RefreshTokenIsAvailable().

[pavely, 2013/05/30 07:42:12]

Done.

 }
 #if defined(OS_ANDROID)
 bool ProfileSyncService::ShouldEnablePasswordSyncForAndroid() const {
@@ -238,6 +233,12 @@ void ProfileSyncService::Initialize() {
 
   TrySyncDatatypePrefRecovery();
 
+  if (IsSyncTokenAvailable()) {
+    // Tell token service to pre-request access token. We likely don't need it
+    // right away but it will be available when we decide to initialize backend.
+    RequestAccessToken(false, false);
+  }
+
   TryStart();
 }
 
@@ -414,10 +415,8 @@ SyncCredentials ProfileSyncService::GetCredentials() {
   SyncCredentials credentials;
   credentials.email = signin_->GetAuthenticatedUsername();
   DCHECK(!credentials.email.empty());
-  TokenService* service = TokenServiceFactory::GetForProfile(profile_);
-  if (service->HasTokenForService(GaiaConstants::kSyncService)) {
-      credentials.sync_token = service->GetTokenForService(
-          GaiaConstants::kSyncService);
+  if (!access_token_.empty()) {

[tim (not reviewing), 2013/05/23 19:03:41]

Can you explain the semantics here in a comment if access_token_ is empty?  What
happens to credentials.sync_token?  Is this not equivalent to assigning even in
that case?

[Andrew T Wilson (Slow), 2013/05/24 14:10:15]

Note the varying UMAs being logged. I do wonder if this is useful at all any
more - this code was trying to trigger an auth error (instead of some kind of
unrecoverable error in sync) in the case that for some reason the TokenDB did
not have a chromiumsync token.

What are the situations where access_token_ can be empty? I can imagine a bunch
of cases:

* Temporary network error
* Persistent auth error (OAuthTokenService can't mint a token for you due to
INVALID_GAIA_CREDENTIALS)
* Local tokendb corruption (user is signed in, but has no oauth login token -
ideally we'd catch this elswhere and report as an auth error)
* "chromiumsync" service is disabled on the server via the dasher admin panel

I think we only need to report an auth error in case #2 - actually doing so in
any other case is bad.

[pavely, 2013/05/30 07:42:12]

Like it was before if access token is empty then sync sends some known bad value
to server ("credentials_lost"). It is equivalent to assigning empty
access_token_, only difference is value that will be seen on the server. I just
wasn't sure if changing this value matters. 

[pavely, 2013/05/30 07:42:12]

Let me explain cases that you listed:
- In temporary network error either PSS wouldn't start backend or token wouldn't
be cleared. 
- In case of auth error refreshing token will cause OnGetTokenFailure which will
display error.
- With local storage corruption PSS won't be able to request access token and
will display error.
- disabled service will be handled differently. I haven't found a way to find
out from client that account is disabled without actually sending RPC to sync
server.

UMA histograms in failure case would be "increment if user is signed in but
login token is not present.
Do you mind if I remove these two histogram statements?

[Andrew T Wilson (Slow), 2013/05/31 12:57:28]

Is this true? I thought that sync would generate an unrecoverable error if we
had an empty access token (that was why I added this known-bad value to begin
with - so users would have some way to fix this problem by signing back in).
Does the sync backend no longer generate an unrecoverable error when there's an
empty token?
- In temporary network error either PSS wouldn't start backend or token wouldn't
be cleared. 

Just curious what happens if we setup sync, start chrome, and on the restart we
can't mint a new token because of a networking error. What happens to sync? We
don't have an access token, so I don't think we can start the backend, which
seems bad (sync should in theory be able to run even if the device is offline -
it just can't hit the server).
Will it? I thought we just act like the user isn't signed in now, but the logic
flow is complex so if you say it results in a visible error that's good.
The important thing I want is to understand the percentage of clients that don't
have a login token. I think I can get that from your proposed UMA, so if you
want to replace the existing UMA with your new one I have no objection.

+    credentials.sync_token = access_token_;
     UMA_HISTOGRAM_BOOLEAN("Sync.CredentialsLost", false);
   } else {
     // We've lost our sync credentials (crbug.com/121755), so just make up some
@@ -499,6 +498,11 @@ void ProfileSyncService::StartUp(StartUpDeferredOption deferred_option) {
 
   DCHECK(IsSyncEnabledAndLoggedIn());
 
+  if (access_token_.empty()) {

[tim (not reviewing), 2013/05/23 19:03:41]

The relationship between TryStart and StartUp is that *all* preconditions for
starting are true by the time StartUp is called.  Everything sync needs should
be there and ready, but all logic of figuring out when to start should be
handled by TryStart.  It seems like this should be made a
DCHECK(!access_token_.empty()).

In TryStart, we shouldn't call StartUp if access_token_ is empty. But we also
shouldn't have to call RequestAccessToken, right?  It would have been called by
Initialize.

[pavely, 2013/05/30 07:42:12]

TryStart gathers all local information to decide if it is time to start up
backend and start sending RPCs. Among other things it checks whenever chrome has
oauth2 refresh token. Obtaining access token grom gaia is not local operation.
Requesting access token in Initialize is not enough, there are scenarios when it
needs to be requested in TryStart codepath, for example
PSS::UnsuppressAndStart(). Also requesting access token from Initialize is not
always appropriate. If tokens are not loaded yet this request is guaranteed to
fail.

+    RequestAccessToken(false, true);

[Andrew T Wilson (Slow), 2013/05/31 12:57:28]

Should we only do this if deferred_option = STARTUP_IMMEDIATE?

+    return;
+  }
+
   last_synced_time_ = sync_prefs_.GetLastSyncedTime();
 
   DCHECK(start_up_time_.is_null());
@@ -607,6 +611,25 @@ syncer::InvalidatorState ProfileSyncService::GetInvalidatorState() const {
   return invalidator_registrar_->GetInvalidatorState();
 }
 
+void ProfileSyncService::OnGetTokenSuccess(
+    const OAuth2TokenService::Request* request,
+    const std::string& access_token,
+    const base::Time& expiration_time) {
+  access_token_request_.reset();

[tim (not reviewing), 2013/05/23 19:03:42]

DCHECK(access_token_request_.get()) in both these functions would be nice.

[Andrew T Wilson (Slow), 2013/05/24 14:10:15]

Actually, I think you should do DCHECK_EQ(access_token_request, request).

[pavely, 2013/05/30 07:42:12]

Done.

+  access_token_ = access_token;
+  if (backend_)
+    backend_->UpdateCredentials(GetCredentials());
+  else
+    TryStart();
+}
+
+void ProfileSyncService::OnGetTokenFailure(
+    const OAuth2TokenService::Request* request,
+    const GoogleServiceAuthError& error) {
+  access_token_request_.reset();
+  UpdateAuthErrorState(error);
+}
+
 void ProfileSyncService::EmitInvalidationForTest(
     const invalidation::ObjectId& id,
     const std::string& payload) {
@@ -1029,10 +1052,14 @@ AuthError ConnectionStatusToAuthError(
 
 void ProfileSyncService::OnConnectionStatusChange(
     syncer::ConnectionStatus status) {
-  const GoogleServiceAuthError auth_error =
-      ConnectionStatusToAuthError(status);
-  DVLOG(1) << "Connection status change: " << auth_error.ToString();
-  UpdateAuthErrorState(auth_error);
+  if (status == syncer::CONNECTION_AUTH_ERROR) {

[tim (not reviewing), 2013/05/23 19:03:42]

Comment this - is this known to be a transient access_token expiration?  Or do
we optimistically try that first regardless?

[pavely, 2013/05/30 07:42:12]

Done.

+    RequestAccessToken(true, true);
+  } else {
+    const GoogleServiceAuthError auth_error =
+        ConnectionStatusToAuthError(status);
+    DVLOG(1) << "Connection status change: " << auth_error.ToString();
+    UpdateAuthErrorState(auth_error);
+  }
 }
 
 void ProfileSyncService::OnStopSyncingPermanently() {
@@ -1777,6 +1804,31 @@ void ProfileSyncService::ConsumeCachedPassphraseIfPossible() {
     SetEncryptionPassphrase(passphrase, IMPLICIT);
 }
 
+void ProfileSyncService::RequestAccessToken(
+    bool invalidate_previous_token,
+    bool invoke_callback)
+{

[tim (not reviewing), 2013/05/23 19:03:42]

nit: { on previous line.

[pavely, 2013/05/30 07:42:12]

Done.

+  // Only one active request at a time.
+  if (access_token_request_ != NULL)

[tim (not reviewing), 2013/05/23 19:03:42]

Think this should be a DCHECK?

[Andrew T Wilson (Slow), 2013/05/24 14:10:15]

I think this can happen in practice - there's nothing in the contract that says
you'll get exactly one OnConnectionStatusChange call, for example.

[pavely, 2013/05/30 07:42:12]

Yes, there could be multiple calls to RequestAccessToken and request might take
long time.

+    return;
+  OAuth2TokenService::ScopeSet oauth2_scopes;
+  for (size_t i = 0; i < arraysize(kOAuth2Scopes); i++)
+    oauth2_scopes.insert(kOAuth2Scopes[i]);
+  OAuth2TokenService* token_service =
+      ProfileOAuth2TokenServiceFactory::GetForProfile(profile_);
+  if (invalidate_previous_token) {
+    // Invalidate previous token, othervise token service will return the same
+    // token again
+    token_service->InvalidateToken(oauth2_scopes, access_token_);
+    access_token_ = std::string();

[tim (not reviewing), 2013/05/23 19:03:42]

use access_token_.clear()

[pavely, 2013/05/30 07:42:12]

Done.

+  }
+  access_token_request_ = token_service->StartRequest(oauth2_scopes, this);
+  if (!invoke_callback) {
+    // Deleting request will not cancel RPC but callbacks won't be invoked.
+    access_token_request_.reset();

[Andrew T Wilson (Slow), 2013/05/24 14:10:15]

I am confused by this - on startup, won't this cause two access_token_requests
to be initiated, since we aren't tracking our first request? Why do we ever want
to pass invoke_callback == true - why not just let the fetch complete and invoke
OnGetTokenSuccess()?

[pavely, 2013/05/30 07:42:12]

I discussed this with Tim, the question was whenever to prerequest access token
upfront hoping that access token will be in OAuth2TokenService cache by the time
it is needed. It saves roundtrip to GAIA when PSS starts up backend.
The way how I implemented it is by requesting token in Initialize but not
requesting OnGetTokenSuccess to be called because at that time it is not obvious
that backend needs to be started. When PSS is about to start up backend it will
call RequestAccessToken with invoke_callback == true.
If PSS initiates two requests OAuth2TokenService will only initiate one RPC to
GAIA for them. 

[Andrew T Wilson (Slow), 2013/05/31 12:57:28]

Is this a real concern? I didn't think we were that concerned about starting up
the backend immediately except in the case where we are signing in with "choose
what to sync" in which case we'd be issuing a RequestAccessToken call in short
order anyway. It just seems like we're optimizing something that doesn't need
optimization, and making the code more complex. I defer to Tim if he really
thinks this is worth the added complexity.

+  }
+}
+
 void ProfileSyncService::SetEncryptionPassphrase(const std::string& passphrase,
                                                  PassphraseType type) {
   // This should only be called when the backend has been initialized.
@@ -1883,7 +1935,8 @@ void ProfileSyncService::Observe(int type,
       const TokenService::TokenRequestFailedDetails& token_details =
           *(content::Details<const TokenService::TokenRequestFailedDetails>(
               details).ptr());
-      if (IsTokenServiceRelevant(token_details.service()) &&
+      if (token_details.service() ==
+          GaiaConstants::kGaiaOAuth2LoginRefreshToken &&

[Andrew T Wilson (Slow), 2013/05/24 14:10:15]

I'd really like to see us get out of the "listen for TOKEN_REQUEST_FAILED"
business. Can you put a TODO(atwilson) in the code here? There's no reason for
sync to be reporting auth errors on behalf of TokenService - this is at best a
leftover from our legacy signin architecture, and at worst obsolete and
superfluous.

[pavely, 2013/05/30 07:42:12]

Done.

           !IsSyncTokenAvailable()) {
         // The additional check around IsSyncTokenAvailable() above prevents us
         // sounding the alarm if we actually have a valid token but a refresh
@@ -1900,7 +1953,8 @@ void ProfileSyncService::Observe(int type,
       const TokenService::TokenAvailableDetails& token_details =
           *(content::Details<const TokenService::TokenAvailableDetails>(
               details).ptr());
-      if (!IsTokenServiceRelevant(token_details.service()))
+      if (token_details.service() !=
+          GaiaConstants::kGaiaOAuth2LoginRefreshToken)

[Andrew T Wilson (Slow), 2013/05/24 14:10:15]

We should really move NOTIFICATION_TOKEN_AVAILABLE to OAuth2TokenService. I've
filed a bug for this - can you add a TODO(atwilson): Listen for notifications on
OAuth2TokenService (crbug.com/243737) here?

[pavely, 2013/05/30 07:42:12]

Done.

         break;
     } // Fall through.
     case chrome::NOTIFICATION_TOKEN_LOADING_FINISHED: {
@@ -1910,14 +1964,16 @@ void ProfileSyncService::Observe(int type,
       // not loaded, GetCredentials() will generate invalid credentials to
       // cause the backend to generate an auth error (crbug.com/121755).
       if (backend_)
-        backend_->UpdateCredentials(GetCredentials());
+        RequestAccessToken(true, true);
       else
+        RequestAccessToken(true, false);
         TryStart();
       break;
     }
     case chrome::NOTIFICATION_TOKENS_CLEARED: {
       // GetCredentials() will generate invalid credentials to cause the backend
       // to generate an auth error.
+      access_token_ = std::string();

[tim (not reviewing), 2013/05/23 19:03:42]

use .clear()

[pavely, 2013/05/30 07:42:12]

Done.

       if (backend_)
         backend_->UpdateCredentials(GetCredentials());
       break;