saml: Add support for syncing user role.

Replace the SOCIAL_AUTH_SYNC_CUSTOM_ATTRS_DICT with
SOCIAL_AUTH_SYNC_ATTRS_DICT, designed to support also regular user attrs
like role or full name (in the future).

Custom attributes can stay configured as they were and will get merged
into SOCIAL_AUTH_SYNC_ATTRS_DICT in computed_settings, or can be
specified in SOCIAL_AUTH_SYNC_ATTRS_DICT directly with "custom__"
prefix.

The role sync is plumbed through to user creation, so users can
immediately be created with their intended role as provided by the IdP
when they're creating their account, even when doing this flow without
an invitiation.

docs/overview/changelog.md

@@ -15,7 +15,12 @@ _Unreleased_
 
 #### Upgrade notes for 10.0
 
-- None yet.
+- The `SOCIAL_AUTH_SYNC_CUSTOM_ATTRS_DICT` setting is deprecated in favor of the
+  more general `SOCIAL_AUTH_SYNC_ATTRS_DICT` setting structure, but still works in
+  this release for a smooth upgrade experience. The new setting supports
+  synchronizing role, and otherwise functions like the old one, except Zulip
+  custom profile fields are referred to with the prefix `custom__`. See the updated
+  comment documentation in `/etc/zulip/settings.py` for details.
 
 ## Zulip Server 9.x series
 

zerver/tests/test_auth_backends.py

@@ -3193,7 +3193,7 @@ class SAMLAuthBackendTest(SocialAuthBase):
             ],
         )
 
-    def test_social_auth_custom_profile_field_sync(self) -> None:
+    def test_social_auth_profile_field_sync(self) -> None:
         birthday_field = CustomProfileField.objects.get(
             realm=self.user_profile.realm, name="Birthday"
         )
@@ -3202,25 +3202,32 @@ class SAMLAuthBackendTest(SocialAuthBase):
         ).value
 
         idps_dict = copy.deepcopy(settings.SOCIAL_AUTH_SAML_ENABLED_IDPS)
-        idps_dict["test_idp"]["extra_attrs"] = ["mobilePhone"]
+        idps_dict["test_idp"]["extra_attrs"] = ["mobilePhone", "zulip_role"]
 
         sync_custom_attrs_dict = {
             "zulip": {
                 "saml": {
-                    "phone_number": "mobilePhone",
+                    "custom__phone_number": "mobilePhone",
+                    "role": "zulip_role",
                 }
             }
         }
 
+        # Before we procee, verify the role, which is supposed to get synced, is like
+        # we expect.
+        self.assertEqual(self.user_profile.role, UserProfile.ROLE_MEMBER)
+
         with self.settings(
             SOCIAL_AUTH_SAML_ENABLED_IDPS=idps_dict,
-            SOCIAL_AUTH_SYNC_CUSTOM_ATTRS_DICT=sync_custom_attrs_dict,
+            SOCIAL_AUTH_SYNC_ATTRS_DICT=sync_custom_attrs_dict,
         ):
             account_data_dict = self.get_account_data_dict(email=self.email, name=self.name)
             result = self.social_auth_test(
                 account_data_dict,
                 subdomain="zulip",
-                extra_attributes=dict(mobilePhone=["123412341234"], birthday=["2021-01-01"]),
+                extra_attributes=dict(
+                    mobilePhone=["123412341234"], birthday=["2021-01-01"], zulip_role=["owner"]
+                ),
             )
         data = load_subdomain_token(result)
         self.assertEqual(data["email"], self.email)
@@ -3242,12 +3249,131 @@ class SAMLAuthBackendTest(SocialAuthBase):
         ).value
         self.assertEqual(new_birthday_field_value, old_birthday_field_value)
 
-    def test_social_auth_custom_profile_field_sync_custom_field_not_existing(self) -> None:
+        self.user_profile.refresh_from_db()
+        self.assertEqual(self.user_profile.role, UserProfile.ROLE_REALM_OWNER)
+
+        # Now test with an invalid role value.
+        idps_dict["test_idp"]["extra_attrs"] = ["zulip_role"]
         sync_custom_attrs_dict = {
             "zulip": {
                 "saml": {
-                    "title": "title",
+                    "role": "zulip_role",
-                    "phone_number": "mobilePhone",
+                }
+            }
+        }
+        with (
+            self.settings(
+                SOCIAL_AUTH_SAML_ENABLED_IDPS=idps_dict,
+                SOCIAL_AUTH_SYNC_ATTRS_DICT=sync_custom_attrs_dict,
+            ),
+            self.assertLogs(self.logger_string, level="WARNING") as m,
+        ):
+            account_data_dict = self.get_account_data_dict(email=self.email, name=self.name)
+            result = self.social_auth_test(
+                account_data_dict,
+                subdomain="zulip",
+                extra_attributes=dict(zulip_role=["wrongrole"]),
+            )
+
+        data = load_subdomain_token(result)
+        self.assertEqual(data["email"], self.email)
+        self.user_profile.refresh_from_db()
+        self.assertEqual(self.user_profile.role, UserProfile.ROLE_REALM_OWNER)
+        self.assertEqual(
+            m.output,
+            [
+                self.logger_output(
+                    f"Ignoring unsupported role value wrongrole for user {self.user_profile.id} in SOCIAL_AUTH_SYNC_ATTRS_DICT",
+                    type="warning",
+                )
+            ],
+        )
+
+        # Verify empty attribute is handled.
+        with self.settings(
+            SOCIAL_AUTH_SAML_ENABLED_IDPS=idps_dict,
+            SOCIAL_AUTH_SYNC_ATTRS_DICT=sync_custom_attrs_dict,
+        ):
+            account_data_dict = self.get_account_data_dict(email=self.email, name=self.name)
+            result = self.social_auth_test(
+                account_data_dict,
+                subdomain="zulip",
+                extra_attributes=dict(zulip_role=[""]),
+            )
+        data = load_subdomain_token(result)
+        self.assertEqual(data["email"], self.email)
+        self.user_profile.refresh_from_db()
+        self.assertEqual(self.user_profile.role, UserProfile.ROLE_REALM_OWNER)
+
+        # Disable syncing of role in SOCIAL_AUTH_SYNC_ATTRS_DICT, while keeping
+        # role in extra_attrs. This edge case means the attribute will be read from the
+        # data provided by the IdP, but won't be used for anything.
+        with self.settings(
+            SOCIAL_AUTH_SAML_ENABLED_IDPS=idps_dict,
+            SOCIAL_AUTH_SYNC_ATTRS_DICT={},
+        ):
+            account_data_dict = self.get_account_data_dict(email=self.email, name=self.name)
+            result = self.social_auth_test(
+                account_data_dict,
+                subdomain="zulip",
+                extra_attributes=dict(zulip_role=["guest"]),
+            )
+        data = load_subdomain_token(result)
+        self.assertEqual(data["email"], self.email)
+        self.user_profile.refresh_from_db()
+        self.assertEqual(self.user_profile.role, UserProfile.ROLE_REALM_OWNER)
+
+    @override_settings(TERMS_OF_SERVICE_VERSION=None)
+    def test_social_auth_create_user_with_synced_role(self) -> None:
+        email = "newuser@zulip.com"
+        name = "Full Name"
+        subdomain = "zulip"
+        realm = get_realm("zulip")
+
+        account_data_dict = self.get_account_data_dict(email=email, name=name)
+        idps_dict = copy.deepcopy(settings.SOCIAL_AUTH_SAML_ENABLED_IDPS)
+        idps_dict["test_idp"]["extra_attrs"] = ["zulip_role"]
+
+        sync_custom_attrs_dict = {
+            "zulip": {
+                "saml": {
+                    "role": "zulip_role",
+                }
+            }
+        }
+
+        with (
+            self.settings(
+                SOCIAL_AUTH_SAML_ENABLED_IDPS=idps_dict,
+                SOCIAL_AUTH_SYNC_ATTRS_DICT=sync_custom_attrs_dict,
+            ),
+            self.assertLogs(self.logger_string, level="INFO") as m,
+        ):
+            result = self.social_auth_test(
+                account_data_dict,
+                subdomain="zulip",
+                is_signup=True,
+                extra_attributes=dict(
+                    mobilePhone=["123412341234"], birthday=["2021-01-01"], zulip_role=["owner"]
+                ),
+            )
+
+        self.stage_two_of_registration(
+            result, realm, subdomain, email, name, name, self.BACKEND_CLASS.full_name_validated
+        )
+        user_profile = get_user_by_delivery_email(email, realm)
+        self.assertEqual(user_profile.role, UserProfile.ROLE_REALM_OWNER)
+        self.assertEqual(
+            m.output[0], self.logger_output("Returning role owner for user creation", type="info")
+        )
+
+    def test_social_auth_sync_field_not_existing(self) -> None:
+        sync_custom_attrs_dict = {
+            "zulip": {
+                "saml": {
+                    "custom__title": "title",
+                    "custom__phone_number": "mobilePhone",
+                    "wrongfield": "wrongfield",
                 }
             }
         }
@@ -3262,7 +3388,7 @@ class SAMLAuthBackendTest(SocialAuthBase):
 
         with self.settings(
             SOCIAL_AUTH_SAML_ENABLED_IDPS=idps_dict,
-            SOCIAL_AUTH_SYNC_CUSTOM_ATTRS_DICT=sync_custom_attrs_dict,
+            SOCIAL_AUTH_SYNC_ATTRS_DICT=sync_custom_attrs_dict,
         ):
             account_data_dict = self.get_account_data_dict(email=self.email, name=self.name)
             with self.assertLogs(self.logger_string, level="WARNING") as m:
@@ -3280,13 +3406,17 @@ class SAMLAuthBackendTest(SocialAuthBase):
         self.assertEqual(
             m.output,
             [
+                self.logger_output(
+                    "Ignoring unsupported UserProfile field wrongfield in SOCIAL_AUTH_SYNC_ATTRS_DICT",
+                    "warning",
+                ),
                 self.logger_output(
                     (
                         "Exception while syncing custom profile fields for user"
                         f" {self.user_profile.id}: Custom profile field with name title not found."
                     ),
                     "warning",
-                )
+                ),
             ],
         )
 

zerver/views/auth.py

@@ -158,6 +158,7 @@ def maybe_send_to_registration(
     email: str,
     *,
     full_name: str = "",
+    role: int | None = None,
     mobile_flow_otp: str | None = None,
     desktop_flow_otp: str | None = None,
     is_signup: bool = False,
@@ -171,6 +172,12 @@ def maybe_send_to_registration(
     the registration flow or the "continue to registration" flow,
     depending on is_signup, whether the email address can join the
     organization (checked in HomepageForm), and similar details.
+
+    Important: role, if specified as argument to this function,
+    takes precedence over anything else, as it is an explicit
+    statement of what the user should be created as, and is likely
+    being synced from some user management system tied to the
+    authentication method used.
     """
 
     # In the desktop and mobile registration flows, the sign up
@@ -189,7 +196,10 @@ def maybe_send_to_registration(
     # approach of putting something in PreregistrationUser, because
     # that would apply to future registration attempts on other
     # devices, e.g. just creating an account on the web on their laptop.
+
     assert not (mobile_flow_otp and desktop_flow_otp)
+    assert (role is None) or (role in PreregistrationUser.INVITE_AS.values())
+
     if mobile_flow_otp:
         set_expirable_session_var(
             request.session,
@@ -243,7 +253,7 @@ def maybe_send_to_registration(
         {"email": email},
         realm=realm,
         from_multiuse_invite=from_multiuse_invite,
-        invited_as=invited_as,
+        invited_as=role or invited_as,
     )
     if form.is_valid():
         # If the email address is allowed to sign up for an account in
@@ -294,6 +304,12 @@ def maybe_send_to_registration(
             prereg_user.streams.set(streams_to_subscribe)
         if include_realm_default_subscriptions is not None:
             prereg_user.include_realm_default_subscriptions = include_realm_default_subscriptions
+
+        if role is not None:
+            # As explained at the top of this function, role, if specified as argument,
+            # takes precedence over the role implied by the invitation.
+            prereg_user.invited_as = role
+        else:
             prereg_user.invited_as = invited_as
         prereg_user.multiuse_invite = multiuse_obj
         prereg_user.save()
@@ -333,6 +349,7 @@ def register_remote_user(request: HttpRequest, result: ExternalAuthResult) -> Ht
     kwargs_to_pass = [
         "email",
         "full_name",
+        "role",
         "mobile_flow_otp",
         "desktop_flow_otp",
         "is_signup",

zproject/backends.py

@@ -74,7 +74,7 @@ from zerver.actions.user_groups import (
     bulk_remove_members_from_user_groups,
 )
 from zerver.actions.user_settings import do_regenerate_api_key
-from zerver.actions.users import do_deactivate_user
+from zerver.actions.users import do_change_user_role, do_deactivate_user
 from zerver.lib.avatar import avatar_url, is_avatar_new
 from zerver.lib.avatar_hash import user_avatar_content_hash
 from zerver.lib.dev_ldap_directory import init_fakeldap
@@ -1413,6 +1413,7 @@ class ExternalAuthDataDict(TypedDict, total=False):
     subdomain: str
     full_name: str
     email: str
+    role: int | None
     is_signup: bool
     is_realm_creation: bool
     redirect_to: str
@@ -1630,6 +1631,89 @@ def redirect_deactivated_user_to_login(realm: Realm, email: str) -> HttpResponse
     return HttpResponseRedirect(redirect_url)
 
 
+def social_auth_sync_user_attributes(
+    realm: Realm, user_profile: UserProfile | None, extra_attrs: dict[str, Any], backend: Any
+) -> int | None:
+    """
+    Syncs user attributes based on the SOCIAL_AUTH_SYNC_ATTRS_DICT setting.
+    Only supports:
+    1. Syncing the role. This is plumbed through to user creation, so can be
+       used to immediately create new users with their role set based on an attribute
+       provided by the IdP.
+    2. Syncing custom attributes. This isn't supported for user creation,
+       so they'll only be synced during the user's next login, not during
+       signup.
+    """
+    # This is only supported for SAML right now, though the design
+    # is meant to be easy to extend this to other backends if desired.
+    # Unlike LDAP or SCIM, this hook can only do syncing during the authentication
+    # flow, as that's when the data is provided and we don't have a way to query
+    # for it otherwise.
+    assert backend.name == "saml"
+
+    attrs_by_backend = settings.SOCIAL_AUTH_SYNC_ATTRS_DICT.get(realm.subdomain, {})
+    profile_field_name_to_attr_name = attrs_by_backend.get(backend.name, {})
+    if not extra_attrs or not profile_field_name_to_attr_name:
+        return None
+
+    user_id = None
+    if user_profile is not None:
+        user_id = user_profile.id
+
+    custom_profile_field_name_to_value = {}
+    new_role = None
+    for field_name, attr_name in profile_field_name_to_attr_name.items():
+        if field_name == "role":
+            attr_value = extra_attrs.get(attr_name)
+            if not attr_value:
+                continue
+            if attr_value not in UserProfile.ROLE_API_NAME_TO_ID:
+                backend.logger.warning(
+                    "Ignoring unsupported role value %s for user %s in SOCIAL_AUTH_SYNC_ATTRS_DICT",
+                    attr_value,
+                    user_id,
+                )
+                continue
+            new_role = UserProfile.ROLE_API_NAME_TO_ID[attr_value]
+        elif field_name.startswith("custom__"):
+            custom_profile_field_name_to_value[field_name.removeprefix("custom__")] = (
+                extra_attrs.get(attr_name)
+            )
+        else:
+            backend.logger.warning(
+                "Ignoring unsupported UserProfile field %s in SOCIAL_AUTH_SYNC_ATTRS_DICT",
+                field_name,
+            )
+
+    if user_profile is None:
+        # We don't support user creation with custom profile fields, so just
+        # return role so that it can be plumbed through to the signup flow.
+        if new_role is not None:
+            backend.logger.info(
+                "Returning role %s for user creation", UserProfile.ROLE_ID_TO_API_NAME[new_role]
+            )
+        return new_role
+
+    # Based on the information collected above, sync what's needed for the user_profile.
+    old_role = user_profile.role
+    if new_role is not None and old_role != new_role:
+        do_change_user_role(user_profile, new_role, acting_user=None)
+        backend.logger.info(
+            "Set role %s for user %s", UserProfile.ROLE_ID_TO_API_NAME[new_role], user_profile.id
+        )
+
+    try:
+        sync_user_profile_custom_fields(user_profile, custom_profile_field_name_to_value)
+    except SyncUserError as e:
+        backend.logger.warning(
+            "Exception while syncing custom profile fields for user %s: %s",
+            user_profile.id,
+            str(e),
+        )
+
+    return None
+
+
 def social_associate_user_helper(
     backend: BaseAuth, return_data: dict[str, Any], *args: Any, **kwargs: Any
 ) -> HttpResponse | UserProfile | None:
@@ -1886,25 +1970,10 @@ def social_auth_finish(
         is_signup = False
 
     extra_attrs = return_data.get("extra_attrs", {})
-    attrs_by_backend = settings.SOCIAL_AUTH_SYNC_CUSTOM_ATTRS_DICT.get(realm.subdomain, {})
+    role_for_new_user = None
-    if user_profile is not None and extra_attrs and attrs_by_backend:
+    if extra_attrs:
-        # This is only supported for SAML right now, though the design
+        role_for_new_user = social_auth_sync_user_attributes(
-        # is meant to be easy to extend this to other backends if desired.
+            realm, user_profile, extra_attrs, backend
-        # Unlike with LDAP, here we can only do syncing during the authentication
-        # flow, as that's when the data is provided and we don't have a way to query
-        # for it otherwise.
-        assert backend.name == "saml"
-        custom_profile_field_name_to_attr_name = attrs_by_backend.get(backend.name, {})
-        custom_profile_field_name_to_value = {}
-        for field_name, attr_name in custom_profile_field_name_to_attr_name.items():
-            custom_profile_field_name_to_value[field_name] = extra_attrs.get(attr_name)
-        try:
-            sync_user_profile_custom_fields(user_profile, custom_profile_field_name_to_value)
-        except SyncUserError as e:
-            backend.logger.warning(
-                "Exception while syncing custom profile fields for user %s: %s",
-                user_profile.id,
-                str(e),
         )
 
     if user_profile:
@@ -1965,7 +2034,7 @@ def social_auth_finish(
         params_to_store_in_authenticated_session=backend.get_params_to_store_in_authenticated_session(),
     )
     if user_profile is None:
-        data_dict.update(dict(full_name=full_name, email=email_address))
+        data_dict.update(dict(full_name=full_name, email=email_address, role=role_for_new_user))
 
     result = ExternalAuthResult(user_profile=user_profile, data_dict=data_dict)
 

zproject/computed_settings.py

@@ -62,6 +62,8 @@ from .configured_settings import (
     SOCIAL_AUTH_SAML_ENABLED_IDPS,
     SOCIAL_AUTH_SAML_SECURITY_CONFIG,
     SOCIAL_AUTH_SUBDOMAIN,
+    SOCIAL_AUTH_SYNC_ATTRS_DICT,
+    SOCIAL_AUTH_SYNC_CUSTOM_ATTRS_DICT,
     STATIC_URL,
     SUBMIT_USAGE_STATISTICS,
     TORNADO_PORTS,
@@ -1181,6 +1183,25 @@ for idp_name, idp_dict in SOCIAL_AUTH_SAML_ENABLED_IDPS.items():
         path = f"/etc/zulip/saml/idps/{idp_name}.crt"
     idp_dict["x509cert"] = get_from_file_if_exists(path)
 
+
+def ensure_dict_path(d: dict[str, Any], keys: list[str]) -> None:
+    for key in keys:
+        if key not in d:
+            d[key] = {}
+        d = d[key]
+
+
+# Merge SOCIAL_AUTH_SYNC_CUSTOM_ATTRS_DICT into SOCIAL_AUTH_SYNC_ATTRS_DICT.
+# This is compat code for the original SOCIAL_AUTH_CUSTOM_ATTRS_DICT setting.
+# TODO/compatibility: Remove this for release Zulip 10.0.
+for subdomain, dict_for_subdomain in SOCIAL_AUTH_SYNC_CUSTOM_ATTRS_DICT.items():
+    for backend_name, custom_attrs_map in dict_for_subdomain.items():
+        ensure_dict_path(SOCIAL_AUTH_SYNC_ATTRS_DICT, [subdomain, backend_name])
+        for custom_attr_name, source_attr_name in custom_attrs_map.items():
+            SOCIAL_AUTH_SYNC_ATTRS_DICT[subdomain][backend_name][f"custom__{custom_attr_name}"] = (
+                source_attr_name
+            )
+
 SOCIAL_AUTH_PIPELINE = [
     "social_core.pipeline.social_auth.social_details",
     "zproject.backends.social_auth_associate_user",

zproject/default_settings.py

@@ -113,6 +113,7 @@ SOCIAL_AUTH_OIDC_ENABLED_IDPS: dict[str, OIDCIdPConfigDict] = {}
 SOCIAL_AUTH_OIDC_FULL_NAME_VALIDATED = False
 
 SOCIAL_AUTH_SYNC_CUSTOM_ATTRS_DICT: dict[str, dict[str, dict[str, str]]] = {}
+SOCIAL_AUTH_SYNC_ATTRS_DICT: dict[str, dict[str, dict[str, str]]] = {}
 
 # Other auth
 SSO_APPEND_DOMAIN: str | None = None

zproject/prod_settings_template.py

@@ -443,7 +443,7 @@ SOCIAL_AUTH_SAML_ENABLED_IDPS: dict[str, Any] = {
         ## List of additional attributes to fetch from the SAMLResponse.
         ## These attributes will be available for synchronizing custom profile fields.
         ## in SOCIAL_AUTH_SYNC_CUSTOM_ATTRS_DICT.
-        # "extra_attrs": ["title", "mobilePhone"],
+        # "extra_attrs": ["title", "mobilePhone", "zulip_role"],
         ##
         ## The "x509cert" attribute is automatically read from
         ## /etc/zulip/saml/idps/{idp_name}.crt; don't specify it here.
@@ -511,12 +511,14 @@ SOCIAL_AUTH_SAML_SUPPORT_CONTACT = {
 ## Note: Any additional SAML attributes that'll be used here must be
 ## listed in the "extra_attrs" field in the SOCIAL_AUTH_SAML_ENABLED_IDPS
 ## configuration for your IdP.
-# SOCIAL_AUTH_SYNC_CUSTOM_ATTRS_DICT = {
+# SOCIAL_AUTH_SYNC_ATTRS_DICT = {
 #     "example_org": {
 #         "saml": {
-#            # Format: "<custom profile field name>": "<attribute name from extra_attrs above>"
+#             # role is currently the only supported major attribute.
-#            "title": "title",
+#             "role": "zulip_role",
-#            "phone_number": "mobilePhone",
+#             # Specify custom profile fields with a custom__ prefix for the
+#             # Zulip field name.
+#             "custom__title": "title",
 #         }
 #     }
 # }