Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 7 down vote favorite
2

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.

share|improve this question
1  
What's up with static? It's really unnecessary, this reads like you are writing procedural code with classes. – Yannis Sep 22 '12 at 9:51
I find them easier to work with. Will it have any impact on the code? From what I understand, static methods are much faster. – Bethesda Sep 22 '12 at 9:55
1  
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. – Yannis Sep 22 '12 at 9:59
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. – Jared Paolin Sep 22 '12 at 16:59
My bad, I meant to write hashing. Well, password hashing, not to mention PHP as a whole is relatively new to me, so I'll have to do some serious studying on bcrypt, blowfish and all those other things I hear so very much about :) – Bethesda Sep 22 '12 at 18:05
show 3 more commentsadd comment (requires an account with 50 reputation)

1 Answer

up vote 8 down vote accepted

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

share|improve this answer
Thanks for the comprehensive answer - very much appreciated! Well, I've got some things to look into :) As for the session_start(), I have an initialize file that takes care of that for me. – Bethesda Sep 22 '12 at 11:06
So I should move away from static methods for this class, or is it okay if methods like guest and check are static? – Bethesda Sep 22 '12 at 11:08
@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. – Yannis Sep 22 '12 at 11:08
Ah, good idea - thanks! – Bethesda Sep 22 '12 at 11:10
@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. – Yannis Sep 22 '12 at 11:13
show 1 more commentadd comment (requires an account with 50 reputation)

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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