From 833dce8a13b8ab0a4b9b7e7e03756013d86d45df Mon Sep 17 00:00:00 2001 From: Mateusz Mandera Date: Sun, 4 Aug 2024 01:32:32 +0200 Subject: [PATCH] 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 | 7 +- zerver/tests/test_auth_backends.py | 150 +++++++++++++++++++++++++++-- zerver/views/auth.py | 21 +++- zproject/backends.py | 113 +++++++++++++++++----- zproject/computed_settings.py | 21 ++++ zproject/default_settings.py | 1 + zproject/prod_settings_template.py | 20 ++-- 7 files changed, 289 insertions(+), 44 deletions(-) diff --git a/docs/overview/changelog.md b/docs/overview/changelog.md index 9509d9b4c6..efcbe71182 100644 --- a/docs/overview/changelog.md +++ b/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 diff --git a/zerver/tests/test_auth_backends.py b/zerver/tests/test_auth_backends.py index 96b3a8890e..1318541b7a 100644 --- a/zerver/tests/test_auth_backends.py +++ b/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", - "phone_number": "mobilePhone", + "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="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", - ) + ), ], ) diff --git a/zerver/views/auth.py b/zerver/views/auth.py index bb8151c156..24169a00a8 100644 --- a/zerver/views/auth.py +++ b/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,7 +304,13 @@ 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 - prereg_user.invited_as = invited_as + + 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", diff --git a/zproject/backends.py b/zproject/backends.py index 0761959f4a..040f9cd3cc 100644 --- a/zproject/backends.py +++ b/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,26 +1970,11 @@ 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, {}) - if user_profile is not None and extra_attrs and attrs_by_backend: - # 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 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), - ) + role_for_new_user = None + if extra_attrs: + role_for_new_user = social_auth_sync_user_attributes( + realm, user_profile, extra_attrs, backend + ) if user_profile: # This call to authenticate() is just to get to invoke the custom_auth_decorator logic. @@ -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) diff --git a/zproject/computed_settings.py b/zproject/computed_settings.py index afd5aae82a..04b65e9e80 100644 --- a/zproject/computed_settings.py +++ b/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", diff --git a/zproject/default_settings.py b/zproject/default_settings.py index 6085d71a1a..dba8557c1d 100644 --- a/zproject/default_settings.py +++ b/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 diff --git a/zproject/prod_settings_template.py b/zproject/prod_settings_template.py index 85a8e515e1..aff324dbce 100644 --- a/zproject/prod_settings_template.py +++ b/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,14 +511,16 @@ 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 = { -# "example_org": { -# "saml": { -# # Format: "": "" -# "title": "title", -# "phone_number": "mobilePhone", -# } -# } +# SOCIAL_AUTH_SYNC_ATTRS_DICT = { +# "example_org": { +# "saml": { +# # role is currently the only supported major attribute. +# "role": "zulip_role", +# # Specify custom profile fields with a custom__ prefix for the +# # Zulip field name. +# "custom__title": "title", +# } +# } # } ########