Avoid sending GoogleSigninSucceeded notifications on reauth.

The desktop re-authentication flow does not update the sign-in manager.
Here are the explinations why:
* SigninManager::StartSigninWithRefreshToken dchecks that the profile
is not authenticated (see https://cs.chromium.org/chromium/src/components/signin/core/browser/signin_manager.cc?rcl=a2b0b2c00ad3b7c02b57fe86b9feba1a4c5221fd&l=105)
* InlineSigninHandler simply updates the refresh tokens in case of a
reauth (see https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/signin/inline_login_handler_impl.cc?rcl=a2b0b2c00ad3b7c02b57fe86b9feba1a4c5221fd&l=246 )

The mobile re-auth flow calls SigninManager::OnExternalSignin which in
some cases may lead to a unexpected call to GoogleSigninSucceeded.
On iOS this flow does not exists as the iSL always removes invalid
accounts.
On Android, there is a possibility for the SigninManager::OnExternalSignin
to be called in a reauth flow, however the Java code ignores
GoogleSigninSucceeded notifications (see
https://cs.chromium.org/chromium/src/chrome/browser/android/signin/signin_manager_android.cc?rcl=a2b0b2c00ad3b7c02b57fe86b9feba1a4c5221fd&l=347)

This CL makes it explicit that:
* SigninManager does not send GoogleSigninSucceeded notifications on
reauth
* Adds a DCHECK when the authenticated account ID is changed.

TBR=mpearson@chromium.org

Bug: 733226
Change-Id: I85cc4497a0d099447a7d8a46b435e93f39d8d4c7
Reviewed-on: https://chromium-review.googlesource.com/579432
Commit-Queue: Mihai Sardarescu <msarda@chromium.org>
Reviewed-by: David Roger <droger@chromium.org>
Reviewed-by: Roger Tawa <rogerta@chromium.org>
Reviewed-by: Pavel Yatsuk <pavely@chromium.org>
Cr-Commit-Position: refs/heads/master@{#491983}

chrome/browser/metrics/ukm_browsertest.cc

@@ -16,6 +16,11 @@
 #include "content/public/common/content_switches.h"
 #include "services/metrics/public/cpp/ukm_recorder.h"
 
+#if defined(OS_CHROMEOS)
+#include "chrome/browser/signin/signin_manager_factory.h"
+#include "components/signin/core/browser/signin_manager_base.h"
+#endif
+
 namespace metrics {
 
 // An observer that returns back to test code after a new profile is
@@ -68,11 +73,21 @@ class UkmBrowserTest : public SyncTest {
         base::MakeUnique<fake_server::FakeServerNetworkResources>(
             GetFakeServer()->AsWeakPtr()));
 
+#if defined(OS_CHROMEOS)
+    // In browser tests, the profile is already authenticated with stub account
+    // |user_manager::kStubUserEmail|.
+    AccountInfo info = SigninManagerFactory::GetForProfile(profile)
+                           ->GetAuthenticatedAccountInfo();
+    std::string username = info.email;
+    std::string gaia_id = info.gaia;
+#else
+    std::string username = "user@gmail.com";
+    std::string gaia_id = "123456789";
+#endif
     std::unique_ptr<ProfileSyncServiceHarness> harness =
         ProfileSyncServiceHarness::Create(
-            profile, "user@gmail.com", "password",
+            profile, username, gaia_id, "unused" /* password */,
             ProfileSyncServiceHarness::SigninType::FAKE_SIGNIN);
-
     EXPECT_TRUE(harness->SetupSync());
     return harness;
   }

chrome/browser/signin/signin_manager_unittest.cc

@@ -423,6 +423,30 @@ TEST_F(SigninManagerTest, ExternalSignIn) {
   EXPECT_EQ(account_id, manager_->GetAuthenticatedAccountId());
 }
 
+TEST_F(SigninManagerTest, ExternalSignIn_ReauthShouldNotSendNotification) {
+  CreateNakedSigninManager();
+  manager_->Initialize(g_browser_process->local_state());
+  EXPECT_EQ("", manager_->GetAuthenticatedAccountInfo().email);
+  EXPECT_EQ("", manager_->GetAuthenticatedAccountId());
+  EXPECT_EQ(0, test_observer_.num_successful_signins_);
+  EXPECT_EQ(0, test_observer_.num_successful_signins_with_password_);
+
+  std::string account_id = AddToAccountTracker("gaia_id", "user@gmail.com");
+  manager_->OnExternalSigninCompleted("user@gmail.com");
+  EXPECT_EQ(1, test_observer_.num_successful_signins_);
+  EXPECT_EQ(1, test_observer_.num_successful_signins_with_password_);
+  EXPECT_EQ(0, test_observer_.num_failed_signins_);
+  EXPECT_EQ("user@gmail.com", manager_->GetAuthenticatedAccountInfo().email);
+  EXPECT_EQ(account_id, manager_->GetAuthenticatedAccountId());
+
+  manager_->OnExternalSigninCompleted("user@gmail.com");
+  EXPECT_EQ(1, test_observer_.num_successful_signins_);
+  EXPECT_EQ(1, test_observer_.num_successful_signins_with_password_);
+  EXPECT_EQ(0, test_observer_.num_failed_signins_);
+  EXPECT_EQ("user@gmail.com", manager_->GetAuthenticatedAccountInfo().email);
+  EXPECT_EQ(account_id, manager_->GetAuthenticatedAccountId());
+}
+
 TEST_F(SigninManagerTest, SigninNotAllowed) {
   std::string user("user@google.com");
   profile()->GetPrefs()->SetString(prefs::kGoogleServicesAccountId, user);

chrome/browser/sync/sync_ui_util_unittest.cc

@@ -267,7 +267,7 @@ sync_ui_util::ActionType GetActionTypeforDistinctCase(int case_number) {
 TEST_F(SyncUIUtilTest, DistinctCasesReportUniqueMessageSets) {
   std::set<base::string16> messages;
   for (int idx = 0; idx != NUMBER_OF_STATUS_CASES; idx++) {
-    std::unique_ptr<Profile> profile(new TestingProfile());
+    std::unique_ptr<Profile> profile = base::MakeUnique<TestingProfile>();
     ProfileSyncService::InitParams init_params =
         CreateProfileSyncServiceParamsForTest(profile.get());
     NiceMock<ProfileSyncServiceMock> service(&init_params);
@@ -308,7 +308,7 @@ TEST_F(SyncUIUtilTest, DistinctCasesReportUniqueMessageSets) {
 // honored.
 TEST_F(SyncUIUtilTest, HtmlNotIncludedInStatusIfNotRequested) {
   for (int idx = 0; idx != NUMBER_OF_STATUS_CASES; idx++) {
-    std::unique_ptr<Profile> profile(MakeSignedInTestingProfile());
+    std::unique_ptr<Profile> profile = base::MakeUnique<TestingProfile>();
     ProfileSyncService::InitParams init_params =
         CreateProfileSyncServiceParamsForTest(profile.get());
     NiceMock<ProfileSyncServiceMock> service(&init_params);

chrome/browser/sync/test/integration/profile_sync_service_harness.cc

@@ -39,10 +39,6 @@ using syncer::SyncCycleSnapshot;
 
 namespace {
 
-std::string GetGaiaIdForUsername(const std::string& username) {
-  return "gaia-id-" + username;
-}
-
 bool HasAuthError(ProfileSyncService* service) {
   return service->GetAuthError().state() ==
              GoogleServiceAuthError::INVALID_GAIA_CREDENTIALS ||
@@ -102,25 +98,27 @@ class SyncSetupChecker : public SingleClientStatusChangeChecker {
 std::unique_ptr<ProfileSyncServiceHarness> ProfileSyncServiceHarness::Create(
     Profile* profile,
     const std::string& username,
+    const std::string& gaia_id,
     const std::string& password,
     SigninType signin_type) {
-  return base::WrapUnique(
-      new ProfileSyncServiceHarness(profile, username, password, signin_type));
+  return base::WrapUnique(new ProfileSyncServiceHarness(
+      profile, username, gaia_id, password, signin_type));
 }
 
 ProfileSyncServiceHarness::ProfileSyncServiceHarness(
     Profile* profile,
     const std::string& username,
+    const std::string& gaia_id,
     const std::string& password,
     SigninType signin_type)
     : profile_(profile),
       service_(ProfileSyncServiceFactory::GetForProfile(profile)),
       username_(username),
+      gaia_id_(gaia_id),
       password_(password),
       signin_type_(signin_type),
       oauth2_refesh_token_number_(0),
-      profile_debug_name_(profile->GetDebugName()) {
-}
+      profile_debug_name_(profile->GetDebugName()) {}
 
 ProfileSyncServiceHarness::~ProfileSyncServiceHarness() { }
 
@@ -172,8 +170,7 @@ bool ProfileSyncServiceHarness::SetupSync(syncer::ModelTypeSet synced_datatypes,
     }
   } else if (signin_type_ == SigninType::FAKE_SIGNIN) {
     // Authenticate sync client using GAIA credentials.
-    std::string gaia_id = GetGaiaIdForUsername(username_);
-    service()->signin()->SetAuthenticatedAccountInfo(gaia_id, username_);
+    service()->signin()->SetAuthenticatedAccountInfo(gaia_id_, username_);
     std::string account_id = service()->signin()->GetAuthenticatedAccountId();
     service()->GoogleSigninSucceeded(account_id, username_);
     ProfileOAuth2TokenServiceFactory::GetForProfile(profile_)->

chrome/browser/sync/test/integration/profile_sync_service_harness.h

@@ -42,6 +42,7 @@ class ProfileSyncServiceHarness {
   static std::unique_ptr<ProfileSyncServiceHarness> Create(
       Profile* profile,
       const std::string& username,
+      const std::string& gaia_id,
       const std::string& password,
       SigninType signin_type);
   virtual ~ProfileSyncServiceHarness();
@@ -139,11 +140,11 @@ class ProfileSyncServiceHarness {
   void FinishSyncSetup();
 
  private:
-  ProfileSyncServiceHarness(
-      Profile* profile,
-      const std::string& username,
-      const std::string& password,
-      SigninType signin_type);
+  ProfileSyncServiceHarness(Profile* profile,
+                            const std::string& username,
+                            const std::string& gaia_id,
+                            const std::string& password,
+                            SigninType signin_type);
 
   // Gets detailed status from |service_| in pretty-printable form.
   std::string GetServiceStatus();
@@ -162,6 +163,7 @@ class ProfileSyncServiceHarness {
 
   // Credentials used for GAIA authentication.
   std::string username_;
+  std::string gaia_id_;
   std::string password_;
 
   // Used to decide what method of profile signin to use.

chrome/browser/sync/test/integration/sync_test.cc

@@ -598,11 +598,9 @@ void SyncTest::InitializeProfile(int index, Profile* profile) {
           : ProfileSyncServiceHarness::SigninType::FAKE_SIGNIN;
 
   DCHECK(!clients_[index]);
-  clients_[index] =
-      ProfileSyncServiceHarness::Create(GetProfile(index),
-                                        username_,
-                                        password_,
-                                        singin_type);
+  clients_[index] = ProfileSyncServiceHarness::Create(
+      GetProfile(index), username_, "gaia-id-" + username_, password_,
+      singin_type);
   EXPECT_NE(nullptr, GetClient(index)) << "Could not create Client " << index;
   InitializeInvalidations(index);
 }

components/signin/core/browser/signin_manager.cc

@@ -376,8 +376,11 @@ void SigninManager::OnExternalSigninCompleted(const std::string& username) {
 }
 
 void SigninManager::OnSignedIn() {
+  bool reauth_in_progress = IsAuthenticated();
+
   client_->GetPrefs()->SetInt64(prefs::kSignedInTime,
                                 base::Time::Now().ToInternalValue());
+
   SetAuthenticatedAccountInfo(possibly_invalid_gaia_id_,
                               possibly_invalid_email_);
   const std::string gaia_id = possibly_invalid_gaia_id_;
@@ -387,14 +390,8 @@ void SigninManager::OnSignedIn() {
   possibly_invalid_email_.clear();
   signin_manager_signed_in_ = true;
 
-  for (auto& observer : observer_list_) {
-    observer.GoogleSigninSucceeded(GetAuthenticatedAccountId(),
-                                   GetAuthenticatedAccountInfo().email);
-
-    observer.GoogleSigninSucceededWithPassword(
-        GetAuthenticatedAccountId(), GetAuthenticatedAccountInfo().email,
-        password_);
-  }
+  if (!reauth_in_progress)
+    FireGoogleSigninSucceeded();
 
   client_->OnSignedIn(GetAuthenticatedAccountId(), gaia_id,
                       GetAuthenticatedAccountInfo().email, password_);
@@ -407,6 +404,15 @@ void SigninManager::OnSignedIn() {
   PostSignedIn();
 }
 
+void SigninManager::FireGoogleSigninSucceeded() {
+  std::string account_id = GetAuthenticatedAccountId();
+  std::string email = GetAuthenticatedAccountInfo().email;
+  for (auto& observer : observer_list_) {
+    observer.GoogleSigninSucceeded(account_id, email);
+    observer.GoogleSigninSucceededWithPassword(account_id, email, password_);
+  }
+}
+
 void SigninManager::PostSignedIn() {
   if (!signin_manager_signed_in_ || !user_info_fetched_by_account_tracker_)
     return;

components/signin/core/browser/signin_manager.h

@@ -184,15 +184,18 @@ class SigninManager : public SigninManagerBase,
   // a sign-in success notification.
   void OnSignedIn();
 
+  // Send all observers |GoogleSigninSucceeded| notifications.
+  void FireGoogleSigninSucceeded();
+
   // Waits for the AccountTrackerService, then sends GoogleSigninSucceeded to
   // the client and clears the local password.
   void PostSignedIn();
 
-  // AccountTrackerService::Observer implementation.
+  // AccountTrackerService::Observer:
   void OnAccountUpdated(const AccountInfo& info) override;
   void OnAccountUpdateFailed(const std::string& account_id) override;
 
-  // OAuth2TokenService::Observer
+  // OAuth2TokenService::Observer:
   void OnRefreshTokensLoaded() override;
 
   // Called when a new request to re-authenticate a user is in progress.

components/signin/core/browser/signin_manager_base.cc

@@ -184,9 +184,9 @@ void SigninManagerBase::SetAuthenticatedAccountId(
     const std::string& account_id) {
   DCHECK(!account_id.empty());
   if (!authenticated_account_id_.empty()) {
-    DLOG_IF(ERROR, account_id != authenticated_account_id_)
-        << "Tried to change the authenticated id to something different: "
-        << "Current: " << authenticated_account_id_ << ", New: " << account_id;
+    DCHECK_EQ(account_id, authenticated_account_id_)
+        << "Changing the authenticated account while authenticated is not "
+           "allowed.";
     return;
   }
 

components/signin/core/browser/signin_manager_base.h

@@ -59,6 +59,7 @@ class SigninManagerBase : public KeyedService {
     virtual void GoogleSigninFailed(const GoogleServiceAuthError& error) {}
 
     // Called when a user signs into Google services such as sync.
+    // This method is not called during a reauth.
     virtual void GoogleSigninSucceeded(const std::string& account_id,
                                        const std::string& username) {}
 
@@ -81,6 +82,7 @@ class SigninManagerBase : public KeyedService {
 
     // Called when a user signs into Google services such as sync. Also passes
     // the password of the Google account that was used to sign in.
+    // This method is not called during a reauth.
     //
     // Observers should override |GoogleSigninSucceeded| if they are not
     // interested in the password thas was used during the sign-in.
@@ -175,6 +177,10 @@ class SigninManagerBase : public KeyedService {
   }
 
   // Sets the authenticated user's account id.
+  // If the user is already authenticated with the same account id, then this
+  // method is a no-op.
+  // It is forbidden to call this method if the user is already authenticated
+  // with a different account (this method will DCHECK in that case).
   void SetAuthenticatedAccountId(const std::string& account_id);
 
   // Used by subclass to clear the authenticated user instead of using