launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #06166
[Merge] lp:~rvb/maas/maas-security-model-api2 into lp:maas
Raphaël Badin has proposed merging lp:~rvb/maas/maas-security-model-api2 into lp:maas with lp:~rvb/maas/maas-templates-fixes as a prerequisite.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~rvb/maas/maas-security-model-api2/+merge/89949
This branch is the second (and final) branch to use the security mechanisms introduced in ~rvb/maas/maas-security-model in the API.
= Notes =
I've removed the admin special case in the backend and in the tests because django already enforces the fact that an admin has all the permissions.
--
https://code.launchpad.net/~rvb/maas/maas-security-model-api2/+merge/89949
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rvb/maas/maas-security-model-api2 into lp:maas.
=== modified file 'docs/models.rst'
--- docs/models.rst 2012-01-20 15:41:55 +0000
+++ docs/models.rst 2012-01-24 17:14:25 +0000
@@ -4,4 +4,6 @@
.. automodule:: maasserver.models
.. autoclass:: Node
+.. autoclass:: NodeManager
+ :members: visible_nodes, get_visible_node_or_404
.. autoclass:: MACAddress
=== modified file 'src/maas/settings.py'
--- src/maas/settings.py 2012-01-24 15:25:49 +0000
+++ src/maas/settings.py 2012-01-24 17:14:25 +0000
@@ -33,6 +33,10 @@
YUI_VERSION = '3.4.1'
STATIC_LOCAL_SERVE = DEBUG
+AUTHENTICATION_BACKENDS = (
+ 'maasserver.models.MaaSAuthorizationBackend',
+ )
+
DATABASES = {
'default': {
# 'postgresql_psycopg2', 'postgresql', 'mysql', 'sqlite3' etc.
=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py 2012-01-24 15:25:49 +0000
+++ src/maasserver/api.py 2012-01-24 17:14:25 +0000
@@ -14,7 +14,10 @@
"NodeMacsHandler",
]
-from django.core.exceptions import ValidationError
+from django.core.exceptions import (
+ PermissionDenied,
+ ValidationError,
+ )
from django.shortcuts import (
get_object_or_404,
render_to_response,
@@ -99,7 +102,7 @@
def read(self, request):
"""Read all Nodes."""
- return Node.objects.all().order_by('id')
+ return Node.objects.visible_nodes(request.user).order_by('id')
def create(self, request):
"""Create a new Node."""
@@ -126,15 +129,23 @@
def read(self, request, system_id):
"""Read all MAC Addresses related to a Node."""
- node = get_object_or_404(Node, system_id=system_id)
+ try:
+ node = Node.objects.get_visible_node_or_404(
+ user=request.user, system_id=system_id)
+ except PermissionDenied:
+ return rc.FORBIDDEN
+
return MACAddress.objects.filter(node=node).order_by('id')
def create(self, request, system_id):
"""Create a MAC Address for a specified Node."""
- node = get_object_or_404(Node, system_id=system_id)
try:
+ node = Node.objects.get_visible_node_or_404(
+ user=request.user, system_id=system_id)
mac = node.add_mac_address(request.data.get('mac_address', None))
return mac
+ except PermissionDenied:
+ return rc.FORBIDDEN
except ValidationError, e:
return bad_request(format_error_message(e))
@@ -151,7 +162,13 @@
def read(self, request, system_id, mac_address):
"""Read a MAC Address related to a Node."""
- node = get_object_or_404(Node, system_id=system_id)
+ try:
+ node = Node.objects.get_visible_node_or_404(
+ user=request.user, system_id=system_id)
+ except PermissionDenied:
+ return rc.FORBIDDEN
+
+
valid, response = validate_mac_address(mac_address)
if not valid:
return response
@@ -164,7 +181,12 @@
if not valid:
return response
- node = get_object_or_404(Node, system_id=system_id)
+ try:
+ node = Node.objects.get_visible_node_or_404(
+ user=request.user, system_id=system_id)
+ except PermissionDenied:
+ return rc.FORBIDDEN
+
mac = get_object_or_404(MACAddress, node=node, mac_address=mac_address)
mac.delete()
return rc.DELETED
=== modified file 'src/maasserver/models.py'
--- src/maasserver/models.py 2012-01-24 15:27:22 +0000
+++ src/maasserver/models.py 2012-01-24 17:14:25 +0000
@@ -20,8 +20,11 @@
from uuid import uuid1
from django.contrib import admin
+from django.contrib.auth.backends import ModelBackend
from django.contrib.auth.models import User
+from django.core.exceptions import PermissionDenied
from django.db import models
+from django.shortcuts import get_object_or_404
from maasserver.macaddress import MACAddressField
@@ -65,14 +68,39 @@
class NodeManager(models.Manager):
+ """A utility to manage collections of Nodes."""
def visible_nodes(self, user):
+ """Fetch all the `Nodes` visible by a User. Available via
+ `Node.objects`.
+
+ :param user: The user that should be used in the permission check.
+ :type user: django.contrib.auth.models.User.
+
+ """
if user.is_superuser:
return self.all()
else:
return self.filter(
models.Q(owner__isnull=True) | models.Q(owner=user))
+ def get_visible_node_or_404(self, system_id, user):
+ """Fetch a `Node` by system_id. Raise exceptions if no `Node` with
+ this system_id exist or if the provided user cannot see this `Node`.
+
+ :param name: The system_id.
+ :type name: str.
+ :param user: The user that should be used in the permission check.
+ :type user: django.contrib.auth.models.User.
+ :raises: django.http.Http404, django.core.exceptions.PermissionDenied
+
+ """
+ node = get_object_or_404(Node, system_id=system_id)
+ if user.has_perm('access', node):
+ return node
+ else:
+ raise PermissionDenied
+
class Node(CommonInfo):
"""A `Node` represents a physical machine used by the MaaS Server."""
@@ -118,6 +146,9 @@
mac_address = MACAddressField()
node = models.ForeignKey(Node)
+ class Meta:
+ verbose_name_plural = "MAC addresses"
+
def __unicode__(self):
return self.mac_address
@@ -127,7 +158,9 @@
admin.site.register(MACAddress)
-class MaaSAuthorizationBackend(object):
+class MaaSAuthorizationBackend(ModelBackend):
+
+ supports_object_permissions = True
def has_perm(self, user, perm, obj=None):
# Only Nodes can be checked. We also don't support perm checking
@@ -141,8 +174,4 @@
raise NotImplementedError(
'Invalid permission check (invalid permission name).')
- # Admins are granted all permissions.
- if user.is_superuser:
- return True
-
return obj.owner in (None, user)
=== modified file 'src/maasserver/testing/__init__.py'
--- src/maasserver/testing/__init__.py 2012-01-19 10:32:27 +0000
+++ src/maasserver/testing/__init__.py 2012-01-24 17:14:25 +0000
@@ -0,0 +1,24 @@
+# Copyright 2012 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+from __future__ import (
+ print_function,
+ unicode_literals,
+ )
+
+"""Test maasserver API."""
+
+__metaclass__ = type
+__all__ = []
+
+from maasserver.testing.factory import factory
+from maastesting import TestCase
+
+
+class LoggedInTestCase(TestCase):
+
+ def setUp(self):
+ super(LoggedInTestCase, self).setUp()
+ self.logged_in_user = factory.make_user(password='test')
+ self.client.login(
+ username=self.logged_in_user.username, password='test')
=== modified file 'src/maasserver/tests/test_api.py'
--- src/maasserver/tests/test_api.py 2012-01-24 15:25:49 +0000
+++ src/maasserver/tests/test_api.py 2012-01-24 17:14:25 +0000
@@ -14,13 +14,16 @@
import httplib
import json
-from django.test.client import Client
from maasserver.models import (
MACAddress,
Node,
+ NODE_STATUS,
+ )
+from maasserver.testing import (
+ LoggedInTestCase,
+ TestCase,
)
from maasserver.testing.factory import factory
-from maastesting import TestCase
class NodeAnonAPITest(TestCase):
@@ -32,28 +35,31 @@
self.assertEqual(httplib.UNAUTHORIZED, response.status_code)
-class LoggedInMixin(object):
+class APITestMixin(object):
def setUp(self):
- super(LoggedInMixin, self).setUp()
- self.user = factory.make_user(username='user', password='test')
- self.client = Client()
- self.client.login(username='user', password='test')
-
-
-class NodeAPITest(LoggedInMixin, TestCase):
+ super(APITestMixin, self).setUp()
+ self.other_user = factory.make_user()
+ self.other_node = factory.make_node(
+ status=NODE_STATUS.DEPLOYED, owner=self.other_user)
+
+
+class NodeAPITest(APITestMixin, LoggedInTestCase):
def test_nodes_GET(self):
"""
The api allows for fetching the list of Nodes.
"""
- node1 = factory.make_node(set_hostname=True)
- node2 = factory.make_node(set_hostname=True)
+ node1 = factory.make_node()
+ node2 = factory.make_node(
+ set_hostname=True, status=NODE_STATUS.DEPLOYED,
+ owner=self.logged_in_user)
response = self.client.get('/api/nodes/')
parsed_result = json.loads(response.content)
- self.assertEqual(200, response.status_code)
+ self.assertEqual(httplib.OK, response.status_code)
+ self.assertEqual(2, len(parsed_result))
self.assertEqual(node1.system_id, parsed_result[0]['system_id'])
self.assertEqual(node2.system_id, parsed_result[1]['system_id'])
@@ -66,11 +72,22 @@
response = self.client.get('/api/nodes/%s/' % node.system_id)
parsed_result = json.loads(response.content)
- self.assertEqual(200, response.status_code)
+ self.assertEqual(httplib.OK, response.status_code)
self.assertEqual(node.hostname, parsed_result['hostname'])
self.assertEqual(node.system_id, parsed_result['system_id'])
- def test_node_GET_404(self):
+ def test_node_GET_non_visible_node(self):
+ """
+ The request to fetch a single node is denied if the node isn't visible
+ by the user.
+
+ """
+ response = self.client.get(
+ '/api/nodes/%s/' % self.other_node.system_id)
+
+ self.assertEqual(httplib.OK, response.status_code)
+
+ def test_node_GET_not_found(self):
"""
When fetching a Node, the api returns a 'Not Found' (404) error
if no node is found.
@@ -89,7 +106,7 @@
'/api/nodes/', {'hostname': 'diane'})
parsed_result = json.loads(response.content)
- self.assertEqual(200, response.status_code)
+ self.assertEqual(httplib.OK, response.status_code)
self.assertEqual('diane', parsed_result['hostname'])
self.assertEqual(1, Node.objects.filter(hostname='diane').count())
@@ -103,12 +120,12 @@
'/api/nodes/%s/' % node.system_id, {'hostname': 'francis'})
parsed_result = json.loads(response.content)
- self.assertEqual(200, response.status_code)
+ self.assertEqual(httplib.OK, response.status_code)
self.assertEqual('francis', parsed_result['hostname'])
self.assertEqual(0, Node.objects.filter(hostname='diane').count())
self.assertEqual(1, Node.objects.filter(hostname='francis').count())
- def test_node_PUT_404(self):
+ def test_node_PUT_not_found(self):
"""
When updating a Node, the api returns a 'Not Found' (404) error
if no node is found.
@@ -144,7 +161,7 @@
self.assertEqual(
[], list(Node.objects.filter(system_id=system_id)))
- def test_node_DELETE_404(self):
+ def test_node_DELETE_not_found(self):
"""
When deleting a Node, the api returns a 'Not Found' (404) error
if no node is found.
@@ -155,7 +172,7 @@
self.assertEqual(httplib.NOT_FOUND, response.status_code)
-class MACAddressAPITest(LoggedInMixin, TestCase):
+class MACAddressAPITest(APITestMixin, LoggedInTestCase):
def setUp(self):
super(MACAddressAPITest, self).setUp()
@@ -171,14 +188,25 @@
response = self.client.get('/api/nodes/%s/macs/' % self.node.system_id)
parsed_result = json.loads(response.content)
- self.assertEqual(200, response.status_code)
+ self.assertEqual(httplib.OK, response.status_code)
self.assertEqual(2, len(parsed_result))
self.assertEqual(
self.mac1.mac_address, parsed_result[0]['mac_address'])
self.assertEqual(
self.mac2.mac_address, parsed_result[1]['mac_address'])
- def test_macs_GET_404(self):
+ def test_macs_GET_forbidden(self):
+ """
+ When fetching MAC Addresses, the api returns a 'Unauthorized' (401)
+ error if the node is not visible to the logged-in user.
+
+ """
+ response = self.client.get(
+ '/api/nodes/%s/macs/' % self.other_node.system_id)
+
+ self.assertEqual(httplib.UNAUTHORIZED, response.status_code)
+
+ def test_macs_GET_not_found(self):
"""
When fetching MAC Addresses, the api returns a 'Not Found' (404)
error if no node is found.
@@ -188,7 +216,7 @@
self.assertEqual(httplib.NOT_FOUND, response.status_code)
- def test_macs_GET_node_404(self):
+ def test_macs_GET_node_not_found(self):
"""
When fetching a MAC Address, the api returns a 'Not Found' (404)
error if the MAC Address does not exist.
@@ -199,7 +227,18 @@
self.assertEqual(httplib.NOT_FOUND, response.status_code)
- def test_macs_GET_node_400(self):
+ def test_macs_GET_node_forbidden(self):
+ """
+ When fetching a MAC Address, the api returns a 'Unauthorized' (401)
+ error if the node is not visible to the logged-in user.
+
+ """
+ response = self.client.get(
+ '/api/nodes/%s/macs/0-aa-22-cc-44-dd/' % self.other_node.system_id)
+
+ self.assertEqual(httplib.UNAUTHORIZED, response.status_code)
+
+ def test_macs_GET_node_bad_request(self):
"""
When fetching a MAC Address, the api returns a 'Bad Request' (400)
error if the MAC Address is not valid.
@@ -221,7 +260,7 @@
{'mac_address': 'AA:BB:CC:DD:EE:FF'})
parsed_result = json.loads(response.content)
- self.assertEqual(200, response.status_code)
+ self.assertEqual(httplib.OK, response.status_code)
self.assertEqual('AA:BB:CC:DD:EE:FF', parsed_result['mac_address'])
self.assertEqual(
nb_macs + 1,
@@ -257,7 +296,19 @@
nb_macs - 1,
self.node.macaddress_set.count())
- def test_macs_DELETE_404(self):
+ def test_macs_DELETE_mac_forbidden(self):
+ """
+ When deleting a MAC Address, the api returns a 'Unauthorized' (401)
+ error if the node is not visible to the logged-in user.
+
+ """
+ response = self.client.delete(
+ '/api/nodes/%s/macs/%s/' % (
+ self.other_node.system_id, self.mac1.mac_address))
+
+ self.assertEqual(httplib.UNAUTHORIZED, response.status_code)
+
+ def test_macs_DELETE_not_found(self):
"""
When deleting a MAC Address, the api returns a 'Not Found' (404)
error if no existing MAC Address is found.
@@ -269,7 +320,7 @@
self.assertEqual(httplib.NOT_FOUND, response.status_code)
- def test_macs_DELETE_400(self):
+ def test_macs_DELETE_bad_request(self):
"""
When deleting a MAC Address, the api returns a 'Bad Request' (400)
error if the provided MAC Address is not valid.
=== modified file 'src/maasserver/tests/test_auth.py'
--- src/maasserver/tests/test_auth.py 2012-01-24 13:21:41 +0000
+++ src/maasserver/tests/test_auth.py 2012-01-24 17:14:25 +0000
@@ -11,6 +11,8 @@
__metaclass__ = type
__all__ = []
+import httplib
+
from django.core.urlresolvers import reverse
from maasserver.models import (
MaaSAuthorizationBackend,
@@ -31,14 +33,14 @@
response = self.client.post(
reverse('login'), {'username': 'test', 'password': 'test'})
- self.assertEqual(302, response.status_code)
+ self.assertEqual(httplib.FOUND, response.status_code)
self.assertEqual(self.user.id, self.client.session['_auth_user_id'])
def test_login_failed(self):
response = self.client.post(
reverse('login'), {'username': 'test', 'password': 'wrong-pw'})
- self.assertEqual(200, response.status_code)
+ self.assertEqual(httplib.OK, response.status_code)
self.assertNotIn('_auth_user_id', self.client.session.keys())
def test_logout(self):
@@ -77,10 +79,6 @@
NotImplementedError, self.backend.has_perm,
self.admin, 'not-access', self.not_owned_node)
- def test_admin_access(self):
- self.assertTrue(self.backend.has_perm(
- self.admin, 'access', self.node_user1))
-
def test_not_owned_status(self):
# A non-admin user can access a node that is not yet owned.
self.assertTrue(self.backend.has_perm(
