launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #06976
[Merge] lp:~rvb/maas/maas-unify-security into lp:maas
Raphaël Badin has proposed merging lp:~rvb/maas/maas-unify-security into lp:maas.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~rvb/maas/maas-unify-security/+merge/100561
This branch:
- introduces a new enum: NODE_PERMISSIONS that will be used instead of passing strings around for permissions checks.
- changes the permission required to delete a node to NODE_PERMISSION.ADMIN (it was edit before).
- changes get_node_or_404 so that no default param is passed as the 'perm' to check. I think this is more safe to force the caller to specify the permission to be checked.
--
https://code.launchpad.net/~rvb/maas/maas-unify-security/+merge/100561
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rvb/maas/maas-unify-security into lp:maas.
=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py 2012-04-02 14:40:17 +0000
+++ src/maasserver/api.py 2012-04-03 07:13:21 +0000
@@ -103,6 +103,7 @@
FileStorage,
MACAddress,
Node,
+ NODE_PERMISSIONS,
NODE_STATUS,
)
from piston.doc import generate_doc
@@ -320,12 +321,12 @@
def read(self, request, system_id):
"""Read a specific Node."""
return Node.objects.get_node_or_404(
- system_id=system_id, user=request.user, perm='access')
+ system_id=system_id, user=request.user, perm=NODE_PERMISSIONS.VIEW)
def update(self, request, system_id):
"""Update a specific Node."""
node = Node.objects.get_node_or_404(
- system_id=system_id, user=request.user, perm='edit')
+ system_id=system_id, user=request.user, perm=NODE_PERMISSIONS.EDIT)
for key, value in request.data.items():
setattr(node, key, value)
node.save()
@@ -334,7 +335,8 @@
def delete(self, request, system_id):
"""Delete a specific Node."""
node = Node.objects.get_node_or_404(
- system_id=system_id, user=request.user, perm='edit')
+ system_id=system_id, user=request.user,
+ perm=NODE_PERMISSIONS.ADMIN)
node.delete()
return rc.DELETED
@@ -385,7 +387,7 @@
def release(self, request, system_id):
"""Release a node. Opposite of `NodesHandler.acquire`."""
node = Node.objects.get_node_or_404(
- system_id=system_id, user=request.user, perm='edit')
+ system_id=system_id, user=request.user, perm=NODE_PERMISSIONS.EDIT)
if node.status == NODE_STATUS.READY:
# Nothing to do. This may be a redundant retry, and the
# postcondition is achieved, so call this success.
@@ -566,14 +568,14 @@
def read(self, request, system_id):
"""Read all MAC Addresses related to a Node."""
node = Node.objects.get_node_or_404(
- user=request.user, system_id=system_id, perm='access')
+ user=request.user, system_id=system_id, perm=NODE_PERMISSIONS.VIEW)
return MACAddress.objects.filter(node=node).order_by('id')
def create(self, request, system_id):
"""Create a MAC Address for a specified Node."""
node = Node.objects.get_node_or_404(
- user=request.user, system_id=system_id, perm='edit')
+ user=request.user, system_id=system_id, perm=NODE_PERMISSIONS.EDIT)
mac = node.add_mac_address(request.data.get('mac_address', None))
return mac
@@ -591,7 +593,7 @@
def read(self, request, system_id, mac_address):
"""Read a MAC Address related to a Node."""
node = Node.objects.get_node_or_404(
- user=request.user, system_id=system_id, perm='access')
+ user=request.user, system_id=system_id, perm=NODE_PERMISSIONS.VIEW)
validate_mac(mac_address)
return get_object_or_404(
@@ -601,7 +603,7 @@
"""Delete a specific MAC Address for the specified Node."""
validate_mac(mac_address)
node = Node.objects.get_node_or_404(
- user=request.user, system_id=system_id, perm='edit')
+ user=request.user, system_id=system_id, perm=NODE_PERMISSIONS.EDIT)
mac = get_object_or_404(MACAddress, node=node, mac_address=mac_address)
mac.delete()
=== modified file 'src/maasserver/forms.py'
--- src/maasserver/forms.py 2012-04-02 16:36:41 +0000
+++ src/maasserver/forms.py 2012-04-03 07:13:21 +0000
@@ -51,6 +51,7 @@
Node,
NODE_AFTER_COMMISSIONING_ACTION,
NODE_AFTER_COMMISSIONING_ACTION_CHOICES,
+ NODE_PERMISSIONS,
NODE_STATUS,
SSHKey,
UserProfile,
@@ -210,7 +211,7 @@
NODE_STATUS.DECLARED: [
{
'display': "Accept Enlisted node",
- 'permission': 'admin',
+ 'permission': NODE_PERMISSIONS.ADMIN,
'execute': lambda node, user: Node.accept_enlistment(node),
},
],
=== modified file 'src/maasserver/models.py'
--- src/maasserver/models.py 2012-04-03 02:31:04 +0000
+++ src/maasserver/models.py 2012-04-03 07:13:21 +0000
@@ -18,6 +18,7 @@
"Config",
"FileStorage",
"NODE_STATUS",
+ "NODE_PERMISSIONS",
"NODE_TRANSITIONS",
"Node",
"MACAddress",
@@ -344,7 +345,7 @@
visible_nodes = self.filter(owner=user)
return self.filter_by_ids(visible_nodes, ids)
- def get_node_or_404(self, system_id, user, perm='access'):
+ def get_node_or_404(self, system_id, user, perm):
"""Fetch a `Node` by system_id. Raise exceptions if no `Node` with
this system_id exist or if the provided user has not the required
permission on this `Node`.
@@ -452,6 +453,12 @@
return None
+class NODE_PERMISSIONS:
+ VIEW = 'view_node'
+ EDIT = 'edit_node'
+ ADMIN = 'admin_node'
+
+
class Node(CommonInfo):
"""A `Node` represents a physical machine used by the MAAS Server.
@@ -1202,12 +1209,12 @@
raise NotImplementedError(
'Invalid permission check (invalid object type).')
- if perm == 'access':
+ if perm == NODE_PERMISSIONS.VIEW:
return obj.owner in (None, user)
- elif perm == 'edit':
+ elif perm == NODE_PERMISSIONS.EDIT:
return obj.owner == user
- elif perm == 'admin':
- # 'admin' permission is solely granted to superusers.
+ elif perm == NODE_PERMISSIONS.ADMIN:
+ # 'admin_node' permission is solely granted to superusers.
return False
else:
raise NotImplementedError(
=== modified file 'src/maasserver/tests/test_api.py'
--- src/maasserver/tests/test_api.py 2012-04-02 16:04:34 +0000
+++ src/maasserver/tests/test_api.py 2012-04-03 07:13:21 +0000
@@ -640,6 +640,7 @@
def test_DELETE_deletes_node(self):
# The api allows to delete a Node.
+ self.become_admin()
node = factory.make_node(set_hostname=True, owner=self.logged_in_user)
system_id = node.system_id
response = self.client.delete(self.get_node_uri(node))
@@ -647,6 +648,13 @@
self.assertEqual(204, response.status_code)
self.assertItemsEqual([], Node.objects.filter(system_id=system_id))
+ def test_DELETE_deletes_node_fails_if_not_admin(self):
+ # Only superusers can delete nodes.
+ node = factory.make_node(set_hostname=True, owner=self.logged_in_user)
+ response = self.client.delete(self.get_node_uri(node))
+
+ self.assertEqual(httplib.FORBIDDEN, response.status_code)
+
def test_DELETE_forbidden_without_edit_permission(self):
# A user without the edit permission cannot delete a Node.
node = factory.make_node(set_hostname=True)
@@ -1140,7 +1148,7 @@
MACAddress.objects.filter(node=node).count())
def test_macs_POST_add_mac_without_edit_perm(self):
- # Adding a MAC Address to a node requires the 'edit' permission.
+ # Adding a MAC Address to a node requires the 'edit_node' permission.
node = factory.make_node()
response = self.client.post(
self.get_uri('nodes/%s/macs/') % node.system_id,
=== modified file 'src/maasserver/tests/test_auth.py'
--- src/maasserver/tests/test_auth.py 2012-04-02 10:00:49 +0000
+++ src/maasserver/tests/test_auth.py 2012-04-03 07:13:21 +0000
@@ -17,6 +17,7 @@
from maasserver.models import (
MAASAuthorizationBackend,
Node,
+ NODE_PERMISSIONS,
NODE_STATUS,
)
from maasserver.testing.factory import factory
@@ -79,7 +80,7 @@
mac = make_unallocated_node().add_mac_address('AA:BB:CC:DD:EE:FF')
self.assertRaises(
NotImplementedError, backend.has_perm,
- factory.make_admin(), 'access', mac)
+ factory.make_admin(), NODE_PERMISSIONS.VIEW, mac)
def test_invalid_check_permission(self):
backend = MAASAuthorizationBackend()
@@ -90,45 +91,53 @@
def test_node_init_user_cannot_access(self):
backend = MAASAuthorizationBackend()
self.assertFalse(backend.has_perm(
- get_node_init_user(), 'access', make_unallocated_node()))
+ get_node_init_user(), NODE_PERMISSIONS.VIEW,
+ make_unallocated_node()))
def test_user_can_access_unowned_node(self):
backend = MAASAuthorizationBackend()
self.assertTrue(backend.has_perm(
- factory.make_user(), 'access', make_unallocated_node()))
+ factory.make_user(), NODE_PERMISSIONS.VIEW,
+ make_unallocated_node()))
def test_user_cannot_access_nodes_owned_by_others(self):
backend = MAASAuthorizationBackend()
self.assertFalse(backend.has_perm(
- factory.make_user(), 'access', make_allocated_node()))
+ factory.make_user(), NODE_PERMISSIONS.VIEW, make_allocated_node()))
def test_owned_status(self):
# A non-admin user can access nodes he owns.
backend = MAASAuthorizationBackend()
node = make_allocated_node()
- self.assertTrue(backend.has_perm(node.owner, 'access', node))
+ self.assertTrue(
+ backend.has_perm(
+ node.owner, NODE_PERMISSIONS.VIEW, node))
def test_user_cannot_edit_nodes_owned_by_others(self):
backend = MAASAuthorizationBackend()
self.assertFalse(backend.has_perm(
- factory.make_user(), 'edit', make_allocated_node()))
+ factory.make_user(), NODE_PERMISSIONS.EDIT, make_allocated_node()))
def test_user_cannot_edit_unowned_node(self):
backend = MAASAuthorizationBackend()
self.assertFalse(backend.has_perm(
- factory.make_user(), 'edit', make_unallocated_node()))
+ factory.make_user(), NODE_PERMISSIONS.EDIT,
+ make_unallocated_node()))
def test_user_can_edit_his_own_nodes(self):
backend = MAASAuthorizationBackend()
user = factory.make_user()
self.assertTrue(backend.has_perm(
- user, 'edit', make_allocated_node(owner=user)))
+ user, NODE_PERMISSIONS.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.
+ # NODE_PERMISSIONS.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()))
+ self.assertFalse(
+ backend.has_perm(
+ user, NODE_PERMISSIONS.ADMIN, factory.make_node()))
class TestNodeVisibility(TestCase):
=== modified file 'src/maasserver/tests/test_forms.py'
--- src/maasserver/tests/test_forms.py 2012-04-02 16:36:41 +0000
+++ src/maasserver/tests/test_forms.py 2012-04-03 07:13:21 +0000
@@ -38,6 +38,7 @@
Config,
DEFAULT_CONFIG,
NODE_AFTER_COMMISSIONING_ACTION_CHOICES,
+ NODE_PERMISSIONS,
NODE_STATUS,
NODE_STATUS_CHOICES_DICT,
POWER_TYPE_CHOICES,
@@ -294,7 +295,7 @@
["Accept Enlisted node"],
[transition['display'] for transition in transitions])
self.assertEqual(
- ['admin'],
+ [NODE_PERMISSIONS.ADMIN],
[transition['permission'] for transition in transitions])
def test_available_transition_methods_for_declared_node_simple_user(self):
@@ -325,7 +326,8 @@
form = get_transition_form(admin)(node)
self.assertItemsEqual(
- {"Accept Enlisted node": ('accept_enlistment', 'admin')},
+ {"Accept Enlisted node": (
+ 'accept_enlistment', NODE_PERMISSIONS.ADMIN)},
form.transition_dict)
def test_get_transition_form_for_user(self):
=== modified file 'src/maasserver/tests/test_models.py'
--- src/maasserver/tests/test_models.py 2012-04-03 02:31:04 +0000
+++ src/maasserver/tests/test_models.py 2012-04-03 07:13:21 +0000
@@ -43,6 +43,7 @@
HELLIPSIS,
MACAddress,
Node,
+ NODE_PERMISSIONS,
NODE_STATUS,
NODE_STATUS_CHOICES,
NODE_STATUS_CHOICES_DICT,
@@ -432,7 +433,9 @@
user = factory.make_user()
node = self.make_node(user)
self.assertEqual(
- node, Node.objects.get_node_or_404(node.system_id, user))
+ node,
+ Node.objects.get_node_or_404(
+ node.system_id, user, NODE_PERMISSIONS.VIEW))
def test_get_visible_node_or_404_raises_PermissionDenied(self):
"""get_node_or_404 raises PermissionDenied if the provided
@@ -441,7 +444,7 @@
self.assertRaises(
PermissionDenied,
Node.objects.get_node_or_404,
- user_node.system_id, factory.make_user(), 'access')
+ user_node.system_id, factory.make_user(), NODE_PERMISSIONS.VIEW)
def test_get_available_node_for_acquisition_finds_available_node(self):
user = factory.make_user()
=== modified file 'src/maasserver/views.py'
--- src/maasserver/views.py 2012-04-02 16:38:56 +0000
+++ src/maasserver/views.py 2012-04-03 07:13:21 +0000
@@ -83,6 +83,7 @@
from maasserver.messages import messaging
from maasserver.models import (
Node,
+ NODE_PERMISSIONS,
SSHKey,
UserProfile,
)
@@ -110,7 +111,8 @@
def get_object(self):
system_id = self.kwargs.get('system_id', None)
node = Node.objects.get_node_or_404(
- system_id=system_id, user=self.request.user, perm='access')
+ system_id=system_id, user=self.request.user,
+ perm=NODE_PERMISSIONS.VIEW)
return node
def get_form_class(self):
@@ -119,7 +121,8 @@
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)
+ context['can_edit'] = self.request.user.has_perm(
+ NODE_PERMISSIONS.EDIT, node)
return context
def get_success_url(self):
@@ -133,7 +136,8 @@
def get_object(self):
system_id = self.kwargs.get('system_id', None)
node = Node.objects.get_node_or_404(
- system_id=system_id, user=self.request.user, perm='edit')
+ system_id=system_id, user=self.request.user,
+ perm=NODE_PERMISSIONS.EDIT)
return node
def get_form_class(self):
