Authentication class

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
iankent
Forum Contributor
Posts: 333
Joined: Mon Nov 16, 2009 4:23 pm
Location: Wales, United Kingdom

Authentication class

Post by iankent »

I've been working on an authentication class and it now all works, but I'd like to know if there's any improvements I can make to it, and whether you think its secure enough? Have I missed anything I should have done or got anything wrong?

Appreciate any feedback :)

Code: Select all

 
class Auth {
    private $current_user_id = -1;
    private $current_user = null;
    public $cfg = null;
    
    function __construct($auth_config) {
        $this->cfg = $auth_config;
        
        if($this->is_authed()) {
            // retrieve user based on session hash
            $this->session_get_user();  
        }
    }
    function __destruct() {   
        unset($this->current_user);
        unset($this->current_user_id);
        
        unset($this->cfg);
    }
        
    public function get_hash($salt, $data) {
        return hash('sha256',$this->cfg['pepper'].$salt.$data);
    }
    
    public function session_get_user() {
        global $session;
        $session_id = $session->get_session_id();
        $user_id = $this->_get_userid_from_session($session_id);
        if($user_id < 0) {
            $_SESSION['is_authed'] = false; 
            return;
        }
        $this->_login_user($user_id);
    }
    
    private function _get_userid_from_session($session_id) {
        global $db;
        $session_id = $db->sql_escape($session_id);
        $ip = $db->sql_escape($_SERVER['REMOTE_ADDR']);
        $q = "SELECT {$this->cfg['db']['user_id']} FROM {$this->cfg['db']['table']} WHERE {$this->cfg['db']['session_id']}='$session_id' AND {$this->cfg['db']['session_ip']} BETWEEN INET_ATON('$ip')-255 AND INET_ATON('$ip')+255 AND {$this->cfg['db']['session_time']} > DATE_SUB(NOW(), INTERVAL {$this->cfg['db']['session_duration']} SECOND);";
        
        $r = $db->sql_query($q);
        $row = $db->sql_fetchrow($r);
        if(!$row) {
            return -1;
        }
    
        $user_id = $row[$this->cfg['db']['user_id']];
        return $user_id;
    }
    
    public function validate_login($username, $password) {
        global $session, $db;
        
        $userhash = $this->get_hash('',$username);
        $passhash = $this->get_hash($userhash, $password);
        $username = $db->sql_escape($username);
        
        $q = "SELECT {$this->cfg['db']['user_id']} FROM {$this->cfg['db']['table']} WHERE {$this->cfg['db']['username']}='$username' AND {$this->cfg['db']['password']}='$passhash';";
        $r = $db->sql_query($q);
        $row = $db->sql_fetchrow($r);
        if(!$row) {
            return "Invalid username or password";
        }
    
        $user_id = $row[$this->cfg['db']['user_id']];
        $session->regenerate_id();
        $this->_login_user($user_id);
 
        return true;
    }
    
    public function create_reset_token($username) {
        global $db;
        
        $q = "SELECT {$this->cfg['db']['user_id']} FROM {$this->cfg['db']['table']} WHERE {$this->cfg['db']['username']}='$username';";
        $r = $db->sql_query($q);
        $row = $db->sql_fetchrow($r);
        if(!$row) {
            return false;
        }
    
        $user_id = $row[$this->cfg['db']['user_id']];
        $reset_token = $this->get_hash(time(), $username);
        $ip = mysql_real_escape_string($_SERVER['REMOTE_ADDR']);
        $q = "UPDATE {$this->cfg['db']['table']} SET {$this->cfg['db']['reset_ip']}=INET_ATON('$ip'), {$this->cfg['db']['reset_expiry']}=DATE_ADD(NOW(),INTERVAL {$this->cfg['db']['reset_duration']} SECOND), {$this->cfg['db']['reset_token']}='$reset_token' WHERE {$this->cfg['db']['user_id']}=$user_id;";
        $db->sql_query($q);
        if($db->sql_affectedrows()>0) {
            return $reset_token;    
        }
        return false;
    }
    public function validate_reset($username, $token) {
        global $db;
        
        $token = $db->sql_escape($token);
        $username = $db->sql_escape($username);
        $ip = $db->sql_escape($_SERVER['REMOTE_ADDR']);
        
        $q = "SELECT {$this->cfg['db']['user_id']} FROM {$this->cfg['db']['table']} WHERE {$this->cfg['db']['username']}='$username' AND {$this->cfg['db']['reset_token']}='$token' AND {$this->cfg['db']['reset_ip']} BETWEEN INET_ATON('$ip')-255 AND INET_ATON('$ip')+255 AND {$this->cfg['db']['reset_expiry']} > NOW();";
        $r = $db->sql_query($q);
        $row = $db->sql_fetchrow($r);
        if(!$row) {
            return false;
        }
        $user_id = $row[$this->cfg['db']['user_id']];
        return $user_id;
    }
    
    public function set_user_password($user_id, $password) {
        global $db;
        
        $q = "SELECT {$this->cfg['db']['username']} FROM {$this->cfg['db']['table']} WHERE {$this->cfg['db']['user_id']}=$user_id;";
        $r = $db->sql_query($q);
        $c = $db->sql_rowcount($r);
        if($c == 0) {
            return false;   
        }
        $row = $db->sql_fetchrow($r);
        $username = $row[$this->cfg['db']['username']];
        $db->sql_freeresult($r);
                
        $userhash = $this->get_hash('',$username);
        $passhash = $this->get_hash($userhash, $password);
                                                        
        $q = "UPDATE {$this->cfg['db']['table']} SET {$this->cfg['db']['password']}='$passhash' WHERE {$this->cfg['db']['user_id']}=$user_id;";
        $db->sql_query($q);
        if($db->sql_affectedrows() > 0) {
            return true;    
        }
        return false;
    }
    
    public function create_user($username, $password) {
        global $db;
        $userhash = $this->get_hash('',$username);
        $passhash = $this->get_hash($userhash, $password);
        
        $username = $db->sql_escape($username);
        
        $q = "SELECT {$this->cfg['db']['user_id']} FROM {$this->cfg['db']['table']} WHERE {$this->cfg['db']['username']}='$username';";
        $r = $db->sql_query($q);
        $c = $db->sql_rowcount($r);
        if($c > 0) {
            $db->sql_freeresult($r);
            return "Username in use";   
        }
        
        $q = "INSERT INTO {$this->cfg['db']['table']} ({$this->cfg['db']['username']},{$this->cfg['db']['password']}) VALUES ('$username','$passhash');";
        $db->sql_query($q);
        if($db->sql_affectedrows()>0) {
            return $db->sql_insertid();
        } else {
            return "Error creating user";   
        }
    }
    
    public function _login_user($user_id) {
        global $user, $db, $session;
                
        $this->current_user_id = $user_id;
        $this->current_user = new User($this, $user_id);
        
        $user = $this->current_user;
        $this->current_user->session_id = $session->get_session_id();
        $_SESSION['is_authed'] = true;
        
        // Update session IP, ID and time in database:
        $ip = $db->sql_escape($_SERVER['REMOTE_ADDR']);
        $q = "UPDATE {$this->cfg['db']['table']} SET {$this->cfg['db']['session_ip']}=INET_ATON('$ip'), {$this->cfg['db']['session_time']}=NOW(), {$this->cfg['db']['session_id']}='{$this->current_user->session_id}' WHERE {$this->cfg['db']['user_id']}=$user_id;";
        $db->sql_query($q);
    }
    
    public function logout() {
        global $session, $db, $user;
        if($this->is_authed()) {
            $_SESSION['is_authed'] = false; 
            $session->regenerate_id();
            
            // Clear the session details
            $q = "UPDATE {$this->cfg['db']['table']} SET {$this->cfg['db']['session_id']}='', {$this->cfg['db']['session_ip']}=0, {$this->cfg['db']['session_time']}='0000-00-00 00:00:00' WHERE {$this->cfg['db']['user_id']}={$this->current_user_id};";
            $db->sql_query($q);
            
            // Reset the user
            $this->current_user = null;
            $this->current_user_id = -1;
            $user = null;
        }
    }
    
    public function is_authed() {
        return (isset($_SESSION['is_authed'])) ? $_SESSION['is_authed'] : false;         
    }
    
    public function get_user() {
        return $this->current_user;
    }   
}   
 
User avatar
iankent
Forum Contributor
Posts: 333
Joined: Mon Nov 16, 2009 4:23 pm
Location: Wales, United Kingdom

Re: Authentication class

Post by iankent »

Anyone?

Or is my code so unbelievably amazing and perpetually secure that any comments would be defeated by its incredibleness? 8O
User avatar
Christopher
Site Administrator
Posts: 13596
Joined: Wed Aug 25, 2004 7:54 pm
Location: New York, NY, US

Re: Authentication class

Post by Christopher »

That is a bit of code to wade through. Perhaps you could show use cases and describe the details of what it does for create user, login, logout, etc. And what options are available.
(#10850)
User avatar
yacahuma
Forum Regular
Posts: 870
Joined: Sun Jul 01, 2007 7:11 am

Re: Authentication class

Post by yacahuma »

I try never to use globals. Only in the rarest of cases where I have no choice(made bad decisions and dont have time to change code). Why, I like to know what is going in and out of my functions. If the class needs $db, why dont you put it in the constructor?
User avatar
social_experiment
DevNet Master
Posts: 2793
Joined: Sun Feb 15, 2009 11:08 am
Location: .za

Re: Authentication class

Post by social_experiment »

Code: Select all

<?php
$q = "SELECT {$this->cfg['db']['user_id']} FROM {$this->cfg['db']['table']} WHERE {$this->cfg['db']['username']}='$username' AND {$this->cfg['db']['password']}='$passhash';";
?>
What about adding 'LIMIT 1' to the end of the query? I haven't read the complete code but to me it seems logical to return only 1 match to a specfic username and password combination. This will also be dependent on whether the script checks for users attempting to register with similar usernames, email addresses.
“Don’t worry if it doesn’t work right. If everything did, you’d be out of a job.” - Mosher’s Law of Software Engineering
User avatar
Christopher
Site Administrator
Posts: 13596
Joined: Wed Aug 25, 2004 7:54 pm
Location: New York, NY, US

Re: Authentication class

Post by Christopher »

There is a lot of SQL and methods with names like _get_userid_from_session() that only fetch from the database. I think I would split out the database code into a separate class that was an example using your database class (which should be provided).
(#10850)
User avatar
Luke
The Ninja Space Mod
Posts: 6424
Joined: Fri Aug 05, 2005 1:53 pm
Location: Paradise, CA

Re: Authentication class

Post by Luke »

I agree with arborint that you should post some examples of how this class works if you want some real critique. Nobody wants to guess. From what I've seen of the code though, I agree that the globals need to go. Pass those things in if you need them, don't assume they exist out in the global scope somewhere.

Also, I agree with arborint that this is a bit of a god class. You have too many things going on in one class. A good rule of thumb is that if you can't describe in one sentence what a class does, it does too much.
User avatar
JNettles
Forum Contributor
Posts: 228
Joined: Mon Oct 05, 2009 4:09 pm

Re: Authentication class

Post by JNettles »

This is nitpicking but you may also want to check the naming convention on a couple of your classes - the first two functions aren't declared as public (even though they're magic methods but for consistency and clearing up ambiguity you should probably go ahead and declare them public) and I saw a public function that started with an underscore - is that supposed to be a private or public? Other functions that started with an underscore were private.
User avatar
Eran
DevNet Master
Posts: 3549
Joined: Fri Jan 18, 2008 12:36 am
Location: Israel, ME

Re: Authentication class

Post by Eran »

Another thing - unless you have an explicit reason, I'd suggest declaring class properties as proected instead of private. Why would you want to prevent extending this class?
Also, everything in the__destruct() method is redundant. When the object is destroyed all object properties are destroyed with it.
zerkig
Forum Newbie
Posts: 10
Joined: Fri Mar 26, 2010 1:44 am

Re: Authentication class

Post by zerkig »

social_experiment wrote:

Code: Select all

<?php
$q = "SELECT {$this->cfg['db']['user_id']} FROM {$this->cfg['db']['table']} WHERE {$this->cfg['db']['username']}='$username' AND {$this->cfg['db']['password']}='$passhash';";
?>
What about adding 'LIMIT 1' to the end of the query? I haven't read the complete code but to me it seems logical to return only 1 match to a specfic username and password combination. This will also be dependent on whether the script checks for users attempting to register with similar usernames, email addresses.
The dba in me screams in frustration when I see a comment like this. Never use LIMIT 1 in cases like this, it will only mask you getting the primary/unique key wrong or a 'or 1=1' condition. You should be checking for too many rows as it would mean a major design flaw or table corruption. If performance is an issue add an index.
Post Reply