Malformed PortRange or ThirdPartyAuthConfig trigger OnPolicyError.

remoting/host/third_party_auth_config.cc

Index: remoting/host/third_party_auth_config.cc
diff --git a/remoting/host/third_party_auth_config.cc b/remoting/host/third_party_auth_config.cc
new file mode 100644
index 0000000000000000000000000000000000000000..84ef57becbf44fc8b29ea2712bfd48f7fa8d730a
--- /dev/null
+++ b/remoting/host/third_party_auth_config.cc
@@ -0,0 +1,112 @@
+// Copyright 2015 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 "remoting/host/third_party_auth_config.h"
+
+#include "base/logging.h"
+#include "base/values.h"
+#include "policy/policy_constants.h"
+
+namespace remoting {
+
+namespace {
+
+bool ParseUrlPolicy(const std::string& str, GURL* out) {
+  if (str.empty()) {
+    *out = GURL();
+    return true;
+  }
+
+  GURL gurl(str);
+  if (!gurl.is_valid()) {
+    LOG(ERROR) << "Not a valid URL: " << str;
+    return false;
+  }
+  if (!gurl.SchemeIsSecure()) {
+    LOG(ERROR) << "Not a secure URL: " << str;

[rmsousa, 2015/02/27 23:56:14]

was this a security requirement? can we make it release only? Or at least make
it secure _or localhost_? This adds a lot of trouble when testing authentication
services locally (one will have to create a root CA, add it as trusted in their
system, and manually override the http server to set up https with that CA when
running locally).

[Łukasz Anforowicz, 2015/03/02 17:34:47]

Not an official security requirement - this is just me, making up extra checks
that might catch issues in policy configs in the wild.  Let's make it release
only.

+    return false;
+  }
+
+  *out = gurl;
+  return true;
+}
+
+}  // namespace
+
+bool ThirdPartyAuthConfig::Parse(
+    const std::string& token_url,
+    const std::string& token_validation_url,
+    const std::string& token_validation_cert_issuer,
+    ThirdPartyAuthConfig* result) {
+  ThirdPartyAuthConfig tmp;
+
+  // Extract raw values for the 3 individial fields.

[rmsousa, 2015/02/27 23:56:14]

nit: individual

[Łukasz Anforowicz, 2015/03/02 17:34:47]

Done.

+  bool urls_valid = true;
+  urls_valid &= ParseUrlPolicy(token_url, &tmp.token_url);
+  urls_valid &= ParseUrlPolicy(token_validation_url, &tmp.token_validation_url);
+  if (!urls_valid) {
+    return false;
+  }
+  tmp.token_validation_cert_issuer = token_validation_cert_issuer;
+
+  // Validate inter-dependencies between the 3 fields.
+  if (tmp.token_url.is_empty() ^ tmp.token_validation_url.is_empty()) {
+    LOG(ERROR) << "TokenUrl and TokenValidationUrl "
+               << "have to be specified together.";
+    return false;
+  }
+  if (!tmp.token_validation_cert_issuer.empty() && tmp.token_url.is_empty()) {
+    LOG(ERROR) << "TokenValidationCertificateIssuer cannot be used "
+               << "without TokenUrl and TokenValidationUrl.";
+    return false;
+  }
+
+  *result = tmp;
+  return true;
+}
+
+namespace {
+
+void ExtractHelper(const base::DictionaryValue& policy_dict,
+                   const std::string& policy_name,
+                   bool* policy_present,
+                   std::string* policy_value) {
+  if (policy_dict.GetString(policy_name, policy_value)) {
+    *policy_present = true;
+  } else {
+    policy_value->clear();
+  }
+}
+
+}  // namespace
+
+bool ThirdPartyAuthConfig::ExtractPolicyValues(
+    const base::DictionaryValue& policy_dict,
+    std::string* token_url,
+    std::string* token_validation_url,
+    std::string* token_validation_cert_issuer) {
+  bool policies_present = false;
+  ExtractHelper(policy_dict, policy::key::kRemoteAccessHostTokenUrl,
+                &policies_present, token_url);
+  ExtractHelper(policy_dict, policy::key::kRemoteAccessHostTokenValidationUrl,
+                &policies_present, token_validation_url);
+  ExtractHelper(policy_dict,
+                policy::key::kRemoteAccessHostTokenValidationCertificateIssuer,
+                &policies_present, token_validation_cert_issuer);
+  return policies_present;
+}
+
+std::ostream& operator<<(std::ostream& os, const ThirdPartyAuthConfig& cfg) {
+  if (cfg.is_null()) {
+    os << "<no 3rd party auth config specified>";
+  } else {
+    os << "TokenUrl = " << cfg.token_url << ", ";

[rmsousa, 2015/02/27 23:56:14]

super nit: enclose in <> as well?

[Łukasz Anforowicz, 2015/03/02 17:34:47]

Done.

+    os << "TokenValidationUrl = " << cfg.token_validation_url << ", ";
+    os << "TokenValidationCertificateIssuer = "
+       << cfg.token_validation_cert_issuer;
+  }
+  return os;
+}
+
+}  // namespace remoting