launchpad-reviewers team mailing list archive

[John A Meinel <john@xxxxxxxxxxxxxxxxx>]

John A Meinel has proposed merging lp:~jameinel/maas/tags-exposing-nodes into lp:maas.

Commit message:
/tag/<tagname>?op=nodes no longer exposes private nodes.

Respect that if there is a node owner, it doesn't get shown to anyone but superusers and that owner.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~jameinel/maas/tags-exposing-nodes/+merge/126438

See commit message.
-- 
https://code.launchpad.net/~jameinel/maas/tags-exposing-nodes/+merge/126438
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jameinel/maas/tags-exposing-nodes into lp:maas.

=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py	2012-09-26 08:54:29 +0000
+++ src/maasserver/api.py	2012-09-26 12:10:28 +0000
@@ -1303,10 +1303,7 @@
     @api_exported('POST')
     def nodes(self, request, name):
         """Get the list of nodes that have this tag."""
-        tag = Tag.objects.get_tag_or_404(name=name, user=request.user)
-        # XXX: JAM 2012-09-25 We need to filter the node set returned by the
-        #      visibility defined by the user.
-        return tag.node_set.all()
+        return Tag.objects.get_nodes(name, user=request.user)
 
     @classmethod
     def resource_uri(cls, tag=None):

=== modified file 'src/maasserver/models/tag.py'
--- src/maasserver/models/tag.py	2012-09-23 14:50:34 +0000
+++ src/maasserver/models/tag.py	2012-09-26 12:10:28 +0000
@@ -21,6 +21,7 @@
     CharField,
     Manager,
     TextField,
+    Q,
     )
 from django.shortcuts import get_object_or_404
 from maasserver import DefaultMeta
@@ -55,6 +56,23 @@
         tag = get_object_or_404(Tag, name=name)
         return tag
 
+    def get_nodes(self, tag_name, user):
+        """Get the list of nodes that have this tag.
+
+        This list is restricted to only nodes that the user has VIEW permission
+        for.
+        """
+        tag = self.get_tag_or_404(name=tag_name, user=user)
+        # The privacy logic is taken from Node. Note that we could filter in
+        # python by iterating over all nodes and checking
+        #   user.has_perm(VIEW, node)
+        # It seems better to do this in the DB, though.
+        if user.is_superuser:
+            return tag.node_set.all()
+        else:
+            return tag.node_set.filter(
+                Q(owner__isnull=True) | Q(owner=user))
+
 
 class Tag(CleanSave, TimestampedModel):
     """A `Tag` is a label applied to a `Node`.

=== modified file 'src/maasserver/tests/test_api.py'
--- src/maasserver/tests/test_api.py	2012-09-26 08:54:29 +0000
+++ src/maasserver/tests/test_api.py	2012-09-26 12:10:28 +0000
@@ -2275,6 +2275,28 @@
         self.assertEqual([node1.system_id],
                          [r['system_id'] for r in parsed_result])
 
+    def test_POST_nodes_hides_invisible_nodes(self):
+        user2 = factory.make_user()
+        node1 = factory.make_node()
+        node1.set_hardware_details('<node><foo /></node>')
+        node2 = factory.make_node(status=NODE_STATUS.ALLOCATED, owner=user2)
+        node2.set_hardware_details('<node><bar /></node>')
+        tag = factory.make_tag(definition='/node')
+        tag.populate_nodes()
+        response = self.client.post(self.get_tag_uri(tag), {'op': 'nodes'})
+
+        self.assertEqual(httplib.OK, response.status_code)
+        parsed_result = json.loads(response.content)
+        self.assertEqual([node1.system_id],
+                         [r['system_id'] for r in parsed_result])
+        # However, for the other user, they should see the result
+        client2 = OAuthAuthenticatedClient(user2)
+        response = client2.post(self.get_tag_uri(tag), {'op': 'nodes'})
+        self.assertEqual(httplib.OK, response.status_code)
+        parsed_result = json.loads(response.content)
+        self.assertItemsEqual([node1.system_id, node2.system_id],
+                              [r['system_id'] for r in parsed_result])
+
     def test_PUT_invalid_definition(self):
         self.become_admin()
         node = factory.make_node()

=== modified file 'src/maasserver/tests/test_tag.py'
--- src/maasserver/tests/test_tag.py	2012-09-26 08:40:33 +0000
+++ src/maasserver/tests/test_tag.py	2012-09-26 12:10:28 +0000
@@ -15,6 +15,8 @@
 from django.db import transaction
 from django.db.utils import DatabaseError
 from maastesting.djangotestcase import TransactionTestCase
+from maasserver.enum import NODE_STATUS
+from maasserver.models import Tag
 from maasserver.testing.factory import factory
 from maasserver.testing.testcase import TestCase
 
@@ -79,6 +81,39 @@
         self.assertItemsEqual([tag1.name], node1.tag_names())
         self.assertItemsEqual([tag2.name], node2.tag_names())
 
+    def test_get_nodes_returns_unowned_nodes(self):
+        user1 = factory.make_user()
+        node1 = factory.make_node()
+        tag = factory.make_tag()
+        node1.tags.add(tag)
+        self.assertItemsEqual([node1], Tag.objects.get_nodes(tag.name, user1))
+
+    def test_get_nodes_returns_self_owned_nodes(self):
+        user1 = factory.make_user()
+        node1 = factory.make_node(owner=user1)
+        tag = factory.make_tag()
+        node1.tags.add(tag)
+        self.assertItemsEqual([node1], Tag.objects.get_nodes(tag.name, user1))
+
+    def test_get_nodes_doesnt_return_other_owned_nodes(self):
+        user1 = factory.make_user()
+        user2 = factory.make_user()
+        node1 = factory.make_node(owner=user1)
+        tag = factory.make_tag()
+        node1.tags.add(tag)
+        self.assertItemsEqual([], Tag.objects.get_nodes(tag.name, user2))
+
+    def test_get_nodes_returns_everything_for_superuser(self):
+        user1 = factory.make_user()
+        user2 = factory.make_user()
+        node1 = factory.make_node(owner=user1)
+        node2 = factory.make_node()
+        tag = factory.make_tag()
+        node1.tags.add(tag)
+        node2.tags.add(tag)
+        self.assertItemsEqual([node1, node2],
+                              Tag.objects.get_nodes(tag.name, user2))
+
 
 class TestTagTransactions(TransactionTestCase):