launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #06940
[Merge] lp:~rvb/maas/maas-admin-approve-nodes into lp:maas
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 refactored NODE_TRANSITIONS (into NODE_TRANSITIONS_INFO) to include a method 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_INFO.
I've added tests to make sure that the structure of NODE_TRANSITIONS_INFO is correct. We will rely on that structure 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 10:39:56 +0000
+++ src/maasserver/models.py 2012-03-30 15:45:50 +0000
@@ -18,6 +18,7 @@
"Config",
"FileStorage",
"NODE_STATUS",
+ "NODE_TRANSITIONS_INFO",
"Node",
"MACAddress",
"SSHKey",
@@ -150,55 +151,93 @@
NODE_STATUS_CHOICES_DICT = OrderedDict(NODE_STATUS_CHOICES)
-NODE_TRANSITIONS = {
+# Information about valid node status transitions and optional associated
+# transition methods.
+# The format is:
+# {
+# old_status1: [
+# [new_status11, {
+# 'name': transition_name11,
+# 'method': method_name11, # Name of the method to call on Node
+# # to perform that transition.
+# 'permission': permission_required11
+# }],
+# [new_status12, {
+# 'name': transition_name12,
+# 'method': method_name12,
+# 'permission': permission_required12
+# }],
+# [new_status13], # Having a transition method is optional.
+# [new_status14],
+# ...
+# old_status2: [
+# ...
+# }
+NODE_TRANSITIONS_INFO = {
None: [
- NODE_STATUS.DECLARED,
- NODE_STATUS.MISSING,
- NODE_STATUS.RETIRED,
+ [NODE_STATUS.DECLARED], # Anonymous enlistment.
+ [NODE_STATUS.READY], # Authenticated enlistement.
+ [NODE_STATUS.MISSING],
+ [NODE_STATUS.RETIRED],
],
NODE_STATUS.DECLARED: [
- NODE_STATUS.COMMISSIONING,
- NODE_STATUS.MISSING,
- NODE_STATUS.READY,
- NODE_STATUS.RETIRED,
+ [NODE_STATUS.COMMISSIONING],
+ [NODE_STATUS.MISSING],
+ [NODE_STATUS.READY, {
+ 'name':"Enlist node", 'method': 'accept_enlistment',
+ 'permission':'admin'}],
+ [NODE_STATUS.RETIRED],
],
NODE_STATUS.COMMISSIONING: [
- NODE_STATUS.FAILED_TESTS,
- NODE_STATUS.READY,
- NODE_STATUS.RETIRED,
- NODE_STATUS.MISSING,
+ [NODE_STATUS.FAILED_TESTS],
+ [NODE_STATUS.READY],
+ [NODE_STATUS.RETIRED],
+ [NODE_STATUS.MISSING],
],
NODE_STATUS.READY: [
- NODE_STATUS.ALLOCATED,
- NODE_STATUS.RESERVED,
- NODE_STATUS.RETIRED,
- NODE_STATUS.MISSING,
+ [NODE_STATUS.ALLOCATED],
+ [NODE_STATUS.RESERVED],
+ [NODE_STATUS.RETIRED],
+ [NODE_STATUS.MISSING],
],
NODE_STATUS.RESERVED: [
- NODE_STATUS.READY,
- NODE_STATUS.ALLOCATED,
- NODE_STATUS.RETIRED,
- NODE_STATUS.MISSING,
+ [NODE_STATUS.READY],
+ [NODE_STATUS.ALLOCATED],
+ [NODE_STATUS.RETIRED],
+ [NODE_STATUS.MISSING],
],
NODE_STATUS.ALLOCATED: [
- NODE_STATUS.READY,
- NODE_STATUS.RETIRED,
- NODE_STATUS.MISSING,
+ [NODE_STATUS.READY],
+ [NODE_STATUS.RETIRED],
+ [NODE_STATUS.MISSING],
],
NODE_STATUS.MISSING: [
- NODE_STATUS.DECLARED,
- NODE_STATUS.READY,
- NODE_STATUS.ALLOCATED,
- NODE_STATUS.COMMISSIONING,
+ [NODE_STATUS.DECLARED],
+ [NODE_STATUS.READY],
+ [NODE_STATUS.ALLOCATED],
+ [NODE_STATUS.COMMISSIONING],
],
NODE_STATUS.RETIRED: [
- NODE_STATUS.DECLARED,
- NODE_STATUS.READY,
- NODE_STATUS.MISSING,
+ [NODE_STATUS.DECLARED],
+ [NODE_STATUS.READY],
+ [NODE_STATUS.MISSING],
],
}
+# Valid node status transitions:
+# The format is:
+# {
+# old_status1: [new_status11, new_status12, ...],
+# old_status2: [new_status21, new_status22, ...],
+# ...
+# }
+NODE_TRANSITIONS = dict(
+ (old_status, [info[0] for info in new_statuses_infos])
+ for old_status, new_statuses_infos in NODE_TRANSITIONS_INFO.items()
+ )
+
+
class NODE_AFTER_COMMISSIONING_ACTION:
"""The vocabulary of a `Node`'s possible value for its field
after_commissioning_action.
@@ -516,6 +555,30 @@
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_INFO.get(self.status, ())
+ for node_transition in node_transitions:
+ if len(node_transition) == 2:
+ # The transition has a method associated with it.
+ new_status, transition_info = node_transition
+ if user.has_perm(transition_info['permission'], self):
+ # The user can perform the transition.
+ valid_transitions.append(transition_info)
+ return valid_transitions
+
def display_status(self):
"""Return status text as displayed to the user.
@@ -536,7 +599,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://
@@ -1176,6 +1239,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-30 15:45:50 +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 10:39:56 +0000
+++ src/maasserver/tests/test_models.py 2012-03-30 15:45:50 +0000
@@ -28,6 +28,7 @@
NodeStateViolation,
PermissionDenied,
)
+import inspect
from maasserver.models import (
Config,
create_auth_token,
@@ -48,6 +49,7 @@
SYSTEM_USERS,
UserProfile,
validate_ssh_public_key,
+ NODE_TRANSITIONS_INFO,
)
from maasserver.provisioning import get_provisioning_api_proxy
from maasserver.testing import get_data
@@ -301,6 +303,65 @@
# 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_INFO."""
+
+ def test_NODE_TRANSITIONS_INFO_initial_states(self):
+ for initial_state in NODE_TRANSITIONS_INFO:
+ self.assertIn(
+ initial_state, NODE_STATUS_CHOICES_DICT.keys() + [None])
+
+ def test_NODE_TRANSITIONS_INFO_destination_state(self):
+ for transitions in NODE_TRANSITIONS_INFO.values():
+ for transition in transitions:
+ destination_state = transition[0]
+ self.assertIn(
+ destination_state, NODE_STATUS_CHOICES_DICT.keys())
+
+ def test_NODE_TRANSITIONS_INFO_transitions(self):
+ node = factory.make_node()
+ for transitions in NODE_TRANSITIONS_INFO.values():
+ for transition in transitions:
+ has_method_transition = len(transition) == 2
+ if has_method_transition:
+ method_transition = transition[1]
+ self.assertItemsEqual(
+ ['name', 'method', 'permission'],
+ method_transition.keys())
+ # The 'method' corresponds to the name of a method
+ # on a Node.
+ self.assertTrue(hasattr(node, method_transition['method']))
+ self.assertTrue(
+ inspect.ismethod(
+ getattr(node, method_transition['method'])))
+ # The 'permission' is a valid Node permission.
+ self.assertIn(
+ method_transition['permission'],
+ ['edit', 'view', 'admin'])
+ # The 'name' is a basestring.
+ self.assertIsInstance(
+ method_transition['name'], basestring)
+
class GetDbStateTest(TestCase):
"""Testing for the method `get_db_state`."""
Follow ups
