Is this login class safe?

[Zephni]

I have created this login class (In all honesty this is the most commented and well structured class I have ever written, I usualy just use random functions in a functions.php file, which works but this felt good when I finished it )

 

I just wanted some advice to how safe and whether the way I have done this is 'good practise', or if there is anything I should add to future proof it.

 

This is the class:

<?php
/* 
	Author: Craig Dennis
	File: user_session.class.php
	Purpose: Flexible user login class that handles logging in, checking a user is logged in, and logging out.

	NOTE TO USE THIS CLASS YOU MUST ALREADY HAVE ALREADY CONNECTED TO THE DATABASE

	Include this file at the top of each page you wish to protect
	 include("inc/user_session.class.php"); //(This could be put at the top of a global include file)

	Use the following code to check the user is logged in:
	 $user_session = new user_session;		//(This could be put at the top of a global include file)
	 $user_session->validate_user();		//(This should only be left on the pages you wish to check for user validation)

	You will want to use the public redirect_if_logged_in() function instead of validate_user() on the login page like this:
	 $user_session->redirect_if_logged_in(); //(This will redirect a user from the current page to the specified landing page)
*/
class user_session{
	// Change these variables below if the table and fields in your database do not match
	public $t_name = "admins";
	public $t_user = "username";
	public $t_pass = "password";
	public $t_lastlogin = "last_login"; //set $t_lastlogin = NULL if you do not have this field in your database

	//Change $login_page and $landing_page if your page names are different to this one
	public $login_page = "login.php";
	public $landing_page = "logged_in.php";

	//Change $log_in_error_msg if you wish to change the general error message when the user is unable to log in
	public $log_in_error_msg = "The username or password you have entered is incorrect or does not exist";

	//Do not touch anything below unless you know what your doing

	/*
	 * logged_in_user()
	 * Returns value of the current logged in username
	 */
	public function logged_in_user(){
		return $_SESSION['user_username'];
	}

	/*
	 * log_in()
	 * Takes 2 parameters ($username, $password)
	 * Attempts to log in with the provided credentials, on success, the username and password are saved in the session for future testing
	 */
	public function log_in($username, $password){
		$username = stripslashes(mysql_real_escape_string($username));
		$password = stripslashes(mysql_real_escape_string($password));

		$query_login = mysql_query("SELECT * FROM ".$this->t_name."
								 		WHERE ".$this->t_user."='$username' AND ".$this->t_pass."='$password'");;

		$login_accepted = mysql_num_rows($query_login);

		if($login_accepted == 1){
			if($t_lastlogin != NULL){
				$query_update_last_login = mysql_query("UPDATE ".$this->t_name." SET ".$this->t_lastlogin."='".time()."'
															WHERE ".$this->t_user."='$username'");	
			}
			$_SESSION['user_username'] = $username;
			$_SESSION['user_password'] = $password;
			return true;
		}else{
			return false;	
		}
	}

	/*
	 * check_user()
	 * Returns true if the current session credentials can be found in the database, otherwise returns false
	 */
	public function check_user(){
		$query_login = mysql_query("SELECT * FROM ".$this->t_name."
								 		WHERE ".$this->t_user."='".$_SESSION['user_username']."' 
										AND ".$this->t_pass."='".$_SESSION['user_password']."'");

		$login_accepted = mysql_num_rows($query_login);

		if($login_accepted == 1){
			return true;
		}else{
			return false;	
		}
	}

	/*
	 * validate_user()
	 * Returns true if the current session credentials can be found in the database, otherwise logs user out and returns false
	 */
	public function validate_user(){
		$login_accepted = $this->check_user();

		if($login_accepted == 1){
			return true;
		}else{
			$this->log_out();
			return false;	
		}
	}

	/*
	 * redirect_if_logged_in()
	 * Redirects the user to the specified landing page if the user is logged in
	 */
	public function redirect_if_logged_in(){
		if($this->check_user()){
			header("Location: ".$this->landing_page);
		}	
	}

	/*
	 * log_out()
	 * Logs the user out by setting the session credentials to an empty string and redirecting them to the specified login page
	 */
	public function log_out(){
		$_SESSION['user_username'] = "";
		$_SESSION['user_password'] = "";
		header("Location: ".$this->login_page);
	}
}
?>

 

Any comments or advice are appreciated.

[trq]

There are lots of issues with such a design. The first is the fact that you define a bunch of properties up front, but expect a user to edit them before they can use your class. Classes should be designed in such a way that you don't need to edit them before you can use them.

 

You can do this by providing methods to set these properties.

 

The other big issue I have is your classes reliance on the mysql extension. Your class should be using a database abstraction layer of some sort, and this dependency should be (again) set via some *setter* method, or even via the construct. This is called *dependency injection*, Google it.

 

There are a few other issues I have (like having the class be responsible for sending headers) but otherwise, I guess it does the job.

[Zephni]

Thanks for your reply.

 

I see what you mean by not setting a load of properties that have to be edited in the class. If I made functions to set these (now private) properties then wouldn't the user have to use them functions to (in this case) set the table and field names at the top of each page (or in a global file)? Is it better to do it that way, than just change the properties once? I guess the idea like you said is that you shouldn't HAVE to change anything in the class.

 

I will look into the database abstraction layer, because as of yet, I have no idea what that is.

 

And finally, I agree with the class not sending the headers, not sure why I did that...

 

Thanks

[trq]

I see what you mean by not setting a load of properties that have to be edited in the class. If I made functions to set these (now private) properties then wouldn't the user have to use them functions to (in this case) set the table and field names at the top of each page (or in a global file)? Is it better to do it that way, than just change the properties once? I guess the idea like you said is that you shouldn't HAVE to change anything in the class.

 

Well, it needs to be configurable somehow, and hacking the code itself is never advised.

[Zephni]

Ok thanks, I will make those changes.

 

As for the database abstraction layer, tbh I only ever use the usual mysql_ functions. Is that wrong?

[trq]

As for the database abstraction layer, tbh I only ever use the usual mysql_ functions. Is that wrong?

 

No it's not wrong. And it's not wrong to create code that relies on it, especially if its just for your own use. I was merely trying to point out that in order to make your code more flexible and useful there are design practices that can make this easy.

 

A database abstraction layer doesn't necessarily meen you need to implement some full blown DBL (like I'm sure you've been reading about). There are very simple methods you can use to make your class more flexible in regards to what databases it can be used within.

 

In your case, you only really use two database functions. One for executing a query, and one that returns the number of results found. If you define an interface that forces these two methods to exist, you can make your code allot more flexible. eg;

 

<?php

interface DB {
    public function query($sql);
    public function num_rows();
}

 

Now, you can force your class to require an object that implements this interface (note that I also changed the name of your class to something more fitting).

 

<?php

class UserLogin {
    private $db;

    public function __construct(DB $db) {
        $this->db = $db;
    }

    // rest of your code
}

 

Within your class your would now use $this->db->query() instead of mysql_query() and $this->db->num_rows() instead of mysql_num_rows().

 

Now, all you need is a database object that implements the DB interface. In this case it's simple:

 

<?php

class MyDb implements DB {
    public function query($sql) {
        return mysql_query($sql);
    }

    public function num_rows() {
        return mysql_num_rows();
    }
}

 

Then, when you want to use your UserLogin class, just pass it an instance of your MyDb object:

 

<?php

$userLogin = new UserLogin(new MyDb);

 

Now, say someone comes along and want's to use your class with an Sqlite database. All they need to is implement the same DB interface (note that this class needs more work, but that is all sqlite specific stuff and is besides the point. It still only needs to implement the query() and num_rows() methods in order to be compatible with your class).

 

<?php

class SomeOtherDB implements DB {
    private $conn;
    private $result;

    public function __construct($file) {
        $this->conn = sqlite_open($file, 0666);
    }

    public function query($sql) {
        $this->result = sqlite_query($this->conn, $sql);
        return $this->result;
    }

    public function num_rows() {
        return sqlite_num_rows($this->result);
    }
}

 

Now, suddenly your class can use a sqlite database without ever needing to edit it's internals.

 

<?php

$userLogin = new UserLogin(new SomeOtherDB('sqlite.db');

 

This is justa  simple example of the advantages of decent design. It's probably riddled with errors, but hopefully it's pretty easy to understand. This is the power of OOP at it's simplest.

[Zephni]

You've really helped me think outside the box, and realise why OOP rules. I'm going to research into your examples (like 'interface', ive never used that before) and use it! Thanks alot thorpe!

 

Exactly what I needed

[trq]

Yeah, there is a lot more to OOP than simply grouping functions into classes. Code libraries and frameworks need these kinds of infrastructure to be able to work together and be extendable. Without them, things get very messy.

 

As you (hopefully) can see, a few simple tricks and tweaks can often make your code a whole lot more robust. The idea above is just one simple design, but it has already made your class compatible with any type of database, without anyone ever having to hack your classes internal code.

[Zephni]

LAST QUESTION PROMISE!

 

Where would be the 'proper' place to keep the DB interface and MyDb class?

 

Currently looks like this (But this means the "maybe" modified content would be in the same file as the class)

 

<?php
/*
 * DB interface
 * Interface for database methods used with the UserLogin class
 */
interface DB {
	public function query($sql);
	public function num_rows($sql);
}

/*
 * MyDb implements DB
 * Modify the returns here if your database differs from the usual mysql_ functions
 */
class MyDb implements DB {
	public function query($sql) {
		return mysql_query($sql);
	}

	public function num_rows($sql) {
		return mysql_num_rows($sql);
	}
}

/*
 * UserLogin class
 * Main class for logging in users
 */
class UserLogin{
	public $db;

	public function __construct(DB $db){
		$this->db = $db;	
	}
	//rest of code

[trq]

Anywhere really. If your going to start building a small library of classes though, you'll want to stick to a common naming convention and make sure each class is in it's own file preferably somewhere on your include_path.

[Zephni]

<?php
    		 /*
	 * db_table_config
	 * Takes 4 string parameters
	 * Sets the table name and the username, pass and lastlogin field names for the database. t_lastlogin is optional
	 */
	 public function db_table_config($t_name, $t_user, $t_pass, $t_lastlogin=NULL){
		 $this->t_name = $t_name;
		 $this->t_user = $t_user;
		 $this->t_pass = $t_pass;
		 $this->t_lastlogin = $t_lastlogin;
	 }
?>

 

I added this for configuring the table like a good boy, and used the function at the top of my global file .

 

Oh and typicaly I use lowercase with _ as delimiters for naming convention, so I set the class name as 'user_login'

 

Again, thanks for all your help.

[batwimp]

It doesn't look like you are hashing your passwords. You may want to consider this for added security.

 

Also, you need to take a few more steps to log the user out. You're going to want to destroy the session and delete any cookies:

 

http://us2.php.net/manual/en/function.session-destroy.php

 

[Zephni]

Also, you need to take a few more steps to log the user out. You're going to want to destroy the session and delete any cookies:

http://us2.php.net/manual/en/function.session-destroy.php

 

Should I MD5 them? Also, is session destroy good enough in this situation?

 

My latest code is:

 

<?php
/* 
	Author: Craig Dennis
	File: user_login.class.php
	Purpose: Flexible user login class that handles logging in, checking a user is logged in, and logging out.

	NOTE TO USE THIS CLASS YOU MUST ALREADY HAVE ALREADY CONNECTED TO THE DATABASE

	Include this file at the top of each page you wish to protect
	 include("inc/user_login.class.php"); //(This could be put at the top of a global include file)

	Use the following code to check the user is logged in:
	 $user_login = new user_login;		//(This could be put at the top of a global include file)
	 $user_login->validate_user();		//(This should only be left on the pages you wish to check for user validation)

	You will want to use the public redirect_if_logged_in() function instead of validate_user() on the login page like this:
	 $user_login->redirect_if_logged_in(); //(This will redirect a user from the current page to the specified landing page)
*/

/*
 * DB interface
 * Interface for database methods used with the user_login class
 */
interface DB {
	public function query($sql);
	public function num_rows($sql);
}

/*
 * MyDb implements DB
 * Modify the returns here if your database differs from the usual mysql_ functions
 */
class MyDb implements DB {
	public function query($sql) {
		return mysql_query($sql);
	}

	public function num_rows($sql) {
		return mysql_num_rows($sql);
	}
}

/*
 * user_login class
 * Main class for logging in users
 */
class user_login{
	private $db;
	private $auto_redirect = true;

	public function __construct($db){
		$this->db = $db;	
	}

	// The variables below are default you can change these with the db_table_config
	public $t_name = "admins";
	public $t_user = "username";
	public $t_pass = "password";
	public $t_lastlogin = "last_login"; //set $t_lastlogin = NULL if you do not have this field in your database

	// Change $login_page and $logged_in_page if your page names are different to this one
	public $login_page = "login.php";
	public $logged_in_page = "logged_in.php";

	// $log_in_error_msg is returned on login failure, can be changed with set_error_msg($error_msg)
	public $log_in_error_msg = "The username or password you have entered is incorrect or does not exist";

	// Do not touch anything below unless you know what your doing

	/*
	 * logged_in_user()
	 * Returns value of the current logged in username
	 */
	public function logged_in_user(){
		return $_SESSION['user_username'];
	}

	/*
	 * automatic_redirect()
	 * Takes 1 bool parameter
	 * Turn automatic redirect on or off. On by default
	 */
	public function automatic_redirect($bool){
		$this->auto_redirect = $bool;
	}

	 /*
	 * db_table_config
	 * Takes 4 string parameters
	 * Sets the table name and the username, pass and lastlogin field names for the database. t_lastlogin is optional
	 */
	 public function db_table_config($t_name, $t_user, $t_pass, $t_lastlogin=NULL){
		 $this->t_name = $t_name;
		 $this->t_user = $t_user;
		 $this->t_pass = $t_pass;
		 $this->t_lastlogin = $t_lastlogin;
	 }

	 /*
	 * set_pages
	 * Takes 2 string parameters
	 * Sets the login page, and on logged in page. If automatic_redirect(true) is used, the class will set the headers and redirect the user automaticaly
	 */
	 public function set_pages($login_page, $logged_in_page){
		 $this->login_page = $login_page;
		 $this->logged_in_page = $logged_in_page;
	 }

	 /*
	 * set_error_msg
	 * Takes 1 string parameter
	 * Sets the error message on login failure
	 */
	 public function set_error_msg($error_msg){
		 $this->log_in_error_msg = $error_msg;
	 }

	/*
	 * log_in()
	 * Takes 2 parameters ($username, $password)
	 * Attempts to log in with the provided credentials, on success, the username and password are saved in the session for future testing
	 */
	public function log_in($username, $password){
		$username = stripslashes(mysql_real_escape_string($username));
		$password = stripslashes(mysql_real_escape_string($password));

		$query_login = $this->db->query("SELECT * FROM ".$this->t_name."
								 		WHERE ".$this->t_user."='$username' AND ".$this->t_pass."='$password'");

		$login_accepted = $this->db->num_rows($query_login);

		if($login_accepted == 1){
			if($t_lastlogin != NULL){
				$query_update_last_login = $this->db->query("UPDATE ".$this->t_name." SET ".$this->t_lastlogin."='".time()."'
															WHERE ".$this->t_user."='$username'");	
			}
			$_SESSION['user_username'] = $username;
			$_SESSION['user_password'] = $password;
			return true;
		}else{
			return false;	
		}
	}

	/*
	 * check_user()
	 * Returns true if the current session credentials can be found in the database, otherwise returns false
	 */
	public function check_user(){
		$query_login = $this->db->query("SELECT * FROM ".$this->t_name."
								 		WHERE ".$this->t_user."='".$_SESSION['user_username']."' 
										AND ".$this->t_pass."='".$_SESSION['user_password']."'");

		$login_accepted = $this->db->num_rows($query_login);

		if($login_accepted == 1){
			return true;
		}else{
			return false;	
		}
	}

	/*
	 * validate_user()
	 * Returns true if the current session credentials can be found in the database, otherwise logs user out and returns false
	 */
	public function validate_user(){
		$login_accepted = $this->check_user();

		if($login_accepted == 1){
			return true;
		}else{
			$this->log_out();
			return false;	
		}
	}

	/*
	 * redirect_if_logged_in()
	 * Redirects the user to the specified landing page if the user is logged in
	 */
	public function redirect_if_logged_in(){
		if($this->auto_redirect == true){
			if($this->check_user()){
				header("Location: ".$this->logged_in_page);	
			}
		}
	}

	/*
	 * log_out()
	 * Logs the user out by setting the session credentials to an empty string and redirecting them to the specified login page
	 */
	public function log_out(){
		$_SESSION['user_username'] = "";
		$_SESSION['user_password'] = "";
		session_destroy();
		if($this->auto_redirect == true){
			header("Location: ".$this->login_page);
		}
	}
}
?>

 

From a quick Google, this looks like it could do the trick http://php.net/manual/en/function.session-unset.php (session_unset())

[Zephni]

Decided on this

 

<?php
/*
	 * log_out()
	 * Logs the user out by removing session data and redirecting them to the specified login page if $this->auto_redirect = true
	 */
	public function log_out(){
		$_SESSION['user_username'] = "";
		$_SESSION['user_password'] = "";
		$this->destroy_session();
		if($this->auto_redirect == true){
			header("Location: ".$this->login_page);
		}
	}

	/*
	 * destroy_session()
	 * Removes session and session cookie data
	 */
	private function destory_session(){
		session_unset();
		session_destroy();
		session_write_close();
		setcookie(session_name(),'',0,'/');	
	}
?>

[Jessica]

I always use MD5 with salt. It's tastier that way.

https://www.google.com/search?q=salting+passwords

[xyph]

MD5 was never meant to hash passwords. Check out the article in my signature, it talks about a class called PHPass.

 

PHPass uses salting and key stretching, and provides two very simple methods for making sure your passwords are nearly impossible to brute force.

 

<?php

require_once 'phpass.php';

$phpass = new PasswordHash(8,FALSE);

$pass = 'abc123';

// Turn password into it's hashed form
$hash = $phpass->HashPassword($pass);

echo 'The hash is: '.$hash.'<br>';

if( $phpass->CheckPassword($pass, $hash) ) {
echo 'Password matched';
} else {
echo 'Password didnt match';
}

?>

 

Makes storing passwords safely pretty easy

 

[batwimp]

There's some debate on whether MD5 is good for hashing passwords. But either way, it's a lot better than nothing. And yes, use salt. Much tastier!

[xyph]

There's some debate on whether MD5 is good for hashing passwords. But either way, it's a lot better than nothing. And yes, use salt. Much tastier!

 

It's not really a debate. MD5 was never meant for hashing passwords. It's meant to make a fingerprint. MD5 is designed for speed, so it's actually counter-intuitive to password hashing.

 

15 years ago, it was 'good enough.' Now, mid-range graphics cards are calculating billions of MD5 hashes a second. Even newer, bigger hashing algos like SHA512 were designed for speed. If you need to use a fast hash algo, you should use a stretching method like PBKDF2.

 

The great part about key stretching is as processors get faster, you can increase the number of 'stretches' performed, making brute-forcing difficult still.

 

Salting is important, but the point of it is to make two identical passwords produce different hashes. A salt should be random and considered public, not be used to try and 'complicate' a simple password.

[Jessica]

MD5 was never meant to hash passwords. Check out the article in my signature, it talks about a class called PHPass.

 

PHPass uses salting and key stretching, and provides two very simple methods for making sure your passwords are nearly impossible to brute force.

They really should have thought about that name.

[xyph]

Got something against donkeys?

[Jessica]

Got something against donkeys?

In fact, I do.