Reland "Changing CupsPrintersManager to use Enterprise Printing Policy"

This reverts commit 11df4514.

Hooked up CupsPrintersManager to receive update callbacks whenever the
value of the UserPrintersAllowed pref is changed. If the pref is set to
false then CupsPrintersManager will disallow any changes made to
non-enterprise printers.

Bug: 827016

Change-Id: Ibcb047f12e0db6414a25e32e5c9909608979c828
Reviewed-on: https://chromium-review.googlesource.com/992857
Commit-Queue: David Valleau <valleau@chromium.org>
Reviewed-by: Sean Kau <skau@chromium.org>
Cr-Commit-Position: refs/heads/master@{#547968}

chrome/browser/chromeos/printing/cups_printers_manager.cc

@@ -25,6 +25,8 @@
 #include "chrome/common/pref_names.h"
 #include "components/policy/policy_constants.h"
 #include "components/pref_registry/pref_registry_syncable.h"
+#include "components/prefs/pref_member.h"
+#include "components/prefs/pref_service.h"
 
 namespace chromeos {
 namespace {
@@ -94,7 +96,8 @@ class CupsPrintersManagerImpl : public CupsPrintersManager,
                           std::unique_ptr<PrinterDetector> usb_detector,
                           std::unique_ptr<PrinterDetector> zeroconf_detector,
                           scoped_refptr<PpdProvider> ppd_provider,
-                          PrinterEventTracker* event_tracker)
+                          PrinterEventTracker* event_tracker,
+                          PrefService* pref_service)
       : synced_printers_manager_(synced_printers_manager),
         synced_printers_manager_observer_(this),
         usb_detector_(std::move(usb_detector)),
@@ -120,6 +123,9 @@ class CupsPrintersManagerImpl : public CupsPrintersManager,
         std::make_unique<PrinterDetectorObserverProxy>(
             this, kZeroconfDetector, zeroconf_detector_.get());
     OnPrintersFound(kZeroconfDetector, zeroconf_detector_->GetPrinters());
+
+    native_printers_allowed_.Init(prefs::kUserNativePrintersAllowed,
+                                  pref_service);
   }
 
   ~CupsPrintersManagerImpl() override = default;
@@ -127,6 +133,12 @@ class CupsPrintersManagerImpl : public CupsPrintersManager,
   // Public API function.
   std::vector<Printer> GetPrinters(PrinterClass printer_class) const override {
     DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_);
+    if (!native_printers_allowed_.GetValue() && printer_class != kEnterprise) {
+      // If native printers are disabled then simply return an empty vector.
+      LOG(WARNING) << "Attempting to retrieve native printers when "
+                      "UserNativePrintersAllowed is set to false";
+      return {};
+    }
     return printers_.at(printer_class);
   }
 
@@ -142,6 +154,11 @@ class CupsPrintersManagerImpl : public CupsPrintersManager,
   // Public API function.
   void UpdateConfiguredPrinter(const Printer& printer) override {
     DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_);
+    if (!native_printers_allowed_.GetValue()) {
+      LOG(WARNING) << "UpdateConfiguredPrinter() called when "
+                      "UserNativePrintersAllowed is set to false";
+      return;
+    }
     // If this is an 'add' instead of just an update, record the event.
     MaybeRecordInstallation(printer);
     synced_printers_manager_->UpdateConfiguredPrinter(printer);
@@ -176,6 +193,11 @@ class CupsPrintersManagerImpl : public CupsPrintersManager,
   // Public API function.
   void PrinterInstalled(const Printer& printer) override {
     DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_);
+    if (!native_printers_allowed_.GetValue()) {
+      LOG(WARNING) << "PrinterInstalled() called when "
+                      "UserNativePrintersAllowed is  set to false";
+      return;
+    }
     MaybeRecordInstallation(printer);
     synced_printers_manager_->PrinterInstalled(printer);
   }
@@ -192,6 +214,12 @@ class CupsPrintersManagerImpl : public CupsPrintersManager,
   // more than just this function.
   std::unique_ptr<Printer> GetPrinter(const std::string& id) const override {
     DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_);
+    if (!native_printers_allowed_.GetValue()) {
+      LOG(WARNING) << "UserNativePrintersAllowed is disabled - only searching "
+                      "enterprise printers";
+      return GetEnterprisePrinter(id);
+    }
+
     for (const auto& printer_list : printers_) {
       for (const auto& printer : printer_list) {
         if (printer.id() == id) {
@@ -246,6 +274,15 @@ class CupsPrintersManagerImpl : public CupsPrintersManager,
   }
 
  private:
+  std::unique_ptr<Printer> GetEnterprisePrinter(const std::string& id) const {
+    for (const auto& printer : printers_[kEnterprise]) {
+      if (printer.id() == id) {
+        return std::make_unique<Printer>(printer);
+      }
+    }
+    return nullptr;
+  }
+
   // Notify observers on the given classes the the relevant lists have changed.
   void NotifyObservers(
       const std::vector<CupsPrintersManager::PrinterClass>& printer_classes) {
@@ -508,6 +545,9 @@ class CupsPrintersManagerImpl : public CupsPrintersManager,
 
   base::ObserverList<CupsPrintersManager::Observer> observer_list_;
 
+  // Holds the current value of the pref |UserNativePrintersAllowed|.
+  BooleanPrefMember native_printers_allowed_;
+
   base::WeakPtrFactory<CupsPrintersManagerImpl> weak_ptr_factory_;
 };
 
@@ -527,7 +567,8 @@ std::unique_ptr<CupsPrintersManager> CupsPrintersManager::Create(
           profile),
       UsbPrinterDetector::Create(), ZeroconfPrinterDetector::Create(),
       CreatePpdProvider(profile),
-      PrinterEventTrackerFactory::GetInstance()->GetForBrowserContext(profile));
+      PrinterEventTrackerFactory::GetInstance()->GetForBrowserContext(profile),
+      profile->GetPrefs());
 }
 
 // static
@@ -536,10 +577,12 @@ std::unique_ptr<CupsPrintersManager> CupsPrintersManager::Create(
     std::unique_ptr<PrinterDetector> usb_detector,
     std::unique_ptr<PrinterDetector> zeroconf_detector,
     scoped_refptr<PpdProvider> ppd_provider,
-    PrinterEventTracker* event_tracker) {
+    PrinterEventTracker* event_tracker,
+    PrefService* pref_service) {
   return std::make_unique<CupsPrintersManagerImpl>(
       synced_printers_manager, std::move(usb_detector),
-      std::move(zeroconf_detector), std::move(ppd_provider), event_tracker);
+      std::move(zeroconf_detector), std::move(ppd_provider), event_tracker,
+      pref_service);
 }
 
 // static

chrome/browser/chromeos/printing/cups_printers_manager.h

@@ -11,6 +11,7 @@
 #include "base/memory/ref_counted.h"
 #include "chrome/browser/chromeos/printing/printer_event_tracker.h"
 #include "chromeos/printing/printer_configuration.h"
+#include "components/prefs/pref_service.h"
 
 class Profile;
 
@@ -57,7 +58,8 @@ class CupsPrintersManager {
       std::unique_ptr<PrinterDetector> usb_printer_detector,
       std::unique_ptr<PrinterDetector> zeroconf_printer_detector,
       scoped_refptr<PpdProvider> ppd_provider,
-      PrinterEventTracker* event_tracker);
+      PrinterEventTracker* event_tracker,
+      PrefService* pref_service);
 
   // Register the printing preferences with the |registry|.
   static void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry);

chrome/browser/chromeos/printing/cups_printers_manager_unittest.cc

@@ -16,6 +16,8 @@
 #include "base/threading/sequenced_task_runner_handle.h"
 #include "chrome/browser/chromeos/printing/synced_printers_manager.h"
 #include "chrome/browser/chromeos/printing/usb_printer_detector.h"
+#include "chrome/common/pref_names.h"
+#include "components/sync_preferences/testing_pref_service_syncable.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace chromeos {
@@ -257,9 +259,14 @@ class CupsPrintersManagerTest : public testing::Test,
     zeroconf_detector_ = zeroconf_detector.get();
     auto usb_detector = std::make_unique<FakePrinterDetector>();
     usb_detector_ = usb_detector.get();
+
+    // Register the pref |UserNativePrintersAllowed|
+    CupsPrintersManager::RegisterProfilePrefs(pref_service_.registry());
+
     manager_ = CupsPrintersManager::Create(
         &synced_printers_manager_, std::move(usb_detector),
-        std::move(zeroconf_detector), ppd_provider_, &event_tracker_);
+        std::move(zeroconf_detector), ppd_provider_, &event_tracker_,
+        &pref_service_);
     manager_->AddObserver(this);
   }
 
@@ -280,6 +287,12 @@ class CupsPrintersManagerTest : public testing::Test,
     ExpectPrinterIdsAre(observed_printers_.at(printer_class), ids);
   }
 
+  void UpdatePolicyValue(const char* name, bool value) {
+    auto value_ptr = std::make_unique<base::Value>(value);
+    // TestingPrefSyncableService assumes ownership of |value_ptr|.
+    pref_service_.SetManagedPref(name, std::move(value_ptr));
+  }
+
  protected:
   base::test::ScopedTaskEnvironment scoped_task_environment_;
 
@@ -295,6 +308,10 @@ class CupsPrintersManagerTest : public testing::Test,
   // This is unused, it's just here for memory ownership.
   PrinterEventTracker event_tracker_;
 
+  // PrefService used to register the |UserNativePrintersAllowed| pref and
+  // change its value for testing.
+  sync_preferences::TestingPrefServiceSyncable pref_service_;
+
   // The manager being tested.  This must be declared after the fakes, as its
   // initialization must come after that of the fakes.
   std::unique_ptr<CupsPrintersManager> manager_;
@@ -474,5 +491,108 @@ TEST_F(CupsPrintersManagerTest, GetPrinter) {
   EXPECT_EQ(printer, nullptr);
 }
 
+// Test that if |UserNativePrintersAllowed| pref is set to false, then
+// GetPrinters() will only return printers from
+// |CupsPrintersManager::kEnterprise|.
+TEST_F(CupsPrintersManagerTest, GetPrintersUserNativePrintersDisabled) {
+  synced_printers_manager_.AddConfiguredPrinters({Printer("Configured")});
+  synced_printers_manager_.AddEnterprisePrinters({Printer("Enterprise")});
+  scoped_task_environment_.RunUntilIdle();
+
+  // Disable the use of non-enterprise printers.
+  UpdatePolicyValue(prefs::kUserNativePrintersAllowed, false);
+
+  // Verify that non-enterprise printers are not returned by GetPrinters()
+  std::vector<Printer> configured_printers =
+      manager_->GetPrinters(CupsPrintersManager::kConfigured);
+  ExpectPrinterIdsAre(configured_printers, {});
+
+  // Verify that enterprise printers are returned by GetPrinters()
+  std::vector<Printer> enterprise_printers =
+      manager_->GetPrinters(CupsPrintersManager::kEnterprise);
+  ExpectPrinterIdsAre(enterprise_printers, {"Enterprise"});
+}
+
+// Test that if |UserNativePrintersAllowed| pref is set to false, then
+// UpdateConfiguredPrinter() will simply do nothing.
+TEST_F(CupsPrintersManagerTest,
+       UpdateConfiguredPrinterUserNativePrintersDisabled) {
+  // Start by installing a configured printer to be used to test than any
+  // changes made to the printer will not be propogated.
+  Printer existing_configured("Configured");
+  synced_printers_manager_.AddConfiguredPrinters({existing_configured});
+  usb_detector_->AddDetections({MakeDiscoveredPrinter("Discovered")});
+  zeroconf_detector_->AddDetections({MakeAutomaticPrinter("Automatic")});
+  scoped_task_environment_.RunUntilIdle();
+
+  // Sanity check that we do, indeed, have one printer in each class.
+  ExpectPrintersInClassAre(CupsPrintersManager::kConfigured, {"Configured"});
+  ExpectPrintersInClassAre(CupsPrintersManager::kAutomatic, {"Automatic"});
+  ExpectPrintersInClassAre(CupsPrintersManager::kDiscovered, {"Discovered"});
+
+  // Disable the use of non-enterprise printers.
+  UpdatePolicyValue(prefs::kUserNativePrintersAllowed, false);
+
+  // Update the existing configured printer. Verify that the changes did not
+  // progogate.
+  existing_configured.set_display_name("New Display Name");
+  manager_->UpdateConfiguredPrinter(existing_configured);
+  scoped_task_environment_.RunUntilIdle();
+
+  // Reenable user printers in order to do checking.
+  UpdatePolicyValue(prefs::kUserNativePrintersAllowed, true);
+  ExpectPrintersInClassAre(CupsPrintersManager::kConfigured, {"Configured"});
+  EXPECT_EQ(
+      manager_->GetPrinters(CupsPrintersManager::kConfigured)[0].display_name(),
+      "");
+  UpdatePolicyValue(prefs::kUserNativePrintersAllowed, false);
+
+  // Attempt to update the Automatic and Discovered printers. In both cases
+  // check that the printers do not move into the configured category.
+  manager_->UpdateConfiguredPrinter(Printer("Automatic"));
+  scoped_task_environment_.RunUntilIdle();
+  UpdatePolicyValue(prefs::kUserNativePrintersAllowed, true);
+  ExpectPrintersInClassAre(CupsPrintersManager::kAutomatic, {"Automatic"});
+  ExpectPrintersInClassAre(CupsPrintersManager::kConfigured, {"Configured"});
+  UpdatePolicyValue(prefs::kUserNativePrintersAllowed, false);
+
+  manager_->UpdateConfiguredPrinter(Printer("Discovered"));
+  scoped_task_environment_.RunUntilIdle();
+  UpdatePolicyValue(prefs::kUserNativePrintersAllowed, true);
+  ExpectPrintersInClassAre(CupsPrintersManager::kDiscovered, {"Discovered"});
+  ExpectPrintersInClassAre(CupsPrintersManager::kConfigured, {"Configured"});
+  UpdatePolicyValue(prefs::kUserNativePrintersAllowed, false);
+
+  // Attempt to update a printer that we haven't seen before, check that nothing
+  // changed.
+  manager_->UpdateConfiguredPrinter(Printer("NewFangled"));
+  scoped_task_environment_.RunUntilIdle();
+  UpdatePolicyValue(prefs::kUserNativePrintersAllowed, true);
+  ExpectPrintersInClassAre(CupsPrintersManager::kConfigured, {"Configured"});
+}
+
+// Test that if |UserNativePrintersAllowed| pref is set to false GetPrinter only
+// returns a printer when the given printer id corresponds to an enterprise
+// printer. Otherwise, it returns nothing.
+TEST_F(CupsPrintersManagerTest, GetPrinterUserNativePrintersDisabled) {
+  synced_printers_manager_.AddConfiguredPrinters({Printer("Configured")});
+  synced_printers_manager_.AddEnterprisePrinters({Printer("Enterprise")});
+  scoped_task_environment_.RunUntilIdle();
+
+  // Sanity check that the printers were added.
+  ExpectPrintersInClassAre(CupsPrintersManager::kConfigured, {"Configured"});
+  ExpectPrintersInClassAre(CupsPrintersManager::kEnterprise, {"Enterprise"});
+
+  // Diable the use of non-enterprise printers.
+  UpdatePolicyValue(prefs::kUserNativePrintersAllowed, false);
+
+  std::unique_ptr<Printer> configured_ptr = manager_->GetPrinter("Configured");
+  EXPECT_EQ(configured_ptr, nullptr);
+
+  std::unique_ptr<Printer> enterprise_ptr = manager_->GetPrinter("Enterprise");
+  ASSERT_NE(enterprise_ptr, nullptr);
+  EXPECT_EQ(enterprise_ptr->id(), "Enterprise");
+}
+
 }  // namespace
 }  // namespace chromeos