Authentication Class (Second OOP attempt)

Coding Critique is the place to post source code for peer review by other members of DevNetwork. Any kind of code can be posted. Code posted does not have to be limited to PHP. All members are invited to contribute constructive criticism with the goal of improving the code. Posted code should include some background information about it and what areas you specifically would like help with.

Popular code excerpts may be moved to "Code Snippets" by the moderators.

Moderator: General Moderators

Post Reply
User avatar
stakes
Forum Commoner
Posts: 48
Joined: Tue Jun 12, 2007 12:05 pm

Authentication Class (Second OOP attempt)

Post by stakes »

Hello php developers!

A few months ago i threw myself into the world of OOP. I've done a few classes since, but this was my first attempt for something that I'm actually going to use on one (and hopefully my future) projects. I'm aware that my OOP design-theory isn't the best. I would like feedback on possible improvements on methods, possible security flaws, and performance issues (are there any obvious resource hogs?).

Code: Select all

 
<?php
 
/** class Auth **/
 
# Description   = Authentication module for users. 
# Dependancy    = class.MySQL
# Last modified = 23 Januari
 
require_once('lib/class.MySQL.php');
 
class Auth {
    
    
    private $userName;
    private $pwd;
    private $user_ip;
    
    /* the db object */
    
    private $db;
    
    /* the user id if authenticated */
    
    private $user_id;
    
    /* timestamp to check inactivity */
    
    private $time_stamp;
    
    /* User session id */
    
    private $session_id;
    
    
    /**
     * Constructor
     */
    
    function __construct($userName=false, $pwd=false) {
            
        /* Intitialize db class */
        
        $this->db = new DB(DB_HOST, DB_USER, DB_PASS, DB_NAME); 
        
        /* escape username, password is hashed no need for escaping */
        
        $this->userName = $this->db->escape($userName);
        $this->pwd = $pwd;
 
        /* Get usersc current IP (needs to be escaped?) */
        
        $this->user_ip = $this->db->escape($_SERVER['REMOTE_ADDR']);
        $this->time_stamp = time();
        $this->session_id = $this->db->escape(session_id());            
        $this->user_id = $_SESSION['user_id'];
 
    }
    
    /**
     * method: authenticate
     *
     * This method is called when a user tries to login
     * Prints error on fail
     * Logs user in on success
     */
    
    public function authenticate(){
        
    /* To prevent login flood */    
        
    if(!isset($_SESSION['loginTries'])) {
    $_SESSION['loginTries'] = 0;
    }
    else
    {
    $_SESSION['loginTries']++;
    }
    
    if($_SESSION['loginTries'] > 10) {
    $_SESSION['error']['tries'] = "För många försök. Spärrat.";
    }
 
    /* check that username is entered */
    
    elseif(empty($this->userName))
    {
    $_SESSION['error']['userName'] = "Fyll i användarnamn.";
    }
    
    /* check that password is entered */
 
    if(empty($this->pwd))
    {       
    $_SESSION['error']['pwd'] = "Fyll i lösenord.";
    }
    
    /* If no error, proceed to check user input against database */
    
    else {
        
    /* Hash the password */
        
    $this->pwd = bin2hex(mhash(MHASH_SHA256, $this->pwd));
 
    /* Look for db matches. */
    
    $result = $this->db->query("SELECT user_id, activation FROM user_auth WHERE user='$this->userName' AND pass='$this->pwd'");
    
    /* If there is no match */
    
    if($result->length() == 0)
    {
    $_SESSION['error']['incorrect'] = "Felaktig inloggning";
    }   
    
    /* if there is a match reset login tries. Check if the activation field in the user db is empty, if not it means that the uer
    has not activated his/hers account */
    
        else
        {   
            
        unset($_SESSION['loginTries']);
        
        $activation = $result->activation;
        $this->user_id = $result->user_id;
        
            if(!empty($activation))
            {
                
                $_SESSION['error']['notActivated']  = 'Konto ej aktiverat';
            }
            else 
            {
                
            /* If everything is alright, proceed with login */  
                
            $this->doLogin();
            }
        }
    }       
    }
    
    private function doLogin() {
        
        /* Cookie for the input field on login page to remember username */
        
        setcookie("lastUser", $this->userName, time() + (364 * 24 * 60 * 60),"/");
        
        /* Make sure the user can't use multiple clients. */
 
        $this->db->query("DELETE from user_session WHERE user_id='$this->user_id'");
        
        /* Create user session in db */
        
        $this->db->query("INSERT INTO user_session (session_id, user_id, user_ip, time_stamp) VALUES 
        ('$this->session_id','$this->user_id','$this->user_ip','$this->time_stamp')");
        
        /* Initialize session variables */
        
        $_SESSION['user_id'] = $this->user_id;
        $_SESSION['userName'] = $this->userName;
        
        /*redirect to users profile page (website.com/user) */
        
        header("Location: /$this->userName");
        exit;
    }
    
    /**
     * method: ReAuthenticate
     *
     * Runs on every page where user authentication is required.
     * Returns true or false depending if user is logged in or not
     */
    
    public function reAuthenticate(){
 
    /* Proceed only if session user_id is set */
        
    if(isset($_SESSION['user_id']))
    
    {
    /* Timeout value for inactive users (5) minutes */
    
        $timeout = $this->time_stamp - 300;
        $this->db->query("DELETE from user_session WHERE time_stamp < '$timeout'");
        
        /* Check if user is really logged in, and that user ip and session id has not changed */
        
        $result = $this->db->query("SELECT user_id FROM user_session WHERE session_id='$this->session_id' AND user_ip='$this->user_ip' AND user_id='$this->user_id'");
            if($result->length() == 0) 
            {
                $this->logout();
                return FALSE;
            }
            
            else
            {   
                
                /* Update user time stamp because user is active */
                
                $this->db->query("UPDATE user_session SET time_stamp = '$this->time_stamp' WHERE user_id='$this->user_id'");
                return TRUE;
                
            }   
        }
        
        else
        {
        return FALSE;
        }   
 
    }
    
    public function logout() {
        
        /* Remove user session from DB And delete session variables*/
    
        $this->db->query("DELETE from user_session WHERE user_id='$this->user_id'");
        session_unset();
        
        header('Location: /');
        exit;
        
        
    }
    
}
    
?>
 
Any comments are welcome. Looking forward for some bashing :)

thanks

/Daniel
User avatar
Christopher
Site Administrator
Posts: 13596
Joined: Wed Aug 25, 2004 7:54 pm
Location: New York, NY, US

Re: Authentication Class (Second OOP attempt)

Post by Christopher »

Why are you using both the session and database to track whether the user is logged-in? You may not need the user_session table.
(#10850)
User avatar
Kieran Huggins
DevNet Master
Posts: 3635
Joined: Wed Dec 06, 2006 4:14 pm
Location: Toronto, Canada
Contact:

Re: Authentication Class (Second OOP attempt)

Post by Kieran Huggins »

You may be waiting a while for that bashing... it looks pretty good to me :)
User avatar
stakes
Forum Commoner
Posts: 48
Joined: Tue Jun 12, 2007 12:05 pm

Re: Authentication Class (Second OOP attempt)

Post by stakes »

arborint wrote:Why are you using both the session and database to track whether the user is logged-in? You may not need the user_session table.
Hmm.. I guess that is a pretty valid point :banghead: The more i think about it, is there any reason to be using the DB at all? Is it just a waste of server resource? The thing is I'm considering adding more fields to the user_session table to keep track on user behaviour, and maybe it will be messy to keep all the information in sessions? I'm clueless :P
User avatar
Christopher
Site Administrator
Posts: 13596
Joined: Wed Aug 25, 2004 7:54 pm
Location: New York, NY, US

Re: Authentication Class (Second OOP attempt)

Post by Christopher »

stakes wrote:The thing is I'm considering adding more fields to the user_session table to keep track on user behaviour,
That sounds like separate functionalty. I would create a separate class for that.
stakes wrote:and maybe it will be messy to keep all the information in sessions? I'm clueless :P
You might want to create a User class that can hold all the user data you need and have methods like isSignedIn(), etc. The store the object in the session.
(#10850)
User avatar
stakes
Forum Commoner
Posts: 48
Joined: Tue Jun 12, 2007 12:05 pm

Re: Authentication Class (Second OOP attempt)

Post by stakes »

Haha you've already totally convinced me to ditch the user_session table. I'll be back with some new code. Now that i think of it, it's probably gonna relieve the the database alot, which could be useful for performance. Anyone beg to differ?
User avatar
Kieran Huggins
DevNet Master
Posts: 3635
Joined: Wed Dec 06, 2006 4:14 pm
Location: Toronto, Canada
Contact:

Re: Authentication Class (Second OOP attempt)

Post by Kieran Huggins »

makes sense, the only thing that storing the users in the DB has going for it is if you're running multiple servers in a random round robin. Even then, you should just use DB session storage, so arborint++

The code looks fine though, even if the overall approach is questionable.
User avatar
mpeacock
Forum Newbie
Posts: 10
Joined: Thu Apr 12, 2007 9:07 am
Location: Mobile AL

Re: Authentication Class (Second OOP attempt)

Post by mpeacock »

This looks pretty good to me - the only thing I would add would be to delete cookie during logout. While you're killing the user's session by whacking the record in the user_session table, you're leaving a crumb in the browser.

While this probably isn't a huge issue - but I though I'd mention it. You're careful to avoid login floods, but you don't do the same thing on logout. Since you're hitting the DB on each logout call, that's a potential DOS vulnerability - especially if your user_session table typically has lots of records.

Maybe a cleaner solution would be to just unset_session and kill the cookie - and then have a trigger or cron script that cleans out old/inactive user session records from the table. That way - logouts don't tie up a DB connection resource.

As far as storing sessions in a db goes - it can certainly come in handy if you need to do some quick and dirty Single Sign On across servers or web applications. For example - if you have a PHP app and a Java app that need to share the same login, then dropping the session into the DB can save you some time and headaches. Other than that - I don't know that storing sessions on the back end buys you much more than you get out of the box with in-memory sessions in PHP.

Either way - I think arborint is right when he suggests putting session management into a different class. The responsibility of this one is to simply let the user identify himself/herself to the system and to cancel out of it. More than that, and you're gilding the lily.

Cheers, Michael
User avatar
stakes
Forum Commoner
Posts: 48
Joined: Tue Jun 12, 2007 12:05 pm

Re: Authentication Class (Second OOP attempt)

Post by stakes »

mpeacock wrote:This looks pretty good to me - the only thing I would add would be to delete cookie during logout. While you're killing the user's session by whacking the record in the user_session table, you're leaving a crumb in the browser.

While this probably isn't a huge issue - but I though I'd mention it. You're careful to avoid login floods, but you don't do the same thing on logout. Since you're hitting the DB on each logout call, that's a potential DOS vulnerability - especially if your user_session table typically has lots of records.

Maybe a cleaner solution would be to just unset_session and kill the cookie - and then have a trigger or cron script that cleans out old/inactive user session records from the table. That way - logouts don't tie up a DB connection resource.

As far as storing sessions in a db goes - it can certainly come in handy if you need to do some quick and dirty Single Sign On across servers or web applications. For example - if you have a PHP app and a Java app that need to share the same login, then dropping the session into the DB can save you some time and headaches. Other than that - I don't know that storing sessions on the back end buys you much more than you get out of the box with in-memory sessions in PHP.

Either way - I think arborint is right when he suggests putting session management into a different class. The responsibility of this one is to simply let the user identify himself/herself to the system and to cancel out of it. More than that, and you're gilding the lily.

Cheers, Michael
Thanks for your input Micheal.

The "cookie crumb" that I'm leaving behind is meant to be. This feature was a request from
one of my clients, saving the hassle for returning users (supposedly on the same computer\browser) to enter their username on the login form. I'm aware that this in a way compromises security, but not to a very critical point? Please tell me if you believe differently.

Discussions on other forums recently brought another question to my mind. I'm using sha256 for password hashing here. Say my user passwords were somehow leaked, how long does a cracker need to perform a collision attack and break a weak password like 6 alphabetical characters. Would adding a salt to password hashing be redundant or useful?

cheerio.
Post Reply