Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork5.3k
Added query_string LDAP config option#7420
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
nietonfir commentedJan 26, 2017
Maybe merge with#7414 ? |
lsmith77 commentedJan 26, 2017
ah doh .. didn't see this PR :-/ lets wait for feedback. for example I am not sure if it made sense adding another example or not like I did. |
nietonfir commentedJan 26, 2017
No problem. ;-) |
lsmith77 commentedJan 27, 2017
integrated#7414 |
a9bd37a to1735f08Compare… 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
lsmith77 commentedJan 28, 2017
the relevant code branch was merged |
…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
reference/configuration/security.rst Outdated
| **type**: ``string`` **default**: ``null`` | ||
| This is the string which will be used to query for the DN. The ``{username}`` | ||
| placeholder will be replaced with the user-provided value (his login). |
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.
We should use "their" instead of "his".
reference/configuration/security.rst Outdated
| This is the string which will be used to query for the DN. The ``{username}`` | ||
| placeholder will be replaced with the user-provided value (his login). | ||
| Depending on your LDAP server's configuration, you will need to override | ||
| this value. This setting is only necessary if the users DN cannot be derived |
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.
user's
reference/configuration/security.rst Outdated
| placeholder will be replaced with the user-provided value (his login). | ||
| Depending on your LDAP server's configuration, you will need to override | ||
| this value. This setting is only necessary if the users DN cannot be derived | ||
| statically using the `dn_string` config option. |
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.
we need double backticks here
| ..code-block::php | ||
| $container->loadFromExtension('security', array( |
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.
we should add the// app/config/security.php comment above
security/ldap.rst Outdated
| # ... | ||
| form_login_ldap: | ||
| login_path:login | ||
| check_path:login_check |
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.
Recently we updated the docs to use the same route forlogin_path andcheck_path. I think we should do that here too.
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.
if I look at form_login.rst it seems like those config options are simply omitted there .. that being said there are also some previously existing places in this file where we have the check_path in examples .. so should i remove login_path/check_path for all examples in this file?
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.
diff --git a/security/ldap.rst b/security/ldap.rstindex bd812fd..d91e977 100644--- a/security/ldap.rst+++ b/security/ldap.rst@@ -310,8 +310,6 @@ Configuration example for form login main: # ... form_login_ldap:- login_path: login- check_path: login_check # ... service: ldap dn_string: 'uid={username},dc=example,dc=com'@@ -329,8 +327,6 @@ Configuration example for form login <config> <firewall name="main"> <form-login-ldap- login-path="login"- check-path="login_check" service="ldap" dn-string="uid={username},dc=example,dc=com" /> </firewall>@@ -343,8 +339,6 @@ Configuration example for form login 'firewalls' => array( 'main' => array( 'form_login_ldap' => array(- 'login_path' => 'login',- 'check_path' => 'login_check', 'service' => 'ldap', 'dn_string' => 'uid={username},dc=example,dc=com', // ...@@ -419,8 +413,6 @@ Configuration example for form login and query_string main: # ... form_login_ldap:- login_path: login- check_path: login_check # ... service: ldap dn_string: 'dc=example,dc=com'@@ -439,8 +431,6 @@ Configuration example for form login and query_string <config> <firewall name="main"> <form-login-ldap- login-path="login"- check-path="login_check" service="ldap" dn-string="dc=example,dc=com" query-string="(&(uid={username})(memberOf=cn=users,ou=Services,dc=example,dc=com))" />@@ -455,8 +445,6 @@ Configuration example for form login and query_string 'firewalls' => array( 'main' => array( 'form_login_ldap' => array(- 'login_path' => 'login',- 'check_path' => 'login_check', 'service' => 'ldap', 'dn_string' => 'dc=example,dc=com', 'query_string' => '(&(uid={username})(memberOf=cn=users,ou=Services,dc=example,dc=com))',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.
looks good to me too
lsmith77 commentedFeb 3, 2017
@xabbuh ok .. all comments should be addressed |
xabbuh commentedFeb 3, 2017
👍 Status: Reviewed |
xabbuh commentedFeb 28, 2017
Thank you@lsmith77. |
…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
xabbuh commentedFeb 28, 2017
And thank you very much@nietonfir! |
Uh oh!
There was an error while loading.Please reload this page.
docs forsymfony/symfony#21402