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-04-01 19:34:22 +0000
@@ -18,6 +18,7 @@
     "Config",
     "FileStorage",
     "NODE_STATUS",
+    "NODE_TRANSITIONS",
     "Node",
     "MACAddress",
     "SSHKey",
@@ -151,6 +152,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,
@@ -537,7 +549,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 +1197,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-04-01 19:34:22 +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-04-01 19:34:22 +0000
@@ -44,6 +44,7 @@
     NODE_STATUS,
     NODE_STATUS_CHOICES,
     NODE_STATUS_CHOICES_DICT,
+    NODE_TRANSITIONS,
     SSHKey,
     SYSTEM_USERS,
     UserProfile,
@@ -303,6 +304,21 @@
         pass
 
 
+class NodeTransitionsTests(TestCase):
+    """Test the structure of NODE_TRANSITIONS."""
+
+    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())
+
+
 class GetDbStateTest(TestCase):
     """Testing for the method `get_db_state`."""
 

=== modified file 'src/maasserver/tests/test_views.py'
--- src/maasserver/tests/test_views.py	2012-03-29 11:40:39 +0000
+++ src/maasserver/tests/test_views.py	2012-04-01 19:34:22 +0000
@@ -30,6 +30,8 @@
 from maasserver.models import (
     Config,
     NODE_AFTER_COMMISSIONING_ACTION,
+    NODE_STATUS,
+    NODE_STATUS_CHOICES_DICT,
     POWER_TYPE_CHOICES,
     SSHKey,
     UserProfile,
@@ -48,8 +50,10 @@
     make_path_relative,
     )
 from maasserver.views import (
+    available_transition_methods,
     get_longpoll_context,
     get_yui_location,
+    NODE_TRANSITIONS_METHODS,
     proxy_to_longpoll,
     )
 from maastesting.rabbit import uses_rabbit_fixture
@@ -240,6 +244,50 @@
         self.assertEqual("Invalid file type requested.", response.content)
 
 
+class NodeTransitionsMethodsTests(TestCase):
+    """Test the structure of NODE_TRANSITIONS_METHODS."""
+
+    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):
+        for transitions in NODE_TRANSITIONS_METHODS.values():
+            for transition in transitions:
+                self.assertItemsEqual(
+                    ['display', 'name', 'permission'], transition.keys())
+                # The 'permission' is a valid Node permission.
+                self.assertIn(
+                    transition['permission'], ['edit', 'view', 'admin'])
+                # The 'name' is a basestring.
+                self.assertIsInstance(transition['name'], basestring)
+                # The 'display' is a basestring.
+                self.assertIsInstance(transition['display'], basestring)
+
+
+class AvailableTransitionsMethodsTests(TestCase):
+
+    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(
+            [{
+                'display': 'Enlist node',
+                'name': 'accept_enlistment_action',
+                'permission': 'admin',
+            }],
+            available_transition_methods(node, 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([], available_transition_methods(node, user))
+
+
 class TestUtilities(TestCase):
 
     def test_get_yui_location_if_static_root_is_none(self):

=== modified file 'src/maasserver/views.py'
--- src/maasserver/views.py	2012-03-28 16:13:04 +0000
+++ src/maasserver/views.py	2012-04-01 19:34:22 +0000
@@ -14,9 +14,11 @@
     "AccountsDelete",
     "AccountsEdit",
     "AccountsView",
+    "available_transition_methods",
     "combo_view",
     "login",
     "logout",
+    "NODE_TRANSITIONS_METHODS",
     "NodeListView",
     "NodesCreateView",
     "NodeView",
@@ -83,10 +85,58 @@
 from maasserver.messages import messaging
 from maasserver.models import (
     Node,
+    NODE_STATUS,
     SSHKey,
     UserProfile,
     )
 
+# Node transitions methods.
+# The format is:
+# {
+#     old_status1: [
+#         {
+#             'display': display_string11,  # The name of the transition
+#                                           # (to be displayed in the UI).
+#             'name': transition_name11,  # The name of the transition.
+#             'permission': permission_required11,
+#         },
+#     ]
+# ...
+#
+NODE_TRANSITIONS_METHODS = {
+    NODE_STATUS.DECLARED: [
+        {
+            'display': "Enlist node",
+            'name': "accept_enlistment_action",
+            'permission': 'admin'
+        },
+    ],
+}
+
+
+def available_transition_methods(node, user):
+    """Return the transitions that this user is allowed to perform on
+    a node.
+
+    :param node: The node for which the check should be performed.
+    :type node: :class:`maasserver.models.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(node.status, ())
+    for node_transition in node_transitions:
+        if user.has_perm(node_transition['permission'], node):
+            # The user can perform the transition.
+            valid_transitions.append(node_transition)
+    return valid_transitions
+
 
 def login(request):
     extra_context = {