launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/block-node-init-user-from-api into lp:maas with lp:~jtv/maas/metadata-auth as a prerequisite.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~jtv/maas/block-node-init-user-from-api/+merge/94227

Block the node-init user (the identity under which nodes log in to get to the metadata service) from accessing the UI and the API.

As per Raphaël's suggestion, this abuses piston's sloppy authentication checking: set the node-init user's is_active to False, which the UI respects but piston ignores; then add explicit permissions checking to the MaaS API.

(Conversely, the metadata service will only provide information meant for the node whose OAuth key is used to authenticate the request.  So there's nothing to gain from similar checking there.)
-- 
https://code.launchpad.net/~jtv/maas/block-node-init-user-from-api/+merge/94227
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/maas/block-node-init-user-from-api into lp:maas.

=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py	2012-02-21 19:05:33 +0000
+++ src/maasserver/api.py	2012-02-22 17:09:57 +0000
@@ -64,6 +64,26 @@
     }
 
 
+def access_restricted(handler_method):
+    """API method decorator: restrict access to handler.
+
+    In particular, the node-init user and deactivated users are not allowed
+    access to API methods.
+
+    Use this only on Piston request-handling methods.  The method is
+    assumed to take a "self" as its first argument, and a Django web
+    request (or something that looks like one) as a second.  There may
+    be other arguments, including keyword arguments.
+    """
+
+    def call_method(self, request, *args, **kwargs):
+        if not request.user.is_active:
+            raise PermissionDenied("User is not allowed access to this API.")
+        return handler_method(self, request, *args, **kwargs)
+
+    return call_method
+
+
 def api_exported(operation_name=True, method='POST'):
     def _decorator(func):
         if method not in dispatch_methods:
@@ -186,11 +206,13 @@
     model = Node
     fields = ('system_id', 'hostname', ('macaddress_set', ('mac_address',)))
 
+    @access_restricted
     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)
 
+    @access_restricted
     def update(self, request, system_id):
         """Update a specific Node."""
         node = Node.objects.get_visible_node_or_404(
@@ -201,6 +223,7 @@
         node.save()
         return node
 
+    @access_restricted
     def delete(self, request, system_id):
         """Delete a specific Node."""
         node = Node.objects.get_visible_node_or_404(
@@ -221,6 +244,7 @@
         return ('node_handler', (node_system_id, ))
 
     @api_exported('stop', 'POST')
+    @access_restricted
     def stop(self, request, system_id):
         """Shut down a node."""
         nodes = Node.objects.stop_nodes([system_id], request.user)
@@ -230,6 +254,7 @@
         return nodes[0]
 
     @api_exported('start', 'POST')
+    @access_restricted
     def start(self, request, system_id):
         """Power up a node."""
         nodes = Node.objects.start_nodes([system_id], request.user)
@@ -245,6 +270,7 @@
     allowed_methods = ('GET', 'POST',)
 
     @api_exported('list', 'GET')
+    @access_restricted
     def list(self, request):
         """List Nodes visible to the user, optionally filtered by id."""
         match_ids = request.GET.getlist('id')
@@ -254,6 +280,7 @@
         return nodes.order_by('id')
 
     @api_exported('new', 'POST')
+    @access_restricted
     def new(self, request):
         """Create a new Node."""
         form = NodeWithMACAddressesForm(request.data)
@@ -265,6 +292,7 @@
                 form.errors, content_type='application/json')
 
     @api_exported('acquire', 'POST')
+    @access_restricted
     def acquire(self, request):
         """Acquire an available node for deployment."""
         node = Node.objects.get_available_node_for_acquisition(request.user)

=== modified file 'src/maasserver/models.py'
--- src/maasserver/models.py	2012-02-22 10:59:05 +0000
+++ src/maasserver/models.py	2012-02-22 17:09:57 +0000
@@ -653,6 +653,11 @@
     supports_object_permissions = True
 
     def has_perm(self, user, perm, obj=None):
+        if not user.is_active:
+            # Deactivated users, and in particular the node-init user,
+            # are prohibited from accessing maasserver services.
+            return False
+
         # Only Nodes can be checked. We also don't support perm checking
         # when obj = None.
         if not isinstance(obj, Node):

=== modified file 'src/maasserver/testing/oauthclient.py'
--- src/maasserver/testing/oauthclient.py	2012-02-22 17:09:57 +0000
+++ src/maasserver/testing/oauthclient.py	2012-02-22 17:09:57 +0000
@@ -16,6 +16,7 @@
 from time import time
 
 from django.test.client import Client
+from maasserver.models import get_auth_tokens
 from oauth.oauth import (
     generate_nonce,
     OAuthConsumer,
@@ -41,7 +42,7 @@
         super(OAuthAuthenticatedClient, self).__init__()
         if token is None:
             # Get the user's first token.
-            token = user.get_profile().get_authorisation_tokens()[0]
+            token = get_auth_tokens(user)[0]
         assert token.user == user, "Token does not match User."
         consumer = token.consumer
         self.consumer = OAuthConsumer(str(consumer.key), str(consumer.secret))

=== modified file 'src/maasserver/tests/test_api.py'
--- src/maasserver/tests/test_api.py	2012-02-21 11:00:32 +0000
+++ src/maasserver/tests/test_api.py	2012-02-22 17:09:57 +0000
@@ -28,6 +28,8 @@
     )
 from maasserver.testing.factory import factory
 from maasserver.testing.oauthclient import OAuthAuthenticatedClient
+from metadataserver.models import NodeKey
+from metadataserver.nodeinituser import get_node_init_user
 
 
 class NodeAnonAPITest(TestCase):
@@ -44,6 +46,12 @@
 
         self.assertEqual(httplib.OK, response.status_code)
 
+    def test_node_init_user_cannot_access(self):
+        token = NodeKey.objects.create_token(factory.make_node())
+        client = OAuthAuthenticatedClient(get_node_init_user(), token)
+        response = client.get('/api/nodes/', {'op': 'list'})
+        self.assertEqual(httplib.FORBIDDEN, response.status_code)
+
 
 class APITestCase(TestCase):
     """Extension to `TestCase`: log in first.

=== modified file 'src/maasserver/tests/test_auth.py'
--- src/maasserver/tests/test_auth.py	2012-02-20 16:09:07 +0000
+++ src/maasserver/tests/test_auth.py	2012-02-22 17:09:57 +0000
@@ -21,6 +21,7 @@
     )
 from maasserver.testing import TestCase
 from maasserver.testing.factory import factory
+from metadataserver.nodeinituser import get_node_init_user
 
 
 class LoginLogoutTest(TestCase):
@@ -86,6 +87,11 @@
             NotImplementedError, backend.has_perm,
             factory.make_admin(), 'not-access', make_unallocated_node())
 
+    def test_node_init_user_cannot_access(self):
+        backend = MaaSAuthorizationBackend()
+        self.assertFalse(backend.has_perm(
+            get_node_init_user(), 'access', make_unallocated_node()))
+
     def test_user_can_access_unowned_node(self):
         backend = MaaSAuthorizationBackend()
         self.assertTrue(backend.has_perm(

=== modified file 'src/metadataserver/middleware.py'
--- src/metadataserver/middleware.py	2012-02-22 17:09:57 +0000
+++ src/metadataserver/middleware.py	2012-02-22 17:09:57 +0000
@@ -14,7 +14,6 @@
     ]
 
 from django.conf import settings
-
 from maasserver.middleware import ExceptionMiddleware