Login with custom authentication

Question

I want a login with a custom field to authenticate users into the platform. The point is to check a field 'pw_expires_at' to \DateTime('now'), to log the user.

Here's what I did so far:

In the controller:

$user->setPassword(
    $passwordEncoder->encodePassword(
    $user,    
    $mdp)
);

$user->setPwExpiresAt(new \DateTime("now + 1 minute"));

$entityManager = $this->getDoctrine()->getManager();
$entityManager->persist($user);
$entityManager->flush();

In the Authenticator:

public function checkCredentials($credentials, UserInterface $user)
{
    $valid = false;
    $validDate = $this->checkDate($credentials, $user);
    $validPassword = $this->passwordEncoder->isPasswordValid($user, $credentials['password']);

    if($validDate && $validPassword) {
        $valid = true;
    }

    return $valid;

}

/**
 * @return bool
 */
public function checkDate($credentials, UserInterface $user){

    $now = new \DateTime('now');
    $pwdate = new \DateTime();

    $pwdate = $this->entityManager->getRepository(Users::class)->findOneBy([
        'email' => $credentials['email']
    ]);

    if ($pwdate > $now) {
        return false;
    }
    else {
        return true;
    }
}

I also added the new function checkDate() in the AuthenticatorInterface.php.

The problem is : I can log in at anytime.

your line $pwdate = $this->entityManager ... doesn't make any sense at all. the result object is a User, not a datetime. then apparently comparing a user to a datetime is always false (whatever that means) so your checkdate always returns true. I guess you're missing a ->getPwExpiresAt() call or something. also your logic is wrong, checkdate should be false, when $now > $pwdate. — Jakumi
– Jakumi, Commented Jun 12, 2020 at 9:47

Jenne · Accepted Answer · 2020-06-12 10:02:12Z

1

You are comparing (>) a user object repository->findBy(...) which returns a Users::class with a DateTime object $now = new \DateTime();.

Also the $user object entityManager reponse is most likely the same object returned by your getUsername function (the one you pass as an argument in this function) and thus can be skipped? If it is a DTO that does not contain this expired value then add it back in.
Also you are not using the credentials for anything anymore then so removed it as well.

I would change this to something like:

public function checkDate(UserInterface $user) {
    $now = new \DateTime();

    $pwdate = $user->getPwExpiresAt();

    // we dont need the if/else as this ($pwdate > $now)
    // is an expression and will already return true/false;
    return $pwdate > $now; 
}

Some more suggestions:

You might want to reconsider renaming the function to something more expressive like $this->hasAuthenticationExpired($user) this should give a clear indication of what the function is doing other than "checking a date (for what?!)" without reading through the function.
You can move this function to the user object like public function hasExpired() { return $this->getPwExpiresAt() && new \DateTime() > $this->getPwExpiresAt(); } and just call if (!$user->hasExpired()) { which is actually a preferred way for many people as this can be easily reused and accessed whenever handling the user object anywhere.

edited Jun 12, 2020 at 10:02

answered Jun 12, 2020 at 9:56

Jenne

9035 silver badges14 bronze badges

Sign up to request clarification or add additional context in comments.

1 Comment

auredev Over a year ago

Thanks a lot. Indeed, the logic was wrong. I still have a lot to learn... Thanks also for giving me tips to have a clearer code ! :)

Collectives™ on Stack Overflow

Login with custom authentication

1 Answer 1

1 Comment

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

1 Answer 1

1 Comment

Your Answer

Sign up or log in

Post as a guest

Related