Show a notification the detachable base needs an update

If hammerd detects that the attached detachable base needs a frimware
update (which is applied on device boot), show the user a system
notification asking them to reboot the device in order to apply the
update.
The notification is closed if the update succeeds, or if the detachable
base gets detached (i.e. the Chrome OS device switches to tablet mode).

Note that, unlike detachable base switch notification, this notification
should be shown on login and lock screen (because login and lock UI do
not display error bubbles in this case), and is thus set as a system
priority notification.

BUG=765416

Change-Id: Ie8d5d30985aab4d0d85e3b35ebb147d212f92658
Reviewed-on: https://chromium-review.googlesource.com/957468
Commit-Queue: Toni Barzic <tbarzic@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#543113}

ash/detachable_base/detachable_base_handler.cc

@@ -147,11 +147,15 @@ void DetachableBaseHandler::OnLocalStatePrefServiceInitialized(
     NotifyPairingStatusChanged();
 }
 
-void DetachableBaseHandler::BaseFirmwareUpdateNeeded() {}
+void DetachableBaseHandler::BaseFirmwareUpdateNeeded() {
+  NotifyBaseRequiresFirmwareUpdate(true /*requires_update*/);
+}
 
 void DetachableBaseHandler::BaseFirmwareUpdateStarted() {}
 
-void DetachableBaseHandler::BaseFirmwareUpdateSucceeded() {}
+void DetachableBaseHandler::BaseFirmwareUpdateSucceeded() {
+  NotifyBaseRequiresFirmwareUpdate(false /*requires_update*/);
+}
 
 void DetachableBaseHandler::BaseFirmwareUpdateFailed() {}
 
@@ -211,6 +215,7 @@ void DetachableBaseHandler::UpdateTabletMode(
   if (*tablet_mode_ == chromeos::PowerManagerClient::TabletMode::ON) {
     authenticated_base_id_.clear();
     pairing_status_ = DetachableBasePairingStatus::kNone;
+    NotifyBaseRequiresFirmwareUpdate(false /*requires_update*/);
   }
 
   if (GetPairingStatus() != old_pairing_status)
@@ -245,4 +250,10 @@ void DetachableBaseHandler::NotifyPairingStatusChanged() {
     observer.OnDetachableBasePairingStatusChanged(GetPairingStatus());
 }
 
+void DetachableBaseHandler::NotifyBaseRequiresFirmwareUpdate(
+    bool requires_update) {
+  for (auto& observer : observers_)
+    observer.OnDetachableBaseRequiresUpdateChanged(requires_update);
+}
+
 }  // namespace ash

ash/detachable_base/detachable_base_handler.h

@@ -123,6 +123,10 @@ class ASH_EXPORT DetachableBaseHandler
   // Notifies observers that the detachable base pairing state has changed.
   void NotifyPairingStatusChanged();
 
+  // Notifies observers about whether the detachable base requires a firmware
+  // update.
+  void NotifyBaseRequiresFirmwareUpdate(bool requires_update);
+
   PrefService* local_state_ = nullptr;
 
   // The shell that owns |this| - used to listen for local state initialization.

ash/detachable_base/detachable_base_handler_unittest.cc

@@ -22,7 +22,7 @@
 #include "components/signin/core/account_id/account_id.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
-using ash::DetachableBasePairingStatus;
+namespace ash {
 
 namespace {
 
@@ -31,36 +31,49 @@ enum class UserType {
   kEphemeral,
 };
 
-ash::mojom::UserInfoPtr CreateUser(const std::string& email,
+mojom::UserInfoPtr CreateUser(const std::string& email,
                               const std::string& gaia_id,
                               UserType user_type) {
-  ash::mojom::UserInfoPtr user = ash::mojom::UserInfo::New();
+  mojom::UserInfoPtr user = mojom::UserInfo::New();
   user->account_id = AccountId::FromUserEmailGaiaId(email, gaia_id);
   user->is_ephemeral = user_type == UserType::kEphemeral;
   return user;
 }
 
-class TestBaseObserver : public ash::DetachableBaseObserver {
+class TestBaseObserver : public DetachableBaseObserver {
  public:
   TestBaseObserver() = default;
   ~TestBaseObserver() override = default;
 
-  int pairing_status_changed_count() { return pairing_status_changed_count_; }
+  int pairing_status_changed_count() const {
+    return pairing_status_changed_count_;
+  }
 
   void reset_pairing_status_changed_count() {
     pairing_status_changed_count_ = 0;
   }
 
-  // ash::DetachableBaseObserver:
+  int update_required_changed_count() const {
+    return update_required_changed_count_;
+  }
+
+  bool requires_update() const { return requires_update_; }
+
+  // DetachableBaseObserver:
   void OnDetachableBasePairingStatusChanged(
       DetachableBasePairingStatus status) override {
     pairing_status_changed_count_++;
   }
 
- private:
-  base::test::ScopedTaskEnvironment task_environment_;
+  void OnDetachableBaseRequiresUpdateChanged(bool requires_update) override {
+    update_required_changed_count_++;
+    requires_update_ = requires_update;
+  }
 
+ private:
   int pairing_status_changed_count_ = 0;
+  int update_required_changed_count_ = 0;
+  bool requires_update_ = false;
 
   DISALLOW_COPY_AND_ASSIGN(TestBaseObserver);
 };
@@ -94,8 +107,8 @@ class DetachableBaseHandlerTest : public testing::Test {
 
     default_user_ = CreateUser("user_1@foo.bar", "111111", UserType::kNormal);
 
-    ash::DetachableBaseHandler::RegisterPrefs(local_state_.registry());
-    handler_ = std::make_unique<ash::DetachableBaseHandler>(nullptr);
+    DetachableBaseHandler::RegisterPrefs(local_state_.registry());
+    handler_ = std::make_unique<DetachableBaseHandler>(nullptr);
     handler_->OnLocalStatePrefServiceInitialized(&local_state_);
     handler_->AddObserver(&detachable_base_observer_);
   }
@@ -122,14 +135,14 @@ class DetachableBaseHandlerTest : public testing::Test {
 
   void RestartHandler() {
     handler_->RemoveObserver(&detachable_base_observer_);
-    handler_ = std::make_unique<ash::DetachableBaseHandler>(nullptr);
+    handler_ = std::make_unique<DetachableBaseHandler>(nullptr);
     handler_->OnLocalStatePrefServiceInitialized(&local_state_);
     handler_->AddObserver(&detachable_base_observer_);
   }
 
   void ResetHandlerWithNoLocalState() {
     handler_->RemoveObserver(&detachable_base_observer_);
-    handler_ = std::make_unique<ash::DetachableBaseHandler>(nullptr);
+    handler_ = std::make_unique<DetachableBaseHandler>(nullptr);
     handler_->AddObserver(&detachable_base_observer_);
   }
 
@@ -144,11 +157,13 @@ class DetachableBaseHandlerTest : public testing::Test {
 
   TestBaseObserver detachable_base_observer_;
 
-  std::unique_ptr<ash::DetachableBaseHandler> handler_;
+  std::unique_ptr<DetachableBaseHandler> handler_;
 
-  ash::mojom::UserInfoPtr default_user_;
+  mojom::UserInfoPtr default_user_;
 
  private:
+  base::test::ScopedTaskEnvironment task_environment_;
+
   TestingPrefServiceSimple local_state_;
 
   DISALLOW_COPY_AND_ASSIGN(DetachableBaseHandlerTest);
@@ -407,7 +422,7 @@ TEST_F(DetachableBaseHandlerTest, MultiUser) {
   RestartHandler();
   base::RunLoop().RunUntilIdle();
 
-  const ash::mojom::UserInfoPtr second_user =
+  const mojom::UserInfoPtr second_user =
       CreateUser("user_2@foo.bar", "222222", UserType::kNormal);
 
   EXPECT_EQ(DetachableBasePairingStatus::kNone, handler_->GetPairingStatus());
@@ -476,7 +491,7 @@ TEST_F(DetachableBaseHandlerTest, SwitchToNonAuthenticatedBase) {
 
   hammerd_client_->FirePairChallengeFailedSignal();
 
-  const ash::mojom::UserInfoPtr second_user =
+  const mojom::UserInfoPtr second_user =
       CreateUser("user_2@foo.bar", "222222", UserType::kNormal);
 
   EXPECT_EQ(DetachableBasePairingStatus::kNotAuthenticated,
@@ -505,7 +520,7 @@ TEST_F(DetachableBaseHandlerTest, SwitchToInvalidBase) {
 
   hammerd_client_->FireInvalidBaseConnectedSignal();
 
-  const ash::mojom::UserInfoPtr second_user =
+  const mojom::UserInfoPtr second_user =
       CreateUser("user_2@foo.bar", "222222", UserType::kNormal);
 
   EXPECT_EQ(DetachableBasePairingStatus::kInvalidDevice,
@@ -517,7 +532,7 @@ TEST_F(DetachableBaseHandlerTest, SwitchToInvalidBase) {
 }
 
 TEST_F(DetachableBaseHandlerTest, RemoveUserData) {
-  const ash::mojom::UserInfoPtr second_user =
+  const mojom::UserInfoPtr second_user =
       CreateUser("user_2@foo.bar", "222222", UserType::kNormal);
 
   // Assume the user_1 has a last used base.
@@ -600,7 +615,7 @@ TEST_F(DetachableBaseHandlerTest, NoLocalState) {
 TEST_F(DetachableBaseHandlerTest, EphemeralUser) {
   base::RunLoop().RunUntilIdle();
 
-  const ash::mojom::UserInfoPtr ephemeral_user =
+  const mojom::UserInfoPtr ephemeral_user =
       CreateUser("user_3@foo.bar", "333333", UserType::kEphemeral);
   hammerd_client_->FirePairChallengeSucceededSignal({0x01, 0x02, 0x03, 0x04});
   handler_->SetPairedBaseAsLastUsedByUser(*ephemeral_user);
@@ -635,3 +650,26 @@ TEST_F(DetachableBaseHandlerTest, EphemeralUser) {
   EXPECT_TRUE(handler_->PairedBaseMatchesLastUsedByUser(*ephemeral_user));
   detachable_base_observer_.reset_pairing_status_changed_count();
 }
+
+TEST_F(DetachableBaseHandlerTest, NotifyObserversWhenBaseUpdateRequired) {
+  hammerd_client_->FireBaseFirmwareNeedUpdateSignal();
+  EXPECT_EQ(1, detachable_base_observer_.update_required_changed_count());
+  EXPECT_TRUE(detachable_base_observer_.requires_update());
+
+  hammerd_client_->FireBaseFirmwareUpdateSucceededSignal();
+  EXPECT_EQ(2, detachable_base_observer_.update_required_changed_count());
+  EXPECT_FALSE(detachable_base_observer_.requires_update());
+}
+
+TEST_F(DetachableBaseHandlerTest, NotifyNoUpdateRequiredOnBaseDetach) {
+  hammerd_client_->FireBaseFirmwareNeedUpdateSignal();
+  EXPECT_EQ(1, detachable_base_observer_.update_required_changed_count());
+  EXPECT_TRUE(detachable_base_observer_.requires_update());
+
+  power_manager_client_->SetTabletMode(
+      chromeos::PowerManagerClient::TabletMode::ON, base::TimeTicks());
+  EXPECT_EQ(2, detachable_base_observer_.update_required_changed_count());
+  EXPECT_FALSE(detachable_base_observer_.requires_update());
+}
+
+}  // namespace ash

ash/detachable_base/detachable_base_notification_controller.cc

@@ -30,6 +30,10 @@ constexpr char kDetachableBaseNotifierId[] = "ash.system.detachable_base";
 const char DetachableBaseNotificationController::kBaseChangedNotificationId[] =
     "chrome://settings/detachable_base/detachable_base_changed";
 
+const char
+    DetachableBaseNotificationController::kBaseRequiresUpdateNotificationId[] =
+        "chrome://settings/detachable_base/detachable_base_requires_update";
+
 DetachableBaseNotificationController::DetachableBaseNotificationController(
     DetachableBaseHandler* detachable_base_handler)
     : detachable_base_handler_(detachable_base_handler),
@@ -47,6 +51,37 @@ void DetachableBaseNotificationController::OnDetachableBasePairingStatusChanged(
   ShowPairingNotificationIfNeeded();
 }
 
+void DetachableBaseNotificationController::
+    OnDetachableBaseRequiresUpdateChanged(bool requires_update) {
+  if (!requires_update) {
+    RemoveUpdateRequiredNotification();
+    return;
+  }
+
+  base::string16 title = l10n_util::GetStringUTF16(
+      IDS_ASH_DETACHABLE_BASE_NOTIFICATION_UPDATE_NEEDED_TITLE);
+  base::string16 message = l10n_util::GetStringUTF16(
+      IDS_ASH_DETACHABLE_BASE_NOTIFICATION_UPDATE_NEEDED_MESSAGE);
+
+  std::unique_ptr<message_center::Notification> notification =
+      message_center::Notification::CreateSystemNotification(
+          message_center::NOTIFICATION_TYPE_SIMPLE,
+          kBaseRequiresUpdateNotificationId, title, message, gfx::Image(),
+          base::string16(), GURL(),
+          message_center::NotifierId(
+              message_center::NotifierId::SYSTEM_COMPONENT,
+              kDetachableBaseNotifierId),
+          message_center::RichNotificationData(), nullptr,
+          kNotificationWarningIcon,
+          message_center::SystemNotificationWarningLevel::CRITICAL_WARNING);
+  // Set system priority so the notification gets shown when the user session is
+  // blocked.
+  notification->SetSystemPriority();
+
+  message_center::MessageCenter::Get()->AddNotification(
+      std::move(notification));
+}
+
 void DetachableBaseNotificationController::OnActiveUserSessionChanged(
     const AccountId& account_id) {
   // Remove notification shown for the provious user.
@@ -128,4 +163,9 @@ void DetachableBaseNotificationController::RemovePairingNotification() {
       kBaseChangedNotificationId, false);
 }
 
+void DetachableBaseNotificationController::RemoveUpdateRequiredNotification() {
+  message_center::MessageCenter::Get()->RemoveNotification(
+      kBaseRequiresUpdateNotificationId, false);
+}
+
 }  // namespace ash

ash/detachable_base/detachable_base_notification_controller.h

@@ -22,11 +22,16 @@ class DetachableBaseHandler;
 //     tracking the user's key strokes).
 //   * when the attached base could not be authenticated, it warns the user that
 //     the base may not be trusted.
+//   * when the detachable base's firmware has to be updated, it shows a
+//     notification asking user to reboot in order to update the base. This
+//     notification will be shown regardless of the user session state - it will
+//     show up on both the lock and the login screen.
 class ASH_EXPORT DetachableBaseNotificationController
     : public DetachableBaseObserver,
       public SessionObserver {
  public:
   static const char kBaseChangedNotificationId[];
+  static const char kBaseRequiresUpdateNotificationId[];
 
   explicit DetachableBaseNotificationController(
       DetachableBaseHandler* detachable_base_handler);
@@ -35,6 +40,7 @@ class ASH_EXPORT DetachableBaseNotificationController
   // DetachableBaseObserver:
   void OnDetachableBasePairingStatusChanged(
       DetachableBasePairingStatus pairing_status) override;
+  void OnDetachableBaseRequiresUpdateChanged(bool requires_update) override;
 
   // SessionObserver:
   void OnActiveUserSessionChanged(const AccountId& account_id) override;
@@ -50,6 +56,9 @@ class ASH_EXPORT DetachableBaseNotificationController
   // within the current session.
   void RemovePairingNotification();
 
+  // Removes kBaseRequiresUpdateNotificationId if it was previously shown.
+  void RemoveUpdateRequiredNotification();
+
   DetachableBaseHandler* detachable_base_handler_;
 
   ScopedObserver<DetachableBaseHandler, DetachableBaseObserver>

ash/detachable_base/detachable_base_notification_controller_unittest.cc

@@ -55,6 +55,12 @@ class DetachableBaseNotificationControllerTest : public NoSessionAshTestBase {
         DetachableBaseNotificationController::kBaseChangedNotificationId);
   }
 
+  bool IsBaseRequiresUpdateNotificationVisible() {
+    return message_center::MessageCenter::Get()->FindVisibleNotificationById(
+        DetachableBaseNotificationController::
+            kBaseRequiresUpdateNotificationId);
+  }
+
   void CloseBaseChangedNotification() {
     message_center::MessageCenter::Get()->RemoveNotification(
         DetachableBaseNotificationController::kBaseChangedNotificationId,
@@ -231,4 +237,48 @@ TEST_F(DetachableBaseNotificationControllerTest,
   EXPECT_TRUE(IsBaseChangedNotificationVisible());
 }
 
+TEST_F(DetachableBaseNotificationControllerTest, NotificationOnUpdateRequired) {
+  CreateUserSessions(1);
+
+  detachable_base_handler()->BaseFirmwareUpdateNeeded();
+  EXPECT_TRUE(IsBaseRequiresUpdateNotificationVisible());
+
+  // The notification should be removed when the base gets detached.
+  GetPowerManagerClient()->SetTabletMode(
+      chromeos::PowerManagerClient::TabletMode::ON, base::TimeTicks());
+  EXPECT_FALSE(IsBaseRequiresUpdateNotificationVisible());
+}
+
+TEST_F(DetachableBaseNotificationControllerTest,
+       NotificationOnUpdateRequiredBeforeLogin) {
+  // Update requirement detected before login - expect the update required
+  // notification to be shown.
+  detachable_base_handler()->BaseFirmwareUpdateNeeded();
+  EXPECT_TRUE(IsBaseRequiresUpdateNotificationVisible());
+
+  // Login, expect the notification to still be there.
+  CreateUserSessions(1);
+  EXPECT_TRUE(IsBaseRequiresUpdateNotificationVisible());
+
+  // The notification should be removed when the base gets detached.
+  GetPowerManagerClient()->SetTabletMode(
+      chromeos::PowerManagerClient::TabletMode::ON, base::TimeTicks());
+  EXPECT_FALSE(IsBaseRequiresUpdateNotificationVisible());
+}
+
+TEST_F(DetachableBaseNotificationControllerTest,
+       NotificationOnUpdateRequiredOnLockScreen) {
+  // Update requirement detected while the session is blocked by the lock
+  // screen - expect the update required notification to be shown.
+  BlockUserSession(UserSessionBlockReason::BLOCKED_BY_LOCK_SCREEN);
+
+  detachable_base_handler()->BaseFirmwareUpdateNeeded();
+  EXPECT_TRUE(IsBaseRequiresUpdateNotificationVisible());
+
+  // The notification should be removed when the base gets detached.
+  GetPowerManagerClient()->SetTabletMode(
+      chromeos::PowerManagerClient::TabletMode::ON, base::TimeTicks());
+  EXPECT_FALSE(IsBaseRequiresUpdateNotificationVisible());
+}
+
 }  // namespace ash

ash/detachable_base/detachable_base_observer.h

@@ -20,6 +20,11 @@ class ASH_EXPORT DetachableBaseObserver {
   // detached.
   virtual void OnDetachableBasePairingStatusChanged(
       DetachableBasePairingStatus status) = 0;
+
+  // Called when the state of whether the current detachable base requires a
+  // firmware update changes.
+  // |requires_update|: Whether the base currently requires a firmware update.
+  virtual void OnDetachableBaseRequiresUpdateChanged(bool requires_update) = 0;
 };
 
 }  // namespace ash

ash/login/ui/login_detachable_base_model.cc

@@ -47,6 +47,7 @@ class LoginDetachableBaseModelImpl : public LoginDetachableBaseModel,
       DetachableBasePairingStatus pairing_status) override {
     login_data_dispatcher_->SetDetachableBasePairingStatus(pairing_status);
   }
+  void OnDetachableBaseRequiresUpdateChanged(bool requires_update) override {}
 
  private:
   DetachableBaseHandler* detachable_base_handler_;