Implement retry logic for consumer print server

- If a scheme is unspecified, we will default with ipps.
- If that fails then we will fallback with ipp with port 631.

Bug: 1045651
Test: end to end manual
Change-Id: Id25cc700340913c6f3df6a7e508c2341d6913cb6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2086687
Commit-Queue: jimmy gong <jimmyxgong@chromium.org>
Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Bailey Berro <baileyberro@chromium.org>
Cr-Commit-Position: refs/heads/master@{#749374}

chrome/browser/ui/BUILD.gn

@@ -2016,6 +2016,8 @@ jumbo_static_library("ui") {
       "webui/settings/chromeos/search/search_concept.h",
       "webui/settings/chromeos/search/settings_user_action_tracker.cc",
       "webui/settings/chromeos/search/settings_user_action_tracker.h",
+      "webui/settings/chromeos/server_printer_url_util.cc",
+      "webui/settings/chromeos/server_printer_url_util.h",
       "webui/settings/chromeos/wallpaper_handler.cc",
       "webui/settings/chromeos/wallpaper_handler.h",
       "webui/settings/tts_handler.cc",

chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc

@@ -39,6 +39,7 @@
 #include "chrome/browser/ui/browser_finder.h"
 #include "chrome/browser/ui/browser_window.h"
 #include "chrome/browser/ui/chrome_select_file_policy.h"
+#include "chrome/browser/ui/webui/settings/chromeos/server_printer_url_util.h"
 #include "chrome/common/chrome_paths.h"
 #include "chrome/common/pref_names.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
@@ -265,40 +266,11 @@ Printer::PpdReference GetPpdReference(const base::Value* info) {
   return ret;
 }
 
-bool ConvertToGURL(const std::string& url, GURL* gurl) {
-  *gurl = GURL(url);
-  if (!gurl->is_valid()) {
-    // URL is not valid.
-    return false;
-  }
-  if (!gurl->SchemeIsHTTPOrHTTPS() && !gurl->SchemeIs("ipp") &&
-      !gurl->SchemeIs("ipps")) {
-    // URL has unsupported scheme; we support only: http, https, ipp, ipps.
-    return false;
-  }
-  // Replaces ipp/ipps by http/https. IPP standard describes protocol built
-  // on top of HTTP, so both types of addresses have the same meaning in the
-  // context of IPP interface. Moreover, the URL must have http/https scheme
-  // to pass IsStandard() test from GURL library (see "Validation of the URL
-  // address" below).
-  bool set_ipp_port = false;
-  if (gurl->SchemeIs("ipp")) {
-    set_ipp_port = (gurl->IntPort() == url::PORT_UNSPECIFIED);
-    *gurl = GURL("http" + url.substr(url.find_first_of(':')));
-  } else if (gurl->SchemeIs("ipps")) {
-    *gurl = GURL("https" + url.substr(url.find_first_of(':')));
-  }
-  // The default port for ipp is 631. If the schema ipp is replaced by http
-  // and the port is not explicitly defined in the url, we have to overwrite
-  // the default http port with the default ipp port. For ipps we do nothing
-  // because implementers use the same port for ipps and https.
-  if (set_ipp_port) {
-    GURL::Replacements replacement;
-    replacement.SetPortStr("631");
-    *gurl = gurl->ReplaceComponents(replacement);
-  }
-  // Validation of the URL address.
-  return gurl->IsStandard();
+GURL GenerateHttpCupsServerUrl(const GURL& server_url) {
+  GURL::Replacements replacement;
+  replacement.SetSchemeStr("http");
+  replacement.SetPortStr("631");
+  return server_url.ReplaceComponents(replacement);
 }
 
 }  // namespace
@@ -1284,27 +1256,45 @@ void CupsPrintersHandler::HandleQueryPrintServer(const base::ListValue* args) {
   CHECK(args->GetString(0, &callback_id));
   CHECK(args->GetString(1, &server_url));
 
-  GURL server_gurl;
-  if (!ConvertToGURL(server_url, &server_gurl)) {
+  base::Optional<GURL> converted_server_url =
+      GenerateServerPrinterUrlWithValidScheme(server_url);
+  if (!converted_server_url) {
     RejectJavascriptCallback(
         base::Value(callback_id),
         base::Value(PrintServerQueryResult::kIncorrectUrl));
     return;
   }
 
+  // Use fallback only if HasValidServerPrinterScheme is false.
+  QueryPrintServer(callback_id, converted_server_url.value(),
+                   !HasValidServerPrinterScheme(GURL(server_url)));
+}
+
+void CupsPrintersHandler::QueryPrintServer(const std::string& callback_id,
+                                           const GURL& server_url,
+                                           bool should_fallback) {
   server_printers_fetcher_ = std::make_unique<ServerPrintersFetcher>(
-      server_gurl, "(from user)",
+      server_url, "(from user)",
       base::BindRepeating(&CupsPrintersHandler::OnQueryPrintServerCompleted,
-                          weak_factory_.GetWeakPtr(), callback_id));
+                          weak_factory_.GetWeakPtr(), callback_id,
+                          should_fallback));
 }
 
 void CupsPrintersHandler::OnQueryPrintServerCompleted(
     const std::string& callback_id,
+    bool should_fallback,
     const ServerPrintersFetcher* sender,
     const GURL& server_url,
     std::vector<PrinterDetector::DetectedPrinter>&& returned_printers) {
   const PrintServerQueryResult result = sender->GetLastError();
   if (result != PrintServerQueryResult::kNoErrors) {
+    if (should_fallback) {
+      // Apply the fallback query.
+      QueryPrintServer(callback_id, GenerateHttpCupsServerUrl(server_url),
+                       /*should_fallback=*/false);
+      return;
+    }
+
     RejectJavascriptCallback(base::Value(callback_id), base::Value(result));
     return;
   }
@@ -1314,9 +1304,10 @@ void CupsPrintersHandler::OnQueryPrintServerCompleted(
       printers_manager_->GetPrinters(PrinterClass::kSaved);
   std::set<GURL> known_printers;
   for (const Printer& printer : saved_printers) {
-    GURL gurl;
-    if (ConvertToGURL(printer.uri(), &gurl))
-      known_printers.insert(gurl);
+    base::Optional<GURL> gurl =
+        GenerateServerPrinterUrlWithValidScheme(printer.uri());
+    if (gurl)
+      known_printers.insert(gurl.value());
   }
 
   // Built final list of printers and a list of current names. If "current name"
@@ -1326,11 +1317,10 @@ void CupsPrintersHandler::OnQueryPrintServerCompleted(
   printers.reserve(returned_printers.size());
   for (PrinterDetector::DetectedPrinter& printer : returned_printers) {
     printers.push_back(std::move(printer.printer));
-    GURL printer_gurl;
-    if (ConvertToGURL(printers.back().uri(), &printer_gurl)) {
-      if (known_printers.count(printer_gurl))
-        printers.pop_back();
-    }
+    base::Optional<GURL> printer_gurl =
+        GenerateServerPrinterUrlWithValidScheme(printers.back().uri());
+    if (printer_gurl && known_printers.count(printer_gurl.value()))
+      printers.pop_back();
   }
 
   // Delete fetcher object.

chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.h

@@ -214,8 +214,14 @@ class CupsPrintersHandler : public ::settings::SettingsPageUIHandler,
                     const net::IPEndPoint& endpoint);
 
   void HandleQueryPrintServer(const base::ListValue* args);
+
+  void QueryPrintServer(const std::string& callback_id,
+                        const GURL& server_url,
+                        bool should_fallback);
+
   void OnQueryPrintServerCompleted(
       const std::string& callback_id,
+      bool should_fallback,
       const ServerPrintersFetcher* sender,
       const GURL& server_url,
       std::vector<PrinterDetector::DetectedPrinter>&& returned_printers);

chrome/browser/ui/webui/settings/chromeos/server_printer_url_util.cc

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/ui/webui/settings/chromeos/server_printer_url_util.h"
+
+#include "url/gurl.h"
+
+namespace {
+
+// Returns an updated |gurl| with the specified components from the params. If
+// |scheme| is not empty, returns an updated GURL with the specified scheme. If
+// |replace_port| is true, returns an updated GURL with 631 as the port. 631 is
+// the default port for IPP.
+GURL UpdateServerPrinterGURL(const GURL& gurl,
+                             const std::string& scheme,
+                             bool replace_ipp_port) {
+  GURL::Replacements replacement;
+  if (!scheme.empty())
+    replacement.SetSchemeStr(scheme);
+  if (replace_ipp_port)
+    replacement.SetPortStr("631");
+  return gurl.ReplaceComponents(replacement);
+}
+
+}  // namespace
+
+namespace chromeos {
+namespace settings {
+
+bool HasValidServerPrinterScheme(const GURL& gurl) {
+  return gurl.SchemeIsHTTPOrHTTPS() || gurl.SchemeIs("ipp") ||
+         gurl.SchemeIs("ipps");
+}
+
+base::Optional<GURL> GenerateServerPrinterUrlWithValidScheme(
+    const std::string& url) {
+  base::Optional<GURL> gurl = base::make_optional(GURL(url));
+  if (!HasValidServerPrinterScheme(*gurl)) {
+    // If we're missing a valid scheme, try querying with IPPS first.
+    gurl = GURL("ipps://" + url);
+  }
+
+  if (!gurl->is_valid())
+    return base::nullopt;
+
+  // Replaces IPP/IPPS by HTTP/HTTPS. IPP standard describes protocol built
+  // on top of HTTP, so both types of addresses have the same meaning in the
+  // context of IPP interface. Moreover, the URL must have HTTP/HTTPS scheme
+  // to pass IsStandard() test from GURL library (see "Validation of the URL
+  // address" below).
+  if (gurl->SchemeIs("ipp")) {
+    gurl = UpdateServerPrinterGURL(*gurl, "http",
+                                   /*replace_ipp_port=*/false);
+    // The default port for IPP is 631. If the schema IPP is replaced by HTTP
+    // and the port is not explicitly defined in the URL, we have to overwrite
+    // the default HTTP port with the default IPP port. For IPPS we do nothing
+    // because implementers use the same port for IPPS and HTTPS.
+    if (gurl->IntPort() == url::PORT_UNSPECIFIED) {
+      gurl = UpdateServerPrinterGURL(*gurl, /*scheme=*/"",
+                                     /*replace_ipp_port=*/true);
+    }
+  } else if (gurl->SchemeIs("ipps")) {
+    gurl = UpdateServerPrinterGURL(*gurl, "https",
+                                   /*replace_ipp_port=*/false);
+  }
+
+  // Check validation of the URL address and return |gurl| if valid.
+  return gurl->IsStandard() ? gurl : base::nullopt;
+}
+
+}  // namespace settings
+}  // namespace chromeos

chrome/browser/ui/webui/settings/chromeos/server_printer_url_util.h

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROME_BROWSER_UI_WEBUI_SETTINGS_CHROMEOS_SERVER_PRINTER_URL_UTIL_H_
+#define CHROME_BROWSER_UI_WEBUI_SETTINGS_CHROMEOS_SERVER_PRINTER_URL_UTIL_H_
+
+#include <string>
+
+#include "base/optional.h"
+
+class GURL;
+
+namespace chromeos {
+namespace settings {
+
+// Returns true if |gurl| has any the following scheme: HTTP, HTTPS, IPP, or
+// IPPS. Returns false for an empty or any other scheme.
+bool HasValidServerPrinterScheme(const GURL& gurl);
+
+// Returns a GURL from the input |url|. Returns base::nullopt if
+// either |url| is invalid or constructing the GURL failed. This will also
+// default the server printer URI to use HTTPS if it detects a missing scheme.
+base::Optional<GURL> GenerateServerPrinterUrlWithValidScheme(
+    const std::string& url);
+
+}  // namespace settings
+}  // namespace chromeos
+
+#endif  // CHROME_BROWSER_UI_WEBUI_SETTINGS_CHROMEOS_SERVER_PRINTER_URL_UTIL_H_

chrome/browser/ui/webui/settings/chromeos/server_printer_url_util_unittest.cc

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/ui/webui/settings/chromeos/server_printer_url_util.h"
+
+#include <string>
+
+#include "base/optional.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "url/gurl.h"
+
+namespace chromeos {
+namespace settings {
+
+class ServerPrinterUrlUtilTest : public testing::Test {
+ public:
+  ServerPrinterUrlUtilTest() = default;
+  ~ServerPrinterUrlUtilTest() override = default;
+};
+
+TEST_F(ServerPrinterUrlUtilTest, IsValidScheme) {
+  GURL gurl1("ipp://123.123.11.11:123");
+  ASSERT_TRUE(HasValidServerPrinterScheme(gurl1));
+
+  GURL gurl2("http://123.123.11.11:123");
+  ASSERT_TRUE(HasValidServerPrinterScheme(gurl2));
+
+  GURL gurl3("ipps://123.123.11.11:123");
+  ASSERT_TRUE(HasValidServerPrinterScheme(gurl3));
+
+  GURL gurl4("https://123.123.11.11:123");
+  ASSERT_TRUE(HasValidServerPrinterScheme(gurl4));
+
+  // Missing scheme.
+  GURL gurl5("123.123.11.11:123");
+  ASSERT_FALSE(HasValidServerPrinterScheme(gurl5));
+
+  // Invalid scheme.
+  GURL gurl6("test://123.123.11.11:123");
+  ASSERT_FALSE(HasValidServerPrinterScheme(gurl6));
+}
+
+TEST_F(ServerPrinterUrlUtilTest, ConvertToGURL) {
+  // Test that a GURL is created with |gurl1| as its source.
+  std::string url1("http://123.123.11.11:631");
+  base::Optional<GURL> gurl1 = GenerateServerPrinterUrlWithValidScheme(url1);
+  DCHECK(gurl1);
+  ASSERT_EQ("http://123.123.11.11:631/", gurl1->spec());
+  ASSERT_EQ("http", gurl1->scheme());
+  ASSERT_EQ("631", gurl1->port());
+
+  // Test that HTTPS is the default scheme if a scheme is not provided.
+  std::string url2("123.123.11.11:631");
+  base::Optional<GURL> gurl2 = GenerateServerPrinterUrlWithValidScheme(url2);
+  DCHECK(gurl2);
+  ASSERT_EQ("https", gurl2->scheme());
+  ASSERT_EQ("https://123.123.11.11:631/", gurl2->spec());
+
+  // Test that if a URL has IPP as its scheme, it will create a new GURL with
+  // HTTP as its scheme and 631 as its port.
+  std::string url3("ipp://123.123.11.11");
+  base::Optional<GURL> gurl3 = GenerateServerPrinterUrlWithValidScheme(url3);
+  DCHECK(gurl3);
+  ASSERT_EQ("http", gurl3->scheme());
+  ASSERT_EQ("631", gurl3->port());
+  ASSERT_EQ("http://123.123.11.11:631/", gurl3->spec());
+
+  // Test that if a URL has IPP as its scheme and a specified port, it will
+  // create a new GURL with HTTP as the scheme and keeps the same port.
+  std::string url4("ipp://123.123.11.11:321");
+  base::Optional<GURL> gurl4 = GenerateServerPrinterUrlWithValidScheme(url4);
+  DCHECK(gurl4);
+  ASSERT_EQ("http", gurl4->scheme());
+  ASSERT_EQ("321", gurl4->port());
+  ASSERT_EQ("http://123.123.11.11:321/", gurl4->spec());
+
+  // Test that if a URL has IPPS as its scheme and a specified port, a new GURL
+  // is created with the scheme as HTTPS and keeps the same port.
+  std::string url5("ipps://123.123.11.11:555");
+  base::Optional<GURL> gurl5 = GenerateServerPrinterUrlWithValidScheme(url5);
+  DCHECK(gurl5);
+  ASSERT_EQ("https", gurl5->scheme());
+  ASSERT_EQ("555", gurl5->port());
+  ASSERT_EQ("https://123.123.11.11:555/", gurl5->spec());
+}
+
+}  // namespace settings
+}  // namespace chromeos

chrome/test/BUILD.gn

@@ -5098,6 +5098,7 @@ test("unit_tests") {
         sources += [
           "../browser/ui/webui/print_preview/local_printer_handler_chromeos_unittest.cc",
           "../browser/ui/webui/settings/chromeos/cups_printers_handler_unittest.cc",
+          "../browser/ui/webui/settings/chromeos/server_printer_url_util_unittest.cc",
         ]
       }
     } else {