[Lacros] Add callback to reauth operations

This CL adds a callback to ShowReauthAccountDialog() and uses it in two places: - clicking signin on the web will now perform a reauth with the existing primary account, rather than adding a new account. - clicking "turn on sync" when the account is in error will launch the sync flow after doing the reauthentication. Bug: 1260291 Change-Id: I25c3bf8163cbf96592226d1070ff15201faa2e89 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3627639 Reviewed-by: Boris Sazonov <bsazonov@chromium.org> Commit-Queue: David Roger <droger@chromium.org> Reviewed-by: Monica Basta <msalama@chromium.org> Cr-Commit-Position: refs/heads/main@{#1002078}
chromium · May 11, 2022 · 54d89c8 · 54d89c8
1 parent 9b724ed
commit 54d89c8
Show file tree

Hide file tree

Showing 17 changed files with 292 additions and 27 deletions.
diff --git a/chrome/browser/ash/arc/auth/arc_auth_service.cc b/chrome/browser/ash/arc/auth/arc_auth_service.cc
@@ -538,7 +538,7 @@ void ArcAuthService::HandleUpdateCredentialsRequest(const std::string& email) {
   ::GetAccountManagerFacade(profile_->GetPath().value())
       ->ShowReauthAccountDialog(
           account_manager::AccountManagerFacade::AccountAdditionSource::kArc,
-          email);
+          email, base::OnceClosure());
 }
 
 void ArcAuthService::OnRefreshTokenUpdatedForAccount(

diff --git a/chrome/browser/lacros/account_manager/profile_account_manager.cc b/chrome/browser/lacros/account_manager/profile_account_manager.cc
@@ -81,7 +81,8 @@ void ProfileAccountManager::ShowAddAccountDialog(
 
 void ProfileAccountManager::ShowReauthAccountDialog(
     AccountAdditionSource source,
-    const std::string& email) {
+    const std::string& email,
+    base::OnceClosure callback) {
   NOTREACHED();
 }
 

diff --git a/chrome/browser/lacros/account_manager/profile_account_manager.h b/chrome/browser/lacros/account_manager/profile_account_manager.h
@@ -70,7 +70,8 @@ class ProfileAccountManager : public KeyedService,
       base::OnceCallback<void(const account_manager::AccountAdditionResult&
                                   result)> callback) override;
   void ShowReauthAccountDialog(AccountAdditionSource source,
-                               const std::string& email) override;
+                               const std::string& email,
+                               base::OnceClosure callback) override;
   void ShowManageAccountsSettings() override;
   std::unique_ptr<OAuth2AccessTokenFetcher> CreateAccessTokenFetcher(
       const account_manager::AccountKey& account,

diff --git a/chrome/browser/signin/chrome_signin_helper.cc b/chrome/browser/signin/chrome_signin_helper.cc
@@ -35,6 +35,7 @@
 #include "chrome/browser/signin/signin_features.h"
 #include "chrome/browser/signin/signin_manager.h"
 #include "chrome/browser/signin/signin_manager_factory.h"
+#include "chrome/browser/signin/signin_ui_util.h"
 #include "chrome/browser/tab_contents/tab_util.h"
 #include "chrome/browser/ui/webui/signin/login_ui_service.h"
 #include "chrome/browser/ui/webui/signin/login_ui_service_factory.h"
@@ -44,6 +45,7 @@
 #include "components/signin/core/browser/account_reconcilor.h"
 #include "components/signin/core/browser/cookie_reminter.h"
 #include "components/signin/public/base/account_consistency_method.h"
+#include "components/signin/public/base/consent_level.h"
 #include "components/signin/public/base/signin_buildflags.h"
 #include "components/signin/public/identity_manager/accounts_cookie_mutator.h"
 #include "content/public/browser/browser_task_traits.h"
@@ -305,22 +307,33 @@ void ProcessMirrorHeader(
     }
 
     // Display a re-authentication dialog.
-    ::GetAccountManagerFacade(profile->GetPath().value())
-        ->ShowReauthAccountDialog(account_manager::AccountManagerFacade::
-                                      AccountAdditionSource::kContentAreaReauth,
-                                  manage_accounts_params.email);
+    signin_ui_util::ShowReauthForAccount(
+        browser, manage_accounts_params.email,
+        signin_metrics::AccessPoint::ACCESS_POINT_WEB_SIGNIN);
     return;
   }
 
   // 3. Displaying an account addition window.
   if (service_type == GAIA_SERVICE_TYPE_ADDSESSION) {
 #if BUILDFLAG(IS_CHROMEOS_LACROS)
+    signin::IdentityManager* const identity_manager =
+        IdentityManagerFactory::GetForProfile(profile);
+    CoreAccountInfo primary_account =
+        identity_manager->GetPrimaryAccountInfo(signin::ConsentLevel::kSignin);
+    if (identity_manager->HasAccountWithRefreshTokenInPersistentErrorState(
+            primary_account.account_id)) {
+      // On Lacros, it is not allowed to add a new account while the primary
+      // account is in error, as the reconcilor cannot generate the cookie until
+      // the primary account is fixed. Display a reauth dialog instead.
+      signin_ui_util::ShowReauthForPrimaryAccountWithAuthError(
+          browser, signin_metrics::AccessPoint::ACCESS_POINT_WEB_SIGNIN);
+      return;
+    }
+
     // As per https://crbug.com/1286822 and internal b/215509741, the session
     // may sometimes become invalid on the server without notice. When this
     // happens, the user may try to fix it by signing-in again.
     // Trigger a cookie jar update now to fix the session if needed.
-    signin::IdentityManager* const identity_manager =
-        IdentityManagerFactory::GetForProfile(profile);
     identity_manager->GetAccountsCookieMutator()->TriggerCookieJarUpdate();
 
     AccountProfileMapper* mapper =

diff --git a/chrome/browser/signin/signin_ui_delegate_impl_lacros.cc b/chrome/browser/signin/signin_ui_delegate_impl_lacros.cc
@@ -9,13 +9,16 @@
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/signin/account_reconcilor_factory.h"
+#include "chrome/browser/signin/identity_manager_factory.h"
 #include "chrome/browser/signin/signin_manager.h"
 #include "chrome/browser/signin/signin_manager_factory.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/webui/signin/turn_sync_on_helper.h"
 #include "components/account_manager_core/account_manager_facade.h"
 #include "components/account_manager_core/chromeos/account_manager_facade_factory.h"
 #include "components/signin/core/browser/account_reconcilor.h"
+#include "components/signin/public/base/signin_metrics.h"
+#include "components/signin/public/identity_manager/identity_manager.h"
 #include "google_apis/gaia/core_account_id.h"
 
 namespace signin_ui_util {
@@ -66,6 +69,9 @@ GetAccountReauthSourceFromAccessPoint(
     case signin_metrics::AccessPoint::ACCESS_POINT_RECENT_TABS:
       return account_manager::AccountManagerFacade::AccountAdditionSource::
           kChromeSyncPromoReauth;
+    case signin_metrics::AccessPoint::ACCESS_POINT_WEB_SIGNIN:
+      return account_manager::AccountManagerFacade::AccountAdditionSource::
+          kContentAreaReauth;
     default:
       NOTREACHED() << "Reauth is requested from an unknown access point "
                    << static_cast<int>(access_point);
@@ -86,7 +92,7 @@ void SigninUiDelegateImplLacros::ShowSigninUI(
       base::BindOnce(&SigninUiDelegateImplLacros::OnAccountAdded,
                      // base::Unretained() is fine because
                      // SigninUiDelegateImplLacros is a singleton.
-                     base::Unretained(this), enable_sync,
+                     base::Unretained(this), enable_sync, /*is_reauth=*/false,
                      browser ? browser->AsWeakPtr() : nullptr,
                      profile->GetPath(), access_point, promo_action);
   signin_manager->StartLacrosSigninFlow(
@@ -105,16 +111,27 @@ void SigninUiDelegateImplLacros::ShowReauthUI(
     bool enable_sync,
     signin_metrics::AccessPoint access_point,
     signin_metrics::PromoAction promo_action) {
-  // TODO(https://crbug.com/1260291): turn on sync after reauth is completed if
-  // `enable_sync` is true.
+  AccountReconcilor* account_reconcilor =
+      AccountReconcilorFactory::GetForProfile(profile);
+  base::OnceClosure reauth_completed_closure =
+      base::BindOnce(&SigninUiDelegateImplLacros::OnReauthComplete,
+                     // base::Unretained() is fine because
+                     // SigninUiDelegateImplLacros is a singleton.
+                     base::Unretained(this), enable_sync,
+                     account_reconcilor->GetConsistencyCookieManager()
+                         ->CreateScopedAccountUpdate(),
+                     browser ? browser->AsWeakPtr() : nullptr,
+                     profile->GetPath(), access_point, promo_action, email);
   account_manager::AccountManagerFacade* account_manager_facade =
       ::GetAccountManagerFacade(profile->GetPath().value());
   account_manager_facade->ShowReauthAccountDialog(
-      GetAccountReauthSourceFromAccessPoint(access_point), email);
+      GetAccountReauthSourceFromAccessPoint(access_point), email,
+      std::move(reauth_completed_closure));
 }
 
 void SigninUiDelegateImplLacros::OnAccountAdded(
     bool enable_sync,
+    bool is_reauth,
     base::WeakPtr<Browser> browser_weak,
     const base::FilePath& profile_path,
     signin_metrics::AccessPoint access_point,
@@ -133,8 +150,33 @@ void SigninUiDelegateImplLacros::OnAccountAdded(
     return;
 
   ShowTurnSyncOnUI(browser, profile, access_point, promo_action,
-                   signin_metrics::Reason::kSigninPrimaryAccount, account_id,
-                   TurnSyncOnHelper::SigninAbortedMode::REMOVE_ACCOUNT);
+                   is_reauth ? signin_metrics::Reason::kReauthentication
+                             : signin_metrics::Reason::kSigninPrimaryAccount,
+                   account_id,
+                   is_reauth
+                       ? TurnSyncOnHelper::SigninAbortedMode::KEEP_ACCOUNT
+                       : TurnSyncOnHelper::SigninAbortedMode::REMOVE_ACCOUNT);
+}
+
+void SigninUiDelegateImplLacros::OnReauthComplete(
+    bool enable_sync,
+    signin::ConsistencyCookieManager::ScopedAccountUpdate&& update,
+    base::WeakPtr<Browser> browser_weak,
+    const base::FilePath& profile_path,
+    signin_metrics::AccessPoint access_point,
+    signin_metrics::PromoAction promo_action,
+    const std::string& email) {
+  Profile* profile =
+      g_browser_process->profile_manager()->GetProfileByPath(profile_path);
+  if (!profile)
+    return;
+
+  signin::IdentityManager* identity_manager =
+      IdentityManagerFactory::GetForProfile(profile);
+  OnAccountAdded(enable_sync, /*is_reauth=*/true, browser_weak, profile_path,
+                 access_point, promo_action,
+                 identity_manager->FindExtendedAccountInfoByEmailAddress(email)
+                     .account_id);
 }
 
 }  // namespace signin_ui_util
diff --git a/chrome/browser/signin/signin_ui_delegate_impl_lacros.h b/chrome/browser/signin/signin_ui_delegate_impl_lacros.h
@@ -7,6 +7,22 @@
 
 #include "chrome/browser/signin/signin_ui_delegate.h"
 
+#include <string>
+
+#include "base/memory/weak_ptr.h"
+#include "components/signin/core/browser/consistency_cookie_manager.h"
+
+namespace base {
+class FilePath;
+}
+
+namespace signin_metrics {
+enum class AccessPoint;
+enum class PromoAction;
+}  // namespace signin_metrics
+
+class Profile;
+
 namespace signin_ui_util {
 
 // Lacros-specific implementation of the SigninUiDelegate interface.
@@ -31,11 +47,21 @@ class SigninUiDelegateImplLacros : public SigninUiDelegate {
 
  private:
   void OnAccountAdded(bool enable_sync,
+                      bool is_reauth,
                       base::WeakPtr<Browser> browser_weak,
                       const base::FilePath& profile_path,
                       signin_metrics::AccessPoint access_point,
                       signin_metrics::PromoAction promo_action,
                       const CoreAccountId& account_id);
+
+  void OnReauthComplete(
+      bool enable_sync,
+      signin::ConsistencyCookieManager::ScopedAccountUpdate&& update,
+      base::WeakPtr<Browser> browser_weak,
+      const base::FilePath& profile_path,
+      signin_metrics::AccessPoint access_point,
+      signin_metrics::PromoAction promo_action,
+      const std::string& email);
 };
 
 }  // namespace signin_ui_util

diff --git a/chrome/browser/signin/signin_ui_util.cc b/chrome/browser/signin/signin_ui_util.cc
@@ -51,6 +51,8 @@
 
 #if BUILDFLAG(IS_CHROMEOS_ASH)
 #include "chrome/browser/ash/profiles/profile_helper.h"
+#include "components/account_manager_core/account_manager_facade.h"
+#include "components/account_manager_core/chromeos/account_manager_facade_factory.h"
 #endif  // BUILDFLAG(IS_CHROMEOS_ASH)
 
 #if BUILDFLAG(ENABLE_DICE_SUPPORT)
@@ -211,13 +213,29 @@ void ShowReauthForPrimaryAccountWithAuthError(
   DCHECK(!primary_account_info.IsEmpty());
   DCHECK(identity_manager->HasAccountWithRefreshTokenInPersistentErrorState(
       primary_account_info.account_id));
+  ShowReauthForAccount(browser, primary_account_info.email, access_point);
+#endif  // BUILDFLAG(IS_CHROMEOS_ASH)
+}
+
+void ShowReauthForAccount(Browser* browser,
+                          const std::string& email,
+                          signin_metrics::AccessPoint access_point) {
+#if BUILDFLAG(IS_CHROMEOS_ASH)
+  // Only `ACCESS_POINT_WEB_SIGNIN` is supported, because `kContentAreaReauth`
+  // is hardcoded.
+  DCHECK_EQ(access_point, signin_metrics::AccessPoint::ACCESS_POINT_WEB_SIGNIN);
+  ::GetAccountManagerFacade(browser->profile()->GetPath().value())
+      ->ShowReauthAccountDialog(account_manager::AccountManagerFacade::
+                                    AccountAdditionSource::kContentAreaReauth,
+                                email, base::OnceClosure());
+#else
   // Pass `false` for `enable_sync`, as this function is not expected to start a
   // sync setup flow after the reauth.
   GetSigninUiDelegate()->ShowReauthUI(
-      browser, browser->profile(), primary_account_info.email,
+      browser, browser->profile(), email,
       /*enable_sync=*/false, access_point,
       signin_metrics::PromoAction::PROMO_ACTION_NO_SIGNIN_PROMO);
-#endif  // BUILDFLAG(IS_CHROMEOS_ASH)
+#endif
 }
 
 void ShowExtensionSigninPrompt(Profile* profile,

diff --git a/chrome/browser/signin/signin_ui_util.h b/chrome/browser/signin/signin_ui_util.h
@@ -48,6 +48,11 @@ void ShowReauthForPrimaryAccountWithAuthError(
     Browser* browser,
     signin_metrics::AccessPoint access_point);
 
+// Shows a reauth page/dialog to reauthanticate an account.
+void ShowReauthForAccount(Browser* browser,
+                          const std::string& email,
+                          signin_metrics::AccessPoint access_point);
+
 // Delegates to an existing sign-in tab if one exists. If not, a new sign-in tab
 // is created.
 void ShowExtensionSigninPrompt(Profile* profile,

diff --git a/chrome/browser/ui/views/profiles/profile_menu_view.h b/chrome/browser/ui/views/profiles/profile_menu_view.h
@@ -50,6 +50,9 @@ class ProfileMenuView : public ProfileMenuViewBase {
   friend class ProfileMenuViewSignoutTest;
   friend class ProfileMenuViewSyncErrorButtonTest;
   friend class ProfileMenuInteractiveUiTest;
+#if BUILDFLAG(IS_CHROMEOS_LACROS)
+  friend class ProfileMenuViewSigninErrorButtonTest;
+#endif
 
   // views::BubbleDialogDelegateView:
   std::u16string GetAccessibleWindowTitle() const override;