From 254b0ff4774594694889f0aa2e20154b73f95ecb Mon Sep 17 00:00:00 2001 From: Mateusz Mandera Date: Sat, 12 Apr 2025 02:56:02 +0800 Subject: [PATCH] scim: Add support for Groups. --- help/scim.md | 58 ++- zerver/lib/scim.py | 360 +++++++++++++++++- zerver/lib/scim_filter.py | 22 +- zerver/tests/test_scim.py | 686 +++++++++++++++++++++++++++++++++- zproject/computed_settings.py | 3 + zproject/urls.py | 4 - 6 files changed, 1107 insertions(+), 26 deletions(-) diff --git a/help/scim.md b/help/scim.md index 53f79093da..aacf3570bd 100644 --- a/help/scim.md +++ b/help/scim.md @@ -9,17 +9,16 @@ integration, both in Zulip Cloud and for [self-hosted](/self-hosting/) Zulip servers. This page describes how to configure SCIM provisioning for Zulip. -Zulip's SCIM integration has the following limitations: - -* Provisioning Groups is not yet implemented. -* While Zulip's SCIM integration is generic, we've only fully - documented the setup process with the Okta and Microsoft EntraID - SCIM providers. [Zulip support](/help/contact-support) is happy to - help customers configure this integration with SCIM providers that - do not yet have detailed self-service documentation on this page. - {!cloud-plus-only.md!} +!!! tip "" + + While Zulip's SCIM integration is generic, we've only fully + documented the setup process with the Okta and Microsoft EntraID + SCIM providers. [Zulip support](/help/contact-support) is happy to + help customers configure this integration with SCIM providers that + do not yet have detailed self-service documentation on this page. + ## Configure SCIM {start_tabs} @@ -160,6 +159,47 @@ Zulip's SCIM integration has the following limitations: Once SCIM has been configured, consider also [configuring SAML](/help/saml-authentication). +## Synchronizing group membership with SCIM + +Zulip also supports syncing of users' [groups](/help/user-groups) via SCIM. + +{start_tabs} + +{tab|okta} + +1. Follow the instructions [above](#configure-scim) to configure SCIM. + +1. Open the **Application** you set up above for the Zulip SCIM integration, and + go to the **Push groups** tab. This menu allows you to choose the Okta groups + which should be synchronized with Zulip's user groups. + +1. Make sure you're familiar with the below rules governing SCIM group sync behavior: + + * When you set up an Okta group in the **Push groups** tab, the SCIM + integration will create a user group in Zulip with the matching name and + user memberships. + * When you add or remove users from the group in Okta, these changes will + be immediately be reflected in group memberships in Zulip. + * You can only enable synchronization of groups which do not yet exist in Zulip. + If you push a group whose name matches an existing Zulip group, the request + will fail. + * SCIM `DELETE` requests are not supported for groups. This means that if you choose + to **Unlink** a group in Okta from the Zulip SCIM integration, you **must** select + **Leave the group in the target app**. The **Delete the group in the target app** + option is not supported. + * In order to ensure consistent state, do not modify + the name or memberships of SCIM-managed groups inside of Zulip. Such groups are + meant to be managed in Okta. Changes made on the Zulip side will not be reflected + in Okta and instead will cause the state of the Zulip group to become inconsistent + with the state of the Okta group. + * Note that while Zulip supports nested groups, Okta does not. Zulip, + unlike Okta, supports nesting groups inside other groups. This means that + you will use SCIM to sync the direct members of groups in Zulip, and then if + you'd like to have a group contain another group in Zulip + permissions, configure that subgroup relationship directly in Zulip. + +{end_tabs} + ## Related articles * [SAML authentication](/help/saml-authentication) diff --git a/zerver/lib/scim.py b/zerver/lib/scim.py index 232073a865..5175dd493a 100644 --- a/zerver/lib/scim.py +++ b/zerver/lib/scim.py @@ -1,22 +1,39 @@ +import re from collections.abc import Callable from typing import Any import django_scim.constants as scim_constants import django_scim.exceptions as scim_exceptions +import django_scim.utils as scim_utils from django.conf import settings from django.core.exceptions import ValidationError -from django.db import models +from django.db import models, transaction from django.http import HttpRequest -from django_scim.adapters import SCIMUser +from django.urls import resolve +from django_scim.adapters import SCIMGroup, SCIMUser from scim2_filter_parser.attr_paths import AttrPath from zerver.actions.create_user import do_create_user, do_reactivate_user +from zerver.actions.user_groups import ( + bulk_add_members_to_user_groups, + bulk_remove_members_from_user_groups, + create_user_group_in_database, + do_update_user_group_name, +) from zerver.actions.user_settings import check_change_full_name, do_change_user_delivery_email from zerver.actions.users import do_change_user_role, do_deactivate_user +from zerver.context_processors import get_realm_from_request from zerver.lib.email_validation import email_allowed_for_realm, validate_email_not_already_in_realm +from zerver.lib.exceptions import JsonableError from zerver.lib.request import RequestNotes from zerver.lib.subdomains import get_subdomain -from zerver.models import UserProfile +from zerver.lib.user_groups import ( + check_user_group_name, + get_role_based_system_groups_dict, + get_user_group_direct_member_ids, +) +from zerver.models import Realm, UserProfile +from zerver.models.groups import NamedUserGroup, SystemGroups from zerver.models.realms import ( DisposableEmailError, DomainNotAllowedForRealmError, @@ -340,6 +357,331 @@ class ZulipSCIMUser(SCIMUser): ) +def validate_group_member_ids_from_request(realm: Realm, member_ids: list[int]) -> None: + if member_ids: + member_ids_set = set(member_ids) + member_realm_ids = list( + UserProfile.objects.filter(id__in=member_ids_set) + .distinct("realm_id") + .values_list("realm_id", flat=True) + ) + if len(member_realm_ids) > 1 or member_realm_ids[0] != realm.id: + raise scim_exceptions.BadRequestError( + "Users outside of the realm can't be removed or added to the group" + ) + + found_member_ids_set = set( + UserProfile.objects.filter(id__in=member_ids_set).values_list("id", flat=True) + ) + if len(member_ids_set) != len(found_member_ids_set): + raise scim_exceptions.BadRequestError( + f"Invalid user ids found in the request: {member_ids_set - found_member_ids_set}" + ) + + +def check_can_manage_group_by_scim(user_group: NamedUserGroup) -> bool: + # Prohibit system groups. + if user_group.is_system_group: + return False + return True + + +class ZulipSCIMGroup(SCIMGroup): + """ + This class contains the core of the implementation of SCIM sync of Groups. + A SCIM Group corresponds to a NamedUserGroup object in Zulip. + + This class follows the same architecture as ZulipSCIMUser, so rather than + re-explaining the purpose of specific method overrides or small bits of + equivalent logic, defer to checking the corresponding comments in the + ZulipSCIMUser implementation. + """ + + id_field = "usergroup_ptr_id" + + def __init__(self, obj: NamedUserGroup, request: HttpRequest | None = None) -> None: + assert request is not None + self.obj: NamedUserGroup + + super().__init__(obj, request) + + self.subdomain = get_subdomain(request) + self.config = settings.SCIM_CONFIG[self.subdomain] + + realm = get_realm_from_request(request) + assert realm is not None + self.realm: Realm = realm + + self._name_new_value: str | None = None + + # The (_member_ids_to_add, _member_ids_to_remove) pair and _intended_member_ids + # are mutually exclusive. A PUT request or PATCH request with the "replace" + # operation to update a group will specify the + # full set of member ids that should belong to the group, thus setting + # _intended_member_ids. + # A PATCH request can specify "add" and/or "remove" operations, which will + # set _member_ids_to_add and/or _member_ids_to_remove. + # + # Hypothetically, a PATCH request could specify all of "add", "remove" + # and "replace" operations at once, in any order. We do not support + # such a mix for now, and it's not something commonly used by SCIM clients, + # if at all. + # If necessary, this isn't too hard to implement however, and can be done + # by sequencing thunks for each of the operations in the PATCH request, + # to be executed in the .save() method, instead of this current approach. + self._member_ids_to_add: set[int] | None = None + self._member_ids_to_remove: set[int] | None = None + self._intended_member_ids: set[int] | None = None + + @property + def display_name(self) -> str: + return self.obj.name + + @property + def members(self) -> list[dict[str, object]]: + """ + Return a list of user dicts (ready for serialization) for the members + of the group. + + Overridden from the superclass to use our method for fetching group + members. + """ + users = UserProfile.objects.filter( + id__in=get_user_group_direct_member_ids(self.obj), is_bot=False, realm=self.realm + ).order_by("id") + scim_users: list[SCIMUser] = [ + scim_utils.get_user_adapter()(user, self.request) for user in users + ] + + dicts = [] + for user in scim_users: + d = { + "value": user.id, + "$ref": user.location, + "display": user.display_name, + "type": "User", + } + dicts.append(d) + + return dicts + + def is_new_group(self) -> bool: + return not bool(self.obj.id) + + def to_dict(self) -> dict[str, object]: + return { + "id": str(self.obj.id), + "schemas": [scim_constants.SchemaURI.GROUP], + "displayName": self.display_name, + # Groups in the process of being created don't have members. + "members": self.members if not self.is_new_group() else [], + "meta": self.meta, + } + + def from_dict(self, d: dict[str, Any]) -> None: + name = d.get("displayName") + if name is not None: + assert isinstance(name, str) + self.change_group_name(name) + + members = d.get("members") + if members: + self._intended_member_ids = {int(member_dict["value"]) for member_dict in members} + + def change_group_name(self, new_value: str) -> None: + if new_value and self.obj.name != new_value: + self._name_new_value = new_value + + def delete(self) -> None: + if not check_can_manage_group_by_scim(self.obj): + raise scim_exceptions.BadRequestError( + f"Group {self.obj.name} can't be managed by SCIM." + ) + + # TODO: We don't currently support DELETE requests for groups. The correct way to handle + # a DELETE would be to deactivate the group - but Zulip currently disallows deactivation + # of groups under certain conditions, such as "the group is used for a permission". + # + # To be able to process a DELETE request, we need to implement a function to forcibly + # deactivate a group, by correctly untangling it from all dependencies such as permissions + # or supergroups. + # See https://github.com/zulip/zulip/pull/34605 for current status of this work. + raise scim_exceptions.NotImplementedError + + def save(self) -> None: + realm = self.realm + assert realm is not None + + if not check_can_manage_group_by_scim(self.obj): + raise scim_exceptions.BadRequestError( + f"Group {self.obj.name} can't be managed by SCIM." + ) + + name_new_value = getattr(self, "_name_new_value", None) + intended_member_ids = getattr(self, "_intended_member_ids", None) + member_ids_to_remove = getattr(self, "_member_ids_to_remove", None) + member_ids_to_add = getattr(self, "_member_ids_to_add", None) + + # Reset the state of pending changes. + self._name_new_value = None + self._intended_member_ids = None + self._member_ids_to_remove = None + self._member_ids_to_add = None + + if name_new_value is not None: + try: + check_user_group_name(name_new_value) + except JsonableError as e: + raise scim_exceptions.BadRequestError(e.msg) + if NamedUserGroup.objects.filter(name=name_new_value, realm=realm).exists(): + raise ConflictError("Group name already in use: " + name_new_value) + + # At most one of the three should be set for a .save() call. If the SCIM request has multiple operations + # on group memberships to run (e.g. "add" some users and "remove" some users), + # .save() is called sequentially, processing one operation at a time. + assert ( + sum( + value is not None + for value in [intended_member_ids, member_ids_to_remove, member_ids_to_add] + ) + <= 1 + ) + if intended_member_ids is not None: + validate_group_member_ids_from_request(realm, intended_member_ids) + elif member_ids_to_remove is not None: + validate_group_member_ids_from_request(realm, member_ids_to_remove) + elif member_ids_to_add is not None: + validate_group_member_ids_from_request(realm, member_ids_to_add) + + if self.is_new_group(): + if intended_member_ids is not None: + members = list(UserProfile.objects.filter(id__in=intended_member_ids, realm=realm)) + else: + members = [] + + system_groups_name_dict = get_role_based_system_groups_dict(realm) + group_nobody = system_groups_name_dict[SystemGroups.NOBODY].usergroup_ptr + group_settings_map = dict( + can_add_members_group=group_nobody, + can_manage_group=group_nobody, + ) + assert name_new_value is not None + self.obj = create_user_group_in_database( + name_new_value, + members, + realm, + description="Created from SCIM", + group_settings_map=group_settings_map, + acting_user=None, + ) + return + + with transaction.atomic(savepoint=False): + # We need to lock the group now to conduct update operations without race conditions. + user_group = NamedUserGroup.objects.select_for_update().get(id=self.obj.id) + current_member_ids = set(get_user_group_direct_member_ids(user_group)) + if name_new_value is not None: + do_update_user_group_name(self.obj, name_new_value, acting_user=None) + if intended_member_ids is not None: + current_member_ids = set(get_user_group_direct_member_ids(user_group)) + member_ids_to_remove = current_member_ids - intended_member_ids + member_ids_to_add = intended_member_ids - current_member_ids + + if member_ids_to_remove: + # Clear out ids of users who have already been removed from the group. + member_ids_to_remove = member_ids_to_remove.intersection(current_member_ids) + bulk_remove_members_from_user_groups( + [user_group], list(member_ids_to_remove), acting_user=None + ) + if member_ids_to_add: + # Clear out ids of users who are already in the group. + member_ids_to_add = member_ids_to_add - current_member_ids + bulk_add_members_to_user_groups( + [user_group], list(member_ids_to_add), acting_user=None + ) + + def handle_replace( + self, + path: AttrPath, + value: str | list[Any] | dict[AttrPath, Any], + operation: Any, + ) -> None: + if not isinstance(value, dict): + value = {path: value} + + assert isinstance(value, dict) + for attr_path, val in value.items(): + if attr_path.first_path == ("displayName", None, None): + name = val + assert isinstance(name, str) + self.change_group_name(name) + elif attr_path.first_path == ("members", None, None): + intended_member_ids = {int(user_dict["value"]) for user_dict in val} + self._intended_member_ids = intended_member_ids + elif attr_path.first_path == ("id", None, None): + # the "id", if present in the request, should just match the id + # of the group - so this is a sanity check. + assert int(val) == self.obj.id + else: # nocoverage + raise scim_exceptions.NotImplementedError + + self.save() + + def handle_add( + self, + path: AttrPath, + value: str | list[Any] | dict[AttrPath, Any], + operation: Any, + ) -> None: + assert path is not None + if path.first_path == ("members", None, None): + members = value or [] + assert isinstance(members, list) + self._member_ids_to_add = {int(member.get("value")) for member in members} + else: # nocoverage + raise scim_exceptions.NotImplementedError + + self.save() + + def handle_remove( + self, + path: AttrPath, + value: str | list[Any] | dict[AttrPath, Any], + operation: Any, + ) -> None: + assert path is not None + + if path.is_complex: + # django-scim2 does not support handling of complex paths and thus we generally + # don't support them either - as they're not used by our supported SCIM clients. + # The exception is Okta requests to remove a user from a group. + # Rather than a PATCH request with a simple path of the form + # { ..., "path": "members", "value": [{"value": ""}] } + # Okta sends a request specifying the user to remove in a complex path: + # { ..., "path": 'members[value eq ""]' } + # + # We don't attempt to implement general handling of complex paths. Instead, + # we just add a hacky approach for detecting and handling this single, specific + # kind of request. + # + # HACK: Detect the strange filter query formed by django-scim2 when preparing + # to parse the path in self.split_path(). + match = re.match(r'^members\[value eq "(\d+)"\] eq ""$', path.filter) + if not match: # nocoverage + raise scim_exceptions.NotImplementedError + + user_profile_id = int(match.group(1)) + self._member_ids_to_remove = {user_profile_id} + elif path.first_path == ("members", None, None): + members = value or [] + assert isinstance(members, list) + self._member_ids_to_remove = {int(member.get("value")) for member in members} + else: # nocoverage + raise scim_exceptions.NotImplementedError + + self.save() + + def get_extra_model_filter_kwargs_getter( model: type[models.Model], ) -> Callable[[HttpRequest, Any, Any], dict[str, object]]: @@ -365,7 +707,17 @@ def get_extra_model_filter_kwargs_getter( ) -> dict[str, object]: realm = RequestNotes.get_notes(request).realm assert realm is not None - return {"realm_id": realm.id, "is_bot": False} + extra_kwargs: dict[str, object] = {"realm_id": realm.id} + # We need to determine if it's /Users or /Groups being queried. + url_name = resolve(request.path).url_name + if url_name in ["users", "users-search"]: + extra_kwargs.update({"is_bot": False}) + elif url_name == "groups": + extra_kwargs.update({"deactivated": False}) + else: + raise AssertionError + + return extra_kwargs return get_extra_filter_kwargs diff --git a/zerver/lib/scim_filter.py b/zerver/lib/scim_filter.py index 2ebd565e00..38958c3b10 100644 --- a/zerver/lib/scim_filter.py +++ b/zerver/lib/scim_filter.py @@ -1,5 +1,5 @@ from django.http import HttpRequest -from django_scim.filters import UserFilterQuery +from django_scim.filters import GroupFilterQuery, UserFilterQuery from zerver.lib.request import RequestNotes @@ -55,3 +55,23 @@ class ZulipUserFilterQuery(UserFilterQuery): "AND zerver_userprofile.realm_id = %s AND zerver_userprofile.is_bot = False ORDER BY zerver_userprofile.id", [realm.id], ) + + +class ZulipGroupFilterQuery(GroupFilterQuery): + attr_map = { + ("displayName", None, None): "zerver_namedusergroup.name", + } + + @classmethod + def get_extras(cls, q: str, request: HttpRequest | None = None) -> tuple[str, list[object]]: + """ + Here we ensure that results are limited to the subdomain of the request. + """ + assert request is not None + realm = RequestNotes.get_notes(request).realm + assert realm is not None + + return ( + "AND zerver_namedusergroup.realm_id = %s AND zerver_namedusergroup.deactivated = False ORDER BY zerver_namedusergroup.usergroup_ptr_id", + [realm.id], + ) diff --git a/zerver/tests/test_scim.py b/zerver/tests/test_scim.py index 2540c3aa31..eb303181f8 100644 --- a/zerver/tests/test_scim.py +++ b/zerver/tests/test_scim.py @@ -8,10 +8,13 @@ import orjson from django.conf import settings from typing_extensions import override +from zerver.actions.user_groups import check_add_user_group, do_deactivate_user_group from zerver.actions.user_settings import do_change_full_name from zerver.lib.stream_subscription import get_subscribed_stream_ids_for_user from zerver.lib.test_classes import ZulipTestCase +from zerver.lib.user_groups import get_user_group_direct_member_ids from zerver.models import UserProfile +from zerver.models.groups import NamedUserGroup from zerver.models.realms import get_realm if TYPE_CHECKING: @@ -830,15 +833,664 @@ class TestSCIMGroup(SCIMTestCase): to actually test desired behavior. """ + def generate_group_schema(self, user_group: NamedUserGroup) -> dict[str, Any]: + return { + "schemas": ["urn:ietf:params:scim:schemas:core:2.0:Group"], + "id": str(user_group.id), + "displayName": user_group.name, + "members": [ + { + "value": str(user_profile.id), + "$ref": f"http://zulip.testserver/scim/v2/Users/{user_profile.id}", + "display": user_profile.full_name, + "type": "User", + } + for user_profile in UserProfile.objects.filter( + id__in=get_user_group_direct_member_ids(user_group), is_bot=False + ).order_by("id") + ], + "meta": { + "resourceType": "Group", + "location": f"http://zulip.testserver/scim/v2/Groups/{user_group.id}", + }, + } + + def test_get_by_id(self) -> None: + realm = get_realm("zulip") + desdemona = self.example_user("desdemona") + hamlet = self.example_user("hamlet") + bot = self.create_test_bot("whatever", hamlet) + + # We include a bot in the group memberships to verify that bots are ignored. + test_group = check_add_user_group(realm, "Test group", [hamlet, bot], acting_user=desdemona) + + expected_data = self.generate_group_schema(test_group) + result = self.client_get(f"/scim/v2/Groups/{test_group.id}", {}, **self.scim_headers()) + + self.assertEqual(result.status_code, 200) + + output_data = orjson.loads(result.content) + self.assertEqual(output_data, expected_data) + + member_ids = [int(user_dict["value"]) for user_dict in output_data["members"]] + self.assertIn(hamlet.id, member_ids) + self.assertNotIn(bot.id, member_ids) + + def test_get_basic_filter_by_display_name(self) -> None: + realm = get_realm("zulip") + hamlet = self.example_user("hamlet") + desdemona = self.example_user("desdemona") + test_group = check_add_user_group(realm, "Test group", [hamlet], acting_user=desdemona) + + expected_response_schema = { + "schemas": ["urn:ietf:params:scim:api:messages:2.0:ListResponse"], + "totalResults": 1, + "itemsPerPage": 50, + "startIndex": 1, + "Resources": [self.generate_group_schema(test_group)], + } + + result = self.client_get( + f'/scim/v2/Groups?filter=displayName eq "{test_group.name}"', + {}, + **self.scim_headers(), + ) + self.assertEqual(result.status_code, 200) + + output_data = orjson.loads(result.content) + self.assertEqual(output_data, expected_response_schema) + + # Deactivated groups are invisible to SCIM. + do_deactivate_user_group(test_group, acting_user=None) + + expected_response_schema = { + "schemas": ["urn:ietf:params:scim:api:messages:2.0:ListResponse"], + "totalResults": 0, + "itemsPerPage": 50, + "startIndex": 1, + "Resources": [], + } + result = self.client_get( + f'/scim/v2/Groups?filter=displayName eq "{test_group.name}"', + {}, + **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_all_with_pagination(self) -> None: + realm = get_realm("zulip") + + result_all = self.client_get("/scim/v2/Groups", {}, **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": NamedUserGroup.objects.filter(realm=realm).count(), + "itemsPerPage": 50, + "startIndex": 1, + "Resources": [ + self.generate_group_schema(group) + for group in NamedUserGroup.objects.filter(realm=realm).order_by("id") + ], + } + + self.assertEqual(output_data_all, expected_response_schema) + + # Next we test pagination, just like in TestSCIMUser.test_get_all_with_pagination. + result_offset_limited = self.client_get( + "/scim/v2/Groups?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_post(self) -> None: + payload: dict[str, Any] = { + "schemas": ["urn:ietf:params:scim:schemas:core:2.0:Group"], + "displayName": "New group without members", + "members": [], + } + original_group_count = NamedUserGroup.objects.count() + result = self.client_post( + "/scim/v2/Groups", payload, content_type="application/json", **self.scim_headers() + ) + self.assertEqual(result.status_code, 201) + output_data = orjson.loads(result.content) + + self.assertEqual(NamedUserGroup.objects.count(), original_group_count + 1) + + new_group = NamedUserGroup.objects.latest("id") + self.assertEqual(new_group.name, "New group without members") + self.assertEqual(set(get_user_group_direct_member_ids(new_group)), set()) + + expected_response_schema = self.generate_group_schema(new_group) + self.assertEqual(expected_response_schema, output_data) + + hamlet = self.example_user("hamlet") + payload = { + "schemas": ["urn:ietf:params:scim:schemas:core:2.0:Group"], + "displayName": "New group with members", + "members": [{"value": str(hamlet.id)}], + } + result = self.client_post( + "/scim/v2/Groups", payload, content_type="application/json", **self.scim_headers() + ) + self.assertEqual(result.status_code, 201) + output_data = orjson.loads(result.content) + + self.assertEqual(NamedUserGroup.objects.count(), original_group_count + 2) + + new_group = NamedUserGroup.objects.latest("id") + self.assertEqual(new_group.name, "New group with members") + self.assertEqual(set(get_user_group_direct_member_ids(new_group)), {hamlet.id}) + + expected_response_schema = self.generate_group_schema(new_group) + self.assertEqual(expected_response_schema, output_data) + + # Can't create a group with a name that already matches another group. + payload = { + "schemas": ["urn:ietf:params:scim:schemas:core:2.0:Group"], + "displayName": new_group.name, + "members": [{"value": str(hamlet.id)}], + } + result = self.client_post( + "/scim/v2/Groups", payload, content_type="application/json", **self.scim_headers() + ) + self.assertEqual( + orjson.loads(result.content), + { + "schemas": ["urn:ietf:params:scim:api:messages:2.0:Error"], + "detail": f"Group name already in use: {new_group.name}", + "status": 409, + "scimType": "uniqueness", + }, + ) + + def test_put_change_name_and_members(self) -> None: + realm = get_realm("zulip") + hamlet = self.example_user("hamlet") + desdemona = self.example_user("desdemona") + othello = self.example_user("othello") + + test_group = check_add_user_group( + realm, "Test group", [hamlet, desdemona], acting_user=desdemona + ) + + # PUT replaces all specified attributes of the group. Thus, + # this payload will replace both the name and set of members of + # the group. + payload = { + "schemas": ["urn:ietf:params:scim:schemas:core:2.0:Group"], + "id": test_group.id, + "displayName": "New test group name", + "members": [{"value": str(hamlet.id)}, {"value": str(othello.id)}], + } + result = self.json_put(f"/scim/v2/Groups/{test_group.id}", payload, **self.scim_headers()) + self.assertEqual(result.status_code, 200) + + test_group.refresh_from_db() + self.assertEqual(test_group.name, "New test group name") + new_member_ids = get_user_group_direct_member_ids(test_group) + self.assertEqual(set(new_member_ids), {hamlet.id, othello.id}) + + output_data = orjson.loads(result.content) + expected_response_schema = self.generate_group_schema(test_group) + self.assertEqual(output_data, expected_response_schema) + + conflicting_group = check_add_user_group( + realm, "Conflicting group", [hamlet, desdemona], acting_user=desdemona + ) + payload = { + "schemas": ["urn:ietf:params:scim:schemas:core:2.0:Group"], + "id": test_group.id, + "displayName": conflicting_group.name, + "members": [{"value": str(hamlet.id)}, {"value": str(othello.id)}], + } + result = self.json_put(f"/scim/v2/Groups/{test_group.id}", payload, **self.scim_headers()) + self.assertEqual( + orjson.loads(result.content), + { + "schemas": ["urn:ietf:params:scim:api:messages:2.0:Error"], + "detail": f"Group name already in use: {conflicting_group.name}", + "status": 409, + "scimType": "uniqueness", + }, + ) + + # Deactivated groups are invisible to SCIM. + do_deactivate_user_group(test_group, acting_user=None) + + payload = { + "schemas": ["urn:ietf:params:scim:schemas:core:2.0:Group"], + "id": test_group.id, + "displayName": "some new name - but group should not get updated", + "members": [{"value": str(hamlet.id)}, {"value": str(othello.id)}], + } + result = self.json_put(f"/scim/v2/Groups/{test_group.id}", payload, **self.scim_headers()) + self.assertEqual( + orjson.loads(result.content), + { + "schemas": ["urn:ietf:params:scim:api:messages:2.0:Error"], + "detail": f"Resource {test_group.id} not found", + "status": 404, + }, + ) + test_group.refresh_from_db() + self.assertEqual(test_group.name, "New test group name") + + def test_patch_add_remove_operations(self) -> None: + realm = get_realm("zulip") + hamlet = self.example_user("hamlet") + desdemona = self.example_user("desdemona") + othello = self.example_user("othello") + cordelia = self.example_user("cordelia") + iago = self.example_user("iago") + + test_group = check_add_user_group( + realm, "Test group", [hamlet, desdemona], acting_user=desdemona + ) + + payload = { + "schemas": ["urn:ietf:params:scim:api:messages:2.0:PatchOp"], + "Operations": [ + { + "op": "add", + "path": "members", + "value": [ + {"value": str(othello.id)}, + {"value": str(cordelia.id)}, + # Include a user who's already in the group in this list, to verify that trying to add + # a user who's already in the group is gracefully ignored. + {"value": str(desdemona.id)}, + ], + } + ], + } + + result = self.json_patch(f"/scim/v2/Groups/{test_group.id}", payload, **self.scim_headers()) + self.assertEqual(result.status_code, 200) + + self.assertEqual( + set(get_user_group_direct_member_ids(test_group)), + {hamlet.id, desdemona.id, othello.id, cordelia.id}, + ) + + output_data = orjson.loads(result.content) + expected_response_schema = self.generate_group_schema(test_group) + self.assertEqual(output_data, expected_response_schema) + + payload = { + "schemas": ["urn:ietf:params:scim:api:messages:2.0:PatchOp"], + "Operations": [ + { + "op": "remove", + "path": "members", + # Similarly to the user addition case, here, for user removal, + # we include a user who's not in the group. Trying to remove a user + # who's not in the group should gracefully be ignored. + "value": [ + {"value": str(othello.id)}, + {"value": str(cordelia.id)}, + {"value": str(iago.id)}, + ], + } + ], + } + + result = self.json_patch(f"/scim/v2/Groups/{test_group.id}", payload, **self.scim_headers()) + self.assertEqual(result.status_code, 200) + + self.assertEqual( + set(get_user_group_direct_member_ids(test_group)), {hamlet.id, desdemona.id} + ) + + output_data = orjson.loads(result.content) + expected_response_schema = self.generate_group_schema(test_group) + self.assertEqual(output_data, expected_response_schema) + + # Now try a sequence of operations and verify the final result + # is as expected. + payload = { + "schemas": ["urn:ietf:params:scim:api:messages:2.0:PatchOp"], + "Operations": [ + { + "op": "add", + "path": "members", + "value": [{"value": str(othello.id)}, {"value": str(cordelia.id)}], + }, + { + "op": "remove", + "path": "members", + "value": [{"value": str(desdemona.id)}, {"value": str(hamlet.id)}], + }, + { + "op": "add", + "path": "members", + "value": [{"value": str(hamlet.id)}], + }, + ], + } + + result = self.json_patch(f"/scim/v2/Groups/{test_group.id}", payload, **self.scim_headers()) + self.assertEqual(result.status_code, 200) + + self.assertEqual( + set(get_user_group_direct_member_ids(test_group)), {hamlet.id, othello.id, cordelia.id} + ) + + output_data = orjson.loads(result.content) + expected_response_schema = self.generate_group_schema(test_group) + self.assertEqual(output_data, expected_response_schema) + + def test_patch_remove_operation_with_complex_path(self) -> None: + # We generally don't support PATCH requests with complex paths. + # but have to make an exception for "remove a user from a group" + # requests from Okta, which for some reason are sent with + # complex paths. + # + # See ZulipSCIMGroup.handle_remove for details. + realm = get_realm("zulip") + hamlet = self.example_user("hamlet") + desdemona = self.example_user("desdemona") + + test_group = check_add_user_group( + realm, "Test group", [hamlet, desdemona], acting_user=desdemona + ) + payload = { + "schemas": ["urn:ietf:params:scim:api:messages:2.0:PatchOp"], + "Operations": [ + { + "op": "remove", + "path": f'members[value eq "{hamlet.id!s}"]', + } + ], + } + + result = self.json_patch(f"/scim/v2/Groups/{test_group.id}", payload, **self.scim_headers()) + self.assertEqual(result.status_code, 200) + + self.assertEqual(set(get_user_group_direct_member_ids(test_group)), {desdemona.id}) + + output_data = orjson.loads(result.content) + expected_response_schema = self.generate_group_schema(test_group) + self.assertEqual(output_data, expected_response_schema) + + def test_patch_replace_operation(self) -> None: + realm = get_realm("zulip") + hamlet = self.example_user("hamlet") + desdemona = self.example_user("desdemona") + othello = self.example_user("othello") + + test_group = check_add_user_group( + realm, "Test group", [hamlet, desdemona], acting_user=desdemona + ) + + payload = { + "schemas": ["urn:ietf:params:scim:api:messages:2.0:PatchOp"], + "Operations": [ + {"op": "replace", "path": "displayName", "value": "New test group name"} + ], + } + + result = self.json_patch(f"/scim/v2/Groups/{test_group.id}", payload, **self.scim_headers()) + self.assertEqual(result.status_code, 200) + + test_group.refresh_from_db() + self.assertEqual(test_group.name, "New test group name") + + output_data = orjson.loads(result.content) + expected_response_schema = self.generate_group_schema(test_group) + self.assertEqual(output_data, expected_response_schema) + + # Next try editing members. + payload = { + "schemas": ["urn:ietf:params:scim:api:messages:2.0:PatchOp"], + "Operations": [ + { + "op": "replace", + "path": "members", + "value": [ + {"value": str(hamlet.id)}, + {"value": str(othello.id)}, + ], + } + ], + } + result = self.json_patch(f"/scim/v2/Groups/{test_group.id}", payload, **self.scim_headers()) + self.assertEqual(result.status_code, 200) + + self.assertEqual(set(get_user_group_direct_member_ids(test_group)), {hamlet.id, othello.id}) + + output_data = orjson.loads(result.content) + expected_response_schema = self.generate_group_schema(test_group) + self.assertEqual(output_data, expected_response_schema) + + # The operation might also specify the attributes to replace by passing a dict + # in the "value" key, instead of providing a "path". + payload = { + "schemas": ["urn:ietf:params:scim:api:messages:2.0:PatchOp"], + "Operations": [ + { + "op": "replace", + "value": {"id": str(test_group.id), "displayName": "New test group name 2"}, + } + ], + } + + result = self.json_patch(f"/scim/v2/Groups/{test_group.id}", payload, **self.scim_headers()) + self.assertEqual(result.status_code, 200) + + test_group.refresh_from_db() + self.assertEqual(test_group.name, "New test group name 2") + + output_data = orjson.loads(result.content) + expected_response_schema = self.generate_group_schema(test_group) + self.assertEqual(output_data, expected_response_schema) + + # Finally, update both SCIM attributes (displayName and members) in a single operation. + payload = { + "schemas": ["urn:ietf:params:scim:api:messages:2.0:PatchOp"], + "Operations": [ + { + "op": "replace", + "value": { + "id": str(test_group.id), + "displayName": "New test group name 3", + "members": [ + {"value": str(hamlet.id)}, + {"value": str(othello.id)}, + {"value": str(desdemona.id)}, + ], + }, + } + ], + } + result = self.json_patch(f"/scim/v2/Groups/{test_group.id}", payload, **self.scim_headers()) + self.assertEqual(result.status_code, 200) + + test_group.refresh_from_db() + self.assertEqual(test_group.name, "New test group name 3") + self.assertEqual( + set(get_user_group_direct_member_ids(test_group)), {hamlet.id, othello.id, desdemona.id} + ) + + def test_patch_add_user_wrong_realm(self) -> None: + realm = get_realm("zulip") + hamlet = self.example_user("hamlet") + mit_user = self.mit_user("sipbtest") + desdemona = self.example_user("desdemona") + othello = self.example_user("othello") + + test_group = check_add_user_group( + realm, "Test group", [hamlet, desdemona], acting_user=desdemona + ) + + payload = { + "schemas": ["urn:ietf:params:scim:api:messages:2.0:PatchOp"], + "Operations": [ + { + "op": "add", + "path": "members", + "value": [ + {"value": str(othello.id)}, + # Also include an id of a user from a different realm. + {"value": str(mit_user.id)}, + ], + } + ], + } + + result = self.json_patch(f"/scim/v2/Groups/{test_group.id}", payload, **self.scim_headers()) + self.assertEqual( + orjson.loads(result.content), + { + "schemas": ["urn:ietf:params:scim:api:messages:2.0:Error"], + "detail": "Users outside of the realm can't be removed or added to the group", + "status": 400, + }, + ) + + payload = { + "schemas": ["urn:ietf:params:scim:api:messages:2.0:PatchOp"], + "Operations": [ + { + "op": "add", + "path": "members", + "value": [ + # Try a request with just the id of a user from a different realm. + {"value": str(mit_user.id)}, + ], + } + ], + } + + result = self.json_patch(f"/scim/v2/Groups/{test_group.id}", payload, **self.scim_headers()) + self.assertEqual( + orjson.loads(result.content), + { + "schemas": ["urn:ietf:params:scim:api:messages:2.0:Error"], + "detail": "Users outside of the realm can't be removed or added to the group", + "status": 400, + }, + ) + + # The requests failed, so there's no change in memberships. + self.assertEqual( + set(get_user_group_direct_member_ids(test_group)), + {hamlet.id, desdemona.id}, + ) + + def test_patch_add_invalid_user_id(self) -> None: + realm = get_realm("zulip") + hamlet = self.example_user("hamlet") + desdemona = self.example_user("desdemona") + othello = self.example_user("othello") + + test_group = check_add_user_group( + realm, "Test group", [hamlet, desdemona], acting_user=desdemona + ) + + payload = { + "schemas": ["urn:ietf:params:scim:api:messages:2.0:PatchOp"], + "Operations": [ + { + "op": "add", + "path": "members", + "value": [ + {"value": str(othello.id)}, + # Also include an id that doesn't match any user. + {"value": "987654321"}, + ], + } + ], + } + + result = self.json_patch(f"/scim/v2/Groups/{test_group.id}", payload, **self.scim_headers()) + self.assertEqual( + orjson.loads(result.content), + { + "schemas": ["urn:ietf:params:scim:api:messages:2.0:Error"], + "detail": "Invalid user ids found in the request: {987654321}", + "status": 400, + }, + ) + + # The request failed, so there's no change in memberships. + self.assertEqual( + set(get_user_group_direct_member_ids(test_group)), + {hamlet.id, desdemona.id}, + ) + + def test_cant_edit_system_groups(self) -> None: + """ + Verifies that system groups are not allowed to be managed by SCIM requests. + """ + realm = get_realm("zulip") + system_group = NamedUserGroup.objects.get(realm=realm, name="role:owners") + + payload = { + "schemas": ["urn:ietf:params:scim:api:messages:2.0:PatchOp"], + "Operations": [ + {"op": "replace", "path": "displayName", "value": "New test group name"} + ], + } + + result = self.json_patch( + f"/scim/v2/Groups/{system_group.id}", payload, **self.scim_headers() + ) + self.assertEqual( + orjson.loads(result.content), + { + "schemas": ["urn:ietf:params:scim:api:messages:2.0:Error"], + "detail": "Group role:owners can't be managed by SCIM.", + "status": 400, + }, + ) + system_group.refresh_from_db() + self.assertEqual(system_group.name, "role:owners") + + result = self.client_delete(f"/scim/v2/Groups/{system_group.id}", **self.scim_headers()) + self.assertEqual( + orjson.loads(result.content), + { + "schemas": ["urn:ietf:params:scim:api:messages:2.0:Error"], + "detail": "Group role:owners can't be managed by SCIM.", + "status": 400, + }, + ) + self.assertTrue(NamedUserGroup.objects.filter(realm=realm, name="role:owners").exists()) + + payload = { + "schemas": ["urn:ietf:params:scim:schemas:core:2.0:Group"], + "displayName": "role:doesntexistyet", + "members": [], + } + original_group_count = NamedUserGroup.objects.count() + result = self.client_post( + "/scim/v2/Groups", payload, content_type="application/json", **self.scim_headers() + ) + self.assertEqual( + orjson.loads(result.content), + { + "schemas": ["urn:ietf:params:scim:api:messages:2.0:Error"], + "detail": "User group name cannot start with 'role:'.", + "status": 400, + }, + ) + self.assertEqual(original_group_count, NamedUserGroup.objects.count()) + 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", @@ -851,6 +1503,24 @@ class TestSCIMGroup(SCIMTestCase): m.output, ["ERROR:django.request:Not Implemented: /scim/v2/Groups/.search"] ) + def test_delete(self) -> None: + # The DELETE endpoint is currently disabled. + + realm = get_realm("zulip") + hamlet = self.example_user("hamlet") + desdemona = self.example_user("desdemona") + + test_group = check_add_user_group( + realm, "Test group", [hamlet, desdemona], acting_user=desdemona + ) + + with self.assertLogs("django.request", "ERROR") as m: + result = self.client_delete(f"/scim/v2/Groups/{test_group.id}", **self.scim_headers()) + self.assertEqual(result.status_code, 501) + self.assertEqual( + m.output, [f"ERROR:django.request:Not Implemented: /scim/v2/Groups/{test_group.id}"] + ) + class TestRemainingUnsupportedSCIMFeatures(SCIMTestCase): def test_endpoints_disabled(self) -> None: diff --git a/zproject/computed_settings.py b/zproject/computed_settings.py index ee349f80a7..f2dc79f266 100644 --- a/zproject/computed_settings.py +++ b/zproject/computed_settings.py @@ -1290,6 +1290,9 @@ SENTRY_DSN = os.environ.get("SENTRY_DSN", SENTRY_DSN) SCIM_SERVICE_PROVIDER = { "USER_ADAPTER": "zerver.lib.scim.ZulipSCIMUser", "USER_FILTER_PARSER": "zerver.lib.scim_filter.ZulipUserFilterQuery", + "GROUP_ADAPTER": "zerver.lib.scim.ZulipSCIMGroup", + "GROUP_MODEL": "zerver.models.groups.NamedUserGroup", + "GROUP_FILTER_PARSER": "zerver.lib.scim_filter.ZulipGroupFilterQuery", # NETLOC is actually overridden 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. diff --git a/zproject/urls.py b/zproject/urls.py index c45776bb83..edba6b8e8f 100644 --- a/zproject/urls.py +++ b/zproject/urls.py @@ -869,10 +869,6 @@ urls += [ 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), - ), re_path(r"^scim/v2/Me$", scim_views.SCIMView.as_view(implemented=False)), re_path( r"^scim/v2/ServiceProviderConfig$",