launchpad-reviewers team mailing list archive

[John A Meinel <john@xxxxxxxxxxxxxxxxx>]

John A Meinel has proposed merging lp:~jameinel/maas/accept-actually-filters into lp:maas.

Commit message:
Change Nodes.accept() to actually check if the requested system_ids are valid. The existing code just checked if there were enough nodes in the db (was only checking the length of Nodes.all() vs the supplied list).
This also has some tweaks so we don't load all nodes all the time. We only need to compare against the requested set.
There isn't a security hole here. You just would get PermissionDenied for an unknown system_id, rather than 404. However, I presume the 404 code is there for a reason.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~jameinel/maas/accept-actually-filters/+merge/129154

In doing performance work on the Tag code, I was looking around for 'get_nodes' calls. I came across this one which was fairly obviously broken.
-- 
https://code.launchpad.net/~jameinel/maas/accept-actually-filters/+merge/129154
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jameinel/maas/accept-actually-filters into lp:maas.

=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py	2012-10-10 13:46:34 +0000
+++ src/maasserver/api.py	2012-10-11 11:02:24 +0000
@@ -731,6 +731,16 @@
             node.accept_enlistment(request.user)
         return node
 
+    def _check_system_ids_exist(self, system_ids):
+        if not system_ids:
+            return
+        existing_nodes = Node.objects.filter(system_id__in=system_ids)
+        existing_ids = set(existing_nodes.values_list('system_id', flat=True))
+        unknown_ids = system_ids - existing_ids
+        if len(unknown_ids) > 0:
+            raise MAASAPIBadRequest(
+                "Unknown node(s): %s." % ', '.join(unknown_ids))
+
     @operation(idempotent=False)
     def accept(self, request):
         """Accept declared nodes into the MAAS.
@@ -752,20 +762,16 @@
         """
         system_ids = set(request.POST.getlist('nodes'))
         # Check the existence of these nodes first.
-        existing_ids = set(Node.objects.filter().values_list(
-            'system_id', flat=True))
-        if len(existing_ids) < len(system_ids):
-            raise MAASAPIBadRequest(
-                "Unknown node(s): %s." % ', '.join(system_ids - existing_ids))
+        self._check_system_ids_exist(system_ids)
         # Make sure that the user has the required permission.
         nodes = Node.objects.get_nodes(
             request.user, perm=NODE_PERMISSION.ADMIN, ids=system_ids)
-        ids = set(node.system_id for node in nodes)
         if len(nodes) < len(system_ids):
+            permitted_ids = set(node.system_id for node in nodes)
             raise PermissionDenied(
                 "You don't have the required permission to accept the "
                 "following node(s): %s." % (
-                    ', '.join(system_ids - ids)))
+                    ', '.join(system_ids - permitted_ids)))
         return filter(
             None, [node.accept_enlistment(request.user) for node in nodes])
 

=== modified file 'src/maasserver/tests/test_api.py'
--- src/maasserver/tests/test_api.py	2012-10-10 13:27:17 +0000
+++ src/maasserver/tests/test_api.py	2012-10-11 11:02:24 +0000
@@ -2079,6 +2079,8 @@
 
     def test_POST_accept_fails_if_node_does_not_exist(self):
         self.become_admin()
+        # Make sure there is a node, it just isn't the one being accepted
+        node = factory.make_node()
         node_id = factory.getRandomString()
         response = self.client.post(
             self.get_uri('nodes/'), {'op': 'accept', 'nodes': [node_id]})