OOP UserAuthenticator Class

I've been a PHP procedural programmer for several years but I'm trying to learn OOP and I'm getting a bit confused with some patterns and principles. I would appreciate it if you could give me some tips and advice.

interface LoginAuthenticator
{
    public function authenticate(UserMapper $userMapper);
}

class UserAuthenticator implements LoginAuthenticator
{
    private $user;
    private $session;

    public function __construct(User $user, Session $session)
    {
        $this->user = $user;
        $this->session = $session;
    }

    public function authenticate(UserMapper $userMapper)
    {
        if (!$user = $userMapper->findByUsernameAndPassword($this->user->getUsername(), $this->user->getPassword()))
        {
            throw new InvalidCredentialsException('Invalid username or password!');
        }

        $this->logUserIn($user);
    }

    private function logUserIn(User $user)
    {
        $this->session->setValue('user', $user);
    }

    public function logUserOut()
    {
        $this->session->unsetValue('user');
        $this->session->destroy();
    }
}

try
{
    $user = new User();
    $user->setUsername($_POST['username']);
    $user->setPassword($_POST['password'], new MD5());

    $pdo = new PDO('mysql:host=localhost;dbname=database', 'root', '');

    $userAuth = new UserAuthenticator($user, new Session());
    $userAuth->authenticate(new PdoUserMapper($pdo));

    header('Location: index.php');
}
catch (InvalidArgumentException $e)
{
    echo $e->getMessage();
}
catch (PDOException $e)
{
    echo $e->getMessage();
}
catch (InvalidCredentialsException $e)
{
    echo $e->getMessage();
}

Well, here is my first concern, the SRP: I don't really know if I should inject a Mapper into my UserAuthenticator::authenticate method or if I should create a UserFinder class and inject it instead. I don't know if it's a Mapper responsibility to find. What do you think ?

Furthermore, I'm also confused about the $user property: my

You are right about your concerns regarding the mapper. A mappers job is to map, not to find. In this case its the job of a repository. The repository finds an entry in a database, uses the mapper to translate between the database fields and the model, and returns the model. I had some more detailed explanation about this here.

The method findByUsernameAndPassword most likely would be a method of this repository, returning an authenticated user on success.

I find the arguments in your UserAuthenticator a bit weird (not wrong) though. Currently this class reads as follows:

• The UserAuthenticator allows to authenticate a given set of credentials against different mappers.

• The authenticate methods authenticates the UserAutheniticators credentials against the passed mapper.

Simplified, it is this:

$authenicator = new Authenticator($login, $password);
$mapper = new PDOMapper();
$authenticator->authenticate($mapper);

For me the last line really reads like Authenticator authenticates mapper using $login, and $password. Here, Authenticator actually does not resemble an class capably of authenticating users, it is missing a vital part. It represents Credentials which we later authenticate against a $mapper.

Normally I would expect it the other way arround:

• The UserAuthenticator allows to authenticate different credentials against a given mapper.

• The authenticate methods authenticates the passed credentials against the previously set mapper.

Simplified, this reads like this:

$mapper = new PDOMapper();
$authenticator = new Authenticator($mapper);
$authenticator->authenticate($login, $password);

Which reads like, authenticator, authenticate $login and $password (using the $mapper). I feel this reads better and follows a more logical mental image on how we think this works.

An analogy maybe would be a gatekeeper which demands your key card to enter a building. You usually pass the key card to the gatekeeper, not the gatekeeper to the key card. Out mental image here serves a gate keeper (your UserAuthenticator) who receives an key card reader (your mapper) at construction time (when he starts his shift). When someone arrives, he gives his key card (your login and password) to the gate keeper.

Neither is wrong or right and it depends on your application requirements. I know popular frameworks to it otherwise too. There are pros and cons for either approach. I prefer the second approach though - feels more natural in most cases. Haven't seen many use-cases where credentials are tried against different mappers or repositories. Could elaborate here more if this really is your intended case.

Your second concern comes from using a model for two different purposes: representing an invalid, unauthenticated user with pending authentication and later on for representing an existing, authenticated user. At this point I think this is more of a modeling issue: I'd either pass password and login name directly to UserAuthenticator::authenticate or create a new class Credentials for this. Your current approach poses one huge problem: you got two objects representing the same entity. Really really really avoid this at all costs.

As another remark: your UserAuthenticator currently has two responsibilities: authentication and storing in a session. I'd move this to the service layer calling the authenticator. Of course this could be abstracted in a another layer (e.g. authentication storage adapter). This also reliefs the UserAuthenticator from performing logouts and so on.

On last thing: your naming indicates you are logging in other entities then users, e.g. animals. The name UserAuthenticator says "I'm the only class responsible for logging in users". (Or you your 'admins' have to log-in by another adapter). While this might be true of course, I suppose your intention is rather more to provide different log-in facilities for users (and only users). Commonly this would result in a slightly different naming like: interface LoginAdapterInterface, DatabaseLoginAdapter, and so on.

Update:

I have updated further explanation about the two modeling approaches inline.

About using different mappers for different databases: That's the wrong place here. Databases should be hidden behind an database abstraction layer (DBAL) as PDO is for example.

The key problem is that you're using mappers to access the database. Mappers really only map between the database fields and your model. A repository queries the database (or any other source) and uses the mapper to map the result to the model. It completely hides all information how and where data is stored. The remainder of the application should never know how / where the data is stored. They interact on the repository only. This could look like:

/- Mysql Mapper
Authenticator <-- User Repository <--- PDO Mapper
                                    \- Mongo Mapper

This way keeps the responsibilitie

StackExchange Code Review Q#43211, answer score: 3