Merge ~blake-rouse/maas:rbac-thread-local into maas:master

[Blake Rouse]

Use a thread-local to keep an RBACClient per request being handler per thread.

This simplifies the API for using RBAC in MAAS as well as its simple as asking rbac.is_enabled() and that will handle all the thread-local transparently.

[Blake Rouse (blake-rouse)]

This builds off of - https://code.launchpad.net/~ack/maas/+git/maas/+merge/357754

[MAAS Lander (maas-lander)]

UNIT TESTS
-b rbac-thread-local lp:~blake-rouse/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: aff3337df3e14f1b482a0147284516958189f06f

review: Approve

[Newell Jensen (newell-jensen)]

LGTM.

review: Approve

[MAAS Lander (maas-lander)]

LANDING
-b rbac-thread-local lp:~blake-rouse/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED MERGE
LOG: http://maas-ci-jenkins.internal:8080/job/maas/job/branch-lander/310520/console

[MAAS Lander (maas-lander)]

UNIT TESTS
-b rbac-thread-local lp:~blake-rouse/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: 9d695f5b6d2af876b16f456934ea02bb16682805

review: Approve

[MAAS Lander (maas-lander)]

LANDING
-b rbac-thread-local lp:~blake-rouse/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED MERGE
LOG: http://maas-ci-jenkins.internal:8080/job/maas/job/branch-lander/310850/console

[MAAS Lander (maas-lander)]

LANDING
-b rbac-thread-local lp:~blake-rouse/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED MERGE
LOG: http://maas-ci-jenkins.internal:8080/job/maas/job/branch-lander/310854/console

diff --git a/src/maasserver/djangosettings/settings.py b/src/maasserver/djangosettings/settings.py
index d49fe96..1587e74 100644
--- a/src/maasserver/djangosettings/settings.py
+++ b/src/maasserver/djangosettings/settings.py
@@ -237,6 +237,9 @@ MIDDLEWARE = (
     # Used for rendering and logging exceptions.
     'maasserver.middleware.ExceptionMiddleware',
 
+    # Used to clear the RBAC thread-local cache.
+    'maasserver.middleware.RBACMiddleware',
+
     # Handle errors that should really be handled in application code:
     # NoConnectionsAvailable, PowerActionAlreadyInProgress, TimeoutError.
     # FIXME.
diff --git a/src/maasserver/middleware.py b/src/maasserver/middleware.py
index 1de6072..22a6012 100644
--- a/src/maasserver/middleware.py
+++ b/src/maasserver/middleware.py
@@ -48,6 +48,7 @@ from maasserver.enum import COMPONENT
 from maasserver.exceptions import MAASAPIException
 from maasserver.models.config import Config
 from maasserver.models.node import RackController
+from maasserver.rbac import rbac
 from maasserver.rpc import getAllClients
 from maasserver.utils.django_urls import reverse
 from maasserver.utils.orm import is_retryable_failure
@@ -491,3 +492,23 @@ class ExternalAuthInfoMiddleware:
                 domain=auth_domain, admin_group=auth_admin_group)
         request.external_auth_info = auth_info
         return self.get_response(request)
+
+
+class RBACMiddleware:
+    """Middleware that cleans the RBAC thread-local cache.
+
+
+    At the end of each request the RBAC client that is held in the thread-local
+    needs to be cleaned up. That way the next request on the same thread will
+    use a new RBAC client.
+    """
+
+    def __init__(self, get_response):
+        self.get_response = get_response
+
+    def __call__(self, request):
+        result = self.get_response(request)
+        # Now that the response has been handled, clear the thread-local
+        # state of the RBAC connection.
+        rbac.clear()
+        return result
diff --git a/src/maasserver/models/node.py b/src/maasserver/models/node.py
index 05da4cf..8c57169 100644
--- a/src/maasserver/models/node.py
+++ b/src/maasserver/models/node.py
@@ -456,7 +456,7 @@ class BaseNodeManager(Manager, NodeQueriesMixin):
             nodes that `user` is allowed to access.
         """
         # Local import to avoid circular imports.
-        from maasserver.rbac import RBAC
+        from maasserver.rbac import rbac
         # If the data is corrupt, this can get called with None for
         # user where a Node should have an owner but doesn't.
         # Nonetheless, the code should not crash with corrupt data.
@@ -484,7 +484,6 @@ class BaseNodeManager(Manager, NodeQueriesMixin):
             raise NotImplementedError(
                 "Invalid permission check (invalid permission name: %s)." %
                 perm)
-        rbac = RBAC()
         if rbac.is_enabled():
             visible_pools = rbac.get_resource_pools(
                 user.username, NODE_PERMISSION.VIEW)
diff --git a/src/maasserver/models/tests/test_node.py b/src/maasserver/models/tests/test_node.py
index b41cd70..e7b418c 100644
--- a/src/maasserver/models/tests/test_node.py
+++ b/src/maasserver/models/tests/test_node.py
@@ -39,7 +39,6 @@ from fixtures import LoggerFixture
 from maasserver import (
     bootresources,
     preseed as preseed_module,
-    rbac,
     server_address,
 )
 from maasserver.clusterrpc import boot_images
@@ -125,6 +124,10 @@ from maasserver.node_status import (
     NODE_TRANSITIONS,
 )
 from maasserver.preseed import CURTIN_INSTALL_LOG
+from maasserver.rbac import (
+    FakeRBACClient,
+    rbac,
+)
 from maasserver.rpc.testing.fixtures import MockLiveRegionToClusterRPCFixture
 import maasserver.server_address as server_address_module
 from maasserver.storage_layouts import (
@@ -5304,10 +5307,10 @@ class NodeManagerGetNodesRBACTest(MAASServerTestCase):
     def setUp(self):
         super().setUp()
         Config.objects.set_config('rbac_url', 'http://rbac.example.com')
-        self.client = rbac.FakeRBACClient()
+        self.client = FakeRBACClient()
+        rbac._store.client = self.client
+        rbac._store.cleared = False  # Prevent re-creation of the client.
         self.store = self.client.store
-        self.mocked_rbacclient = self.patch(rbac, 'RBACClient')
-        self.mocked_rbacclient.return_value = self.client
 
     def make_ResourcePool(self, *args, **kwargs):
         """Create a resource pool and register it with RBAC."""
diff --git a/src/maasserver/rbac.py b/src/maasserver/rbac.py
index 8274f81..21c54bf 100644
--- a/src/maasserver/rbac.py
+++ b/src/maasserver/rbac.py
@@ -1,5 +1,6 @@
 from collections import defaultdict
 from functools import partial
+import threading
 from typing import (
     Sequence,
     Union,
@@ -71,7 +72,7 @@ class RBACClient(MacaroonClient):
             self, resource_type: str,
             updates: Sequence[Resource]=None,
             removals: Sequence[int]=None,
-            last_sync_id: int=None):
+            last_sync_id: str=None):
         """Put all the resources for `resource_type`.
 
         This replaces all the resources for `resource_type`.
@@ -157,8 +158,11 @@ class FakeRBACClient(RBACClient):
     server.
     """
 
-    def __init__(self):
-        self._url = Config.objects.get_config('rbac_url')
+    def __init__(self, url: str=None, auth_info: AuthInfo=None):
+        if url is None:
+            url = Config.objects.get_config('rbac_url')
+        self._url = url
+        self._auth_info = auth_info
         self.store = FakeResourceStore()
 
     def _request(self, method, url):
@@ -182,14 +186,51 @@ NODE_PERMISSION_TO_RBAC = {
 }
 
 
-class RBAC:
+class RBACWrapper:
     """Object for querying RBAC information."""
 
-    def __init__(self, client=None):
-        url = Config.objects.get_config('rbac_url')
-        if client is None and url:
-            client = RBACClient(url)
-        self.client = client
+    def __init__(self, client_class=None):
+        # A client is created per thread.
+        self._store = threading.local()
+        self._client_class = client_class
+        if self._client_class is None:
+            self._client_class = RBACClient
+
+    @property
+    def client(self):
+        """Get thread-local client."""
+        # Get the current cleared status and reset it to False for the
+        # next request on this thread.
+        cleared = getattr(self._store, 'cleared', False)
+        self._store.cleared = False
+
+        client = getattr(self._store, 'client', None)
+        if client is None:
+            url = Config.objects.get_config('rbac_url')
+            if url:
+                client = self._client_class(url)
+                self._store.client = client
+        elif cleared:
+            # Check that the `rbac_url` and the credentials match.
+            url = Config.objects.get_config('rbac_url')
+            if not url:
+                # RBAC is now disabled.
+                delattr(self._store, 'client')
+                return None
+            auth_info = get_auth_info()
+            if (client._url != url or client._auth_info != auth_info):
+                # URL or creds differ, re-create the client.
+                client = self._client_class(url, auth_info)
+                self._store.client = client
+        return client
+
+    def clear(self):
+        """Clear the current client.
+
+        This marks a client as cleared that way only a new client is created
+        if the `rbac_url` is changed.
+        """
+        self._store.cleared = True
 
     def is_enabled(self):
         """Return whether MAAS has been configured to use RBAC."""
@@ -209,3 +250,6 @@ class RBAC:
             pool_ids = [int(identifier) for identifier in pool_identifiers]
             pools = pools.filter(id__in=pool_ids)
         return pools
+
+
+rbac = RBACWrapper()
diff --git a/src/maasserver/testing/fixtures.py b/src/maasserver/testing/fixtures.py
index 307b6d7..89cbcc1 100644
--- a/src/maasserver/testing/fixtures.py
+++ b/src/maasserver/testing/fixtures.py
@@ -13,6 +13,7 @@ import logging
 
 import fixtures
 from maasserver.models.config import Config
+from maasserver.rbac import rbac
 from maasserver.testing.factory import factory
 
 
@@ -77,3 +78,10 @@ class LogSQL(fixtures.Fixture):
         if self.include_stacktrace:
             log.removeFilter(self._addedFilter)
         self.removeHandler(self._setHandler)
+
+
+class RBACClearFixture(fixtures.Fixture):
+    """Fixture that clears the RBAC thread-local cache between tests."""
+
+    def _setUp(self):
+        self.addCleanup(rbac.clear)
diff --git a/src/maasserver/testing/testcase.py b/src/maasserver/testing/testcase.py
index 931d04e..bb6c802 100644
--- a/src/maasserver/testing/testcase.py
+++ b/src/maasserver/testing/testcase.py
@@ -30,6 +30,7 @@ from maasserver.fields import register_mac_type
 from maasserver.testing.fixtures import (
     IntroCompletedFixture,
     PackageRepositoryFixture,
+    RBACClearFixture,
 )
 from maasserver.testing.orm import PostCommitHooksTestMixin
 from maasserver.testing.resources import DjangoDatabasesManager
@@ -86,6 +87,9 @@ class MAASRegionTestCaseBase(PostCommitHooksTestMixin):
         # Avoid circular imports.
         from maasserver.models import signals
 
+        # Always clear the RBAC thread-local between tests.
+        self.useFixture(RBACClearFixture())
+
         # XXX: allenap bug=1427628 2015-03-03: This should not be here.
         self.useFixture(IntroCompletedFixture())
 
diff --git a/src/maasserver/tests/test_middleware.py b/src/maasserver/tests/test_middleware.py
index b01914a..04bd540 100644
--- a/src/maasserver/tests/test_middleware.py
+++ b/src/maasserver/tests/test_middleware.py
@@ -41,9 +41,11 @@ from maasserver.middleware import (
     ExternalAuthInfoMiddleware,
     ExternalComponentsMiddleware,
     is_public_path,
+    RBACMiddleware,
     RPCErrorsMiddleware,
 )
 from maasserver.models.config import Config
+from maasserver.rbac import rbac
 from maasserver.testing import extract_redirect
 from maasserver.testing.factory import factory
 from maasserver.testing.testcase import MAASServerTestCase
@@ -688,3 +690,21 @@ class TestExternalAuthInfoMiddleware(MAASServerTestCase):
         self.assertEqual(request.external_auth_info.type, 'candid')
         self.assertEqual(
             request.external_auth_info.url, 'https://example.com')
+
+
+class RBACMiddlewareTest(MAASServerTestCase):
+    """Tests for the RBACMiddleware."""
+
+    def process_request(self, request):
+
+        def get_response(request):
+            return None
+
+        middleware = RBACMiddleware(get_response)
+        return middleware(request)
+
+    def test_calls_rbac_clear(self):
+        mock_clear = self.patch(rbac, 'clear')
+        request = factory.make_fake_request(factory.make_string(), 'GET')
+        self.process_request(request)
+        self.assertThat(mock_clear, MockCalledOnceWith())
diff --git a/src/maasserver/tests/test_rbac.py b/src/maasserver/tests/test_rbac.py
index 9867580..cd5ebf1 100644
--- a/src/maasserver/tests/test_rbac.py
+++ b/src/maasserver/tests/test_rbac.py
@@ -1,5 +1,8 @@
+from queue import Queue
+from threading import Thread
 from unittest import mock
 
+from django.db import transaction
 from maasserver.enum import NODE_PERMISSION
 from maasserver.models import (
     Config,
@@ -8,12 +11,16 @@ from maasserver.models import (
 from maasserver.rbac import (
     ALL_RESOURCES,
     FakeRBACClient,
-    RBAC,
+    rbac,
     RBACClient,
+    RBACWrapper,
     Resource,
 )
 from maasserver.testing.factory import factory
-from maasserver.testing.testcase import MAASServerTestCase
+from maasserver.testing.testcase import (
+    MAASServerTestCase,
+    MAASTransactionServerTestCase,
+)
 from maastesting.matchers import MockCalledOnceWith
 from macaroonbakery.bakery import PrivateKey
 from macaroonbakery.httpbakery.agent import (
@@ -186,7 +193,7 @@ class TestRBACClient(MAASServerTestCase):
                 auth=mock.ANY, cookies=mock.ANY, json=None))
 
 
-class TestRBACIsEnabled(MAASServerTestCase):
+class TestRBACWrapperIsEnabled(MAASServerTestCase):
 
     def setUp(self):
         super().setUp()
@@ -198,31 +205,31 @@ class TestRBACIsEnabled(MAASServerTestCase):
     def test_local_disabled(self):
         Config.objects.set_config('external_auth_url', '')
         Config.objects.set_config('rbac_url', '')
-        rbac = RBAC()
+        rbac = RBACWrapper()
         self.assertFalse(rbac.is_enabled())
 
     def test_candid_disabled(self):
         Config.objects.set_config(
             'external_auth_url', 'http://candid.example.com')
         Config.objects.set_config('rbac_url', '')
-        rbac = RBAC()
+        rbac = RBACWrapper()
         self.assertFalse(rbac.is_enabled())
 
     def test_rbac_enabled(self):
         Config.objects.set_config('external_auth_url', '')
         Config.objects.set_config('rbac_url', 'http://rbac.example.com')
-        rbac = RBAC()
+        rbac = RBACWrapper()
         self.assertTrue(rbac.is_enabled())
 
 
-class TestRBACGetResourcePools(MAASServerTestCase):
+class TestRBACWrapperGetResourcePools(MAASServerTestCase):
 
     def setUp(self):
         super().setUp()
         Config.objects.set_config('rbac_url', 'http://rbac.example.com')
-        self.client = FakeRBACClient()
+        self.rbac = RBACWrapper(client_class=FakeRBACClient)
+        self.client = self.rbac.client
         self.store = self.client.store
-        self.rbac = RBAC(client=self.client)
         self.default_pool = (
             ResourcePool.objects.get_default_resource_pool())
         self.store.add_pool(self.default_pool)
@@ -267,3 +274,76 @@ class TestRBACGetResourcePools(MAASServerTestCase):
         self.assertEqual(
             sorted([pool1]),
             sorted(self.rbac.get_resource_pools('user', NODE_PERMISSION.VIEW)))
+
+
+class TestRBACWrapperClient(MAASServerTestCase):
+
+    def setUp(self):
+        super().setUp()
+        Config.objects.set_config('rbac_url', 'http://rbac.example.com')
+        Config.objects.set_config(
+            'external_auth_url', 'http://candid.example.com')
+        Config.objects.set_config('external_auth_user', 'user@candid')
+        Config.objects.set_config(
+            'external_auth_key',
+            'x0NeASLPFhOFfq3Q9M0joMveI4HjGwEuJ9dtX/HTSRY=')
+
+    def test_same_client(self):
+        self.assertIs(rbac.client, rbac.client)
+
+    def test_clear_same_url_same_client(self):
+        rbac1 = rbac.client
+        rbac.clear()
+        self.assertIs(rbac1, rbac.client)
+
+    def test_clear_new_url_creates_new_client(self):
+        rbac1 = rbac.client
+        rbac.clear()
+        Config.objects.set_config('rbac_url', 'http://rbac-other.example.com')
+        self.assertIsNot(rbac1, rbac.client)
+
+    def test_clear_new_auth_url_creates_new_client(self):
+        rbac1 = rbac.client
+        rbac.clear()
+        Config.objects.set_config(
+            'external_auth_url', 'http://candid-other.example.com')
+        self.assertIsNot(rbac1, rbac.client)
+
+
+class TestRBACWrapperClientThreads(MAASTransactionServerTestCase):
+
+    def test_different_clients_per_threads(self):
+
+        # Commit the settings to the database so the created threads have
+        # access to the same data. Each thread will start its own transaction
+        # so the settings must be committed.
+        #
+        # Since actually data is committed into the database the
+        # `MAASTransactionServerTestCase` is used to reset the database to
+        # a clean state after this test.
+        with transaction.atomic():
+            Config.objects.set_config('rbac_url', 'http://rbac.example.com')
+            Config.objects.set_config(
+                'external_auth_url', 'http://candid.example.com')
+            Config.objects.set_config('external_auth_user', 'user@candid')
+            Config.objects.set_config(
+                'external_auth_key',
+                'x0NeASLPFhOFfq3Q9M0joMveI4HjGwEuJ9dtX/HTSRY=')
+
+        queue = Queue()
+
+        def target():
+            queue.put(rbac.client)
+
+        thread1 = Thread(target=target)
+        thread1.start()
+        thread2 = Thread(target=target)
+        thread2.start()
+
+        rbac1 = queue.get()
+        queue.task_done()
+        rbac2 = queue.get()
+        queue.task_done()
+        thread1.join()
+        thread2.join()
+        self.assertIsNot(rbac1, rbac2)