User authentication system

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

• What am I doing right and what not?

• Is this right according to the MVC model?

Connection.class.php

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

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

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('test@test.nl', 'testPa

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('foo@bar.nl', '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. :)

StackExchange Code Review Q#61864, answer score: 7