Skip to content

[Ldap] Allow multiple values on extra_fields #49187

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

Merged

Conversation

mvhirsch
Copy link
Contributor

@mvhirsch mvhirsch commented Feb 1, 2023

Q A
Branch? 5.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets ~
License MIT
Doc PR

Loading users with the LdapUserProvider, using extra_fields fails if I want to get memberOf attribute.
This happens, if my user has multiple attributes set. After taking a look at Ldap\Entry (which supports multiple values per attribute). Looking at LdapUserProvider::getAttributeValue() it was very clear, that every attribute must be unique.

The method getAttributeValue() has originated from getPassword dbf45e4, forcing the password attribute to be unique. Same goes for uidKey c91689b. Loading extra_fields should allow for multiple values per attribute (like memberOf).

Did I break backward compatibility? No, I didn't. Simply because it was never usable as array while loading users via LdapUserProvider, instead an InvalidArgumentException was thrown.

@mvhirsch mvhirsch force-pushed the bugfix-multiple-ldap-extra-fields branch from 765d893 to e51b502 Compare February 1, 2023 13:36
@OskarStark
Copy link
Contributor

OskarStark commented Feb 1, 2023

One could argue, that this is a new feature, but I am for handling this as a bugfix and merge it in 5.4

@nicolas-grekas
Copy link
Member

Thank you @mvhirsch.

@nicolas-grekas nicolas-grekas merged commit 69f113a into symfony:5.4 Feb 1, 2023
@mvhirsch mvhirsch deleted the bugfix-multiple-ldap-extra-fields branch February 1, 2023 20:36
This was referenced Feb 28, 2023
@DemigodCode
Copy link
Contributor

DemigodCode commented Apr 11, 2023

This "fix" breaks BC in my case.
We're loading the mail attribute of the user from ldap. That is a single-value.
Now I have to access all attributes like this: "$user->getExtraFields()['mail'][0]" before: "$user->getExtraFields()['mail']".

Had to update all occurences. An upgrade notice would be very nice for that.

That happens for all extraFields that are not "uidKey" or "passwordAttribute".
Each of those fields are now returned as array and not as string.

@OskarStark
Copy link
Contributor

OskarStark commented Apr 11, 2023

I think a BC break as not planned here.

Not sure how to fix this.

javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request Mar 13, 2024
…ds()` (mvhirsch)

This PR was merged into the 5.4 branch.

Discussion
----------

[Ldap] Adds return value hint of `LdapUser::getExtraFields()`

Hi,

I'm not sure if this is the correct approach to document that. Please advice me where to document this. I'd love to get this done correctly.

- Fixes symfony/symfony#49994
- refs symfony/symfony#49187

Commits
-------

f75e437 adds hint of return value
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants