Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

[WIP][Security] Added Ldap provider #5189

Closed
wants to merge 4 commits into from
@lyrixx

Bug fix: no, but may be
Feature addition: yes
Backwards compatibility break: no, but may be
Symfony2 tests pass: no, not yet
Fixes the following tickets: -
Todo: Finish implementation, Add more tests. Fix \DependencyInjection\Security\Factory*`
License of the code: MIT
Documentation PR: not available yet

Hello.

First, it will be useful to have this provider into the security component. Thanks to this, the feature will be available into silex application, and also drupal 8 and ezpublish 5.

I open a PR, but my work is not finished. It could be great to have an early feedback (not about CS, just about architecture)

I read the codebase of bundles available on http://knpbundles.com/search?q=ldap but there is some issues with these bundles:

  • Dependency on Zend\Ldap (opensky)
  • A global username / password account is not needed to check is a user is able to connect to ldap. (all bundle)
  • The factory are no enough flexible.

Of course, I will rebase / squash all my commits before final merge (if happen).

I am really not sure about f40c85b

I tried to keep the default workflow of the user authentification
But with ldap, we can not retrieve user password. The only way to check if a user is in a ldap server, and if he is able to connect to the ldap, it is to try to connect the user against the ldap.

Then, we have an issue in src/Symfony/Bundle/SecurityBundle/DependencyInjection/Security/UserProvider/*Factory.php.
For exemple in https://github.com/lyrixx/symfony/blob/1834aaa11a9342455eaf30f1c6cd7ef69f951bdb/src/Symfony/Bundle/SecurityBundle/DependencyInjection/Security/Factory/FormLoginFactory.php#L65 the $provider (AuthentificationProvider) is hardcoded. It should not be.
So, just for my test, i changed this value (this change breaks tests).
But we have to find a way to do this cleanly. How can we do that ?

  • (A) Duplicate the factory (Not ok with that)
  • (B) Add a new argument createAuthProvider
  • ??

With A option, there will be lot of code.
With B option, we could add BC break.

...ndencyInjection/Security/UserProvider/LdapFactory.php
((38 lines not shown))
+ }
+ }
+
+ public function getKey()
+ {
+ return 'ldap';
+ }
+
+ public function addConfiguration(NodeDefinition $node)
+ {
+ $node
+ ->children()
+ ->scalarNode('host')->isRequired()->cannotBeEmpty()->end()
+ ->scalarNode('port')->defaultValue(389)->end()
+ ->scalarNode('dn')->isRequired()->cannotBeEmpty()->end()
+ ->scalarNode('username')->isRequired()->cannotBeEmpty()->end()
@lyrixx
lyrixx added a note

Not needed anymore.

@lyrixx
lyrixx added a note

Fixed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...uthentication/Provider/LdapAuthenticationProvider.php
((35 lines not shown))
+ $this->ldap = $ldap;
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ protected function checkAuthentication(UserInterface $user, UsernamePasswordToken $token)
+ {
+ if ("" === ($presentedPassword = $token->getCredentials())) {
+ throw new BadCredentialsException('The presented password cannot be empty.');
+ }
+
+ try {
+ // @TODO : How to achieved cleanly this hack ?
+ // The end dev should add this in the form ?
+ // But how to do that with http basic ?
@lyrixx
lyrixx added a note

It could be another configuration param ?

@lyrixx
lyrixx added a note

Added a new param.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@travisbot

This pull request fails (merged 1834aaa1 into 0ccda38).

...ndencyInjection/Security/Factory/FormLoginFactory.php
((7 lines not shown))
$container
- ->setDefinition($provider, new DefinitionDecorator('security.authentication.provider.dao'))
+ // @todo : Fix that hack
@stof Collaborator
stof added a note

This is really important as you are currently breaking everything for people not using ldap.

@lyrixx
lyrixx added a note

Yes of course. We have to find a clever way to fix that.

@stof Collaborator
stof added a note

IMO, you should add a separate factory for now instead of hacking the form_login one.

@lyrixx
lyrixx added a note

Ok.
This point is my real issue.

I can add another factory, but it will a copy / paste of FormLoginFactory.
Moreover, a start to work with FormLoginFactory, but I will have to duplicate HttpBasicFactory. And maybe others ...

So I don't think it's a good idea.

We need feedback here.

@stof Collaborator
stof added a note

Well, as I said during our IRC discussion last week, we indeed need to find a better way. But currently, your PR is breaking existing functionalities, so I don't think it is a good idea to keep it as is. Either refactor the factories to provide a clean way (which requires some work and is likely not so easy) or create another factory for now (which could extend from the form_login one to avoid duplicating everything)

@stof Collaborator
stof added a note

And btw, you are talking about duplicating the HttpBasicFactory. but your current PR does not support http_basic. And if you go the current way, you would be hacking the existing factory, thus breaking it too.

@lyrixx
lyrixx added a note

I totally agree with you. Of course I will not let this code in place.

So for now, i will duplicate the factory ..

@lyrixx
lyrixx added a note

Added 2 new factories.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/Symfony/Component/Security/.gitignore
@@ -1,2 +1,3 @@
vendor/
composer.lock
+Tests/bootstrap_ldap.php
@stof Collaborator
stof added a note

what is this ignored file ?

@lyrixx
lyrixx added a note

This is for tests of ldap/ldap class. May be we should remove tests of that class.

@lyrixx
lyrixx added a note

I removed all tests about ldap/ldap class

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...uthentication/Provider/LdapAuthenticationProvider.php
((36 lines not shown))
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ protected function checkAuthentication(UserInterface $user, UsernamePasswordToken $token)
+ {
+ if ("" === ($presentedPassword = $token->getCredentials())) {
+ throw new BadCredentialsException('The presented password cannot be empty.');
+ }
+
+ try {
+ // @TODO : How to achieved cleanly this hack ?
+ // The end dev should add this in the form ?
+ // But how to do that with http basic ?
+ $this->ldap->setUsername($token->getUser().'@SENSIO.local');
@stof Collaborator
stof added a note

what is this hardcoded Sensio stuff ?

@lyrixx
lyrixx added a note

This is a wip. Please read lines above

@lyrixx
lyrixx added a note

Fixed. Added a new param.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...ony/Component/Security/Core/User/LdapUserProvider.php
((25 lines not shown))
+
+ /**
+ * @param LdapInterface $ldap
+ */
+ public function __construct(LdapInterface $ldap)
+ {
+ $this->ldap = $ldap;
+ }
+
+ /**
+ * {@inheritDoc}
+ */
+ public function loadUserByUsername($username)
+ {
+ // @TODO : How to manage role ?
+ return new User($username, null, array('ROLE_USER'));
@stof Collaborator
stof added a note

This is wrong. refreshUser will accept any user, even if the username is invalid. It is a security issue

@lyrixx
lyrixx added a note

I don't think it is. The verification is done in LdapAuthenficationProvider.

@stof Collaborator
stof added a note

@lyrixx the LdapAuthenticationProvider is used when authenticating from the form, not when refreshing the user.

@lyrixx
lyrixx added a note

Fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/Symfony/Component/Security/Ldap/Ldap.php
((16 lines not shown))
+ private $port;
+ private $username;
+ private $password;
+ private $dn;
+ private $query;
+
+ private $connection;
+
+ public function __construct(
+ $host,
+ $port = 389,
+ $username = null,
+ $password = null,
+ $dn = null,
+ $query = null,
+ $filter = '*'
@stof Collaborator
stof added a note

Symfony does not wrap the constructor arguments.

@lyrixx
lyrixx added a note

Fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...uthentication/Provider/LdapAuthenticationProvider.php
((38 lines not shown))
+ /**
+ * {@inheritdoc}
+ */
+ protected function checkAuthentication(UserInterface $user, UsernamePasswordToken $token)
+ {
+ if ("" === ($presentedPassword = $token->getCredentials())) {
+ throw new BadCredentialsException('The presented password cannot be empty.');
+ }
+
+ try {
+ // @TODO : How to achieved cleanly this hack ?
+ // The end dev should add this in the form ?
+ // But how to do that with http basic ?
+ $this->ldap->setUsername($token->getUser().'@SENSIO.local');
+ $this->ldap->setPassword($presentedPassword);
+ $this->ldap->getConnection();
@stof Collaborator
stof added a note

these methods are not part of the LdapInterface expected in the constructor.

@lyrixx
lyrixx added a note

Fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof
Collaborator

Btw, @JMSBot found a few other issues as the symfony PRs are simulated in its test repository to test the code-review tool. You may want to take a look at it: JMSBot/test-repository#240 (note that it will not update the simulated PR for your new changes)

@travisbot

This pull request fails (merged dc9211eb into 0ccda38).

@travisbot

This pull request fails (merged 610e03ca into 0ccda38).

@travisbot

This pull request passes (merged 7a7dba1c into 0ccda38).

@travisbot

This pull request passes (merged 981f3b46 into 0ccda38).

...uthentication/Provider/LdapAuthenticationProvider.php
((51 lines not shown))
+ }
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ protected function retrieveUser($username, UsernamePasswordToken $token)
+ {
+ $user = $token->getUser();
+ if ($user instanceof UserInterface) {
+ return $user;
+ }
+
+ // @todo : how to manage roles ?
+ return new User($username, $token->getCredentials(), array('ROLE_USER'));
+ }
@stof Collaborator
stof added a note

why are you not using the user provider to retrieve the user ?

@lyrixx
lyrixx added a note

Because I need to store the password in the User.

@stof Collaborator
stof added a note

but this means that it is impossible to use a user provider, which seems quite strange

@lyrixx
lyrixx added a note

We can use a UserProvider, be we will have to add another User class which will support setPassword(). I think it's useless to add an overhead here.

UserProvider::loadUserByUsername is not possible due to ldap limitation. Thats why, IMHO it's easier to no use this method.

@lyrixx
lyrixx added a note

BTW : It's possible to check if a user exists in the ldap. But the LDAP will not return the password (plain text or not)...
Then, the query to find the user is very difficult to find.

@lyrixx
lyrixx added a note

Fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...ony/Component/Security/Core/User/LdapUserProvider.php
((45 lines not shown))
+
+ /**
+ * {@inheritDoc}
+ */
+ public function refreshUser(UserInterface $user)
+ {
+ if (!$user instanceof User) {
+ throw new UnsupportedUserException(sprintf('Instances of "%s" are not supported.', get_class($user)));
+ }
+
+ try {
+ $this->ldap->setUsername($user->getUsername());
+ $this->ldap->setPassword($user->getPassword());
+ $this->ldap->getConnection();
+ } catch (ConnectionException $e) {
+ throw new BadCredentialsException(sprintf('The presented password is invalid. "%s"', $e->getMessage()));
@stof Collaborator
stof added a note

this class should not throw a BadCredentialsException

@lyrixx
lyrixx added a note

What exception can be throw ?

@lyrixx
lyrixx added a note

Should I let the ConnectionException ?

@lyrixx
lyrixx added a note

Fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...ony/Component/Security/Core/User/LdapUserProvider.php
((42 lines not shown))
+ {
+ return new User($username);
+ }
+
+ /**
+ * {@inheritDoc}
+ */
+ public function refreshUser(UserInterface $user)
+ {
+ if (!$user instanceof User) {
+ throw new UnsupportedUserException(sprintf('Instances of "%s" are not supported.', get_class($user)));
+ }
+
+ try {
+ $this->ldap->setUsername($user->getUsername());
+ $this->ldap->setPassword($user->getPassword());
@stof Collaborator
stof added a note

if you are able to do this, you have an issue. It means that you stored the plain text password in the session

@lyrixx
lyrixx added a note

Yes, the plain-text password is store in the session.
What do you recommend ?

@lyrixx
lyrixx added a note

I can not figure out how to achieve that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...onent/Security/Ldap/Exception/ConnectionException.php
@@ -0,0 +1,20 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\Security\Ldap\Exception;
+
+/**
+ * ConnectionException is throwed if binding to ldap can not be established
@stof Collaborator
stof added a note

typo: thrown

@lyrixx
lyrixx added a note

fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...y/Component/Security/Ldap/Exception/LdapException.php
@@ -0,0 +1,20 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\Security\Ldap\Exception;
+
+/**
+ * LdapException is throwed if php ldap module is not laoded
@stof Collaborator
stof added a note

typo

@lyrixx
lyrixx added a note

Fixed thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@travisbot

This pull request fails (merged bb975f75 into 0ccda38).

@travisbot

This pull request fails (merged 2b67f3ea into 0ccda38).

@travisbot

This pull request fails (merged 0d15ef14 into 842b599).

@travisbot

This pull request passes (merged e548552c into 842b599).

@travisbot

This pull request passes (merged d9ab53ca into 842b599).

...ony/Component/Security/Core/User/LdapUserProvider.php
((29 lines not shown))
+ * @param LdapInterface $ldap
+ */
+ public function __construct(LdapInterface $ldap)
+ {
+ $this->ldap = $ldap;
+ }
+
+ /**
+ * {@inheritDoc}
+ *
+ * Due to Ldap limitation, this method should never be called.
+ * Call loadUserByUsernameAndPassword instead.
+ */
+ public function loadUserByUsername($username)
+ {
+ throw new \RuntimeException(sprintf('You sould not call the method "%s"', __METHOD__));
@stof Collaborator
stof added a note

this should be a BadMethodCallException

@lyrixx
lyrixx added a note

Fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on the diff
...ony/Component/Security/Core/User/LdapUserProvider.php
((54 lines not shown))
+ $this->ldap->setPassword($password);
+ $this->ldap->getConnection();
+ } catch (ConnectionException $e) {
+ throw new UsernameNotFoundException(sprintf('The presented password is invalid. "%s"', $e->getMessage()));
+ }
+
+ // @todo : how to manage roles ?
+ return new User($username, $password, array('ROLE_USER'));
+ }
+
+ /**
+ * {@inheritDoc}
+ */
+ public function refreshUser(UserInterface $user)
+ {
+ if (!$user instanceof User) {
@stof Collaborator
stof added a note

This method looks weird. This class is also used by the in_memory user provider, and in such case, getPassword would be the hashed password

@lyrixx
lyrixx added a note

It's a problem ? in this case, the ldap bind can be possible, so a UsernameNotFoundException will be throw.

@lyrixx
lyrixx added a note

Fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/Symfony/Component/Security/Ldap/LdapInterface.php
((13 lines not shown))
+ *
+ * @throws ConnectionException If username / password are not bindable
+ */
+ public function getConnection();
+
+ public function getPassword();
+
+ public function setPassword($password);
+
+ public function getPort();
+
+ public function setPort($port);
+
+ public function getUsername();
+
+ public function setUsername($username);
@stof Collaborator
stof added a note

missing phpdoc for all these methods

@lyrixx
lyrixx added a note

Fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@travisbot

This pull request passes (merged f77f02e4 into 842b599).

@lyrixx

At this point we have 3 issues :

  • (A) The username / password is stored in the session. I have to do that to make UserProvider::refreshUser works.
  • (B) The roles are hardcoded.
  • (C) Some factories are duplicated

(A) I don't know how to avoid that.
(B) We could add to LdapUserProviderInterface a new method : getRoles. This method could fetch into the ldap some information about roles. What do you think ?

@travisbot

This pull request fails (merged 8e65021b into 842b599).

@travisbot

This pull request passes (merged fd66fff2 into 842b599).

...ony/Component/Security/Core/User/LdapUserProvider.php
((29 lines not shown))
+ * @param LdapInterface $ldap
+ */
+ public function __construct(LdapInterface $ldap)
+ {
+ $this->ldap = $ldap;
+ }
+
+ /**
+ * {@inheritDoc}
+ *
+ * Due to Ldap limitation, this method should never be called.
+ * Call loadUserByUsernameAndPassword instead.
+ */
+ public function loadUserByUsername($username)
+ {
+ throw new \BadMethodCallException(sprintf('You sould not call the method "%s"', __METHOD__));
@relwell
relwell added a note

Isn't this a violation of the Liskov Substitution Principle?

@lyrixx
lyrixx added a note

I don't think, why ?

sould => should

@lyrixx
lyrixx added a note

@pborreli Thanks, fixed.

@schmittjoh +1 for OAuth. We will see this in another PR. Can you review this one ?

@relwell
relwell added a note

@lyrixx If the subclass method shouldn't be called because it isn't supported, then it violates the concept that you should be able to call any method in the subclass supported by the parent class. This violates the LSP because it means that you can't universally replace a different UserProvider class with the LdapUserProvider class, since we can expect that the loadByUserName method may possibly be called somewhere.

@lyrixx
lyrixx added a note

@relwell OK, i understand now. You are right.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@lyrixx

I rebase commit against master.

  • (A) I remove the password from the session. It will be ok if issue #4498 is fixed.
  • (B) Still here (role are hardcoded)
  • (C) Still here (factories duplication)
@beberlei

This is subject to ldap injection, please use ldap_escape() to escape username and group when seraching.

@asm89

Does this belong in core? How about CAS/oauth etc in that case? /cc @schmittjoh

@lyrixx

@beberlei Fixed

@asm89 Yes, it can be in the core : It can be usefull in silex, drupal ... And yes, it could be great to get oauth in core. BTW spring already embed all theses providers : https://github.com/SpringSource/spring-security

@sstok

Try using the mirror directly like http://fr.php.net/

@lyrixx

@sstok I tried :) http://fr.php.net/ldap_escape ... php.net is really unstable right now :(

@lyrixx

I rebased against master, and squashed my commits.

@titomiguelcosta

not sure if this is the right place, but here it goes: how can we implement an Ldap User Provider class when we do not have access to the user's password (plain or hashed)? Active directory does not allow read access to user's password, and since the loadUserByUsername method of the UserProviderInterface interface does not have access to the password typed, i see no option to check if the passwords match.

@relwell

This is handled during bind. You can't provide a user if you're not bound.

@nietonfir

What is the status of this PR? Does anyone have this working in the wild?

@lyrixx

I will work on this issue during this summer.

@nietonfir

@lyrixx Is there something specific to be done?

@lyrixx

see #5189 (comment) and my next comment.

@amenophis

@lyrixx Hi, i try put your code in my a bundle, and i got the error above.
Any idea about this issue ?

I'm in Symfony 2.3

RuntimeException: The definition "security.context_listener" has a reference to an abstract definition "security.authentication.provider.ldap". Abstract definitions cannot be the target of references.

And the config

security:
    encoders:
        FOS\UserBundle\Model\UserInterface: sha512

    role_hierarchy:
        ROLE_ADMIN:       ROLE_USER
        ROLE_SUPER_ADMIN: ROLE_ADMIN

    providers:
        fos_userbundle:
            id: fos_user.user_provider.username
        ldap:
            id: security.user.provider.ldap

    firewalls:
        dev:
            pattern:  ^/(_(profiler|wdt)|css|images|js)/
            security: false

        main:
            pattern: ^/
            form-login-ldap:
                provider: ldap
            logout:       true
            anonymous:    true

    access_control:
        - { path: ^/login$, role: IS_AUTHENTICATED_ANONYMOUSLY }
        - { path: ^/register, role: IS_AUTHENTICATED_ANONYMOUSLY }
        - { path: ^/resetting, role: IS_AUTHENTICATED_ANONYMOUSLY }
        - { path: ^/admin/, role: ROLE_ADMIN }
        - { path: ^/, role: ROLE_USER }
@lyrixx

@amenophis no sorry. But someone took this code and created a bundle. But I cant remember who it was.

@amenophis

hi @relwell I tried it, but it seems not working when it try to connect to ldap with user credential.

I got Operations error message.

Can you help me ?

@relwell

@amenophis please open a ticket on the repo and I'll try to get to it soon.

@damienalexandre

DapsBundle is great but the Security Component deserve a proper Ldap support, :+1: for this as a Silex user.

@ghost

ubot detected some small issues in this pull request.

You can make the modifications by hand or run the following command from the repository root directory:

curl http://ubot.io/patch/symfony/symfony/5189.diff | patch -p0
diff -ru src/Symfony/Bundle/SecurityBundle/DependencyInjection/Security/UserProvider/LdapFactory.php src/Symfony/Bundle/SecurityBundle/DependencyInjection/Security/UserProvider/LdapFactory.php
--- src/Symfony/Bundle/SecurityBundle/DependencyInjection/Security/UserProvider/LdapFactory.php 2013-12-15 20:41:32.483122873 +0000
+++ src/Symfony/Bundle/SecurityBundle/DependencyInjection/Security/UserProvider/LdapFactory.php 2013-12-15 20:41:34.351122873 +0000
@@ -67,7 +67,7 @@
                 ->scalarNode('use_start_tls')->defaultFalse()->end()
                 ->scalarNode('opt_referrals')->defaultFalse()->end()
                 ->arrayNode('default_roles')
-                    ->beforeNormalization()->ifString()->then(function($v) { return preg_split('/\s*,\s*/', $v); })->end()
+                    ->beforeNormalization()->ifString()->then(function ($v) { return preg_split('/\s*,\s*/', $v); })->end()
                     ->prototype('scalar')->end()
                 ->end()
             ->end()
diff -ru src/Symfony/Component/Security/Ldap/LdapInterface.php src/Symfony/Component/Security/Ldap/LdapInterface.php
--- src/Symfony/Component/Security/Ldap/LdapInterface.php   2013-12-15 20:41:33.883122873 +0000
+++ src/Symfony/Component/Security/Ldap/LdapInterface.php   2013-12-15 20:41:36.027122873 +0000
@@ -174,7 +174,7 @@
     /*
      * find a username into ldap connection
      *
-     * @todo : not needed anymore. But could be usefull to retrieve roles
+     * @todo : not needed anymore. But could be useful to retrieve roles
      *
      * @param  string     $username
      * @param  string     $query
@jakzal
Collaborator

@lyrixx do you plan to finish this one?

@lyrixx
@alex-git-hub

@lyrixx Hello,
I am looking for Ldap Bundle and have used BorisMorel`s Bundle yet, but it has not been useful for my issue finally. Maybe, can you suggest some extensible bundle, although there is almost no choice ?

@ylynfatt

Has this officially been added to Symfony yet?

@cordoval

it obviously has not @ylynfatt because it is not merged, it is not a finished PR either

@ylynfatt

Hi @lyrixx Are the outstanding issues the same as those you mentioned in your initial pull request comment? is there anything else the community can do to help? Or is there anything from @relwell's DapsBundle that can be added to your code?

@Renrhaf

Hello, is the goal of this PR to add an authentication provider against a simple LDAP bind ?

@ylynfatt

@Renrhaf I believe so.

@iltar

Any updates on this PR? I'm quite interested to see this implemented by default.

@phalcon phalcon referenced this pull request in phalcon/cphalcon
Open

The brand-new Phalcon/Zephir development workflow #3246

@ChadSikorra ChadSikorra commented on the diff
src/Symfony/Component/Security/Ldap/Ldap.php
((304 lines not shown))
+
+ private function connect()
+ {
+ if (!$this->connection) {
+ $host = $this->getHost();
+ if ($this->getUseSsl()) {
+ $host = 'ldaps://' . $host;
+ }
+ ldap_set_option($this->connection, LDAP_OPT_PROTOCOL_VERSION, $this->getVersion());
+ ldap_set_option($this->connection, LDAP_OPT_REFERRALS, $this->getOptReferrals());
+
+ if ($this->getUseStartTls()) {
+ ldap_start_tls($this->connection);
+ }
+
+ $this->connection = ldap_connect($host, $this->getPort());

You should do the ldap_connect() first, then do the ldap_set_option() and ldap_start_tls().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jakzal
Collaborator

@lyrixx if you don't have time to work on this, or there's no one to take over, I think it's better to close this PR.

I'm not fully convinced we need this in the core by the way. If the existing bundles are no good, perhaps its easier to fix them, or implement a new bundle?

@lyrixx

@jakzal Actually, I don't need this PR anymore. So it's a bit hard for me to find time and motivation to finish it.

But I'm not sure closing this PR is our best option. I know Everybody want less open issue / PR, but This PR is almost complete. I (really) will try to finish it ! ;)

@fabpot
Owner

... and I and it looks like a lot of people are looking for a good LDAP support in the Symfony Security Component.

@jakzal
Collaborator

@lyrixx let me know if I can help with finishing it off in any way.

@csarrazi

No personal feelings, here, but two things are wrong in this PR:

  • Why would we need to keep the password in the session?
  • Why would we need to call the ldap_bind() method for each request?

We only need to

  • either call ldap_bind() in the authenticator
  • or compare the provided password with the LDAP attributes

and do this once. The only case where we would need to do this for every request is if we are using HTTP Basic as the authentication mechanism.

Binding to an LDAP means initiating a session on the server. However, there is no support in PHP for keeping alive the LDAP session (the connection is not persistent). Thus, using the ldap_bind() for every subsequent request also means potentially creating hundreds of LDAP sessions on the directory service. Using ldap_bind() is also not possible without:

  • either using HTTP Basic as an authentication mechanism (which sends the password for each request)
  • or keeping the user's password in the session (which would induce a security flaw)

This also means that apart from HTTP Basic, stateless authentication mechanisms won't work with the current implementation, which basically makes the user provider useless.

Until we can actually use persistent LDAP connections, or until we have a broad usage of PHP applications with WSGI-like capability, we should not call the ldap_bind() method in the UserProvider.

Until then, for a correct implementation, we would need:

  • An LDAP user provider which simply fetches the DN from the LDAP directory, which involves a search in the directory, unless the exact mapping is already known.
  • Two LDAP authentication providers, which either:
    • binds the user to the LDAP connection on first request (authentication)
    • compares the encrypted password from the LDAP information
  • Loading the list of roles of the user from the LDAP attributes.

This also means that we don't need to create a new method (loadUserByUsernameAndPassword) in the new user provider (which currently does way more it is supposed to do).

@amenophis amenophis referenced this pull request in tempo-project/tempo
Open

Allow LDAP authentication #87

@amenophis

@lyrixx Should we wait for something new about this PR ?

@csarrazi

Actually, I'm working on this PR :)

I'll give more info next week, as the code has received a major overhaul. :)

@amenophis

Really a good news to hear this !
If i can help you, don't hesitate !

@csarrazi

Here is the current status:

  • LDAP Authentication providers
    • LDAP HTTP Basic authentication provider
    • LDAP Form login authentication provider
  • LDAP User provider

In the updated PR, the bind() method is only used when we want to actually check the user's credentials. This means:

  • For each request, if we are using the LDAP HTTP Basic authentication provider
  • When checking the username and password, during login check, for the LDAP Form login authentication provider
  • When the LDAP User provider actually needs to authenticate to the LDAP to issue queries

Regardless, as seen with @fabpot last day, I should be able to issue a WIP PR against the 2.8 branch by sunday (tests won't be working yet, though).

@amenophis

@csarrazi Do you have news on this PR ?

@csarrazi

Yes. The groundwork is basically done, but needs some cleanup.

Also, I'm working on an edge case, where a developer would like to use both the LDAP user provider and an LDAP authentication provider.

In the current implementation, each use a different instance of the LDAP service.
To support this case, we would need to either

  • duplicate the configuration of the LDAP services (one in the authentication provider's factory, another in the user provider's factory), and have two different instances of LDAP services (and connections),
  • or use the same LDAP service, but this means that the common LDAP configuration would need to be set neither in the providers node nor the firewalls section.

In short, it's basically something like:

security:
    providers:
        ldap_users:
            ldap:
                host: ldap.mydomain.tld
                uid_key: sAMAccountName
                base_dn: my_base_dn
                bind_dn: default_bind_dn
                password: P4$$w0rd
                filter: my_filter_string
    firewalls:
        demo_secured_area:
            stateless: true
            provider: ldap_users
            pattern:    ^/
            http_basic_ldap:
                host: ldap.mydomain.tld
                base: dc=mydomain,dc=local
                username_suffix: "@MYDOMAIN"

vs

services:
    my_ldap_service_id:
        arguments: [ ldap.mydomain.tld, my_base_dn, default_bind_dn, my_filter_string, p4$$w0rd ]

security:
    providers:
        ldap_users:
            ldap:
                id: my_ldap_service_id
                filter: my_filter_string
                uid_key: sAMAccountName
    firewalls:
        demo_secured_area:
            stateless: true
            provider: ldap_users
            pattern:    ^/
            http_basic_ldap:
                id: my_ldap_service_id
                username_suffix: "@MYDOMAIN"

Of course, we support both ways: if a service is provided, then we use a pre-configured service. Otherwise, we create a new service instance.

Regardless of whether we chose the a common service or two separate services, I would actually make another change, in the authentication providers: currently, all authentication providers start by fetching the user (from the user provider), before authenticating the user (call the bind() method). But to support this specific case, inverting the logic would prevent us to make a double bind() (one for the search user, another authenticating the actual user).

Does this seem okay for everyone?

@iltar

In my opinion the configuration is supposed to be done in your user-provider and not your firewall. LDAP configuration is not related to your firewall, but just how you load the users. I'm not sure why it's even required to create "http_basic_ldap" and the form variant, what's wrong with the current implementations (or the Simple* variants for that matter), where you have to implement more than a user provider?

@csarrazi

You don't necessarily have to use the http_basic_ldap or form_login_ldap.

These are only for authenticating against LDAP (i.e. authenticating using ldap_bind(), instead of comparing password hashes as, depending on your LDAP server's configuration, you may not receive the password hash in the LDAP info.

Thus, a different password validation strategy must be used with LDAP.

The LDAP user provider's goal is to actually fetch the user's information from the LDAP.

If your LDAP server returns the password hash, then you're good to go simply with the LDAP user provider (using either anonymous search, or a search user's credentials) + any "normal" firewall configuration.
However, if you don't have the password hash in the info (which is the case with most LDAP implementations), the only way to validate your credentials is by using the ldap_bind() PHP function.

For this, you either need:

  • A new authentication provider
  • implement a strategy to switch between password comparison and ldap_bind() in the DAO authentication provider

Also, setting the authentication provider needs to be done either by overriding HttpBasicFactory::create() and FormLoginFactory:createAuthProvider() using class inheritance, or by introducing a new option in both factories, which can be used to switch between the DAO authentication provider and LDAPBindAuthenticationProvider, and setting the default to the DAO authentication provider.

Creating new firewall factories is not necessary. But my work is based on the original PR, in which both factories were introduced (HttpBasicLdapFactory and FormLoginLdapFactory). Currently, I feel that those are not necessary, and I would actually prefer the using an option to switch between the dao or ldap_bind authentication providers.

The reason behind separating the LDAP user provider from the authentication provider is that you may use ldap_bind() to authenticate your users, while fetching them from another data source (entity, in_memory, etc.), or use password comparison while fetching users from an LDAP server.

@iltar

I propose to use an implementation SimpleAuthenticatorInterface instead of the DAO.

@csarrazi

Point taken. The base PR is quite old (from 2012), and the SimpleAuthenticator didn't exist at the time. But we're getting there! ;)

@csarrazi csarrazi referenced this pull request
Open

[WIP][Security] Added Ldap provider #14602

1 of 3 tasks complete
@csarrazi

I've released my PR for this issue. I would be glad to receive some feedback on this.

@lyrixx

Closing in favor of #14602

@lyrixx lyrixx closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Oct 5, 2012
  1. @lyrixx

    [Security] Added ldap

    lyrixx authored
  2. @lyrixx

    Fixed tests

    lyrixx authored
  3. @lyrixx

    Added default roles options

    lyrixx authored
  4. @lyrixx

    Fixed php doc

    lyrixx authored
This page is out of date. Refresh to see the latest.
Showing with 1,161 additions and 0 deletions.
  1. +41 −0 src/Symfony/Bundle/SecurityBundle/DependencyInjection/Security/Factory/FormLoginLdapFactory.php
  2. +51 −0 src/Symfony/Bundle/SecurityBundle/DependencyInjection/Security/Factory/HttpBasicLdapFactory.php
  3. +76 −0 src/Symfony/Bundle/SecurityBundle/DependencyInjection/Security/UserProvider/LdapFactory.php
  4. +11 −0 src/Symfony/Bundle/SecurityBundle/Resources/config/security.xml
  5. +8 −0 src/Symfony/Bundle/SecurityBundle/Resources/config/security_listeners.xml
  6. +6 −0 src/Symfony/Bundle/SecurityBundle/SecurityBundle.php
  7. +77 −0 src/Symfony/Component/Security/Core/Authentication/Provider/LdapAuthenticationProvider.php
  8. +95 −0 src/Symfony/Component/Security/Core/User/LdapUserProvider.php
  9. +47 −0 src/Symfony/Component/Security/Core/User/LdapUserProviderInterface.php
  10. +21 −0 src/Symfony/Component/Security/Ldap/Exception/ConnectionException.php
  11. +21 −0 src/Symfony/Component/Security/Ldap/Exception/LdapException.php
  12. +370 −0 src/Symfony/Component/Security/Ldap/Ldap.php
  13. +185 −0 src/Symfony/Component/Security/Ldap/LdapInterface.php
  14. +82 −0 src/Symfony/Component/Security/Tests/Core/Authentication/Provider/LdapAuthenticationProviderTest.php
  15. +69 −0 src/Symfony/Component/Security/Tests/Core/User/LdapUserProviderTest.php
  16. +1 −0  src/Symfony/Component/Security/composer.json
View
41 src/Symfony/Bundle/SecurityBundle/DependencyInjection/Security/Factory/FormLoginLdapFactory.php
@@ -0,0 +1,41 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Bundle\SecurityBundle\DependencyInjection\Security\Factory;
+
+use Symfony\Component\DependencyInjection\DefinitionDecorator;
+use Symfony\Component\DependencyInjection\ContainerBuilder;
+use Symfony\Component\DependencyInjection\Reference;
+
+/**
+ * FormLoginLdapFactory creates services for form login ldap authentication.
+ *
+ * @author Grégoire Pineau <lyrixx@lyrixx.info>
+ */
+class FormLoginLdapFactory extends FormLoginFactory
+{
+ protected function createAuthProvider(ContainerBuilder $container, $id, $config, $userProviderId)
+ {
+ $provider = 'security.authentication.provider.ldap.'.$id;
+ $container
+ ->setDefinition($provider, new DefinitionDecorator('security.authentication.provider.ldap'))
+ ->replaceArgument(0, new Reference($userProviderId))
+ ->replaceArgument(2, $id)
+ ;
+
+ return $provider;
+ }
+
+ public function getKey()
+ {
+ return 'form-login-ldap';
+ }
+}
View
51 src/Symfony/Bundle/SecurityBundle/DependencyInjection/Security/Factory/HttpBasicLdapFactory.php
@@ -0,0 +1,51 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Bundle\SecurityBundle\DependencyInjection\Security\Factory;
+
+use Symfony\Component\DependencyInjection\DefinitionDecorator;
+use Symfony\Component\DependencyInjection\ContainerBuilder;
+use Symfony\Component\DependencyInjection\Reference;
+
+/**
+ * HttpBasicFactory creates services for HTTP basic authentication.
+ *
+ * @author Fabien Potencier <fabien@symfony.com>
+ * @author Grégoire Pineau <lyrixx@lyrixx.info>
+ */
+class HttpBasicLdapFactory extends HttpBasicFactory
+{
+ public function create(ContainerBuilder $container, $id, $config, $userProvider, $defaultEntryPoint)
+ {
+ $provider = 'security.authentication.provider.ldap.'.$id;
+ $container
+ ->setDefinition($provider, new DefinitionDecorator('security.authentication.provider.ldap'))
+ ->replaceArgument(0, new Reference($userProvider))
+ ->replaceArgument(2, $id)
+ ;
+
+ // entry point
+ $entryPointId = $this->createEntryPoint($container, $id, $config, $defaultEntryPoint);
+
+ // listener
+ $listenerId = 'security.authentication.listener.basic.'.$id;
+ $listener = $container->setDefinition($listenerId, new DefinitionDecorator('security.authentication.listener.basic'));
+ $listener->replaceArgument(2, $id);
+ $listener->replaceArgument(3, new Reference($entryPointId));
+
+ return array($provider, $listenerId, $entryPointId);
+ }
+
+ public function getKey()
+ {
+ return 'http-basic-ldap';
+ }
+}
View
76 src/Symfony/Bundle/SecurityBundle/DependencyInjection/Security/UserProvider/LdapFactory.php
@@ -0,0 +1,76 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Bundle\SecurityBundle\DependencyInjection\Security\UserProvider;
+
+use Symfony\Component\Config\Definition\Builder\NodeDefinition;
+
+use Symfony\Bundle\SecurityBundle\DependencyInjection\Security\UserProvider\UserProviderFactoryInterface;
+use Symfony\Component\DependencyInjection\DefinitionDecorator;
+use Symfony\Component\DependencyInjection\ContainerBuilder;
+use Symfony\Component\DependencyInjection\Reference;
+
+/**
+ * LdapFactory creates services for Ldap user provider.
+ *
+ * @author Grégoire Pineau <lyrixx@lyrixx.info>
+ */
+class LdapFactory implements UserProviderFactoryInterface
+{
+ private $key;
+ private $providerId;
+
+ public function create(ContainerBuilder $container, $id, $config)
+ {
+ $container
+ ->setDefinition('security.ldap.ldap.'.$id , new DefinitionDecorator('security.ldap.ldap'))
+ ->addArgument($config['host'])
+ ->addArgument($config['port'])
+ ->addArgument($config['dn'])
+ ->addArgument($config['username_suffix'])
+ ->addArgument($config['version'])
+ ->addArgument($config['use_ssl'])
+ ->addArgument($config['use_start_tls'])
+ ->addArgument($config['opt_referrals'])
+ ;
+
+ $container
+ ->setDefinition($id, new DefinitionDecorator('security.user.provider.ldap'))
+ ->replaceArgument(0, new Reference('security.ldap.ldap.'.$id))
+ ->replaceArgument(1, $config['default_roles'])
+ ;
+ }
+
+ public function getKey()
+ {
+ return 'ldap';
+ }
+
+ public function addConfiguration(NodeDefinition $node)
+ {
+ $node
+ ->children()
+ ->scalarNode('host')->isRequired()->cannotBeEmpty()->end()
+ ->scalarNode('port')->cannotBeEmpty()->defaultValue(389)->end()
+ ->scalarNode('dn')->isRequired()->cannotBeEmpty()->end()
+ ->scalarNode('username_suffix')->defaultValue('')->end()
+ ->scalarNode('version')->defaultValue(3)->end()
+ ->scalarNode('use_ssl')->defaultFalse()->end()
+ ->scalarNode('use_start_tls')->defaultFalse()->end()
+ ->scalarNode('opt_referrals')->defaultFalse()->end()
+ ->arrayNode('default_roles')
+ ->beforeNormalization()->ifString()->then(function($v) { return preg_split('/\s*,\s*/', $v); })->end()
+ ->prototype('scalar')->end()
+ ->end()
+ ->end()
+ ;
+ }
+}
View
11 src/Symfony/Bundle/SecurityBundle/Resources/config/security.xml
@@ -15,6 +15,7 @@
<parameter key="security.user.provider.in_memory.class">Symfony\Component\Security\Core\User\InMemoryUserProvider</parameter>
<parameter key="security.user.provider.in_memory.user.class">Symfony\Component\Security\Core\User\User</parameter>
+ <parameter key="security.user.provider.ldap.class">Symfony\Component\Security\Core\User\LdapUserProvider</parameter>
<parameter key="security.user.provider.chain.class">Symfony\Component\Security\Core\User\ChainUserProvider</parameter>
<parameter key="security.authentication.trust_resolver.class">Symfony\Component\Security\Core\Authentication\AuthenticationTrustResolver</parameter>
@@ -41,6 +42,8 @@
<parameter key="security.http_utils.class">Symfony\Component\Security\Http\HttpUtils</parameter>
<parameter key="security.validator.user_password.class">Symfony\Component\Security\Core\Validator\Constraint\UserPasswordValidator</parameter>
+
+ <parameter key="security.ldap.ldap.class">Symfony\Component\Security\Ldap\Ldap</parameter>
</parameters>
<services>
@@ -124,6 +127,11 @@
<service id="security.user.provider.in_memory" class="%security.user.provider.in_memory.class%" abstract="true" public="false" />
<service id="security.user.provider.in_memory.user" class="%security.user.provider.in_memory.user.class%" abstract="true" public="false" />
+ <service id="security.user.provider.ldap" class="%security.user.provider.ldap.class%" abstract="true" public="false">
+ <argument /> <!-- security.ldap.ldap -->
+ <argument /> <!-- default_roles -->
+ </service>
+
<service id="security.user.provider.chain" class="%security.user.provider.chain.class%" abstract="true" public="false" />
<service id="security.http_utils" class="%security.http_utils.class%" public="false">
@@ -137,5 +145,8 @@
<argument type="service" id="security.context" />
<argument type="service" id="security.encoder_factory" />
</service>
+
+ <!-- Other -->
+ <service id="security.ldap.ldap" class="%security.ldap.ldap.class%" abstract="true" public="false"/>
</services>
</container>
View
8 src/Symfony/Bundle/SecurityBundle/Resources/config/security_listeners.xml
@@ -35,6 +35,7 @@
<parameter key="security.context_listener.class">Symfony\Component\Security\Http\Firewall\ContextListener</parameter>
<parameter key="security.authentication.provider.dao.class">Symfony\Component\Security\Core\Authentication\Provider\DaoAuthenticationProvider</parameter>
+ <parameter key="security.authentication.provider.ldap.class">Symfony\Component\Security\Core\Authentication\Provider\LdapAuthenticationProvider</parameter>
<parameter key="security.authentication.provider.pre_authenticated.class">Symfony\Component\Security\Core\Authentication\Provider\PreAuthenticatedAuthenticationProvider</parameter>
<parameter key="security.authentication.provider.anonymous.class">Symfony\Component\Security\Core\Authentication\Provider\AnonymousAuthenticationProvider</parameter>
@@ -170,6 +171,13 @@
<argument>%security.authentication.hide_user_not_found%</argument>
</service>
+ <service id="security.authentication.provider.ldap" class="%security.authentication.provider.ldap.class%" abstract="true" public="false">
+ <argument /> <!-- User Provider -->
+ <argument type="service" id="security.user_checker" />
+ <argument /> <!-- Provider-shared Key -->
+ <argument>%security.authentication.hide_user_not_found%</argument>
+ </service>
+
<service id="security.authentication.provider.pre_authenticated" class="%security.authentication.provider.pre_authenticated.class%" abstract="true" public="false">
<argument /> <!-- User Provider -->
<argument type="service" id="security.user_checker" />
View
6 src/Symfony/Bundle/SecurityBundle/SecurityBundle.php
@@ -15,11 +15,14 @@
use Symfony\Component\DependencyInjection\ContainerBuilder;
use Symfony\Bundle\SecurityBundle\DependencyInjection\Compiler\AddSecurityVotersPass;
use Symfony\Bundle\SecurityBundle\DependencyInjection\Security\Factory\FormLoginFactory;
+use Symfony\Bundle\SecurityBundle\DependencyInjection\Security\Factory\FormLoginLdapFactory;
use Symfony\Bundle\SecurityBundle\DependencyInjection\Security\Factory\HttpBasicFactory;
+use Symfony\Bundle\SecurityBundle\DependencyInjection\Security\Factory\HttpBasicLdapFactory;
use Symfony\Bundle\SecurityBundle\DependencyInjection\Security\Factory\HttpDigestFactory;
use Symfony\Bundle\SecurityBundle\DependencyInjection\Security\Factory\RememberMeFactory;
use Symfony\Bundle\SecurityBundle\DependencyInjection\Security\Factory\X509Factory;
use Symfony\Bundle\SecurityBundle\DependencyInjection\Security\UserProvider\InMemoryFactory;
+use Symfony\Bundle\SecurityBundle\DependencyInjection\Security\UserProvider\LdapFactory;
/**
* Bundle.
@@ -34,12 +37,15 @@ public function build(ContainerBuilder $container)
$extension = $container->getExtension('security');
$extension->addSecurityListenerFactory(new FormLoginFactory());
+ $extension->addSecurityListenerFactory(new FormLoginLdapFactory());
$extension->addSecurityListenerFactory(new HttpBasicFactory());
+ $extension->addSecurityListenerFactory(new HttpBasicLdapFactory());
$extension->addSecurityListenerFactory(new HttpDigestFactory());
$extension->addSecurityListenerFactory(new RememberMeFactory());
$extension->addSecurityListenerFactory(new X509Factory());
$extension->addUserProviderFactory(new InMemoryFactory());
+ $extension->addUserProviderFactory(new LdapFactory());
$container->addCompilerPass(new AddSecurityVotersPass());
}
}
View
77 src/Symfony/Component/Security/Core/Authentication/Provider/LdapAuthenticationProvider.php
@@ -0,0 +1,77 @@
+<?php
+
+namespace Symfony\Component\Security\Core\Authentication\Provider;
+
+use Symfony\Component\Security\Core\Authentication\Token\UsernamePasswordToken;
+use Symfony\Component\Security\Core\Exception\AuthenticationServiceException;
+use Symfony\Component\Security\Core\Exception\BadCredentialsException;
+use Symfony\Component\Security\Core\Exception\UsernameNotFoundException;
+use Symfony\Component\Security\Core\User\LdapUserProviderInterface;
+use Symfony\Component\Security\Core\User\User;
+use Symfony\Component\Security\Core\User\UserCheckerInterface;
+use Symfony\Component\Security\Core\User\UserInterface;
+
+/**
+ * LdapAuthenticationProvider uses a LdapUserProviderInterface to retrieve the user
+ * for a UsernamePasswordToken.
+ *
+ * The only way to check user credentials is to try to connect the user with its
+ * credentials to the ldap.
+ *
+ * @author Grégoire Pineau <lyrixx@lyrixx.info>
+ */
+class LdapAuthenticationProvider extends UserAuthenticationProvider
+{
+ private $userProvider;
+ private $ldap;
+
+ /**
+ * Constructor.
+ *
+ * @param LdapUserProviderInterface $userProvider An LdapUserProvider instance
+ * @param UserCheckerInterface $userChecker An UserCheckerInterface instance
+ * @param string $providerKey The provider key
+ * @param Boolean $hideUserNotFoundExceptions Whether to hide user not found exception or not
+ */
+ public function __construct(LdapUserProviderInterface $userProvider, UserCheckerInterface $userChecker, $providerKey, $hideUserNotFoundExceptions = true)
+ {
+ parent::__construct($userChecker, $providerKey, $hideUserNotFoundExceptions);
+
+ $this->userProvider = $userProvider;
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ protected function checkAuthentication(UserInterface $user, UsernamePasswordToken $token)
+ {
+ if ("" === ($presentedPassword = $token->getCredentials())) {
+ throw new BadCredentialsException('The presented password cannot be empty.');
+ }
+
+ // At this point, the $user is already authenticated
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ protected function retrieveUser($username, UsernamePasswordToken $token)
+ {
+ $user = $token->getUser();
+ if ($user instanceof UserInterface) {
+ return $user;
+ }
+
+ try {
+ $user = $this->userProvider->loadUserByUsernameAndPassword($username, $token->getCredentials());
+
+ if (!$user instanceof UserInterface) {
+ throw new AuthenticationServiceException('The user provider must return a UserInterface object.');
+ }
+
+ return $user;
+ } catch (UsernameNotFoundException $notFound) {
+ throw $notFound;
+ }
+ }
+}
View
95 src/Symfony/Component/Security/Core/User/LdapUserProvider.php
@@ -0,0 +1,95 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\Security\Core\User;
+
+use Symfony\Component\Security\Core\Exception\UnsupportedUserException;
+use Symfony\Component\Security\Core\Exception\UsernameNotFoundException;
+use Symfony\Component\Security\Ldap\Exception\ConnectionException;
+use Symfony\Component\Security\Ldap\LdapInterface;
+
+/**
+ * LdapUserProvider is a simple user provider on top of ldap.
+ *
+ * @author Grégoire Pineau <lyrixx@lyrixx.info>
+ */
+class LdapUserProvider implements LdapUserProviderInterface
+{
+ private $ldap;
+ private $defaultRoles;
+
+ /**
+ * @param LdapInterface $ldap
+ * @param array $defaultRoles
+ */
+ public function __construct(LdapInterface $ldap, array $defaultRoles = array())
+ {
+ $this->ldap = $ldap;
+ $this->defaultRoles = $defaultRoles;
+ }
+
+ /**
+ * {@inheritDoc}
+ *
+ * Due to Ldap limitation, this method should never be called.
+ * Call loadUserByUsernameAndPassword instead.
+ */
+ public function loadUserByUsername($username)
+ {
+ throw new \BadMethodCallException(sprintf('You should not call the method "%s"', __METHOD__));
+ }
+
+ /**
+ * {@inheritDoc}
+ */
+ public function loadUserByUsernameAndPassword($username, $password)
+ {
+ $this->ldap->setUsername($username);
+ $this->ldap->setPassword($password);
+
+ try {
+ $this->ldap->bind();
+ } catch (ConnectionException $e) {
+ throw new UsernameNotFoundException(sprintf('The presented password is invalid. "%s"', $e->getMessage()));
+ }
+
+ return new User($username, null, $this->loadRoles());
+ }
+
+ /**
+ * {@inheritDoc}
+ */
+ public function loadRoles()
+ {
+ return $this->defaultRoles;
+ }
+
+ /**
+ * {@inheritDoc}
+ */
+ public function refreshUser(UserInterface $user)
+ {
+ if (!$user instanceof User) {
@stof Collaborator
stof added a note

This method looks weird. This class is also used by the in_memory user provider, and in such case, getPassword would be the hashed password

@lyrixx
lyrixx added a note

It's a problem ? in this case, the ldap bind can be possible, so a UsernameNotFoundException will be throw.

@lyrixx
lyrixx added a note

Fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ throw new UnsupportedUserException(sprintf('Instances of "%s" are not supported.', get_class($user)));
+ }
+
+ return new User($user->getUsername(), null, $user->getRoles());
+ }
+
+ /**
+ * {@inheritDoc}
+ */
+ public function supportsClass($class)
+ {
+ return $class === 'Symfony\Component\Security\Core\User\User';
+ }
+
+}
View
47 src/Symfony/Component/Security/Core/User/LdapUserProviderInterface.php
@@ -0,0 +1,47 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\Security\Core\User;
+
+use Symfony\Component\Security\Core\Exception\UsernameNotFoundException;
+
+/**
+ * LdapUserProviderInterface.
+ *
+ * @author Grégoire Pineau <lyrixx@lyrixx.info>
+ */
+interface LdapUserProviderInterface extends UserProviderInterface
+{
+ /**
+ * Loads the user for the given username and password.
+ *
+ * This method must throw UsernameNotFoundException if the user is not
+ * found.
+ *
+ * @param string $username The username
+ * @param password $username The password
+ *
+ * @return UserInterface
+ *
+ * @see UsernameNotFoundException
+ *
+ * @throws UsernameNotFoundException if the user is not found
+ *
+ */
+ public function loadUserByUsernameAndPassword($username, $password);
+
+ /**
+ * Return roles for current user
+ *
+ * @return array roles
+ */
+ public function loadRoles();
+}
View
21 src/Symfony/Component/Security/Ldap/Exception/ConnectionException.php
@@ -0,0 +1,21 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\Security\Ldap\Exception;
+
+/**
+ * ConnectionException is throw if binding to ldap can not be established
+ *
+ * @author Grégoire Pineau <lyrixx@lyrixx.info>
+ */
+class ConnectionException extends \RuntimeException
+{
+}
View
21 src/Symfony/Component/Security/Ldap/Exception/LdapException.php
@@ -0,0 +1,21 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\Security\Ldap\Exception;
+
+/**
+ * LdapException is throw if php ldap module is not laoded
+ *
+ * @author Grégoire Pineau <lyrixx@lyrixx.info>
+ */
+class LdapException extends \RuntimeException
+{
+}
View
370 src/Symfony/Component/Security/Ldap/Ldap.php
@@ -0,0 +1,370 @@
+<?php
+
+namespace Symfony\Component\Security\Ldap;
+
+use Symfony\Component\Security\Ldap\Exception\ConnectionException;
+use Symfony\Component\Security\Ldap\Exception\LdapException;
+
+/*
+ * @author Grégoire Pineau <lyrixx@lyrixx.info>
+ * @author Francis Besset <francis.besset@gmail.com>
+ */
+class Ldap implements LdapInterface
+{
+ private $host;
+ private $port;
+ private $dn;
+ private $usernameSuffix;
+ private $version;
+ private $useSsl;
+ private $useStartTls;
+ private $optReferrals;
+ private $username;
+ private $password;
+
+ private $connection;
+
+ /**
+ * contructor
+ *
+ * @param string $host
+ * @param integer $port
+ * @param string $dn
+ * @param string $usernameSuffix
+ * @param integer $version
+ * @param boolean $useSsl
+ * @param boolean $useStartTls
+ * @param boolean $optReferrals
+ */
+ public function __construct($host = null, $port = 389, $dn = null, $usernameSuffix = null, $version = 3, $useSsl = false, $useStartTls = false, $optReferrals = false)
+ {
+ if (!extension_loaded('ldap')) {
+ throw new LdapException('Ldap module is needed.');
+ }
+
+ $this->host = $host;
+ $this->port = $port;
+ $this->dn = $dn;
+ $this->usernameSuffix = $usernameSuffix;
+ $this->version = $version;
+ $this->useSsl = (boolean) $useSsl;
+ $this->useStartTls = (boolean) $useStartTls;
+ $this->optReferrals = (boolean) $optReferrals;
+
+ $this->connection = null;
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function getConnection()
+ {
+ if (!$this->connection) {
+ $this->connect();
+ }
+
+ return $this->connection;
+ }
+
+ public function __destruct()
+ {
+ $this->disconnect();
+ }
+
+ public function bind()
+ {
+ if (!$this->connection) {
+ $this->connect();
+ }
+
+ if (false === @ldap_bind($this->connection, $this->getUsernameWithSuffix(), $this->getPassword())) {
+ throw new ConnectionException(sprintf('Username / password invalid to connect on Ldap server %s:%s', $this->host, $this->port));
+ }
+
+ return $this->connection;
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function getHost()
+ {
+ return $this->host;
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function setHost($host)
+ {
+ $this->host = $host;
+
+ return $this;
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function getPort()
+ {
+ return $this->port;
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function setPort($port)
+ {
+ $this->port = $port;
+
+ return $this;
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function getDn()
+ {
+ return $this->dn;
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function setDn($dn)
+ {
+ $this->dn = $dn;
+
+ return $this;
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function getUsernameSuffix()
+ {
+ return $this->usernameSuffix;
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function setUsernameSuffix($usernameSuffix)
+ {
+ $this->usernameSuffix = $usernameSuffix;
+
+ return $this;
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function getVersion()
+ {
+ return $this->version;
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function setVersion($version)
+ {
+ $this->version = $version;
+
+ return $this;
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function getUseSsl()
+ {
+ return $this->useSsl;
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function setUseSsl($useSsl)
+ {
+ $this->useSsl = (boolean) $useSsl;
+
+ return $this;
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function getUseStartTls()
+ {
+ return $this->useStartTls;
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function setUseStartTls($useStartTls)
+ {
+ $this->useStartTls = (boolean) $useStartTls;
+
+ return $this;
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function getOptReferrals()
+ {
+ return $this->optReferrals;
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function setOptReferrals($optReferrals)
+ {
+ $this->optReferrals = (boolean) $optReferrals;
+
+ return $this;
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function getUsername()
+ {
+ return $this->username;
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function setUsername($username)
+ {
+ $this->username = $username;
+
+ return $this;
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function getPassword()
+ {
+ return $this->password;
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function setPassword($password)
+ {
+ $this->password = $password;
+
+ return $this;
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function getUsernameWithSuffix($username = null)
+ {
+ if (null === $username) {
+ $username = $this->username;
+ }
+
+ return $username.$this->usernameSuffix;
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function findByUsername($username, $query, $filter = '*')
+ {
+ if (!$this->connection) {
+ $this->connect();
+ }
+
+ if (!is_array($filter)) {
+ $filter = array($filter);
+ }
+
+ $username = $this->escapeValue($this->getUsernameWithSuffix());
+
+ $query = sprintf($query, $username);
+ $search = ldap_search($this->connection, $this->getDn(), $query, $filter);
+ $infos = ldap_get_entries($this->connection, $search);
+
+ if (0 === $infos['count']) {
+ return null;
+ }
+
+ return $infos[0];
+ }
+
+ private function connect()
+ {
+ if (!$this->connection) {
+ $host = $this->getHost();
+ if ($this->getUseSsl()) {
+ $host = 'ldaps://' . $host;
+ }
+ ldap_set_option($this->connection, LDAP_OPT_PROTOCOL_VERSION, $this->getVersion());
+ ldap_set_option($this->connection, LDAP_OPT_REFERRALS, $this->getOptReferrals());
+
+ if ($this->getUseStartTls()) {
+ ldap_start_tls($this->connection);
+ }
+
+ $this->connection = ldap_connect($host, $this->getPort());

You should do the ldap_connect() first, then do the ldap_set_option() and ldap_start_tls().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ }
+
+ return $this;
+ }
+
+ private function disconnect()
+ {
+ if ($this->connection && is_resource($this->connection)) {
+ ldap_unbind($this->connection);
+ }
+
+ $this->connection = null;
+
+ return $this;
+ }
+
+ /**
+ * Escapes the given VALUES according to RFC 2254 so that they can be safely used in LDAP filters.
+ *
+ * Any control characters with an ASCII code < 32 as well as the characters with special meaning in
+ * LDAP filters "*", "(", ")", and "\" (the backslash) are converted into the representation of a
+ * backslash followed by two hex digits representing the hexadecimal value of the character.
+
+ * @see Net_LDAP2_Util::escape_filter_value() from Benedikt Hallinger <beni@php.net>
+ * @link http://pear.php.net/package/Net_LDAP2
+ * @author Benedikt Hallinger <beni@php.net>
+ *
+ * @param string|array $values Array of values to escape
+ * @return array Array $values, but escaped
+ */
+ private function escapeValue($value)
+ {
+ // Escaping of filter meta characters
+ $value = str_replace(array('\\', '*', '(', ')'), array('\5c', '\2a', '\28', '\29'), $value);
+ // ASCII < 32 escaping
+ for ($i = 0; $i < strlen($value); $i++) {
+ $char = substr($value, $i, 1);
+ if (ord($char) < 32) {
+ $hex = dechex(ord($char));
+ if (strlen($hex) == 1)
+ $hex = '0' . $hex;
+ $value = str_replace($char, '\\' . $hex, $value);
+ }
+ }
+ if (null === $value) {
+ $value = '\0'; // apply escaped "null" if string is empty
+ }
+
+ return $value;
+ }
+}
View
185 src/Symfony/Component/Security/Ldap/LdapInterface.php
@@ -0,0 +1,185 @@
+<?php
+
+namespace Symfony\Component\Security\Ldap;
+
+use Symfony\Component\Security\Ldap\Exception\ConnectionException;
+
+interface LdapInterface
+{
+ /**
+ * return a connection
+ *
+ * @return ressource A connection
+ */
+ public function getConnection();
+
+ /**
+ * return a connection binded to the ldap
+ *
+ * @return ressource A connection
+ *
+ * @throws ConnectionException If username / password are not bindable
+ */
+ public function bind();
+
+ /**
+ * getHost
+ *
+ * @return string The host
+ */
+ public function getHost();
+
+ /**
+ * setHost
+ *
+ * @param string $host The host
+ */
+ public function setHost($host);
+
+ /**
+ * getPort
+ *
+ * @return integer The port
+ */
+ public function getPort();
+
+ /**
+ * setPort
+ *
+ * @param integer $port The port
+ */
+ public function setPort($port);
+
+ /**
+ * getDn
+ *
+ * @return string The dn
+ */
+ public function getDn();
+
+ /**
+ * setDn
+ *
+ * @param string $dn The dn
+ */
+ public function setDn($dn);
+
+ /**
+ * getUsernameSuffix
+ *
+ * @return string The username suffix
+ */
+ public function getUsernameSuffix();
+
+ /**
+ * setUsernameSuffix
+ *
+ * @param string $usernameSuffix The username Suffix
+ */
+ public function setUsernameSuffix($usernameSuffix);
+
+ /**
+ * getVersion
+ *
+ * @return integer The version
+ */
+ public function getVersion();
+
+ /**
+ * setVersion
+ *
+ * @param integer $version The version
+ */
+ public function setVersion($version);
+
+ /**
+ * getUseSsl
+ *
+ * @return boolean The use SSL
+ */
+ public function getUseSsl();
+
+ /**
+ * setUseSsl
+ *
+ * @param boolean $useSsl The use SSL
+ */
+ public function setUseSsl($useSsl);
+
+ /**
+ * getUseStartTls
+ *
+ * @return boolean The use start ssl
+ */
+ public function getUseStartTls();
+
+ /**
+ * setUseStartTls
+ *
+ * @param boolean $useStartTls The use start ssl
+ */
+ public function setUseStartTls($useStartTls);
+
+ /**
+ * getOptReferrals
+ *
+ * @return boolean The opt referrals
+ */
+ public function getOptReferrals();
+
+ /**
+ * setOptReferrals
+ *
+ * @param boolean $optReferrals the opt referrals
+ */
+ public function setOptReferrals($optReferrals);
+
+ /**
+ * getUsername
+ *
+ * @return string The username
+ */
+ public function getUsername();
+
+ /**
+ * setUsername
+ *
+ * @param string $username The username
+ */
+ public function setUsername($username);
+
+ /**
+ * getPassword
+ *
+ * @return string The password
+ */
+ public function getPassword();
+
+ /**
+ * setPassword
+ *
+ * @param string $password The password
+ */
+ public function setPassword($password);
+
+ /**
+ * getUsernameWithSuffix return the concatenation
+ * between the username and the usernameSufix
+ *
+ * @param string|null $username A username
+ * @return string The username with the suffix
+ */
+ public function getUsernameWithSuffix($username = null);
+
+ /*
+ * find a username into ldap connection
+ *
+ * @todo : not needed anymore. But could be usefull to retrieve roles
+ *
+ * @param string $username
+ * @param string $query
+ * @param string $filter
+ * @return array|null
+ */
+ public function findByUsername($username, $query, $filter);
+}
View
82 src/Symfony/Component/Security/Tests/Core/Authentication/Provider/LdapAuthenticationProviderTest.php
@@ -0,0 +1,82 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\Security\Tests\Core\Authentication\Provider;
+
+use Symfony\Component\Security\Core\Authentication\Provider\LdapAuthenticationProvider;
+use Symfony\Component\Security\Core\Authentication\Token\UsernamePasswordToken;
+use Symfony\Component\Security\Core\Exception\UsernameNotFoundException;
+use Symfony\Component\Security\Core\User\User;
+
+class LdapAuthenticationProviderTest extends \PHPUnit_Framework_TestCase
+{
+ /**
+ * @expectedException Symfony\Component\Security\Core\Exception\BadCredentialsException
+ * @expectedExceptionMessage The presented password cannot be empty.
+ */
+ public function testCheckAuthenticationWithoutCredentials()
+ {
+ $provider = $this->getProvider(false);
+ $reflection = new \ReflectionMethod($provider, 'checkAuthentication');
+ $reflection->setAccessible(true);
+
+ $reflection->invoke($provider, new User('foo', null), new UsernamePasswordToken('foo', '', 'key'));
+ }
+
+ /**
+ * @expectedException Symfony\Component\Security\Core\Exception\UsernameNotFoundException
+ */
+ public function testRetrieveUserWithBadCredentials()
+ {
+ $provider = $this->getProvider(true, false);
+ $reflection = new \ReflectionMethod($provider, 'retrieveUser');
+ $reflection->setAccessible(true);
+
+ $reflection->invoke($provider, 'foo', new UsernamePasswordToken('foo', 'bar', 'key'));
+ }
+
+ public function testRetrieveUserOk()
+ {
+ $provider = $this->getProvider(true, true);
+ $reflection = new \ReflectionMethod($provider, 'retrieveUser');
+ $reflection->setAccessible(true);
+
+ $reflection->invoke($provider, 'foo', new UsernamePasswordToken('foo', 'bar', 'key'));
+ }
+
+ public function getProvider($isLoadMethodCalled = false, $validUser = true)
+ {
+ $userProvider = $this
+ ->getMockBuilder('Symfony\\Component\\Security\\Core\\User\\LdapUserProvider')
+ ->disableOriginalConstructor()
+ ->getMock()
+ ;
+ if ($isLoadMethodCalled) {
+ if ($validUser) {
+ $userProvider
+ ->expects($this->once())
+ ->method('loadUserByUsernameAndPassword')
+ ->will($this->returnValue(new User('foo', 'bar')))
+ ;
+ } else {
+ $userProvider
+ ->expects($this->once())
+ ->method('loadUserByUsernameAndPassword')
+ ->will($this->throwException(new UsernameNotFoundException('baz')))
+ ;
+ }
+ }
+
+ $userChecker = $this->getMock('Symfony\\Component\\Security\\Core\\User\\UserCheckerInterface');
+
+ return new LdapAuthenticationProvider($userProvider, $userChecker, 'key');
+ }
+}
View
69 src/Symfony/Component/Security/Tests/Core/User/LdapUserProviderTest.php
@@ -0,0 +1,69 @@
+<?php
+
+namespace Symfony\Component\Security\Tests\Core\User;
+
+use Symfony\Component\Security\Core\User\LdapUserProvider;
+use Symfony\Component\Security\Core\User\User;
+use Symfony\Component\Security\Ldap\Exception\ConnectionException;
+
+class LdapUserProviderTest extends \PHPUnit_Framework_TestCase
+{
+ /**
+ * @expectedException BadMethodCallException
+ */
+ public function testLoadUserByUsername()
+ {
+ $ldap = $this->getMock('Symfony\Component\Security\Ldap\LdapInterface');
+
+ $provider = new LdapUserProvider($ldap);
+ $user = $provider->loadUserByUsername('foo');
+ }
+
+ public function testLoadUserByUsernameAndPasswordOk()
+ {
+ $ldap = $this->getMock('Symfony\Component\Security\Ldap\LdapInterface');
+ $ldap
+ ->expects($this->once())
+ ->method('setUsername')
+ ;
+ $ldap
+ ->expects($this->once())
+ ->method('setPassword')
+ ;
+ $ldap
+ ->expects($this->once())
+ ->method('bind')
+ ;
+
+ $provider = new LdapUserProvider($ldap);
+ $user = $provider->loadUserByUsernameAndPassword('foo', 'bar');
+
+ $this->assertInstanceOf('Symfony\Component\Security\Core\User\User', $user);
+ $this->assertEquals('foo', $user->getUsername());
+ $this->assertEquals(null, $user->getPassword());
+ }
+
+ /**
+ * @expectedException \Symfony\Component\Security\Core\Exception\UsernameNotFoundException
+ */
+ public function testLoadUserByUsernameAndPasswordNOk()
+ {
+ $ldap = $this->getMock('Symfony\Component\Security\Ldap\LdapInterface');
+ $ldap
+ ->expects($this->once())
+ ->method('setUsername')
+ ;
+ $ldap
+ ->expects($this->once())
+ ->method('setPassword')
+ ;
+ $ldap
+ ->expects($this->once())
+ ->method('bind')
+ ->will($this->throwException(new ConnectionException('baz')))
+ ;
+
+ $provider = new LdapUserProvider($ldap);
+ $provider->loadUserByUsernameAndPassword('foo', 'bar');
+ }
+}
View
1  src/Symfony/Component/Security/composer.json
@@ -36,6 +36,7 @@
"symfony/routing": "2.2.*",
"doctrine/dbal": "to use the built-in ACL implementation"
},
+ "minimum-stability": "dev",
"autoload": {
"psr-0": { "Symfony\\Component\\Security\\": "" }
},
Something went wrong with that request. Please try again.