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 add() 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 + duplicatedSecurityFactory classes
LDAP is done in one centralized authentication provider. This provider is configured by security factories for each core factory (e.g.form_login becomesform_login_ldap,http_basic becomeshttp_basic_ldap).
Implementation in this PR: A listener is executed before the defaultVerifyCredentialsListener, to verifyPasswordCredentials
This 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 aLdapCredentials class and always register a listener
If an authentictor returnsLdapCredentials, 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_login getting a newldap sub option to configure the settings.
The main disadvantage (I think) is that we would need to makeLdapCredentials configure 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.ldap configuration, that registers the listener for all authenticators returningLdapCredentials?

carsonbot added Status: Needs Review Security Feature labels

Apr 27, 2020

Copy link

Member

weaverryan commentedApr 28, 2020

After chatting about this, here is my proposal:

Could we decorateFormLoginAuthenticator with anLdapAuthenticator? The decorator would call the innerauthenticate() but then ADD anLdapCredentialsBadge (not a "credentials" badge, it would just hold LDAP configuration, including the LDAP service id, but not the actual service object
Register a listener, likeVerifyLdapCredentialsListener (higher priority thanVerifyCredentialsListener ) that looks for theLdapCredentialsBadge. If found, it would use the password from thePasswordCredentials and the config from theLdapCredentialsBadge to do the credentials checking. To avoid passing the Ldap service around in theLdapCredentialsBadge, we could inject a service locator into the listener. It would contain all LDAP services that were registered for any ldap authenticators. So... in reality, it would contain a maximum of 2 - one for the service configured underhttp_basic_ldap and one for the service configured underform_login_ldap.

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
B) Give it the new tag we invent
C) ReturnPasswordCredentials andLdapCredentialsBadge

nicolas-grekas added this to thenext milestone

May 1, 2020

wouterj force-pushed thesecurity/new-system-ldap branch 2 times, most recently from06d65ef toe71a5a3Compare

May 2, 2020 12:40

wouterj marked this pull request as ready for review

May 2, 2020 12:40

wouterj force-pushed thesecurity/new-system-ldap branch 6 times, most recently from3f0e4d9 tof3b5ca6Compare

May 3, 2020 15:04

weaverryan reviewed

May 3, 2020

View reviewed changes

src/Symfony/Component/Ldap/Security/CheckLdapCredentialsListener.php

		$ldapBadge = $passport->getBadge(LdapBadge::class);
		if ($ldapBadge->isResolved()) {
		return;
		}

Copy link

Member

weaverryanMay 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

src/Symfony/Component/Ldap/Security/CheckLdapCredentialsListener.php


		/** @var PasswordCredentials $passwordCredentials */
		$passwordCredentials = $passport->getBadge(PasswordCredentials::class);
		if ($passwordCredentials->isResolved()) {

Copy link

Member

weaverryanMay 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

This would mean thatLdapBadge is NOT resolved, butPasswordCredentials IS resolved. I would prefer to throw an exception: this is an unexpected case: a resolvedPasswordCredentials is basically the same (at this point) as NOPasswordCredentials since a resolvedPasswordCredentials has a null password.