Perform InterfaceProviderSpec intersection in the ServiceManager

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 63a0d7c487a7b22bf06e1f7708fed8e498a9d791..73f76e9171a04a08027402dade5c3b04a823b3c9 100644
--- a/services/service_manager/service_manager.cc
+++ b/services/service_manager/service_manager.cc
@@ -47,6 +47,21 @@ const char kCapability_InstancePerChild[] =
     "service_manager:instance_per_child";
 const char kCapability_ServiceManager[] = "service_manager:service_manager";
 
+bool Succeeded(mojom::ConnectResult result) {
+  return result == mojom::ConnectResult::SUCCEEDED;
+}
+
+void RunCallback(ConnectParams* params,
+                 mojom::ConnectResult result,
+                 const std::string& user_id) {
+  if (!params->connect_callback().is_null()) {
+    params->connect_callback().Run(result, user_id);
+    return;
+  }
+  if (!params->bind_interface_callback().is_null())
+    params->bind_interface_callback().Run(result, user_id);
+}
+
 }  // namespace
 
 Identity CreateServiceManagerIdentity() {
@@ -125,18 +140,19 @@ class ServiceManager::Instance
     Stop();
   }
 
-  bool ConnectToService(std::unique_ptr<ConnectParams>* connect_params) {
+  bool OnConnect(std::unique_ptr<ConnectParams>* in_params) {
     if (!service_.is_bound())
       return false;
 
-    std::unique_ptr<ConnectParams> params(std::move(*connect_params));
+    std::unique_ptr<ConnectParams> params(std::move(*in_params));
     if (!params->connect_callback().is_null()) {
       params->connect_callback().Run(mojom::ConnectResult::SUCCEEDED,
                                      identity_.user_id());
     }
 
     InterfaceProviderSpecMap specs;
-    Instance* source = service_manager_->GetExistingInstance(params->source());
+    Instance* source =
+        service_manager_->GetExistingInstance(params->source());
     if (source)
       specs = source->interface_provider_specs_;
 
@@ -148,6 +164,48 @@ class ServiceManager::Instance
     return true;
   }
 
+  bool OnBindInterface(std::unique_ptr<ConnectParams>* in_params) {
+    if (!service_.is_bound())

[Ken Rockot(use gerrit already), 2017/01/12 23:05:00]

We need to run the response callback in this case. I suppose we just might never
(or rarely) hit this branch in OnConnect and hence that hasn't been triggering a
DCHECK similar to http://crbug.com/680700).

+      return false;
+
+    std::unique_ptr<ConnectParams> params(std::move(*in_params));
+    InterfaceProviderSpecMap source_specs;
+    InterfaceProviderSpec source_connection_spec;
+    Instance* source =
+        service_manager_->GetExistingInstance(params->source());
+    if (source) {
+      source_specs = source->interface_provider_specs_;
+      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();
+      params->bind_interface_callback().Run(mojom::ConnectResult::ACCESS_DENIED,
+                                            identity_.user_id());
+      return false;
+    }
+
+    params->bind_interface_callback().Run(mojom::ConnectResult::SUCCEEDED,
+                                          identity_.user_id());
+
+    pending_service_connections_++;
+    service_->OnBindInterface(
+        ServiceInfo(params->source(), source_specs),
+        params->interface_name(),
+        params->TakeInterfaceRequestPipe(),
+        base::Bind(&Instance::OnConnectComplete, base::Unretained(this)));
+    return true;
+  }
+
   void OnConnectComplete() {
     DCHECK_GT(pending_service_connections_, 0);
     pending_service_connections_--;
@@ -229,72 +287,65 @@ class ServiceManager::Instance
 
   // mojom::Connector implementation:
   void StartService(
-      const Identity& target,
+      const Identity& in_target,
       mojo::ScopedMessagePipeHandle service_handle,
       mojom::PIDReceiverRequest pid_receiver_request) override {
+    Identity target = in_target;
+    mojom::ConnectResult result =
+        ValidateConnectParams(&target, nullptr, nullptr);
+    if (!Succeeded(result))
+      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));
-    ConnectImpl(
-        target,
-        mojom::InterfaceProviderRequest(),
-        std::move(service),
-        std::move(pid_receiver_request),
-        base::Bind(
-            &service_manager::ServiceManager::Instance::EmptyConnectCallback,
-            weak_factory_.GetWeakPtr()));
+    params->set_client_process_info(std::move(service),
+                                    std::move(pid_receiver_request));
+    service_manager_->Connect(
+        std::move(params), nullptr, weak_factory_.GetWeakPtr());
   }
 
-  void Connect(const service_manager::Identity& target,
+  void Connect(const service_manager::Identity& in_target,
                mojom::InterfaceProviderRequest remote_interfaces,
                const ConnectCallback& callback) override {
-    ConnectImpl(target, std::move(remote_interfaces), mojom::ServicePtr(),
-                mojom::PIDReceiverRequest(), callback);
+    Identity target = in_target;
+    mojom::ConnectResult result =
+        ValidateConnectParams(&target, nullptr, nullptr);
+    if (!Succeeded(result)) {
+      callback.Run(result, mojom::kInheritUserID);
+      return;
+    }
+
+    std::unique_ptr<ConnectParams> params(new ConnectParams);
+    params->set_source(identity_);
+    params->set_target(target);
+    params->set_remote_interfaces(std::move(remote_interfaces));
+    params->set_connect_callback(callback);
+    service_manager_->Connect(
+        std::move(params), nullptr, weak_factory_.GetWeakPtr());
   }
 
-  void BindInterface(const service_manager::Identity& target,
+  void BindInterface(const service_manager::Identity& in_target,
                      const std::string& interface_name,
                      mojo::ScopedMessagePipeHandle interface_pipe,
                      const BindInterfaceCallback& callback) override {
-    mojom::InterfaceProviderPtr remote_interfaces;
-    ConnectImpl(
-        target,
-        MakeRequest(&remote_interfaces),
-        mojom::ServicePtr(),
-        mojom::PIDReceiverRequest(),
-        base::Bind(
-            &service_manager::ServiceManager::Instance::BindCallbackWrapper,
-            weak_factory_.GetWeakPtr(),
-            callback));
-    remote_interfaces->GetInterface(interface_name, std::move(interface_pipe));
-    // TODO(beng): Rather than just forwarding thru to InterfaceProvider, do
-    //             manifest policy intersection here.
-  }
-
-  void ConnectImpl(const service_manager::Identity& in_target,
-                   mojom::InterfaceProviderRequest remote_interfaces,
-                   mojom::ServicePtr service,
-                   mojom::PIDReceiverRequest pid_receiver_request,
-                   const ConnectCallback& callback) {
     Identity target = in_target;
-    if (target.user_id() == mojom::kInheritUserID)
-      target.set_user_id(identity_.user_id());
-
-    if (!ValidateIdentity(target, callback))
-      return;
-    if (!ValidateClientProcessInfo(&service, &pid_receiver_request, target,
-                                   callback)) {
+    mojom::ConnectResult result =
+        ValidateConnectParams(&target, nullptr, nullptr);
+    if (!Succeeded(result)) {
+      callback.Run(result, mojom::kInheritUserID);
       return;
     }
-    if (!ValidateConnectionSpec(target, callback))
-      return;
 
     std::unique_ptr<ConnectParams> params(new ConnectParams);
     params->set_source(identity_);
     params->set_target(target);
-    params->set_remote_interfaces(std::move(remote_interfaces));
-    params->set_client_process_info(std::move(service),
-                                    std::move(pid_receiver_request));
-    params->set_connect_callback(callback);
+    params->set_interface_request_info(interface_name,
+                                       std::move(interface_pipe));
+    params->set_bind_interface_callback(callback);
     service_manager_->Connect(
         std::move(params), nullptr, weak_factory_.GetWeakPtr());
   }
@@ -321,61 +372,66 @@ class ServiceManager::Instance
     service_manager_->AddListener(std::move(listener));
   }
 
-  bool ValidateIdentity(const Identity& identity,
-                        const ConnectCallback& callback) {
+  mojom::ConnectResult ValidateConnectParams(
+      Identity* target,
+      mojom::ServicePtr* service,
+      mojom::PIDReceiverRequest* pid_receiver_request) {
+    if (target->user_id() == mojom::kInheritUserID)
+      target->set_user_id(identity_.user_id());
+
+    mojom::ConnectResult result = ValidateIdentity(*target);
+    if (!Succeeded(result))
+      return result;
+
+    result = ValidateClientProcessInfo(service, pid_receiver_request, *target);
+    if (!Succeeded(result))
+      return result;
+    return ValidateConnectionSpec(*target);
+  }
+
+  mojom::ConnectResult ValidateIdentity(const Identity& identity) {
     if (identity.name().empty()) {
       LOG(ERROR) << "Error: empty service name.";
-      callback.Run(mojom::ConnectResult::INVALID_ARGUMENT,
-                   mojom::kInheritUserID);
-      return false;
+      return mojom::ConnectResult::INVALID_ARGUMENT;
     }
     if (!base::IsValidGUID(identity.user_id())) {
       LOG(ERROR) << "Error: invalid user_id: " << identity.user_id();
-      callback.Run(mojom::ConnectResult::INVALID_ARGUMENT,
-                   mojom::kInheritUserID);
-      return false;
+      return mojom::ConnectResult::INVALID_ARGUMENT;
     }
-    return true;
+    return mojom::ConnectResult::SUCCEEDED;
   }
 
-  bool ValidateClientProcessInfo(
+  mojom::ConnectResult ValidateClientProcessInfo(
       mojom::ServicePtr* service,
       mojom::PIDReceiverRequest* pid_receiver_request,
-      const Identity& target,
-      const ConnectCallback& callback) {
-    if (service->is_bound() || pid_receiver_request->is_pending()) {
+      const Identity& target) {
+    if (service && pid_receiver_request &&
+        (service->is_bound() || pid_receiver_request->is_pending())) {
       if (!HasCapability(GetConnectionSpec(), kCapability_ClientProcess)) {
         LOG(ERROR) << "Instance: " << identity_.name() << " attempting "
                    << "to register an instance for a process it created for "
                    << "target: " << target.name() << " without the "
                    << "service_manager{client_process} capability "
                    << "class.";
-        callback.Run(mojom::ConnectResult::ACCESS_DENIED,
-                     mojom::kInheritUserID);
-        return false;
+        return mojom::ConnectResult::ACCESS_DENIED;
       }
 
       if (!service->is_bound() || !pid_receiver_request->is_pending()) {
         LOG(ERROR) << "Must supply both service AND "
                    << "pid_receiver_request when sending client process info";
-        callback.Run(mojom::ConnectResult::INVALID_ARGUMENT,
-                     mojom::kInheritUserID);
-        return false;
+        return mojom::ConnectResult::INVALID_ARGUMENT;
       }
       if (service_manager_->GetExistingInstance(target)) {
         LOG(ERROR) << "Cannot client process matching existing identity:"
                    << "Name: " << target.name() << " User: "
                    << target.user_id() << " Instance: " << target.instance();
-        callback.Run(mojom::ConnectResult::INVALID_ARGUMENT,
-                     mojom::kInheritUserID);
-        return false;
+        return mojom::ConnectResult::INVALID_ARGUMENT;
       }
     }
-    return true;
+    return mojom::ConnectResult::SUCCEEDED;
   }
 
-  bool ValidateConnectionSpec(const Identity& target,
-                              const ConnectCallback& callback) {
+  mojom::ConnectResult ValidateConnectionSpec(const Identity& target) {
     InterfaceProviderSpec connection_spec = GetConnectionSpec();
     // TODO(beng): Need to do the following additional policy validation of
     // whether this instance is allowed to connect using:
@@ -388,9 +444,7 @@ class ServiceManager::Instance
                  << " attempting to connect to: " << target.name()
                  << " as: " << target.user_id() << " without "
                  << " the service:service_manager{user_id} capability.";
-      callback.Run(mojom::ConnectResult::ACCESS_DENIED,
-                   mojom::kInheritUserID);
-      return false;
+      return mojom::ConnectResult::ACCESS_DENIED;
     }
     if (!target.instance().empty() &&
         target.instance() != target.name() &&
@@ -400,19 +454,17 @@ class ServiceManager::Instance
                  << " using Instance name: " << target.instance()
                  << " without the "
                  << "service_manager{instance_name} capability.";
-      callback.Run(mojom::ConnectResult::ACCESS_DENIED, mojom::kInheritUserID);
-      return false;
+      return mojom::ConnectResult::ACCESS_DENIED;
     }
 
     if (allow_any_application_ ||
         connection_spec.requires.find(target.name()) !=
             connection_spec.requires.end()) {
-      return true;
+      return mojom::ConnectResult::SUCCEEDED;
     }
     LOG(ERROR) << "InterfaceProviderSpec prevented connection from: "
                << identity_.name() << " to: " << target.name();
-    callback.Run(mojom::ConnectResult::ACCESS_DENIED, mojom::kInheritUserID);
-    return false;
+    return mojom::ConnectResult::ACCESS_DENIED;
   }
 
   uint32_t GenerateUniqueID() const {
@@ -782,7 +834,14 @@ void ServiceManager::NotifyServiceFailedToStart(const Identity& identity) {
 bool ServiceManager::ConnectToExistingInstance(
     std::unique_ptr<ConnectParams>* params) {
   Instance* instance = GetExistingInstance((*params)->target());
-  return instance && instance->ConnectToService(params);
+  if (instance) {
+    if ((*params)->HasInterfaceRequestInfo()) {
+      instance->OnBindInterface(params);
+      return true;
+    }
+    return instance->OnConnect(params);
+  }
+  return false;
 }
 
 ServiceManager::Instance* ServiceManager::CreateInstance(
@@ -868,10 +927,7 @@ void ServiceManager::OnGotResolvedName(std::unique_ptr<ConnectParams> params,
   // If name resolution failed, we drop the connection.
   if (!result) {
     LOG(ERROR) << "Failed to resolve service name: " << params->target().name();
-    if (!params->connect_callback().is_null()) {
-      params->connect_callback().Run(
-          mojom::ConnectResult::INVALID_ARGUMENT, "");
-    }
+    RunCallback(params.get(), mojom::ConnectResult::INVALID_ARGUMENT, "");
     return;
   }
 
@@ -941,8 +997,7 @@ void ServiceManager::OnGotResolvedName(std::unique_ptr<ConnectParams> params,
       LOG(ERROR)
           << "Error: The catalog was unable to read a manifest for service \""
           << result->name << "\".";
-      if (!params->connect_callback().is_null())
-        params->connect_callback().Run(mojom::ConnectResult::ACCESS_DENIED, "");
+      RunCallback(params.get(), mojom::ConnectResult::ACCESS_DENIED, "");
       return;
     }
 
@@ -980,18 +1035,19 @@ void ServiceManager::OnGotResolvedName(std::unique_ptr<ConnectParams> params,
 
       if (!instance->StartWithFilePath(package_path)) {
         OnInstanceError(instance);
-        if (!params->connect_callback().is_null()) {
-          params->connect_callback().Run(
-              mojom::ConnectResult::INVALID_ARGUMENT, "");
-        }
+        RunCallback(params.get(), mojom::ConnectResult::INVALID_ARGUMENT, "");
         return;
       }
     }
   }
 
   // Now that the instance has a Service, we can connect to it.
-  bool connected = instance->ConnectToService(&params);
-  DCHECK(connected);
+  if (params->HasInterfaceRequestInfo()) {
+    instance->OnBindInterface(&params);
+  } else {
+    bool connected = instance->OnConnect(&params);
+    DCHECK(connected);
+  }
 }
 
 base::WeakPtr<ServiceManager> ServiceManager::GetWeakPtr() {