From 55342efd33cc482daecdf9d0f560b4f32122540f Mon Sep 17 00:00:00 2001 From: Anders Kaseorg Date: Fri, 4 Nov 2022 16:26:39 -0700 Subject: [PATCH] scim: Upgrade django-scim2; remove request.user monkey patching. Signed-off-by: Anders Kaseorg --- requirements/dev.txt | 6 ++-- requirements/prod.txt | 6 ++-- version.py | 2 +- zerver/middleware.py | 52 ++++++++++++----------------------- zproject/computed_settings.py | 2 +- 5 files changed, 25 insertions(+), 43 deletions(-) diff --git a/requirements/dev.txt b/requirements/dev.txt index 3b83cc1887..4743b28383 100644 --- a/requirements/dev.txt +++ b/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 \ diff --git a/requirements/prod.txt b/requirements/prod.txt index 169bc292b7..a1d6470220 100644 --- a/requirements/prod.txt +++ b/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 diff --git a/version.py b/version.py index 0f2b2f4ea4..a6dbf51ce5 100644 --- a/version.py +++ b/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) diff --git a/zerver/middleware.py b/zerver/middleware.py index 9670f2068b..e029dbb197 100644 --- a/zerver/middleware.py +++ b/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 diff --git a/zproject/computed_settings.py b/zproject/computed_settings.py index f49618e966..f8b5528d97 100644 --- a/zproject/computed_settings.py +++ b/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",