🏷️ [#43] Restructure classes/types to increase type safety

Fixing one pyright error at a time...

The protocols are replaced with abstract base classes,
as that is effectively how they are used. This will
also improve the type checking situation for downstream
projects.

Author: sergei-maertens

mozilla_django_oidc_db/backends.py

@@ -1,7 +1,7 @@
 from __future__ import annotations
 
 import logging
-from typing import Any, cast, override
+from typing import Any, override
 
 from django.contrib.auth import get_user_model
 from django.contrib.auth.models import (
@@ -19,10 +19,7 @@
 from .exceptions import MissingInitialisation
 from .jwt import verify_and_decode_token
 from .models import OIDCClient, UserInformationClaimsSources
-from .plugins import (
-    AbstractUserOIDCPluginProtocol,
-    AnonymousUserOIDCPluginProtocol,
-)
+from .plugins import AbstractUserOIDCPlugin, AnonymousUserOIDCPlugin
 from .registry import register as registry
 from .typing import JSONObject
 from .utils import extract_content_type
@@ -75,10 +72,12 @@ def __init__(self, *args, **kwargs) -> None:
 
         # django-stubs returns AbstractBaseUser, but we depend on properties of
         # AbstractUser.
-        self.UserModel = cast(AbstractUser, get_user_model())
+        UserModel = get_user_model()
+        assert issubclass(UserModel, AbstractUser)
+        self.UserModel = UserModel
 
     @override
-    def get_settings(self, attr: str, *args: Any) -> Any:  # type: ignore
+    def get_settings(self, attr: str, *args: Any) -> Any:  # pyright: ignore[reportIncompatibleMethodOverride]
         """
         Override the upstream library get_settings.
 
@@ -106,7 +105,7 @@ def _check_candidate_backend(self) -> bool:
     # `OIDCAuthenticationCallbackView` for the `auth.authenticate(**kwargs)` call if this
     # needs updating.
     @override
-    def authenticate(  # type: ignore
+    def authenticate(  # pyright: ignore[reportIncompatibleMethodOverride]
         self,
         request: HttpRequest | None,
         nonce: str | None = None,
@@ -138,7 +137,7 @@ def authenticate(  # type: ignore
         # Store the config class identifier on the user, so that we can store this in the user's
         # session after they have been successfully authenticated (by listening to the `user_logged_in` signal)
         if user:
-            user._oidcdb_config_identifier = self.config.identifier  # type: ignore
+            user._oidcdb_config_identifier = self.config.identifier  # pyright: ignore[reportAttributeAccessIssue]
 
         return user
 
@@ -149,7 +148,7 @@ def verify_claims(self, claims: JSONObject) -> bool:
         assert self.config
 
         plugin = registry[self.config.identifier]
-        assert isinstance(plugin, AbstractUserOIDCPluginProtocol)
+        assert isinstance(plugin, AbstractUserOIDCPlugin)
         return plugin.verify_claims(claims)
 
     @override
@@ -214,11 +213,11 @@ def get_userinfo(
     @override
     def filter_users_by_claims(
         self, claims: JSONObject
-    ) -> models.Manager[AbstractUser]:  # type: ignore (parent function returns UserManager which is more specific than Manager)
+    ) -> models.QuerySet[AbstractUser]:
         assert self.config
         plugin = registry[self.config.identifier]
 
-        assert isinstance(plugin, AbstractUserOIDCPluginProtocol)
+        assert isinstance(plugin, AbstractUserOIDCPlugin)
         return plugin.filter_users_by_claims(claims)
 
     @override
@@ -227,30 +226,35 @@ def create_user(self, claims: JSONObject) -> AbstractUser:
         assert self.config
         plugin = registry[self.config.identifier]
 
-        assert isinstance(plugin, AbstractUserOIDCPluginProtocol)
+        assert isinstance(plugin, AbstractUserOIDCPlugin)
         return plugin.create_user(claims)
 
     @override
     def update_user(self, user: AbstractUser, claims: JSONObject):
         assert self.config
         plugin = registry[self.config.identifier]
 
-        assert isinstance(plugin, AbstractUserOIDCPluginProtocol)
+        assert isinstance(plugin, AbstractUserOIDCPlugin)
         return plugin.update_user(user, claims)
 
     @override
     def get_or_create_user(
         self, access_token: str, id_token: str, payload: JSONObject
-    ) -> AnonymousUser | AbstractUser | None:  # type: ignore (parent function returns only an AbstractUser | None)
+    ) -> AnonymousUser | AbstractUser | None:
         """Get or create a user based on the tokens received."""
         assert self.config
 
         plugin = registry[self.config.identifier]
         assert isinstance(self.request, HttpRequest)
 
-        if isinstance(plugin, AnonymousUserOIDCPluginProtocol):
+        # shortcut for "anonymous users" where the OIDC authentication *does* happen,
+        # but no actual Django user instance is created.
+        if isinstance(plugin, AnonymousUserOIDCPlugin):
             return plugin.get_or_create_user(
                 access_token, id_token, payload, self.request
             )
 
-        return super().get_or_create_user(access_token, id_token, payload)
+        user = super().get_or_create_user(access_token, id_token, payload)
+        if user is not None:
+            assert isinstance(user, self.UserModel)
+        return user

mozilla_django_oidc_db/config.py

@@ -6,7 +6,7 @@
 settings that are still defined in the django settings layer.
 """
 
-from typing import Any, Generic, Protocol, Self, TypeVar, Unpack, overload
+from typing import Any, Protocol, Self, TypeVar, Unpack, overload
 
 from django.core.exceptions import (
     BadRequest,
@@ -70,7 +70,7 @@ class SettingsHolder(Protocol[T]):
     def get_settings(self, attr: str, *args: T) -> T: ...
 
 
-class DynamicSettingKwargs(TypedDict, Generic[T], total=False):
+class DynamicSettingKwargs[T](TypedDict, total=False):
     default: T
 
 

mozilla_django_oidc_db/forms.py

@@ -56,15 +56,15 @@ def get_endpoints_from_discovery(cls, base_url: str):
         return endpoints
 
     def clean(self):
-        cleaned_data = super().clean()
+        super().clean()
 
-        discovery_endpoint = cleaned_data.get("oidc_op_discovery_endpoint")
+        discovery_endpoint = self.cleaned_data.get("oidc_op_discovery_endpoint")
 
         # Derive the endpoints from the discovery endpoint
         if discovery_endpoint:
             try:
                 endpoints = self.get_endpoints_from_discovery(discovery_endpoint)
-                cleaned_data.update(**endpoints)
+                self.cleaned_data.update(**endpoints)
             except (
                 requests.exceptions.RequestException,
                 json.decoder.JSONDecodeError,
@@ -80,7 +80,7 @@ def clean(self):
             # Verify that the required endpoints were derived from the
             # discovery endpoint
             for field in self.required_endpoints:
-                if not cleaned_data.get(field):
+                if not self.cleaned_data.get(field):
                     self.add_error(field, _("This field is required."))
 
-        return cleaned_data
+        return self.cleaned_data

mozilla_django_oidc_db/middleware.py

@@ -67,8 +67,11 @@ def process_request(self, request):
 
         return super().process_request(request)
 
+    # we can't use cached_property, because a middleware instance exists for the whole
+    # duration of the django server life cycle, and the relevant config can change
+    # between requests. See ``process_request``.
     @property
-    def exempt_urls(self):
+    def exempt_urls(self):  # pyright: ignore[reportIncompatibleVariableOverride]
         # In many cases, the OIDC_AUTHENTICATION_CALLBACK_URL will be the generic
         # callback handler and already be part of super().exempt_urls. However, this is
         # not a given, and consumers might have implemented different callback handlers,