launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #07030
[Merge] lp:~rvb/maas/maas-error-auth-enlist into lp:maas
Raphaël Badin has proposed merging lp:~rvb/maas/maas-error-auth-enlist into lp:maas.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #975267 in MAAS: "When an admin registers a node with invalid data, an error 500 is triggered."
https://bugs.launchpad.net/maas/+bug/975267
For more details, see:
https://code.launchpad.net/~rvb/maas/maas-error-auth-enlist/+merge/101124
This branch fixes a bug triggered when an admin user enlists a node and provides wrong data that won't validate. The main change is this branch is that api.py:create_node raises a validation error if the form is not valid instead of returning an bad request http response; this is to prevent any further processing. I refactored all the enlistment tests: we only tested anon enlistment and now most of the enlistment tests are run: for an anon user, for a normal user and for an admin user.
Drive-by fix: added missing signal cleanup.
--
https://code.launchpad.net/~rvb/maas/maas-error-auth-enlist/+merge/101124
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rvb/maas/maas-error-auth-enlist into lp:maas.
=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py 2012-04-05 14:32:23 +0000
+++ src/maasserver/api.py 2012-04-06 16:01:25 +0000
@@ -408,17 +408,15 @@
The node will be in the Declared state.
:param request: The http request for this node to be created.
- :return: A suitable return value for the handler receiving the "new"
- request that this implements.
- :rtype: :class:`maasserver.models.Node` or
- :class:`django.http.HttpResponseBadRequest`.
+ :return: A `Node`.
+ :rtype: :class:`maasserver.models.Node`.
+ :raises: ValidationError
"""
form = NodeWithMACAddressesForm(request.data)
if form.is_valid():
return form.save()
else:
- return HttpResponseBadRequest(
- form.errors, content_type='application/json')
+ raise ValidationError(form.errors)
@api_operations
=== modified file 'src/maasserver/testing/testcase.py'
--- src/maasserver/testing/testcase.py 2012-04-02 16:04:34 +0000
+++ src/maasserver/testing/testcase.py 2012-04-06 16:01:25 +0000
@@ -10,6 +10,7 @@
__metaclass__ = type
__all__ = [
+ 'AdminLoggedInTestCase',
'LoggedInTestCase',
'TestCase',
'TestModelTestCase',
@@ -43,3 +44,10 @@
"""Promote the logged-in user to admin."""
self.logged_in_user.is_superuser = True
self.logged_in_user.save()
+
+
+class AdminLoggedInTestCase(LoggedInTestCase):
+
+ def setUp(self):
+ super(AdminLoggedInTestCase, self).setUp()
+ self.become_admin()
=== modified file 'src/maasserver/tests/test_api.py'
--- src/maasserver/tests/test_api.py 2012-04-05 14:32:23 +0000
+++ src/maasserver/tests/test_api.py 2012-04-06 16:01:25 +0000
@@ -45,6 +45,7 @@
from maasserver.testing.factory import factory
from maasserver.testing.oauthclient import OAuthAuthenticatedClient
from maasserver.testing.testcase import (
+ AdminLoggedInTestCase,
LoggedInTestCase,
TestCase,
)
@@ -94,8 +95,7 @@
extract_constraints(QueryDict('name=%s' % name)))
-class AnonymousEnlistmentAPITest(APIv10TestMixin, TestCase):
- # Nodes can be enlisted anonymously.
+class EnlistmentAPITest(APIv10TestMixin):
def test_POST_new_creates_node(self):
# The API allows a Node to be created.
@@ -119,25 +119,6 @@
self.assertEqual(2, diane.after_commissioning_action)
self.assertEqual(architecture, diane.architecture)
- def test_POST_new_anonymous_creates_node_in_declared_state(self):
- # Upon anonymous enlistment, a node goes into the Declared
- # state. Deliberate approval is required before we start
- # reinstalling the system, wiping its disks etc.
- response = self.client.post(
- self.get_uri('nodes/'),
- {
- 'op': 'new',
- 'hostname': factory.getRandomString(),
- 'architecture': factory.getRandomChoice(ARCHITECTURE_CHOICES),
- 'after_commissioning_action': '2',
- 'mac_addresses': ['aa:bb:cc:dd:ee:ff'],
- })
- self.assertEqual(httplib.OK, response.status_code)
- system_id = json.loads(response.content)['system_id']
- self.assertEqual(
- NODE_STATUS.DECLARED,
- Node.objects.get(system_id=system_id).status)
-
def test_POST_new_power_type_defaults_to_asking_config(self):
architecture = factory.getRandomChoice(ARCHITECTURE_CHOICES)
response = self.client.post(
@@ -194,25 +175,6 @@
system_id=json.loads(response.content)['system_id'])
self.assertEqual('node-aabbccddeeff.local', node.hostname)
- def test_POST_returns_limited_fields(self):
- architecture = factory.getRandomChoice(ARCHITECTURE_CHOICES)
- response = self.client.post(
- self.get_uri('nodes/'),
- {
- 'op': 'new',
- 'hostname': 'diane',
- 'architecture': architecture,
- 'after_commissioning_action': '2',
- 'mac_addresses': ['aa:bb:cc:dd:ee:ff', '22:bb:cc:dd:ee:ff'],
- })
- parsed_result = json.loads(response.content)
- self.assertItemsEqual(
- [
- 'hostname', 'system_id', 'macaddress_set', 'architecture',
- 'status'
- ],
- list(parsed_result))
-
def test_POST_fails_without_operation(self):
# If there is no operation ('op=operation_name') specified in the
# request data, a 'Bad request' response is returned.
@@ -259,6 +221,7 @@
self.fail("post_save should not have been called")
post_save.connect(node_created, sender=Node)
+ self.addCleanup(post_save.disconnect, node_created, sender=Node)
self.client.post(
self.get_uri('nodes/'),
{
@@ -318,6 +281,32 @@
self.assertIn('application/json', response['Content-Type'])
self.assertItemsEqual(['architecture'], parsed_result)
+
+class NonAdminEnlistmentAPITest(EnlistmentAPITest):
+
+ def test_POST_new_anonymous_creates_node_in_declared_state(self):
+ # Upon anonymous enlistment, a node goes into the Declared
+ # state. Deliberate approval is required before we start
+ # reinstalling the system, wiping its disks etc.
+ response = self.client.post(
+ self.get_uri('nodes/'),
+ {
+ 'op': 'new',
+ 'hostname': factory.getRandomString(),
+ 'architecture': factory.getRandomChoice(ARCHITECTURE_CHOICES),
+ 'after_commissioning_action': '2',
+ 'mac_addresses': ['aa:bb:cc:dd:ee:ff'],
+ })
+ self.assertEqual(httplib.OK, response.status_code)
+ system_id = json.loads(response.content)['system_id']
+ self.assertEqual(
+ NODE_STATUS.DECLARED,
+ Node.objects.get(system_id=system_id).status)
+
+
+class AnonymousEnlistmentAPITest(NonAdminEnlistmentAPITest, TestCase):
+ # Nodes can be enlisted anonymously.
+
def test_POST_accept_not_allowed(self):
# An anonymous user is not allowed to accept an anonymously
# enlisted node. That would defeat the whole purpose of holding
@@ -329,6 +318,100 @@
(httplib.UNAUTHORIZED, "You must be logged in to accept nodes."),
(response.status_code, response.content))
+ def test_POST_returns_limited_fields(self):
+ response = self.client.post(
+ self.get_uri('nodes/'),
+ {
+ 'op': 'new',
+ 'architecture': factory.getRandomChoice(ARCHITECTURE_CHOICES),
+ 'hostname': factory.getRandomString(),
+ 'after_commissioning_action': '2',
+ 'mac_addresses': ['aa:bb:cc:dd:ee:ff', '22:bb:cc:dd:ee:ff'],
+ })
+ parsed_result = json.loads(response.content)
+ self.assertItemsEqual(
+ [
+ 'hostname', 'system_id', 'macaddress_set', 'architecture',
+ 'status',
+ ],
+ list(parsed_result))
+
+
+class SimpleUserLoggedInEnlistmentAPITest(NonAdminEnlistmentAPITest,
+ LoggedInTestCase):
+ # Nodes can be enlisted when logged-in.
+
+ def test_POST_accept_not_allowed(self):
+ # An anonymous user is not allowed to accept an anonymously
+ # enlisted node. That would defeat the whole purpose of holding
+ # those nodes for approval.
+ node_id = factory.make_node(status=NODE_STATUS.DECLARED).system_id
+ response = self.client.post(
+ self.get_uri('nodes/'), {'op': 'accept', 'nodes': [node_id]})
+ self.assertEqual(
+ (httplib.FORBIDDEN,
+ "You don't have the required permission to accept the "
+ "following node(s): %s." % node_id),
+ (response.status_code, response.content))
+
+ def test_POST_returns_limited_fields(self):
+ response = self.client.post(
+ self.get_uri('nodes/'),
+ {
+ 'op': 'new',
+ 'hostname': factory.getRandomString(),
+ 'architecture': factory.getRandomChoice(ARCHITECTURE_CHOICES),
+ 'after_commissioning_action': '2',
+ 'mac_addresses': ['aa:bb:cc:dd:ee:ff', '22:bb:cc:dd:ee:ff'],
+ })
+ parsed_result = json.loads(response.content)
+ self.assertItemsEqual(
+ [
+ 'hostname', 'system_id', 'macaddress_set', 'architecture',
+ 'status', 'resource_uri',
+ ],
+ list(parsed_result))
+
+
+class AdminLoggedInEnlistmentAPITest(EnlistmentAPITest,
+ AdminLoggedInTestCase):
+ # Nodes can be enlisted when logged-in as an admin user.
+
+ def test_POST_admin_creates_node_in_ready_state(self):
+ # When an admin user enlists a node, it goes into the Ready state.
+ # This will change once we start doing proper commissioning.
+ response = self.client.post(
+ self.get_uri('nodes/'),
+ {
+ 'op': 'new',
+ 'hostname': factory.getRandomString(),
+ 'architecture': factory.getRandomChoice(ARCHITECTURE_CHOICES),
+ 'after_commissioning_action': '2',
+ 'mac_addresses': ['aa:bb:cc:dd:ee:ff'],
+ })
+ self.assertEqual(httplib.OK, response.status_code)
+ system_id = json.loads(response.content)['system_id']
+ self.assertEqual(
+ NODE_STATUS.READY, Node.objects.get(system_id=system_id).status)
+
+ def test_POST_returns_limited_fields(self):
+ response = self.client.post(
+ self.get_uri('nodes/'),
+ {
+ 'op': 'new',
+ 'hostname': factory.getRandomString(),
+ 'architecture': factory.getRandomChoice(ARCHITECTURE_CHOICES),
+ 'after_commissioning_action': '2',
+ 'mac_addresses': ['aa:bb:cc:dd:ee:ff', '22:bb:cc:dd:ee:ff'],
+ })
+ parsed_result = json.loads(response.content)
+ self.assertItemsEqual(
+ [
+ 'hostname', 'system_id', 'macaddress_set', 'architecture',
+ 'status', 'resource_uri',
+ ],
+ list(parsed_result))
+
class AnonymousIsRegisteredAPITest(APIv10TestMixin, TestCase):
@@ -779,24 +862,6 @@
NODE_STATUS.DECLARED,
Node.objects.get(system_id=system_id).status)
- def test_POST_new_when_admin_creates_node_in_ready_state(self):
- # When an admin user enlists a node, it goes into the Ready state.
- # This will change once we start doing proper commissioning.
- self.become_admin()
- response = self.client.post(
- self.get_uri('nodes/'),
- {
- 'op': 'new',
- 'hostname': factory.getRandomString(),
- 'architecture': factory.getRandomChoice(ARCHITECTURE_CHOICES),
- 'after_commissioning_action': '2',
- 'mac_addresses': ['aa:bb:cc:dd:ee:ff'],
- })
- self.assertEqual(httplib.OK, response.status_code)
- system_id = json.loads(response.content)['system_id']
- self.assertEqual(
- NODE_STATUS.READY, Node.objects.get(system_id=system_id).status)
-
def test_GET_list_lists_nodes(self):
# The api allows for fetching the list of Nodes.
node1 = factory.make_node()
