Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pass multi-valued fields to clean_user_data #277

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

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