3
\$\begingroup\$

I am currently developing a login script for my application. The login will use SSL and all required resources will be served through this. It is not protecting anything like a bank however I would like to know what is right and wrong especially for learning purposes

I would love some feedback on my class that I have developed. I have been reading various sources on the net and a lot seems to be contradictory. I would love to know if there is anything else I can do with regard to this code and also the login process as a whole

Areas I feel need improvement:

  • Use something stronger than sha1 for storing passwords.
  • Maintaining login - currently it times out after 20 minutes.

class User extends Model{

private $logLocation; 
private $loginLog;


public function __construct(){
    $this->logLocation = 'system/logs/';
    $this->loginLog = "logins";
}

/**
*
* Add User
* @param    array   $data An array of data that will get added to User table.
*/
public function add($data){
    $db = Database::getInstance();

    $salt =  substr(md5(uniqid(rand(), true)),0,3);

    $query = 'INSERT INTO user(  user_id, user_username, user_password, user_salt, user_forename, user_lastname, user_email, user_attempts)
        VALUES( :user_id, :user_username, sha1(:user_password), :user_salt, :user_forename, :user_lastname, :user_email, 0)';
    $args = array(
        ':user_id' => $data['user_id'],
        ':user_username' => $data['user_username'],
        ':user_password' => $data['user_password'].$salt,
        ':user_salt' => $salt,
        ':user_forename' => $data['user_forename'],
        ':user_lastname' => $data['user_lastname'],
        ':user_email' => $data['user_email']);
    $db->query($query, $args);

    SessionRegistry::instance()->addFeedback('user Saved Successfully');
    return true;
}

public function getUserId($username){
    $db = Database::getInstance();
    //Check to see if the username exists
    $query = "SELECT user_id FROM user WHERE user_username = :username LIMIT 1";
    $results = $db->query($query, array(':username' => $username));
    return $results[0]['user_id'];
}

public function getUsername($userId){
    $db = Database::getInstance();
    //Check to see if the username exists
    $query = "SELECT user_username FROM user WHERE user_username = :username LIMIT 1";
    $results = $db->query($query, array(':username' => $username));
    return $results[0]['user_username'];
}

/**
 * 
 * Checks login details against that in the database
 * @param string $username
 * @param string $password
 */
public function checkLogin($username, $password){
    $db = Database::getInstance();
    //Check to see if the username exists
    $query = "SELECT user_salt, user_password, user_attempts FROM user WHERE user_username = :username LIMIT 1";
    $results = $db->query($query, array(':username' => $username));

    //No results return false
    if(count($results) < 1){
        $this->logLoginAttempt($username, 'Incorrect Username');
        return false;
    }

    //Check to see if the user is blocked
    if((int)$results[0]['user_attempts'] >= 3){
        $this->logLoginAttempt($username, 'Blocked User Login');
        return false;
    }

    //Check to see if the passwords match 
    if(sha1($password.$results[0]['user_salt']) == $results[0]['user_password']){
        $this->setLogin($username);
        return true;
    }
    else{
        //Incorrect Password
        $this->logLoginAttempt($username,  'Incorrect Password');
        $this->failedLoginIncrement($username);
        return false;
    }
}

/**
 * 
 * Increments the failed login attempt for a user. 
 * 3 Strikes and they get locked out.
 * @param string $username
 */
private function failedLoginIncrement($username){
    $db = Database::getInstance();          
    //Update the IP address of the user from where they last logged in
    $query = 'UPDATE user SET user_attempts = user_attempts + 1 WHERE user_username = :username';
    $db->query($query, array(':username' => $username));

    //Check to see if the user has reached 3 strikes if so block them.
    $query = 'SELECT user_attempts FROM user WHERE user_username = :username LIMIT 1';
    $results = $db->query($query, array(':username' => $username));

    if($results[0]['user_attempts'] >= 3){
        //We need to block the user
        $query = 'UPDATE user SET user_blocked = 1 WHERE user_username = :username';
        $db->query($query, array(':username' => $username));
    }
    return true;
}

/**
 * 
 * Logs a failed login attempt to a log file so these can be monitored
 * @param string $username 
 * @param string $reason
 */
private function logLoginAttempt($username, $reason){
    $fh = fopen($this->logLocation.$this->loginLog, 'a+') or die("can't open file");
    $logLine = date('d/m/Y h:i') . ' Login Attempt: ' . $username . ' Failure Reason: ' . $reason . " IP: " . $_SERVER['REMOTE_ADDR'] . "\n";
    fwrite($fh, $logLine);
    fclose($fh);
    return true;
}

/**
 * 
 * Sets the login data in the session. Also logs IP and resets the failed attempts.
 * @param string $username
 */
private function setLogin($username){           
    $db = Database::getInstance();          
    //Update the IP address of the user from where they last logged in
    $query = 'UPDATE user SET user_ip = :ip, user_attempts = 0 WHERE user_username = :username';
    $db->query($query, array(':username' => $username, ':ip' => $_SERVER['REMOTE_ADDR']));

    ini_set("session.use_only_cookies", TRUE);  //Forces the session to be stored only in cookies and not passed over a URI.
    ini_set("session.use_trans_sid", FALSE);    //Stop leaking session IDs onto the URI before browser can check to see if cookies are enabled.
    ini_set("session.cookie_lifetime", 1200);    //Time out after 20mins

    //Now add the session vars to set the user to logged in.
    session_start();
    session_regenerate_id(true); //Regenerate the session Id deleting old session files.
    $_SESSION['valid'] = 1;
    $_SESSION['userid'] = sha1($this->getUserId($_POST['username'] . "SALTHERE"));
}

/**
 * 
 * Checks to see if a user is currently logged in.
 */
public function loggedIn(){
    if($_SESSION['valid']){
        return true;
    }   
    else{
        return false;
    }       
}   

/**
 * 
 * Logs a current user out by destroying the session
 */
public function logout(){
    // 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 then use this class like so:

require_once('User.php');
$user = new User();
$loggedIn = $user->checkLogin($_POST['username'], $_POST['password']);
if($loggedIn){
  //redirect to member area
}
else{
  //show login screen
}

Then on a page where I need to check if a user is logged in

require_once('User.php');
$user = new User();
if(!$user->loggedIn()){
  //redirect to login page
}
\$\endgroup\$
  • \$\begingroup\$ take care of sql injection: mysql_real_escape_string(form[data]) \$\endgroup\$ – Rahul Jul 6 '11 at 10:02
  • 2
    \$\begingroup\$ Thanks Rahul but I am using the PDO library to escape the data. Sorry I should of mentioned that in the Post as it is not apparent. \$\endgroup\$ – matt1982 Jul 6 '11 at 10:51
  • \$\begingroup\$ Looks like a good effort. A simple thing I'd suggest is check if your select query actually returns anything before trying to access the array: ` $results[0]['user_username'];`. Depending on your error_reporting level you will notice warnings on this. \$\endgroup\$ – Fanis Jul 8 '11 at 12:22
  • \$\begingroup\$ Ahh yes Fanis very good point. Little things like that add more polish to the code. I will go back through and add some of the details. \$\endgroup\$ – matt1982 Jul 11 '11 at 11:01
  • \$\begingroup\$ gist.github.com/1151912 for some nice blowfish crypt function wrappers. Not that the latest sha isn't okay, though. \$\endgroup\$ – Kzqai Nov 13 '11 at 18:20
2
\$\begingroup\$

Like Fanis said in his comment, you might want to put error checking on both your getUserId() and getUsername().

Good code overall (except maybe the singleton for your database object), few minor things like replacing rand() with mt_rand(). If you're worried about the security, you could check out crypt() too.

Although getting user IP's is never 100% reliable you shoulud to look into using something like this function for getting the IP of a user.

\$\endgroup\$
  • \$\begingroup\$ A thanks for the tips Stoosh. I know singleton isn't really a good pattern for the DB however I was more concentrating on the security side of things. \$\endgroup\$ – matt1982 Jul 11 '11 at 11:00
  • \$\begingroup\$ +1 for crypt, in my research blowfish crypt is the way to go for passwords, it's a slow hash that also allows you to store the salted + hashed password in a single field because the salt and the hash become a single entity, so that when you need the salt, the first bit of the hash acts as the salt, and they're never in different places from each-other. \$\endgroup\$ – Kzqai Jul 20 '11 at 17:02

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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