[Ldap] Add security LdapUser and provider #32824
Conversation
5126007 to
979d6af
Compare
93bfe64 to
6cfc280
Compare
|
Travis failure is unrelated |
src/Symfony/Component/Ldap/Tests/Security/User/LdapUserProviderTest.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Ldap/Tests/Security/User/LdapUserProviderTest.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Ldap/Tests/Security/User/LdapUserProviderTest.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Ldap/Tests/Security/User/LdapUserProviderTest.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Ldap/Tests/Security/User/LdapUserProviderTest.php
Outdated
Show resolved
Hide resolved
| /** | ||
| * @author Robin Chalas <robin.chalas@gmail.com> | ||
| * | ||
| * @internal |
There was a problem hiding this comment.
is this a requirement? do we anticipate possible direct use in userland?
There was a problem hiding this comment.
People have been using the InMemoryUser (aka User) in user-land as well, albeit having a more generic name, I think it's good to mention that this is (package) internal.
There was a problem hiding this comment.
yes, the security-core User should be internal since day 1 to me. Reasoning is that UserInterface should be the only core type needed in userland.
But, given this one exposes a getExtraFields() introduced not so long ago (#31532), I switched to just @final.
There was a problem hiding this comment.
Would it be an option to move those to the token attributes instead?
There was a problem hiding this comment.
they are about domain user info such as email address, phone number... so not sure actually. If I had to use this provider, I would probably write a user class with proper getters and build it from the LdapUser
There was a problem hiding this comment.
If you'd not have the user object outside of the inner security workings anymore, thus move it to just the domain object, how would you solve that for LDAP?
| /** | ||
| * @author Robin Chalas <robin.chalas@gmail.com> | ||
| * | ||
| * @internal |
There was a problem hiding this comment.
People have been using the InMemoryUser (aka User) in user-land as well, albeit having a more generic name, I think it's good to mention that this is (package) internal.
src/Symfony/Component/Ldap/Tests/Security/User/LdapUserProviderTest.php
Outdated
Show resolved
Hide resolved
990ffd9 to
18b4354
Compare
|
Comments addressed, thanks for the reviews |
|
Thank you @chalasr. |
This PR was merged into the 4.4 branch. Discussion ---------- [Ldap] Add security LdapUser and provider | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - Moves `LdapUserProvider` from `Security\Core` to the Ldap component, the provider now deals with a new `LdapUser` aware of its ldap `Entry` (should help in #31843). Commits ------- 6736cdf [Ldap] Add security LdapUser and provider
This PR was squashed before being merged into the 4.4 branch (closes #12423). Discussion ---------- [Ldap] Add security LdapUser and provider (4.4) #12088 and symfony/symfony#32824  Commits ------- a4ea784 [Ldap] Add security LdapUser and provider (4.4)
This PR was squashed before being merged into the 4.4 branch (closes #37177). Discussion ---------- [Ldap] fix refreshUser() ignoring extra_fields | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | yes | New feature? | no | Deprecations? |no | Tickets | - | License | MIT | Doc PR | - While #31532 introduced `extra_fields` in general, #32824 later added `LdapUser` & `LdapUserProvider` and ignored `extra_fields` on `refreshUser()`. This PR fixes `refreshUser()` and adds a test which makes sure, that the refreshed ldap user doesn't lose its default values. Commits ------- cb8f129 [Ldap] fix refreshUser() ignoring extra_fields
Moves
LdapUserProviderfromSecurity\Coreto the Ldap component, the provider now deals with a newLdapUseraware of its ldapEntry(should help in #31843).