[Security] Added LDAP support to Authenticator system #36600
Conversation
|
After chatting about this, here is my proposal:
For the last part, we would leverage a new tag for ldap: if your ldap service has a tag, it's injected into the listener's locator. This will allow people to write custom authenticators that use LDAP. They would just need to: A) Register the LDAP service |
06d65ef to
e71a5a3
Compare
3f0e4d9 to
f3b5ca6
Compare
| $ldapBadge = $passport->getBadge(LdapBadge::class); | ||
| if ($ldapBadge->isResolved()) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
I guess technically this should be moved before the if statement above? In theory, someone could resolve this and not even have a PasswordCredentials? Not sure why, but I think it makes sense higher.
|
|
||
| /** @var PasswordCredentials $passwordCredentials */ | ||
| $passwordCredentials = $passport->getBadge(PasswordCredentials::class); | ||
| if ($passwordCredentials->isResolved()) { |
There was a problem hiding this comment.
This would mean that LdapBadge is NOT resolved, but PasswordCredentials IS resolved. I would prefer to throw an exception: this is an unexpected case: a resolved PasswordCredentials is basically the same (at this point) as NO PasswordCredentials since a resolved PasswordCredentials has a null password.
2c49273 to
20962e6
Compare
|
Thank you @wouterj. |
The last missing authenticator in the new system 🎉
I have no experience with LDAP at all and I didn't succeed in setting up a server locally. So I can't test whether this works, but the unit test works (and also tested in a real app, while adding a
dd()call in the listener).I want to share with you the current state of Security LDAP, how this PR implements it and a possible other solution (which I think I would prefer most). Is there anyone who can share their opinions on this? (hopefully @weaverryan and @csarrazi can share their opinion, as they have most experience on this topic)
Current Solution: An LDAP authentication provider + duplicated
SecurityFactoryclassesLDAP is done in one centralized authentication provider. This provider is configured by security factories for each core factory (e.g.
form_loginbecomesform_login_ldap,http_basicbecomeshttp_basic_ldap).Implementation in this PR: A listener is executed before the default
VerifyCredentialsListener, to verifyPasswordCredentialsThis listener must be configured for each specific authenticator wanting to use LDAP. This is a technique similar to (1). It's a bit difficult to use this for your own authenticator (you need to configure a custom listener service) and still needs the duplicated factory classes
Proposal: Introduce a
LdapCredentialsclass and always register a listenerIf an authentictor returns
LdapCredentials, it'll be checked using the LDAP verification listener. This is the easiest for custom authenticators and would remove the duplicated factories, I can imagineform_logingetting a newldapsub option to configure the settings.The main disadvantage (I think) is that we would need to make
LdapCredentialsconfigure all options: ldap service, dnString, searchDn, searchPassword & queryString. Especially passing around the ldap service seems a bit weird. The main questions here are: Is it weird to pass all these things in theLdapCredentials? And, do we really need to support having multiple LDAP configuration sets for different authenticators? Or can we e.g. add a globalsecurity.ldapconfiguration, that registers the listener for all authenticators returningLdapCredentials?