cros: AuthSyncObserver observes SigninErrorController

- Make AuthSyncObserver observe SigninErrorController in addition
  to sync service so that auth errors caused by other services
  are caught as well.
- Fix an edge case that OAuth2LoginManager marks valid oauth2
  token when it thinks /MergeSession succeeds regardless of
  whether there is any auth error;

Bug: 760610
Test: OAuth2Test.SetInvalidTokenStatus

Change-Id: Idbaee9f7faa1aa2aff939a1540bfcfdd9f49c96c
Reviewed-on: https://chromium-review.googlesource.com/658378
Commit-Queue: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: Achuith Bhandarkar <achuith@chromium.org>
Cr-Commit-Position: refs/heads/master@{#504391}

chrome/browser/chromeos/login/session/user_session_manager.cc

@@ -75,6 +75,7 @@
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/signin/account_tracker_service_factory.h"
 #include "chrome/browser/signin/easy_unlock_service.h"
+#include "chrome/browser/signin/signin_error_controller_factory.h"
 #include "chrome/browser/signin/signin_manager_factory.h"
 #include "chrome/browser/supervised_user/child_accounts/child_account_service.h"
 #include "chrome/browser/supervised_user/child_accounts/child_account_service_factory.h"
@@ -105,6 +106,7 @@
 #include "components/session_manager/core/session_manager.h"
 #include "components/signin/core/account_id/account_id.h"
 #include "components/signin/core/browser/account_tracker_service.h"
+#include "components/signin/core/browser/signin_error_controller.h"
 #include "components/signin/core/browser/signin_manager_base.h"
 #include "components/user_manager/known_user.h"
 #include "components/user_manager/user.h"
@@ -845,7 +847,15 @@ void UserSessionManager::OnSessionRestoreStateChanged(
   bool connection_error = false;
   switch (state) {
     case OAuth2LoginManager::SESSION_RESTORE_DONE:
-      user_status = user_manager::User::OAUTH2_TOKEN_STATUS_VALID;
+      // Session restore done does not always mean valid token because the
+      // merge session operation could be skipped when the first account in
+      // Gaia cookies matches the primary account in TokenService. However
+      // the token could still be invalid in some edge cases. See
+      // http://crbug.com/760610
+      user_status =
+          SigninErrorControllerFactory::GetForProfile(user_profile)->HasError()
+              ? user_manager::User::OAUTH2_TOKEN_STATUS_INVALID
+              : user_manager::User::OAUTH2_TOKEN_STATUS_VALID;
       break;
     case OAuth2LoginManager::SESSION_RESTORE_FAILED:
       user_status = user_manager::User::OAUTH2_TOKEN_STATUS_INVALID;

chrome/browser/chromeos/login/signin/auth_sync_observer.cc

@@ -10,65 +10,97 @@
 #include "chrome/browser/chromeos/login/users/chrome_user_manager.h"
 #include "chrome/browser/chromeos/login/users/supervised_user_manager.h"
 #include "chrome/browser/chromeos/profiles/profile_helper.h"
+#include "chrome/browser/signin/signin_error_controller_factory.h"
+#include "chrome/browser/signin/signin_manager_factory.h"
 #include "chrome/browser/sync/profile_sync_service_factory.h"
 #include "components/browser_sync/profile_sync_service.h"
-#include "components/prefs/pref_service.h"
+#include "components/signin/core/browser/signin_manager_base.h"
 #include "components/user_manager/user_manager.h"
 #include "components/user_manager/user_type.h"
-#include "google_apis/gaia/gaia_auth_util.h"
-
-class Profile;
-
-namespace browser_sync {
-class ProfileSyncService;
-}  // namespace browser_sync
 
 namespace chromeos {
 
-AuthSyncObserver::AuthSyncObserver(Profile* profile)
-    : profile_(profile) {
+// static
+bool AuthSyncObserver::ShouldObserve(Profile* profile) {
+  const user_manager::User* const user =
+      ProfileHelper::Get()->GetUserByProfile(profile);
+  return user && (user->HasGaiaAccount() ||
+                  user->GetType() == user_manager::USER_TYPE_SUPERVISED);
 }
 
-AuthSyncObserver::~AuthSyncObserver() {
+AuthSyncObserver::AuthSyncObserver(Profile* profile) : profile_(profile) {
+  DCHECK(ShouldObserve(profile));
 }
 
+AuthSyncObserver::~AuthSyncObserver() {}
+
 void AuthSyncObserver::StartObserving() {
-  browser_sync::ProfileSyncService* sync_service =
+  browser_sync::ProfileSyncService* const sync_service =
       ProfileSyncServiceFactory::GetForProfile(profile_);
   if (sync_service)
     sync_service->AddObserver(this);
+
+  SigninErrorController* const error_controller =
+      SigninErrorControllerFactory::GetForProfile(profile_);
+  if (error_controller) {
+    error_controller->AddObserver(this);
+    OnErrorChanged();
+  }
 }
 
 void AuthSyncObserver::Shutdown() {
-  browser_sync::ProfileSyncService* sync_service =
+  browser_sync::ProfileSyncService* const sync_service =
       ProfileSyncServiceFactory::GetForProfile(profile_);
   if (sync_service)
     sync_service->RemoveObserver(this);
+
+  SigninErrorController* const error_controller =
+      SigninErrorControllerFactory::GetForProfile(profile_);
+  if (error_controller)
+    error_controller->RemoveObserver(this);
 }
 
 void AuthSyncObserver::OnStateChanged(syncer::SyncService* sync) {
-  DCHECK(user_manager::UserManager::Get()->IsLoggedInAsUserWithGaiaAccount() ||
-         user_manager::UserManager::Get()->IsLoggedInAsSupervisedUser());
-  const user_manager::User* user =
+  HandleAuthError(sync->GetAuthError());
+}
+
+void AuthSyncObserver::OnErrorChanged() {
+  SigninErrorController* const error_controller =
+      SigninErrorControllerFactory::GetForProfile(profile_);
+  const std::string error_account_id = error_controller->error_account_id();
+
+  const std::string primary_account_id =
+      SigninManagerFactory::GetForProfile(profile_)
+          ->GetAuthenticatedAccountId();
+
+  // Bail if there is an error account id and it is not the primary account id.
+  if (!error_account_id.empty() && error_account_id != primary_account_id)
+    return;
+
+  HandleAuthError(error_controller->auth_error());
+}
+
+void AuthSyncObserver::HandleAuthError(
+    const GoogleServiceAuthError& auth_error) {
+  const user_manager::User* const user =
       ProfileHelper::Get()->GetUserByProfile(profile_);
-  GoogleServiceAuthError::State state = sync->GetAuthError().state();
-  if (state != GoogleServiceAuthError::NONE &&
-      state != GoogleServiceAuthError::CONNECTION_FAILED &&
-      state != GoogleServiceAuthError::SERVICE_UNAVAILABLE &&
-      state != GoogleServiceAuthError::REQUEST_CANCELED) {
+  DCHECK(user->HasGaiaAccount() ||
+         user->GetType() == user_manager::USER_TYPE_SUPERVISED);
+
+  if (auth_error.IsPersistentError()) {
     // Invalidate OAuth2 refresh token to force Gaia sign-in flow. This is
     // needed because sign-out/sign-in solution is suggested to the user.
-    // TODO(nkostylev): Remove after crosbug.com/25978 is implemented.
-    LOG(WARNING) << "Invalidate OAuth token because of a sync error: "
-                 << sync->GetAuthError().ToString();
+    LOG(WARNING) << "Invalidate OAuth token because of an auth error: "
+                 << auth_error.ToString();
     const AccountId& account_id = user->GetAccountId();
     DCHECK(account_id.is_valid());
-    // TODO(nkostyelv): Change observer after active user has changed.
+
     user_manager::User::OAuthTokenStatus old_status =
         user->oauth_token_status();
     user_manager::UserManager::Get()->SaveUserOAuthStatus(
         account_id, user_manager::User::OAUTH2_TOKEN_STATUS_INVALID);
     RecordReauthReason(account_id, ReauthReason::SYNC_FAILED);
+
     if (user->GetType() == user_manager::USER_TYPE_SUPERVISED &&
         old_status != user_manager::User::OAUTH2_TOKEN_STATUS_INVALID) {
        // Attempt to restore token from file.
@@ -81,7 +113,7 @@ void AuthSyncObserver::OnStateChanged(syncer::SyncService* sync) {
       base::RecordAction(
           base::UserMetricsAction("ManagedUsers_Chromeos_Sync_Invalidated"));
     }
-  } else if (state == GoogleServiceAuthError::NONE) {
+  } else if (auth_error.state() == GoogleServiceAuthError::NONE) {
     if (user->GetType() == user_manager::USER_TYPE_SUPERVISED &&
         user->oauth_token_status() ==
             user_manager::User::OAUTH2_TOKEN_STATUS_INVALID) {

chrome/browser/chromeos/login/signin/auth_sync_observer.h

@@ -7,23 +7,30 @@
 
 #include <string>
 
-#include "base/compiler_specific.h"
 #include "base/macros.h"
 #include "components/keyed_service/core/keyed_service.h"
+#include "components/signin/core/browser/signin_error_controller.h"
 #include "components/sync/driver/sync_service_observer.h"
 
+class GoogleServiceAuthError;
 class Profile;
 
 namespace chromeos {
 
 // This class is responsible for detecting authentication problems reported
-// by sync service and
+// by sync service and SigninErrorController on a user profile.
 class AuthSyncObserver : public KeyedService,
-                         public syncer::SyncServiceObserver {
+                         public syncer::SyncServiceObserver,
+                         public SigninErrorController::Observer {
  public:
-  explicit AuthSyncObserver(Profile* user_profile);
+  // Whether |profile| should be observed. Currently, this returns true only
+  // when |profile| is a user profile of a gaia user or a supervised user.
+  static bool ShouldObserve(Profile* profile);
+
+  explicit AuthSyncObserver(Profile* profile);
   ~AuthSyncObserver() override;
 
+  // Starts to observe SyncService and SigninErrorController.
   void StartObserving();
 
  private:
@@ -35,10 +42,16 @@ class AuthSyncObserver : public KeyedService,
   // syncer::SyncServiceObserver implementation.
   void OnStateChanged(syncer::SyncService* sync) override;
 
+  // SigninErrorController::Observer implementation.
+  void OnErrorChanged() override;
+
+  // Handles an auth error.
+  void HandleAuthError(const GoogleServiceAuthError& auth_error);
+
   // Called on attempt to restore supervised user token.
   void OnSupervisedTokenLoaded(const std::string& token);
 
-  Profile* profile_;
+  Profile* const profile_;
 
   DISALLOW_COPY_AND_ASSIGN(AuthSyncObserver);
 };

chrome/browser/chromeos/login/signin/auth_sync_observer_factory.cc

@@ -6,6 +6,7 @@
 
 #include "chrome/browser/chromeos/login/signin/auth_sync_observer.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/signin/signin_error_controller_factory.h"
 #include "chrome/browser/sync/profile_sync_service_factory.h"
 #include "components/keyed_service/content/browser_context_dependency_manager.h"
 
@@ -16,6 +17,7 @@ AuthSyncObserverFactory::AuthSyncObserverFactory()
         "AuthSyncObserver",
         BrowserContextDependencyManager::GetInstance()) {
   DependsOn(ProfileSyncServiceFactory::GetInstance());
+  DependsOn(SigninErrorControllerFactory::GetInstance());
 }
 
 AuthSyncObserverFactory::~AuthSyncObserverFactory() {
@@ -24,6 +26,9 @@ AuthSyncObserverFactory::~AuthSyncObserverFactory() {
 // static
 AuthSyncObserver* AuthSyncObserverFactory::GetForProfile(
     Profile* profile) {
+  if (!AuthSyncObserver::ShouldObserve(profile))
+    return nullptr;
+
   return static_cast<AuthSyncObserver*>(
       GetInstance()->GetServiceForBrowserContext(profile, true));
 }

chrome/browser/chromeos/login/users/chrome_user_manager_impl.cc

@@ -496,9 +496,11 @@ void ChromeUserManagerImpl::Observe(
           ManagerPasswordServiceFactory::GetForProfile(profile);
 
         if (!profile->IsOffTheRecord()) {
-          AuthSyncObserver* sync_observer =
-              AuthSyncObserverFactory::GetInstance()->GetForProfile(profile);
-          sync_observer->StartObserving();
+          if (AuthSyncObserver::ShouldObserve(profile)) {
+            AuthSyncObserver* sync_observer =
+                AuthSyncObserverFactory::GetInstance()->GetForProfile(profile);
+            sync_observer->StartObserving();
+          }
           multi_profile_user_controller_->StartObserving(profile);
         }
       }