10
\$\begingroup\$

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.

\$\endgroup\$
  • 1
    \$\begingroup\$ What's up with static? It's really unnecessary, this reads like you are writing procedural code with classes. \$\endgroup\$ – yannis Sep 22 '12 at 9:51
  • 1
    \$\begingroup\$ Using a static method is faster than instantiating an object and then calling the method, but it's not noticeably faster. You are hurting the testability and readability of your code for a fraction of a millisecond. \$\endgroup\$ – yannis Sep 22 '12 at 9:59
  • \$\begingroup\$ When it's ready, I would be interested in seeing the final version including proper password hashing (not encryption). Authentication can be tricky, so getting it reviewed is always a good idea. \$\endgroup\$ – Jared Paolin Sep 22 '12 at 16:59
  • \$\begingroup\$ @Bethesda for password hashing, look no further than ITSec.SE. This search might prove useful... \$\endgroup\$ – yannis Sep 24 '12 at 8:33
  • \$\begingroup\$ PHPASS is a good choice (with an unfortunate name ;). You might also find this SO question interesting... The top voted answer was written in 2008, but from quickly reading it it doesn't seem outdated (but you should read it more thoroughly than I did). \$\endgroup\$ – yannis Sep 24 '12 at 11:38
11
\$\begingroup\$

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

\$\endgroup\$
  • \$\begingroup\$ @Bethesda I imagined you did call session_start() somewhere, but your class can't live without it, so it would make sense to write something like if(!isset($_SESSION)) { session_start() } at the top of the file or in the constructor. This way, when you'll need to re-use your class in another project, you won't have to worry about re-writing / copying the init file, your class will just work. \$\endgroup\$ – yannis Sep 22 '12 at 11:08
  • \$\begingroup\$ @Bethesda Either move away from static and go full OO, or go procedural, both are equally valid, mixing the two not so much. You could leave all your functions that don't deal with state ($_SESSION and $user) static, but you wouldn't be gaining anything. Your class works as it is, you can even leave it as it is, most of my answer is concerned with its future re-usability and testability. If you take those out of the equation, it serves its purpose just fine. \$\endgroup\$ – yannis Sep 22 '12 at 11:13
  • \$\begingroup\$ I don't think you stressed the "don't use globals" point enough. They are bad everywhere and should be abolished from the language, but besides that, another +1 for you sir :) One more thing that might be of interest here: Helper functions are still quite beneficial, even in OOP, though many OOP enthusiasts think functions are the plague, this is not true. A helper function file would be of more use, IMO, than any "utility class". \$\endgroup\$ – mseancole Sep 24 '12 at 17:21

Your Answer

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