Skip to content
This repository has been archived by the owner on May 17, 2021. It is now read-only.

No chaining provider and no persisted entities - auth workflow locks LDAP account #76

Closed
Saaha opened this issue Jan 5, 2015 · 7 comments
Milestone

Comments

@Saaha
Copy link

Saaha commented Jan 5, 2015

Hello there,

First, sorry of my writing, I'll do my best.
I created an application which uses FR3DLdapBundle for authentication. The initial need it's to NOT persist the user in DB and reload him directly from LDAP database on each request (that's the bundle does). The goal it's to not duplicate the LDAP DB in a web app.
In that way, the documentation told me that you can disable the chaining provider with FOS to avoid persistance, so I did. I just override the LdapManager for return a personnal instance of User when the method createUser is called by the LdapManager.

namespace NameSpace\SecurityBundle\Security\User;
use FR3D\LdapBundle\Model\UserManagerInterface;
use NameSpace\SecurityBundle\Entity\User;

class UserManager implements UserManagerInterface {
    public function createUser() {
        return new User();
    }
}

Next, my issue. In the process of authentication (tell me if I get wrong), the user is searched in LDAP as : $user = $this->userProvider->loadUserByUsername($username); in LDAPAuthenticationProvider. The first time, no session token exists, then the User is created, hydrated and binded to LDAP to determine if ID are corrects.

if (!$this->ldapManager->bind($user, $presentedPassword)) {
     throw new BadCredentialsException('The presented password is invalid.');
}

It's ok for me. But when Symfony redirects, for instance, to home page after successfully authentication, the User is re-searched in LDAP, the token is re-created from UserAuthenticationProvider in method authenticate but while my entity is not persited, the password do not exist any longer and bind method will failed. In result, the account is locked in my LDAP DB, not good ! The method checkAuthtication there, $currentUser->getPassword() is null (I tried $token->getCredentials() as mentionned in other issue).

protected function checkAuthentication(UserInterface $user, UsernamePasswordToken $token)
{
        $currentUser = $token->getUser();
        if ($currentUser instanceof UserInterface) {
            if (!$this->ldapManager->bind($currentUser, $currentUser->getPassword())) {
                throw new BadCredentialsException('The credentials were changed from another session.');
            }
        } else { ... }
}

So what am I missing ?
Is there a way to keep credentials in session to revalid bind when User naviguate ? It seems, the bundle doest not cover this case ?

I tried many (many !) debug with var_dump in Symfony components, bundle to analyse the behavior. I could precise any point if needed (but not the solution : ))

@Maks3w
Copy link
Owner

Maks3w commented Jan 5, 2015

I've to admit that branch of the code is a mistery for me.

I was following the same code of DaoAuthenticationProvider but it's truth getPassword is empty or is a hashed password so the return value won't be valid in any case.

So the question is How we can re authenticate an already logged user?

@Maks3w
Copy link
Owner

Maks3w commented Jan 5, 2015

What is the return value of $token->getCredentials()?

@Saaha
Copy link
Author

Saaha commented Jan 6, 2015

$token->getCredentials() return the password filled by the user while the process of authentication starts (before redirect). I actually tried to replace $currentUser->getPassword() by $token->getCredentials() but no change. I my opinion the problem is deeper in the process.

When the User is authenticated, the condition ($currentUser instanceof UserInterface) is true and the User is rebind from the informations returned by the token (aka getUser() and getCredentials()). The User returned by getCredentials() is free of password (by default, it's empty because it's overrided on hydrate method on LdapManager : the password is set to blank.
I tried to set the password on the instance User in checkAuthentication but no change too. Seems when the token is re-recreated the password is lost...

@Saaha
Copy link
Author

Saaha commented Jan 6, 2015

Here is the solution !

Change $currentUser->getPassword() to $token->getCredentials() in LdapAuthenticationProvider, and turn off the property erase_credentials in security.yml.
The generated token keeps the password after login and the bind method is working correctly. I don't know if it's the really waited behaviour or just a bad way to resolve it.

I notice a strange thing btw, the Zend/Ldap driver is called twice, the first time is when the default connection defined in config.yml with username and password are used. The second one is when the bind method is directly called to authenticate User. In this case, I trace all the workflow to show up how it behaves : an Exception is correctly thrown (because username is filled, password is empty and AllowEmptyPassword is false) but in UserAuthenticationProvider the code below does not throw the Exception to the caller class (AuthenticationProviderManager) :

try {
      $this->userChecker->checkPreAuth($user);
      $this->checkAuthentication($user, $token);
      $this->userChecker->checkPostAuth($user);
} catch (BadCredentialsException $e) {
      if ($this->hideUserNotFoundExceptions) {
            throw new BadCredentialsException('Bad credentials', 0, $e);
      }
      throw $e;
}

I don't know why and when the Exception is catched. This issue messed up my brain !

@Maks3w
Copy link
Owner

Maks3w commented Jan 6, 2015

Do you want create a PR with the fix?

About the driver called twice:
a) The first one is the user provider part. (Symfony expects to provider a user object, so you can provide a user from bd but later authenticate against ldap)
b) The second is the user authentication.

@Saaha
Copy link
Author

Saaha commented Jan 7, 2015

I am not sure if the fix is correct, I gonna make new tests with fresh installation and chaining providers to be sure it does not bring regression.

@Maks3w
Copy link
Owner

Maks3w commented Jan 7, 2015

The change will make a BC Break with objects retrieved from BD. But this have to be fixed as you proposed.

@Maks3w Maks3w added this to the 2.0 milestone Apr 13, 2015
@Maks3w Maks3w closed this as completed in 3b5280f Apr 29, 2015
Maks3w added a commit that referenced this issue Apr 29, 2015
Use token `getCrendentials` instead user `getPassword`. Fix #76
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants