launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #06962
[Merge] lp:~rvb/maas/maas-api-permissions2 into lp:maas
Raphaël Badin has proposed merging lp:~rvb/maas/maas-api-permissions2 into lp:maas with lp:~rvb/maas/maas-api-permissions as a prerequisite.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~rvb/maas/maas-api-permissions2/+merge/100434
This branches fixes some security issues. It may seem large but most of the changes are fairly mechanical. The main goal is to fix a few security issues and to unify the API and the view code when it comes to security checks. I think we need to audit the API/view security but this is a first step.
In details this branch:
- changes "Node.objects.get_visible_node_or_404" into "Node.objects.get_node_or_404" which now takes a permission name. It's only an extension of what get_visible_node_or_404 use to do: it can now check other permissions. I used that new method in the api code and in the view code where it was missing or mis-used. For instance API.NodeHandled:update was checking for the 'access' permission instead of checking for the 'edit' permission. I've updated the tests accordingly and added new tests when appropriate.
- removes our custom maasserver.exceptions.PermissionDenied exception. Instead, django.code.exception.PermissionDenied is used. The reason for that is that we want to raise PermissionDenied both from the view code and from the API code. Django handles all the raised django.code.exception.PermissionDenied by showing a predefined 403 error page. I also had to change our APIExceptionMiddleware to handle those errors. Another solution would have been to create another middleware and handle maasserver.exceptions.PermissionDenied raised from the view code but I thought sticking to Django's exception was the most simple way to fix this.
--
https://code.launchpad.net/~rvb/maas/maas-api-permissions2/+merge/100434
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rvb/maas/maas-api-permissions2 into lp:maas.
=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py 2012-03-30 10:39:56 +0000
+++ src/maasserver/api.py 2012-04-02 14:50:29 +0000
@@ -73,7 +73,10 @@
from textwrap import dedent
import types
-from django.core.exceptions import ValidationError
+from django.core.exceptions import (
+ PermissionDenied,
+ ValidationError,
+ )
from django.http import (
HttpResponse,
HttpResponseBadRequest,
@@ -91,7 +94,6 @@
MAASAPINotFound,
NodesNotAvailable,
NodeStateViolation,
- PermissionDenied,
Unauthorized,
)
from maasserver.fields import validate_mac
@@ -317,23 +319,22 @@
def read(self, request, system_id):
"""Read a specific Node."""
- return Node.objects.get_visible_node_or_404(
- system_id=system_id, user=request.user)
+ return Node.objects.get_node_or_404(
+ system_id=system_id, user=request.user, perm='access')
def update(self, request, system_id):
"""Update a specific Node."""
- node = Node.objects.get_visible_node_or_404(
- system_id=system_id, user=request.user)
+ node = Node.objects.get_node_or_404(
+ system_id=system_id, user=request.user, perm='edit')
for key, value in request.data.items():
setattr(node, key, value)
- node.full_clean()
node.save()
return node
def delete(self, request, system_id):
"""Delete a specific Node."""
- node = Node.objects.get_visible_node_or_404(
- system_id=system_id, user=request.user)
+ node = Node.objects.get_node_or_404(
+ system_id=system_id, user=request.user, perm='edit')
node.delete()
return rc.DELETED
@@ -383,8 +384,8 @@
@api_exported('release', 'POST')
def release(self, request, system_id):
"""Release a node. Opposite of `NodesHandler.acquire`."""
- node = Node.objects.get_visible_node_or_404(
- system_id=system_id, user=request.user)
+ node = Node.objects.get_node_or_404(
+ system_id=system_id, user=request.user, perm='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.
@@ -564,20 +565,17 @@
def read(self, request, system_id):
"""Read all MAC Addresses related to a Node."""
- node = Node.objects.get_visible_node_or_404(
- user=request.user, system_id=system_id)
+ node = Node.objects.get_node_or_404(
+ user=request.user, system_id=system_id, perm='access')
return MACAddress.objects.filter(node=node).order_by('id')
def create(self, request, system_id):
"""Create a MAC Address for a specified Node."""
- 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 ValidationError as e:
- return HttpResponseBadRequest(e.message_dict)
+ node = Node.objects.get_node_or_404(
+ user=request.user, system_id=system_id, perm='edit')
+ mac = node.add_mac_address(request.data.get('mac_address', None))
+ return mac
@classmethod
def resource_uri(cls, *args, **kwargs):
@@ -592,8 +590,8 @@
def read(self, request, system_id, mac_address):
"""Read a MAC Address related to a Node."""
- node = Node.objects.get_visible_node_or_404(
- user=request.user, system_id=system_id)
+ node = Node.objects.get_node_or_404(
+ user=request.user, system_id=system_id, perm='access')
validate_mac(mac_address)
return get_object_or_404(
@@ -602,8 +600,8 @@
def delete(self, request, system_id, mac_address):
"""Delete a specific MAC Address for the specified Node."""
validate_mac(mac_address)
- node = Node.objects.get_visible_node_or_404(
- user=request.user, system_id=system_id)
+ node = Node.objects.get_node_or_404(
+ user=request.user, system_id=system_id, perm='edit')
mac = get_object_or_404(MACAddress, node=node, mac_address=mac_address)
mac.delete()
=== modified file 'src/maasserver/exceptions.py'
--- src/maasserver/exceptions.py 2012-03-26 05:07:12 +0000
+++ src/maasserver/exceptions.py 2012-04-02 14:50:29 +0000
@@ -15,7 +15,6 @@
"MAASAPIException",
"MAASAPINotFound",
"NodeStateViolation",
- "PermissionDenied",
]
@@ -52,16 +51,6 @@
api_error = httplib.NOT_FOUND
-class PermissionDenied(MAASAPIException):
- """HTTP error 403: Forbidden. User is logged in, but lacks permission.
-
- Do not confuse this with 401: Unauthorized ("you need to be logged in
- for this, so please authenticate"). The Piston error codes do confuse
- the two.
- """
- api_error = httplib.FORBIDDEN
-
-
class Unauthorized(MAASAPIException):
"""HTTP error 401: Unauthorized. Login required."""
api_error = httplib.UNAUTHORIZED
=== modified file 'src/maasserver/middleware.py'
--- src/maasserver/middleware.py 2012-03-23 19:20:59 +0000
+++ src/maasserver/middleware.py 2012-04-02 14:50:29 +0000
@@ -25,11 +25,15 @@
import re
from django.conf import settings
-from django.core.exceptions import ValidationError
+from django.core.exceptions import (
+ PermissionDenied,
+ ValidationError,
+ )
from django.core.urlresolvers import reverse
from django.http import (
HttpResponse,
HttpResponseBadRequest,
+ HttpResponseForbidden,
HttpResponseRedirect,
)
from django.utils.http import urlquote_plus
@@ -141,6 +145,10 @@
return HttpResponseBadRequest(
unicode(''.join(exception.messages)).encode(encoding),
mimetype=b"text/plain; charset=%s" % encoding)
+ elif isinstance(exception, PermissionDenied):
+ return HttpResponseForbidden(
+ content=unicode(exception).encode(encoding),
+ mimetype=b"text/plain; charset=%s" % encoding)
else:
# Return an API-readable "Internal Server Error" response.
return HttpResponse(
=== modified file 'src/maasserver/models.py'
--- src/maasserver/models.py 2012-03-30 17:01:12 +0000
+++ src/maasserver/models.py 2012-04-02 14:50:29 +0000
@@ -44,7 +44,10 @@
from django.contrib import admin
from django.contrib.auth.backends import ModelBackend
from django.contrib.auth.models import User
-from django.core.exceptions import ValidationError
+from django.core.exceptions import (
+ PermissionDenied,
+ ValidationError,
+ )
from django.core.files.base import ContentFile
from django.core.files.storage import FileSystemStorage
from django.db import models
@@ -54,7 +57,6 @@
from maasserver.exceptions import (
CannotDeleteUserException,
NodeStateViolation,
- PermissionDenied,
)
from maasserver.fields import (
JSONObjectField,
@@ -329,7 +331,7 @@
visible_nodes = self.filter(owner=user)
return self.filter_by_ids(visible_nodes, ids)
- def get_visible_node_or_404(self, system_id, user):
+ def get_node_or_404(self, system_id, user, perm='access'):
"""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`.
@@ -337,6 +339,8 @@
:type name: str
:param user: The user that should be used in the permission check.
:type user: django.contrib.auth.models.User
+ :param perm: The permission to assert that the user has on the node.
+ :type perm: basestring
:raises: django.http.Http404_,
:class:`maasserver.exceptions.PermissionDenied`.
@@ -345,10 +349,10 @@
#the-http404-exception
"""
node = get_object_or_404(Node, system_id=system_id)
- if user.has_perm('access', node):
+ if user.has_perm(perm, node):
return node
else:
- raise PermissionDenied
+ raise PermissionDenied()
def get_available_node_for_acquisition(self, for_user, constraints=None):
"""Find a `Node` to be acquired by the given user.
@@ -1187,7 +1191,8 @@
return obj.owner == user
else:
raise NotImplementedError(
- 'Invalid permission check (invalid permission name).')
+ 'Invalid permission check (invalid permission name: %s).' %
+ perm)
# 'provisioning' is imported so that it can register its signal handlers early
=== modified file 'src/maasserver/testing/testcase.py'
--- src/maasserver/testing/testcase.py 2012-03-21 05:38:18 +0000
+++ src/maasserver/testing/testcase.py 2012-04-02 14:50:29 +0000
@@ -38,3 +38,13 @@
self.logged_in_user = factory.make_user(password='test')
self.client.login(
username=self.logged_in_user.username, password='test')
+
+ def become_admin(self):
+ """Promote the logged-in user to admin."""
+ self.logged_in_user.is_superuser = True
+ self.logged_in_user.save()
+ self.addCleanup(self.become_simpleuser)
+
+ def become_simpleuser(self):
+ self.logged_in_user.is_superuser = False
+ self.logged_in_user.save()
=== modified file 'src/maasserver/tests/test_api.py'
--- src/maasserver/tests/test_api.py 2012-03-30 10:39:56 +0000
+++ src/maasserver/tests/test_api.py 2012-04-02 14:50:29 +0000
@@ -507,12 +507,19 @@
self.assertIs(None, node.token)
def test_POST_release_does_nothing_for_unowned_node(self):
- node = factory.make_node(status=NODE_STATUS.READY)
+ node = factory.make_node(
+ status=NODE_STATUS.READY, owner=self.logged_in_user)
response = self.client.post(
self.get_node_uri(node), {'op': 'release'})
self.assertEqual(httplib.OK, response.status_code)
self.assertEqual(NODE_STATUS.READY, reload_object(node).status)
+ def test_POST_release_forbidden_if_user_cannot_edit_node(self):
+ node = factory.make_node(status=NODE_STATUS.READY)
+ response = self.client.post(
+ self.get_node_uri(node), {'op': 'release'})
+ self.assertEqual(httplib.FORBIDDEN, response.status_code)
+
def test_POST_release_fails_for_other_node_states(self):
releasable_statuses = [
NODE_STATUS.RESERVED,
@@ -577,7 +584,7 @@
def test_PUT_updates_node(self):
# The api allows the updating of a Node.
- node = factory.make_node(hostname='diane')
+ node = factory.make_node(hostname='diane', owner=self.logged_in_user)
response = self.client.put(
self.get_node_uri(node), {'hostname': 'francis'})
parsed_result = json.loads(response.content)
@@ -590,7 +597,7 @@
def test_resource_uri_points_back_at_node(self):
# When a Node is returned by the API, the field 'resource_uri'
# provides the URI for this Node.
- node = factory.make_node(hostname='diane')
+ node = factory.make_node(hostname='diane', owner=self.logged_in_user)
response = self.client.put(
self.get_node_uri(node), {'hostname': 'francis'})
parsed_result = json.loads(response.content)
@@ -602,7 +609,7 @@
def test_PUT_rejects_invalid_data(self):
# If the data provided to update a node is invalid, a 'Bad request'
# response is returned.
- node = factory.make_node(hostname='diane')
+ node = factory.make_node(hostname='diane', owner=self.logged_in_user)
response = self.client.put(
self.get_node_uri(node), {'hostname': 'too long' * 100})
parsed_result = json.loads(response.content)
@@ -633,13 +640,20 @@
def test_DELETE_deletes_node(self):
# The api allows to delete a Node.
- node = factory.make_node(set_hostname=True)
+ 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))
self.assertEqual(204, response.status_code)
self.assertItemsEqual([], Node.objects.filter(system_id=system_id))
+ 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)
+ response = self.client.delete(self.get_node_uri(node))
+
+ self.assertEqual(httplib.FORBIDDEN, response.status_code)
+
def test_DELETE_refuses_to_delete_invisible_node(self):
# The request to delete a single node is denied if the node isn't
# visible by the user.
@@ -1043,8 +1057,8 @@
class MACAddressAPITest(APITestCase):
- def createNodeWithMacs(self):
- node = factory.make_node()
+ def createNodeWithMacs(self, owner=None):
+ node = factory.make_node(owner=owner)
mac1 = node.add_mac_address('aa:bb:cc:dd:ee:ff')
mac2 = node.add_mac_address('22:bb:cc:dd:aa:ff')
return node, mac1, mac2
@@ -1112,7 +1126,7 @@
def test_macs_POST_add_mac(self):
# The api allows to add a MAC Address to an existing node.
- node = factory.make_node()
+ node = factory.make_node(owner=self.logged_in_user)
nb_macs = MACAddress.objects.filter(node=node).count()
response = self.client.post(
self.get_uri('nodes/%s/macs/') % node.system_id,
@@ -1125,10 +1139,19 @@
nb_macs + 1,
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.
+ node = factory.make_node()
+ response = self.client.post(
+ self.get_uri('nodes/%s/macs/') % node.system_id,
+ {'mac_address': '01:BB:CC:DD:EE:FF'})
+
+ self.assertEqual(httplib.FORBIDDEN, response.status_code)
+
def test_macs_POST_add_mac_invalid(self):
# A 'Bad Request' response is returned if one tries to add an invalid
# MAC Address to a node.
- node = self.createNodeWithMacs()[0]
+ node = self.createNodeWithMacs(self.logged_in_user)[0]
response = self.client.post(
self.get_uri('nodes/%s/macs/') % node.system_id,
{'mac_address': 'invalid-mac'})
@@ -1142,7 +1165,7 @@
def test_macs_DELETE_mac(self):
# The api allows to delete a MAC Address.
- node, mac1, mac2 = self.createNodeWithMacs()
+ node, mac1, mac2 = self.createNodeWithMacs(self.logged_in_user)
nb_macs = node.macaddress_set.count()
response = self.client.delete(
self.get_uri('nodes/%s/macs/%s/') % (
@@ -1168,7 +1191,18 @@
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.
- node = factory.make_node()
+ node = factory.make_node(owner=self.logged_in_user)
+ response = self.client.delete(
+ self.get_uri('nodes/%s/macs/%s/') % (
+ node.system_id, '00-aa-22-cc-44-dd'))
+
+ self.assertEqual(httplib.NOT_FOUND, response.status_code)
+
+ def test_macs_DELETE_forbidden(self):
+ # When deleting a MAC Address, the api returns a 'Forbidden'
+ # (403) error if the user has not the 'edit' permission on the
+ # node.
+ node = factory.make_node(owner=self.logged_in_user)
response = self.client.delete(
self.get_uri('nodes/%s/macs/%s/') % (
node.system_id, '00-aa-22-cc-44-dd'))
=== 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 14:50:29 +0000
@@ -20,13 +20,15 @@
from django.conf import settings
from django.contrib.auth.models import User
-from django.core.exceptions import ValidationError
+from django.core.exceptions import (
+ PermissionDenied,
+ ValidationError,
+ )
from django.utils.safestring import SafeUnicode
from fixtures import TestWithFixtures
from maasserver.exceptions import (
CannotDeleteUserException,
NodeStateViolation,
- PermissionDenied,
)
from maasserver.models import (
Config,
@@ -408,20 +410,20 @@
Node.objects.get_editable_nodes(user, ids=ids[wanted_slice]))
def test_get_visible_node_or_404_ok(self):
- """get_visible_node_or_404 fetches nodes by system_id."""
+ """get_node_or_404 fetches nodes by system_id."""
user = factory.make_user()
node = self.make_node(user)
self.assertEqual(
- node, Node.objects.get_visible_node_or_404(node.system_id, user))
+ node, Node.objects.get_node_or_404(node.system_id, user))
def test_get_visible_node_or_404_raises_PermissionDenied(self):
- """get_visible_node_or_404 raises PermissionDenied if the provided
- user cannot access the returned node."""
+ """get_node_or_404 raises PermissionDenied if the provided
+ user has not the right permission on the returned node."""
user_node = self.make_node(factory.make_user())
self.assertRaises(
PermissionDenied,
- Node.objects.get_visible_node_or_404,
- user_node.system_id, factory.make_user())
+ Node.objects.get_node_or_404,
+ user_node.system_id, factory.make_user(), 'access')
def test_get_available_node_for_acquisition_finds_available_node(self):
user = factory.make_user()
=== modified file 'src/maasserver/tests/test_views.py'
--- src/maasserver/tests/test_views.py 2012-04-02 14:50:29 +0000
+++ src/maasserver/tests/test_views.py 2012-04-02 14:50:29 +0000
@@ -490,12 +490,11 @@
node_edit_link = reverse('node-edit', args=[node.system_id])
self.assertIn(node_edit_link, get_content_links(response))
- def test_view_node_no_link_to_edit_someonelses_node(self):
+ def test_user_cannot_view_someonelses_node(self):
node = factory.make_node(owner=factory.make_user())
- node_link = reverse('node-view', args=[node.system_id])
- response = self.client.get(node_link)
- node_edit_link = reverse('node-edit', args=[node.system_id])
- self.assertNotIn(node_edit_link, get_content_links(response))
+ node_view_link = reverse('node-view', args=[node.system_id])
+ response = self.client.get(node_view_link)
+ self.assertEqual(httplib.FORBIDDEN, response.status_code)
def test_user_cannot_edit_someonelses_node(self):
node = factory.make_node(owner=factory.make_user())
@@ -503,6 +502,20 @@
response = self.client.get(node_edit_link)
self.assertEqual(httplib.FORBIDDEN, response.status_code)
+ def test_admin_can_view_someonelses_node(self):
+ self.become_admin()
+ node = factory.make_node(owner=factory.make_user())
+ node_view_link = reverse('node-view', args=[node.system_id])
+ response = self.client.get(node_view_link)
+ self.assertEqual(httplib.OK, response.status_code)
+
+ def test_admin_can_edit_someonelses_node(self):
+ self.become_admin()
+ node = factory.make_node(owner=factory.make_user())
+ node_edit_link = reverse('node-edit', args=[node.system_id])
+ response = self.client.get(node_edit_link)
+ self.assertEqual(httplib.OK, response.status_code)
+
def test_user_can_access_the_edition_page_for_his_nodes(self):
node = factory.make_node(owner=self.logged_in_user)
node_edit_link = reverse('node-edit', args=[node.system_id])
=== modified file 'src/maasserver/views.py'
--- src/maasserver/views.py 2012-04-02 14:50:29 +0000
+++ src/maasserver/views.py 2012-04-02 14:50:29 +0000
@@ -44,7 +44,6 @@
login as dj_login,
logout as dj_logout,
)
-from django.core.exceptions import PermissionDenied
from django.core.urlresolvers import reverse
from django.http import (
HttpResponse,
@@ -109,7 +108,9 @@
def get_object(self):
system_id = self.kwargs.get('system_id', None)
- return get_object_or_404(Node, system_id=system_id)
+ node = Node.objects.get_node_or_404(
+ system_id=system_id, user=self.request.user, perm='access')
+ return node
def get_context_data(self, **kwargs):
context = super(NodeView, self).get_context_data(**kwargs)
@@ -124,9 +125,8 @@
def get_object(self):
system_id = self.kwargs.get('system_id', None)
- node = get_object_or_404(Node, system_id=system_id)
- if not self.request.user.has_perm('edit', node):
- raise PermissionDenied()
+ node = Node.objects.get_node_or_404(
+ system_id=system_id, user=self.request.user, perm='edit')
return node
def get_form_class(self):
=== modified file 'src/metadataserver/api.py'
--- src/metadataserver/api.py 2012-03-27 04:09:05 +0000
+++ src/metadataserver/api.py 2012-04-02 14:50:29 +0000
@@ -16,11 +16,11 @@
'VersionIndexHandler',
]
+from django.core.exceptions import PermissionDenied
from django.http import HttpResponse
from maasserver.api import extract_oauth_key
from maasserver.exceptions import (
MAASAPINotFound,
- PermissionDenied,
Unauthorized,
)
from maasserver.models import SSHKey
