scim: Upgrade django-scim2; remove request.user monkey patching.

Signed-off-by: Anders Kaseorg <anders@zulip.com>

requirements/dev.txt

@@ -489,8 +489,9 @@ django-phonenumber-field==6.4.0 \
     --hash=sha256:72a3e7a3e7493bf2a12c07a3bc77ce89813acc16592bf04d0eee3b5a452097ed \
     --hash=sha256:a31b4f05ac0ff898661516c84940f83adb5cdcf0ae4b9b1d31bb8ad3ff345b58
     # via django-two-factor-auth
-django-scim2==0.17.3 \
-    --hash=sha256:6fcff9389e3a6bad1599f8873a5fbf4e0101400cd62beac4d7bf053bd4944447
+django-scim2==0.18.0 \
+    --hash=sha256:5055099fdbfa55b46488cece7b378225263265ae4acd1669c47b2c286d2cfbb2 \
+    --hash=sha256:f3353df68b469f494a5c7f53bb487411125a08be77d2ccc2d4c048138895614c
     # via -r requirements/common.in
 django-sendfile2==0.7.0 \
     --hash=sha256:0ee17b4f7ce8cc7159f75fa4e5d62e7795c1217de8f1e52ee6265d4aa46dce03 \
@@ -1608,7 +1609,6 @@ python-dateutil==2.8.2 \
     #   -r requirements/common.in
     #   arrow
     #   botocore
-    #   django-scim2
     #   moto
 python-debian==0.1.48 \
     --hash=sha256:00edb2c95a9e28e17067c055d17a66e90abf9d7c80cfda93095e7193e5ae58c3 \

requirements/prod.txt

@@ -331,8 +331,9 @@ django-phonenumber-field==6.4.0 \
     --hash=sha256:72a3e7a3e7493bf2a12c07a3bc77ce89813acc16592bf04d0eee3b5a452097ed \
     --hash=sha256:a31b4f05ac0ff898661516c84940f83adb5cdcf0ae4b9b1d31bb8ad3ff345b58
     # via django-two-factor-auth
-django-scim2==0.17.3 \
-    --hash=sha256:6fcff9389e3a6bad1599f8873a5fbf4e0101400cd62beac4d7bf053bd4944447
+django-scim2==0.18.0 \
+    --hash=sha256:5055099fdbfa55b46488cece7b378225263265ae4acd1669c47b2c286d2cfbb2 \
+    --hash=sha256:f3353df68b469f494a5c7f53bb487411125a08be77d2ccc2d4c048138895614c
     # via -r requirements/common.in
 django-sendfile2==0.7.0 \
     --hash=sha256:0ee17b4f7ce8cc7159f75fa4e5d62e7795c1217de8f1e52ee6265d4aa46dce03 \
@@ -1102,7 +1103,6 @@ python-dateutil==2.8.2 \
     # via
     #   -r requirements/common.in
     #   botocore
-    #   django-scim2
 python-gcm==0.4 \
     --hash=sha256:511c35fc5ae829f7fc3cbdb45c4ec3fda02f85e4fae039864efe82682ccb9c18
     # via -r requirements/common.in

version.py

@@ -48,4 +48,4 @@ API_FEATURE_LEVEL = 154
 #   historical commits sharing the same major version, in which case a
 #   minor version bump suffices.
 
-PROVISION_VERSION = (210, 0)
+PROVISION_VERSION = (211, 0)

zerver/middleware.py

@@ -696,29 +696,18 @@ class ZulipCommonMiddleware(CommonMiddleware):
         return super().should_redirect_with_slash(request)
 
 
-class SCIMClient:
-    @property
-    def is_authenticated(self) -> bool:
-        """
-        The purpose of this is to make SCIMClient behave like a UserProfile
-        when an instance is assigned to request.user - we need it to pass
-        request.user.is_authenticated verifications.
-        """
-        return True
-
-
-def validate_scim_bearer_token(request: HttpRequest) -> Optional[SCIMClient]:
+def validate_scim_bearer_token(request: HttpRequest) -> bool:
     """
     This function verifies the request is allowed to make SCIM requests on this subdomain,
     by checking the provided bearer token and ensuring it matches a scim client configured
     for this subdomain in settings.SCIM_CONFIG.
-    If successful, returns the corresponding SCIMClient object. Returns None otherwise.
+    Returns True if successful.
     """
 
     subdomain = get_subdomain(request)
     scim_config_dict = settings.SCIM_CONFIG.get(subdomain)
     if not scim_config_dict:
-        return None
+        return False
 
     valid_bearer_token = scim_config_dict.get("bearer_token")
     scim_client_name = scim_config_dict.get("scim_client_name")
@@ -728,7 +717,7 @@ def validate_scim_bearer_token(request: HttpRequest) -> Optional[SCIMClient]:
     assert scim_client_name
 
     if request.headers.get("Authorization") != f"Bearer {valid_bearer_token}":
-        return None
+        return False
 
     request_notes = RequestNotes.get_notes(request)
     assert request_notes.realm is not None
@@ -736,10 +725,7 @@ def validate_scim_bearer_token(request: HttpRequest) -> Optional[SCIMClient]:
         f"scim-client:{scim_client_name}:realm:{request_notes.realm.id}"
     )
 
-    # While API authentication code paths are sufficiently high
-    # traffic that we prefer to use a cache, SCIM is much lower
-    # traffic, and doing a database query is plenty fast.
-    return SCIMClient()
+    return True
 
 
 class ZulipSCIMAuthCheckMiddleware(SCIMAuthCheckMiddleware):
@@ -747,9 +733,17 @@ class ZulipSCIMAuthCheckMiddleware(SCIMAuthCheckMiddleware):
     Overridden version of middleware implemented in django-scim2
     (https://github.com/15five/django-scim2/blob/master/src/django_scim/middleware.py)
     to also handle authenticating the client.
+
+    This doesn't actually function as a regular middleware class that's registered in
+    settings.MIDDLEWARE, but rather is called inside django-scim2 logic to authenticate
+    the request when accessing SCIM endpoints.
     """
 
     def process_request(self, request: HttpRequest) -> Optional[HttpResponse]:
+        # Defensive assertion to ensure this can't accidentally get called on a request
+        # to a non-SCIM endpoint.
+        assert request.path.startswith(self.reverse_url)
+
         # This determines whether this is a SCIM request based on the request's path
         # and if it is, logs request information, including the body, as well as the response
         # for debugging purposes to the `django_scim.middleware` logger, at DEBUG level.
@@ -757,24 +751,12 @@ class ZulipSCIMAuthCheckMiddleware(SCIMAuthCheckMiddleware):
         if self.should_log_request(request):
             self.log_request(request)
 
-        # Here we verify the request is indeed to a SCIM endpoint. That's ensured
-        # by comparing the path with self.reverse_url, which is the root SCIM path /scim/b2/.
-        # Of course we don't want to proceed with authenticating the request for SCIM
-        # if a non-SCIM endpoint is being queried.
-        if not request.path.startswith(self.reverse_url):
-            return None
-
-        scim_client = validate_scim_bearer_token(request)
-        if not scim_client:
+        if not validate_scim_bearer_token(request):
+            # In case of failed authentication, a response should be returned to
+            # prevent going further down the codepath (to the SCIM endpoint), since
+            # this aspect works like regular middleware.
             response = HttpResponse(status=401)
             response["WWW-Authenticate"] = scim_settings.WWW_AUTHENTICATE_HEADER
             return response
 
-        # The client has been successfully authenticated for SCIM on this subdomain,
-        # so we can assign the corresponding SCIMClient object to request.user - which
-        # will allow this request to pass request.user.is_authenticated checks from now on,
-        # to be served by the relevant views implemented in django-scim2.
-        # Since request.user must be a UserProfile or AnonymousUser, this is a type-unsafe
-        # workaround to make this monkey-patching work.
-        request.user = scim_client  # type: ignore[assignment] # wrong type
         return None

zproject/computed_settings.py

@@ -176,7 +176,6 @@ MIDDLEWARE = (
     "zerver.middleware.LocaleMiddleware",
     "zerver.middleware.HostDomainMiddleware",
     "django.middleware.csrf.CsrfViewMiddleware",
-    "zerver.middleware.ZulipSCIMAuthCheckMiddleware",
     # Make sure 2FA middlewares come after authentication middleware.
     "django_otp.middleware.OTPMiddleware",  # Required by two factor auth.
     "two_factor.middleware.threadlocals.ThreadLocals",  # Required by Twilio
@@ -1235,6 +1234,7 @@ SCIM_SERVICE_PROVIDER = {
     "SCHEME": EXTERNAL_URI_SCHEME,
     "GET_EXTRA_MODEL_FILTER_KWARGS_GETTER": "zerver.lib.scim.get_extra_model_filter_kwargs_getter",
     "BASE_LOCATION_GETTER": "zerver.lib.scim.base_scim_location_getter",
+    "AUTH_CHECK_MIDDLEWARE": "zerver.middleware.ZulipSCIMAuthCheckMiddleware",
     "AUTHENTICATION_SCHEMES": [
         {
             "type": "bearer",