launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #07284
[Merge] lp:~jtv/maas/bug-986185-conditional-node-actions into lp:maas
Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/bug-986185-conditional-node-actions into lp:maas with lp:~jtv/maas/pre-986185-exception-responses as a prerequisite.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #986185 in MAAS: "MAAS boots inaccessible nodes when there's no SSH registered for a given user"
https://bugs.launchpad.net/maas/+bug/986185
For more details, see:
https://code.launchpad.net/~jtv/maas/bug-986185-conditional-node-actions/+merge/103540
Actually I split off two preparatory branches, which are independent of one another but both needed for this one. And an MP will only take one prerequisite branch, so review may have to wait until both the pre-986185-* branches are reviewed and landed.
The Delete Node button is special in many ways that set it apart from generic, easily defined “node actions”:
- It can be visible-but-disabled if the user has the requisite permissions, but circumstances prevent the action.
- It's actually a link, not a button, even though it's dressed up to look like a button.
- It requires special treatment in the view in order to control its visibility and availability like a node action's.
- It forwards to a confirmation page on a different view.
Nevertheless, this branch turns it into a regular node action. To that end I had to shave a few yaks:
1. Inhibiting node actions.
Every node action can now provide an “inhibit” function. This function looks for a reason to disable the action, but leave it visible. It returns None if there is no reason to disable it, or an explanatory text.
As it stands, all actions' inhibitions are calculated on every use of the view. This is actually a bit wasteful: on an action POST the only action that needs to be checked for inhibitions is the one that's being exercised. (The action does a double-check, so that for instance you don't accidentally delete a node that's been acquired since you loaded its overview page.) There shouldn't normally be too many actions though.
2. Compiling actions.
The node-action form had a list of references to NODE_ACTION entries, plus a more compact version of the same information. I unify them into “compiled” actions, which are like the applicable actions but any inhibitions are stored as an extra entry.
3. Disabled buttons.
A disabled action button is still a button, but with some different styling, plus a tooltip showing what's inhibiting the action, such as “you haven't registered an SSH key yet.”
4. Redirection.
The delete-node action returns a redirect to the confirmation page. This is how an action POST can support the same workflow as the first half of the old GET/POST combination. It's probably a bit less inefficient, but if that becomes a problem, we should probably be looking at mass-deletion UIs instead of shaving milliseconds off the individual operation.
In one of the prep branches I introduced a new exception type, Redirect, which conveys a temporary-redirect response to the exception middleware or, in this case, a wrapper in the view that catches MAASAPIExceptions and returns their HTTP responses. (This was already documented; the prep branch puts the logic for creating the response in the exception itself so that it's easily reusable.)
My final branch in this series will apply the new infrastructure to the Start Node button, so that you won't be able to acquire and start a node unless you have an SSH key registered to be installed on the node.
Jeroen
--
https://code.launchpad.net/~jtv/maas/bug-986185-conditional-node-actions/+merge/103540
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/maas/bug-986185-conditional-node-actions into lp:maas.
=== modified file 'src/maasserver/forms.py'
--- src/maasserver/forms.py 2012-04-25 07:07:42 +0000
+++ src/maasserver/forms.py 2012-04-25 18:33:54 +0000
@@ -37,6 +37,7 @@
PermissionDenied,
ValidationError,
)
+from django.core.urlresolvers import reverse
from django.core.validators import URLValidator
from django.forms import (
CharField,
@@ -51,6 +52,7 @@
NODE_PERMISSION,
NODE_STATUS,
)
+from maasserver.exceptions import Redirect
from maasserver.fields import MACAddressFormField
from maasserver.models import (
Config,
@@ -251,6 +253,23 @@
node.start_commissioning(user)
+def inhibit_delete(node, user, request=None):
+ """Give me one reason why `user` can't delete `node`."""
+ if node.owner is not None:
+ return "You cannot delete this node because it's in use."
+ else:
+ return None
+
+
+def delete_node(node, user, request=None):
+ """Delete-a-node action.
+
+ Permissions and inhibitions have already been checked at this point. We
+ redirect to the deletion view, which asks the user if they're sure.
+ """
+ raise Redirect(reverse('node-delete', args=[node.system_id]))
+
+
# Node actions per status.
#
# This maps each NODE_STATUS to a list of actions applicable to a node
@@ -281,8 +300,18 @@
# node, favourite_colour(user)),
# }
#
+DELETE_ACTION = {
+ 'display': "Delete node",
+ 'permission': NODE_PERMISSION.ADMIN,
+ 'execute': delete_node,
+ 'message': "Node deleted.",
+ 'inhibit': inhibit_delete,
+}
+
+
NODE_ACTIONS = {
NODE_STATUS.DECLARED: [
+ DELETE_ACTION,
{
'display': "Accept & commission",
'permission': NODE_PERMISSION.ADMIN,
@@ -291,6 +320,7 @@
},
],
NODE_STATUS.FAILED_TESTS: [
+ DELETE_ACTION,
{
'display': "Retry commissioning",
'permission': NODE_PERMISSION.ADMIN,
@@ -298,7 +328,14 @@
'message': "Started a new attempt to commission this node.",
},
],
+ NODE_STATUS.COMMISSIONING: [
+ DELETE_ACTION,
+ ],
+ NODE_STATUS.MISSING: [
+ DELETE_ACTION,
+ ],
NODE_STATUS.READY: [
+ DELETE_ACTION,
{
'display': "Start node",
'permission': NODE_PERMISSION.VIEW,
@@ -308,7 +345,11 @@
"It has been asked to start up."),
},
],
+ NODE_STATUS.RESERVED: [
+ DELETE_ACTION,
+ ],
NODE_STATUS.ALLOCATED: [
+ DELETE_ACTION,
{
'display': "Start node",
'permission': NODE_PERMISSION.EDIT,
@@ -316,6 +357,9 @@
'message': "The node has been asked to start up.",
},
],
+ NODE_STATUS.RETIRED: [
+ DELETE_ACTION,
+ ],
}
@@ -335,30 +379,62 @@
def __init__(self, instance, *args, **kwargs):
super(NodeActionForm, self).__init__(*args, **kwargs)
self.node = instance
- self.action_buttons = self.available_action_methods(
- self.node, self.user)
- # Create a convenient dict to fetch the action's name and
- # the permission to be checked from the button name.
- self.action_dict = {
- action['display']: (
- action['permission'], action['execute'], action['message'])
- for action in self.action_buttons
- }
-
- def available_action_methods(self, node, user):
+ self.action_buttons = self.compile_actions()
+
+ def have_permission(self, action=None):
+ """Does the current user have the permission required by `action`?
+
+ The permission is relative to `self.node`.
+
+ :param action: Action to check. If no action is given, the answer is
+ always False.
+ :return: Does the user have the required permission to perform
+ `action` on `self.node`?
+ """
+ if action is None:
+ return False
+ else:
+ return self.user.has_perm(action['permission'], self.node)
+
+ def compile_action(self, action):
+ """Return a copy of `action`, but add "inhibition" entry.
+
+ The key "inhibition" maps to a unicode string explaining why the
+ action is not currently available for `self.node`, or None if the
+ action can be executed.
+ """
+ compiled_action = action.copy()
+ inhibit = action.get('inhibit', lambda node, user, request: None)
+ compiled_action['inhibition'] = inhibit(
+ node=self.node, user=self.user, request=self.request)
+ return compiled_action
+
+ def compile_actions(self):
"""Return the actions that this user is allowed to perform on a node.
+ Each action will be "filled out," i.e. it will have an "inhibit"
+ callable even if the original action in NODE_ACTIONS did not.
+
:param node: The node for which the check should be performed.
:type node: :class:`maasserver.models.Node`
:param user: The user who would be performing the action. Only the
actions available to this user will be returned.
:type user: :class:`django.contrib.auth.models.User`
- :return: Any applicable action dicts, as found in NODE_ACTIONS.
+ :return: Any applicable action dicts, as compiled based on
+ NODE_ACTIONS and the current user and node.
:rtype: Sequence
"""
return [
- action for action in NODE_ACTIONS.get(node.status, ())
- if user.has_perm(action['permission'], node)]
+ self.compile_action(action)
+ for action in NODE_ACTIONS.get(self.node.status, [])
+ if self.have_permission(action)]
+
+ def find_action(self, action_name):
+ """Find the compiled action of the given name, or return None."""
+ for action in self.action_buttons:
+ if action['display'] == action_name:
+ return action
+ return None
def display_message(self, message):
"""Show `message` as feedback after performing an action."""
@@ -368,14 +444,17 @@
def save(self):
"""An action was requested. Perform it."""
action_name = self.data.get(self.input_name)
- permission, execute, message = (
- self.action_dict.get(action_name, (None, None, None)))
- if execute is None:
- raise PermissionDenied()
- if not self.user.has_perm(permission, self.node):
- raise PermissionDenied()
- execute(node=self.node, user=self.user, request=self.request)
- self.display_message(message)
+ action = self.find_action(action_name)
+
+ if not self.have_permission(action):
+ raise PermissionDenied("Not a permitted action: %s" % action_name)
+
+ if action['inhibition'] is not None:
+ raise PermissionDenied(action['inhibition'])
+
+ action['execute'](
+ node=self.node, user=self.user, request=self.request)
+ self.display_message(action.get('message'))
def get_action_form(user, request=None):
=== modified file 'src/maasserver/templates/maasserver/node_view.html'
--- src/maasserver/templates/maasserver/node_view.html 2012-04-16 16:22:32 +0000
+++ src/maasserver/templates/maasserver/node_view.html 2012-04-25 18:33:54 +0000
@@ -12,30 +12,20 @@
Edit node
</a>
{% endif %}
- {% if form.action_buttons or can_delete %}
+ {% if form.action_buttons %}
<h4>Actions</h4>
- {% endif %}
- {% if can_delete %}
- {% if node.owner %}
- <a href="#"
- title="You cannot delete this node because it's in use."
- class="disabled button">
- Delete node
- </a>
- {% else %}
- <a href="{% url 'node-delete' node.system_id %}"
- class="button secondary">
- Delete node
- </a>
- {% endif %}
- {% endif %}
- {% if form.action_buttons %}
<form id="node_actions" method="post" action=".">
{% for action in form.action_buttons %}
- <input class="secondary {% if not forloop.first or can_delete %}space-top{% endif %}"
- type="submit"
- name="{{ form.input_name }}"
- value="{{ action.display }}" />
+ <input
+ class="secondary {% if action.inhibition %}disabled{% endif %} {% if not forloop.first %}space-top{% endif %}"
+ type="submit"
+ name="{{ form.input_name }}"
+ value="{{ action.display }}"
+ {% if action.inhibition %}}
+ title="{{ action.inhibition }}"
+ disabled="disabled"
+ {% endif %}
+ />
{% endfor %}
</form>
{% endif %}
=== modified file 'src/maasserver/tests/test_forms.py'
--- src/maasserver/tests/test_forms.py 2012-04-25 02:23:38 +0000
+++ src/maasserver/tests/test_forms.py 2012-04-25 18:33:54 +0000
@@ -12,11 +12,15 @@
__metaclass__ = type
__all__ = []
+import httplib
+from urlparse import urlparse
+
from django import forms
from django.core.exceptions import (
PermissionDenied,
ValidationError,
)
+from django.core.urlresolvers import reverse
from django.http import QueryDict
import maasserver.api
from maasserver.enum import (
@@ -26,11 +30,14 @@
NODE_STATUS,
NODE_STATUS_CHOICES_DICT,
)
+from maasserver.exceptions import MAASAPIException
from maasserver.forms import (
ConfigForm,
+ delete_node,
EditUserForm,
get_action_form,
HostnameFormField,
+ inhibit_delete,
NewUserCreationForm,
NODE_ACTIONS,
NodeActionForm,
@@ -48,10 +55,7 @@
from maasserver.testing.factory import factory
from maasserver.testing.testcase import TestCase
from provisioningserver.enum import POWER_TYPE_CHOICES
-from testtools.matchers import (
- AllMatch,
- Equals,
- )
+from testtools.testcase import ExpectedException
class NodeWithMACAddressesFormTest(TestCase):
@@ -248,41 +252,119 @@
def test_NODE_ACTIONS_initial_states(self):
allowed_states = set(NODE_STATUS_CHOICES_DICT.keys() + [None])
-
self.assertTrue(set(NODE_ACTIONS.keys()) <= allowed_states)
- def test_NODE_ACTIONS_dict(self):
+ def test_NODE_ACTIONS_dict_contains_only_accepted_keys(self):
actions = sum(NODE_ACTIONS.values(), [])
- keys = ['permission', 'display', 'execute', 'message']
- self.assertThat(
- [sorted(action.keys()) for action in actions],
- AllMatch(Equals(sorted(keys))))
+ accepted_keys = set([
+ 'permission',
+ 'inhibit',
+ 'display',
+ 'execute',
+ 'message',
+ ])
+ actual_keys = set(sum([action.keys() for action in actions], []))
+ unknown_keys = actual_keys - accepted_keys
+ self.assertEqual(set(), unknown_keys)
class TestNodeActionForm(TestCase):
- def test_available_action_methods_for_declared_node_admin(self):
+ def test_inhibit_delete_allows_deletion_of_unowned_node(self):
+ admin = factory.make_admin()
+ node = factory.make_node(status=NODE_STATUS.DECLARED)
+ self.assertIsNone(inhibit_delete(node, admin))
+
+ def test_inhibit_delete_inhibits_deletion_of_owned_node(self):
+ admin = factory.make_admin()
+ node = factory.make_node(
+ status=NODE_STATUS.ALLOCATED, owner=factory.make_user())
+ self.assertEqual(
+ "You cannot delete this node because it's in use.",
+ inhibit_delete(node, admin))
+
+ def test_delete_node_redirects_to_confirmation_page(self):
+ admin = factory.make_admin()
+ node = factory.make_node(status=NODE_STATUS.DECLARED)
+ try:
+ delete_node(node, admin)
+ except MAASAPIException as e:
+ pass
+ else:
+ self.fail("delete_node should have raised a Redirect.")
+ response = e.make_http_response()
+ self.assertEqual(httplib.FOUND, response.status_code)
+ self.assertEqual(
+ reverse('node-delete', args=[node.system_id]),
+ urlparse(response['Location']).path)
+
+ def have_permission_returns_False_if_action_is_None(self):
+ admin = factory.make_admin()
+ node = factory.make_node(status=NODE_STATUS.DECLARED)
+ self.assertFalse(get_action_form(admin)(node).have_permission(None))
+
+ def have_permission_returns_False_if_lacking_permission(self):
+ user = factory.make_user()
+ node = factory.make_node(
+ status=NODE_STATUS.ALLOCATED, owner=factory.make_user())
+ action = {
+ 'permission': NODE_PERMISSION.EDIT,
+ }
+ self.assertFalse(get_action_form(user)(node).have_permission(action))
+
+ def have_permission_returns_True_given_permission(self):
+ node = factory.make_node(
+ status=NODE_STATUS.ALLOCATED, owner=factory.make_user())
+ action = {
+ 'permission': NODE_PERMISSION.EDIT,
+ }
+ form = get_action_form(node.owner)(node)
+ self.assertTrue(form.have_permission(action))
+
+ def test_compile_action_records_inhibition(self):
+ node = factory.make_node()
+ user = factory.make_user()
+ form = get_action_form(user)(node)
+ inhibition = factory.getRandomString()
+
+ def inhibit(*args, **kwargs):
+ return inhibition
+
+ compiled_action = form.compile_action({'inhibit': inhibit})
+ self.assertEqual(inhibition, compiled_action['inhibition'])
+
+ def test_compile_action_records_None_if_no_inhibition(self):
+ node = factory.make_node()
+ user = factory.make_user()
+ form = get_action_form(user)(node)
+
+ def inhibit(*args, **kwargs):
+ return None
+
+ compiled_action = form.compile_action({'inhibit': inhibit})
+ self.assertIsNone(compiled_action['inhibition'])
+
+ def test_compile_actions_for_declared_node_admin(self):
# Check which transitions are available for an admin on a
# 'Declared' node.
admin = factory.make_admin()
node = factory.make_node(status=NODE_STATUS.DECLARED)
form = get_action_form(admin)(node)
- actions = form.available_action_methods(node, admin)
- self.assertEqual(
- ["Accept & commission"],
+ actions = form.compile_actions()
+ self.assertItemsEqual(
+ ["Accept & commission", "Delete node"],
[action['display'] for action in actions])
# All permissions should be ADMIN.
self.assertEqual(
[NODE_PERMISSION.ADMIN] * len(actions),
[action['permission'] for actions in actions])
- def test_available_action_methods_for_declared_node_simple_user(self):
+ def test_compile_actions_for_declared_node_simple_user(self):
# A simple user sees no actions for a 'Declared' node.
user = factory.make_user()
node = factory.make_node(status=NODE_STATUS.DECLARED)
form = get_action_form(user)(node)
- self.assertItemsEqual(
- [], form.available_action_methods(node, user))
+ self.assertItemsEqual([], form.compile_actions())
def test_get_action_form_creates_form_class_with_attributes(self):
user = factory.make_admin()
@@ -304,8 +386,8 @@
form = get_action_form(admin)(node)
self.assertItemsEqual(
- ["Accept & commission"],
- form.action_dict)
+ ["Accept & commission", "Delete node"],
+ [action['display'] for action in form.action_buttons])
def test_get_action_form_for_user(self):
user = factory.make_user()
@@ -314,7 +396,7 @@
self.assertIsInstance(form, NodeActionForm)
self.assertEqual(node, form.node)
- self.assertItemsEqual({}, form.action_dict)
+ self.assertItemsEqual({}, form.action_buttons)
def test_get_action_form_node_for_admin_save(self):
admin = factory.make_admin()
@@ -341,6 +423,15 @@
self.assertRaises(PermissionDenied, form.save)
+ def test_save_double_checks_for_inhibitions(self):
+ admin = factory.make_admin()
+ node = factory.make_node(
+ status=NODE_STATUS.ALLOCATED, owner=factory.make_user())
+ form = get_action_form(admin)(
+ node, {NodeActionForm.input_name: "Delete node"})
+ with ExpectedException(PermissionDenied, "You cannot delete.*"):
+ form.save()
+
def test_start_action_acquires_and_starts_ready_node_for_user(self):
node = factory.make_node(status=NODE_STATUS.READY)
user = factory.make_user()
=== modified file 'src/maasserver/tests/test_views_nodes.py'
--- src/maasserver/tests/test_views_nodes.py 2012-04-25 13:03:13 +0000
+++ src/maasserver/tests/test_views_nodes.py 2012-04-25 18:33:54 +0000
@@ -129,14 +129,6 @@
else:
self.assertNotIn(help_link, links)
- def test_view_node_shows_link_to_delete_node_for_admin(self):
- self.become_admin()
- node = factory.make_node()
- node_link = reverse('node-view', args=[node.system_id])
- response = self.client.get(node_link)
- node_delete_link = reverse('node-delete', args=[node.system_id])
- self.assertIn(node_delete_link, get_content_links(response))
-
def test_admin_can_delete_nodes(self):
self.become_admin()
node = factory.make_node()
@@ -156,7 +148,7 @@
self.assertEqual(httplib.OK, response.status_code)
self.assertNotIn(node_delete_link, get_content_links(response))
self.assertIn(
- "You cannot delete this node because it's in use.",
+ "You cannot delete this node because",
response.content)
def test_allocated_node_cannot_be_deleted(self):
@@ -256,6 +248,26 @@
content_text = doc.cssselect('#content')[0].text_content()
self.assertNotIn("Error output", content_text)
+ def test_view_node_POST_admin_can_delete_unused_node(self):
+ self.become_admin()
+ node = factory.make_node(status=NODE_STATUS.READY)
+ response = self.client.post(
+ reverse('node-view', args=[node.system_id]),
+ data={NodeActionForm.input_name: "Delete node"})
+ self.assertEqual(httplib.FOUND, response.status_code)
+ self.assertEqual(
+ reverse('node-delete', args=[node.system_id]),
+ urlparse(response['Location']).path)
+
+ def test_view_node_POST_admin_cannot_delete_used_node(self):
+ self.become_admin()
+ node = factory.make_node(
+ status=NODE_STATUS.ALLOCATED, owner=factory.make_user())
+ response = self.client.post(
+ reverse('node-view', args=[node.system_id]),
+ data={NodeActionForm.input_name: "Delete node"})
+ self.assertEqual(httplib.FORBIDDEN, response.status_code)
+
def test_view_node_POST_admin_can_start_commissioning_node(self):
self.become_admin()
node = factory.make_node(status=NODE_STATUS.DECLARED)
=== modified file 'src/maasserver/views/nodes.py'
--- src/maasserver/views/nodes.py 2012-04-25 10:31:44 +0000
+++ src/maasserver/views/nodes.py 2012-04-25 18:33:54 +0000
@@ -22,7 +22,6 @@
from django.contrib import messages
from django.core.exceptions import PermissionDenied
from django.core.urlresolvers import reverse
-from django.http import HttpResponse
from django.utils.safestring import mark_safe
from django.views.generic import (
ListView,
@@ -108,8 +107,6 @@
node = self.get_object()
context['can_edit'] = self.request.user.has_perm(
NODE_PERMISSION.EDIT, node)
- context['can_delete'] = self.request.user.has_perm(
- NODE_PERMISSION.ADMIN, node)
if node.status in (NODE_STATUS.COMMISSIONING, NODE_STATUS.READY):
messages.info(self.request, NODE_BOOT_INFO)
context['error_text'] = (
@@ -128,7 +125,7 @@
try:
return super(NodeView, self).dispatch(*args, **kwargs)
except MAASAPIException as e:
- return HttpResponse(unicode(e), status=e.api_error)
+ return e.make_http_response()
def get_success_url(self):
return reverse('node-view', args=[self.get_object().system_id])
