Support socket endpoint permissions for AppsV2 Socket API.

chrome/common/extensions/permissions/socket_permission_data.cc

+// Copyright (c) 2012 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/common/extensions/permissions/socket_permission_data.h"
+
+#include <cstdlib>
+#include <sstream>
+#include <vector>
+
+#include "base/logging.h"
+#include "base/string_split.h"
+#include "base/string_util.h"
+
+namespace extensions {
+
+SocketPermissionData::SocketPermissionData()
+  : type_(NONE),
+    host_("*"),

[miket_OOO, 2012/08/06 21:04:06]

Consider making these special characters (*, :, ., etc.) into string constants.

[Peng, 2012/08/07 21:31:55]

Done.

[miket_OOO, 2012/08/07 23:02:47]

I don't see that this is done.

[Peng, 2012/08/08 15:40:07]

Done.

+    match_subdomains_(false),
+    port_(0) {
+}
+
+SocketPermissionData::~SocketPermissionData() {
+}
+
+bool SocketPermissionData::operator<(const SocketPermissionData& rhs) const {
+  if (type_ < rhs.type_)
+    return true;
+  if (type_ > rhs.type_)
+    return false;
+
+  if (host_ < rhs.host_)
+    return true;
+  if (host_ > rhs.host_)
+    return false;
+
+  if (match_subdomains_ < rhs.match_subdomains_)
+    return true;
+  if (match_subdomains_ > rhs.match_subdomains_)
+    return false;
+
+  if (port_ < rhs.port_)
+    return true;
+  return false;
+}
+
+bool SocketPermissionData::operator==(const SocketPermissionData& rhs) const {
+  return (type_ == rhs.type_) && (host_ == rhs.host_) &&
+      (match_subdomains_ == rhs.match_subdomains_) &&
+      (port_ == rhs.port_);
+}
+
+bool SocketPermissionData::Match(
+    OperationType type, const std::string& host, int port) const {
+  if (type_ != type)
+    return false;
+
+  if (host_ != host) {
+    if (!match_subdomains_)
+      return false;
+
+    if (host.length() < host_.length() + 2)

[miket_OOO, 2012/08/06 21:04:06]

Can you add a comment about the + 2? It's not at all obvious what it means.

[Peng, 2012/08/07 21:31:55]

Done.

+      return false;
+
+    if (host.compare(host.length() - host_.length(),
+                          host_.length(), host_) != 0)

[miket_OOO, 2012/08/06 21:04:06]

This doesn't appear to be properly indented.

[Peng, 2012/08/07 21:31:55]

Done.

+      return false;
+  }
+
+  if (port_ != port && port_ != 0)
+    return false;
+
+  return true;
+}
+
+bool SocketPermissionData::Parse(const std::string& permission) {
+  // Reset
+  type_ = NONE;

[miket_OOO, 2012/08/06 21:04:06]

Ideally the reset functionality would be in a separate, testable private method.

[Peng, 2012/08/07 21:31:55]

Done. I changed it to *this = SocketPermissionData(), Is it OK? (otherwise it
has to have duplicate in reset and constructor, and we have to make sure they
are doing same thing, when we modify the initial values.)

[miket_OOO, 2012/08/07 23:02:47]

Hmmm. This looks weird. Why can't you call reset() in the constructor?

+  host_.clear();
+  match_subdomains_ = true;
+  port_ = 0;
+  spec_.clear();
+
+  std::vector<std::string> tokens;
+  base::SplitString(permission, ':', &tokens);

[miket_OOO, 2012/08/06 21:04:06]

Please have a look at base/string_tokenizer.h and see whether that's a better
fit than SplitString.

[Peng, 2012/08/07 21:31:55]

Seems StringTokenizer can not handle "aaa::cc" well. It will return aaa, cc
instead aaa, , cc. So probably using SplitString is simpler.

+
+  if (tokens.size() > 3)
+    return false;
+

[miket_OOO, 2012/08/06 21:04:06]

Does this crash if tokens is empty at this point?

[Peng, 2012/08/07 21:31:55]

Done.

+  if (tokens[0] == "tcp-connect")

[miket_OOO, 2012/08/06 21:04:06]

These should be constants in a common file.

[Peng, 2012/08/07 21:31:55]

Done.

+    type_ = TCP_CONNECT;
+  else if (tokens[0] == "tcp-listen")
+    type_ = TCP_LISTEN;
+  else if (tokens[0] == "udp-bind")
+    type_ = UDP_BIND;
+  else if (tokens[0] == "udp-send-to")
+    type_ = UDP_SEND_TO;
+  else
+    return false;
+
+  if (tokens.size() == 1)
+    return true;
+
+  // The first component can optionally be '*' to match all subdomains.
+  host_ = tokens[1];
+  if (!host_.empty()) {
+    std::vector<std::string> host_components;
+    base::SplitString(host_, '.', &host_components);

[miket_OOO, 2012/08/06 21:04:06]

Same question. I'm not sure how you can guarantee that host_components is not
empty at this point.

[Peng, 2012/08/07 21:31:55]

Done. host_ is checked at 107 line. And it is from tokens[1] which should
trimmed by SplitString() function. So it has to have some non space chars.
Anyway, I will add a DCHECK() here. And I will add some unittest for it.

+    if (host_components[0] == "*") {
+      host_components.erase(host_components.begin(),
+                            host_components.begin() + 1);
+    } else {
+      match_subdomains_ = false;
+    }
+    host_ = JoinString(host_components, '.');
+  }
+
+  if (tokens.size() == 2)
+    return true;
+
+  if (tokens.empty() || tokens[2] == "*")
+    return true;
+
+  port_ = atoi(tokens[2].c_str());

[miket_OOO, 2012/08/06 21:04:06]

Please look at base/string_number_conversions.h. Given atoi's poor record with
error-handling, I'd advise using Chrome wrappers wherever possible.

[Peng, 2012/08/07 21:31:55]

Done.

+  if (port_ < 0 || port_ > 65535)
+    return false;
+
+  return true;
+}
+
+const std::string& SocketPermissionData::GetAsString() const {
+  if (!spec_.empty())

[miket_OOO, 2012/08/06 21:04:06]

Are you sure a stringstream is necessary here? It looks like fairly simple
string concatenation that a std::string ought to handle just fine.

[Peng, 2012/08/07 21:31:55]

Done.

+    return spec_;
+
+  std::stringstream spec;
+  switch (type_) {

[miket_OOO, 2012/08/06 21:04:06]

Consider two separate methods that map type to string and vice-versa. They'll be
easier to test, and it'll be more obvious what needs to change if someone else
comes in needing to alter the permission scheme.

[Peng, 2012/08/07 21:31:55]

Done.

+    case TCP_CONNECT:
+      spec << "tcp-connect";
+      break;
+    case TCP_LISTEN:
+      spec << "tcp-listen";
+      break;
+    case UDP_BIND:
+      spec << "udp-bind";
+      break;
+    case UDP_SEND_TO:
+      spec << "udp-send-to";
+      break;
+    default:
+      return spec_;
+  }
+
+  if (match_subdomains_) {
+    spec << ":*";
+    if (!host_.empty())
+      spec << "." << host_;
+  } else {
+    spec << ":" << host_;
+  }
+
+  if (port_ == 0)
+    spec << ":*";
+  else
+    spec << ":" << port_;
+
+  spec_ = spec.str();
+  return spec_;
+}
+
+}  // namespace extensions