Fixing bugs for editing configured zeroconf printer

This change fixes the bug where if a user attempts to change the URI of
an existing configured zeroconf printer from a link local address to a
static IP address the attempt to add the device would timeout and fail.

This issue was caused by wrongly attempting to perform domain name resolution on
the IP address instead of simply performing the configuration with the
new address.

Bug: 838677
Change-Id: Iba1a8fb8dfd0194138e2d0aa881592027fd713bd
Reviewed-on: https://chromium-review.googlesource.com/c/1109387
Commit-Queue: David Valleau <valleau@chromium.org>
Reviewed-by: Sean Kau <skau@chromium.org>
Cr-Commit-Position: refs/heads/master@{#622091}

chrome/browser/chromeos/printing/printer_configurer.cc

@@ -76,13 +76,28 @@ class PrinterConfigurerImpl : public PrinterConfigurer {
       return;
     }
 
-    PRINTER_LOG(DEBUG) << printer.make_and_model() << " Resolving IP";
+    // Ensure that |address| is non-empty before attempting to resolve it.
+    // If the uri in |printer| does not contain both a hostname and a port
+    // number then GetHostAndPort() will return an empty string.
+    auto address = printer.GetHostAndPort();
+    if (address.IsEmpty()) {
+      // Return an error and abort printer setup. If we attempt to call
+      // EndpointResolver::Start() with an empty address then it will fail
+      // silently without returning into the callback.
+      PRINTER_LOG(ERROR) << "Address is invalid";
+      std::move(callback).Run(PrinterSetupResult::kPrinterUnreachable);
+      return;
+    }
+
+    PRINTER_LOG(DEBUG) << printer.make_and_model()
+                       << " Resolving IP: " << address.ToString();
+
     // Resolve the uri to an ip with a mutable copy of the printer.
     endpoint_resolver_->Start(
-        printer.GetHostAndPort(),
-        base::BindOnce(
-            &PrinterConfigurerImpl::OnIpResolved, weak_factory_.GetWeakPtr(),
-            std::make_unique<Printer>(printer), std::move(callback)));
+        address, base::BindOnce(&PrinterConfigurerImpl::OnIpResolved,
+                                weak_factory_.GetWeakPtr(),
+                                std::make_unique<Printer>(printer),
+                                std::move(callback)));
   }
 
  private:

chromeos/printing/printer_configuration.cc

@@ -118,8 +118,17 @@ bool Printer::IsIppEverywhere() const {
 }
 
 bool Printer::RequiresIpResolution() const {
-  return effective_uri_.empty() &&
-         base::StartsWith(id_, "zeroconf-", base::CompareCase::SENSITIVE);
+  if (effective_uri_.empty() &&
+      base::StartsWith(id_, "zeroconf-", base::CompareCase::SENSITIVE)) {
+    // Check to see if |uri_| is a contains a ".local" hostname. This is to
+    // catch the case where a user edits the address of an existing configured
+    // zeroconf printer to a static IP address.
+    base::Optional<UriComponents> components_optional = ParseUri(uri_);
+    UriComponents uri_components = components_optional.value();
+    return base::EndsWith(uri_components.host(), ".local",
+                          base::CompareCase::SENSITIVE);
+  }
+  return false;
 }
 
 net::HostPortPair Printer::GetHostAndPort() const {

chromeos/printing/printer_configuration_unittest.cc

@@ -84,4 +84,42 @@ TEST(PrinterConfigurationTest, ParseUriSubdomainQueueAndPort) {
   EXPECT_EQ(result->path(), "/ipp/print");
 }
 
+TEST(PrinterConfigurationTest, RequiresIpResolution) {
+  chromeos::Printer printer;
+  printer.set_effective_uri("");
+  printer.set_id("zeroconf-testid");
+  printer.set_uri("ipp://test.local:631/ipp/print");
+  EXPECT_TRUE(printer.RequiresIpResolution());
+}
+
+// In this case we expect RequiresIpResolution() to return false since
+// |effective_uri_| has already been computed.
+TEST(PrinterConfigurationTest, RequiresIpResolutionNonEmptyEffectiveUri) {
+  chromeos::Printer printer;
+  printer.set_effective_uri("test.local:631");
+  printer.set_id("zeroconf-testid");
+  printer.set_uri("ipp://test.local:631/ipp/print");
+  EXPECT_FALSE(printer.RequiresIpResolution());
+}
+
+// In this case we expect RequiresIpResolution() to return false since |id_|
+// does not have the "zeroconf-" prefix.
+TEST(PrinterConfigurationTest, RequiresIpResolutionNotZeroconf) {
+  chromeos::Printer printer;
+  printer.set_effective_uri("");
+  printer.set_id("testid");
+  printer.set_uri("ipp://test.local:631/ipp/print");
+  EXPECT_FALSE(printer.RequiresIpResolution());
+}
+
+// In this case we expect RequiresIpResolution() to return false since the
+// hostname in |uri_| does not end with ".local".
+TEST(PrinterConfigurationTest, RequiresIpResolutionNoLocal) {
+  chromeos::Printer printer;
+  printer.set_effective_uri("");
+  printer.set_id("zeroconf-testid");
+  printer.set_uri("ipp://localhost:631/ipp/print");
+  EXPECT_FALSE(printer.RequiresIpResolution());
+}
+
 }  // namespace