Authentication service

Question

This is the AuthenticationService I made based on Barry Jaspan's design, which is way better than the code of which I asked in a question a couple of months ago.

I would like it mainly reviewed on a couple of aspects:

• Readability (clean code)
• Accurate comments
• Accurate naming (especially for the last class)

AuthenticationService.php

namespace Model\Application;

use Http\HttpSession;
use Model\Infrastructure\Mapper\UserMapper;
use Model\Application\AuthenticationStorage;

/**
 * An authentication service.
 *
 * @author John Doe <john@doe.com>
 */
class AuthenticationService
{
    /**
     * @var HttpSession $httpSession An HttpSession instance.
     */
    private $httpSession;

    /**
     * @var UserMapper $userMapper A UserMapper instance.
     */
    private $userMapper;

    /**
     * @var AuthenticationStorage $authenticationStorage An AuthenticationStorage instance.
     */
    private $authenticationStorage;

    /**
     * Constructs the object.
     *
     * @param HttpSession            $httpSession           An HttpSession instance.
     * @param UserMapper             $userMapper            A UserMapper instance.
     * @param AuthenticationStorage  $authenticationStorage An AuthenticationStorage instance.
     */
    public function __construct(
        HttpSession           $httpSession,
        UserMapper            $userMapper,
        AuthenticationStorage $authenticationStorage
    ) {
        $this->httpSession            = $httpSession;
        $this->userMapper             = $userMapper;
        $this->authenticationStorage  = $authenticationStorage;
    }

    /**
     * Finds the authenticated User entity.
     *
     * @return User|void|null The User entity instance if found.
     */
    public function findAuthenticatedUser()
    {
        if ($identifier = $this->httpSession->findParameter('userIdentifier')) {
            return $this->userMapper->findByIdentifier($identifier);
        }

        if ($user = $this->authenticationStorage->authenticateUser()) {
            $this->httpSession->regenerateIdentifier();

            $this->httpSession->setParameter('userIdentifier', $user->getIdentifier());

            $this->authenticationStorage->updateAuthenticationData();

            return $user;
        }
    }

    /**
     * Authenticates a User entity.
     *
     * @param string  $email    An email address.
     * @param string  $password A password.
     * @param boolean $persist  True for authentication persistence, false if not. (optional)
     *
     * @return boolean True if authenticated, false if not.
     */
    public function authenticateUser($email, $password, $persist = false)
    {
        if ($user = $this->userMapper->findByEmail($email)) {
            if (password_verify($password, $user->getPassword())) {
                $this->httpSession->regenerateIdentifier();

                $this->httpSession->setParameter('userIdentifier', $user->getIdentifier());

                if ($persist) {
                    $this->authenticationStorage->storeAuthenticationData($user);
                }

                return true;
            }
        } else {
            // Prevent timing attacks.
            password_verify('password', '$2y$12$liVAn/nttQIipJI3LagCAOehLX0iamDjIn2WlX38ZEKS/dDHR3e1W');
        }

        return false;
    }

    /**
     * Deauthenticates the authenticated User entity.
     *
     * @return void
     */
    public function deauthenticateUser()
    {
        $this->httpSession->unsetParameter('userIdentifier');

        $this->httpSession->regenerateIdentifier();

        $this->authenticationStorage->deleteAuthenticationData();
    }
}

AuthenticationStorage.php

namespace Model\Application;

use Http\HttpCookies;
use Model\Domain\User\User;
use Library\CryptoRandCharGen;
use Model\Infrastructure\Mapper\UserMapper;
use Model\Domain\Authentication\Value\Token;
use Model\Domain\Authentication\Authentication;
use Model\Infrastructure\Mapper\AuthenticationMapper;
use Model\Domain\Authentication\AuthenticationFactory;

/**
 * Handles the authentication persistence of the authentication service.
 *
 * @author John Doe <john@doe.com>
 */
class AuthenticationStorage
{
    /**
     * @var HttpCookies $httpCookies An HttpCookies instance.
     */
    private $httpCookies;

    /**
     * @var AuthenticationMapper $authenticationMapper An AuthenticationMapper instance.
     */
    private $authenticationMapper;

    /**
     * @var UserMapper $userMapper A UserMapper instance.
     */
    private $userMapper;

    /**
     * @var AuthenticationFactory $authenticationFactory An AuthenticationFactory instance.
     */
    private $authenticationFactory;

    /**
     * Constructs the object.
     *
     * @param HttpCookies           $httpCookies           An HttpCookies instance.
     * @param AuthenticationMapper  $authenticationMapper  An AuthenticationMapper instance.
     * @param UserMapper            $userMapper            A UserMapper instance.
     * @param AuthenticationFactory $authenticationFactory An AuthenticationFactory instance.
     */
    public function __construct(
        HttpCookies           $httpCookies,
        AuthenticationMapper  $authenticationMapper,
        UserMapper            $userMapper,
        AuthenticationFactory $authenticationFactory
    ) {
        $this->httpCookies           = $httpCookies;
        $this->authenticationMapper  = $authenticationMapper;
        $this->userMapper            = $userMapper;
        $this->authenticationFactory = $authenticationFactory;
    }

    /**
     * Authenticates a User entity by the stored authentication data.
     *
     * @return User|boolean The User entity instance if authenticated, false if not.
     */
    public function authenticateUser()
    {
        if (!$cookie = $this->httpCookies->getParameter('auth')) {
            return false;
        }

        if (substr_count($cookie, ':') !== 2) {
            return false;
        }

        list($data['userIdentifier'], $data['seriesNumber'], $data['token']) = explode(':', $cookie);

        if (!$this->authenticationMapper->findAllByUserIdentifier($data['userIdentifier'])) {
            return false;
        }

        if (!($authentication = $this->authenticationMapper->findByIdentifier($data['userIdentifier'], $data['seriesNumber']) and password_verify($data['token'], $authentication->getToken()))) {
            $this->authenticationMapper->deleteAllByUserIdentifier($data['userIdentifier']);

            return false;
        }

        return $this->userMapper->findByIdentifier($authentication->getIdentifier()->getUserIdentifier());
    }

    /**
     * Generates authentication data for a User entity and stores it.
     *
     * @param User $user A User entity instance.
     *
     * @return void
     */
    public function storeAuthenticationData(User $user)
    {
        // Character pool for CryptoRandCharGen::generateString().
        $lowerAlpha = 'abcdefghijklmnopqrstuvwxyz';
        $upperAlpha = strtoupper($lowerAlpha);
        $digits     = '0123456789';
        $symbols    = './<>?;"\'`!@#$%^&*()[]{}_+=|\-';
        $characters = $lowerAlpha . $upperAlpha . $digits . $symbols;

        // Generate the authentication data.
        $seriesNumber       = CryptoRandCharGen::generateInteger(0, 2147483647);
        $token['cleartext'] = CryptoRandCharGen::generateString($characters, 60);
        $token['hash']      = password_hash($token['cleartext'], PASSWORD_BCRYPT);
        $registerDateTime   = date('Y-m-d H:i:s');

        // Store it in the database.
        $authentication = $this->authenticationFactory->build($user->getIdentifier(), $seriesNumber, $token['hash'], $registerDateTime);
        $this->authenticationMapper->insert($authentication);

        // Issue a data with this data equivalent to the client.
        $auth   = $user->getIdentifier() . ':'. $seriesNumber . ':' . $token['cleartext'];
        $expiry = strtotime($registerDateTime . ' + 90 days');
        $this->httpCookies->setParameter('auth', $auth, $expiry, '/');
    }

    /**
     * Regenerates the authentication token for the found User entity and updates it.
     *
     * @return void|null
     */
    public function updateAuthenticationData()
    {
        if (!($cookie = $this->httpCookies->getParameter('auth') and substr_count($cookie, ':') === 2)) {
            return null;
        }

        list($data['userIdentifier'], $data['seriesNumber']) = explode(':', $cookie, -1);

        if (!$authentication = $this->authenticationMapper->findByIdentifier($data['userIdentifier'], $data['seriesNumber'])) {
            return null;
        }

        // Character pool for CryptoRandCharGen::generateString().
        $lowerAlpha = 'abcdefghijklmnopqrstuvwxyz';
        $upperAlpha = strtoupper($lowerAlpha);
        $digits     = '0123456789';
        $symbols    = './<>?;"\'`!@#$%^&*()[]{}_+=|\-';
        $characters = $lowerAlpha . $upperAlpha . $digits . $symbols;

        // Generate a new token.
        $token['cleartext'] = CryptoRandCharGen::generateString($characters, 60);
        $token['hash']      = password_hash($token['cleartext'], PASSWORD_BCRYPT);

        // Update it in the database.
        $authentication->setToken(new Token($token['hash']));
        $this->authenticationMapper->update($authentication);

        // Issue a data with the updated data equivalent to the client.
        $auth   = $authentication->getIdentifier()->getUserIdentifier() . ':' . $authentication->getIdentifier()->getSeriesNumber() . ':' . $token['cleartext'];
        $expiry = strtotime($authentication->getRegisterDateTime() . ' + 90 days');
        $this->httpCookies->setParameter('auth', $auth, $expiry, '/');
    }

    /**
     * Deletes the found User entity its authentication data.
     *
     * @return void
     */
    public function deleteAuthenticationData()
    {
        if ($cookie = $this->httpCookies->getParameter('auth') and substr_count($cookie, ':') === 2) {
            list($data['userIdentifier'], $data['seriesNumber']) = explode(':', $cookie, -1);

            $this->authenticationMapper->deleteByIdentifier($data['userIdentifier'], $data['seriesNumber']);
        }

        $this->httpCookies->unsetParameter('auth');
    }
}

Answer 1

In addition to what @janos said:

authenticateUser

Personally, I find these kind of fall-through nested if statements hard to read. It takes a while to see that false will be returned if the actual password_verify returns false.

If you write it like this, it would be clearer, and you would get rid of one level of nesting:

public function authenticateUser($email, $password, $persist = false)
{
    $user = $this->userMapper->findByEmail($email);
    if (!$user) {
        // Prevent timing attacks.
        password_verify('password', '$2y$12$liVAn/nttQIipJI3LagCAOehLX0iamDjIn2WlX38ZEKS/dDHR3e1W'); // you could also return here and change else-if to if.
    } else if (password_verify($password, $user->getPassword())) {
        // [... session code ...]

        return true;
    }
    return false;
}

I like that you thought about defending against email harvesting timing attacks (of course, if you have a sign-up page they can be harvested there, but it's still good).

Comments

@return User|void|null The User entity instance if found.

I would be interested in when this returns void, and when null.

True for authentication persistence, false if not. (optional)

I would also mention what the default option is ((optional; false by default)

// Issue a data with this data equivalent to the client.

This doesn't sound quite right. Issue a Cookie?

Naming

• you could use id instead of identifier. It is well understood and shorter, thus might be easier to read (findByIdentifier -> findById, $identifier -> $id, etc).
• getPassword of user doesn't really express what it does, as it returns a hash. getPasswordHash might be a better fit.
• $data is very generic, $cookieData might be better.
• it might be a bit confusing to have authenticateUser in two classes for different activities. I don't have good alternatives, but maybe authenticateUserByCookie? But in that case it doesn't fit in with the rest of the methods. You could change the whole naming schema of AuthenticationStorage to fix this (this class mainly deals with managing cookies, but the naming doesn't reflect this).

Misc

• some of your lines are too long. I would aim for 80 characters per line, you could aim for more, but 195 characters don't fit on a lot of monitors.
• I would probably use a bit less newlines. In AuthenticationStorage:authenticateUser() before the return false;, and generally in AuthenticationService.
• your importing Authentication in AuthenticationStorage but are not using it.

Answer 2

In storeAuthenticationData, these variables are constants:

// Character pool for CryptoRandCharGen::generateString().
$lowerAlpha = 'abcdefghijklmnopqrstuvwxyz';
$upperAlpha = strtoupper($lowerAlpha);
$digits     = '0123456789';
$symbols    = './<>?;"\'`!@#$%^&*()[]{}_+=|\-';
$characters = $lowerAlpha . $upperAlpha . $digits . $symbols;

Instead of rebuilding $characters every time this method is called, move these outside of the method and do it only once.

This is especially important because you have these duplicated in updateAuthenticationData too.

---

The logic of joining and splitting cookie values by : appear at multiple places. It would be better to encapsulate that logic in a dedicated class, which hides such details as the : separator from the rest of the implementation.

---

A minor thing, but in a void method, why not simply return; instead of return null; ?