User authentication system

Question

I'm new to OO PHP and I'm trying to create a simple properly-designed user authentication system.

1. What am I doing right and what not?
2. Is this right according to the MVC model?

Connection.class.php

<?php
class Connection {
 private $host = 'host';
 private $name = 'name';
 private $user = 'user';
 private $pass = 'pass';
 public $con;
 function __construct() {
 try {
 $this->con = new PDO("mysql:host=$this->host;dbname=$this->name;charset=utf8", $this->user, $this->pass);
 $this->con->setAttribute(PDO::ATTR_ERRMODE,PDO::ERRMODE_EXCEPTION);
 } catch(PDOException $ex) {
 die('Failed trying to connect to the database');
 } 
 } 
}

User.class.php

<?php
class User {
 private $id;
 private $username;
 private $email;
 private $password;
 function __construct($id, $username, $email, $password) {
 $this->id = $id;
 $this->username = $username;
 $this->email = $email;
 $this->password = $password;
 }
 function getId() {
 return $this->id;
 }
 function getUsername() {
 return $this->username;
 }
 function getEmail() {
 return $this->email;
 } 
 function getPassword() {
 return $this->password;
 } 
}

UserService.class.php

<?php
class UserService {
 protected $db;
 public function __construct($db) {
 $this->db = $db;
 }
 public function login($email, $password) {
 $stmt = $this->db->con->prepare('SELECT * FROM user WHERE email = ?');
 $stmt->execute(array($email));
 if ($stmt->rowCount() > 0) {
 //Check password here
 //Return user object and set session on success
 //$user = $stmt->fetchObject('User');
 $user = $stmt->fetch(PDO::FETCH_ASSOC);
 return new User($user['id'], $user['username'], $user['email'], $user['password']);
 }
 return false;
 }
 public function logout() { }
 public function register() { }
 public function getUser($userId) {
 $stmt = $this->db->con->prepare('SELECT * FROM user WHERE id = ?');
 $stmt->execute(array($userId));
 if ($stmt->rowCount() > 0) {
 return $stmt->fetch(PDO::FETCH_ASSOC);
 }
 return false;
 }
}

Example usage

//Include class files
$userService = new UserService($db);
if ($user = $userService->login('[email protected]', 'testPass')) {
 echo 'Logged in as: ' . $user->getEmail();
} else {
 echo 'Invalid login';
}

Answer 1

Connection

• Is just wrapping PDO in a class Connection.
• Contains hard-coded credentials.

This class provides no significant benefit over using the PDO class directly. If somehow you need to change the database credentials, you'll have to change the class' properties directly, which is also a bad thing.

I suggest you get rid of your Connection class and just stick to using the PDO class directly. It already provides everything you need.

---

User

Just an anemic domain object with a bunch of getters and setters. I don't see anything wrong with that.

---

UserService

• Violates the Single Responsibility Principle.

  This class serves to:

  • Authenticate Users.
  • Register Users.
  • Write / read from the database.

I would transform it into 3 application service classes and give them all their own responsibility:

• AuthenticationService:

  This will serve to authenticate Users so it contains the following methods:

  • Find logged in User.
  • Log in.
  • Log out.

• RegistrationService:

  This class will take care of your User domain object registrations and contains the following method:

  • Register User.

• And finally UserMapper:

  This class will contain all the CRUD operations for your domain object User, and it will spit out User domain objects when reading from the database. It will contain the methods:

  • Find user by ID.
  • Find user by Email.

---

Is this right according to the MVC model?

Hmm, not really. You're missing the dedicated Views and Controllers of the M V C pattern.

From what I see, you've got responsibilities of the View mixed with your code on the bootstrap level. The logic of which you are controlling the model classes with is supposed to be in a dedicated Controller. And the logic of what you output to the browser needs to be in it's corresponding View.

Btw, the 3 classes I've discussed above are supposed to reside in the Model layer in your MVC structure because they contain business and application logic ( = model).

Blueprint of a potential controller:

// declare namespace
// import statements
class AuthController
{
 // dependencies
 public function __construct(AuthService $authService, View $view)
 {
 // set the dependencies
 }
 public function index()
 {
 return $this->view->index()
 }
 public function submit()
 {
 if ($user = $this->authService->logInUser('[email protected]', 'password')) {
 return $this->view->success([
 'user' => [
 'email' => $user->getEmail()
 ]
 ]);
 }
 return $this->view->failed();
 }
 // log out action
}

_{Note: In the original MVC pattern the Controller is not supposed to call or instantiate any views, but solely control the Model layer, nothing else. However, over the years this has changed. Nowadays what you'll mostly see and hear is that a Controller's job is to control the model layer and call / return the appropriate View.}

---

Wrapping up

Luckily your code is not in a super bad state. All it needs is some refactoring and restructuring to make it valid according to the MVC pattern and abide by the separation of concerns.

Try to adhere to the coding standards defined by the PHP Framework Interop Group. It gives better readability to your code, especially for other programmers who do it as well. Of course, you do not have to be perfect with it.

I hope my answer is clear enough and of some help to you. :)

• JasonK: 1 First of all, thanks for the effort! I don't exactly understand the first point about the Connection class. Isn't this far more better then creating a new PDO class everytime? I'll have to edit every PDO line if I change my DB credentials. 2: I will transform UserService into three classes right away. 3 This is still hard to understand for me :) I will read this part untill I understand it.
• CodeX: Some excellent points there kid, i was a bit miffed about the V & C not being in the question but the M is ok.
• CodeX: You could move your db credentials to a separate config.php include file inside or outside of your root
• JasonK: @CodeX Should I declare the variables as constants in the config.php?
• CodeX: You can, it's definitely an option, it's also the way I do it

Answer 2

It looks ok to me, just a few points

Use the attribute:

PDO::ATTR_EMULATE_PREPARES => false

You can use this is an array and pass it in:

$this->con = new PDO("mysql:host=$this->host;dbname=$this->name;charset=utf8", $this->user, $this->pass, $options);
 $options = array(
 PDO::ATTR_PERSISTENT => true,
 PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION,
 PDO::ATTR_EMULATE_PREPARES => false
 );

You could also construct something like this:

public function __construct() {
 $dsn = 'mysql:host=' . $this->host . ';dbname=' . $this->dbname;
 // Set options
 $options = array(
 PDO::ATTR_PERSISTENT => true,
 PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION,
 PDO::ATTR_EMULATE_PREPARES => false
 );
 // Create a new PDO instanace
 try{
 $this->dbh = new PDO($dsn, $this->user, $this->pass, $options);
 }
 // Catch any errors
 catch(PDOException $e){
 $this->error = $e->getMessage();
 }
}

Autoload your Classes if possible

spl_autoload_register(function ($class) {
 require_once 'classes/class.'. $class .'.php';
});

Never select * in a login function

I would only check for the password here then run a function if they logged in to return the users credentials

public function login($email, $password) {
 $stmt = $this->db->con->prepare('SELECT id, username, email, password FROM user WHERE email = ?');
 $stmt->execute(array($email));
 if ($stmt->rowCount() > 0) {
 //Check password here
 //Return user object and set session on success
 //$user = $stmt->fetchObject('User');
 $user = $stmt->fetch(PDO::FETCH_ASSOC);
 return new User($user['id'], $user['username'], $user['email'], $user['password']);
 }
 return false;
}

• JasonK: Thanks for the tips, I will certainly use them. Should I replace $this->con->setAttribute(PDO::ATTR_ERRMODE,PDO::ERRMODE_EXCEPTION); with $this->con->setAttribute(PDO::ATTR_EMULATE_PREPARES => false); or should I just add the third attribute? And what does it do?
• CodeX: Oops, yeah sorry keep your other attributes, ill update my answer
• CodeX: "What does it do" - php.net/manual/en/pdo.setattribute.php