Remove the class UriComponents

The goal of this patch is to replace UriComponent class with the Uri class. This patch also enforces better validation of printer URI every time a new URI is set for class Printer. Now, objects of class Printer can have only validated or empty URI. BUG=chromium:821497 TEST=unittests on my laptop and a few cases on atlas Change-Id: I9d80fc531b36fc91fbc983fa31cf5012fe0661a5 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2204269 Reviewed-by: Lei Zhang <thestig@chromium.org> Reviewed-by: Sean Kau <skau@chromium.org> Reviewed-by: James Cook <jamescook@chromium.org> Commit-Queue: Piotr Pawliczek <pawliczek@chromium.org> Cr-Commit-Position: refs/heads/master@{#788162}
qweri0p · Jul 14, 2020 · 9ca4401 · 9ca4401
1 parent 3b6e9d0
commit 9ca4401
Show file tree

Hide file tree

Showing 29 changed files with 391 additions and 487 deletions.
diff --git a/chrome/browser/chromeos/extensions/autotest_private/autotest_private_api.cc b/chrome/browser/chromeos/extensions/autotest_private/autotest_private_api.cc
@@ -2226,8 +2226,13 @@ ExtensionFunction::ResponseAction AutotestPrivateUpdatePrinterFunction::Run() {
   if (js_printer.printer_make_and_model)
     printer.set_make_and_model(*js_printer.printer_make_and_model);
 
-  if (js_printer.printer_uri)
-    printer.set_uri(*js_printer.printer_uri);
+  if (js_printer.printer_uri) {
+    std::string message;
+    if (!printer.SetUri(*js_printer.printer_uri, &message)) {
+      LOG(ERROR) << message;
+      return RespondNow(Error("Incorrect URI: " + message));
+    }
+  }
 
   if (js_printer.printer_ppd) {
     const GURL ppd =

diff --git a/chrome/browser/chromeos/extensions/printing/printing_api_utils.cc b/chrome/browser/chromeos/extensions/printing/printing_api_utils.cc
@@ -89,7 +89,7 @@ idl::Printer PrinterToIdl(
   idl_printer.id = printer.id();
   idl_printer.name = printer.display_name();
   idl_printer.description = printer.description();
-  idl_printer.uri = printer.uri();
+  idl_printer.uri = printer.uri().GetNormalized();
   idl_printer.source = PrinterSourceToIdl(printer.source());
   idl_printer.is_default =
       DoesPrinterMatchDefaultPrinterRules(printer, default_printer_rules);

diff --git a/chrome/browser/chromeos/printing/automatic_usb_printer_configurer_unittest.cc b/chrome/browser/chromeos/printing/automatic_usb_printer_configurer_unittest.cc
@@ -24,21 +24,21 @@ namespace {
 Printer CreateUsbPrinter(const std::string& id) {
   Printer printer;
   printer.set_id(id);
-  printer.set_uri("usb:printer");
+  printer.SetUri("usb://usb/printer");
   return printer;
 }
 
 Printer CreateIppUsbPrinter(const std::string& id) {
   Printer printer;
   printer.set_id(id);
-  printer.set_uri("ippusb:printer");
+  printer.SetUri("ippusb://usb/printer");
   return printer;
 }
 
 Printer CreateIppPrinter(const std::string& id) {
   Printer printer;
   printer.set_id(id);
-  printer.set_uri("ipp:printer");
+  printer.SetUri("ipp://usb/printer");
   return printer;
 }
 

diff --git a/chrome/browser/chromeos/printing/cups_printers_manager.cc b/chrome/browser/chromeos/printing/cups_printers_manager.cc
@@ -38,7 +38,7 @@
 #include "chrome/common/pref_names.h"
 #include "chromeos/printing/cups_printer_status.h"
 #include "chromeos/printing/printing_constants.h"
-#include "chromeos/printing/uri_components.h"
+#include "chromeos/printing/uri.h"
 #include "chromeos/services/network_config/public/mojom/cros_network_config.mojom.h"
 #include "components/device_event_log/device_event_log.h"
 #include "components/policy/policy_constants.h"
@@ -51,15 +51,8 @@
 
 namespace chromeos {
 
-bool IsIppUri(base::StringPiece printer_uri) {
-  base::StringPiece::size_type separator_location =
-      printer_uri.find(url::kStandardSchemeSeparator);
-  if (separator_location == base::StringPiece::npos) {
-    return false;
-  }
-
-  base::StringPiece scheme_part = printer_uri.substr(0, separator_location);
-  return scheme_part == kIppScheme || scheme_part == kIppsScheme;
+bool IsIppUri(const Uri& uri) {
+  return (uri.GetScheme() == kIppScheme || uri.GetScheme() == kIppsScheme);
 }
 
 namespace {
@@ -348,9 +341,8 @@ class CupsPrintersManagerImpl
       return;
     }
 
-    base::Optional<UriComponents> parsed_uri = ParseUri(printer->uri());
     // Behavior for querying a non-IPP uri is undefined and disallowed.
-    if (!parsed_uri || !IsIppUri(printer->uri())) {
+    if (IsIppUri(printer->uri())) {
       PRINTER_LOG(ERROR) << "Unable to complete printer status request. "
                          << "Printer uri is invalid. Printer id: "
                          << printer_id;
@@ -362,9 +354,10 @@ class CupsPrintersManagerImpl
       return;
     }
 
-    const UriComponents& uri = parsed_uri.value();
     QueryIppPrinter(
-        uri.host(), uri.port(), uri.path(), uri.encrypted(),
+        printer->uri().GetHostEncoded(), printer->uri().GetPort(),
+        printer->uri().GetPathEncodedAsString(),
+        printer->uri().GetScheme() == kIppsScheme,
         base::BindOnce(&CupsPrintersManagerImpl::OnPrinterInfoFetched,
                        weak_ptr_factory_.GetWeakPtr(), printer_id,
                        std::move(cb)));
@@ -563,10 +556,10 @@ class CupsPrintersManagerImpl
           // If the detected printer supports ipp-over-usb and we could not find
           // a ppd for it, then we switch to the ippusb scheme and mark it as
           // autoconf.
-          printer.set_uri(
-              base::StringPrintf("ippusb://%04x_%04x/ipp/print",
-                                 detected.ppd_search_data.usb_vendor_id,
-                                 detected.ppd_search_data.usb_product_id));
+          printer.SetUri(
+              Uri(base::StringPrintf("ippusb://%04x_%04x/ipp/print",
+                                     detected.ppd_search_data.usb_vendor_id,
+                                     detected.ppd_search_data.usb_product_id)));
           printer.mutable_ppd_reference()->autoconf = true;
           printers_.Insert(PrinterClass::kAutomatic, printer);
         } else {

diff --git a/chrome/browser/chromeos/printing/cups_printers_manager.h b/chrome/browser/chromeos/printing/cups_printers_manager.h
@@ -12,6 +12,7 @@
 #include "base/memory/ref_counted.h"
 #include "chrome/browser/chromeos/printing/printer_installation_manager.h"
 #include "chromeos/printing/printer_configuration.h"
+#include "chromeos/printing/uri.h"
 #include "components/keyed_service/core/keyed_service.h"
 
 class PrefService;
@@ -33,7 +34,7 @@ class SyncedPrintersManager;
 class UsbPrinterNotificationController;
 
 // Returns true if |printer_uri| is an IPP uri.
-bool IsIppUri(base::StringPiece printer_uri);
+bool IsIppUri(const Uri& printer_uri);
 
 // Top level manager of available CUPS printers in ChromeOS.  All functions
 // in this class must be called from a sequenced context.

diff --git a/chrome/browser/chromeos/printing/cups_printers_manager_unittest.cc b/chrome/browser/chromeos/printing/cups_printers_manager_unittest.cc
@@ -434,7 +434,7 @@ PrinterDetector::DetectedPrinter MakeDiscoveredPrinter(const std::string& id,
                                                        const std::string& uri) {
   PrinterDetector::DetectedPrinter ret;
   ret.printer.set_id(id);
-  ret.printer.set_uri(uri);
+  ret.printer.SetUri(uri);
   return ret;
 }
 
@@ -446,7 +446,7 @@ PrinterDetector::DetectedPrinter MakeDiscoveredPrinter(const std::string& id) {
 // Calls MakeDiscoveredPrinter with the USB protocol as the uri.
 PrinterDetector::DetectedPrinter MakeUsbDiscoveredPrinter(
     const std::string& id) {
-  return MakeDiscoveredPrinter(id, "usb:");
+  return MakeDiscoveredPrinter(id, "usb://host/path");
 }
 
 // Pseudo-constructor for inline creation of a DetectedPrinter that should (in
@@ -747,7 +747,7 @@ TEST_F(CupsPrintersManagerTest, UpdatedPrinterConfiguration) {
   manager_->PrinterInstalled(printer, /*is_automatic=*/false);
 
   Printer updated(printer);
-  updated.set_uri("different value");
+  updated.SetUri("ipp://different.value");
   EXPECT_FALSE(manager_->IsPrinterInstalled(updated));
 
   updated = printer;
@@ -774,7 +774,7 @@ TEST_F(CupsPrintersManagerTest, UpdatedPrinterConfiguration) {
 // Test that we can save non-discovered printers.
 TEST_F(CupsPrintersManagerTest, SavePrinterSucceedsOnManualPrinter) {
   Printer printer(kPrinterId);
-  printer.set_uri("manual uri");
+  printer.SetUri("ipp://manual.uri");
   manager_->SavePrinter(printer);
 
   auto saved_printers = manager_->GetPrinters(PrinterClass::kSaved);
@@ -800,7 +800,7 @@ TEST_F(CupsPrintersManagerTest, SavePrinterUpdatesPreviouslyInstalledPrinter) {
   EXPECT_TRUE(manager_->IsPrinterInstalled(printer));
 
   Printer updated(printer);
-  updated.set_uri("different value");
+  updated.SetUri("ipps://different/value");
   EXPECT_FALSE(manager_->IsPrinterInstalled(updated));
 
   manager_->SavePrinter(updated);
@@ -816,7 +816,7 @@ TEST_F(CupsPrintersManagerTest, SavePrinterUpdatesPreviouslyInstalledPrinter) {
 // Automatic USB Printer is configured automatically.
 TEST_F(CupsPrintersManagerTest, AutomaticUsbPrinterIsInstalledAutomatically) {
   auto automatic_printer = MakeAutomaticPrinter(kPrinterId);
-  automatic_printer.printer.set_uri("usb:");
+  automatic_printer.printer.SetUri("usb://host/path");
 
   usb_detector_->AddDetections({automatic_printer});
 
@@ -832,7 +832,7 @@ TEST_F(CupsPrintersManagerTest, AutomaticUsbPrinterNotInstalledAutomatically) {
   UpdatePolicyValue(prefs::kUserNativePrintersAllowed, false);
 
   auto automatic_printer = MakeAutomaticPrinter(kPrinterId);
-  automatic_printer.printer.set_uri("usb:");
+  automatic_printer.printer.SetUri("usb://host/path");
 
   zeroconf_detector_->AddDetections({automatic_printer});
 
@@ -845,7 +845,7 @@ TEST_F(CupsPrintersManagerTest, AutomaticUsbPrinterNotInstalledAutomatically) {
 // installed.
 TEST_F(CupsPrintersManagerTest, OtherNearbyPrintersNotInstalledAutomatically) {
   auto discovered_printer = MakeDiscoveredPrinter("Discovered");
-  discovered_printer.printer.set_uri("usb:");
+  discovered_printer.printer.SetUri("usb://host/path");
   auto automatic_printer = MakeAutomaticPrinter("Automatic");
 
   usb_detector_->AddDetections({discovered_printer});
@@ -861,7 +861,7 @@ TEST_F(CupsPrintersManagerTest, OtherNearbyPrintersNotInstalledAutomatically) {
 
 TEST_F(CupsPrintersManagerTest, DetectedUsbPrinterConfigurationNotification) {
   auto discovered_printer = MakeDiscoveredPrinter("Discovered");
-  discovered_printer.printer.set_uri("usb:");
+  discovered_printer.printer.SetUri("usb://host/path");
 
   usb_detector_->AddDetections({discovered_printer});
   task_environment_.RunUntilIdle();
@@ -877,7 +877,7 @@ TEST_F(CupsPrintersManagerTest, DetectedUsbPrinterConfigurationNotification) {
 TEST_F(CupsPrintersManagerTest,
        DetectedZeroconfDiscoveredPrinterNoNotification) {
   auto discovered_printer = MakeDiscoveredPrinter("Discovered");
-  discovered_printer.printer.set_uri("ipp:");
+  discovered_printer.printer.SetUri("ipp://host");
 
   zeroconf_detector_->AddDetections({discovered_printer});
   task_environment_.RunUntilIdle();
@@ -937,18 +937,5 @@ TEST_F(CupsPrintersManagerTest, RecordNearbyNetworkPrinterCounts) {
                                      2, 1);
 }
 
-TEST_F(CupsPrintersManagerTest, IsIppUri) {
-  // IPP protocol
-  ASSERT_TRUE(IsIppUri("ipp://1.2.3.4"));
-  // IPPS protocol
-  ASSERT_TRUE(IsIppUri("ipps://1.2.3.4"));
-  // USB protocol
-  ASSERT_FALSE(IsIppUri("usb://1.2.3.4"));
-  // Malformed URI
-  ASSERT_FALSE(IsIppUri("ipp/1.2.3.4"));
-  // Empty URI
-  ASSERT_FALSE(IsIppUri(""));
-}
-
 }  // namespace
 }  // namespace chromeos
diff --git a/chrome/browser/chromeos/printing/history/print_job_info_proto_conversions.cc b/chrome/browser/chromeos/printing/history/print_job_info_proto_conversions.cc
@@ -4,6 +4,8 @@
 
 #include "chrome/browser/chromeos/printing/history/print_job_info_proto_conversions.h"
 
+#include <string>
+
 #include "base/optional.h"
 #include "chrome/browser/chromeos/printing/printer_error_codes.h"
 #include "printing/mojom/print.mojom.h"
@@ -133,7 +135,7 @@ int64_t TimeToMillisecondsPastUnixEpoch(const base::Time& time) {
 proto::Printer PrinterToProto(const chromeos::Printer& printer) {
   proto::Printer printer_proto;
   printer_proto.set_name(printer.display_name());
-  printer_proto.set_uri(printer.uri());
+  printer_proto.set_uri(printer.uri().GetNormalized());
   printer_proto.set_source(PrinterSourceToProto(printer.source()));
   return printer_proto;
 }

diff --git a/chrome/browser/chromeos/printing/history/print_job_info_proto_conversions_unittest.cc b/chrome/browser/chromeos/printing/history/print_job_info_proto_conversions_unittest.cc
@@ -64,7 +64,7 @@ TEST(PrintJobInfoProtoConversionsTest, CupsPrintJobToProto) {
 
   chromeos::Printer printer;
   printer.set_display_name(kName);
-  printer.set_uri(kUri);
+  printer.SetUri(kUri);
   printer.set_source(chromeos::Printer::Source::SRC_POLICY);
 
   proto::PrintSettings settings;

diff --git a/chrome/browser/chromeos/printing/print_management/print_job_info_mojom_conversions.cc b/chrome/browser/chromeos/printing/print_management/print_job_info_mojom_conversions.cc
@@ -4,6 +4,8 @@
 
 #include "chrome/browser/chromeos/printing/print_management/print_job_info_mojom_conversions.h"
 
+#include <utility>
+
 #include "base/strings/utf_string_conversions.h"
 #include "base/time/time.h"
 #include "chrome/browser/chromeos/printing/cups_print_job.h"
@@ -130,7 +132,7 @@ mojom::PrintJobInfoPtr CupsPrintJobToMojom(const CupsPrintJob& job) {
   print_job_mojom->number_of_pages = job.total_page_number();
   print_job_mojom->printer_name =
       base::UTF8ToUTF16(job.printer().display_name());
-  print_job_mojom->printer_uri = GURL(job.printer().uri());
+  print_job_mojom->printer_uri = GURL(job.printer().uri().GetNormalized());
   return print_job_mojom;
 }
 

diff --git a/...e/browser/chromeos/printing/print_management/print_job_info_mojom_conversions_unittest.cc b/...e/browser/chromeos/printing/print_management/print_job_info_mojom_conversions_unittest.cc
@@ -4,6 +4,8 @@
 
 #include "chrome/browser/chromeos/printing/print_management/print_job_info_mojom_conversions.h"
 
+#include <memory>
+
 #include "base/strings/utf_string_conversions.h"
 #include "base/time/time.h"
 #include "chrome/browser/chromeos/printing/cups_print_job.h"
@@ -56,7 +58,7 @@ proto::PrintJobInfo CreatePrintJobInfoProto() {
 std::unique_ptr<CupsPrintJob> CreateCupsPrintJob() {
   Printer printer;
   printer.set_display_name(kName);
-  printer.set_uri(kUri);
+  printer.SetUri(kUri);
   printer.set_id(kPrinterId);
 
   auto cups_print_job = std::make_unique<CupsPrintJob>(

diff --git a/chrome/browser/chromeos/printing/printer_configurer.cc b/chrome/browser/chromeos/printing/printer_configurer.cc
@@ -111,7 +111,7 @@ class PrinterConfigurerImpl : public PrinterConfigurer {
                     PrinterSetupCallback callback) override {
     DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
     DCHECK(!printer.id().empty());
-    DCHECK(!printer.uri().empty());
+    DCHECK(printer.HasUri());
     PRINTER_LOG(USER) << printer.make_and_model() << " Printer setup requested";
     // Record if autoconf and a PPD are set.  crbug.com/814374.
     RecordValidPpdReference(printer);
@@ -130,7 +130,7 @@ class PrinterConfigurerImpl : public PrinterConfigurer {
                        << " Attempting autoconf setup";
     auto* client = DBusThreadManager::Get()->GetDebugDaemonClient();
     client->CupsAddAutoConfiguredPrinter(
-        printer.id(), printer.uri(),
+        printer.id(), printer.uri().GetNormalized(),
         base::BindOnce(&PrinterConfigurerImpl::OnAddedPrinter,
                        weak_factory_.GetWeakPtr(), printer,
                        std::move(callback)));
@@ -160,7 +160,7 @@ class PrinterConfigurerImpl : public PrinterConfigurer {
 
     PRINTER_LOG(EVENT) << printer.make_and_model() << " Manual printer setup";
     client->CupsAddManuallyConfiguredPrinter(
-        printer.id(), printer.uri(), ppd_contents,
+        printer.id(), printer.uri().GetNormalized(), ppd_contents,
         base::BindOnce(&PrinterConfigurerImpl::OnAddedPrinter,
                        weak_factory_.GetWeakPtr(), printer, std::move(cb)));
   }
@@ -205,7 +205,7 @@ std::string PrinterConfigurer::SetupFingerprint(const Printer& printer) {
   base::MD5Context ctx;
   base::MD5Init(&ctx);
   base::MD5Update(&ctx, printer.id());
-  base::MD5Update(&ctx, printer.uri());
+  base::MD5Update(&ctx, printer.uri().GetNormalized());
   base::MD5Update(&ctx, printer.ppd_reference().user_supplied_ppd_url);
   base::MD5Update(&ctx, printer.ppd_reference().effective_make_and_model);
   char autoconf = printer.ppd_reference().autoconf ? 1 : 0;