Merge pull request #106 from maykinmedia/rework/clean-up-caching-and-…

…middleware

Rework/clean up caching and middleware

CHANGELOG.rst

@@ -21,7 +21,11 @@ aspects could not be avoided.
   if you were subclassing this backend to override these attributes.
 
   You can provide these in your custom configuration model(s) as the
-  ``oidcdb_sensitive_claims`` and ``oidcdb_username_claim`` model fields or properties. See the implementation of the ``OpenIDConnectConfigBase`` model for more details.
+  ``oidcdb_sensitive_claims`` and ``oidcdb_username_claim`` model fields or properties.
+  See the implementation of the ``OpenIDConnectConfigBase`` model for more details.
+
+* ``mozilla_django_oidc_db.models.CachingMixin`` is removed. Our base model overrides the
+  generated cache key so that it uniquely points to a specific Django model.
 
 0.16.0 (2024-05-02)
 ===================

docs/quickstart.rst

@@ -76,8 +76,9 @@ Furthermore, ensure the following settings are configured:
 
     OIDC_AUTHENTICATE_CLASS = "mozilla_django_oidc_db.views.OIDCAuthenticationRequestView"
     OIDC_CALLBACK_CLASS = "mozilla_django_oidc_db.views.OIDCCallbackView"
-    MOZILLA_DJANGO_OIDC_DB_CACHE = "oidc"
-    MOZILLA_DJANGO_OIDC_DB_CACHE_TIMEOUT = 1
+    # Optionally, configure django-solo caching
+    # SOLO_CACHE = "solo"
+    # SOLO_CACHE_TIMEOUT = 1
 
 In order to properly catch admin login errors, add the following to urlpatterns:
 
@@ -91,15 +92,19 @@ In order to properly catch admin login errors, add the following to urlpatterns:
         ...
     ]
 
-``MOZILLA_DJANGO_OIDC_DB_CACHE`` is used to cache the configuration that is stored in the database,
-to prevent a lot of database lookups. Ensure this cache is configured in ``CACHES`` (using the backend of choice):
+You can optionally enable the configuration cache (prevent database lookups) by enabling
+the django-solo cache: ``SOLO_CACHE = "solo"``. Ensure this cache is configured in
+``CACHES`` (using the backend of choice):
 
 .. code-block:: python
 
     CACHES = {
         "default": {"BACKEND": "django.core.cache.backends.locmem.LocMemCache"},
         ...
-        "oidc": {"BACKEND": "django.core.cache.backends.locmem.LocMemCache"},
+        "solo": {
+            "BACKEND": "django.core.cache.backends.redis.RedisCache",
+            "LOCATION": "redis://127.0.0.1:6379",
+        },
     }
 
 Add the urlpatterns:

mozilla_django_oidc_db/middleware.py

@@ -1,22 +1,75 @@
-from mozilla_django_oidc.middleware import SessionRefresh as _SessionRefresh
+from typing import Any, ClassVar, Generic, TypeVar, cast
 
-from .mixins import GetAttributeMixin, SoloConfigMixin
+from django.http import HttpRequest
 
+from mozilla_django_oidc.middleware import SessionRefresh as BaseSessionRefresh
+from typing_extensions import override
+
+from .config import dynamic_setting, get_setting_from_config
+from .models import OpenIDConnectConfig, OpenIDConnectConfigBase
+
+T = TypeVar("T", bound=OpenIDConnectConfigBase)
+
+
+class BaseRefreshMiddleware(Generic[T], BaseSessionRefresh):
+    """
+    Point the middleware to a particular config class to use.
+
+    This base class sets up the dynamic settings mechanism.
+    """
+
+    _config: T
+    config_class: ClassVar[type[OpenIDConnectConfigBase]]
+    """
+    The config model/class to get the endpoints/credentials from.
+    """
+
+    OIDC_EXEMPT_URLS = dynamic_setting[list[str]](default=[])
+    OIDC_OP_AUTHORIZATION_ENDPOINT = dynamic_setting[str]()
+    OIDC_RP_CLIENT_ID = dynamic_setting[str]()
+    OIDC_STATE_SIZE = dynamic_setting[int](default=32)
+    OIDC_AUTHENTICATION_CALLBACK_URL = dynamic_setting[str](
+        default="oidc_authentication_callback"
+    )
+    OIDC_RP_SCOPES = dynamic_setting[str](default="openid email")
+    OIDC_USE_NONCE = dynamic_setting[bool](default=True)
+    OIDC_NONCE_SIZE = dynamic_setting[int](default=32)
 
-class SessionRefresh(GetAttributeMixin, SoloConfigMixin, _SessionRefresh):
     def __init__(self, get_response):
-        # `super().__init__` is not called here, because this attempts to initialize
-        # the settings (which should be retrieved from `OpenIDConnectConfig`).
+        # `super().__init__` is not called here, because it calls self.get_setting()
+        # The retrieval of these settings is handled via dynamic settings above.
+        super(BaseSessionRefresh, self).__init__(get_response=get_response)
 
-        # The retrieval of these settings has been moved to runtime (`__getattribute__` from the `GetAttributeMixin`)
-        super(_SessionRefresh, self).__init__(get_response=get_response)
+    @override
+    def get_settings(self, attr: str, *args: Any) -> Any:  # type: ignore
+        """
+        Look up the request setting from the database config.
 
-    def process_request(self, request):
-        # Initialize to retrieve the settings from config model
-        super().__init__(self.get_response)
+        We cache the resolved config instance on the middleware instance for when
+        settings are repeatedly looked up. Note however, that we also override the
+        __call__ method to delete this cached property, as a middleware instance lives
+        for the entire lifecycle of the applicatation, and each request must not look
+        at stale cached configuration.
+        """
+        if (config := getattr(self, "_config", None)) is None:
+            # django-solo and type checking is challenging, but a new release is on the
+            # way and should fix that :fingers_crossed:
+            config = cast(T, self.config_class.get_solo())
+            self._config = config
+        return get_setting_from_config(config, attr, *args)
 
-        self.refresh_config()
-        if not self.config.enabled:
-            return
+    def __call__(self, request: HttpRequest):
+        # reset the python-level cache for each request
+        if hasattr(self, "_config"):
+            del self._config
+        return super().__call__(request)
 
+    def process_request(self, request):
+        # do nothing if the configuration is not enabled
+        if not self.get_settings("ENABLED"):
+            return None
         return super().process_request(request)
+
+
+class SessionRefresh(BaseRefreshMiddleware[OpenIDConnectConfig]):
+    config_class = OpenIDConnectConfig

mozilla_django_oidc_db/migrations/0002_migrate_to_claim_field.py

@@ -6,15 +6,10 @@
 
 import mozilla_django_oidc_db.fields
 import mozilla_django_oidc_db.models
-import mozilla_django_oidc_db.settings as oidc_settings
 
 
 def flush_cache():
-    cache_name = getattr(
-        settings,
-        "MOZILLA_DJANGO_OIDC_DB_CACHE",
-        oidc_settings.MOZILLA_DJANGO_OIDC_DB_CACHE,
-    )
+    cache_name = getattr(settings, "MOZILLA_DJANGO_OIDC_DB_CACHE", "oidc")
     if not cache_name:
         return
     caches[cache_name].clear()

mozilla_django_oidc_db/models.py

@@ -9,13 +9,11 @@
 from django.core.exceptions import FieldDoesNotExist, ValidationError
 from django.db import models
 from django.utils.encoding import force_str
-from django.utils.functional import classproperty
 from django.utils.translation import gettext_lazy as _
 
 from django_jsonform.models.fields import ArrayField
-from solo.models import SingletonModel, get_cache
-
-import mozilla_django_oidc_db.settings as oidc_settings
+from solo import settings as solo_settings
+from solo.models import SingletonModel
 
 from .fields import ClaimField
 from .typing import ClaimPath, DjangoView
@@ -51,62 +49,6 @@ def get_default_groups_claim() -> list[str]:
     return ["roles"]
 
 
-class CachingMixin:
-    @classmethod
-    def clear_cache(cls):
-        cache_name = getattr(
-            settings, "OIDC_CACHE", oidc_settings.MOZILLA_DJANGO_OIDC_DB_CACHE
-        )
-        if cache_name:
-            cache = get_cache(cache_name)
-            cache_key = cls.get_cache_key()
-            cache.delete(cache_key)
-
-    def set_to_cache(self):
-        cache_name = getattr(
-            settings,
-            "MOZILLA_DJANGO_OIDC_DB_CACHE",
-            oidc_settings.MOZILLA_DJANGO_OIDC_DB_CACHE,
-        )
-        if not cache_name:
-            return None
-        cache = get_cache(cache_name)
-        cache_key = self.get_cache_key()
-        timeout = getattr(
-            settings,
-            "MOZILLA_DJANGO_OIDC_DB_CACHE_TIMEOUT",
-            oidc_settings.MOZILLA_DJANGO_OIDC_DB_CACHE_TIMEOUT,
-        )
-        cache.set(cache_key, self, timeout)
-
-    @classmethod
-    def get_cache_key(cls) -> str:
-        prefix = cls.custom_oidc_db_prefix or getattr(
-            settings,
-            "MOZILLA_DJANGO_OIDC_DB_PREFIX",
-            oidc_settings.MOZILLA_DJANGO_OIDC_DB_PREFIX,
-        )
-        return "%s:%s" % (prefix, cls.__name__.lower())
-
-    @classmethod
-    def get_solo(cls) -> SingletonModel:
-        cache_name = getattr(
-            settings,
-            "MOZILLA_DJANGO_OIDC_DB_CACHE",
-            oidc_settings.MOZILLA_DJANGO_OIDC_DB_CACHE,
-        )
-        if not cache_name:
-            obj, created = cls.objects.get_or_create(pk=cls.singleton_instance_id)
-            return obj
-        cache = get_cache(cache_name)
-        cache_key = cls.get_cache_key()
-        obj = cache.get(cache_key)
-        if not obj:
-            obj, created = cls.objects.get_or_create(pk=cls.singleton_instance_id)
-            obj.set_to_cache()
-        return obj
-
-
 class OpenIDConnectConfigBase(SingletonModel):
     """
     Defines the required fields for a config to establish an OIDC connection
@@ -251,6 +193,17 @@ class Meta:
     def __str__(self) -> str:
         return force_str(self._meta.verbose_name)
 
+    @classmethod
+    def get_cache_key(cls):
+        """
+        Overridden cache key to take into account the app label.
+        """
+        solo_prefix = getattr(
+            settings, "SOLO_CACHE_PREFIX", solo_settings.SOLO_CACHE_PREFIX
+        )
+        prefix: str = getattr(settings, "MOZILLA_DJANGO_OIDC_DB_PREFIX", solo_prefix)
+        return ":".join([prefix, cls._meta.app_label, str(cls._meta.model_name)])
+
     @property
     def oidc_rp_scopes(self) -> str:
         """
@@ -287,7 +240,7 @@ def get_callback_view(self) -> DjangoView:
         return default_callback_view
 
 
-class OpenIDConnectConfig(CachingMixin, OpenIDConnectConfigBase):
+class OpenIDConnectConfig(OpenIDConnectConfigBase):
     """
     Configuration for authentication/authorization via OpenID connect
     """
@@ -387,13 +340,6 @@ def clean(self):
                 }
             )
 
-    @classproperty
-    def custom_oidc_db_prefix(cls) -> str:
-        """
-        Cache prefix that can be overridden
-        """
-        return oidc_settings.MOZILLA_DJANGO_OIDC_DB_PREFIX
-
     @property
     def oidcdb_username_claim(self) -> ClaimPath:
         """

testapp/settings.py

@@ -72,7 +72,6 @@
     "mozilla_django_oidc_db.backends.OIDCAuthenticationBackend",
 ]
 
-MOZILLA_DJANGO_OIDC_DB_PREFIX = "default"
 # These settings are evaluated at import-time of the urlconf in mozilla_django_oidc.urls.
 # Changing them via @override_settings (or the pytest-django settings fixture) has no
 # effect.