Added query_string LDAP config option #7420

lsmith77 · 2017-01-26T22:07:29Z

docs for symfony/symfony#21402

nietonfir · 2017-01-26T22:17:51Z

Maybe merge with #7414 ?

lsmith77 · 2017-01-26T22:20:14Z

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 · 2017-01-26T22:57:58Z

No problem. ;-)
Your example looks fine to me.

lsmith77 · 2017-01-27T09:50:12Z

integrated #7414

… 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#7420 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): ```diff diff --git a/src/Symfony/Component/Security/Core/Authentication/Provider/LdapBindAuthenticationProvider.php b/src/Symfony/Component/Security/Core/Authentication/Provider/LdapBindAuthenticationProvider.php index 5ebb09a..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.php index fc42419..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_ldap a783e5c Update HttpBasicLdapFactory a30191f make LdapBindAuthenticationProvider capable of searching for the DN

lsmith77 · 2017-01-28T22:50:12Z

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#7420 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): ```diff diff --git a/src/Symfony/Component/Security/Core/Authentication/Provider/LdapBindAuthenticationProvider.php b/src/Symfony/Component/Security/Core/Authentication/Provider/LdapBindAuthenticationProvider.php index 5ebb09a..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.php index fc42419..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_ldap a783e5c Update HttpBasicLdapFactory a30191f make LdapBindAuthenticationProvider capable of searching for the DN

xabbuh · 2017-02-03T08:41:36Z

reference/configuration/security.rst

+**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).


We should use "their" instead of "his".

xabbuh · 2017-02-03T08:41:46Z

reference/configuration/security.rst

+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


xabbuh · 2017-02-03T08:41:58Z

reference/configuration/security.rst

+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.


we need double backticks here

xabbuh · 2017-02-03T08:43:20Z

security/ldap.rst

+
+    .. code-block:: php
+
+        $container->loadFromExtension('security', array(


we should add the // app/config/security.php comment above

xabbuh · 2017-02-03T08:45:02Z

security/ldap.rst

+                    # ...
+                    form_login_ldap:
+                        login_path: login
+                        check_path: login_check


Recently we updated the docs to use the same route for login_path and check_path. I think we should do that here too.

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?

diff --git a/security/ldap.rst b/security/ldap.rst index 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))',

looks good to me too

Refs symfony/symfony#21402

lsmith77 · 2017-02-03T12:06:41Z

@xabbuh ok .. all comments should be addressed

xabbuh · 2017-02-03T12:24:54Z

👍

Status: Reviewed

xabbuh · 2017-02-28T21:06:45Z

Thank you @lsmith77.

…reguiluz, lsmith77) This PR was merged into the master branch. Discussion ---------- Added query_string LDAP config option docs for symfony/symfony#21402 Commits ------- b82cafd clean up 446ba38 added query_string LDAP config option ed58da8 Minor reword f133269 Explain the query_string ldap authentication provider configuration key

xabbuh · 2017-02-28T21:07:00Z

And thank you very much @nietonfir!

carsonbot added the Status: Needs Review label Jan 26, 2017

lsmith77 mentioned this pull request Jan 26, 2017

[Security] make LdapBindAuthenticationProvider capable of searching for the DN symfony/symfony#21402

Merged

lsmith77 mentioned this pull request Jan 27, 2017

Explain the query_string ldap authentication provider configuration key #7414

Closed

lsmith77 force-pushed the ldap_query_string branch 2 times, most recently from a9bd37a to 1735f08 Compare January 27, 2017 09:51

javiereguiluz changed the title ~~added query_string LDAP config option~~ Added query_string LDAP config option Feb 1, 2017

javiereguiluz added the Ldap label Feb 1, 2017

javiereguiluz added this to the 3.3 milestone Feb 1, 2017

xabbuh reviewed Feb 3, 2017

View reviewed changes

lsmith77 force-pushed the ldap_query_string branch from 1735f08 to 3e4e277 Compare February 3, 2017 08:58

nietonfir and others added 3 commits February 3, 2017 09:58

Explain the query_string ldap authentication provider configuration key

f133269

Refs symfony/symfony#21402

Minor reword

ed58da8

added query_string LDAP config option

446ba38

lsmith77 force-pushed the ldap_query_string branch from 3e4e277 to 143ae0a Compare February 3, 2017 09:01

clean up

b82cafd

lsmith77 force-pushed the ldap_query_string branch from 143ae0a to b82cafd Compare February 3, 2017 12:06

carsonbot removed the Status: Needs Review label Feb 3, 2017

carsonbot added the Status: Reviewed label Feb 3, 2017

xabbuh merged commit b82cafd into symfony:master Feb 28, 2017

lsmith77 deleted the ldap_query_string branch February 28, 2017 21:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added query_string LDAP config option #7420

Added query_string LDAP config option #7420

lsmith77 commented Jan 26, 2017 •

edited

nietonfir commented Jan 26, 2017

lsmith77 commented Jan 26, 2017

nietonfir commented Jan 26, 2017

lsmith77 commented Jan 27, 2017

lsmith77 commented Jan 28, 2017

xabbuh Feb 3, 2017

xabbuh Feb 3, 2017

xabbuh Feb 3, 2017

xabbuh Feb 3, 2017

xabbuh Feb 3, 2017

lsmith77 Feb 3, 2017

lsmith77 Feb 3, 2017

xabbuh Feb 3, 2017

lsmith77 commented Feb 3, 2017

xabbuh commented Feb 3, 2017

xabbuh commented Feb 28, 2017

xabbuh commented Feb 28, 2017


		.. code-block:: php

		$container->loadFromExtension('security', array(

Added query_string LDAP config option #7420

Added query_string LDAP config option #7420

Conversation

lsmith77 commented Jan 26, 2017 • edited

nietonfir commented Jan 26, 2017

lsmith77 commented Jan 26, 2017

nietonfir commented Jan 26, 2017

lsmith77 commented Jan 27, 2017

lsmith77 commented Jan 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lsmith77 commented Feb 3, 2017

xabbuh commented Feb 3, 2017

xabbuh commented Feb 28, 2017

xabbuh commented Feb 28, 2017

lsmith77 commented Jan 26, 2017 •

edited