Skip to content

Conversation

Freddo3000
Copy link
Contributor

Resolves #276

Breaks backwards compatibility slightly in that it may pass lists as input to clean_user_data, which may break already existing custom implementations of it, but the fix is easy.

@etianen
Copy link
Owner

etianen commented Jul 29, 2024

In retrospect, the original behavior was a bit odd. I'm not sure, however, whether you new proposed behaviour isn't also somewhat odd.

Wouldn't it be better to just leave lists as lists, and single values as single values? This would also be a backwards-incompatible change, but one that would leave data in the form sent by the LDAP server rather than applying arbitrary rules to coerce it to single values or lists.

@Freddo3000
Copy link
Contributor Author

Either case works, the reason I went with this solution is because this solution breaks backwards compatibility slightly less. I'd imagine in most LDAP databases, each field that you'd want to transfer to Django only has a single value as expected in a custom clean_user_data which means that those would not break. But it really just leaves a hole if someone were to accidentally add a second value to a field in LDAP and suddenly break it.

As you say, either solution would break backwards compatibility, but your solution would at least force someone to account for it instead of possibly silently failing.

@etianen
Copy link
Owner

etianen commented Jul 29, 2024

Can we go with my suggestion of leaving the server data as-is? A breaking change is fine, they happen sometimes.

@Freddo3000
Copy link
Contributor Author

Can we go with my suggestion of leaving the server data as-is?

Fine with me, I just want all LDAP data to be preserved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

LDAP_AUTH_CLEAN_USER_DATA does not receive all values for multi-valued fields

2 participants