Remove all legacy supervised users on startup.

The flag is: RemoveSupervisedUsersOnStartup.
It is disabled by default.

The tests in this CL verify that when the feature is disabled, no users
are removed, and when the feature is enabled, *only* the Supervised
users are removed.

Bug: 916685
Change-Id: I32882e786f457a27368724415b65e6a428e4b70b
Reviewed-on: https://chromium-review.googlesource.com/c/1384934Reviewed-by: Michael Giuffrida <michaelpg@chromium.org>
Reviewed-by: Jacob Dufault <jdufault@chromium.org>
Commit-Queue: Danan S <danan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#626865}

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

@@ -9,6 +9,7 @@
 #include <cstddef>
 #include <set>
 #include <utility>
+#include <vector>
 
 #include "ash/public/cpp/ash_pref_names.h"
 #include "ash/public/interfaces/constants.mojom.h"
@@ -847,9 +848,23 @@ void ChromeUserManagerImpl::PerformPreUserListLoadingActions() {
 }
 
 void ChromeUserManagerImpl::PerformPostUserListLoadingActions() {
-  for (user_manager::UserList::iterator ui = users_.begin(), ue = users_.end();
-       ui != ue; ++ui) {
-    GetUserImageManager((*ui)->GetAccountId())->LoadUserImage();
+  std::vector<user_manager::User*> users_to_remove;
+
+  for (user_manager::User* user : users_) {
+    // TODO(http://crbug/866790): Remove supervised user accounts. After we have
+    // enough confidence that there are no more supervised users on devices in
+    // the wild, remove this.
+    if (base::FeatureList::IsEnabled(
+            features::kRemoveSupervisedUsersOnStartup) &&
+        user->IsSupervised()) {
+      users_to_remove.push_back(user);
+    } else {
+      GetUserImageManager(user->GetAccountId())->LoadUserImage();
+    }
+  }
+
+  for (user_manager::User* user : users_to_remove) {
+    RemoveUser(user->GetAccountId(), nullptr);
   }
 }
 

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

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include <memory>
+#include <vector>
+
+#include "base/macros.h"
+#include "base/test/scoped_feature_list.h"
+#include "chrome/browser/chromeos/login/login_manager_test.h"
+#include "chrome/browser/chromeos/login/users/chrome_user_manager.h"
+#include "chrome/browser/chromeos/login/users/chrome_user_manager_impl.h"
+#include "chrome/browser/chromeos/settings/cros_settings.h"
+#include "chrome/browser/chromeos/settings/scoped_testing_cros_settings.h"
+#include "chrome/common/chrome_features.h"
+#include "components/account_id/account_id.h"
+#include "components/user_manager/scoped_user_manager.h"
+#include "components/user_manager/user_manager.h"
+#include "components/user_manager/user_manager_base.h"
+
+namespace chromeos {
+
+namespace {
+
+struct {
+  const char* email;
+  const char* gaia_id;
+} const kTestUsers[] = {
+    {"test-user1@gmail.com", "1111111111"},
+    {"test-user2@gmail.com", "2222222222"},
+    // Test Supervised User.
+    // The domain is defined in user_manager::kSupervisedUserDomain.
+    // That const isn't directly referenced here to keep this code readable by
+    // avoiding std::string concatenations.
+    {"test-superviseduser@locally-managed.localhost", "3333333333"},
+};
+
+}  // namespace
+
+class RemoveSupervisedUsersBrowserTest : public LoginManagerTest {
+ public:
+  RemoveSupervisedUsersBrowserTest() : LoginManagerTest(false, false) {}
+
+  ~RemoveSupervisedUsersBrowserTest() override = default;
+
+  void SetUpOnMainThread() override {
+    LoginManagerTest::SetUpOnMainThread();
+    InitializeTestUsers();
+  }
+
+  void TearDownOnMainThread() override {
+    LoginManagerTest::TearDownOnMainThread();
+    scoped_user_manager_.reset();
+  }
+
+ protected:
+  std::vector<AccountId> test_users_;
+
+ private:
+  void InitializeTestUsers() {
+    for (size_t i = 0; i < base::size(kTestUsers); ++i) {
+      test_users_.emplace_back(AccountId::FromUserEmailGaiaId(
+          kTestUsers[i].email, kTestUsers[i].gaia_id));
+      RegisterUser(test_users_[i]);
+    }
+
+    // Setup a scoped real ChromeUserManager, since this is an end-to-end
+    // test.  This will read the users registered to the local state just
+    // before this in RegisterUser.
+    scoped_user_manager_ = std::make_unique<user_manager::ScopedUserManager>(
+        chromeos::ChromeUserManagerImpl::CreateChromeUserManager());
+  }
+
+  std::unique_ptr<user_manager::ScopedUserManager> scoped_user_manager_;
+  ScopedTestingCrosSettings scoped_testing_cros_settings_;
+
+  DISALLOW_COPY_AND_ASSIGN(RemoveSupervisedUsersBrowserTest);
+};
+
+class RemoveSupervisedUsersBrowserEnabledTest
+    : public RemoveSupervisedUsersBrowserTest {
+ protected:
+  void SetUpInProcessBrowserTestFixture() override {
+    scoped_feature_list_.InitAndEnableFeature(
+        features::kRemoveSupervisedUsersOnStartup);
+  }
+
+ private:
+  base::test::ScopedFeatureList scoped_feature_list_;
+};
+
+class RemoveSupervisedUsersBrowserDisabledTest
+    : public RemoveSupervisedUsersBrowserTest {
+ protected:
+  void SetUpInProcessBrowserTestFixture() override {
+    scoped_feature_list_.InitAndDisableFeature(
+        features::kRemoveSupervisedUsersOnStartup);
+  }
+
+ private:
+  base::test::ScopedFeatureList scoped_feature_list_;
+};
+
+IN_PROC_BROWSER_TEST_F(RemoveSupervisedUsersBrowserEnabledTest,
+                       SupervisedUserRemoved) {
+  // When Supervised users are removed, only 2 test users should be returned.
+  EXPECT_EQ(2U, user_manager::UserManager::Get()->GetUsers().size());
+
+  // None of the non-supervised users should have been removed.
+  for (user_manager::User* user :
+       user_manager::UserManager::Get()->GetUsers()) {
+    EXPECT_EQ(false, user->IsSupervised());
+  }
+}
+
+IN_PROC_BROWSER_TEST_F(RemoveSupervisedUsersBrowserDisabledTest,
+                       SupervisedUserNotRemoved) {
+  // When Supervised users are not removed, all 3 test users should be returned.
+  EXPECT_EQ(3U, user_manager::UserManager::Get()->GetUsers().size());
+}
+
+}  // namespace chromeos

chrome/common/chrome_features.cc

@@ -495,6 +495,12 @@ const base::Feature kNupPrinting{"NupPrinting",
 const base::Feature kPushMessagingBackgroundMode{
     "PushMessagingBackgroundMode", base::FEATURE_DISABLED_BY_DEFAULT};
 
+#if defined(OS_CHROMEOS)
+// Enables permanent removal of Legacy Supervised Users on startup.
+const base::Feature kRemoveSupervisedUsersOnStartup{
+    "RemoveSupervisedUsersOnStartup", base::FEATURE_DISABLED_BY_DEFAULT};
+#endif
+
 const base::Feature kSafeSearchUrlReporting{"SafeSearchUrlReporting",
                                             base::FEATURE_DISABLED_BY_DEFAULT};
 

chrome/common/chrome_features.h

@@ -327,6 +327,11 @@ COMPONENT_EXPORT(CHROME_FEATURES) extern const base::Feature kNupPrinting;
 COMPONENT_EXPORT(CHROME_FEATURES)
 extern const base::Feature kPushMessagingBackgroundMode;
 
+#if defined(OS_CHROMEOS)
+COMPONENT_EXPORT(CHROME_FEATURES)
+extern const base::Feature kRemoveSupervisedUsersOnStartup;
+#endif
+
 COMPONENT_EXPORT(CHROME_FEATURES)
 extern const base::Feature kSafeSearchUrlReporting;
 

chrome/test/BUILD.gn

@@ -1787,6 +1787,7 @@ test("browser_tests") {
         "../browser/chromeos/login/users/avatar/user_image_manager_browsertest.cc",
         "../browser/chromeos/login/users/avatar/user_image_manager_test_util.cc",
         "../browser/chromeos/login/users/avatar/user_image_manager_test_util.h",
+        "../browser/chromeos/login/users/remove_supervised_users_browsertest.cc",
         "../browser/chromeos/login/users/user_manager_hide_supervised_users_browsertest.cc",
         "../browser/chromeos/login/users/wallpaper_policy_browsertest.cc",
         "../browser/chromeos/login/webview_login_browsertest.cc",