mirror of https://github.com/zulip/zulip.git
saml: Fix exception when syncing missing value to custom profile field.
There was a bug here that would trigger an exception inside `sync_user_profile_custom_fields`, causing it to get logged with logging.warning, when an attribute configured for SAML custom profile field sync was missing from a SAMLResponse or had an empty value. `sync_user_profile_custom_fields` expects valid values, and None is not valid. We could consider a slightly different behavior here instead - when an attribute is sent with no value in the SAMLResponse, that means the attr has no value in the IdP's user directory - so perhaps a better behavior would be to also remove the custom profile field value in Zulip. However there are two issues with that: 1. It's not necessarily the best behavior, because an organization might want the "user doesn't have this attribute set at the IdP level" state to just mean that the user should be free to set the value manually in Zulip if they wish. And having that value get reset on every login would then be an issue. The implementation in this commit is consistent with this philosophy. 2. There's some implementation difficulty - upstream `self.get_attr(...)`, which we use for reading the attr value from the SAMLResponse, doesn't distinguish between an attribute being sent with no value and the attribute not being sent at all - in both cases it returns None. So we'd need some extra work here with parsing the SAMLResponse properly, to be able to know when the custom profile field should get cleared.
This commit is contained in:
parent
2a158cb8d9
commit
7e1f468f04
|
@ -3253,14 +3253,6 @@ class SAMLAuthBackendTest(SocialAuthBase):
|
|||
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": {
|
||||
"role": "zulip_role",
|
||||
}
|
||||
}
|
||||
}
|
||||
with (
|
||||
self.settings(
|
||||
SOCIAL_AUTH_SAML_ENABLED_IDPS=idps_dict,
|
||||
|
@ -3298,12 +3290,36 @@ class SAMLAuthBackendTest(SocialAuthBase):
|
|||
result = self.social_auth_test(
|
||||
account_data_dict,
|
||||
subdomain="zulip",
|
||||
extra_attributes=dict(zulip_role=[""]),
|
||||
extra_attributes=dict(mobilePhone=[""], 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)
|
||||
phone_field_value = CustomProfileFieldValue.objects.get(
|
||||
user_profile=self.user_profile, field=phone_field
|
||||
).value
|
||||
self.assertEqual(phone_field_value, "123412341234")
|
||||
|
||||
# Verify with none of these attributes sent at all.
|
||||
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(),
|
||||
)
|
||||
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)
|
||||
phone_field_value = CustomProfileFieldValue.objects.get(
|
||||
user_profile=self.user_profile, field=phone_field
|
||||
).value
|
||||
self.assertEqual(phone_field_value, "123412341234")
|
||||
|
||||
# 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
|
||||
|
@ -3395,7 +3411,9 @@ class SAMLAuthBackendTest(SocialAuthBase):
|
|||
result = self.social_auth_test(
|
||||
account_data_dict,
|
||||
subdomain="zulip",
|
||||
extra_attributes=dict(mobilePhone=["123412341234"], birthday=["2021-01-01"]),
|
||||
extra_attributes=dict(
|
||||
mobilePhone=["123412341234"], title=["some title"], birthday=["2021-01-01"]
|
||||
),
|
||||
)
|
||||
data = load_subdomain_token(result)
|
||||
self.assertEqual(data["email"], self.email)
|
||||
|
|
|
@ -1676,9 +1676,10 @@ def social_auth_sync_user_attributes(
|
|||
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)
|
||||
)
|
||||
attr_value = extra_attrs.get(attr_name)
|
||||
if attr_value is None:
|
||||
continue
|
||||
custom_profile_field_name_to_value[field_name.removeprefix("custom__")] = attr_value
|
||||
else:
|
||||
backend.logger.warning(
|
||||
"Ignoring unsupported UserProfile field %s in SOCIAL_AUTH_SYNC_ATTRS_DICT",
|
||||
|
|
Loading…
Reference in New Issue