[Sync] Use account_id instead of username in a few places in sync

Currently sync gets username from SigninManagerBase::GetAuthenticatedAccountInfo(). In some cases it returns empty result which prevents sync backend from starting. Username is used in following places: - passed to sync directory database to be stored as share id - passed to server in ClientToServerMessage.share - passed to attachment uploader/downloader to be used as account_id when requesting access tokens The change is to switch to more stable SigninManagerBase::GetAuthenticatedAccountId(). account_id will be passed to directory and attachment uploader/downloader. Username is still used in communications with server but it is not enforced to be nonempty. BUG=554551 R=zea@chromium.org Review URL: https://codereview.chromium.org/1544893002 Cr-Commit-Position: refs/heads/master@{#366721}
hylschwy · Dec 23, 2015 · 7623e62 · 7623e62
1 parent e725975
commit 7623e62
Show file tree

Hide file tree

Showing 11 changed files with 42 additions and 32 deletions.
diff --git a/components/browser_sync/browser/profile_sync_components_factory_impl.cc b/components/browser_sync/browser/profile_sync_components_factory_impl.cc
@@ -318,7 +318,7 @@ ProfileSyncComponentsFactoryImpl::CreateAttachmentService(
   // Only construct an AttachmentUploader and AttachmentDownload if we have sync
   // credentials. We may not have sync credentials because there may not be a
   // signed in sync user (e.g. sync is running in "backup" mode).
-  if (!user_share.sync_credentials.email.empty() &&
+  if (!user_share.sync_credentials.account_id.empty() &&
       !user_share.sync_credentials.scope_set.empty()) {
     scoped_refptr<OAuth2TokenServiceRequest::TokenServiceProvider>
         token_service_provider(
@@ -328,15 +328,15 @@ ProfileSyncComponentsFactoryImpl::CreateAttachmentService(
     // per AttachmentService (bug 369536).
     attachment_uploader.reset(new syncer::AttachmentUploaderImpl(
         sync_service_url_, url_request_context_getter_,
-        user_share.sync_credentials.email,
+        user_share.sync_credentials.account_id,
         user_share.sync_credentials.scope_set, token_service_provider,
         store_birthday, model_type));
 
     token_service_provider =
         new TokenServiceProvider(ui_thread_, token_service_);
     attachment_downloader = syncer::AttachmentDownloader::Create(
         sync_service_url_, url_request_context_getter_,
-        user_share.sync_credentials.email,
+        user_share.sync_credentials.account_id,
         user_share.sync_credentials.scope_set, token_service_provider,
         store_birthday, model_type);
   }

diff --git a/components/browser_sync/browser/profile_sync_service.cc b/components/browser_sync/browser/profile_sync_service.cc
@@ -499,8 +499,9 @@ void ProfileSyncService::OnSessionRestoreComplete() {
 SyncCredentials ProfileSyncService::GetCredentials() {
   SyncCredentials credentials;
   if (backend_mode_ == SYNC) {
+    credentials.account_id = signin_->GetAccountIdToUse();
+    DCHECK(!credentials.account_id.empty());
     credentials.email = signin_->GetEffectiveUsername();
-    DCHECK(!credentials.email.empty());
     credentials.sync_token = access_token_;
 
     if (credentials.sync_token.empty())

diff --git a/components/sync_driver/glue/sync_backend_host_impl_unittest.cc b/components/sync_driver/glue/sync_backend_host_impl_unittest.cc
@@ -5,6 +5,7 @@
 #include "components/sync_driver/glue/sync_backend_host_impl.h"
 
 #include <cstddef>
+#include <map>
 
 #include "base/files/file_util.h"
 #include "base/files/scoped_temp_dir.h"
@@ -168,8 +169,7 @@ class BackendSyncClient : public sync_driver::FakeSyncClient {
 class SyncBackendHostTest : public testing::Test {
  protected:
   SyncBackendHostTest()
-      : fake_manager_(NULL) {
-   }
+      : fake_manager_(NULL) {}
 
   ~SyncBackendHostTest() override {}
 
@@ -185,6 +185,7 @@ class SyncBackendHostTest : public testing::Test {
         nullptr,
         sync_prefs_->AsWeakPtr(),
         temp_dir_.path().Append(base::FilePath(kTestSyncDir))));
+    credentials_.account_id = "user@example.com";
     credentials_.email = "user@example.com";
     credentials_.sync_token = "sync_token";
     credentials_.scope_set.insert(GaiaConstants::kChromeSyncOAuth2Scope);

diff --git a/components/sync_driver/startup_controller.cc b/components/sync_driver/startup_controller.cc
@@ -4,6 +4,8 @@
 
 #include "components/sync_driver/startup_controller.h"
 
+#include <string>
+
 #include "base/command_line.h"
 #include "base/location.h"
 #include "base/metrics/histogram.h"
@@ -124,14 +126,13 @@ bool StartupController::TryStart() {
   if (!sync_prefs_->IsSyncRequested())
     return false;
 
-  if (signin_->GetEffectiveUsername().empty())
+  if (signin_->GetAccountIdToUse().empty())
     return false;
 
   if (!token_service_)
     return false;
 
-  if (!token_service_->RefreshTokenIsAvailable(
-          signin_->GetAccountIdToUse())) {
+  if (!token_service_->RefreshTokenIsAvailable(signin_->GetAccountIdToUse())) {
     return false;
   }
 

diff --git a/components/sync_driver/startup_controller_unittest.cc b/components/sync_driver/startup_controller_unittest.cc
@@ -4,6 +4,8 @@
 
 #include "components/sync_driver/startup_controller.h"
 
+#include <string>
+
 #include "base/command_line.h"
 #include "base/message_loop/message_loop.h"
 #include "base/run_loop.h"
@@ -31,14 +33,16 @@ static const char kStateStringNotStarted[] = "Not started";
 class FakeSigninManagerWrapper : public SigninManagerWrapper {
  public:
   FakeSigninManagerWrapper() : SigninManagerWrapper(NULL) {}
-  std::string GetEffectiveUsername() const override { return account_; }
+  std::string GetEffectiveUsername() const override { return std::string(); }
 
-  std::string GetAccountIdToUse() const override { return account_; }
+  std::string GetAccountIdToUse() const override { return account_id_; }
 
-  void set_account(const std::string& account) { account_ = account; }
+  void set_account_id(const std::string& account_id) {
+    account_id_ = account_id;
+  }
 
  private:
-  std::string account_;
+  std::string account_id_;
 };
 
 class StartupControllerTest : public testing::Test {
@@ -105,7 +109,7 @@ TEST_F(StartupControllerTest, Basic) {
   sync_prefs()->SetSyncSetupCompleted();
   controller()->TryStart();
   EXPECT_FALSE(started());
-  signin()->set_account(kTestUser);
+  signin()->set_account_id(kTestUser);
   controller()->TryStart();
   EXPECT_FALSE(started());
   token_service()->UpdateCredentials(kTestUser, kTestToken);
@@ -124,7 +128,7 @@ TEST_F(StartupControllerTest, Basic) {
 TEST_F(StartupControllerTest, NotRequested) {
   sync_prefs()->SetSyncSetupCompleted();
   sync_prefs()->SetSyncRequested(false);
-  signin()->set_account(kTestUser);
+  signin()->set_account_id(kTestUser);
   token_service()->UpdateCredentials(kTestUser, kTestToken);
   controller()->TryStart();
   EXPECT_FALSE(started());
@@ -136,7 +140,7 @@ TEST_F(StartupControllerTest, NotRequested) {
 TEST_F(StartupControllerTest, Managed) {
   sync_prefs()->SetSyncSetupCompleted();
   sync_prefs()->SetManagedForTest(true);
-  signin()->set_account(kTestUser);
+  signin()->set_account_id(kTestUser);
   token_service()->UpdateCredentials(kTestUser, kTestToken);
   controller()->TryStart();
   EXPECT_FALSE(started());
@@ -148,7 +152,7 @@ TEST_F(StartupControllerTest, Managed) {
 // data type triggers sync startup.
 TEST_F(StartupControllerTest, DataTypeTriggered) {
   sync_prefs()->SetSyncSetupCompleted();
-  signin()->set_account(kTestUser);
+  signin()->set_account_id(kTestUser);
   token_service()->UpdateCredentials(kTestUser, kTestToken);
   controller()->TryStart();
   EXPECT_FALSE(started());
@@ -170,7 +174,7 @@ TEST_F(StartupControllerTest, DataTypeTriggered) {
 // conditions are met and no data type requests sync.
 TEST_F(StartupControllerTest, FallbackTimer) {
   sync_prefs()->SetSyncSetupCompleted();
-  signin()->set_account(kTestUser);
+  signin()->set_account_id(kTestUser);
   token_service()->UpdateCredentials(kTestUser, kTestToken);
   controller()->TryStart();
   EXPECT_FALSE(started());
@@ -190,7 +194,7 @@ TEST_F(StartupControllerTest, NoDeferralWithoutSessionsSync) {
   sync_prefs()->SetPreferredDataTypes(syncer::UserTypes(), types);
   controller()->Reset(syncer::UserTypes());
   sync_prefs()->SetSyncSetupCompleted();
-  signin()->set_account(kTestUser);
+  signin()->set_account_id(kTestUser);
   token_service()->UpdateCredentials(kTestUser, kTestToken);
   controller()->TryStart();
   EXPECT_TRUE(started());
@@ -208,7 +212,7 @@ TEST_F(StartupControllerTest, FallbackTimerWaits) {
 // Test that sync starts without the user having to explicitly ask for
 // setup when AUTO_START is the startup behavior requested.
 TEST_F(StartupControllerTest, FirstSetupWithAutoStart) {
-  signin()->set_account(kTestUser);
+  signin()->set_account_id(kTestUser);
   token_service()->UpdateCredentials(kTestUser, kTestToken);
   controller()->TryStart();
   EXPECT_TRUE(started());
@@ -217,7 +221,7 @@ TEST_F(StartupControllerTest, FirstSetupWithAutoStart) {
 // Test that sync starts only after user explicitly asks for setup when
 // MANUAL_START is the startup behavior requested.
 TEST_F(StartupControllerTest, FirstSetupWithManualStart) {
-  signin()->set_account(kTestUser);
+  signin()->set_account_id(kTestUser);
   token_service()->UpdateCredentials(kTestUser, kTestToken);
   SetUpController(MANUAL_START);
   controller()->TryStart();
@@ -229,7 +233,7 @@ TEST_F(StartupControllerTest, FirstSetupWithManualStart) {
 
 TEST_F(StartupControllerTest, Reset) {
   sync_prefs()->SetSyncSetupCompleted();
-  signin()->set_account(kTestUser);
+  signin()->set_account_id(kTestUser);
   token_service()->UpdateCredentials(kTestUser, kTestToken);
   controller()->TryStart();
   const bool deferred_start =
@@ -248,7 +252,7 @@ TEST_F(StartupControllerTest, Reset) {
 
 // Test that setup-in-progress tracking is persistent across a Reset.
 TEST_F(StartupControllerTest, ResetDuringSetup) {
-  signin()->set_account(kTestUser);
+  signin()->set_account_id(kTestUser);
   token_service()->UpdateCredentials(kTestUser, kTestToken);
 
   // Simulate UI telling us setup is in progress.

diff --git a/sync/engine/sync_scheduler_impl.cc b/sync/engine/sync_scheduler_impl.cc
@@ -233,7 +233,6 @@ void SyncSchedulerImpl::Start(Mode mode, base::Time last_poll_time) {
     SendInitialSnapshot();
   }
 
-  DCHECK(!session_context_->account_name().empty());
   DCHECK(syncer_.get());
 
   if (mode == CLEAR_SERVER_DATA_MODE) {

diff --git a/sync/internal_api/public/sync_manager.h b/sync/internal_api/public/sync_manager.h
@@ -68,6 +68,9 @@ struct SYNC_EXPORT SyncCredentials {
   SyncCredentials();
   ~SyncCredentials();
 
+  // Account_id of signed in account.
+  std::string account_id;
+
   // The email associated with this account.
   std::string email;
 

diff --git a/sync/internal_api/sync_manager_impl.cc b/sync/internal_api/sync_manager_impl.cc
@@ -228,7 +228,7 @@ void SyncManagerImpl::Init(InitArgs* args) {
   CHECK(!initialized_);
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(args->post_factory.get());
-  DCHECK(!args->credentials.email.empty());
+  DCHECK(!args->credentials.account_id.empty());
   DCHECK(!args->credentials.sync_token.empty());
   DCHECK(!args->credentials.scope_set.empty());
   DCHECK(args->cancelation_signal);
@@ -263,7 +263,7 @@ void SyncManagerImpl::Init(InitArgs* args) {
   scoped_ptr<syncable::DirectoryBackingStore> backing_store =
       args->internal_components_factory->BuildDirectoryBackingStore(
           InternalComponentsFactory::STORAGE_ON_DISK,
-          args->credentials.email, absolute_db_path).Pass();
+          args->credentials.account_id, absolute_db_path).Pass();
 
   DCHECK(backing_store.get());
   share_.directory.reset(
@@ -279,9 +279,9 @@ void SyncManagerImpl::Init(InitArgs* args) {
   // sync token so clear sync_token from the UserShare.
   share_.sync_credentials.sync_token = "";
 
-  const std::string& username = args->credentials.email;
-  DVLOG(1) << "Username: " << username;
-  if (!OpenDirectory(username)) {
+  DVLOG(1) << "Username: " << args->credentials.email;
+  DVLOG(1) << "AccountId: " << args->credentials.account_id;
+  if (!OpenDirectory(args->credentials.account_id)) {
     NotifyInitializationFailure();
     LOG(ERROR) << "Sync manager initialization failed!";
     return;
@@ -337,7 +337,6 @@ void SyncManagerImpl::Init(InitArgs* args) {
                                              model_type_registry_.get(),
                                              args->invalidator_client_id)
           .Pass();
-  session_context_->set_account_name(args->credentials.email);
   scheduler_ = args->internal_components_factory->BuildScheduler(
       name_, session_context_.get(), args->cancelation_signal).Pass();
 
@@ -506,9 +505,10 @@ bool SyncManagerImpl::PurgeDisabledTypes(
 void SyncManagerImpl::UpdateCredentials(const SyncCredentials& credentials) {
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(initialized_);
-  DCHECK(!credentials.email.empty());
+  DCHECK(!credentials.account_id.empty());
   DCHECK(!credentials.sync_token.empty());
   DCHECK(!credentials.scope_set.empty());
+  session_context_->set_account_name(credentials.email);
 
   observing_network_connectivity_changes_ = true;
   if (!connection_manager_->SetAuthToken(credentials.sync_token))

diff --git a/sync/internal_api/sync_manager_impl_unittest.cc b/sync/internal_api/sync_manager_impl_unittest.cc
@@ -882,6 +882,7 @@ class SyncManagerTest : public testing::Test,
     extensions_activity_ = new ExtensionsActivity();
 
     SyncCredentials credentials;
+    credentials.account_id = "foo@bar.com";
     credentials.email = "foo@bar.com";
     credentials.sync_token = "sometoken";
     OAuth2TokenService::ScopeSet scope_set;

diff --git a/sync/sessions/sync_session_context.h b/sync/sessions/sync_session_context.h
@@ -84,7 +84,6 @@ class SYNC_EXPORT SyncSessionContext {
 
   // Account name, set once a directory has been opened.
   void set_account_name(const std::string& name) {
-    DCHECK(account_name_.empty());
     account_name_ = name;
   }
   const std::string& account_name() const { return account_name_; }

diff --git a/sync/tools/sync_client.cc b/sync/tools/sync_client.cc
@@ -310,6 +310,7 @@ int SyncClientMain(int argc, char* argv[]) {
   const base::CommandLine& command_line =
       *base::CommandLine::ForCurrentProcess();
   SyncCredentials credentials;
+  credentials.account_id = command_line.GetSwitchValueASCII(kEmailSwitch);
   credentials.email = command_line.GetSwitchValueASCII(kEmailSwitch);
   credentials.sync_token = command_line.GetSwitchValueASCII(kTokenSwitch);
   // TODO(akalin): Write a wrapper script that gets a token for an