Implement Connector::ApplySpec() & use to enforce navigation:frame

services/service_manager/service_manager.cc

Index: services/service_manager/service_manager.cc
diff --git a/services/service_manager/service_manager.cc b/services/service_manager/service_manager.cc
index d29b0c9ad960ddaa0ecf2844270e479d7cef74a5..5db7cae74290010d4619fbaf71d0b3a0ad410444 100644
--- a/services/service_manager/service_manager.cc
+++ b/services/service_manager/service_manager.cc
@@ -22,6 +22,7 @@
 #include "mojo/public/cpp/bindings/associated_binding.h"
 #include "mojo/public/cpp/bindings/binding.h"
 #include "mojo/public/cpp/bindings/binding_set.h"
+#include "mojo/public/cpp/bindings/strong_binding.h"
 #include "services/catalog/public/interfaces/constants.mojom.h"
 #include "services/service_manager/connect_util.h"
 #include "services/service_manager/public/cpp/connector.h"
@@ -77,6 +78,25 @@ bool HasCapability(const InterfaceProviderSpec& spec,
   return it->second.find(capability) != it->second.end();
 }
 
+bool AllowsInterface(const Identity& source,
+                     const InterfaceProviderSpec& source_spec,
+                     const Identity& target,
+                     const InterfaceProviderSpec& target_spec,
+                     const std::string& interface_name) {
+  InterfaceSet exposed =
+      GetInterfacesToExpose(source_spec, target, target_spec);
+  bool allowed = (exposed.size() == 1 && exposed.count("*") == 1) ||
+                 exposed.count(interface_name) > 0;
+  if (!allowed) {
+    std::stringstream ss;
+    ss << "Connection InterfaceProviderSpec prevented service: "
+       << source.name() << " from binding interface: " << interface_name
+       << " exposed by: " << target.name();
+    LOG(ERROR) << ss.str();
+  }
+  return allowed;
+}
+
 // Encapsulates a connection to an instance of a service, tracked by the
 // Service Manager.
 class ServiceManager::Instance
@@ -145,17 +165,8 @@ class ServiceManager::Instance
       source_connection_spec = source->GetConnectionSpec();
     }
 
-    InterfaceSet exposed = GetInterfacesToExpose(source_connection_spec,
-                                                 identity_,
-                                                 GetConnectionSpec());
-    bool allowed = (exposed.size() == 1 && exposed.count("*") == 1) ||
-        exposed.count(params->interface_name()) > 0;
-    if (!allowed) {
-      std::stringstream ss;
-      ss << "Connection InterfaceProviderSpec prevented service: "
-         << params->source().name() << " from binding interface: "
-         << params->interface_name() << " exposed by: " << identity_.name();
-      LOG(ERROR) << ss.str();
+    if (!AllowsInterface(params->source(), source_connection_spec, identity_,
+                         GetConnectionSpec(), params->interface_name())) {
       params->set_response_data(mojom::ConnectResult::ACCESS_DENIED, identity_);
       return false;
     }
@@ -215,10 +226,17 @@ class ServiceManager::Instance
   }
 
   const InterfaceProviderSpec& GetConnectionSpec() const {
-    auto it = interface_provider_specs_.find(
-        mojom::kServiceManager_ConnectorSpec);
+    return GetSpecNamed(mojom::kServiceManager_ConnectorSpec);
+  }
+  bool HasSpecNamed(const std::string& spec) const {

[Ken Rockot(use gerrit already), 2017/04/26 04:32:48]

nits: GetSpec and HasSpec? And vertical whitespace between methods.

+    auto it = interface_provider_specs_.find(spec);
+    return it != interface_provider_specs_.end();
+  }
+  const InterfaceProviderSpec& GetSpecNamed(const std::string& spec) const {
+    auto it = interface_provider_specs_.find(spec);
     return it != interface_provider_specs_.end() ? it->second : empty_spec_;
   }
+
   const Identity& identity() const { return identity_; }
   void set_identity(const Identity& identity) { identity_ = identity; }
   uint32_t id() const { return id_; }
@@ -240,7 +258,7 @@ class ServiceManager::Instance
   }
 
  private:
-   enum class State {
+  enum class State {
     // The service was not started yet.
     IDLE,
 
@@ -252,74 +270,137 @@ class ServiceManager::Instance
     STARTED
   };
 
+  class InterfaceProviderImpl : public mojom::InterfaceProvider {
+   public:
+    InterfaceProviderImpl(const std::string& spec,
+                          const Identity& source_identity,
+                          const Identity& target_identity,
+                          service_manager::ServiceManager* service_manager,
+                          mojom::InterfaceProviderPtr target)
+        : spec_(spec),
+          source_identity_(source_identity),
+          target_identity_(target_identity),
+          service_manager_(service_manager),
+          target_(std::move(target)) {}
+    ~InterfaceProviderImpl() override {}
+
+   private:
+    // mojom::InterfaceProvider:
+    void GetInterface(const std::string& interface_name,
+                      mojo::ScopedMessagePipeHandle interface_pipe) override {
+      Instance* source =
+          service_manager_->GetExistingInstance(source_identity_);
+      Instance* target =
+          service_manager_->GetExistingInstance(target_identity_);
+      if (!source || !target)
+        return;
+      if (!ValidateSpec(source) || !ValidateSpec(target))
+        return;
+
+      if (AllowsInterface(source_identity_, source->GetSpecNamed(spec_),
+                          target_identity_, target->GetSpecNamed(spec_),
+                          interface_name)) {
+        target_->GetInterface(interface_name, std::move(interface_pipe));
+      }
+    }
+
+    bool ValidateSpec(Instance* instance) const {
+      if (!instance->HasSpecNamed(spec_)) {
+        LOG(ERROR) << "Instance for: " << instance->identity().name()
+                   << " did not have spec named: " << spec_;
+        return false;
+      }
+      return true;
+    }
+
+    const std::string spec_;
+    const Identity source_identity_;
+    const Identity target_identity_;
+    service_manager::ServiceManager* service_manager_;

[Ken Rockot(use gerrit already), 2017/04/26 04:32:48]

nit: this could be const too (service_manager::ServiceManager* const
service_manager_)

+
+    mojom::InterfaceProviderPtr target_;
+
+    DISALLOW_COPY_AND_ASSIGN(InterfaceProviderImpl);
+  };
+
   // mojom::Connector implementation:
-   void BindInterface(const service_manager::Identity& in_target,
-                      const std::string& interface_name,
-                      mojo::ScopedMessagePipeHandle interface_pipe,
-                      const BindInterfaceCallback& callback) override {
-     Identity target = in_target;
-     mojom::ConnectResult result =
-         ValidateConnectParams(&target, nullptr, nullptr);
-     if (!Succeeded(result)) {
-       callback.Run(result, Identity());
-       return;
-     }
-
-     std::unique_ptr<ConnectParams> params(new ConnectParams);
-     params->set_source(identity_);
-     params->set_target(target);
-     params->set_interface_request_info(interface_name,
-                                        std::move(interface_pipe));
-     params->set_start_service_callback(callback);
-     service_manager_->Connect(std::move(params), weak_factory_.GetWeakPtr());
-   }
-
-   void StartService(const Identity& in_target,
-                     const StartServiceCallback& callback) override {
-     Identity target = in_target;
-     mojom::ConnectResult result =
-         ValidateConnectParams(&target, nullptr, nullptr);
-     if (!Succeeded(result)) {
-       callback.Run(result, Identity());
-       return;
-     }
-
-     std::unique_ptr<ConnectParams> params(new ConnectParams);
-     params->set_source(identity_);
-     params->set_target(target);
-     params->set_start_service_callback(callback);
-     service_manager_->Connect(std::move(params), weak_factory_.GetWeakPtr());
-   }
-
-   void StartServiceWithProcess(
-       const Identity& in_target,
-       mojo::ScopedMessagePipeHandle service_handle,
-       mojom::PIDReceiverRequest pid_receiver_request,
-       const StartServiceWithProcessCallback& callback) override {
-     Identity target = in_target;
-     mojom::ConnectResult result =
-         ValidateConnectParams(&target, nullptr, nullptr);
-     if (!Succeeded(result)) {
-       callback.Run(result, Identity());
-       return;
-     }
-
-     std::unique_ptr<ConnectParams> params(new ConnectParams);
-     params->set_source(identity_);
-     params->set_target(target);
-
-     mojom::ServicePtr service;
-     service.Bind(mojom::ServicePtrInfo(std::move(service_handle), 0));
-     params->set_client_process_info(std::move(service),
-                                     std::move(pid_receiver_request));
-     params->set_start_service_callback(callback);
-     service_manager_->Connect(std::move(params), weak_factory_.GetWeakPtr());
+  void BindInterface(const service_manager::Identity& in_target,
+                     const std::string& interface_name,
+                     mojo::ScopedMessagePipeHandle interface_pipe,
+                     const BindInterfaceCallback& callback) override {
+    Identity target = in_target;
+    mojom::ConnectResult result =
+        ValidateConnectParams(&target, nullptr, nullptr);
+    if (!Succeeded(result)) {
+      callback.Run(result, Identity());
+      return;
+    }
+
+    std::unique_ptr<ConnectParams> params(new ConnectParams);
+    params->set_source(identity_);
+    params->set_target(target);
+    params->set_interface_request_info(interface_name,
+                                       std::move(interface_pipe));
+    params->set_start_service_callback(callback);
+    service_manager_->Connect(std::move(params), weak_factory_.GetWeakPtr());
+  }
+
+  void StartService(const Identity& in_target,
+                    const StartServiceCallback& callback) override {
+    Identity target = in_target;
+    mojom::ConnectResult result =
+        ValidateConnectParams(&target, nullptr, nullptr);
+    if (!Succeeded(result)) {
+      callback.Run(result, Identity());
+      return;
+    }
+
+    std::unique_ptr<ConnectParams> params(new ConnectParams);
+    params->set_source(identity_);
+    params->set_target(target);
+    params->set_start_service_callback(callback);
+    service_manager_->Connect(std::move(params), weak_factory_.GetWeakPtr());
+  }
+
+  void StartServiceWithProcess(
+      const Identity& in_target,
+      mojo::ScopedMessagePipeHandle service_handle,
+      mojom::PIDReceiverRequest pid_receiver_request,
+      const StartServiceWithProcessCallback& callback) override {
+    Identity target = in_target;
+    mojom::ConnectResult result =
+        ValidateConnectParams(&target, nullptr, nullptr);
+    if (!Succeeded(result)) {
+      callback.Run(result, Identity());
+      return;
+    }
+
+    std::unique_ptr<ConnectParams> params(new ConnectParams);
+    params->set_source(identity_);
+    params->set_target(target);
+
+    mojom::ServicePtr service;
+    service.Bind(mojom::ServicePtrInfo(std::move(service_handle), 0));
+    params->set_client_process_info(std::move(service),
+                                    std::move(pid_receiver_request));
+    params->set_start_service_callback(callback);
+    service_manager_->Connect(std::move(params), weak_factory_.GetWeakPtr());
   }
 
   void Clone(mojom::ConnectorRequest request) override {
     connectors_.AddBinding(this, std::move(request));
   }
 
+  void ApplySpec(const std::string& spec,
+                 const Identity& source,
+                 mojom::InterfaceProviderRequest source_request,
+                 mojom::InterfaceProviderPtr target) override {
+    mojo::MakeStrongBinding(

[Ken Rockot(use gerrit already), 2017/04/26 04:32:48]

It's not safe to use StrongBinding here if you're going to hold a raw pointer to
ServiceManager. How about just having InterfaceProviderImpl own its own Binding,
and let Instance own all its active InterfaceProviderImpls?

+        base::MakeUnique<InterfaceProviderImpl>(
+            spec, source, identity_, service_manager_, std::move(target)),
+        std::move(source_request));
+  }
+
   // mojom::PIDReceiver:
   void SetPID(uint32_t pid) override {
     PIDAvailable(pid);