MVC Auth Structure and Dependency Injection with PHP-DI 5 and Slim 3

Question

I'm creating an authentication with the Slim 3 framework and PHP-DI 5 (PHP-DI/PHP-DI).

The project will be the base/starter template for my new projects. I want it to be easy to extend, also to larger projects later on. So I find it important that it's a really good structured project. At the moment, I'm trying to learn all the best practices of structuring projects, so all tips are welcome!

My questions are:

1. Would you structure the User-model, Account-controller and Auth-class differently? Or do I have a good base here?
2. Am I using dependency injection as how I should use it? Is there a better way?
3. Do the container items only get instantiated when it's called or is this a performance hit? Should I implement this differently or is this correct?
4. Would you prefer to place the isActivated() and verifyPassword() methods from the User-model in the Auth-class? Or is it at the right place there?

---

My container definitions

<?php

use Slimproject\Models\User;
use Slimproject\Auth\Auth;

return [
    User::class => function (ContainerInterface $c) {
        return new User;
    },
    Auth::class => function (ContainerInterface $c) {
        return new Auth($c->get(User::class));
    }
];

The router

<?php

use Slimproject\Middleware\GuestMiddleware;
use Slimproject\Middleware\AuthMiddleware;

$app->get('/', ['Slimproject\Controllers\HomeController', 'index'])->setName('home');

$app->group('/account', function() {
    $this->get('/login', ['Slimproject\Controllers\AccountController', 'login'])->setName('account.login');
    $this->post('/login', ['Slimproject\Controllers\AccountController', 'doLogin'])->setName('account.login');
    // etc..
})->add(new GuestMiddleware($container));

$app->group('/myaccount', function() {
    $this->get('/', ['Slimproject\Controllers\MyAccountController', 'index'])->setName('myaccount.index');
    // etc..
})->add(new AuthMiddleware($container));

The Account-controller

<?php

namespace Slimproject\Controllers;

use Slim\Views\Twig;
use Psr\Http\Message\ResponseInterface as Response;
use Psr\Http\Message\ServerRequestInterface as Request;
use Slimproject\Auth\Auth;
use Slimproject\Models\User;

class AccountController
{
    public function doLogin(Request $request, Response $response, Twig $view, Auth $auth, User $user)
    {

        $user = $user->get($request->getParam('username'));

        if (!$user->isActivated() {
            $this->flash->addMessage('error', 'Your account is not yet activated.');
            return $view->render($response, 'account/login.twig');
        }

        if (!$user->verifyPassword($request->getParam('password')) {
            $this->flash->addMessage('error', 'Login incorrect.');
            return $view->render($response, 'account/login.twig');
        }

        $auth->login($user);


        return $view->render($response, 'myaccount/index.twig');
    }
}

The User-model

<?php

namespace Slimproject\Models;

use Illuminate\Database\Eloquent\Model;

class User extends Model
{
    protected $fillable = [
        'name',
        'username',
        'email',
        'password',
        'level',
        'activated'
    ];

    public function get($username) 
    {
        return $this->where('email', $username)->orWhere('username', $username)->first() ?: false;
    }

    public function isActivated()
    {
        return (bool) $this->activated;
    }

    public function verifyPassword($password)
    {
       return password_verify($password, $this->password);
    }
}

The Auth-middleware

<?php

namespace Slimproject\Middleware;

class AuthMiddleware extends Middleware
{
    public function __invoke($request, $response, $next)
    {
        if (!$this->auth->isLoggedIn()) {
            $this->flash->addMessage('error', 'You need to be logged in to visit that page.');
            return $response->withRedirect($this->router->pathFor('account.login'));
        }

        $response = $next($request, $response);
        return $response;
    }
}

The Auth-class

<?php

namespace Slimproject\Auth;

use Slimproject\Models\User;

class Auth
{
    protected $user;

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

    public function user()
    {
        return $this->user->where('id', $_SESSION['user']['id'])->first();
    }

    public function isLoggedin()
    {
        return isset($_SESSION['user']);
    }

    public function hasAccess(array $levels)
    {
        return in_array($_SESSION['user']['level'], $levels);
    }

    public function login(User $user)
    {
        $_SESSION['user'] = [
            'id' => $user->id,
            'username' => $user->username,
            'email' => $user->email,
            'level' => $user->level,
            'loggedin_at' => time(),
        ];
    }

    public function logout()
    {
        unset($_SESSION['user']);
    }
}

Answer 1

public function doLogin(Request $request, Response $response, Twig
$view, Auth $auth, User $user)

Why do you need to have both Auth and User as parameter here if your DI framework has already injected a User into Auth for you? Do you now have two different User objects in play in the controller?

---

if (!$user->isActivated() {

Missing closing parenthesis.

---

You should verify password before activation status. You don't want to leak account verification status data to a user making request with bad password.

---

return $this->where('email', $username)->orWhere('username',
$username)->first() ?: false;

This seems problematic. Are you insuring uniqueness across your email and username fields? Can there be one record with username = [email protected] and another with email = [email protected]? If so, which one are you actually retrieving? Should you provide ORDER BY to give preference to username match over email match? Why is your application not architected to treat these fields uniquely? It just seems like you are building a potential problem into your app here.

---

I don't understand your Auth class, in that it doesn't actually perform user authentication. It just persists authentication status to session. It seems odd to have password verification in User while having a login() method on Auth.

Why do you need anything other than user id and authentication status/timestamp to be stored in session? The other fields should come from user object. The only thing the session should be used for is to recover status of user login between requests, not be authoritative on user information, including authorization privileges.

I would think once the session has recovered the user object (something that is unclear how you are handling from shown code), it should no longer be the authority on user status. Most of your methods should probably be move to User class.

In fact, I would consider just ditching this class altogether in favor of just providing methods for login/logout along with session persistence and object recovery from session there.

---

You are really handling sessions in an insecure manner. At a minimum you should be regenerating session id's across login/logout event boundaries, and when you want to destroy a session, there is more to it than just unsetting the $_SESSION superglobal. You are not calling any of the session_* functions at all!

As it stands this framework is likely ripe for session hijacking and fixation attacks.

Please have a look at PHP's session management basics to get some better context on how you should be handling sessions in a more secure fashion.