scim: Add supporting for syncing the user role.

This adds support for syncing user role via the newly added "role"
attribute, which can be set to either of
['owner', 'administrator', 'moderator', 'member', 'guest'].

Removes durable=True from the atomic decorator of do_change_user_role,
as django-scim2 runs PATCH operations in an atomic block.

help/scim.md

@@ -70,6 +70,20 @@ Zulip's SCIM integration has the following limitations:
      * **givenName**
      * **familyName**
 
+1. **Optional:** If you'd like to also sync [user role](/help/roles-and-permissions),
+   you can do it by by adding a custom attribute in Okta. Go to the **Profile Editor**,
+   click into the entry of the SCIM app you've just set up and **Add Attribute**.
+   Configure the following:
+    * **Data type**: `string`
+    * **Variable name**: `role`
+    * **External name**: `role`
+    * **External namespace**: `urn:ietf:params:scim:schemas:core:2.0:User`
+
+    With the attribute added, you will now be able to set it for your users directly
+    or configure an appropriate **Attribute mapping** in the app's **Provisioning**
+    section.
+    The valid values are: **owner**, **administrator**, **moderator**, **member**, **guest**.
+
 1. Now that the integration is ready to manage Zulip user accounts, **assign**
    users to the SCIM app.
      * When you assign a user, Okta will check if the account exists in your

zerver/actions/users.py

@@ -372,7 +372,7 @@ def send_stream_events_for_role_update(
         send_event_on_commit(user_profile.realm, event, [user_profile.id])
 
 
-@transaction.atomic(durable=True)
+@transaction.atomic(savepoint=False)
 def do_change_user_role(
     user_profile: UserProfile, value: int, *, acting_user: Optional[UserProfile]
 ) -> None:

zerver/lib/scim.py

@@ -11,7 +11,7 @@ from scim2_filter_parser.attr_paths import AttrPath
 
 from zerver.actions.create_user import do_create_user, do_reactivate_user
 from zerver.actions.user_settings import check_change_full_name, do_change_user_delivery_email
-from zerver.actions.users import do_deactivate_user
+from zerver.actions.users import do_change_user_role, do_deactivate_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
@@ -31,6 +31,15 @@ class ZulipSCIMUser(SCIMUser):
 
     id_field = "id"
 
+    ROLE_TYPE_TO_NAME = {
+        UserProfile.ROLE_REALM_OWNER: "owner",
+        UserProfile.ROLE_REALM_ADMINISTRATOR: "administrator",
+        UserProfile.ROLE_MODERATOR: "moderator",
+        UserProfile.ROLE_MEMBER: "member",
+        UserProfile.ROLE_GUEST: "guest",
+    }
+    ROLE_NAME_TO_TYPE = {v: k for k, v in ROLE_TYPE_TO_NAME.items()}
+
     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.
@@ -54,6 +63,7 @@ class ZulipSCIMUser(SCIMUser):
         self._email_new_value: Optional[str] = None
         self._is_active_new_value: Optional[bool] = None
         self._full_name_new_value: Optional[str] = None
+        self._role_new_value: Optional[int] = None
         self._password_set_to: Optional[str] = None
 
     def is_new_user(self) -> bool:
@@ -105,6 +115,7 @@ class ZulipSCIMUser(SCIMUser):
             "name": name,
             "displayName": self.display_name,
             "active": self.obj.is_active,
+            "role": self.ROLE_TYPE_TO_NAME[self.obj.role],
             # 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
@@ -166,6 +177,11 @@ class ZulipSCIMUser(SCIMUser):
             assert isinstance(active, bool)
             self.change_is_active(active)
 
+        role_name = d.get("role")
+        if role_name:
+            assert isinstance(role_name, str)
+            self.change_role(role_name)
+
     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().
@@ -181,6 +197,16 @@ class ZulipSCIMUser(SCIMUser):
         if new_value != self.obj.is_active:
             self._is_active_new_value = new_value
 
+    def change_role(self, new_role_name: str) -> None:
+        try:
+            role = self.ROLE_NAME_TO_TYPE[new_role_name]
+        except KeyError:
+            raise scim_exceptions.BadRequestError(
+                f"Invalid role: {new_role_name}. Valid values are: {list(self.ROLE_NAME_TO_TYPE.keys())}"
+            )
+        if role != self.obj.role:
+            self._role_new_value = role
+
     def handle_replace(
         self,
         path: Optional[AttrPath],
@@ -215,6 +241,9 @@ class ZulipSCIMUser(SCIMUser):
             elif path.first_path == ("active", None, None):
                 assert isinstance(val, bool)
                 self.change_is_active(val)
+            elif path.first_path == ("role", None, None):
+                assert isinstance(val, str)
+                self.change_role(val)
             else:
                 raise scim_exceptions.NotImplementedError("Not Implemented")
 
@@ -232,6 +261,7 @@ class ZulipSCIMUser(SCIMUser):
         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)
+        role_new_value = getattr(self, "_role_new_value", None)
         password = getattr(self, "_password_set_to", None)
 
         # Clean up the internal "pending change" state, now that we've
@@ -240,6 +270,7 @@ class ZulipSCIMUser(SCIMUser):
         self._is_active_new_value = None
         self._full_name_new_value = None
         self._password_set_to = None
+        self._role_new_value = None
 
         if email_new_value:
             try:
@@ -270,6 +301,7 @@ class ZulipSCIMUser(SCIMUser):
                 password,
                 realm,
                 full_name_new_value,
+                role=role_new_value,
                 tos_version=UserProfile.TOS_VERSION_BEFORE_FIRST_LOGIN,
                 acting_user=None,
             )
@@ -287,6 +319,9 @@ class ZulipSCIMUser(SCIMUser):
         if email_new_value:
             do_change_user_delivery_email(self.obj, email_new_value)
 
+        if role_new_value is not None:
+            do_change_user_role(self.obj, role_new_value, acting_user=None)
+
         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:

zerver/tests/test_scim.py

@@ -7,6 +7,7 @@ import orjson
 from django.conf import settings
 
 from zerver.actions.user_settings import do_change_full_name
+from zerver.lib.scim import ZulipSCIMUser
 from zerver.lib.test_classes import ZulipTestCase
 from zerver.models import UserProfile, get_realm
 
@@ -33,6 +34,7 @@ class SCIMTestCase(ZulipTestCase):
             "userName": user_profile.delivery_email,
             "name": {"formatted": user_profile.full_name},
             "displayName": user_profile.full_name,
+            "role": ZulipSCIMUser.ROLE_TYPE_TO_NAME[user_profile.role],
             "active": True,
             "meta": {
                 "resourceType": "User",
@@ -337,6 +339,49 @@ class TestSCIMUser(SCIMTestCase):
         assert new_user is not None
         self.assertEqual(new_user.delivery_email, "newuser@zulip.com")
         self.assertEqual(new_user.full_name, "New User")
+        self.assertEqual(new_user.role, UserProfile.ROLE_MEMBER)
+
+        expected_response_schema = self.generate_user_schema(new_user)
+        self.assertEqual(output_data, expected_response_schema)
+
+    def test_post_with_role(self) -> None:
+        # A payload for creating a new user with the specified account details, including
+        # specifying the role.
+
+        # Start with a payload with an invalid role value, to test error handling.
+        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,
+            "role": "wrongrole",
+        }
+
+        result = self.client_post(
+            "/scim/v2/Users", 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": "Invalid role: wrongrole. Valid values are: ['owner', 'administrator', 'moderator', 'member', 'guest']",
+                "status": 400,
+            },
+        )
+
+        # Now fix the role to make a valid request to create an administrator and proceed.
+        payload["role"] = "administrator"
+        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 = UserProfile.objects.last()
+        assert new_user is not None
+        self.assertEqual(new_user.delivery_email, "newuser@zulip.com")
+        self.assertEqual(new_user.role, UserProfile.ROLE_REALM_ADMINISTRATOR)
 
         expected_response_schema = self.generate_user_schema(new_user)
         self.assertEqual(output_data, expected_response_schema)
@@ -562,6 +607,28 @@ class TestSCIMUser(SCIMTestCase):
             result, f"['{cordelia.delivery_email} already has an account']"
         )
 
+    def test_put_change_user_role(self) -> None:
+        hamlet = self.example_user("hamlet")
+        hamlet_email = hamlet.delivery_email
+        self.assertEqual(hamlet.role, UserProfile.ROLE_MEMBER)
+
+        # This payload changes hamlet's role to administrator.
+        payload = {
+            "schemas": ["urn:ietf:params:scim:schemas:core:2.0:User"],
+            "id": hamlet.id,
+            "userName": hamlet_email,
+            "role": "administrator",
+        }
+        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.role, UserProfile.ROLE_REALM_ADMINISTRATOR)
+
+        output_data = orjson.loads(result.content)
+        expected_response_schema = self.generate_user_schema(hamlet)
+        self.assertEqual(output_data, expected_response_schema)
+
     def test_put_deactivate_reactivate_user(self) -> None:
         hamlet = self.example_user("hamlet")
         # This payload flips the active attribute to deactivate the user.
@@ -647,6 +714,20 @@ class TestSCIMUser(SCIMTestCase):
         expected_response_schema = self.generate_user_schema(hamlet)
         self.assertEqual(output_data, expected_response_schema)
 
+    def test_patch_change_user_role(self) -> None:
+        hamlet = self.example_user("hamlet")
+        # Payload for a PATCH request to change hamlet's role to administrator.
+        payload = {
+            "schemas": ["urn:ietf:params:scim:api:messages:2.0:PatchOp"],
+            "Operations": [{"op": "replace", "path": "role", "value": "administrator"}],
+        }
+
+        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.role, UserProfile.ROLE_REALM_ADMINISTRATOR)
+
     def test_patch_deactivate_reactivate_user(self) -> None:
         hamlet = self.example_user("hamlet")
         # Payload for a PATCH request to deactivate the user.