Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Security] make LdapBindAuthenticationProvider capable of searching for the DN#21402
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| $provider ='security.authentication.provider.ldap_'.$method.'.'.$id; | ||
| switch($method) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
there is probably a more elegant way to do this without duplicating so much code
d7b1939 to12e9d07Compare12e9d07 toa26e0b1Compare| thrownewBadCredentialsException('The presented password is invalid.'); | ||
| } | ||
| $this->ldap->bind($result->getIterator()->current()->getDn(),$password); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this could probably be rewritten as$this->ldap->bind($result[0]->getDn(), $password);
| $provider ='security.authentication.provider.ldap_'.$method.'.'.$id; | ||
| switch ($method) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
there is probably a more elegant way to reduce the code duplication
nicolas-grekas commentedJan 25, 2017
ping@csarrazi |
90395dd to04b82a8Compare| * | ||
| * The only way to check user credentials is to first find the DN via query and then bind | ||
| * | ||
| * @author Charles Sarrazin <charles@sarraz.in> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@csarrazi since I usedLdapBindAuthenticationProvider as the base I figured I should leave you as an author
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
BTW I did however think its not appropriate to extend fromLdapBindAuthenticationProvider even though it would reduce some code duplication.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
another approach could be to integrate the logic intoLdapBindAuthenticationProvider
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think it would actually be better to integrate the logic insideLdapBindAuthenticationProvider.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@csarrazi ok .. how should I handle the new parameter without breaking BC on the constructor signature? use setter injection?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
something like
diff --git a/src/Symfony/Bundle/SecurityBundle/DependencyInjection/Security/Factory/FormLoginLdapFactory.php b/src/Symfony/Bundle/SecurityBundle/DependencyInjection/Security/Factory/FormLoginLdapFactory.phpindex cd4158d..3d9d4b2 100644--- a/src/Symfony/Bundle/SecurityBundle/DependencyInjection/Security/Factory/FormLoginLdapFactory.php+++ b/src/Symfony/Bundle/SecurityBundle/DependencyInjection/Security/Factory/FormLoginLdapFactory.php@@ -27,7 +27,7 @@ class FormLoginLdapFactory extends FormLoginFactory protected function createAuthProvider(ContainerBuilder $container, $id, $config, $userProviderId) { $provider = 'security.authentication.provider.ldap_bind.'.$id;- $container+ $definition = $container ->setDefinition($provider, new ChildDefinition('security.authentication.provider.ldap_bind')) ->replaceArgument(0, new Reference($userProviderId)) ->replaceArgument(1, new Reference('security.user_checker.'.$id))@@ -36,6 +36,10 @@ class FormLoginLdapFactory extends FormLoginFactory ->replaceArgument(4, $config['dn_string']) ; + if (!empty($config['query_string'])) {+ $definition->addMethodCall('setQueryString', [$config['query_string']]);+ }+ return $provider; } @@ -47,6 +51,7 @@ class FormLoginLdapFactory extends FormLoginFactory ->children() ->scalarNode('service')->defaultValue('ldap')->end() ->scalarNode('dn_string')->defaultValue('{username}')->end()+ ->scalarNode('query_string')->end() ->end() ; }diff --git a/src/Symfony/Component/Security/Core/Authentication/Provider/LdapBindAuthenticationProvider.php b/src/Symfony/Component/Security/Core/Authentication/Provider/LdapBindAuthenticationProvider.phpindex 5ebb09a..3dfe028 100644--- a/src/Symfony/Component/Security/Core/Authentication/Provider/LdapBindAuthenticationProvider.php+++ b/src/Symfony/Component/Security/Core/Authentication/Provider/LdapBindAuthenticationProvider.php@@ -33,6 +33,7 @@ class LdapBindAuthenticationProvider extends UserAuthenticationProvider private $userProvider; private $ldap; private $dnString;+ private $queryString; /** * Constructor.@@ -54,6 +55,16 @@ class LdapBindAuthenticationProvider extends UserAuthenticationProvider } /**+ * Set a query string to use in order to find a DN for the username+ *+ * @param string $queryString+ */+ public function setQueryString($queryString)+ {+ $this->queryString = $queryString;+ }++ /** * {@inheritdoc} */ protected function retrieveUser($username, UsernamePasswordToken $token)@@ -79,7 +90,21 @@ class LdapBindAuthenticationProvider extends UserAuthenticationProvider try { $username = $this->ldap->escape($username, '', LdapInterface::ESCAPE_DN);- $dn = str_replace('{username}', $username, $this->dnString);++ if ($this->queryString) {+ $query = str_replace('{username}', $username, $this->queryString);++ $query = $this->ldap->query($this->dnString, $query);+ $result = $query->execute();+ if (1 !== $result->count()) {+ throw new BadCredentialsException('The presented username is invalid.');+ }++ $dn = $result[0]->getDn();+ } else {+ $username = $this->ldap->escape($username, '', LdapInterface::ESCAPE_DN);+ $dn = str_replace('{username}', $username, $this->dnString);+ } $this->ldap->bind($dn, $password); } catch (ConnectionException $e) {There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yup, setter injection would be the way to go. By default the new attribute should be null, and the current behaviour should be the correct one in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
One thing, though. It seems that you have duplicate logic in theelse part of yourif statement:
$username =$this->ldap->escape($username,'', LdapInterface::ESCAPE_DN);
04b82a8 to4f7487cComparelsmith77 commentedJan 25, 2017
@csarrazi pushed (and fixed the duplicate code) |
4f7487c toa70f80aComparea70f80a to93a02b2Compare93a02b2 toa30191fComparelsmith77 commentedJan 25, 2017
added some tests |
lsmith77 commentedJan 25, 2017
I will test it tomorrow again when I have direct access to the LDAP server of the project where I am implementing this to confirm the tests are sufficient and the code is indeed working. |
nietonfir commentedJan 25, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Just tested (both the original PR and the updated one - against currentstable@3.2.2) against an OpenLDAP directory configured with a meta backend serving as a proxy for two different OpenLDAP instances (thus two different DNs - see#16823), works like a charm. Your changes can also be applied directly to diff --git a/src/Symfony/Bundle/SecurityBundle/DependencyInjection/Security/Factory/HttpBasicLdapFactory.php b/src/Symfony/Bundle/SecurityBundle/DependencyInjection/Security/Factory/HttpBasicLdapFactory.phpindex 961af71..4665c07 100644--- a/src/Symfony/Bundle/SecurityBundle/DependencyInjection/Security/Factory/HttpBasicLdapFactory.php+++ b/src/Symfony/Bundle/SecurityBundle/DependencyInjection/Security/Factory/HttpBasicLdapFactory.php@@ -28,7 +28,7 @@ class HttpBasicLdapFactory extends HttpBasicFactory public function create(ContainerBuilder $container, $id, $config, $userProvider, $defaultEntryPoint) { $provider = 'security.authentication.provider.ldap_bind.'.$id;- $container+ $definition = $container ->setDefinition($provider, new ChildDefinition('security.authentication.provider.ldap_bind')) ->replaceArgument(0, new Reference($userProvider)) ->replaceArgument(1, new Reference('security.user_checker.'.$id))@@ -40,6 +40,11 @@ class HttpBasicLdapFactory extends HttpBasicFactory // entry point $entryPointId = $this->createEntryPoint($container, $id, $config, $defaultEntryPoint);++ if (!empty($config['query_string'])) {+ $definition->addMethodCall('setQueryString', array($config['query_string']));+ }+ // listener $listenerId = 'security.authentication.listener.basic.'.$id; $listener = $container->setDefinition($listenerId, new ChildDefinition('security.authentication.listener.basic'));@@ -57,6 +62,7 @@ class HttpBasicLdapFactory extends HttpBasicFactory ->children() ->scalarNode('service')->defaultValue('ldap')->end() ->scalarNode('dn_string')->defaultValue('{username}')->end()+ ->scalarNode('query_string')->end() ->end() ; } |
so that it matches the current FormLoginLdapFactory ldap implementation,where the DN can be search for before binding.
nietonfir commentedJan 26, 2017
@lsmith77 created a PR to your branch. Hope that's what you meant… ;-) |
lsmith77 commentedJan 26, 2017
thx |
5f89a03 to0679a74Comparelsmith77 commentedJan 26, 2017
added support for HTTP basic as well and linked a doc PR. will do final testing on my end tomorrow. @csarrazi are the unit tests sufficient or do we also need some functional tests? |
| if ($this->queryString) { | ||
| $query =str_replace('{username}',$username,$this->queryString); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Remove newline.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
done
| $query =str_replace('{username}',$username,$this->queryString); | ||
| $query =$this->ldap->query($this->dnString,$query); | ||
| $result =$query->execute(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Add newline after this one and merge this line with the previous one (no need to keep the query variable, as it is not used for anything but for running itsexecute() method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
done
csarrazi commentedJan 26, 2017
I don't think we need functional tests for this one. Functional tests are limited to the Ldap component's integration with OpenLDAP. The component was not updated here, so no functional tests are needed. Unit tests are sufficient here. |
Update HttpBasicLdapFactory
0679a74 to8ddd533Comparelsmith77 commentedJan 27, 2017
@csarrazi ok fixed your comments and tested it against the production LDAP server. all is well to merge from my side |
csarrazi commentedJan 27, 2017
👍 |
fabpot commentedJan 28, 2017
Thank you@lsmith77. |
… of searching for the DN (lsmith77, nietonfir)This PR was merged into the 3.3-dev branch.Discussion----------[Security] make LdapBindAuthenticationProvider capable of searching for the DN| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#16823,#20905| License | MIT| Doc PR |symfony/symfony-docs#7420I guess due to the separation between the user and auth provider something like the following isn't ok (note: the following works just fine for me but if course in the end the username is the DN and not the user provided shorter username):```diffdiff --git a/src/Symfony/Component/Security/Core/Authentication/Provider/LdapBindAuthenticationProvider.php b/src/Symfony/Component/Security/Core/Authentication/Provider/LdapBindAuthenticationProvider.phpindex5ebb09a..18d7825 100644--- a/src/Symfony/Component/Security/Core/Authentication/Provider/LdapBindAuthenticationProvider.php+++ b/src/Symfony/Component/Security/Core/Authentication/Provider/LdapBindAuthenticationProvider.php@@ -70,7 +70,7 @@ class LdapBindAuthenticationProvider extends UserAuthenticationProvider */ protected function checkAuthentication(UserInterface $user, UsernamePasswordToken $token) {- $username = $token->getUsername();+ $username = $user->getUsername(); $password = $token->getCredentials(); if ('' === $password) {@@ -78,10 +78,7 @@ class LdapBindAuthenticationProvider extends UserAuthenticationProvider } try {- $username = $this->ldap->escape($username, '', LdapInterface::ESCAPE_DN);- $dn = str_replace('{username}', $username, $this->dnString);-- $this->ldap->bind($dn, $password);+ $this->ldap->bind($username, $password); } catch (ConnectionException $e) { throw new BadCredentialsException('The presented password is invalid.'); }diff --git a/src/Symfony/Component/Security/Core/User/LdapUserProvider.php b/src/Symfony/Component/Security/Core/User/LdapUserProvider.phpindexfc42419..8194c4c 100644--- a/src/Symfony/Component/Security/Core/User/LdapUserProvider.php+++ b/src/Symfony/Component/Security/Core/User/LdapUserProvider.php@@ -115,7 +115,7 @@ class LdapUserProvider implements UserProviderInterface { $password = $this->getPassword($entry);- return new User($username, $password, $this->defaultRoles);+ return new User($entry->getDn(), $password, $this->defaultRoles); } /**```Therefore I created an entire new auth provider.Commits-------8ddd533 Merge pull request#1 from nietonfir/http_basic_ldapa783e5c Update HttpBasicLdapFactorya30191f make LdapBindAuthenticationProvider capable of searching for the DN
…capable of searching for the DN (lsmith77, nietonfir)This PR was merged into the 3.3-dev branch.Discussion----------[Security] make LdapBindAuthenticationProvider capable of searching for the DN| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |symfony#16823,symfony#20905| License | MIT| Doc PR |symfony/symfony-docs#7420I guess due to the separation between the user and auth provider something like the following isn't ok (note: the following works just fine for me but if course in the end the username is the DN and not the user provided shorter username):```diffdiff --git a/src/Symfony/Component/Security/Core/Authentication/Provider/LdapBindAuthenticationProvider.php b/src/Symfony/Component/Security/Core/Authentication/Provider/LdapBindAuthenticationProvider.phpindex5ebb09a..18d7825 100644--- a/src/Symfony/Component/Security/Core/Authentication/Provider/LdapBindAuthenticationProvider.php+++ b/src/Symfony/Component/Security/Core/Authentication/Provider/LdapBindAuthenticationProvider.php@@ -70,7 +70,7 @@ class LdapBindAuthenticationProvider extends UserAuthenticationProvider */ protected function checkAuthentication(UserInterface $user, UsernamePasswordToken $token) {- $username = $token->getUsername();+ $username = $user->getUsername(); $password = $token->getCredentials(); if ('' === $password) {@@ -78,10 +78,7 @@ class LdapBindAuthenticationProvider extends UserAuthenticationProvider } try {- $username = $this->ldap->escape($username, '', LdapInterface::ESCAPE_DN);- $dn = str_replace('{username}', $username, $this->dnString);-- $this->ldap->bind($dn, $password);+ $this->ldap->bind($username, $password); } catch (ConnectionException $e) { throw new BadCredentialsException('The presented password is invalid.'); }diff --git a/src/Symfony/Component/Security/Core/User/LdapUserProvider.php b/src/Symfony/Component/Security/Core/User/LdapUserProvider.phpindexfc42419..8194c4c 100644--- a/src/Symfony/Component/Security/Core/User/LdapUserProvider.php+++ b/src/Symfony/Component/Security/Core/User/LdapUserProvider.php@@ -115,7 +115,7 @@ class LdapUserProvider implements UserProviderInterface { $password = $this->getPassword($entry);- return new User($username, $password, $this->defaultRoles);+ return new User($entry->getDn(), $password, $this->defaultRoles); } /**```Therefore I created an entire new auth provider.Commits-------8ddd533 Merge pull request#1 from nietonfir/http_basic_ldapa783e5c Update HttpBasicLdapFactorya30191f make LdapBindAuthenticationProvider capable of searching for the DN
…reguiluz, lsmith77)This PR was merged into the master branch.Discussion----------Added query_string LDAP config optiondocs forsymfony/symfony#21402Commits-------b82cafd clean up446ba38 added query_string LDAP config optioned58da8 Minor rewordf133269 Explain the query_string ldap authentication provider configuration key
Uh oh!
There was an error while loading.Please reload this page.
I guess due to the separation between the user and auth provider something like the following isn't ok (note: the following works just fine for me but if course in the end the username is the DN and not the user provided shorter username):
Therefore I created an entire new auth provider.