cros: Notify CloudExternalDataPolicyObserver when policy doesn't exist

When the policy doesn't exist, an "OnExternalDataCleared" call could
be fired to notify clients, in case they still keep the old data from
a previous policy. It should be a no-op for clients that do not store
policy data.

Bug: 889108
Change-Id: I6061f3be1b650f026ea88f8d353b710fb0b3b16d
Reviewed-on: https://chromium-review.googlesource.com/c/1266695Reviewed-by: Sean Kau <skau@chromium.org>
Reviewed-by: Xiaoqian Dai <xdai@chromium.org>
Reviewed-by: Maksim Ivanov <emaxx@chromium.org>
Reviewed-by: Alexander Alekseev <alemate@chromium.org>
Commit-Queue: Wenzhao (Colin) Zang <wzang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#600430}

ash/wallpaper/wallpaper_controller.cc

@@ -1236,7 +1236,9 @@ void WallpaperController::RemoveUserWallpaper(
 void WallpaperController::RemovePolicyWallpaper(
     mojom::WallpaperUserInfoPtr user_info,
     const std::string& wallpaper_files_id) {
-  DCHECK(IsPolicyControlled(user_info->account_id, user_info->is_ephemeral));
+  if (!IsPolicyControlled(user_info->account_id, user_info->is_ephemeral))
+    return;
+
   // Updates the screen only when the user has logged in.
   const bool show_wallpaper =
       Shell::Get()->session_controller()->IsActiveUserSessionStarted();

ash/wallpaper/wallpaper_controller_unittest.cc

@@ -16,6 +16,7 @@
 #include "ash/shell_test_api.h"
 #include "ash/test/ash_test_base.h"
 #include "ash/wallpaper/wallpaper_controller_observer.h"
+#include "ash/wallpaper/wallpaper_utils/wallpaper_resizer.h"
 #include "ash/wallpaper/wallpaper_view.h"
 #include "ash/wallpaper/wallpaper_widget_controller.h"
 #include "ash/wm/window_state.h"
@@ -531,6 +532,8 @@ class WallpaperControllerTest : public AshTestBase {
     controller_->decode_requests_for_testing_.clear();
   }
 
+  void ClearWallpaper() { controller_->current_wallpaper_.reset(); }
+
   WallpaperController* controller_;  // Not owned.
 
   base::ScopedTempDir user_data_dir_;
@@ -1059,9 +1062,17 @@ TEST_F(WallpaperControllerTest, SetAndRemovePolicyWallpaper) {
   EXPECT_EQ(1, GetWallpaperCount());
   EXPECT_EQ(controller_->GetWallpaperType(), POLICY);
 
-  // Log out the user and remove the policy wallpaper. Verify the wallpaper
-  // info is reset to default and the user is no longer policy controlled.
+  // Clear the wallpaper and log out the user. Verify the policy wallpaper is
+  // shown in the login screen.
+  ClearWallpaper();
   ClearLogin();
+  controller_->ShowUserWallpaper(InitializeUser(account_id_1));
+  RunAllTasksUntilIdle();
+  EXPECT_EQ(controller_->GetWallpaperType(), POLICY);
+  EXPECT_TRUE(
+      controller_->IsPolicyControlled(account_id_1, false /*is_ephemeral=*/));
+  // Remove the policy wallpaper. Verify the wallpaper info is reset to default
+  // and the user is no longer policy controlled.
   controller_->RemovePolicyWallpaper(InitializeUser(account_id_1),
                                      wallpaper_files_id_1);
   WaitUntilCustomWallpapersDeleted(account_id_1);
@@ -1073,7 +1084,8 @@ TEST_F(WallpaperControllerTest, SetAndRemovePolicyWallpaper) {
   EXPECT_EQ(wallpaper_info, default_wallpaper_info);
   EXPECT_FALSE(
       controller_->IsPolicyControlled(account_id_1, false /*is_ephemeral=*/));
-  // Verify the wallpaper is not updated since the user hasn't logged in.
+  // Verify the wallpaper is not updated since the user hasn't logged in (to
+  // avoid abrupt wallpaper change in login screen).
   EXPECT_EQ(0, GetWallpaperCount());
   EXPECT_EQ(controller_->GetWallpaperType(), POLICY);
 
@@ -1084,6 +1096,40 @@ TEST_F(WallpaperControllerTest, SetAndRemovePolicyWallpaper) {
   EXPECT_EQ(controller_->GetWallpaperType(), DEFAULT);
 }
 
+TEST_F(WallpaperControllerTest, RemovePolicyWallpaperNoOp) {
+  auto verify_custom_wallpaper_info = [&]() {
+    EXPECT_EQ(CUSTOMIZED, controller_->GetWallpaperType());
+    EXPECT_EQ(kWallpaperColor, GetWallpaperColor());
+    WallpaperInfo wallpaper_info;
+    EXPECT_TRUE(controller_->GetUserWallpaperInfo(account_id_1, &wallpaper_info,
+                                                  false /*is_ephemeral=*/));
+    WallpaperInfo expected_wallpaper_info(
+        base::FilePath(wallpaper_files_id_1).Append(file_name_1).value(),
+        WALLPAPER_LAYOUT_CENTER, CUSTOMIZED, base::Time::Now().LocalMidnight());
+    EXPECT_EQ(expected_wallpaper_info, wallpaper_info);
+  };
+
+  // Set a custom wallpaper. Verify the user is not policy controlled and the
+  // wallpaper info is correct.
+  SimulateUserLogin(kUser1);
+  controller_->SetCustomWallpaper(
+      InitializeUser(account_id_1), wallpaper_files_id_1, file_name_1,
+      WALLPAPER_LAYOUT_CENTER, CreateImage(640, 480, kWallpaperColor),
+      false /*preview_mode=*/);
+  RunAllTasksUntilIdle();
+  EXPECT_EQ(1, GetWallpaperCount());
+  EXPECT_FALSE(
+      controller_->IsPolicyControlled(account_id_1, false /*is_ephemeral=*/));
+  verify_custom_wallpaper_info();
+
+  // Verify RemovePolicyWallpaper() is a no-op when the user doesn't have a
+  // policy wallpaper.
+  controller_->RemovePolicyWallpaper(InitializeUser(account_id_1),
+                                     wallpaper_files_id_1);
+  RunAllTasksUntilIdle();
+  verify_custom_wallpaper_info();
+}
+
 TEST_F(WallpaperControllerTest, SetThirdPartyWallpaper) {
   SetBypassDecode();
   SimulateUserLogin(kUser1);

chrome/browser/chromeos/login/users/avatar/user_image_manager_impl.cc

@@ -759,6 +759,9 @@ void UserImageManagerImpl::OnExternalDataSet(const std::string& policy) {
 
 void UserImageManagerImpl::OnExternalDataCleared(const std::string& policy) {
   DCHECK_EQ(policy::key::kUserAvatarImage, policy);
+  if (!IsUserImageManaged())
+    return;
+
   has_managed_image_ = false;
   SetInitialUserImage();
   TryToCreateImageSyncObserver();

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

@@ -383,29 +383,6 @@ IN_PROC_BROWSER_TEST_F(WallpaperPolicyTest, DISABLED_SetResetClear) {
   ASSERT_EQ(3, wallpaper_change_count_);
 }
 
-IN_PROC_BROWSER_TEST_F(WallpaperPolicyTest, PRE_PRE_PersistOverLogout) {
-  SetSystemSalt();
-  RegisterUser(testUsers_[0]);
-  StartupUtils::MarkOobeCompleted();
-}
-
-IN_PROC_BROWSER_TEST_F(WallpaperPolicyTest, PRE_PersistOverLogout) {
-  SetSystemSalt();
-  LoginUser(testUsers_[0]);
-
-  // Set wallpaper policy to red image.
-  InjectPolicy(0, kRedImageFileName);
-
-  // Run until wallpaper has changed to expected color.
-  RunUntilWallpaperChangeToColor(kRedImageColor);
-  StartupUtils::MarkOobeCompleted();
-}
-
-IN_PROC_BROWSER_TEST_F(WallpaperPolicyTest, PersistOverLogout) {
-  LoginUser(testUsers_[0]);
-  RunUntilWallpaperChangeToColor(kRedImageColor);
-}
-
 IN_PROC_BROWSER_TEST_F(WallpaperPolicyTest, PRE_DevicePolicyTest) {
   SetSystemSalt();
   RegisterUser(testUsers_[0]);

chrome/browser/chromeos/policy/cloud_external_data_policy_observer.cc

@@ -74,8 +74,9 @@ CloudExternalDataPolicyObserver::PolicyServiceObserver::PolicyServiceObserver(
     const PolicyMap::Entry* entry = policy_service_->GetPolicies(
         PolicyNamespace(POLICY_DOMAIN_CHROME, std::string()))
             .Get(parent_->policy_);
-    if (entry)
-      parent_->HandleExternalDataPolicyUpdate(user_id_, entry);
+    // Notify |parent_| even when |entry| is null (i.e. the policy never existed
+    // or once existed but was cleared later).
+    parent_->HandleExternalDataPolicyUpdate(user_id_, entry);
   }
 }