Attempt at a simple secure login class

[nathanblogs]

Hi,

 

I am trying to make a simple secure login class which can login, logout and check if a user already logged in. It doesn't need to be able to remember a user after they close the browser as I want the user to login every time they visit the site.

 

I am using propel to interact with my mysql database where I have created a user table like so:

 

CREATE TABLE `user` (
                 `user_id` int(11) NOT NULL auto_increment,             
                 `username` varchar(30) NOT NULL,                       
                 `password` char(40) default NULL,                      
                 `session` char(32) default NULL,                       
                 `ip` varchar(15) default NULL,                         
                 PRIMARY KEY  (`user_id`)                               
               ) ENGINE=MyISAM AUTO_INCREMENT=2 DEFAULT CHARSET=latin1  

 

And this is my current login class:

 

class LoginClass {
    private $userId;
    private $logged_in;

    function __construct() {
       $this->logged_in = false;
       $this->userId = 0;
    }

   public function isLoggedIn() {
       $this->checkSession();
       return $this->logged_in;
   }

   public function login($username, $password) {
        //build query to check username and password.
        $c = Util::buildCriteria(UserPeer::TABLE_NAME);
        $c->add(UserPeer::USERNAME, $username);
        $c->add(UserPeer::PASSWORD, sha1($password));

        $user = PropelWrapper::retrieveRecord($c);
       
        // if the username and password is valid the database should of return an array of that user's data.
        if(is_array($user)){
            $this->setSession($user);
        } else {
            $this->logout();
        }
    }

    private function setSession($user, $new_login = true) {
        // set the session data so we can confirmed they logged in later.
        $_SESSION['uid'] = $user['user_id'];
        $_SESSION['username'] = $user['username'];
        $_SESSION['logged'] = true;

        // if a new login update the database.
        if ($new_login) {
            $session = session_id();
            $ip = $_SERVER['REMOTE_ADDR'];

            $c1 = new Criteria();
            $c1->add(UserPeer::USER_ID, $user['user_id']);

            $c2 = new Criteria();
            $c2->add(UserPeer::SESSION, $session);
            $c2->add(UserPeer::IP, $ip);

            PropelWrapper::updateRecords($c1, $c2);
        }

        $this->logged_in = true;
    }

    private function checkSession() {
        // grab all the variables from the session to check against the database.
        $username = $_SESSION['username'];
        $session = session_id();
        $ip = $_SERVER['REMOTE_ADDR'];
        $user_id = $_SESSION['uid'];

        // query the database.
        $c = Util::buildCriteria(UserPeer::TABLE_NAME);
        $c->add(UserPeer::USERNAME, $username);
        $c->add(UserPeer::USER_ID, $user_id);
        $c->add(UserPeer::SESSION, $session);
        $c->add(UserPeer::IP, $ip);

        $user = PropelWrapper::retrieveRecord($c);

        // if the user is logged in the database would of returned an array with that users details.
        if(is_array($user)){
            $this->setSession($user, false);
        } else {
            $this->logout();
        }
    }

    public function logout(){
        $this->logged_in = false;

        // Unset all of the session variables.
        $_SESSION = array();

        // If it's desired to kill the session, also delete the session cookie.
        // Note: This will destroy the session, and not just the session data!
        if (ini_get("session.use_cookies")) {
            $params = session_get_cookie_params();
            setcookie(session_name(), '', time() - 42000,
                $params["path"], $params["domain"],
                $params["secure"], $params["httponly"]
            );
        }

        // Finally, destroy the session.
        session_destroy();
    }
}

 

I am looking for any suggestions to improve this class in any way(mostly worried about security however).

 

Thanks.