Merge ~blake-rouse/maas:rbac-pools-cache-perms into maas:master

[Blake Rouse]

Use a thread-local cache to cache resource pool identifiers per request.

37c1e2a... by Blake Rouse on 2018-11-20

Add tests.

Unmerged commits

37c1e2a... by Blake Rouse on 2018-11-20

Add tests.

bcd535d... by Blake Rouse on 2018-11-20

Split 1.

diff --git a/src/maasserver/models/__init__.py b/src/maasserver/models/__init__.py
index 3888e17..ca79112 100644
--- a/src/maasserver/models/__init__.py
+++ b/src/maasserver/models/__init__.py
@@ -348,12 +348,10 @@ class MAASAuthorizationBackend(ModelBackend):
         rbac_enabled = rbac.is_enabled()
         visible_pools, admin_pools = [], []
         if rbac_enabled:
-            visible_pools = rbac.get_resource_pools(
-                user.username, 'view').values_list(
-                    'id', flat=True)
-            admin_pools = rbac.get_resource_pools(
-                user.username, 'admin-machines').values_list(
-                    'id', flat=True)
+            visible_pools = rbac.get_resource_pool_ids(
+                user.username, 'view')
+            admin_pools = rbac.get_resource_pool_ids(
+                user.username, 'admin-machines')
 
         # Sanity check that a `ResourcePool` is being checked against
         # `ResourcePoolPermission`.
@@ -363,6 +361,22 @@ class MAASAuthorizationBackend(ModelBackend):
                 'obj type of ResourcePool must be checked '
                 'against a `ResourcePoolPermission`.')
 
+        # Handle node permissions without objects.
+        if perm == NodePermission.admin and obj is None:
+            # User wants to admin writes to all nodes (aka. create a node),
+            # must be superuser for those permissions.
+            return user.is_superuser
+        elif perm == NodePermission.view and obj is None:
+            # XXX 2018-11-20 blake_r: View permission without an obj is used
+            # for device create as a standard user. Currently there is no
+            # specific DevicePermission and no way for this code path to know
+            # its for a device. So it is represented using this path.
+            #
+            # View is only used for the create action, modifying a created
+            # device uses the appropriate `NodePermission.edit` scoped to the
+            # device being editted.
+            return True
+
         # ResourcePool permissions are handled specifically.
         if isinstance(perm, ResourcePoolPermission):
             return self._perm_resource_pool(
@@ -386,7 +400,7 @@ class MAASAuthorizationBackend(ModelBackend):
                     rbac_enabled, user, obj, visible_pools, admin_pools)
                 return obj.pool_id is not None and can_edit
             elif perm == NodePermission.admin:
-                return self._can_admin(
+                return not obj.locked and self._can_admin(
                     rbac_enabled, user, obj, admin_pools)
             else:
                 raise NotImplementedError(
@@ -501,8 +515,8 @@ class MAASAuthorizationBackend(ModelBackend):
 
         if perm == ResourcePoolPermission.edit:
             if rbac_enabled:
-                return obj.id in rbac.get_resource_pools(
-                    user.username, 'edit').values_list('id', flat=True)
+                return obj.id in rbac.get_resource_pool_ids(
+                    user.username, 'edit')
             return user.is_superuser
         elif perm == ResourcePoolPermission.view:
             if rbac_enabled:
diff --git a/src/maasserver/models/node.py b/src/maasserver/models/node.py
index 18beda4..cdbeb8a 100644
--- a/src/maasserver/models/node.py
+++ b/src/maasserver/models/node.py
@@ -485,11 +485,11 @@ class BaseNodeManager(Manager, NodeQueriesMixin):
                 "Invalid permission check (invalid permission name: %s)." %
                 perm)
         if rbac.is_enabled():
-            visible_pools = rbac.get_resource_pools(user.username, 'view')
-            admin_pools = rbac.get_resource_pools(
+            visible_pools = rbac.get_resource_pool_ids(user.username, 'view')
+            admin_pools = rbac.get_resource_pool_ids(
                 user.username, 'admin-machines')
-            condition |= Q(pool__in=admin_pools)
-            nodes = nodes.filter(pool__in=visible_pools)
+            condition |= Q(pool_id__in=admin_pools)
+            nodes = nodes.filter(pool_id__in=visible_pools)
 
         return nodes.filter(condition)
 
diff --git a/src/maasserver/models/resourcepool.py b/src/maasserver/models/resourcepool.py
index 618f218..fc7af6c 100644
--- a/src/maasserver/models/resourcepool.py
+++ b/src/maasserver/models/resourcepool.py
@@ -98,7 +98,8 @@ class ResourcePoolManager(Manager, ResourcePoolQueriesMixin):
         # Circular imports.
         from maasserver.rbac import rbac
         if rbac.is_enabled():
-            return rbac.get_resource_pools(user.username, 'view')
+            return self.filter(
+                id__in=rbac.get_resource_pool_ids(user.username, 'view'))
         return self.all()
 
 
diff --git a/src/maasserver/rbac.py b/src/maasserver/rbac.py
index ba24cf8..bff808c 100644
--- a/src/maasserver/rbac.py
+++ b/src/maasserver/rbac.py
@@ -258,26 +258,65 @@ class RBACWrapper:
         This marks a client as cleared that way only a new client is created
         if the `rbac_url` is changed.
         """
+        self.clear_cache()
         self._store.cleared = True
 
     def is_enabled(self):
         """Return whether MAAS has been configured to use RBAC."""
         return self.client is not None
 
-    def get_resource_pools(self, user, permission):
-        """Get the resource pools that given user has the given permission on.
+    def get_cache(self, resource, user, default=dict):
+        """Return the cache for the `resource` and `user`."""
+        cache = getattr(self._store, 'cache', None)
+        if cache is None:
+            cache = {}
+            setattr(self._store, 'cache', cache)
+        key = (resource, user)
+        if key in cache:
+            return cache[key]
+        scoped = default()
+        cache[key] = scoped
+        return scoped
+
+    def clear_cache(self):
+        """Clears the entire cache."""
+        if hasattr(self._store, 'cache'):
+            delattr(self._store, 'cache')
+
+    def _get_resource_pool_identifiers(self, user, permission):
+        """Get the resource pool identifiers from RBAC.
+
+        Uses the thread-local cache so only one request is made to RBAC per
+        request to MAAS.
 
         @param user: The user name of the user.
         @param permission: A permission that the user should
             have on the resource pool.
         """
-        pool_identifiers = self.client.allowed_for_user(
-            'resource-pool', user, permission)
-        pools = ResourcePool.objects.all()
-        if pool_identifiers is not ALL_RESOURCES:
+        cache = self.get_cache('resource-pool', user)
+        pool_identifiers = cache.get(permission, None)
+        if pool_identifiers is None:
+            pool_identifiers = self.client.allowed_for_user(
+                'resource-pool', user, permission)
+            cache[permission] = pool_identifiers
+        return pool_identifiers
+
+    def get_resource_pool_ids(self, user, permission):
+        """Get the resource pools ids that given user has the given
+        permission on.
+
+        @param user: The user name of the user.
+        @param permission: A permission that the user should
+            have on the resource pool.
+        """
+        pool_identifiers = self._get_resource_pool_identifiers(
+            user, permission)
+        if pool_identifiers is ALL_RESOURCES:
+            pool_ids = list(
+                ResourcePool.objects.all().values_list('id', flat=True))
+        else:
             pool_ids = [int(identifier) for identifier in pool_identifiers]
-            pools = pools.filter(id__in=pool_ids)
-        return pools
+        return pool_ids
 
     def can_create_resource_pool(self, user):
         """Return True if the `user` can create a resource pool.
@@ -287,8 +326,8 @@ class RBACWrapper:
 
         @param user: The user name of the user.
         """
-        pool_identifiers = self.client.allowed_for_user(
-            'resource-pool', user, 'edit')
+        pool_identifiers = self._get_resource_pool_identifiers(
+            user, 'edit')
         return pool_identifiers is ALL_RESOURCES
 
 
diff --git a/src/maasserver/testing/fixtures.py b/src/maasserver/testing/fixtures.py
index 747d6eb..f1cdae2 100644
--- a/src/maasserver/testing/fixtures.py
+++ b/src/maasserver/testing/fixtures.py
@@ -11,9 +11,13 @@ __all__ = [
 import inspect
 import logging
 
+from django.db import connection
 import fixtures
 from maasserver.models.config import Config
-from maasserver.rbac import rbac
+from maasserver.rbac import (
+    FakeRBACClient,
+    rbac,
+)
 from maasserver.testing.factory import factory
 
 
@@ -102,3 +106,23 @@ class RBACForceOffFixture(fixtures.Fixture):
             rbac._get_rbac_url = orig_get_url
 
         self.addCleanup(cleanup)
+
+
+class RBACEnabled(fixtures.Fixture):
+    """Fixture that enables RBAC."""
+
+    def _setUp(self):
+        # Must be called inside a transaction.
+        assert connection.in_atomic_block
+
+        Config.objects.set_config('rbac_url', 'http://rbac.example.com')
+        client = FakeRBACClient()
+        rbac._store.client = client
+        rbac._store.cleared = False
+        self.store = client.store
+
+        def cleanup():
+            rbac._store.client = None
+            rbac.clear()
+
+        self.addCleanup(cleanup)
diff --git a/src/maasserver/tests/test_auth.py b/src/maasserver/tests/test_auth.py
index e94107d..09153e9 100644
--- a/src/maasserver/tests/test_auth.py
+++ b/src/maasserver/tests/test_auth.py
@@ -234,6 +234,30 @@ class TestMAASAuthorizationBackend(MAASServerTestCase, EnableRBACMixin):
             NodePermission.admin,
             make_allocated_node()))
 
+    def test_admin_cannot_admin_locked_nodes(self):
+        backend = MAASAuthorizationBackend()
+        node = make_allocated_node()
+        node.locked = True
+        node.save()
+        self.assertFalse(backend.has_perm(
+            factory.make_admin(),
+            NodePermission.admin, node))
+
+    def test_user_cannot_admin_all_nodes(self):
+        backend = MAASAuthorizationBackend()
+        self.assertFalse(
+            backend.has_perm(factory.make_User(), NodePermission.admin))
+
+    def test_user_can_admin_all_nodes(self):
+        backend = MAASAuthorizationBackend()
+        self.assertTrue(
+            backend.has_perm(factory.make_admin(), NodePermission.admin))
+
+    def test_user_can_view_all_nodes(self):
+        backend = MAASAuthorizationBackend()
+        self.assertTrue(
+            backend.has_perm(factory.make_User(), NodePermission.view))
+
     def test_user_cannot_view_nodes_owned_by_others(self):
         backend = MAASAuthorizationBackend()
         self.assertFalse(backend.has_perm(
diff --git a/src/maasserver/tests/test_rbac.py b/src/maasserver/tests/test_rbac.py
index 721d552..f76238b 100644
--- a/src/maasserver/tests/test_rbac.py
+++ b/src/maasserver/tests/test_rbac.py
@@ -254,24 +254,24 @@ class TestRBACWrapperGetResourcePools(MAASServerTestCase):
             ResourcePool.objects.get_default_resource_pool())
         self.store.add_pool(self.default_pool)
 
-    def test_get_resource_pools_unknown_user(self):
+    def test_get_resource_pool_ids_unknown_user(self):
         self.store.add_pool(factory.make_ResourcePool())
         self.assertNotIn('user', self.store.allowed)
         self.assertEqual(
             [],
-            list(self.rbac.get_resource_pools('user', 'view')))
+            list(self.rbac.get_resource_pool_ids('user', 'view')))
 
-    def test_get_resource_pools_user_allowed_all(self):
+    def test_get_resource_pools_ids_user_allowed_all(self):
         pool1 = factory.make_ResourcePool()
         pool2 = factory.make_ResourcePool()
         self.store.add_pool(pool1)
         self.store.add_pool(pool2)
         self.store.allow('user', ALL_RESOURCES, 'view')
         self.assertCountEqual(
-            [self.default_pool, pool1, pool2],
-            self.rbac.get_resource_pools('user', 'view'))
+            [self.default_pool.id, pool1.id, pool2.id],
+            self.rbac.get_resource_pool_ids('user', 'view'))
 
-    def test_get_resource_pools_user_allowed_other_permission(self):
+    def test_get_resource_pools_ids_user_allowed_other_permission(self):
         pool1 = factory.make_ResourcePool()
         pool2 = factory.make_ResourcePool()
         self.store.add_pool(pool1)
@@ -279,21 +279,44 @@ class TestRBACWrapperGetResourcePools(MAASServerTestCase):
         self.store.allow('user', pool1, 'view')
         self.store.allow('user', pool2, 'edit')
         self.assertCountEqual(
-            [pool1],
-            self.rbac.get_resource_pools('user', 'view'))
+            [pool1.id],
+            self.rbac.get_resource_pool_ids('user', 'view'))
         self.assertCountEqual(
             [],
-            self.rbac.get_resource_pools('user', 'admin-machines'))
+            self.rbac.get_resource_pool_ids('user', 'admin-machines'))
 
-    def test_get_resource_pools_user_allowed_some(self):
+    def test_get_resource_pool_ids_user_allowed_some(self):
         pool1 = factory.make_ResourcePool()
         pool2 = factory.make_ResourcePool()
         self.store.add_pool(pool1)
         self.store.add_pool(pool2)
         self.store.allow('user', pool1, 'view')
         self.assertEqual(
-            sorted([pool1]),
-            sorted(self.rbac.get_resource_pools('user', 'view')))
+            sorted([pool1.id]),
+            sorted(self.rbac.get_resource_pool_ids('user', 'view')))
+
+    def test_get_resource_pool_ids_one_request_per_clear_cache(self):
+        self.store.allow('user', self.default_pool, 'view')
+        pools_one = self.rbac.get_resource_pool_ids('user', 'view')
+        new_pool = factory.make_ResourcePool()
+        self.store.allow('user', new_pool, 'view')
+        pools_two = self.rbac.get_resource_pool_ids('user', 'view')
+        self.rbac.clear_cache()
+        pools_three = self.rbac.get_resource_pool_ids('user', 'view')
+        self.assertItemsEqual([self.default_pool.id], pools_one)
+        self.assertItemsEqual([self.default_pool.id], pools_two)
+        self.assertItemsEqual([self.default_pool.id, new_pool.id], pools_three)
+
+    def test_get_resource_pool_ids_ALL_RESOURCES_always_returns_all(self):
+        self.store.allow('user', ALL_RESOURCES, 'view')
+        pools_one = self.rbac.get_resource_pool_ids('user', 'view')
+        new_pool = factory.make_ResourcePool()
+        pools_two = self.rbac.get_resource_pool_ids('user', 'view')
+        self.rbac.clear_cache()
+        pools_three = self.rbac.get_resource_pool_ids('user', 'view')
+        self.assertItemsEqual([self.default_pool.id], pools_one)
+        self.assertItemsEqual([self.default_pool.id, new_pool.id], pools_two)
+        self.assertItemsEqual([self.default_pool.id, new_pool.id], pools_three)
 
     def test_can_create_resource_pool_returns_True(self):
         self.store.allow('user', ALL_RESOURCES, 'edit')