0
\$\begingroup\$

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']);
    }
}
\$\endgroup\$
1
\$\begingroup\$
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 = some-email@domain.com and another with email = some-email@domain.com? 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.

\$\endgroup\$
  • \$\begingroup\$ Thanks for your reaction. Your first answer, did'nt thought about that, thanks. In reaction to your second answer, partially. "You don't want to leak account verification status data to a user making request with bad password": Why not? This was actually my intention. Yes, the fields are unique and the username field got priority. But in shown code not implemented. The project is far from finished. n. "It seems odd to have password verification in User while having a login() method on Auth": I can find your opinion there. Going to replace it. \$\endgroup\$ – Ramon Bakker Apr 25 '17 at 21:39
  • \$\begingroup\$ "Why do you need anything other than user id and authentication status/timestamp to be stored in session?": Tbh, i dont know. I'll remove them. "Most of your methods should probably be move to User class.": Which methods do you mean here? "You are really handling sessions in an insecure manner.": I am aware of all security concerns. The code provided was not about security, because i still need to implement that. Just started on this project. The question was only about dependency injection and structuring the project. \$\endgroup\$ – Ramon Bakker Apr 25 '17 at 21:43
  • \$\begingroup\$ @RamonBakker Based on your comments, this question might be flirting with being off topic, as the expectation is that your you are not posting partially implemented code for review. The philosophy is that you are posting your code and context on what your code does and you can get review comments around any aspects of what you have shown - thus my reason for security commentary. :) To your question on what methods should be moved from Auth, as I said, I would honestly consider just getting rid of the class altogether, as it doesn't really perform authentication anyway, that is in User. \$\endgroup\$ – Mike Brant Apr 26 '17 at 1:42

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.