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..ac3a1169735392b2c0d50531f22154c7c5d62138
--- /dev/null
+++ b/remoting/host/third_party_auth_config.cc
@@ -0,0 +1,113 @@
+// 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 key = ::policy::key;

[Sergey Ulanov, 2015/02/27 03:05:19]

nit: I don't think this is useful give that you refer to key:: only 3 times in
this file.

[Łukasz Anforowicz, 2015/02/27 18:36:12]

Done.

+
+namespace {
+
+bool ParseSecureUrl(const std::string& str, GURL* out) {

[Sergey Ulanov, 2015/02/27 03:05:19]

call it ParseUrlPolicy()? with current name accepting empty strings makes no
sense.

[Łukasz Anforowicz, 2015/02/27 18:36:12]

Done.

+  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;
+    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* out) {

[Sergey Ulanov, 2015/02/27 03:05:19]

call it |result|

[Łukasz Anforowicz, 2015/02/27 18:36:12]

Done.

+  bool success = true;
+  ThirdPartyAuthConfig tmp;
+
+  // Extract raw values for the 3 individial fields.
+  success &= ParseSecureUrl(token_url, &tmp.token_url);
+  success &= ParseSecureUrl(token_validation_url, &tmp.token_validation_url);

[Sergey Ulanov, 2015/02/27 03:05:19]

I think the checks below make little sense if you fail to parse the urls. Just
return here if we fail to parse either of the URLs?

[Łukasz Anforowicz, 2015/02/27 18:36:12]

Good point.  Done.

+  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.";
+    success = false;

[Sergey Ulanov, 2015/02/27 03:05:19]

return false?

[Łukasz Anforowicz, 2015/02/27 18:36:12]

Done.  It is indeed cleaner to only have a "local" |urls_valid| variable and
avoid having to check |success| before the final assignment.

+  }
+  if (!tmp.token_validation_cert_issuer.empty() && tmp.token_url.is_empty()) {
+    LOG(ERROR) << "TokenValidationCertificateIssuer cannot be used "
+               << "without TokenUrl and TokenValidationUrl.";
+    success = false;

[Sergey Ulanov, 2015/02/27 03:05:19]

return false?

[Łukasz Anforowicz, 2015/02/27 18:36:12]

Done.

+  }
+
+  if (success) {
+    *out = tmp;
+  }
+  return success;
+}
+
+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, key::kRemoteAccessHostTokenUrl, &policies_present,
+                token_url);
+  ExtractHelper(policy_dict, key::kRemoteAccessHostTokenValidationUrl,
+                &policies_present, token_validation_url);
+  ExtractHelper(policy_dict,
+                key::kRemoteAccessHostTokenValidationCertificateIssuer,
+                &policies_present, token_validation_cert_issuer);
+  return policies_present;
+}
+
+std::ostream& operator<<(std::ostream& os, const ThirdPartyAuthConfig& cfg) {
+  if (cfg.is_empty()) {
+    os << "<no 3rd party auth config specified>";
+  } else {
+    os << "TokenUrl = " << cfg.token_url << ", ";
+    os << "TokenValidationUrl = " << cfg.token_validation_url << ", ";
+    os << "TokenValidationCertificateIssuer = "
+       << cfg.token_validation_cert_issuer;
+  }
+  return os;
+}
+
+}  // namespace remoting