launchpad-reviewers team mailing list archive

[Raphaël Badin <raphael.badin@xxxxxxxxxxxxx>]

Raphaël Badin has proposed merging lp:~rvb/maas/power-param-api into lp:maas.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~rvb/maas/power-param-api/+merge/107220

This branch is a preparatory branch for the upcoming work on exposing power_type and power_parameters on the API.  It cleans up the API call that updates a node:
- restrict the allowed parameters to a subset of a node's fields.
- return a proper error message if one tries to update a node and specifies an unknown field.
-- 
https://code.launchpad.net/~rvb/maas/power-param-api/+merge/107220
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rvb/maas/power-param-api into lp:maas.

=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py	2012-04-30 07:31:02 +0000
+++ src/maasserver/api.py	2012-05-24 14:43:17 +0000
@@ -350,21 +350,31 @@
         raise Unauthorized("Unknown OAuth token.")
 
 
-NODE_FIELDS = (
+# Node's fields exposed on the API.
+DISPLAYED_NODE_FIELDS = (
     'system_id',
     'hostname',
     ('macaddress_set', ('mac_address',)),
     'architecture',
     'status',
+    'power_type',
+    'power_parameters',
     )
 
 
+EDITABLE_NODE_FIELDS = (
+    'hostname',
+    'architecture',
+    'power_type',
+     )
+
+
 @api_operations
 class NodeHandler(BaseHandler):
     """Manage individual Nodes."""
     allowed_methods = ('GET', 'DELETE', 'POST', 'PUT')
     model = Node
-    fields = NODE_FIELDS
+    fields = DISPLAYED_NODE_FIELDS
 
     def read(self, request, system_id):
         """Read a specific Node."""
@@ -372,10 +382,28 @@
             system_id=system_id, user=request.user, perm=NODE_PERMISSION.VIEW)
 
     def update(self, request, system_id):
-        """Update a specific Node."""
+        """Update a specific Node.
+
+        :param hostname: The new hostname for this node.
+        :type hostname: basestring
+        :param architecture: The new architecture for this node (see
+            vocabulary `ARCHITECTURE`).
+        :type architecture: basestring
+        :param power_type: The new power type for this node (see
+            vocabulary `POWER_TYPE`).
+        :type power_type: basestring
+        """
+
         node = Node.objects.get_node_or_404(
             system_id=system_id, user=request.user, perm=NODE_PERMISSION.EDIT)
-        for key, value in request.data.items():
+        items = [(key, value) for key, value in request.data.items()]
+        unknown_fields = set(
+            [key for key, unused in items]).difference(EDITABLE_NODE_FIELDS)
+        if len(unknown_fields) != 0:
+            raise PermissionDenied(
+                "Unable to set field(s): %s. Allowed fields are: %s." % (
+                    ''.join(unknown_fields), ''.join(EDITABLE_NODE_FIELDS)))
+        for key, value in items:
             setattr(node, key, value)
         node.save()
         return node
@@ -470,7 +498,7 @@
 class AnonNodesHandler(AnonymousBaseHandler):
     """Create Nodes."""
     allowed_methods = ('GET', 'POST',)
-    fields = NODE_FIELDS
+    fields = DISPLAYED_NODE_FIELDS
 
     @api_exported('POST')
     def new(self, request):

=== modified file 'src/maasserver/tests/test_api.py'
--- src/maasserver/tests/test_api.py	2012-05-23 16:32:15 +0000
+++ src/maasserver/tests/test_api.py	2012-05-24 14:43:17 +0000
@@ -35,6 +35,7 @@
 from fixtures import Fixture
 from maasserver import api
 from maasserver.api import (
+    EDITABLE_NODE_FIELDS,
     extract_constraints,
     extract_oauth_key,
     extract_oauth_key_from_auth_header,
@@ -442,8 +443,13 @@
         parsed_result = json.loads(response.content)
         self.assertItemsEqual(
             [
-                'hostname', 'system_id', 'macaddress_set', 'architecture',
+                'hostname',
+                'system_id',
+                'macaddress_set',
+                'architecture',
                 'status',
+                'power_type',
+                'power_parameters',
             ],
             list(parsed_result))
 
@@ -483,6 +489,8 @@
                 'macaddress_set',
                 'architecture',
                 'status',
+                'power_type',
+                'power_parameters',
                 'resource_uri',
             ],
             list(parsed_result))
@@ -524,8 +532,14 @@
         parsed_result = json.loads(response.content)
         self.assertItemsEqual(
             [
-                'hostname', 'system_id', 'macaddress_set', 'architecture',
-                'status', 'resource_uri',
+                'hostname',
+                'system_id',
+                'macaddress_set',
+                'architecture',
+                'status',
+                'power_type',
+                'power_parameters',
+                'resource_uri',
             ],
             list(parsed_result))
 
@@ -844,6 +858,21 @@
         self.assertEqual(0, Node.objects.filter(hostname='diane').count())
         self.assertEqual(1, Node.objects.filter(hostname='francis').count())
 
+    def test_PUT_rejects_unknown_fields(self):
+        node = factory.make_node(owner=self.logged_in_user)
+        field = factory.getRandomString()
+        response = self.client.put(
+            self.get_node_uri(node),
+            {field: factory.getRandomString}
+            )
+
+        error_msg = (
+            "Unable to set field(s): %s. Allowed fields are: %s." % (
+                (field, ''.join(EDITABLE_NODE_FIELDS))))
+        self.assertEqual(
+            (httplib.FORBIDDEN, error_msg),
+            (response.status_code, response.content))
+
     def test_resource_uri_points_back_at_node(self):
         # When a Node is returned by the API, the field 'resource_uri'
         # provides the URI for this Node.