Some security checks around destruction/setting bounds.

. validate on the client that the node can only be deleted or have its bounds changed by the connection that created the node. note that it's still possible for the client to call the interface directly, but the service will validate also, and then the client's inconsistent state is the client's own fault.
. pass bounds through the INodes array

R=sky@chromium.org
http://crbug.com/365012

Review URL: https://codereview.chromium.org/296133012

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@273116 0039d316-1c4b-4281-b951-d872f2087c98

mojo/services/public/cpp/view_manager/lib/view_manager_synchronizer.cc

@@ -26,13 +26,15 @@ uint32_t MakeTransportId(uint16_t connection_id, uint16_t local_id) {
 ViewTreeNode* AddNodeToViewManager(ViewManager* manager,
                                    ViewTreeNode* parent,
                                    TransportNodeId node_id,
-                                   TransportViewId view_id) {
+                                   TransportViewId view_id,
+                                   const gfx::Rect& bounds) {
   // We don't use the ctor that takes a ViewManager here, since it will call
   // back to the service and attempt to create a new node.
   ViewTreeNode* node = ViewTreeNodePrivate::LocalCreate();
   ViewTreeNodePrivate private_node(node);
   private_node.set_view_manager(manager);
   private_node.set_id(node_id);
+  private_node.LocalSetBounds(gfx::Rect(), bounds);
   if (parent)
     ViewTreeNodePrivate(parent).LocalAddChild(node);
   ViewManagerPrivate private_manager(manager);
@@ -69,7 +71,8 @@ ViewTreeNode* BuildNodeTree(ViewManager* manager,
         manager,
         !parents.empty() ? parents.back() : NULL,
         nodes[i].node_id(),
-        nodes[i].view_id());
+        nodes[i].view_id(),
+        nodes[i].bounds());
     if (!last_node)
       root = node;
     last_node = node;

mojo/services/public/cpp/view_manager/lib/view_tree_node.cc

@@ -180,6 +180,12 @@ class ScopedDestructionNotifier {
   DISALLOW_COPY_AND_ASSIGN(ScopedDestructionNotifier);
 };
 
+// Some operations are only permitted in the connection that created the node.
+bool OwnsNode(ViewManager* manager, ViewTreeNode* node) {
+  return !manager ||
+      ViewManagerPrivate(manager).synchronizer()->OwnsNode(node->id());
+}
+
 }  // namespace
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -193,7 +199,9 @@ ViewTreeNode* ViewTreeNode::Create(ViewManager* view_manager) {
 }
 
 void ViewTreeNode::Destroy() {
-  // TODO(beng): only proceed if |manager_| OwnsNode(this).
+  if (!OwnsNode(manager_, this))
+    return;
+
   if (manager_)
     ViewManagerPrivate(manager_).synchronizer()->DestroyViewTreeNode(id_);
   while (!children_.empty())
@@ -202,7 +210,9 @@ void ViewTreeNode::Destroy() {
 }
 
 void ViewTreeNode::SetBounds(const gfx::Rect& bounds) {
-  // TODO(beng): only proceed if |manager_| OwnsNode(this).
+  if (!OwnsNode(manager_, this))
+    return;
+
   if (manager_)
     ViewManagerPrivate(manager_).synchronizer()->SetBounds(id_, bounds);
   LocalSetBounds(bounds_, bounds);
@@ -217,6 +227,8 @@ void ViewTreeNode::RemoveObserver(ViewTreeNodeObserver* observer) {
 }
 
 void ViewTreeNode::AddChild(ViewTreeNode* child) {
+  // TODO(beng): not necessarily valid to all connections, but possibly to the
+  //             embeddee in an embedder-embeddee relationship.
   if (manager_)
     CHECK_EQ(ViewTreeNodePrivate(child).view_manager(), manager_);
   LocalAddChild(child);
@@ -225,6 +237,8 @@ void ViewTreeNode::AddChild(ViewTreeNode* child) {
 }
 
 void ViewTreeNode::RemoveChild(ViewTreeNode* child) {
+  // TODO(beng): not necessarily valid to all connections, but possibly to the
+  //             embeddee in an embedder-embeddee relationship.
   if (manager_)
     CHECK_EQ(ViewTreeNodePrivate(child).view_manager(), manager_);
   LocalRemoveChild(child);
@@ -256,6 +270,8 @@ ViewTreeNode* ViewTreeNode::GetChildById(TransportNodeId id) {
 }
 
 void ViewTreeNode::SetActiveView(View* view) {
+  // TODO(beng): not necessarily valid to all connections, but possibly to the
+  //             embeddee in an embedder-embeddee relationship.
   if (manager_)
     CHECK_EQ(ViewPrivate(view).view_manager(), manager_);
   LocalSetActiveView(view);

mojo/services/public/cpp/view_manager/tests/view_manager_unittest.cc

@@ -216,6 +216,36 @@ void WaitForDestruction(ViewManager* view_manager,
   DoRunLoop();
 }
 
+// Tracks a node's destruction. Query is_valid() for current state.
+class NodeTracker : public ViewTreeNodeObserver {
+ public:
+  explicit NodeTracker(ViewTreeNode* node) : node_(node) {
+    node_->AddObserver(this);
+  }
+  virtual ~NodeTracker() {
+    if (node_)
+      node_->RemoveObserver(this);
+  }
+
+  bool is_valid() const { return !!node_; }
+
+ private:
+  // Overridden from ViewTreeNodeObserver:
+  virtual void OnNodeDestroy(
+      ViewTreeNode* node,
+      ViewTreeNodeObserver::DispositionChangePhase phase) OVERRIDE {
+    if (phase != ViewTreeNodeObserver::DISPOSITION_CHANGED)
+      return;
+    DCHECK_EQ(node, node_);
+    node_ = NULL;
+  }
+
+  int id_;
+  ViewTreeNode* node_;
+
+  DISALLOW_COPY_AND_ASSIGN(NodeTracker);
+};
+
 }  // namespace
 
 // ViewManager -----------------------------------------------------------------
@@ -548,6 +578,8 @@ TEST_F(ViewManagerTest, MapSubtreeOnAttach) {
   ViewTreeNode* node11 = CreateNodeInParent(node1);
   View* view11 = View::Create(view_manager_1());
   node11->SetActiveView(view11);
+  gfx::Rect node11_bounds(800, 600);
+  node11->SetBounds(node11_bounds);
   WaitForAllChangesToBeAcked(view_manager_1());
 
   // Now attach this node tree to the root & wait for it to show up in the
@@ -559,6 +591,7 @@ TEST_F(ViewManagerTest, MapSubtreeOnAttach) {
   View* view11_2 = view_manager_2()->GetViewById(view11->id());
   EXPECT_TRUE(node11_2 != NULL);
   EXPECT_EQ(view11_2, node11_2->active_view());
+  EXPECT_EQ(node11_bounds, node11_2->bounds());
 }
 
 // Verifies that bounds changes applied to a node hierarchy in one connection
@@ -573,8 +606,35 @@ TEST_F(ViewManagerTest, SetBounds) {
   node1->SetBounds(gfx::Rect(0, 0, 100, 100));
   WaitForBoundsToChange(node1_2);
   EXPECT_EQ(node1->bounds(), node1_2->bounds());
+}
+
+// Verifies that bounds changes applied to a node owned by a different
+// connection are refused.
+TEST_F(ViewManagerTest, SetBoundsSecurity) {
+  ViewTreeNode* node1 = CreateNodeInParent(view_manager_1()->tree());
+  node1->SetBounds(gfx::Rect(800, 600));
+  WaitForTreeSizeToMatch(view_manager_2()->tree(), 2);
+
+  ViewTreeNode* node1_2 = view_manager_2()->GetNodeById(node1->id());
+  node1_2->SetBounds(gfx::Rect(1024, 768));
+  // Bounds change should have been rejected.
+  EXPECT_EQ(node1->bounds(), node1_2->bounds());
+}
+
+// Verifies that a node can only be destroyed by the connection that created it.
+TEST_F(ViewManagerTest, DestroySecurity) {
+  ViewTreeNode* node1 = CreateNodeInParent(view_manager_1()->tree());
+  WaitForTreeSizeToMatch(view_manager_2()->tree(), 3);
 
+  ViewTreeNode* node1_2 = view_manager_2()->GetNodeById(node1->id());
+  NodeTracker tracker2(node1_2);
+  node1_2->Destroy();
+  // Node should not have been destroyed.
+  EXPECT_TRUE(tracker2.is_valid());
 
+  NodeTracker tracker1(node1);
+  node1->Destroy();
+  EXPECT_FALSE(tracker1.is_valid());
 }
 
 }  // namespace view_manager

mojo/services/public/interfaces/view_manager/view_manager.mojom

@@ -10,6 +10,7 @@ struct INode {
   uint32 parent_id;
   uint32 node_id;
   uint32 view_id;
+  mojo.Rect bounds;
 };
 
 // Functions that mutate the hierarchy take a change id. This is an ever

mojo/services/view_manager/node.h

@@ -39,6 +39,8 @@ class MOJO_VIEW_MANAGER_EXPORT Node
 
   aura::Window* window() { return &window_; }
 
+  const gfx::Rect& bounds() const { return window_.bounds(); }
+
   const Node* GetParent() const;
   Node* GetParent() {
     return const_cast<Node*>(const_cast<const Node*>(this)->GetParent());

mojo/services/view_manager/view_manager_connection.cc

@@ -436,6 +436,7 @@ Array<INode> ViewManagerConnection::NodesToINodes(
     node_builder.set_node_id(NodeIdToTransportId(node->id()));
     node_builder.set_view_id(ViewIdToTransportId(
         node->view() ? node->view()->id() : ViewId()));
+    node_builder.set_bounds(node->bounds());
     array_builder[i] = node_builder.Finish();
   }
   return array_builder.Finish();