OOP UserAuthenticator Class

Question

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 findByUsernameAndPassword method returns a User object, so I have two Users instances in the same class: one injected and another returned by the Mapper. Should I inject just $username and $password instead of a User object in order to authenticate ?

I have also some wrappers classes like Session and MD5 but they are not needed to understand how my classes works.

Edit

My classes after changes and user related classes:

interface Authenticator { public function authenticate(UserCredentials $userCredentials); } class LoginAuthenticator implements Authenticator { private $userMapper; public function __construct(UserMapper $userMapper) { $this->userMapper = $userMapper; } public function authenticate(UserCredentials $userCredentials) { if (!$user = $this->userMapper->findByUsernameAndPassword($userCredentials->getUsername(), $userCredentials->getPassword())) { throw new InvalidCredentialsException('Invalid username or password!'); } return $user; } } class UserCredentials { private $username; private $password; public function getUsername() { return $this->username; } public function setUsername($username) { if (!is_string($username) || strlen($username) < 3) { throw new InvalidArgumentException('Invalid username.'); } $this->username = $username; } public function getPassword() { return $this->password; } public function setPassword($password, Encryptor $encryptor) { if (!is_string($password) || strlen($password) < 8) { throw new InvalidArgumentException('Invalid password.'); } $this->password = $encryptor->encrypt($password); } } class User { private $id; private $firstName; private $lastName; private $email; private $username; private $password; public function getPassword() { return $this->password; } public function setPassword($password, Encryptor $encryptor) { $this->password = $encryptor->encrypt($password); } //more getters and setters } interface UserMapper { public function insert(User $user); public function update(User $user); public function delete($id); public function findByUsernameAndPassword($username, $password); public function findAll(); } class PdoUserMapper implements UserMapper { private $pdo; private $table = 'users'; public function __construct(PDO $pdo) { $this->pdo = $pdo; } public function insert(User $user) { $statement = $this->pdo->prepare("INSERT INTO {$this->table} VALUES(null, ?, ?, ?, ?)"); $userValues = array( $user->getFirstName(), $user->getLastName(), $user->getEmail(), $user->getUsername(), $user->getPassword() ); $statement->execute($userValues); return $this->pdo->lastInsertId(); } public function update(User $user) { $statement = $this->pdo->prepare("UPDATE {$this->table} SET name = ?, last_name = ?, email = ?, password = ? WHERE id = ?"); $userValues = array( $user->getFirstName(), $user->getLastName(), $user->getEmail(), $user->getPassword(), $user->getId() ); $statement->execute($userValues); } public function delete($id) { $statement = $this->pdo->prepare("DELETE FROM {$this->table} WHERE id = ?"); $statement->bindValue(1, $id); $statement->execute(); } public function findById($id) { $statement = $this->pdo->prepare("SELECT * FROM {$this->table} WHERE id = ?"); $statement->bindValue(1, $id); if (!$result = $statement->execute()) { return null; } $user = new User(); $user->setId($result['id']); $user->setFirstName($result['name']); $user->setLastName($result['last_name']); $user->setUsername($result['username']); $user->setEmail($result['email']); return $user; } public function findByUsernameAndPassword($username, $password) { $statement = $this->pdo->prepare("SELECT * FROM {$this->table} WHERE username = ? AND password = ?"); $statement->bindValue(1, $username); $statement->bindValue(2, $password); $statement->execute(); if (!$result = $statement->fetch()) { return null; } $user = new User(); $user->setId($result['id']); $user->setFirstName($result['name']); $user->setLastName($result['last_name']); $user->setEmail($result['email']); $user->setUsername($result['username']); $user->setPassword($result['password'], new MD5()); return $user; } public function findAll() { $statement = $this->pdo->query("SELECT * FROM {$this->table}"); while ($result = $statement->fetch(PDO::FETCH_ASSOC)) { $user = new User(); $user->setId($result['id']); $user->setFirstName($result['name']); $user->setLastName($result['last_name']); $user->setUsername($result['username']); $user->setEmail($result['email']); $userCollection[] = $user; } return $userCollection; } } try { $userCredentials = new UserCredentials(); $userCredentials->setUsername($_POST['username']); $userCredentials->setPassword($_POST['password'], new MD5()); $pdo = new PDO('mysql:host=localhost;dbname=database', 'root', ''); $loginAuth = new LoginAuthenticator(new PdoUserMapper($pdo)); $user = $loginAuth->authenticate($userCredentials); $session = new Session(); $session->setValue('user', $user); header('Location: index.php'); } catch (InvalidArgumentException $e) { echo $e->getMessage(); } catch (PDOException $e) { echo $e->getMessage(); } catch (InvalidCredentialsException $e) { echo $e->getMessage(); } 

One thing that is bothering me is the need to pass an Encryptor two times: to the User::setPassword method and to UserCredentials::setPassword method. If I need to change my encryption algorithm, I'll have to change it in more than one place, what leads me to think that I'm still making something wrong.

Answer 1

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 responsibilities clean: The mapper doesn't need to know how to query a database (actually in most cases its completely database agnostic), the repository knows how to query a particular data source (e.g. database), and the authenticator looks for the provided user without knowing where it actually does come from.

In my opinion it is particularly important to name the things right. If I read Mapper I know exactly what to expect. Mapper is a 'reserved' terms in regards to patterns:

In software engineering, a design pattern is a general reusable solution to a commonly occurring problem

If your code doesn't follow this I do become really confused. I have to do a lot of read-up, and stuff. Your code becomes a surprise, which generally is really bad in terms of maintainability.

So If your mapper is actually a repository, please please name it as so. But then keep in mind that a repository is suggesting behavior too.

On the should the authenticator store to session thing: The authenticator directly: no. Maybe you want to re-use it without storing users to a session directly (e.g. API-Calls, ...). Commonly it is the job of a Service to provide concrete login / logout functionality. This usually is a two-step process: authenticate the user at a authenticator, and on success, store the result in a session. This keeps your authenticators reusable for situations where you don't want to start a session.