From 73a6f2a1a7fbfb939b8bdaded4148f9dcf8a6e81 Mon Sep 17 00:00:00 2001 From: Mateusz Mandera Date: Fri, 10 Sep 2021 18:36:56 +0200 Subject: [PATCH] auth: Add support for using SCIM for account management. --- pyproject.toml | 2 + requirements/common.in | 3 + requirements/dev.txt | 12 + requirements/prod.txt | 12 + version.py | 2 +- zerver/lib/export.py | 3 + zerver/lib/scim.py | 363 +++++++++++ zerver/lib/scim_filter.py | 62 ++ zerver/lib/test_classes.py | 12 + zerver/lib/test_helpers.py | 6 + zerver/management/commands/add_scim_client.py | 20 + zerver/middleware.py | 72 ++- zerver/migrations/0367_scimclient.py | 38 ++ zerver/models.py | 23 + zerver/tests/test_scim.py | 610 ++++++++++++++++++ zproject/computed_settings.py | 33 + zproject/dev_settings.py | 8 + zproject/test_extra_settings.py | 8 + zproject/urls.py | 20 +- 19 files changed, 1306 insertions(+), 3 deletions(-) create mode 100644 zerver/lib/scim.py create mode 100644 zerver/lib/scim_filter.py create mode 100644 zerver/management/commands/add_scim_client.py create mode 100644 zerver/migrations/0367_scimclient.py create mode 100644 zerver/tests/test_scim.py diff --git a/pyproject.toml b/pyproject.toml index 035d03facc..ad359256d7 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -63,6 +63,7 @@ module = [ "django_auth_ldap.*", "django_cte.*", "django_otp.*", + "django_scim.*", "django_sendfile.*", "django_statsd.*", "DNS.*", @@ -84,6 +85,7 @@ module = [ "pyuca.*", "re2.*", "requests_oauthlib.*", + "scim2_filter_parser.attr_paths", "scrapy.*", "social_core.*", "social_django.*", diff --git a/requirements/common.in b/requirements/common.in index 87242166e9..138d733476 100644 --- a/requirements/common.in +++ b/requirements/common.in @@ -189,3 +189,6 @@ google-re2 # For querying recursive group membership django-cte + +# SCIM integration +django-scim2 diff --git a/requirements/dev.txt b/requirements/dev.txt index 59cd28f087..1040057dac 100644 --- a/requirements/dev.txt +++ b/requirements/dev.txt @@ -377,8 +377,10 @@ django[argon2]==3.2.7 \ # django-formtools # django-otp # django-phonenumber-field + # django-scim2 # django-sendfile2 # django-two-factor-auth + # scim2-filter-parser django-auth-ldap==3.0.0 \ --hash=sha256:19ee19034f344d9efd07ed88d3187e256ec33ae39d6a47222083b89f7d35c5f6 \ --hash=sha256:1f2d5c562d9ba9a5e9a64099ae9798e1a63840a11afe4d1c4a9c74121f066eaa @@ -406,6 +408,9 @@ django-phonenumber-field==5.2.0 \ --hash=sha256:52b2e5970133ec5ab701218b802f7ab237229854dc95fd239b7e9e77dc43731d \ --hash=sha256:5547fb2b2cc690a306ba77a5038419afc8fa8298a486fb7895008e9067cc7e75 # via django-two-factor-auth +django-scim2==0.17.0 \ + --hash=sha256:dc3cb3c0d5b6ebf4ae8a28dd1dac1e0658fb543c8c178e72e3c19975816da092 + # via -r requirements/common.in django-sendfile2==0.6.0 \ --hash=sha256:7f850040ddc29c9c42192ed85b915465a3ed7cced916c4fafdd5eda057dd06ec # via -r requirements/common.in @@ -1344,6 +1349,7 @@ python-dateutil==2.8.2 \ # via # -r requirements/common.in # botocore + # django-scim2 # moto python-debian==0.1.40 \ --hash=sha256:385dfb965eca75164d256486c7cf9bae772d24144249fd18b9d15d3cffb70eea \ @@ -1558,6 +1564,9 @@ s3transfer==0.5.0 \ --hash=sha256:50ed823e1dc5868ad40c8dc92072f757aa0e653a192845c94a3b676f4a62da4c \ --hash=sha256:9c1dc369814391a6bda20ebbf4b70a0f34630592c9aa520856bf384916af2803 # via boto3 +scim2-filter-parser==0.3.5 \ + --hash=sha256:f46b6ffa01cdad6011d3d991bd167af1a9822ab917c225bdf49bc7a44ad4ae53 + # via django-scim2 https://github.com/scrapy/scrapy/archive/c5b1ee810167266fcd259f263dbfc0fe0204761a.zip#egg=Scrapy==2.5.0+git \ --hash=sha256:d12f88f2cfb31e487170ee4e68f6e59a2af100ee690add873831c368fac6e0a7 # via -r requirements/dev.in @@ -1599,6 +1608,9 @@ six==1.16.0 \ # talon-core # traitlets # w3lib +sly==0.3 \ + --hash=sha256:be6a3825b042a9e1b6f5730fc747e6d983c917f0f002d798d0b9f86ca5c05ad9 + # via scim2-filter-parser snakeviz==2.1.0 \ --hash=sha256:8ce375b18ae4a749516d7e6c6fbbf8be6177c53974f53534d8eadb646cd279b1 \ --hash=sha256:92ad876fb6a201a7e23a6b85ea96d9643a51e285667c253a8653643804f7cb68 diff --git a/requirements/prod.txt b/requirements/prod.txt index 69faa3663e..c655d8d5d8 100644 --- a/requirements/prod.txt +++ b/requirements/prod.txt @@ -231,8 +231,10 @@ django[argon2]==3.2.7 \ # django-formtools # django-otp # django-phonenumber-field + # django-scim2 # django-sendfile2 # django-two-factor-auth + # scim2-filter-parser django-auth-ldap==3.0.0 \ --hash=sha256:19ee19034f344d9efd07ed88d3187e256ec33ae39d6a47222083b89f7d35c5f6 \ --hash=sha256:1f2d5c562d9ba9a5e9a64099ae9798e1a63840a11afe4d1c4a9c74121f066eaa @@ -260,6 +262,9 @@ django-phonenumber-field==5.2.0 \ --hash=sha256:52b2e5970133ec5ab701218b802f7ab237229854dc95fd239b7e9e77dc43731d \ --hash=sha256:5547fb2b2cc690a306ba77a5038419afc8fa8298a486fb7895008e9067cc7e75 # via django-two-factor-auth +django-scim2==0.17.0 \ + --hash=sha256:dc3cb3c0d5b6ebf4ae8a28dd1dac1e0658fb543c8c178e72e3c19975816da092 + # via -r requirements/common.in django-sendfile2==0.6.0 \ --hash=sha256:7f850040ddc29c9c42192ed85b915465a3ed7cced916c4fafdd5eda057dd06ec # via -r requirements/common.in @@ -909,6 +914,7 @@ 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 @@ -1059,6 +1065,9 @@ s3transfer==0.5.0 \ --hash=sha256:50ed823e1dc5868ad40c8dc92072f757aa0e653a192845c94a3b676f4a62da4c \ --hash=sha256:9c1dc369814391a6bda20ebbf4b70a0f34630592c9aa520856bf384916af2803 # via boto3 +scim2-filter-parser==0.3.5 \ + --hash=sha256:f46b6ffa01cdad6011d3d991bd167af1a9822ab917c225bdf49bc7a44ad4ae53 + # via django-scim2 sentry-sdk==1.3.1 \ --hash=sha256:ebe99144fa9618d4b0e7617e7929b75acd905d258c3c779edcd34c0adfffe26c \ --hash=sha256:f33d34c886d0ba24c75ea8885a8b3a172358853c7cbde05979fc99c29ef7bc52 @@ -1081,6 +1090,9 @@ six==1.16.0 \ # qrcode # talon-core # traitlets +sly==0.3 \ + --hash=sha256:be6a3825b042a9e1b6f5730fc747e6d983c917f0f002d798d0b9f86ca5c05ad9 + # via scim2-filter-parser social-auth-app-django==5.0.0 \ --hash=sha256:52241a25445a010ab1c108bafff21fc5522d5c8cd0d48a92c39c7371824b065d \ --hash=sha256:b6e3132ce087cdd6e1707aeb1b588be41d318408fcf6395435da0bc6fe9a9795 diff --git a/version.py b/version.py index b12b083d62..1cb2d4da1a 100644 --- a/version.py +++ b/version.py @@ -48,4 +48,4 @@ API_FEATURE_LEVEL = 105 # historical commits sharing the same major version, in which case a # minor version bump suffices. -PROVISION_VERSION = "162.2" +PROVISION_VERSION = "162.3" diff --git a/zerver/lib/export.py b/zerver/lib/export.py index 7e12baf987..6e77317397 100644 --- a/zerver/lib/export.py +++ b/zerver/lib/export.py @@ -150,6 +150,7 @@ ALL_ZULIP_TABLES = { "zerver_scheduledemail_users", "zerver_scheduledmessage", "zerver_scheduledmessagenotificationemail", + "zerver_scimclient", "zerver_service", "zerver_stream", "zerver_submessage", @@ -199,6 +200,8 @@ NON_EXPORTED_TABLES = { "zerver_scheduledemail", "zerver_scheduledemail_users", "zerver_scheduledmessage", + # SCIMClient should be manually created for the new realm after importing. + "zerver_scimclient", # These tables are related to a user's 2FA authentication # configuration, which will need to be set up again on the new # server. diff --git a/zerver/lib/scim.py b/zerver/lib/scim.py new file mode 100644 index 0000000000..50e7cf5637 --- /dev/null +++ b/zerver/lib/scim.py @@ -0,0 +1,363 @@ +from typing import Any, Callable, Dict, List, Optional, Type, Union + +import django_scim.constants as scim_constants +import django_scim.exceptions as scim_exceptions +from django.conf import settings +from django.core.exceptions import ValidationError +from django.db import models, transaction +from django.http import HttpRequest +from django_scim.adapters import SCIMUser +from scim2_filter_parser.attr_paths import AttrPath + +from zerver.lib.actions import ( + check_change_full_name, + do_change_user_delivery_email, + do_create_user, + do_deactivate_user, + do_reactivate_user, +) +from zerver.lib.email_validation import email_allowed_for_realm, validate_email_not_already_in_realm +from zerver.lib.request import RequestNotes +from zerver.lib.subdomains import get_subdomain +from zerver.models import ( + DisposableEmailError, + DomainNotAllowedForRealmError, + EmailContainsPlusError, + UserProfile, +) + + +class ZulipSCIMUser(SCIMUser): + """With django-scim2, the core of a project's SCIM implementation is + this user adapter class, which defines how to translate between the + concepts of users in the SCIM specification and the Zulip users. + """ + + id_field = "id" + + def __init__(self, obj: UserProfile, request: Optional[HttpRequest] = None) -> None: + # We keep the function signature from the superclass, but this actually + # shouldn't be called with request being None. + assert request is not None + + # self.obj is populated appropriately by django-scim2 views with + # an instance of UserProfile - either fetched from the database + # or constructed via UserProfile() if the request currently being + # handled is a User creation request (POST). + self.obj: UserProfile + + super().__init__(obj, request) + self.subdomain = get_subdomain(request) + self.config = settings.SCIM_CONFIG[self.subdomain] + + # These attributes are custom to this class and will be + # populated with values in handle_replace and similar methods + # in response to a request for the the corresponding + # UserProfile fields to change. The .save() method inspects + # these fields an executes the requested changes. + self._email_new_value: Optional[str] = None + self._is_active_new_value: Optional[bool] = None + self._full_name_new_value: Optional[str] = None + self._password_set_to: Optional[str] = None + + def is_new_user(self) -> bool: + return not bool(self.obj.id) + + @property + def display_name(self) -> str: + """ + Return the displayName of the user per the SCIM spec. + + Overridden because UserProfile uses the .full_name attribute, + while the superclass expects .first_name and .last_name. + """ + return self.obj.full_name + + def to_dict(self) -> Dict[str, Any]: + """ + Return a ``dict`` conforming to the SCIM User Schema, + ready for conversion to a JSON object. + + The attribute names appearing in the dict are those defined in the SCIM User Schema: + https://datatracker.ietf.org/doc/html/rfc7643#section-4.1 + """ + if self.config["name_formatted_included"]: + name = { + "formatted": self.obj.full_name, + } + else: + # Some clients (e.g. Okta) operate with a first_name, + # last_name model and don't support a full name field. + # While we strive never to do this in the project because + # not every culture has the first/last name structure, + # Okta's design means we have to convert our full_name + # into a first_name/last_name pair to provide to the + # client. We do naive conversion with `split`. + if " " in self.obj.full_name: + first_name, last_name = self.obj.full_name.split(" ", 1) + else: + first_name, last_name = self.obj.full_name, "" + name = { + "givenName": first_name, + "familyName": last_name, + } + d = dict( + { + "schemas": [scim_constants.SchemaURI.USER], + "id": self.obj.id, + "userName": self.obj.delivery_email, + "name": name, + "displayName": self.display_name, + "active": self.obj.is_active, + # meta is a property implemented in the superclass + # TODO: The upstream implementation uses `user_profile.date_joined` + # as the value of the lastModified meta attribute, which is not + # a correct simplification. We should add proper tracking + # of this value. + "meta": self.meta, + } + ) + + return d + + def from_dict(self, d: Dict[str, Any]) -> None: + """Consume a dictionary conforming to the SCIM User Schema. The + dictionary was originally submitted as JSON by the client in + PUT (update a user) and POST (create a new user) requests. A + PUT request tells us to update User attributes to match those + passed in the dict. A POST request tells us to create a new + User with attributes as specified in the dict. + + The superclass implements some very basic default behavior, + that doesn't support changing attributes via our actions.py + functions (which update audit logs, send events, etc.) or + doing application-specific validation. + + Thus, we've completely overriden the upstream implementation + to store the values of the supported attributes that the + request would like to change. Actually modifying the database + is implemented in self.save(). + + Given that SCIMUser is an adapter class, this method is meant + to be completely overridden, and we can expect it remain the + case that no important django-scim2 logic relies on the + superclass's implementation of this function. + """ + email = d.get("userName") + assert isinstance(email, str) + self.change_delivery_email(email) + + name_attr_dict = d.get("name", {}) + if self.config["name_formatted_included"]: + full_name = name_attr_dict.get("formatted", "") + else: + # Some providers (e.g. Okta) don't provide name.formatted. + first_name = name_attr_dict.get("givenName", "") + last_name = name_attr_dict.get("familyName", "") + full_name = f"{first_name} {last_name}".strip() + + if full_name: + assert isinstance(full_name, str) + self.change_full_name(full_name) + + if self.is_new_user() and not full_name: + raise scim_exceptions.BadRequestError( + "Must specify name.formatted, name.givenName or name.familyName when creating a new user" + ) + + active = d.get("active") + if self.is_new_user() and not active: + raise scim_exceptions.BadRequestError("New user must have active=True") + + if active is not None: + assert isinstance(active, bool) + self.change_is_active(active) + + def change_delivery_email(self, new_value: str) -> None: + # Note that the email_allowed_for_realm check that usually + # appears adjacent to validate_email is present in save(). + self.validate_email(new_value) + if self.obj.delivery_email != new_value: + self._email_new_value = new_value + + def change_full_name(self, new_value: str) -> None: + if new_value and self.obj.full_name != new_value: + self._full_name_new_value = new_value + + def change_is_active(self, new_value: bool) -> None: + if new_value is not None and new_value != self.obj.is_active: + self._is_active_new_value = new_value + + def handle_replace( + self, + path: Optional[AttrPath], + value: Union[str, List[object], Dict[AttrPath, object]], + operation: Any, + ) -> None: + """ + PATCH requests specify a list of operations of types "add", "remove", "replace". + So far we only implement "replace" as that should be sufficient. + + This method is forked from the superclass and is called to handle "replace" + PATCH operations. Such an operation tells us to change the values + of a User's attributes as specified. The superclass implements a very basic + behavior in this method and is meant to be overriden, since this is an adapter class. + """ + if not isinstance(value, dict): + # Restructure for use in loop below. Taken from the + # overriden upstream method. + assert path is not None + value = {path: value} + + assert isinstance(value, dict) + for path, val in (value or {}).items(): + if path.first_path == ("userName", None, None): + assert isinstance(val, str) + self.change_delivery_email(val) + elif path.first_path == ("name", "formatted", None): + # TODO: Add support name_formatted_included=False config like we do + # for updates via PUT. + assert isinstance(val, str) + self.change_full_name(val) + elif path.first_path == ("active", None, None): + assert isinstance(val, bool) + self.change_is_active(val) + else: + raise scim_exceptions.NotImplementedError("Not Implemented") + + self.save() + + def save(self) -> None: + """ + This method is called at the end of operations modifying a user, + and is responsible for actually applying the requested changes, + writing them to the database. + """ + realm = RequestNotes.get_notes(self._request).realm + assert realm is not None + + email_new_value = getattr(self, "_email_new_value", None) + is_active_new_value = getattr(self, "_is_active_new_value", None) + full_name_new_value = getattr(self, "_full_name_new_value", None) + password = getattr(self, "_password_set_to", None) + + # Clean up the internal "pending change" state, now that we've + # fetched the values: + self._email_new_value = None + self._is_active_new_value = None + self._full_name_new_value = None + self._password_set_to = None + + if email_new_value: + try: + # Note that the validate_email check that usually + # appears adjacent to email_allowed_for_realm is + # present in save(). + email_allowed_for_realm(email_new_value, realm) + except DomainNotAllowedForRealmError: + raise scim_exceptions.BadRequestError( + "This email domain isn't allowed in this organization." + ) + except DisposableEmailError: # nocoverage + raise scim_exceptions.BadRequestError( + "Disposable email domains are not allowed for this realm." + ) + except EmailContainsPlusError: # nocoverage + raise scim_exceptions.BadRequestError("Email address can't contain + characters.") + + try: + validate_email_not_already_in_realm(realm, email_new_value) + except ValidationError as e: + raise ConflictError("Email address already in use: " + str(e)) + + if self.is_new_user(): + self.obj = do_create_user( + email_new_value, + password, + realm, + full_name_new_value, + acting_user=None, + ) + return + + with transaction.atomic(): + # We process full_name first here, since it's the only one that can fail. + if full_name_new_value: + check_change_full_name(self.obj, full_name_new_value, acting_user=None) + + if email_new_value: + do_change_user_delivery_email(self.obj, email_new_value) + + if is_active_new_value is not None and is_active_new_value: + do_reactivate_user(self.obj, acting_user=None) + elif is_active_new_value is not None and not is_active_new_value: + do_deactivate_user(self.obj, acting_user=None) + + def delete(self) -> None: + """ + This is consistent with Okta SCIM - users don't get DELETEd, they're deactivated + by changing their "active" attr to False. + """ + raise scim_exceptions.BadRequestError( + 'DELETE operation not supported. Use PUT or PATCH to modify the "active" attribute instead.' + ) + + +def get_extra_model_filter_kwargs_getter( + model: Type[models.Model], +) -> Callable[[HttpRequest, Any, Any], Dict[str, object]]: + """Registered as GET_EXTRA_MODEL_FILTER_KWARGS_GETTER in our + SCIM configuration. + + Returns a function which generates additional kwargs + to add to QuerySet's .filter() when fetching a UserProfile + corresponding to the requested SCIM User from the database. + + It's *crucial* for security that we filter by realm_id (based on + the subdomain of the request) to prevent a SCIM client authorized + for subdomain X from being able to interact with all of the Users + on the entire server. + + This should be extended for Groups when implementing them by + checking the `model` parameter; because we only support + UserProfiles, such a check is unnecessary. + """ + + def get_extra_filter_kwargs( + request: HttpRequest, *args: Any, **kwargs: Any + ) -> Dict[str, object]: + realm = RequestNotes.get_notes(request).realm + assert realm is not None + return {"realm_id": realm.id, "is_bot": False} + + return get_extra_filter_kwargs + + +def base_scim_location_getter(request: HttpRequest, *args: Any, **kwargs: Any) -> str: + """Used as the base url for constructing the Location of a SCIM resource. + + Since SCIM synchronization is scoped to an individual realm, we + need these locations to be namespaced within the realm's domain + namespace, which is conveniently accessed via realm.uri. + """ + + realm = RequestNotes.get_notes(request).realm + assert realm is not None + + return realm.uri + + +class ConflictError(scim_exceptions.IntegrityError): + """ + Per https://datatracker.ietf.org/doc/html/rfc7644#section-3.3 + + If the service provider determines that the creation of the requested + resource conflicts with existing resources (e.g., a "User" resource + with a duplicate "userName"), the service provider MUST return HTTP + status code 409 (Conflict) with a "scimType" error code of + "uniqueness" + + scim_exceptions.IntegrityError class omits to include the scimType. + """ + + scim_type = "uniqueness" diff --git a/zerver/lib/scim_filter.py b/zerver/lib/scim_filter.py new file mode 100644 index 0000000000..a3dff20388 --- /dev/null +++ b/zerver/lib/scim_filter.py @@ -0,0 +1,62 @@ +from typing import List, Optional, Tuple + +from django.http import HttpRequest +from django_scim.filters import UserFilterQuery + +from zerver.lib.request import RequestNotes + + +# This is in a separate file due to circular import issues django-scim2 runs into +# when this is placed in zerver.lib.scim. +class ZulipUserFilterQuery(UserFilterQuery): + """This class implements the filter functionality of SCIM2. + E.g. requests such as + /scim/v2/Users?filter=userName eq "hamlet@zulip.com" + can be made to refer to resources via their properties. + This gets fairly complicated in its full scope + (https://datatracker.ietf.org/doc/html/rfc7644#section-3.4.2.2) + and django-scim2 implements an entire mechanism of converting + this SCIM2 filter syntax into SQL queries. + + What we have to do in this class is to customize django-scim2 so + that it knows which SCIM attributes map to which UserProfile + fields. We can assume that get_extra_model_filter_kwargs_getter + has already ensured that we will only interact with non-bot user + accounts in the realm associated with this SCIM configuration. + """ + + # attr_map describes which table.column the given SCIM2 User + # attributes refer to. + attr_map = { + # attr, sub attr, uri + ("userName", None, None): "zerver_userprofile.delivery_email", + # We can only reasonably support filtering by name.formatted + # as UserProfile.full_name is its equivalent. We don't store + # first/last name information for UserProfile, so we can't + # support filtering based on name.givenName or name.familyName. + ("name", "formatted", None): "zerver_userprofile.full_name", + ("active", None, None): "zerver_userprofile.is_active", + } + + # joins tells django-scim2 to always add the specified JOINS + # to the formed SQL queries. We need to JOIN the Realm table + # because we need to limit the results to the realm (subdomain) + # of the request. + joins = ("INNER JOIN zerver_realm ON zerver_realm.id = realm_id",) + + @classmethod + def get_extras(cls, q: str, request: Optional[HttpRequest] = None) -> Tuple[str, List[object]]: + """ + Return extra SQL and params to be attached to end of current Query's + SQL and params. The return format matches the format that should be used + for providing raw SQL with params to Django's .raw(): + https://docs.djangoproject.com/en/3.2/topics/db/sql/#passing-parameters-into-raw + + Here we ensure that results are limited to the subdomain of the request + and also exclude bots, as we currently don't want them to be managed by SCIM2. + """ + assert request is not None + realm = RequestNotes.get_notes(request).realm + assert realm is not None + + return "AND zerver_realm.id = %s AND zerver_userprofile.is_bot = False", [realm.id] diff --git a/zerver/lib/test_classes.py b/zerver/lib/test_classes.py index ae485282c5..083bebc97e 100644 --- a/zerver/lib/test_classes.py +++ b/zerver/lib/test_classes.py @@ -326,6 +326,12 @@ Output: self.validate_api_response_openapi(url, "patch", result, info, kwargs) return result + def json_patch(self, url: str, payload: Dict[str, Any] = {}, **kwargs: Any) -> HttpResponse: + data = orjson.dumps(payload) + django_client = self.client # see WRAPPER_COMMENT + self.set_http_headers(kwargs) + return django_client.patch(url, data=data, content_type="application/json", **kwargs) + @instrument_url def client_put(self, url: str, info: Dict[str, Any] = {}, **kwargs: Any) -> HttpResponse: encoded = urllib.parse.urlencode(info) @@ -333,6 +339,12 @@ Output: self.set_http_headers(kwargs) return django_client.put(url, encoded, **kwargs) + def json_put(self, url: str, payload: Dict[str, Any] = {}, **kwargs: Any) -> HttpResponse: + data = orjson.dumps(payload) + django_client = self.client # see WRAPPER_COMMENT + self.set_http_headers(kwargs) + return django_client.put(url, data=data, content_type="application/json", **kwargs) + @instrument_url def client_delete(self, url: str, info: Dict[str, Any] = {}, **kwargs: Any) -> HttpResponse: encoded = urllib.parse.urlencode(info) diff --git a/zerver/lib/test_helpers.py b/zerver/lib/test_helpers.py index 711cacf674..81fa75f397 100644 --- a/zerver/lib/test_helpers.py +++ b/zerver/lib/test_helpers.py @@ -489,6 +489,12 @@ def write_instrumentation_reports(full_suite: bool, include_webhooks: bool) -> N "static/(?P.+)", "flush_caches", "external_content/(?P[^/]+)/(?P[^/]+)", + # These are SCIM2 urls overriden from django-scim2 to return Not Implemented. + # We actually test them, but it's not being detected as a tested pattern, + # possibly due to the use of re_path. TODO: Investigate and get them + # recognized as tested. + "scim/v2/Groups(?:/(?P[^/]+))?", + "scim/v2/Groups/.search", *(webhook.url for webhook in WEBHOOK_INTEGRATIONS if not include_webhooks), } diff --git a/zerver/management/commands/add_scim_client.py b/zerver/management/commands/add_scim_client.py new file mode 100644 index 0000000000..c2c1bddadf --- /dev/null +++ b/zerver/management/commands/add_scim_client.py @@ -0,0 +1,20 @@ +from argparse import ArgumentParser +from typing import Any + +from zerver.lib.management import ZulipBaseCommand +from zerver.models import SCIMClient + + +class Command(ZulipBaseCommand): + help = """Create a SCIM client entry in the database.""" + + def add_arguments(self, parser: ArgumentParser) -> None: + self.add_realm_args(parser) + parser.add_argument("name", help="name of the client") + + def handle(self, *args: Any, **options: Any) -> None: + client_name = options["name"] + realm = self.get_realm(options) + assert realm + + SCIMClient.objects.create(realm=realm, name=client_name) diff --git a/zerver/middleware.py b/zerver/middleware.py index 037e311a84..00952aa845 100644 --- a/zerver/middleware.py +++ b/zerver/middleware.py @@ -28,6 +28,8 @@ from django.utils.cache import patch_vary_headers from django.utils.deprecation import MiddlewareMixin from django.utils.translation import gettext as _ from django.views.csrf import csrf_failure as html_csrf_failure +from django_scim.middleware import SCIMAuthCheckMiddleware +from django_scim.settings import scim_settings from sentry_sdk import capture_exception from sentry_sdk.integrations.logging import ignore_logger @@ -44,7 +46,7 @@ from zerver.lib.subdomains import get_subdomain from zerver.lib.types import ViewFuncT from zerver.lib.user_agent import parse_user_agent from zerver.lib.utils import statsd -from zerver.models import Realm, flush_per_request_caches, get_realm +from zerver.models import Realm, SCIMClient, flush_per_request_caches, get_realm logger = logging.getLogger("zulip.requests") slow_query_logger = logging.getLogger("zulip.slow_queries") @@ -664,3 +666,71 @@ class ZulipCommonMiddleware(CommonMiddleware): if settings.RUNNING_INSIDE_TORNADO: return False return super().should_redirect_with_slash(request) + + +def validate_scim_bearer_token(request: HttpRequest) -> Optional[SCIMClient]: + """ + 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 succesful, returns the corresponding SCIMClient object. Returns None otherwise. + """ + + subdomain = get_subdomain(request) + scim_config_dict = settings.SCIM_CONFIG.get(subdomain) + if not scim_config_dict: + return None + + valid_bearer_token = scim_config_dict.get("bearer_token") + scim_client_name = scim_config_dict.get("scim_client_name") + # We really don't want a misconfiguration where this is unset, + # allowing free access to the SCIM API: + assert valid_bearer_token + assert scim_client_name + + if request.headers.get("Authorization") != f"Bearer {valid_bearer_token}": + return None + + request_notes = RequestNotes.get_notes(request) + assert request_notes.realm + + # 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.objects.get(realm=request_notes.realm, name=scim_client_name) + + +class ZulipSCIMAuthCheckMiddleware(SCIMAuthCheckMiddleware): + """ + Overriden 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. + """ + + def process_request(self, request: HttpRequest) -> Optional[HttpResponse]: + # 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. + # We keep those logs in /var/log/zulip/scim.log + 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: + 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. + request.user = scim_client + return None diff --git a/zerver/migrations/0367_scimclient.py b/zerver/migrations/0367_scimclient.py new file mode 100644 index 0000000000..5d9a38c817 --- /dev/null +++ b/zerver/migrations/0367_scimclient.py @@ -0,0 +1,38 @@ +# Generated by Django 3.2.7 on 2021-10-03 18:04 + +import django.db.models.deletion +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ("zerver", "0366_group_group_membership"), + ] + + operations = [ + migrations.CreateModel( + name="SCIMClient", + fields=[ + ( + "id", + models.AutoField( + auto_created=True, primary_key=True, serialize=False, verbose_name="ID" + ), + ), + ( + "name", + models.TextField(), + ), + ( + "realm", + models.ForeignKey( + on_delete=django.db.models.deletion.CASCADE, to="zerver.realm" + ), + ), + ], + options={ + "unique_together": {("realm", "name")}, + }, + ), + ] diff --git a/zerver/models.py b/zerver/models.py index 89824ffafe..6580bc3e6a 100644 --- a/zerver/models.py +++ b/zerver/models.py @@ -4184,3 +4184,26 @@ def flush_alert_word(*, instance: AlertWord, **kwargs: object) -> None: post_save.connect(flush_alert_word, sender=AlertWord) post_delete.connect(flush_alert_word, sender=AlertWord) + + +class SCIMClient(models.Model): + realm: Realm = models.ForeignKey(Realm, on_delete=CASCADE) + name: str = models.TextField() + + class Meta: + unique_together = ("realm", "name") + + def __str__(self) -> str: + return f"" + + def format_requestor_for_logs(self) -> str: + return f"scim-client:{self.name}:realm:{self.realm_id}" + + @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 diff --git a/zerver/tests/test_scim.py b/zerver/tests/test_scim.py new file mode 100644 index 0000000000..78af14fee1 --- /dev/null +++ b/zerver/tests/test_scim.py @@ -0,0 +1,610 @@ +import copy +from contextlib import contextmanager +from typing import Any, Dict, Iterator, Mapping +from unittest import mock + +import orjson +from django.conf import settings +from django.http import HttpResponse + +from zerver.lib.actions import do_change_full_name +from zerver.lib.test_classes import ZulipTestCase +from zerver.models import SCIMClient, UserProfile, get_realm + + +class SCIMTestCase(ZulipTestCase): + def setUp(self) -> None: + super().setUp() + self.realm = get_realm("zulip") + self.scim_client = SCIMClient.objects.create( + realm=self.realm, name=settings.SCIM_CONFIG["zulip"]["scim_client_name"] + ) + + def scim_headers(self) -> Mapping[str, str]: + return {"HTTP_AUTHORIZATION": f"Bearer {settings.SCIM_CONFIG['zulip']['bearer_token']}"} + + def generate_user_schema(self, user_profile: UserProfile) -> Dict[str, Any]: + return { + "schemas": ["urn:ietf:params:scim:schemas:core:2.0:User"], + "id": user_profile.id, + "userName": user_profile.delivery_email, + "name": {"formatted": user_profile.full_name}, + "displayName": user_profile.full_name, + "active": True, + "meta": { + "resourceType": "User", + "created": user_profile.date_joined.isoformat(), + "lastModified": user_profile.date_joined.isoformat(), + "location": f"http://zulip.testserver/scim/v2/Users/{user_profile.id}", + }, + } + + def assert_uniqueness_error(self, result: HttpResponse, extra_message: str) -> None: + self.assertEqual(result.status_code, 409) + output_data = orjson.loads(result.content) + + expected_response_schema = { + "schemas": ["urn:ietf:params:scim:api:messages:2.0:Error"], + "detail": f"Email address already in use: {extra_message}", + "status": 409, + "scimType": "uniqueness", + } + self.assertEqual(output_data, expected_response_schema) + + @contextmanager + def mock_name_formatted_included(self, value: bool) -> Iterator[None]: + config_dict = copy.deepcopy(settings.SCIM_CONFIG) + config_dict["zulip"]["name_formatted_included"] = value + with self.settings(SCIM_CONFIG=config_dict): + yield + + +class TestNonSCIMAPIAccess(SCIMTestCase): + def test_scim_client_cant_access_different_apis(self) -> None: + """ + Verify that the SCIM client credentials can't be used to get + authenticated for non-SCIM API. + """ + hamlet = self.example_user("hamlet") + + # First verify validate_scim_bearer_token doesn't even get called, + # as verification of SCIM credentials shouldn't even be attempted, + # because we're not querying a SCIM endpoint. + with mock.patch("zerver.middleware.validate_scim_bearer_token", return_value=None) as m: + result = self.client_get(f"/api/v1/users/{hamlet.id}", **self.scim_headers()) + + # The SCIM format of the Authorization header (bearer token) is rejected as a bad request + # by our regular API authentication logic. + self.assert_json_error(result, "This endpoint requires HTTP basic authentication.", 400) + m.assert_not_called() + + # Now simply test end-to-end that access gets denied, without any mocking + # interfering with the process. + result = self.client_get(f"/api/v1/users/{hamlet.id}", **self.scim_headers()) + self.assert_json_error(result, "This endpoint requires HTTP basic authentication.", 400) + + +class TestSCIMUser(SCIMTestCase): + def test_get_by_id(self) -> None: + hamlet = self.example_user("hamlet") + expected_response_schema = self.generate_user_schema(hamlet) + + result = self.client_get(f"/scim/v2/Users/{hamlet.id}", **self.scim_headers()) + + self.assertEqual(result.status_code, 200) + output_data = orjson.loads(result.content) + self.assertEqual(output_data, expected_response_schema) + + def test_get_basic_filter_by_username(self) -> None: + hamlet = self.example_user("hamlet") + + expected_response_schema = { + "schemas": ["urn:ietf:params:scim:api:messages:2.0:ListResponse"], + "totalResults": 1, + "itemsPerPage": 50, + "startIndex": 1, + "Resources": [self.generate_user_schema(hamlet)], + } + + result = self.client_get( + f'/scim/v2/Users?filter=userName eq "{hamlet.delivery_email}"', **self.scim_headers() + ) + self.assertEqual(result.status_code, 200) + output_data = orjson.loads(result.content) + self.assertEqual(output_data, expected_response_schema) + + # Now we verify the filter feature doesn't allow access to users + # on different subdomains. + different_realm_user = self.mit_user("starnine") + self.assertNotEqual(different_realm_user.realm_id, hamlet.realm_id) + + result = self.client_get( + f'/scim/v2/Users?filter=userName eq "{different_realm_user.delivery_email}"', + **self.scim_headers(), + ) + self.assertEqual(result.status_code, 200) + output_data = orjson.loads(result.content) + + expected_empty_results_response_schema = { + "schemas": ["urn:ietf:params:scim:api:messages:2.0:ListResponse"], + "totalResults": 0, + "itemsPerPage": 50, + "startIndex": 1, + "Resources": [], + } + + self.assertEqual(output_data, expected_empty_results_response_schema) + + def test_get_all_with_pagination(self) -> None: + realm = get_realm("zulip") + + result_all = self.client_get("/scim/v2/Users", **self.scim_headers()) + self.assertEqual(result_all.status_code, 200) + output_data_all = orjson.loads(result_all.content) + + expected_response_schema = { + "schemas": ["urn:ietf:params:scim:api:messages:2.0:ListResponse"], + "totalResults": UserProfile.objects.filter(realm=realm, is_bot=False).count(), + "itemsPerPage": 50, + "startIndex": 1, + "Resources": [], + } + for user_profile in UserProfile.objects.filter(realm=realm, is_bot=False).order_by("id"): + user_schema = self.generate_user_schema(user_profile) + expected_response_schema["Resources"].append(user_schema) + + self.assertEqual(output_data_all, expected_response_schema) + + # Test pagination works, as defined in https://datatracker.ietf.org/doc/html/rfc7644#section-3.4.2.4 + result_offset_limited = self.client_get( + "/scim/v2/Users?startIndex=4&count=3", **self.scim_headers() + ) + self.assertEqual(result_offset_limited.status_code, 200) + output_data_offset_limited = orjson.loads(result_offset_limited.content) + self.assertEqual(output_data_offset_limited["itemsPerPage"], 3) + self.assertEqual(output_data_offset_limited["startIndex"], 4) + self.assertEqual( + output_data_offset_limited["totalResults"], output_data_all["totalResults"] + ) + self.assert_length(output_data_offset_limited["Resources"], 3) + + self.assertEqual(output_data_offset_limited["Resources"], output_data_all["Resources"][3:6]) + + def test_get_user_with_no_name_formatted_included_config(self) -> None: + """ + Some clients don't support name.formatted and rely and name.givenName and name.familyName. + We have the name_formatted_included configuration option for it for supporting that + behavior. Here we test the return dict representation of the User has the appropriate + format and values. + """ + hamlet = self.example_user("hamlet") + do_change_full_name(hamlet, "Firstname Lastname", acting_user=None) + expected_response_schema = self.generate_user_schema(hamlet) + expected_response_schema["name"] = {"givenName": "Firstname", "familyName": "Lastname"} + + with self.mock_name_formatted_included(False): + result = self.client_get(f"/scim/v2/Users/{hamlet.id}", **self.scim_headers()) + + self.assertEqual(result.status_code, 200) + output_data = orjson.loads(result.content) + self.assertEqual(output_data, expected_response_schema) + + do_change_full_name(hamlet, "Firstnameonly", acting_user=None) + expected_response_schema = self.generate_user_schema(hamlet) + expected_response_schema["name"] = {"givenName": "Firstnameonly", "familyName": ""} + + with self.mock_name_formatted_included(False): + result = self.client_get(f"/scim/v2/Users/{hamlet.id}", **self.scim_headers()) + + self.assertEqual(result.status_code, 200) + output_data = orjson.loads(result.content) + self.assertEqual(output_data, expected_response_schema) + + def test_post(self) -> None: + # A payload for creating a new user with the specified account details. + payload = { + "schemas": ["urn:ietf:params:scim:schemas:core:2.0:User"], + "userName": "newuser@zulip.com", + "name": {"formatted": "New User", "givenName": "New", "familyName": "User"}, + "active": True, + } + + original_user_count = UserProfile.objects.count() + result = self.client_post( + "/scim/v2/Users", payload, content_type="application/json", **self.scim_headers() + ) + + self.assertEqual(result.status_code, 201) + output_data = orjson.loads(result.content) + + new_user_count = UserProfile.objects.count() + self.assertEqual(new_user_count, original_user_count + 1) + + new_user = UserProfile.objects.last() + self.assertEqual(new_user.delivery_email, "newuser@zulip.com") + self.assertEqual(new_user.full_name, "New User") + + expected_response_schema = self.generate_user_schema(new_user) + self.assertEqual(output_data, expected_response_schema) + + def test_post_with_no_name_formatted_included_config(self) -> None: + # A payload for creating a new user with the specified account details. + payload = { + "schemas": ["urn:ietf:params:scim:schemas:core:2.0:User"], + "userName": "newuser@zulip.com", + "name": {"givenName": "New", "familyName": "User"}, + "active": True, + } + + original_user_count = UserProfile.objects.count() + with self.mock_name_formatted_included(False): + result = self.client_post( + "/scim/v2/Users", payload, content_type="application/json", **self.scim_headers() + ) + + self.assertEqual(result.status_code, 201) + output_data = orjson.loads(result.content) + + new_user_count = UserProfile.objects.count() + self.assertEqual(new_user_count, original_user_count + 1) + + new_user = UserProfile.objects.last() + self.assertEqual(new_user.delivery_email, "newuser@zulip.com") + self.assertEqual(new_user.full_name, "New User") + + expected_response_schema = self.generate_user_schema(new_user) + expected_response_schema["name"] = {"givenName": "New", "familyName": "User"} + self.assertEqual(output_data, expected_response_schema) + + def test_post_email_exists(self) -> None: + hamlet = self.example_user("hamlet") + # A payload for creating a new user with an email that already exists. Thus + # this should fail. + payload = { + "schemas": ["urn:ietf:params:scim:schemas:core:2.0:User"], + "userName": hamlet.delivery_email, + "name": {"formatted": "New User", "givenName": "New", "familyName": "User"}, + "active": True, + } + + result = self.client_post( + "/scim/v2/Users", payload, content_type="application/json", **self.scim_headers() + ) + self.assert_uniqueness_error(result, f"['{hamlet.delivery_email} already has an account']") + + def test_post_name_attribute_missing(self) -> None: + # A payload for creating a new user without a name, which should make this request fail. + payload = { + "schemas": ["urn:ietf:params:scim:schemas:core:2.0:User"], + "userName": "newuser@zulip.com", + "active": True, + } + + result = self.client_post( + "/scim/v2/Users", payload, content_type="application/json", **self.scim_headers() + ) + response_dict = result.json() + self.assertEqual( + response_dict, + { + "schemas": ["urn:ietf:params:scim:api:messages:2.0:Error"], + "detail": "Must specify name.formatted, name.givenName or name.familyName when creating a new user", + "status": 400, + }, + ) + + def test_post_active_set_to_false(self) -> None: + # A payload for creating a new user with is_active=False, which is an invalid operation. + payload = { + "schemas": ["urn:ietf:params:scim:schemas:core:2.0:User"], + "userName": "newuser@zulip.com", + "name": {"formatted": "New User", "givenName": "New", "familyName": "User"}, + "active": False, + } + + result = self.client_post( + "/scim/v2/Users", payload, content_type="application/json", **self.scim_headers() + ) + response_dict = result.json() + self.assertEqual( + response_dict, + { + "schemas": ["urn:ietf:params:scim:api:messages:2.0:Error"], + "detail": "New user must have active=True", + "status": 400, + }, + ) + + def test_post_email_domain_not_allow(self) -> None: + realm = get_realm("zulip") + realm.emails_restricted_to_domains = True + realm.save(update_fields=["emails_restricted_to_domains"]) + + # A payload for creating a new user with the specified details. + payload = { + "schemas": ["urn:ietf:params:scim:schemas:core:2.0:User"], + "userName": "newuser@acme.com", + "name": {"formatted": "New User", "givenName": "New", "familyName": "User"}, + "active": True, + } + + result = self.client_post( + "/scim/v2/Users", payload, content_type="application/json", **self.scim_headers() + ) + response_dict = result.json() + self.assertEqual( + response_dict, + { + "schemas": ["urn:ietf:params:scim:api:messages:2.0:Error"], + "detail": "This email domain isn't allowed in this organization.", + "status": 400, + }, + ) + + def test_post_to_try_creating_new_user_on_different_subdomain(self) -> None: + # A payload for creating a new user with the specified details. + payload = { + "schemas": ["urn:ietf:params:scim:schemas:core:2.0:User"], + "userName": "newuser@acme.com", + "name": {"formatted": "New User", "givenName": "New", "familyName": "User"}, + "active": True, + } + + # Now we make the SCIM request to a different subdomain than our credentials + # are configured for. Unauthorized is the expected response. + result = self.client_post( + "/scim/v2/Users", + payload, + content_type="application/json", + subdomain="lear", + **self.scim_headers(), + ) + self.assertEqual(result.status_code, 401) + + def test_delete(self) -> None: + hamlet = self.example_user("hamlet") + result = self.client_delete(f"/scim/v2/Users/{hamlet.id}", **self.scim_headers()) + + expected_response_schema = { + "schemas": ["urn:ietf:params:scim:api:messages:2.0:Error"], + "detail": 'DELETE operation not supported. Use PUT or PATCH to modify the "active" attribute instead.', + "status": 400, + } + + self.assertEqual(result.status_code, 400) + output_data = orjson.loads(result.content) + self.assertEqual(output_data, expected_response_schema) + + def test_put_change_email_and_name(self) -> None: + hamlet = self.example_user("hamlet") + # PUT replaces all specified attributes of the user. Thus, + # this payload will replace hamlet's account details with the new ones, + # as specified. + payload = { + "schemas": ["urn:ietf:params:scim:schemas:core:2.0:User"], + "id": hamlet.id, + "userName": "bjensen@zulip.com", + "name": { + "formatted": "Ms. Barbara J Jensen III", + "familyName": "Jensen", + "givenName": "Barbara", + "middleName": "Jane", + }, + } + result = self.json_put(f"/scim/v2/Users/{hamlet.id}", payload, **self.scim_headers()) + self.assertEqual(result.status_code, 200) + + hamlet.refresh_from_db() + self.assertEqual(hamlet.delivery_email, "bjensen@zulip.com") + self.assertEqual(hamlet.full_name, "Ms. Barbara J Jensen III") + + output_data = orjson.loads(result.content) + expected_response_schema = self.generate_user_schema(hamlet) + self.assertEqual(output_data, expected_response_schema) + + def test_put_change_name_only(self) -> None: + hamlet = self.example_user("hamlet") + hamlet_email = hamlet.delivery_email + # This payload specified hamlet's current email to not change this attribute, + # and only alters the name. + payload = { + "schemas": ["urn:ietf:params:scim:schemas:core:2.0:User"], + "id": hamlet.id, + "userName": hamlet_email, + "name": { + "formatted": "Ms. Barbara J Jensen III", + "familyName": "Jensen", + "givenName": "Barbara", + "middleName": "Jane", + }, + } + result = self.json_put(f"/scim/v2/Users/{hamlet.id}", payload, **self.scim_headers()) + self.assertEqual(result.status_code, 200) + + hamlet.refresh_from_db() + self.assertEqual(hamlet.delivery_email, hamlet_email) + self.assertEqual(hamlet.full_name, "Ms. Barbara J Jensen III") + + output_data = orjson.loads(result.content) + expected_response_schema = self.generate_user_schema(hamlet) + self.assertEqual(output_data, expected_response_schema) + + def test_put_email_exists(self) -> None: + hamlet = self.example_user("hamlet") + cordelia = self.example_user("cordelia") + # This payload will attempt to change hamlet's email to cordelia's email. + # That would would violate email uniqueness of course, so should fail. + payload = { + "schemas": ["urn:ietf:params:scim:schemas:core:2.0:User"], + "id": hamlet.id, + "userName": cordelia.delivery_email, + "name": { + "formatted": "Ms. Barbara J Jensen III", + "familyName": "Jensen", + "givenName": "Barbara", + "middleName": "Jane", + }, + } + result = self.json_put(f"/scim/v2/Users/{hamlet.id}", payload, **self.scim_headers()) + self.assert_uniqueness_error( + result, f"['{cordelia.delivery_email} already has an account']" + ) + + def test_put_deactivate_reactivate_user(self) -> None: + hamlet = self.example_user("hamlet") + # This payload flips the active attribute to deactivate the user. + payload = { + "schemas": ["urn:ietf:params:scim:schemas:core:2.0:User"], + "id": hamlet.id, + "userName": hamlet.delivery_email, + "active": False, + } + result = self.json_put(f"/scim/v2/Users/{hamlet.id}", payload, **self.scim_headers()) + self.assertEqual(result.status_code, 200) + + hamlet.refresh_from_db() + self.assertEqual(hamlet.is_active, False) + + # We modify the active attribute in the payload to cause reactivation of the user. + payload["active"] = True + result = self.json_put(f"/scim/v2/Users/{hamlet.id}", payload, **self.scim_headers()) + self.assertEqual(result.status_code, 200) + + hamlet.refresh_from_db() + self.assertEqual(hamlet.is_active, True) + + def test_patch_with_path(self) -> None: + hamlet = self.example_user("hamlet") + # Payload for a PATCH request to change the user's email to the specified value. + payload = { + "schemas": ["urn:ietf:params:scim:api:messages:2.0:PatchOp"], + "Operations": [{"op": "replace", "path": "userName", "value": "hamlet_new@zulip.com"}], + } + + result = self.json_patch(f"/scim/v2/Users/{hamlet.id}", payload, **self.scim_headers()) + self.assertEqual(result.status_code, 200) + + hamlet.refresh_from_db() + self.assertEqual(hamlet.delivery_email, "hamlet_new@zulip.com") + + output_data = orjson.loads(result.content) + expected_response_schema = self.generate_user_schema(hamlet) + self.assertEqual(output_data, expected_response_schema) + + # Multiple operations: + # This payload changes the user's email and name to the specified values. + payload = { + "schemas": ["urn:ietf:params:scim:api:messages:2.0:PatchOp"], + "Operations": [ + {"op": "replace", "path": "userName", "value": "hamlet_new2@zulip.com"}, + {"op": "replace", "path": "name.formatted", "value": "New Name"}, + ], + } + result = self.json_patch(f"/scim/v2/Users/{hamlet.id}", payload, **self.scim_headers()) + self.assertEqual(result.status_code, 200) + + hamlet.refresh_from_db() + self.assertEqual(hamlet.full_name, "New Name") + self.assertEqual(hamlet.delivery_email, "hamlet_new2@zulip.com") + + output_data = orjson.loads(result.content) + expected_response_schema = self.generate_user_schema(hamlet) + self.assertEqual(output_data, expected_response_schema) + + def test_patch_without_path(self) -> None: + """ + PATCH requests can also specify Operations in a different form, + without specifying the "path" op attribute and instead specifying + the user attribute to modify in the "value" dict. + """ + + hamlet = self.example_user("hamlet") + # This payload changes the user's email to the specified value. + payload = { + "schemas": ["urn:ietf:params:scim:api:messages:2.0:PatchOp"], + "Operations": [{"op": "replace", "value": {"userName": "hamlet_new@zulip.com"}}], + } + + result = self.json_patch(f"/scim/v2/Users/{hamlet.id}", payload, **self.scim_headers()) + self.assertEqual(result.status_code, 200) + + hamlet.refresh_from_db() + self.assertEqual(hamlet.delivery_email, "hamlet_new@zulip.com") + + output_data = orjson.loads(result.content) + expected_response_schema = self.generate_user_schema(hamlet) + self.assertEqual(output_data, expected_response_schema) + + def test_patch_deactivate_reactivate_user(self) -> None: + hamlet = self.example_user("hamlet") + # Payload for a PATCH request to deactivate the user. + payload = { + "schemas": ["urn:ietf:params:scim:api:messages:2.0:PatchOp"], + "Operations": [{"op": "replace", "path": "active", "value": False}], + } + + result = self.json_patch(f"/scim/v2/Users/{hamlet.id}", payload, **self.scim_headers()) + self.assertEqual(result.status_code, 200) + + hamlet.refresh_from_db() + self.assertEqual(hamlet.is_active, False) + + # Payload for a PATCH request to reactivate the user. + payload = { + "schemas": ["urn:ietf:params:scim:api:messages:2.0:PatchOp"], + "Operations": [{"op": "replace", "path": "active", "value": True}], + } + result = self.json_patch(f"/scim/v2/Users/{hamlet.id}", payload, **self.scim_headers()) + self.assertEqual(result.status_code, 200) + hamlet.refresh_from_db() + self.assertEqual(hamlet.is_active, True) + + def test_patch_unsupported_attribute(self) -> None: + hamlet = self.example_user("hamlet") + # Payload for a PATCH request to change the middle name of the user - which is not supported. + payload = { + "schemas": ["urn:ietf:params:scim:api:messages:2.0:PatchOp"], + "Operations": [{"op": "replace", "path": "name.middleName", "value": "John"}], + } + + with self.assertLogs("django.request", "ERROR") as m: + result = self.json_patch(f"/scim/v2/Users/{hamlet.id}", payload, **self.scim_headers()) + self.assertEqual( + result.json(), + { + "schemas": ["urn:ietf:params:scim:api:messages:2.0:Error"], + "detail": "Not Implemented", + "status": 501, + }, + ) + self.assertEqual( + m.output, [f"ERROR:django.request:Not Implemented: /scim/v2/Users/{hamlet.id}"] + ) + + +class TestSCIMGroup(SCIMTestCase): + """ + SCIM groups aren't implemented yet. An implementation will modify this class + to actually test desired behavior. + """ + + def test_endpoints_disabled(self) -> None: + with self.assertLogs("django.request", "ERROR") as m: + result = self.client_get("/scim/v2/Groups", **self.scim_headers()) + self.assertEqual(result.status_code, 501) + self.assertEqual(m.output, ["ERROR:django.request:Not Implemented: /scim/v2/Groups"]) + with self.assertLogs("django.request", "ERROR") as m: + result = self.client_get("/scim/v2/Groups/1", **self.scim_headers()) + self.assertEqual(result.status_code, 501) + self.assertEqual(m.output, ["ERROR:django.request:Not Implemented: /scim/v2/Groups/1"]) + with self.assertLogs("django.request", "ERROR") as m: + result = self.client_post( + "/scim/v2/Groups/.search", + {}, + content_type="application/json", + **self.scim_headers(), + ) + self.assertEqual(result.status_code, 501) + self.assertEqual( + m.output, ["ERROR:django.request:Not Implemented: /scim/v2/Groups/.search"] + ) diff --git a/zproject/computed_settings.py b/zproject/computed_settings.py index 1b1c2fee2f..a40b2990fc 100644 --- a/zproject/computed_settings.py +++ b/zproject/computed_settings.py @@ -187,6 +187,7 @@ MIDDLEWARE = ( "zerver.middleware.HostDomainMiddleware", "django.middleware.csrf.CsrfViewMiddleware", "django.contrib.auth.middleware.AuthenticationMiddleware", + "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 @@ -213,6 +214,7 @@ INSTALLED_APPS = [ "confirmation", "zerver", "social_django", + "django_scim", # 2FA related apps. "django_otp", "django_otp.plugins.otp_static", @@ -716,6 +718,7 @@ TRACEMALLOC_DUMP_DIR = zulip_path("/var/log/zulip/tracemalloc") DELIVER_SCHEDULED_MESSAGES_LOG_PATH = zulip_path("/var/log/zulip/deliver_scheduled_messages.log") RETENTION_LOG_PATH = zulip_path("/var/log/zulip/message_retention.log") AUTH_LOG_PATH = zulip_path("/var/log/zulip/auth.log") +SCIM_LOG_PATH = zulip_path("/var/log/zulip/scim.log") ZULIP_WORKER_TEST_FILE = "/tmp/zulip-worker-test-file" @@ -817,6 +820,12 @@ LOGGING: Dict[str, Any] = { "formatter": "default", "filename": LDAP_LOG_PATH, }, + "scim_file": { + "level": "DEBUG", + "class": "logging.handlers.WatchedFileHandler", + "formatter": "default", + "filename": SCIM_LOG_PATH, + }, "slow_queries_file": { "level": "INFO", "class": "logging.handlers.WatchedFileHandler", @@ -913,6 +922,11 @@ LOGGING: Dict[str, Any] = { "handlers": ["console", "ldap_file", "errors_file"], "propagate": False, }, + "django_scim": { + "level": "DEBUG", + "handlers": ["scim_file", "errors_file"], + "propagate": False, + }, "pika": { # pika is super chatty on INFO. "level": "WARNING", @@ -1194,3 +1208,22 @@ if SENTRY_DSN: from .sentry import setup_sentry setup_sentry(SENTRY_DSN, get_config("machine", "deploy_type", "development")) + +SCIM_SERVICE_PROVIDER = { + "USER_ADAPTER": "zerver.lib.scim.ZulipSCIMUser", + "USER_FILTER_PARSER": "zerver.lib.scim_filter.ZulipUserFilterQuery", + # NETLOC is actually overriden by the behavior of base_scim_location_getter, + # but django-scim2 requires it to be set, even though it ends up not being used. + # So we need to give it some value here, and EXTERNAL_HOST is the most generic. + "NETLOC": EXTERNAL_HOST, + "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", + "AUTHENTICATION_SCHEMES": [ + { + "type": "bearer", + "name": "Bearer", + "description": "Bearer token", + }, + ], +} diff --git a/zproject/dev_settings.py b/zproject/dev_settings.py index f39f3e177a..3da5d1fa93 100644 --- a/zproject/dev_settings.py +++ b/zproject/dev_settings.py @@ -179,3 +179,11 @@ SOCIAL_AUTH_SAML_SP_ENTITY_ID = "http://localhost:9991" SOCIAL_AUTH_SUBDOMAIN = "auth" MEMCACHED_USERNAME: Optional[str] = None + +SCIM_CONFIG = { + "zulip": { + "bearer_token": "token1234", + "scim_client_name": "test-scim-client", + "name_formatted_included": True, + } +} diff --git a/zproject/test_extra_settings.py b/zproject/test_extra_settings.py index 4dbcac3227..ee8b630ec6 100644 --- a/zproject/test_extra_settings.py +++ b/zproject/test_extra_settings.py @@ -271,3 +271,11 @@ RATE_LIMITING_RULES: Dict[str, List[Tuple[int, int]]] = { } FREE_TRIAL_DAYS: Optional[int] = None + +SCIM_CONFIG = { + "zulip": { + "bearer_token": "token1234", + "scim_client_name": "test-scim-client", + "name_formatted_included": True, + } +} diff --git a/zproject/urls.py b/zproject/urls.py index 8dd5c8e0e4..f433dfbef3 100644 --- a/zproject/urls.py +++ b/zproject/urls.py @@ -10,7 +10,7 @@ from django.contrib.auth.views import ( PasswordResetConfirmView, PasswordResetDoneView, ) -from django.urls import path +from django.urls import path, re_path from django.urls.resolvers import URLPattern, URLResolver from django.utils.module_loading import import_string from django.views.generic import RedirectView, TemplateView @@ -740,6 +740,24 @@ urls += [ urls += [path("", include("social_django.urls", namespace="social"))] urls += [path("saml/metadata.xml", saml_sp_metadata)] +# SCIM2 + +from django_scim import views as scim_views + +urls += [ + # These first couple entries mark the Groups feature of SCIM as + # something we haven't implemented. + re_path( + r"^scim/v2/Groups/.search$", + scim_views.SCIMView.as_view(implemented=False), + ), + re_path( + r"^scim/v2/Groups(?:/(?P[^/]+))?$", + scim_views.SCIMView.as_view(implemented=False), + ), + path("scim/v2/", include("django_scim.urls", namespace="scim")), +] + # User documentation site help_documentation_view = MarkdownDirectoryView.as_view( template_name="zerver/documentation_main.html", path_template="/zerver/help/%s.md"