User re-creation during password change flow should not trigger GAIA.

crrev.com/c/4208788 introduced one regression: if user going through
re-creation flow is the only user on the device, code would incorrectly
trigger showing GAIA screen as there will be 0 users on login screen.

This CL fixes that, and makes PasswordChangeTest.SkipDataRecovery
work correctly when CryptohomeRecovery flag is enabled.

Bug: b:257225574
Change-Id: I9927a09509412732df017fab5691a558f8340f81
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4255628
Reviewed-by: Anastasiia N <anastasiian@chromium.org>
Reviewed-by: Maksim Ivanov <emaxx@chromium.org>
Reviewed-by: Hidehiko Abe <hidehiko@chromium.org>
Commit-Queue: Denis Kuznetsov <antrim@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1107668}

chrome/browser/ash/login/password_change_browsertest.cc

@@ -6,6 +6,7 @@
 #include <string>
 #include <utility>
 
+#include "ash/constants/ash_features.h"
 #include "ash/public/cpp/login_screen_test_api.h"
 #include "base/auto_reset.h"
 #include "base/files/file_path.h"
@@ -301,10 +302,16 @@ IN_PROC_BROWSER_TEST_F(PasswordChangeTest, SkipDataRecovery) {
   // Click "Proceed anyway".
   test::OobeJS().ClickOnPath(kProceedAnyway);
 
-  // User session should start, and whole OOBE screen is expected to be hidden.
-  OobeWindowVisibilityWaiter(false).Wait();
+  if (features::IsCryptohomeRecoveryEnabled()) {
+    // With cryptohome recovery we re-create session and re-run onboarding.
+    OobeWindowVisibilityWaiter(true).Wait();
+  } else {
+    // User session should start, and whole OOBE screen is expected to be
+    // hidden.
+    OobeWindowVisibilityWaiter(false).Wait();
 
-  login_mixin_.WaitForActiveSession();
+    login_mixin_.WaitForActiveSession();
+  }
   EXPECT_FALSE(TestingFileExists());
 }
 

chrome/browser/ash/login/screens/gaia_password_changed_screen.cc

@@ -211,7 +211,12 @@ void GaiaPasswordChangedScreen::OnRemovedUserDirectory(
   }
   // Force user to go through onboarding again, so that they have
   // consistent experience.
-  user_manager::UserManager::Get()->RemoveUserFromList(
+  // Do not notify about removal, as we are still inside the login
+  // flow. Otherwise, GAIA screen might be shown (if this user was
+  // the only user on the device).
+  // TODO(b/270040728): Use `RemoveUserFromList` once internal architecture
+  // allows better solution.
+  user_manager::UserManager::Get()->RemoveUserFromListForRecreation(
       context()->user_context->GetAccountId());
   // Now that user is deleted, reset everything in UserContext
   // related to cryptohome state.

components/user_manager/fake_user_manager.cc

@@ -110,6 +110,11 @@ void FakeUserManager::RemoveUserFromList(const AccountId& account_id) {
   }
 }
 
+void FakeUserManager::RemoveUserFromListForRecreation(
+    const AccountId& account_id) {
+  RemoveUserFromList(account_id);
+}
+
 const UserList& FakeUserManager::GetUsers() const {
   return users_;
 }

components/user_manager/fake_user_manager.h

@@ -97,6 +97,7 @@ class USER_MANAGER_EXPORT FakeUserManager : public UserManagerBase {
                   UserRemovalReason reason,
                   RemoveUserDelegate* delegate) override {}
   void RemoveUserFromList(const AccountId& account_id) override;
+  void RemoveUserFromListForRecreation(const AccountId& account_id) override;
   bool IsKnownUser(const AccountId& account_id) const override;
   const User* FindUser(const AccountId& account_id) const override;
   User* FindUserAndModify(const AccountId& account_id) override;

components/user_manager/user_manager.h

@@ -211,6 +211,15 @@ class USER_MANAGER_EXPORT UserManager {
   // picture.
   virtual void RemoveUserFromList(const AccountId& account_id) = 0;
 
+  // Removes the user from persistent list, without triggering user removal
+  // notification.
+  // Used to re-create user in Password changed flow when user can not
+  // remember old password and decides to delete existing user directory and
+  // re-create it.
+  // TODO(b/270040728): Remove this method once internal architecture allows
+  // better solution.
+  virtual void RemoveUserFromListForRecreation(const AccountId& account_id) = 0;
+
   // Returns true if a user with the given account id is found in the persistent
   // list or currently logged in as ephemeral.
   virtual bool IsKnownUser(const AccountId& account_id) const = 0;

components/user_manager/user_manager_base.cc

@@ -380,15 +380,24 @@ void UserManagerBase::RemoveNonOwnerUserInternal(AccountId account_id,
 }
 
 void UserManagerBase::RemoveUserFromList(const AccountId& account_id) {
+  RemoveUserFromListImpl(account_id, /* notify=*/true);
+}
+
+void UserManagerBase::RemoveUserFromListForRecreation(
+    const AccountId& account_id) {
+  RemoveUserFromListImpl(account_id, /* notify=*/false);
+}
+
+void UserManagerBase::RemoveUserFromListImpl(const AccountId& account_id,
+                                             bool notify) {
   DCHECK(!task_runner_ || task_runner_->RunsTasksInCurrentSequence());
   RemoveNonCryptohomeData(account_id);
   KnownUser(GetLocalState()).RemovePrefs(account_id);
   if (user_loading_stage_ == STAGE_LOADED) {
     // After the User object is deleted from memory in DeleteUser() here,
     // the account_id reference will be invalid if the reference points
     // to the account_id in the User object.
-    DeleteUser(
-        RemoveRegularOrSupervisedUserFromList(account_id, true /* notify */));
+    DeleteUser(RemoveRegularOrSupervisedUserFromList(account_id, notify));
   } else {
     NOTREACHED() << "Users are not loaded yet.";
     return;

components/user_manager/user_manager_base.h

@@ -97,6 +97,7 @@ class USER_MANAGER_EXPORT UserManagerBase : public UserManager {
                   UserRemovalReason reason,
                   RemoveUserDelegate* delegate) override;
   void RemoveUserFromList(const AccountId& account_id) override;
+  void RemoveUserFromListForRecreation(const AccountId& account_id) override;
   bool IsKnownUser(const AccountId& account_id) const override;
   const User* FindUser(const AccountId& account_id) const override;
   User* FindUserAndModify(const AccountId& account_id) override;
@@ -234,6 +235,9 @@ class USER_MANAGER_EXPORT UserManagerBase : public UserManager {
   User* RemoveRegularOrSupervisedUserFromList(const AccountId& account_id,
                                               bool notify);
 
+  // Implementation for `RemoveUserFromList`[`ForRecreation`] methods.
+  void RemoveUserFromListImpl(const AccountId& account_id, bool notify);
+
   // Implementation for RemoveUser method. This is an asynchronous part of the
   // method, that verifies that owner will not get deleted, and calls
   // |RemoveNonOwnerUserInternal|.