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", +# } +# } # } ########