From a1fa2a8cf502f93bdead1f12fbde34fb12499342 Mon Sep 17 00:00:00 2001 From: Mateusz Mandera Date: Fri, 4 Feb 2022 19:57:11 +0100 Subject: [PATCH] scim: Upgrade to django-scim2 0.17.1. The new release adds the commit: https://github.com/15five/django-scim2/commit/20ac22b96d1496f2cb117be6067ab862fa38960d Which allows us to get rid of the entire ugly override that was needed to do this commit's job in our code. What we do here in this commit: * Use django-scim2 0.17.1 * Revert the relevant parts of f5a65846a8296dc697639e97ba34815e7cc2376d * Adjust the expected error message in test_exception_details_not_revealed_to_client since the message thrown by django-scim2 in this release is slightly different. We do not have to add anything to set EXPOSE_SCIM_EXCEPTIONS, since django-scim2 uses False as the default, which is what we want - and we have the aforementioned test verifying that indeed information doesn't get revealed to the SCIM client. --- requirements/dev.txt | 4 +-- requirements/prod.txt | 4 +-- version.py | 2 +- zerver/lib/scim.py | 63 +-------------------------------------- zerver/tests/test_scim.py | 2 +- zproject/urls.py | 33 +++++++------------- 6 files changed, 18 insertions(+), 90 deletions(-) diff --git a/requirements/dev.txt b/requirements/dev.txt index 9a9f7bb7de..24c1c8308f 100644 --- a/requirements/dev.txt +++ b/requirements/dev.txt @@ -432,8 +432,8 @@ django-phonenumber-field==6.0.0 \ --hash=sha256:9695d3beda772c503ad4e04a4f7012a8227e9e3e4fd0ea4ffb07c43245bf4a8d \ --hash=sha256:bbb9cb2e6fc53c476de40428e1354c313a040e8b2fb69ea8ead4ba41a60f926a # via django-two-factor-auth -django-scim2==0.17.0 \ - --hash=sha256:dc3cb3c0d5b6ebf4ae8a28dd1dac1e0658fb543c8c178e72e3c19975816da092 +django-scim2==0.17.1 \ + --hash=sha256:346e9b3e9bff6aab59e533c735b9892bcc52d06ed042772b4d48fcb494c2e22a # via -r requirements/common.in django-sendfile2==0.6.1 \ --hash=sha256:312b4501960e6b3a3390c48a6bdcfdae2c0516efacf24bdd0c97c6f2f2d2fc30 \ diff --git a/requirements/prod.txt b/requirements/prod.txt index 688426abc5..d40871239f 100644 --- a/requirements/prod.txt +++ b/requirements/prod.txt @@ -289,8 +289,8 @@ django-phonenumber-field==6.0.0 \ --hash=sha256:9695d3beda772c503ad4e04a4f7012a8227e9e3e4fd0ea4ffb07c43245bf4a8d \ --hash=sha256:bbb9cb2e6fc53c476de40428e1354c313a040e8b2fb69ea8ead4ba41a60f926a # via django-two-factor-auth -django-scim2==0.17.0 \ - --hash=sha256:dc3cb3c0d5b6ebf4ae8a28dd1dac1e0658fb543c8c178e72e3c19975816da092 +django-scim2==0.17.1 \ + --hash=sha256:346e9b3e9bff6aab59e533c735b9892bcc52d06ed042772b4d48fcb494c2e22a # via -r requirements/common.in django-sendfile2==0.6.1 \ --hash=sha256:312b4501960e6b3a3390c48a6bdcfdae2c0516efacf24bdd0c97c6f2f2d2fc30 \ diff --git a/version.py b/version.py index 7cd446a79c..98f35779af 100644 --- a/version.py +++ b/version.py @@ -48,4 +48,4 @@ API_FEATURE_LEVEL = 115 # historical commits sharing the same major version, in which case a # minor version bump suffices. -PROVISION_VERSION = "173.4" +PROVISION_VERSION = "174.0" diff --git a/zerver/lib/scim.py b/zerver/lib/scim.py index f2e356b7e3..d259a8cc93 100644 --- a/zerver/lib/scim.py +++ b/zerver/lib/scim.py @@ -2,17 +2,11 @@ from typing import Any, Callable, Dict, List, Optional, Type, Union import django_scim.constants as scim_constants import django_scim.exceptions as scim_exceptions -import orjson from django.conf import settings -from django.contrib.auth.decorators import login_required from django.core.exceptions import ValidationError from django.db import models, transaction -from django.http import HttpRequest, HttpResponse -from django.utils.decorators import method_decorator -from django.views.decorators.csrf import csrf_exempt +from django.http import HttpRequest from django_scim.adapters import SCIMUser -from django_scim.views import SCIMView, SearchView, UserSearchView, UsersView -from django_scim.views import logger as scim_views_logger from scim2_filter_parser.attr_paths import AttrPath from zerver.lib.actions import ( @@ -368,58 +362,3 @@ class ConflictError(scim_exceptions.IntegrityError): """ scim_type = "uniqueness" - - -class ZulipSCIMViewMixin(SCIMView): - """ - Default django-scim2 behavior is to convert any exception that occurs while processing - the request within the view code to a string and put it - in the HttpResponse. We don't want that due to the risk of leaking sensitive information - through the error message. - - The way we implement this override is by having this mixin override the main dispatch() - method - and then all the specific view classes are re-defined to inherit from this mixin - and the original django-scim2 class. This means that we have to also re-register all - the URL patterns so that our View classes are used. - """ - - @method_decorator(csrf_exempt) - @method_decorator(login_required) - def dispatch(self, request: HttpRequest, *args: Any, **kwargs: Any) -> HttpResponse: - """ - This method through which all SCIM views are processed needs to be forked - to change its logic of how exceptions are handled. - """ - if not self.implemented: - return self.status_501(request, *args, **kwargs) - - try: - return super(SCIMView, self).dispatch(request, *args, **kwargs) - except Exception as e: - if not isinstance(e, scim_exceptions.SCIMException): - # This is where we adjust the exception-handling behavior. Instead of - # putting str(e) in the response, we use a generic error that won't leak - # information. - scim_views_logger.exception("Unable to complete SCIM call.") - e = scim_exceptions.SCIMException("Exception while processing SCIM request.") - - content = orjson.dumps(e.to_dict()) - return HttpResponse( - content=content, content_type=scim_constants.SCIM_CONTENT_TYPE, status=e.status - ) - - -class ZulipSCIMView(ZulipSCIMViewMixin, SCIMView): - pass - - -class ZulipSCIMUsersView(ZulipSCIMViewMixin, UsersView): - pass - - -class ZulipSCIMSearchView(ZulipSCIMViewMixin, SearchView): - pass - - -class ZulipSCIMUserSearchView(ZulipSCIMViewMixin, UserSearchView): - pass diff --git a/zerver/tests/test_scim.py b/zerver/tests/test_scim.py index 81f0d9f0bc..727f922b9a 100644 --- a/zerver/tests/test_scim.py +++ b/zerver/tests/test_scim.py @@ -101,7 +101,7 @@ class TestExceptionDetailsNotRevealedToClient(SCIMTestCase): result.json(), { "schemas": ["urn:ietf:params:scim:api:messages:2.0:Error"], - "detail": "Exception while processing SCIM request.", + "detail": "Exception occurred while processing the SCIM request", "status": 500, }, ) diff --git a/zproject/urls.py b/zproject/urls.py index 8d55164bd3..2740878e55 100644 --- a/zproject/urls.py +++ b/zproject/urls.py @@ -754,45 +754,34 @@ urls += [path("saml/metadata.xml", saml_sp_metadata)] # SCIM2 -from zerver.lib.scim import ( - ZulipSCIMSearchView, - ZulipSCIMUserSearchView, - ZulipSCIMUsersView, - ZulipSCIMView, -) +from django_scim import views as scim_views urls += [ - # We have to register all the SCIM URL patterns first, because we override - # all the SCIM View classes and we need Django to use them instead of - # the django-scim2 Views that the app will register. - re_path(r"^scim/v2/$", ZulipSCIMView.as_view(implemented=False)), - re_path(r"^scim/v2/.search$", ZulipSCIMSearchView.as_view(implemented=False)), - re_path(r"^scim/v2/Users/.search$", ZulipSCIMUserSearchView.as_view()), - re_path(r"^scim/v2/Users(?:/(?P[^/]+))?$", ZulipSCIMUsersView.as_view()), # Everything below here are features that we don't yet support and we want # to explicitly mark them to return "Not Implemented" rather than running # the django-scim2 code for them. re_path( r"^scim/v2/Groups/.search$", - ZulipSCIMView.as_view(implemented=False), + scim_views.SCIMView.as_view(implemented=False), ), re_path( r"^scim/v2/Groups(?:/(?P[^/]+))?$", - ZulipSCIMView.as_view(implemented=False), + scim_views.SCIMView.as_view(implemented=False), ), - re_path(r"^scim/v2/Me$", ZulipSCIMView.as_view(implemented=False)), + re_path(r"^scim/v2/Me$", scim_views.SCIMView.as_view(implemented=False)), re_path( r"^scim/v2/ServiceProviderConfig$", - ZulipSCIMView.as_view(implemented=False), + scim_views.SCIMView.as_view(implemented=False), ), re_path( r"^scim/v2/ResourceTypes(?:/(?P[^/]+))?$", - ZulipSCIMView.as_view(implemented=False), + scim_views.SCIMView.as_view(implemented=False), ), - re_path(r"^scim/v2/Schemas(?:/(?P[^/]+))?$", ZulipSCIMView.as_view(implemented=False)), - re_path(r"^scim/v2/Bulk$", ZulipSCIMView.as_view(implemented=False)), - # At the end we still register the django-scim2 url patterns (even though we override them all above) - # so that reverse("scim:viewname") still works like the internal library code expects. + re_path( + r"^scim/v2/Schemas(?:/(?P[^/]+))?$", scim_views.SCIMView.as_view(implemented=False) + ), + re_path(r"^scim/v2/Bulk$", scim_views.SCIMView.as_view(implemented=False)), + # This registers the remaining SCIM endpoints. path("scim/v2/", include("django_scim.urls", namespace="scim")), ]