launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #06958
[Merge] lp:~rvb/maas/maas-admin-approve-nodes-ui2 into lp:maas
Raphaël Badin has proposed merging lp:~rvb/maas/maas-admin-approve-nodes-ui2 into lp:maas.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~rvb/maas/maas-admin-approve-nodes-ui2/+merge/100377
This branch is a refactored version of https://code.launchpad.net/~rvb/maas/maas-admin-approve-nodes and https://code.launchpad.net/~rvb/maas/maas-admin-approve-nodes-ui. available_transition_methods has been moved in NodeTransitionForm, I've added comments where appropriate and renamed variables to more meaningful names in NodeTransitionForm.
--
https://code.launchpad.net/~rvb/maas/maas-admin-approve-nodes-ui2/+merge/100377
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rvb/maas/maas-admin-approve-nodes-ui2 into lp:maas.
=== modified file 'src/maasserver/forms.py'
--- src/maasserver/forms.py 2012-03-29 11:40:39 +0000
+++ src/maasserver/forms.py 2012-04-02 10:08:18 +0000
@@ -11,10 +11,12 @@
__metaclass__ = type
__all__ = [
"CommissioningForm",
+ "get_transition_form",
"HostnameFormField",
"NodeForm",
"MACAddressForm",
"MAASAndNetworkForm",
+ "NodeTransitionForm",
"SSHKeyForm",
"UbuntuForm",
"UIAdminNodeEditForm",
@@ -26,8 +28,14 @@
UserChangeForm,
UserCreationForm,
)
-from django.contrib.auth.models import User
-from django.core.exceptions import ValidationError
+from django.contrib.auth.models import (
+ AnonymousUser,
+ User,
+ )
+from django.core.exceptions import (
+ PermissionDenied,
+ ValidationError,
+ )
from django.core.validators import URLValidator
from django.forms import (
CharField,
@@ -43,6 +51,7 @@
Node,
NODE_AFTER_COMMISSIONING_ACTION,
NODE_AFTER_COMMISSIONING_ACTION_CHOICES,
+ NODE_STATUS,
SSHKey,
UserProfile,
)
@@ -184,6 +193,104 @@
return node
+# 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'
+ },
+ ],
+}
+
+
+class NodeTransitionForm(forms.Form):
+ """A form used to perform a status change on a Node.
+
+ That form class should not be used directly but through subclasses
+ created using `get_transition_form`.
+ """
+
+ user = AnonymousUser()
+
+ # The name of the input button used with this form.
+ input_name = 'node_transition'
+
+ def __init__(self, instance, *args, **kwargs):
+ super(NodeTransitionForm, self).__init__(*args, **kwargs)
+ self.node = instance
+ self.transition_buttons = self.available_transition_methods(
+ self.node, self.user)
+ # Create a convenient dict to fetch the transition name and
+ # permission to be checked from the button name.
+ self.transition_dict = dict(
+ [(transition['display'],
+ (transition['name'], transition['permission']))
+ for transition in self.transition_buttons])
+
+ def available_transition_methods(self, 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 save(self):
+ transition_name = self.data.get(self.input_name)
+ action_name, permission = self.transition_dict.get(
+ transition_name, (None, None))
+ if action_name is not None:
+ if not self.user.has_perm(permission, self.node):
+ raise PermissionDenied()
+ if action_name == 'accept_enlistment_action':
+ self.node.accept_enlistment()
+ else:
+ raise PermissionDenied()
+
+
+def get_transition_form(user):
+ """Return a class derived from NodeTransitionForm for a specific user.
+
+ :param user: The user for which to build a form derived from
+ NodeTransitionForm.
+ :type user: :class:`django.contrib.auth.models.User`
+ :return: A form class derived from NodeTransitionForm.
+ :rtype: class:`django.forms.Form`
+ """
+ return type(
+ str("SpecificNodeTransitionForm"), (NodeTransitionForm,),
+ {'user': user})
+
+
class ProfileForm(ModelForm):
# We use the field 'last_name' to store the user's full name (and
# don't display Django's 'first_name' field).
=== modified file 'src/maasserver/models.py'
--- src/maasserver/models.py 2012-03-30 17:01:12 +0000
+++ src/maasserver/models.py 2012-04-02 10:08:18 +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://
@@ -1170,6 +1182,9 @@
supports_object_permissions = True
def has_perm(self, user, perm, obj=None):
+ # Note that a check for a superuser will never reach this code
+ # because Django will return True (as an optimization) for every
+ # permission check performed on a superuser.
if not user.is_active:
# Deactivated users, and in particular the node-init user,
# are prohibited from accessing maasserver services.
@@ -1185,6 +1200,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/templates/maasserver/node_view.html'
--- src/maasserver/templates/maasserver/node_view.html 2012-03-21 18:53:44 +0000
+++ src/maasserver/templates/maasserver/node_view.html 2012-04-02 10:08:18 +0000
@@ -12,7 +12,19 @@
<a href="{% url 'node-edit' node.id %}" class="button secondary">
Edit node
</a>
- {% endif%}
+ {% endif %}
+ {% if form.transition_buttons %}
+ <form id="node_actions" method="post" action=".">
+ {% for transition in form.transition_buttons %}
+ {% if forloop.first %}<br /><br /><h4>Actions</h4>{% endif %}
+ <input class="secondary"
+ type="submit"
+ name="{{ form.input_name }}"
+ value="{{ transition.display }}" />
+ {% if not forloop.last %}<br /><br />{% endif %}
+ {% endfor %}
+ </form>
+ {% endif %}
</div>
{% endblock %}
=== 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-02 10:08:18 +0000
@@ -124,6 +124,12 @@
self.assertTrue(backend.has_perm(
user, 'edit', make_allocated_node(owner=user)))
+ def test_user_has_no_admin_permission_on_node(self):
+ # 'admin' permission on nodes is granted to super users only.
+ 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_forms.py'
--- src/maasserver/tests/test_forms.py 2012-03-22 17:34:36 +0000
+++ src/maasserver/tests/test_forms.py 2012-04-02 10:08:18 +0000
@@ -14,13 +14,19 @@
import random
from django import forms
-from django.core.exceptions import ValidationError
+from django.core.exceptions import (
+ PermissionDenied,
+ ValidationError,
+ )
from django.http import QueryDict
from maasserver.forms import (
ConfigForm,
EditUserForm,
+ get_transition_form,
HostnameFormField,
NewUserCreationForm,
+ NODE_TRANSITIONS_METHODS,
+ NodeTransitionForm,
NodeWithMACAddressesForm,
ProfileForm,
UIAdminNodeEditForm,
@@ -32,6 +38,8 @@
Config,
DEFAULT_CONFIG,
NODE_AFTER_COMMISSIONING_ACTION_CHOICES,
+ NODE_STATUS,
+ NODE_STATUS_CHOICES_DICT,
POWER_TYPE_CHOICES,
)
from maasserver.testing.factory import factory
@@ -251,6 +259,97 @@
self.assertItemsEqual(['', user.id], user_ids)
+class NodeTransitionsMethodsTests(TestCase):
+ """Test the structure of NODE_TRANSITIONS_METHODS."""
+
+ def test_NODE_TRANSITION_METHODS_initial_states(self):
+ allowed_states = set(NODE_STATUS_CHOICES_DICT.keys() + [None])
+
+ self.assertTrue(set(NODE_TRANSITIONS_METHODS.keys()) <= allowed_states)
+
+
+class TestNodeTransitionForm(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)
+ form = get_transition_form(admin)(node)
+ self.assertItemsEqual(
+ [{
+ 'display': 'Enlist node',
+ 'name': 'accept_enlistment_action',
+ 'permission': 'admin',
+ }],
+ form.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)
+ form = get_transition_form(user)(node)
+ self.assertItemsEqual(
+ [], form.available_transition_methods(node, user))
+
+ def test_get_transition_form_creates_form_class_with_attributes(self):
+ user = factory.make_admin()
+ form_class = get_transition_form(user)
+
+ self.assertEqual(user, form_class.user)
+
+ def test_get_transition_form_creates_form_instance(self):
+ user = factory.make_admin()
+ node = factory.make_node(status=NODE_STATUS.DECLARED)
+ form = get_transition_form(user)(node)
+
+ self.assertIsInstance(form, NodeTransitionForm)
+ self.assertEqual(node, form.node)
+
+ def test_get_transition_form_for_admin(self):
+ admin = factory.make_admin()
+ node = factory.make_node(status=NODE_STATUS.DECLARED)
+ form = get_transition_form(admin)(node)
+
+ self.assertItemsEqual(
+ {"Enlist node": ('accept_enlistment', 'admin')},
+ form.transition_dict)
+
+ def test_get_transition_form_for_user(self):
+ user = factory.make_user()
+ node = factory.make_node(status=NODE_STATUS.DECLARED)
+ form = get_transition_form(user)(node)
+
+ self.assertIsInstance(form, NodeTransitionForm)
+ self.assertEqual(node, form.node)
+ self.assertItemsEqual({}, form.transition_dict)
+
+ def test_get_transition_form_node_for_admin_save(self):
+ admin = factory.make_admin()
+ node = factory.make_node(status=NODE_STATUS.DECLARED)
+ form = get_transition_form(admin)(
+ node, {NodeTransitionForm.input_name: "Enlist node"})
+ form.save()
+
+ self.assertEqual(NODE_STATUS.READY, node.status)
+
+ def test_get_transition_form_for_user_save(self):
+ user = factory.make_user()
+ node = factory.make_node(status=NODE_STATUS.DECLARED)
+ form = get_transition_form(user)(
+ node, {NodeTransitionForm.input_name: "Enlist node"})
+
+ self.assertRaises(PermissionDenied, form.save)
+
+ def test_get_transition_form_for_user_save_unknown_trans(self):
+ user = factory.make_user()
+ node = factory.make_node(status=NODE_STATUS.DECLARED)
+ form = get_transition_form(user)(
+ node, {NodeTransitionForm.input_name: factory.getRandomString()})
+
+ self.assertRaises(PermissionDenied, form.save)
+
+
class TestHostnameFormField(TestCase):
def test_validate_hostname_validates_valid_hostnames(self):
=== modified file 'src/maasserver/tests/test_models.py'
--- src/maasserver/tests/test_models.py 2012-04-02 03:03:47 +0000
+++ src/maasserver/tests/test_models.py 2012-04-02 10:08:18 +0000
@@ -44,6 +44,7 @@
NODE_STATUS,
NODE_STATUS_CHOICES,
NODE_STATUS_CHOICES_DICT,
+ NODE_TRANSITIONS,
SSHKey,
SYSTEM_USERS,
UserProfile,
@@ -303,6 +304,23 @@
pass
+class NodeTransitionsTests(TestCase):
+ """Test the structure of NODE_TRANSITIONS."""
+
+ def test_NODE_TRANSITIONS_initial_states(self):
+ allowed_states = set(NODE_STATUS_CHOICES_DICT.keys() + [None])
+
+ self.assertTrue(set(NODE_TRANSITIONS.keys()) <= allowed_states)
+
+ def test_NODE_TRANSITIONS_destination_state(self):
+ all_destination_states = []
+ for destination_states in NODE_TRANSITIONS.values():
+ all_destination_states.extend(destination_states)
+ allowed_states = set(NODE_STATUS_CHOICES_DICT.keys())
+
+ self.assertTrue(set(all_destination_states) <= allowed_states)
+
+
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-02 10:08:18 +0000
@@ -27,9 +27,11 @@
views,
)
from maasserver.exceptions import NoRabbit
+from maasserver.forms import NodeTransitionForm
from maasserver.models import (
Config,
NODE_AFTER_COMMISSIONING_ACTION,
+ NODE_STATUS,
POWER_TYPE_CHOICES,
SSHKey,
UserProfile,
@@ -528,6 +530,44 @@
self.assertEqual(
after_commissioning_action, node.after_commissioning_action)
+ def test_view_node_admin_has_button_to_accept_enlistement(self):
+ self.logged_in_user.is_superuser = True
+ self.logged_in_user.save()
+ node = factory.make_node(status=NODE_STATUS.DECLARED)
+ node_link = reverse('node-view', args=[node.id])
+ response = self.client.get(node_link)
+ doc = fromstring(response.content)
+ inputs = [
+ input for input in doc.cssselect('form#node_actions input')
+ if input.name == NodeTransitionForm.input_name]
+
+ self.assertSequenceEqual(
+ ['Enlist node'], [input.value for input in inputs])
+
+ def test_view_node_POST_admin_can_enlist_node(self):
+ self.logged_in_user.is_superuser = True
+ self.logged_in_user.save()
+ node = factory.make_node(status=NODE_STATUS.DECLARED)
+ node_link = reverse('node-view', args=[node.id])
+ response = self.client.post(
+ node_link,
+ data={
+ NodeTransitionForm.input_name: 'Enlist node',
+ })
+
+ self.assertEqual(httplib.FOUND, response.status_code)
+ self.assertEqual(
+ NODE_STATUS.READY, reload_object(node).status)
+
+ def test_view_node_has_button_to_accept_enlistement_for_user(self):
+ # A simple user can't see the button to enlist a declared node.
+ node = factory.make_node(status=NODE_STATUS.DECLARED)
+ node_link = reverse('node-view', args=[node.id])
+ response = self.client.get(node_link)
+ doc = fromstring(response.content)
+
+ self.assertEqual(0, len(doc.cssselect('form#node_actions input')))
+
class AdminNodeViewsTest(AdminLoggedInTestCase):
=== modified file 'src/maasserver/views.py'
--- src/maasserver/views.py 2012-03-28 16:13:04 +0000
+++ src/maasserver/views.py 2012-04-02 10:08:18 +0000
@@ -72,6 +72,7 @@
AddArchiveForm,
CommissioningForm,
EditUserForm,
+ get_transition_form,
MAASAndNetworkForm,
NewUserCreationForm,
ProfileForm,
@@ -101,7 +102,7 @@
return dj_logout(request, next_page=reverse('login'))
-class NodeView(DetailView):
+class NodeView(UpdateView):
template_name = 'maasserver/node_view.html'
@@ -111,12 +112,18 @@
id = self.kwargs.get('id', None)
return get_object_or_404(Node, id=id)
+ def get_form_class(self):
+ return get_transition_form(self.request.user)
+
def get_context_data(self, **kwargs):
context = super(NodeView, self).get_context_data(**kwargs)
node = self.get_object()
context['can_edit'] = self.request.user.has_perm('edit', node)
return context
+ def get_success_url(self):
+ return reverse('node-view', args=[self.get_object().id])
+
class NodeEdit(UpdateView):
