ash: Fix system tray UAF around user switching

Bug: 965517
Change-Id: Ic059d5b5a0c70701b657d66424d47f5c4de9ae41
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1623496Reviewed-by: Tetsui Ohkubo <tetsui@chromium.org>
Commit-Queue: Xiyuan Xia <xiyuan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#662187}

ash/BUILD.gn

@@ -1797,6 +1797,7 @@ test("ash_unittests") {
     "system/unified/unified_system_info_view_unittest.cc",
     "system/unified/unified_system_tray_controller_unittest.cc",
     "system/unified/unified_system_tray_unittest.cc",
+    "system/unified/user_chooser_detailed_view_controller_unittest.cc",
     "system/update/update_notification_controller_unittest.cc",
     "system/virtual_keyboard/virtual_keyboard_tray_unittest.cc",
     "test/ash_test_helper_unittest.cc",

ash/public/cpp/ash_view_ids.h

@@ -19,6 +19,8 @@ enum ViewID {
   VIEW_ID_ACCESSIBILITY_VIRTUAL_KEYBOARD_ENABLED,
   // Accessibility feature pod button in main view.
   VIEW_ID_ACCESSIBILITY_TRAY_ITEM,
+  // System tray AddUserButton in UserChooserView.
+  VIEW_ID_ADD_USER_BUTTON,
   VIEW_ID_BLUETOOTH_DEFAULT_VIEW,
   // System tray casting row elements.
   VIEW_ID_CAST_CAST_VIEW,
@@ -38,6 +40,15 @@ enum ViewID {
   VIEW_ID_TRAY_UPDATE_ICON,
   // System tray menu item label for updates (e.g. "Restart to update").
   VIEW_ID_TRAY_UPDATE_MENU_LABEL,
+  // System tray UserAvatarButton in TopShortcutsView.
+  VIEW_ID_USER_AVATAR_BUTTON,
+
+  // Start and end of system tray UserItemButton in UserChooserView. First
+  // user gets VIEW_ID_USER_ITEM_BUTTON_START. DCHECKs if the number of user
+  // is more than 10.
+  VIEW_ID_USER_ITEM_BUTTON_START,
+  VIEW_ID_USER_ITEM_BUTTON_END = VIEW_ID_USER_ITEM_BUTTON_START + 10,
+
   VIEW_ID_USER_VIEW_MEDIA_INDICATOR,
   // Keep alphabetized.
 };

ash/session/test_session_controller_client.cc

@@ -10,6 +10,7 @@
 #include "ash/login_status.h"
 #include "ash/session/session_controller_impl.h"
 #include "ash/session/test_pref_service_provider.h"
+#include "ash/shell.h"
 #include "base/bind.h"
 #include "base/logging.h"
 #include "base/run_loop.h"
@@ -19,6 +20,7 @@
 #include "components/prefs/pref_service.h"
 #include "components/session_manager/session_manager_types.h"
 #include "components/user_manager/user_type.h"
+#include "ui/views/widget/widget.h"
 
 namespace ash {
 
@@ -209,18 +211,9 @@ void TestSessionControllerClient::RequestSignOut() {
 
 void TestSessionControllerClient::SwitchActiveUser(
     const AccountId& account_id) {
-  std::vector<uint32_t> session_order;
-  session_order.reserve(controller_->GetUserSessions().size());
-
-  for (const auto& user_session : controller_->GetUserSessions()) {
-    if (user_session->user_info.account_id == account_id) {
-      session_order.insert(session_order.begin(), user_session->session_id);
-    } else {
-      session_order.push_back(user_session->session_id);
-    }
-  }
-
-  controller_->SetUserSessionOrder(session_order);
+  controller_->CanSwitchActiveUser(
+      base::BindOnce(&TestSessionControllerClient::DoSwitchUser,
+                     weak_ptr_factory_.GetWeakPtr(), account_id));
 }
 
 void TestSessionControllerClient::CycleActiveUser(
@@ -265,7 +258,16 @@ void TestSessionControllerClient::CycleActiveUser(
   SwitchActiveUser((*it)->user_info.account_id);
 }
 
-void TestSessionControllerClient::ShowMultiProfileLogin() {}
+void TestSessionControllerClient::ShowMultiProfileLogin() {
+  views::Widget::InitParams params;
+  params.ownership = views::Widget::InitParams::WIDGET_OWNS_NATIVE_WIDGET;
+  params.bounds = gfx::Rect(0, 0, 400, 300);
+  params.context = Shell::GetPrimaryRootWindow();
+
+  multi_profile_login_widget_ = std::make_unique<views::Widget>();
+  multi_profile_login_widget_->Init(params);
+  multi_profile_login_widget_->Show();
+}
 
 void TestSessionControllerClient::EmitAshInitialized() {}
 
@@ -278,4 +280,23 @@ PrefService* TestSessionControllerClient::GetUserPrefService(
   return prefs_provider_ ? prefs_provider_->GetUserPrefs(account_id) : nullptr;
 }
 
+void TestSessionControllerClient::DoSwitchUser(const AccountId& account_id,
+                                               bool switch_user) {
+  if (!switch_user)
+    return;
+
+  std::vector<uint32_t> session_order;
+  session_order.reserve(controller_->GetUserSessions().size());
+
+  for (const auto& user_session : controller_->GetUserSessions()) {
+    if (user_session->user_info.account_id == account_id) {
+      session_order.insert(session_order.begin(), user_session->session_id);
+    } else {
+      session_order.push_back(user_session->session_id);
+    }
+  }
+
+  controller_->SetUserSessionOrder(session_order);
+}
+
 }  // namespace ash

ash/session/test_session_controller_client.h

@@ -7,6 +7,7 @@
 
 #include <stdint.h>
 
+#include <memory>
 #include <string>
 
 #include "ash/public/cpp/session/session_controller_client.h"
@@ -17,6 +18,10 @@
 #include "base/token.h"
 #include "components/user_manager/user_type.h"
 
+namespace views {
+class Widget;
+}
+
 class AccountId;
 class PrefService;
 
@@ -114,6 +119,8 @@ class TestSessionControllerClient : public ash::SessionControllerClient {
   PrefService* GetUserPrefService(const AccountId& account_id) override;
 
  private:
+  void DoSwitchUser(const AccountId& account_id, bool switch_user);
+
   SessionControllerImpl* const controller_;
   TestPrefServiceProvider* const prefs_provider_;
 
@@ -123,6 +130,8 @@ class TestSessionControllerClient : public ash::SessionControllerClient {
   bool use_lower_case_user_id_ = true;
   int request_sign_out_count_ = 0;
 
+  std::unique_ptr<views::Widget> multi_profile_login_widget_;
+
   base::WeakPtrFactory<TestSessionControllerClient> weak_ptr_factory_{this};
 
   DISALLOW_COPY_AND_ASSIGN(TestSessionControllerClient);

ash/system/unified/top_shortcuts_view.cc

@@ -41,6 +41,7 @@ class UserAvatarButton : public views::Button {
 
 UserAvatarButton::UserAvatarButton(views::ButtonListener* listener)
     : Button(listener) {
+  SetID(VIEW_ID_USER_AVATAR_BUTTON);
   SetLayoutManager(std::make_unique<views::FillLayout>());
   SetBorder(views::CreateEmptyBorder(kUnifiedCircularButtonFocusPadding));
   AddChildView(CreateUserAvatarView(0 /* user_index */));

ash/system/unified/unified_system_tray_test_api.cc

@@ -64,7 +64,7 @@ bool UnifiedSystemTrayTestApi::IsBubbleViewVisible(int view_id,
 void UnifiedSystemTrayTestApi::ClickBubbleView(int view_id) {
   views::View* view = GetBubbleView(view_id);
   if (view && view->GetVisible()) {
-    gfx::Point cursor_location(1, 1);
+    gfx::Point cursor_location = view->GetLocalBounds().CenterPoint();
     views::View::ConvertPointToScreen(view, &cursor_location);
 
     ui::test::EventGenerator generator(GetRootWindow(view->GetWidget()));

ash/system/unified/user_chooser_detailed_view_controller.cc

@@ -56,15 +56,17 @@ void UserChooserDetailedViewController::HandleUserSwitch(int user_index) {
 
   MultiProfileUMA::RecordSwitchActiveUser(
       MultiProfileUMA::SWITCH_ACTIVE_USER_BY_TRAY);
+  tray_controller_->CloseBubble();
   controller->SwitchActiveUser(
       controller->GetUserSession(user_index)->user_info.account_id);
-  tray_controller_->CloseBubble();
+  // SwitchActiveUser may delete us.
 }
 
 void UserChooserDetailedViewController::HandleAddUserAction() {
   MultiProfileUMA::RecordSigninUser(MultiProfileUMA::SIGNIN_USER_BY_TRAY);
-  Shell::Get()->session_controller()->ShowMultiProfileLogin();
   tray_controller_->CloseBubble();
+  Shell::Get()->session_controller()->ShowMultiProfileLogin();
+  // ShowMultiProfileLogin may delete us.
 }
 
 views::View* UserChooserDetailedViewController::CreateView() {

ash/system/unified/user_chooser_detailed_view_controller_unittest.cc

+// Copyright 2019 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 "ash/public/cpp/ash_view_ids.h"
+#include "ash/public/cpp/system_tray_test_api.h"
+#include "ash/root_window_controller.h"
+#include "ash/session/session_controller_impl.h"
+#include "ash/shell.h"
+#include "ash/system/unified/unified_system_tray.h"
+#include "ash/test/ash_test_base.h"
+#include "ash/wm/overview/overview_controller.h"
+#include "base/macros.h"
+#include "components/account_id/account_id.h"
+#include "ui/views/widget/widget.h"
+
+namespace ash {
+
+namespace {
+
+AccountId GetActiveUser() {
+  return Shell::Get()
+      ->session_controller()
+      ->GetUserSession(/*user_index=*/0)
+      ->user_info.account_id;
+}
+
+}  // namespace
+
+class UserChooserDetailedViewControllerTest : public AshTestBase {
+ public:
+  UserChooserDetailedViewControllerTest() = default;
+  ~UserChooserDetailedViewControllerTest() override = default;
+
+  // AshTestBase
+  void SetUp() override {
+    AshTestBase::SetUp();
+    tray_test_api_ = ash::SystemTrayTestApi::Create();
+    tray_test_api_->DisableAnimations();
+  }
+
+  SystemTrayTestApi* tray_test_api() { return tray_test_api_.get(); }
+
+ private:
+  std::unique_ptr<SystemTrayTestApi> tray_test_api_;
+  DISALLOW_COPY_AND_ASSIGN(UserChooserDetailedViewControllerTest);
+};
+
+TEST_F(UserChooserDetailedViewControllerTest,
+       ShowMultiProfileLoginWithOverview) {
+  // Enter ovewview mode.
+  Shell::Get()->overview_controller()->ToggleOverview();
+  ASSERT_TRUE(Shell::Get()->overview_controller()->InOverviewSession());
+
+  // Show system tray.
+  tray_test_api()->ShowBubble();
+  ASSERT_TRUE(tray_test_api()->IsTrayBubbleOpen());
+
+  // Click on user avatar button to start user adding.
+  ASSERT_TRUE(tray_test_api()->IsBubbleViewVisible(VIEW_ID_USER_AVATAR_BUTTON,
+                                                   /*open_tray=*/false));
+  tray_test_api()->ClickBubbleView(VIEW_ID_USER_AVATAR_BUTTON);
+
+  // Click on add user button to show multi profile login window.
+  ASSERT_TRUE(tray_test_api()->IsBubbleViewVisible(VIEW_ID_ADD_USER_BUTTON,
+                                                   /*open_tray=*/false));
+  tray_test_api()->ClickBubbleView(VIEW_ID_ADD_USER_BUTTON);
+}
+
+TEST_F(UserChooserDetailedViewControllerTest, SwitchUserWithOverview) {
+  // Add a secondary user.
+  const AccountId secondary_user =
+      AccountId::FromUserEmail("secondary@gmail.com");
+  GetSessionControllerClient()->AddUserSession(secondary_user.GetUserEmail());
+  ASSERT_FALSE(GetActiveUser() == secondary_user);
+
+  // Create an activatable widget.
+  std::unique_ptr<views::Widget> widget = CreateTestWidget();
+
+  // Enter overview mode.
+  Shell::Get()->overview_controller()->ToggleOverview();
+  ASSERT_TRUE(Shell::Get()->overview_controller()->InOverviewSession());
+
+  // Show system tray.
+  tray_test_api()->ShowBubble();
+  ASSERT_TRUE(tray_test_api()->IsTrayBubbleOpen());
+
+  // Click on user avatar button to select user.
+  ASSERT_TRUE(tray_test_api()->IsBubbleViewVisible(VIEW_ID_USER_AVATAR_BUTTON,
+                                                   /*open_tray=*/false));
+  tray_test_api()->ClickBubbleView(VIEW_ID_USER_AVATAR_BUTTON);
+
+  const int secondary_user_button_id = VIEW_ID_USER_ITEM_BUTTON_START + 1;
+  ASSERT_TRUE(tray_test_api()->IsBubbleViewVisible(secondary_user_button_id,
+                                                   /*open_tray=*/false));
+  tray_test_api()->ClickBubbleView(secondary_user_button_id);
+
+  // Active user is switched.
+  EXPECT_TRUE(GetActiveUser() == secondary_user);
+}
+
+}  // namespace ash

ash/system/unified/user_chooser_view.cc

@@ -7,6 +7,7 @@
 #include <memory>
 #include <string>
 
+#include "ash/public/cpp/ash_view_ids.h"
 #include "ash/public/cpp/shell_window_ids.h"
 #include "ash/resources/vector_icons/vector_icons.h"
 #include "ash/session/session_controller_impl.h"
@@ -75,6 +76,7 @@ class AddUserButton : public views::Button, public views::ButtonListener {
 
 AddUserButton::AddUserButton(UserChooserDetailedViewController* controller)
     : Button(this), controller_(controller) {
+  SetID(VIEW_ID_ADD_USER_BUTTON);
   SetLayoutManager(std::make_unique<views::BoxLayout>(
       views::BoxLayout::kHorizontal, gfx::Insets(kUnifiedTopShortcutSpacing),
       kUnifiedTopShortcutSpacing));
@@ -190,6 +192,9 @@ UserItemButton::UserItemButton(int user_index,
       capture_icon_(new views::ImageView),
       name_(new views::Label),
       email_(new views::Label) {
+  DCHECK_GT(VIEW_ID_USER_ITEM_BUTTON_END,
+            VIEW_ID_USER_ITEM_BUTTON_START + user_index);
+  SetID(VIEW_ID_USER_ITEM_BUTTON_START + user_index);
   auto* layout = SetLayoutManager(std::make_unique<views::BoxLayout>(
       views::BoxLayout::kHorizontal, gfx::Insets(0, kUnifiedTopShortcutSpacing),
       kUnifiedTopShortcutSpacing));