PHP Authentication Class

Question

I could use some review on this Auth class, as I'm sure it could use many improvements!

I wrote this fairly quickly, so please forgive any overseen bugs.

Example usage:

// Login a user
if (Auth::attempt($_POST['username'], $_POST['password']))
{
    echo 'You have successfully logged in.';
}

// Check if the user is a guest
if (Auth::guest())
{
    echo 'Please log in to see this page.';
}

// Logout a user
Auth::logout();

Source code:

<?php

class Auth {

    /**
     * Whether or not the user is currently logged in
     *
     * @var bool
     */
    public static $user;

    /**
     * Attempts to log a user in with the given credentials
     *
     * @param   string  $username
     * @param   string  $password
     * @return  bool
     */
    public static function attempt($username, $password)
    {
        global $db;

        $stmt = $db->prepare('SELECT * FROM `users` WHERE `username` = :username AND `password` = :password');
        $stmt->execute(array(':username' => $username, ':password' => $password));

        if ($user = $stmt->fetch(PDO::FETCH_ASSOC))
        {
            static::$user = $user;

            static::login($user['id']);

            return true;
        }

        return false;
    }

    /**
     * Login the user with the given id
     *
     * @param   int     $user_id
     * @return  void
     */
    public static function login($user_id)
    {
        if ($user_id)
        {
            $_SESSION['user_id'] = $user_id;
        }
    }

    /**
     * Log out the current user
     *
     * @return  void
     */
    public static function logout()
    {
        static::$user = null;

        $_SESSION = array();
        unset($_SESSION['user_id']);
        session_destroy();
    }

    /**
     * Checks if a user is currently logged in
     *
     * @return bool
     */
    public static function check()
    {
        return isset($_SESSION['user_id']);
    }

    /**
     * Checks if the current user is a guest
     *
     * @return  bool
     */
    public static function guest()
    {
        return ! isset($_SESSION['user_id']);
    }

}

I'm also not sure if using all static methods in the class could lead to problems later on?

I know there is currently no password hashing - I'm still working on the hashing method.

Answer 1

What's up with static?

Your class is build like a utility class:

UtilityClasses are classes that have only static methods that perform some operation on the objects passed as parameters. Such classes typically have no state.

but it's not really one as it has state, you have a $user property you need to pass around, and your functions depend on it. You are writing procedural code using classes, and there really isn't any reason for that, if procedural programming feels more natural with you, go with it, object orientation is not the one true paradigm. PHP supports both paradigms equally, and they are both equally valid.

The only benefit of using static like this is namespacing your functions, but that's not a real benefit as PHP supports namespaces. You should either write your class as an object oriented class, or a set of namespaced utility functions, everything in between is just bad form.

Since you mentioned execution speed, yes calling a static function is faster than instantiating an object and then calling the function, but the difference is in the range of a fraction of a millisecond. A more convincing argument for using static would be memory use, not execution speed, as the instantiated object will take up some memory. Still the argument is moot as there's absolutely no reason to write procedural code with classes, other than perhaps a misguided notion that everything needs to be object oriented.

Public properties

public static $user;

Assuming you rewrite this to be an object oriented class you should avoid public properties, or you'd be breaking encapsulation. All your class properties should be private and accessible through getters and setters, you should even avoid protected if you are not going to extend the class.

Global

global $db;

Don't. Please forget the global keyword even exists, and pass your dependencies through method parameters:

public static function attempt(PDO $db, $username, $password) {
    ...
}

Note the added benefit of type hinting. If you need that PDO object anywhere else in your class, then you should really transform this into an object oriented class and feed the dependency through its constructor:

class Auth {

    private $pdo; 
    
    public function __construct(PDO $pdo) {
        $this->setPDO($pdo);
    }
    
    public function setPDO(PDO $pdo) {
        $this->pdo = $pdo;
        
        return $this;
    }
    
    public function getPDO() {
        return $this->pdo;
    }

    ... 

}

Auth::setPDO() and Auth::getPDO() are mutator methods, keeping the class in line with encapsulation and any half decent IDE can generate them automatically for you. Notice that I return $this; in the setter? That's because I like chaining my methods, up to you if you want to follow my style.

Session

$_SESSION is also a global, and suffers from everything global variables do, but given that it's a native (almost) always on global, we can live with it. Obviously your class would be useless if at some point you decide to store session information in a different storage, but let's worry about than when and if it happens. However you need to keep in mind that $_SESSION is an external dependency, and your class needs to compensate for the rare occasion that you forgot to call session_start(). For example this:

public static function login($user_id)
{
    if ($user_id)
    {
        $_SESSION['user_id'] = $user_id;
    }
}

will fail. Either call session_start() before your class definition or in the constructor. In either case, do check if $_SESSION already exists before calling it.

Further reading

• Why is Global State so Evil?
• SOLID - Don't cheat, read all the linked articles.
• Dependency inversion principle
• Dependency injection
• Inversion of Control Containers and the Dependency Injection pattern
• When to Use Static Classes in C# - Not really C# specific, at least not the top voted answer.