launchpad-reviewers team mailing list archive

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

Raphaël Badin has proposed merging lp:~rvb/maas/maas-admin-approve-nodes into lp:maas.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~rvb/maas/maas-admin-approve-nodes/+merge/100175

This branch features the backend side of "An admin can enlist nodes" story.  I've add NODE_TRANSITIONS_METHODS which features transition information that associates a status change to a method being called on the node.  The goal is to add actions to the UI only by modifying NODE_TRANSITIONS_METHODS.

I've added tests to make sure that the structure of both NODE_TRANSITIONS_METHODS and NODE_TRANSITIONS are correct.  We will rely on these structures in the follow-up branch (https://code.launchpad.net/~rvb/maas/maas-admin-approve-nodes-ui/+merge/100176). Also, don't be surprised by the new permission check that returns always false: Django replies always true when a permission check is done on a super user so we only have to take care of the case where the user is not a super user.
-- 
https://code.launchpad.net/~rvb/maas/maas-admin-approve-nodes/+merge/100175
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rvb/maas/maas-admin-approve-nodes into lp:maas.

=== modified file 'src/maasserver/models.py'
--- src/maasserver/models.py	2012-03-30 17:01:12 +0000
+++ src/maasserver/models.py	2012-03-31 20:53:21 +0000
@@ -18,6 +18,8 @@
     "Config",
     "FileStorage",
     "NODE_STATUS",
+    "NODE_TRANSITIONS",
+    "NODE_TRANSITIONS_METHODS",
     "Node",
     "MACAddress",
     "SSHKey",
@@ -151,6 +153,17 @@
 NODE_STATUS_CHOICES_DICT = OrderedDict(NODE_STATUS_CHOICES)
 
 
+# Information about valid node status transitions.
+# The format is:
+# {
+#  old_status1: [
+#      new_status11,
+#      new_status12,
+#      new_status13,
+#      ],
+# ...
+# }
+#
 NODE_TRANSITIONS = {
     None: [
         NODE_STATUS.DECLARED,
@@ -199,6 +212,35 @@
         ],
     }
 
+# Node transitions methods.
+# The format is:
+# {
+#     old_status1: [
+#         {
+#             'name': transition_name11,
+#             'method': method_name11,   # Name of the method to call on Node
+#                                        # to perform that transition.
+#             'permission': permission_required11,
+#         },
+#         {
+#             'name': transition_name12,
+#             'method': method_name12,   # Name of the method to call on Node
+#                                        # to perform that transition.
+#             'permission': permission_required12,
+#         },
+#     ]
+# ...
+#
+NODE_TRANSITIONS_METHODS = {
+    NODE_STATUS.DECLARED: [
+        {
+            'name': "Enlist node",
+            'method': 'accept_enlistment',
+            'permission': 'admin'
+        }
+    ],
+}
+
 
 class NODE_AFTER_COMMISSIONING_ACTION:
     """The vocabulary of a `Node`'s possible value for its field
@@ -517,6 +559,27 @@
             self.full_clean()
         return super(Node, self).save(*args, **kwargs)
 
+    def available_transition_methods(self, user):
+        """Return the transitions that this user is allowed to perform on
+        this node.
+
+        :param user: The user used to perform the permission checks.  Only the
+            transitions available to this user will be returned.
+        :type user: :class:`django.contrib.auth.models.User`
+        :return: A list of transition dicts (each dict contains 3 values:
+            'name': the name of the transition, 'permission': the permission
+            required to perform this transition, 'method': the name of the
+            method to execute on the node to perform the transition).
+        :rtype: Sequence
+        """
+        valid_transitions = []
+        node_transitions = NODE_TRANSITIONS_METHODS.get(self.status, ())
+        for node_transition in node_transitions:
+            if user.has_perm(node_transition['permission'], self):
+                # The user can perform the transition.
+                valid_transitions.append(node_transition)
+        return valid_transitions
+
     def display_status(self):
         """Return status text as displayed to the user.
 
@@ -537,7 +600,7 @@
         """Add a new MAC Address to this `Node`.
 
         :param mac_address: The MAC Address to be added.
-        :type mac_address: str
+        :type mac_address: basestring
         :raises: django.core.exceptions.ValidationError_
 
         .. _django.core.exceptions.ValidationError: https://
@@ -1185,6 +1248,9 @@
             return obj.owner in (None, user)
         elif perm == 'edit':
             return obj.owner == user
+        elif perm == 'admin':
+            # 'admin' permission is solely granted to superusers.
+            return False
         else:
             raise NotImplementedError(
                 'Invalid permission check (invalid permission name).')

=== modified file 'src/maasserver/tests/test_auth.py'
--- src/maasserver/tests/test_auth.py	2012-03-22 11:35:03 +0000
+++ src/maasserver/tests/test_auth.py	2012-03-31 20:53:21 +0000
@@ -124,6 +124,11 @@
         self.assertTrue(backend.has_perm(
             user, 'edit', make_allocated_node(owner=user)))
 
+    def test_user_has_not_admin_permission_on_node(self):
+        backend = MAASAuthorizationBackend()
+        user = factory.make_user()
+        self.assertFalse(backend.has_perm(user, 'admin', factory.make_node()))
+
 
 class TestNodeVisibility(TestCase):
 

=== modified file 'src/maasserver/tests/test_models.py'
--- src/maasserver/tests/test_models.py	2012-03-30 17:01:12 +0000
+++ src/maasserver/tests/test_models.py	2012-03-31 20:53:21 +0000
@@ -28,6 +28,7 @@
     NodeStateViolation,
     PermissionDenied,
     )
+import inspect
 from maasserver.models import (
     Config,
     create_auth_token,
@@ -48,6 +49,8 @@
     SYSTEM_USERS,
     UserProfile,
     validate_ssh_public_key,
+    NODE_TRANSITIONS,
+    NODE_TRANSITIONS_METHODS,
     )
 from maasserver.provisioning import get_provisioning_api_proxy
 from maasserver.testing import get_data
@@ -302,6 +305,63 @@
         # The test is that this does not raise an error.
         pass
 
+    def test_available_transition_methods_for_declared_node_admin(self):
+        # An admin has access to the "Enlist node" transition for a
+        # 'Declared' node.
+        admin = factory.make_admin()
+        node = factory.make_node(status=NODE_STATUS.DECLARED)
+        self.assertItemsEqual(
+            [{
+                'method': 'accept_enlistment',
+                'name': 'Enlist node',
+                'permission': 'admin',
+            }],
+            node.available_transition_methods(admin))
+
+    def test_available_transition_methods_for_declared_node_simple_user(self):
+        # A simple user sees not transition for a 'Declared' node.
+        user = factory.make_user()
+        node = factory.make_node(status=NODE_STATUS.DECLARED)
+        self.assertItemsEqual([], node.available_transition_methods(user))
+
+
+class NodeTransitionsInfoTests(TestCase):
+    """Test the structure of NODE_TRANSITIONS and NODE_TRANSITIONS_METHODS."""
+
+    def test_NODE_TRANSITIONS_initial_states(self):
+        for initial_state in NODE_TRANSITIONS:
+            self.assertIn(
+                initial_state, NODE_STATUS_CHOICES_DICT.keys() + [None])
+
+    def test_NODE_TRANSITIONS_destination_state(self):
+        for transitions in NODE_TRANSITIONS.values():
+            for destination_state in transitions:
+                self.assertIn(
+                    destination_state, NODE_STATUS_CHOICES_DICT.keys())
+
+    def test_NODE_TRANSITION_METHODS_initial_states(self):
+        for state in NODE_TRANSITIONS_METHODS:
+            self.assertIn(
+                state, NODE_STATUS_CHOICES_DICT.keys() + [None])
+
+    def test_NODE_TRANSITIONS_METHODS_transitions(self):
+        node = factory.make_node()
+        for transitions in NODE_TRANSITIONS_METHODS.values():
+            for transition in transitions:
+                self.assertItemsEqual(
+                    ['name', 'method', 'permission'], transition.keys())
+                # The 'method' corresponds to the name of a method
+                # on a Node.
+                self.assertTrue(hasattr(node, transition['method']))
+                self.assertTrue(
+                    inspect.ismethod(
+                        getattr(node, transition['method'])))
+                # The 'permission' is a valid Node permission.
+                self.assertIn(
+                    transition['permission'], ['edit', 'view', 'admin'])
+                # The 'name' is a basestring.
+                self.assertIsInstance(transition['name'], basestring)
+
 
 class GetDbStateTest(TestCase):
     """Testing for the method `get_db_state`."""