Page 1 of 1

Authentication class

Posted: Sun Nov 22, 2009 9:14 am
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;
    }   
}   
 

Re: Authentication class

Posted: Wed Nov 25, 2009 10:42 am
by iankent
Anyone?

Or is my code so unbelievably amazing and perpetually secure that any comments would be defeated by its incredibleness? 8O

Re: Authentication class

Posted: Wed Nov 25, 2009 1:42 pm
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.

Re: Authentication class

Posted: Tue Dec 01, 2009 7:25 am
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?

Re: Authentication class

Posted: Thu Jan 28, 2010 5:00 pm
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.

Re: Authentication class

Posted: Fri Jan 29, 2010 12:56 am
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).

Re: Authentication class

Posted: Mon Feb 01, 2010 11:58 pm
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.

Re: Authentication class

Posted: Thu Feb 04, 2010 9:14 am
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.

Re: Authentication class

Posted: Thu Feb 04, 2010 10:58 am
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.

Re: Authentication class

Posted: Fri Mar 26, 2010 11:26 pm
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.