Add device policy print servers to ServerPrintersProvider

This complements the existing user policy and adds print servers defined
by device policy. The device policy has its own corresponding whitelist
and if there is existing print servers in user policy also in device
policy, then the user policy print servers take precedence (which is
only for the purpose of the print server names currently).
Also changed to using shared url loader factory since the
SystemNetworkContext is non-unit-testable.

Bug: 1100777
Change-Id: I0e45597437b59b5eb940c4fbf5fb63596b8504dd
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2293178Reviewed-by: Lei Zhang <thestig@chromium.org>
Reviewed-by: Luum Habtemariam <luum@chromium.org>
Reviewed-by: Piotr Pawliczek <pawliczek@chromium.org>
Commit-Queue: Matt Menard <mattme@google.com>
Cr-Commit-Position: refs/heads/master@{#800322}

chrome/browser/chromeos/BUILD.gn

@@ -3475,6 +3475,7 @@ source_set("unit_tests") {
     "printing/printer_event_tracker_unittest.cc",
     "printing/printers_map_unittest.cc",
     "printing/printers_sync_bridge_unittest.cc",
+    "printing/server_printers_provider_unittest.cc",
     "printing/specifics_translation_unittest.cc",
     "printing/synced_printers_manager_unittest.cc",
     "printing/test_cups_print_job_manager.cc",

chrome/browser/chromeos/DEPS

@@ -52,8 +52,4 @@ specific_include_rules = {
     "+chrome/browser/ui/views/extensions/extension_dialog.h",
     "+chrome/browser/ui/views/select_file_dialog_extension.h",
   ],
-  "server_printers_fetcher\.cc": [
-    # IPP protocol; it is needed for communication with print servers.
-    "+third_party/libipp/libipp/ipp.h",
-  ],
 }

chrome/browser/chromeos/printing/DEPS

+specific_include_rules = {
+  "(server_printers_fetcher|server_printers_provider_unittest)\.cc": [
+    # IPP protocol; it is needed for communication with print servers.
+    "+third_party/libipp/libipp/ipp.h",
+  ],
+}

chrome/browser/chromeos/printing/server_printers_fetcher.cc

@@ -21,6 +21,7 @@
 #include "components/device_event_log/device_event_log.h"
 #include "net/base/load_flags.h"
 #include "services/network/public/cpp/resource_request.h"
+#include "services/network/public/cpp/shared_url_loader_factory.h"
 #include "services/network/public/cpp/simple_url_loader.h"
 #include "services/network/public/cpp/simple_url_loader_stream_consumer.h"
 #include "third_party/libipp/libipp/ipp.h"
@@ -58,9 +59,11 @@ class ServerPrintersFetcher::PrivateImplementation
     CHECK(base::SequencedTaskRunnerHandle::IsSet());
     task_runner_for_callback_ = base::SequencedTaskRunnerHandle::Get();
     // Post task to execute.
-    task_runner_->PostTask(FROM_HERE,
-                           base::BindOnce(&PrivateImplementation::SendQuery,
-                                          base::Unretained(this)));
+    task_runner_->PostTask(
+        FROM_HERE,
+        base::BindOnce(
+            &PrivateImplementation::SendQuery, base::Unretained(this),
+            g_browser_process->shared_url_loader_factory()->Clone()));
   }
 
   ~PrivateImplementation() override = default;
@@ -141,7 +144,8 @@ class ServerPrintersFetcher::PrivateImplementation
 
  private:
   // The main task. It is scheduled in the constructor.
-  void SendQuery() {
+  void SendQuery(std::unique_ptr<network::PendingSharedURLLoaderFactory>
+                     url_loader_factory) {
     DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
     // Preparation of the IPP frame.
@@ -157,22 +161,18 @@ class ServerPrintersFetcher::PrivateImplementation
     auto resource_request = std::make_unique<network::ResourceRequest>();
     resource_request->url = server_url_;
     resource_request->method = "POST";
-    resource_request->headers.SetHeader(net::HttpRequestHeaders::kContentType,
-                                        "application/ipp");
     resource_request->load_flags =
         net::LOAD_BYPASS_CACHE | net::LOAD_DISABLE_CACHE;
-    resource_request->request_body =
-        network::ResourceRequestBody::CreateFromBytes(
-            reinterpret_cast<char*>(request_frame.data()),
-            request_frame.size());
     resource_request->credentials_mode = network::mojom::CredentialsMode::kOmit;
     // TODO(pawliczek): create a traffic annotation for printing network traffic
     simple_url_loader_ = network::SimpleURLLoader::Create(
         std::move(resource_request), MISSING_TRAFFIC_ANNOTATION);
-    network::mojom::URLLoaderFactory* loader_factory =
-        g_browser_process->system_network_context_manager()
-            ->GetURLLoaderFactory();
-    simple_url_loader_->DownloadAsStream(loader_factory, this);
+    std::string request_body(request_frame.begin(), request_frame.end());
+    simple_url_loader_->AttachStringForUpload(request_body, "application/ipp");
+    simple_url_loader_->DownloadAsStream(
+        network::SharedURLLoaderFactory::Create(std::move(url_loader_factory))
+            .get(),
+        this);
   }
 
   // Posts a response with a list of printers.

chrome/browser/chromeos/printing/server_printers_provider.cc

@@ -11,9 +11,12 @@
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
 #include "base/metrics/histogram_functions.h"
+#include "base/optional.h"
+#include "base/scoped_observer.h"
 #include "base/sequenced_task_runner.h"
 #include "base/stl_util.h"
 #include "base/task/post_task.h"
+#include "chrome/browser/browser_process.h"
 #include "chrome/browser/chromeos/printing/print_server.h"
 #include "chrome/browser/chromeos/printing/print_servers_provider.h"
 #include "chrome/browser/chromeos/printing/print_servers_provider_factory.h"
@@ -23,6 +26,8 @@
 #include "components/device_event_log/device_event_log.h"
 #include "url/gurl.h"
 
+class PrefService;
+
 namespace chromeos {
 
 namespace {
@@ -35,25 +40,72 @@ struct PrintServerWithPrinters {
   std::vector<PrinterDetector::DetectedPrinter> printers;  // queried printers
 };
 
+class PrintServersPolicyProvider : public PrintServersProvider::Observer {
+ public:
+  PrintServersPolicyProvider(base::WeakPtr<PrintServersProvider> provider,
+                             PrefService* prefs,
+                             const std::string& pref_name)
+      : provider_(provider) {
+    provider_->SetAllowlistPref(prefs, pref_name);
+    provider->AddObserver(this);
+  }
+
+  ~PrintServersPolicyProvider() override {
+    if (provider_) {
+      provider_->RemoveObserver(this);
+    }
+  }
+
+  base::Optional<std::vector<PrintServer>>& GetPrinterServers() {
+    return servers_;
+  }
+
+  void SetListener(const base::RepeatingCallback<void()>& callback) {
+    callback_ = std::make_unique<base::RepeatingCallback<void()>>(callback);
+    callback_->Run();
+  }
+
+  // PrintServersProvider::Observer implementation.
+  void OnServersChanged(bool servers_are_complete,
+                        const std::vector<PrintServer>& servers) override {
+    servers_ =
+        servers_are_complete ? base::make_optional(servers) : base::nullopt;
+    if (callback_) {
+      callback_->Run();
+    }
+  }
+
+ private:
+  base::WeakPtr<PrintServersProvider> provider_;
+  base::Optional<std::vector<PrintServer>> servers_;
+  std::unique_ptr<base::RepeatingCallback<void()>> callback_;
+};
+
 class ServerPrintersProviderImpl
     : public ServerPrintersProvider,
-      public PrintServersProvider::Observer,
       public base::SupportsWeakPtr<ServerPrintersProviderImpl> {
  public:
   explicit ServerPrintersProviderImpl(Profile* profile)
-      : servers_provider_(
-            PrintServersProviderFactory::Get()->GetForProfile(profile)) {
+      : user_policy_provider_(std::make_unique<PrintServersPolicyProvider>(
+            PrintServersProviderFactory::Get()->GetForProfile(profile),
+            profile->GetPrefs(),
+            prefs::kExternalPrintServersAllowlist)),
+        device_policy_provider_(std::make_unique<PrintServersPolicyProvider>(
+            PrintServersProviderFactory::Get()->GetForDevice(),
+            g_browser_process->local_state(),
+            prefs::kDeviceExternalPrintServersAllowlist)) {
     DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-    servers_provider_->SetAllowlistPref(profile->GetPrefs(),
-                                        prefs::kExternalPrintServersAllowlist);
-    servers_provider_->AddObserver(this);
-  }
 
-  ~ServerPrintersProviderImpl() override {
-    DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-    servers_provider_->RemoveObserver(this);
+    user_policy_provider_->SetListener(
+        base::BindRepeating(&ServerPrintersProviderImpl::NotifyPolicyChanged,
+                            weak_ptr_factory_.GetWeakPtr()));
+    device_policy_provider_->SetListener(
+        base::BindRepeating(&ServerPrintersProviderImpl::NotifyPolicyChanged,
+                            weak_ptr_factory_.GetWeakPtr()));
   }
 
+  ~ServerPrintersProviderImpl() override = default;
+
   void RegisterPrintersFoundCallback(OnPrintersUpdateCallback cb) override {
     DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
     callback_ = std::move(cb);
@@ -69,9 +121,8 @@ class ServerPrintersProviderImpl
     return printers;
   }
 
-  // PrintServersProvider::Observer implementation.
   void OnServersChanged(bool servers_are_complete,
-                        const std::vector<PrintServer>& servers) override {
+                        const std::map<GURL, PrintServer>& servers) {
     DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
     // Create an entry in the device log.
     if (servers_are_complete) {
@@ -88,7 +139,8 @@ class ServerPrintersProviderImpl
     servers_are_complete_ = servers_are_complete;
     // Fill new map with new servers and compare with the old map.
     std::map<GURL, PrintServerWithPrinters> new_servers;
-    for (const auto& server : servers) {
+    for (const auto& server_pair : servers) {
+      const PrintServer& server = server_pair.second;
       const GURL& url = server.GetUrl();
       const std::string& name = server.GetName();
       auto it_new = new_servers.emplace(url, server).first;
@@ -158,6 +210,25 @@ class ServerPrintersProviderImpl
   }
 
  private:
+  void NotifyPolicyChanged() {
+    std::map<GURL, PrintServer> all_servers;
+    auto& device_servers = device_policy_provider_->GetPrinterServers();
+    if (device_servers.has_value()) {
+      for (const auto& server : device_servers.value()) {
+        all_servers.emplace(server.GetUrl(), server);
+      }
+    }
+    auto& user_servers = user_policy_provider_->GetPrinterServers();
+    if (user_servers.has_value()) {
+      for (const auto& server : user_servers.value()) {
+        all_servers.emplace(server.GetUrl(), server);
+      }
+    }
+
+    bool is_complete = user_servers.has_value() || device_servers.has_value();
+    OnServersChanged(is_complete, all_servers);
+  }
+
   // Returns true <=> all policies have been parsed and applied and all servers
   // have been queried (even when some errors occurred).
   bool IsComplete() const {
@@ -176,7 +247,9 @@ class ServerPrintersProviderImpl
   // URLs that are being queried now with corresponding fetcher objects.
   std::map<GURL, std::unique_ptr<ServerPrintersFetcher>> fetchers_;
 
-  base::WeakPtr<PrintServersProvider> servers_provider_;
+  std::unique_ptr<PrintServersPolicyProvider> user_policy_provider_;
+  std::unique_ptr<PrintServersPolicyProvider> device_policy_provider_;
+
   SEQUENCE_CHECKER(sequence_checker_);
   base::WeakPtrFactory<ServerPrintersProviderImpl> weak_ptr_factory_{this};
   DISALLOW_COPY_AND_ASSIGN(ServerPrintersProviderImpl);