Login Code

[Sharif]

Here is my attempt of create a login page that has the ability to remember user info without setting cookies... please tell me what I messed up in or what needs to be fixed or anything at all.

<?php
session_start();
header("Cache-control: private"); 
include('required/config.php');
include('required/language.php');

//Build required variables
$username = $_POST['username'];
$password = $_POST['password'];
$MD5 = md5($password);

//Check and see if a session exists
if($_SESSION['loggedin'] == true && $_SESSION['userid'])
{
	//Attempt to login user
	$sql = "SELECT * FROM users WHERE userid = '" . $_SESSION['userid'] . "'";
	$query = mysql_query($sql) or die("Error: ". mysql_error());
	$row = mysql_fetch_array($query);

//If database finds a match
if (mysql_num_rows($query) > 0)
{
	$status = $row['status'];

	//If user hasn't been activated, turn 'em away!
	if($status == 1)
	{
		echo $login['not_activated'];
		exit();
	}

	//If they have been activated, welcome them with open webpages!
	else
	{
		//Create variables with data
		$type = $row['type'];
		$first_name = $row['first_name'];

		//Create session variables
		$_SESSION['loggedin'] = true;
		$_SESSION['type'] = $type;
		$_SESSION['first_name'] = $first_name;
		header('Location: ' . $full_domain . 'index.php');
		exit();
	}
}
}
elseif ($_POST['submit']=='Login')
{
	//If the username or password isn't set...
	if(!$username || !$password)
	{
		echo $loginMISSING;
		include('required/login_form.php');
	}
	else 
	{		
		//Attempt to login...
		$sql = "SELECT username, password FROM users WHERE username = '$username' AND password = '$MD5'";
		$query = mysql_query($sql) or die("Error: ". mysql_error());
		$row = mysql_fetch_array($query);

	//If database finds a match...
	if (mysql_num_rows($query) > 0)
	{
		$status = $row['status'];

		//If user hasn't been activated, turn 'em away!
		if($status == 1)
		{
			echo $login['not_activated'];
			exit();
		}

		//If they have been activated, welcome them with open webpages!
		else
		{
			//Create variables with data
			$userid = $row['userid'];
			$type = $row['type'];
			$first_name = $row['first_name'];

			//If they want the website to remember their login information, set session data to last for one week
			if($remember)
			{
				ini_set('session.cookie_lifetime', 604800); 
				ini_set('session.gc_maxlifetime', 604800);  
				$_SESSION['loggedin'] = true;
			}

			//Create session variables
			$_SESSION['userid'] = $userid;
			$_SESSION['type'] = $type;
			$_SESSION['first_name'] = $first_name;

			//Send the user to the index.php page
			header('Location: ' . $full_domain . 'index.php');
			exit();
		}
	}
	//If the database couldn't find a match...
	else
	{
		echo $login['denied'];
		include('required/login_form.php');
		exit();
	}
}
}
//Check and see if the user is ALREADY logged in
elseif(isset($_SESSION['userid']) && isset($_SESSION['type']) && isset($_SESSION['first_name'])) 
{
	header('Location: index.php');
}
else 
{
	echo $login['first'];
	include('required/login_form.php');
}
?>

[tbach2]

$sql = "SELECT username, password FROM users WHERE username = '$username' AND password = '$MD5'";

Can't trust $username to be a useable value. It could even be evil.

following comment moved to new thread
perhaps somebody knows of a better way to check for alpha-numeric content, but here's my idea:

function assign($variable,$type,$restrictions) {
	$temp='';
	switch($type) {
		case 'get':		$temp = $_GET[$variable]; break;
		case 'post':		$temp = $_POST[$variable]; break;
		case 'request':	$temp = $_REQUEST[$variable]; break;
		case 'cookie':		$temp = $_COOKIE[$variable]; break;
	}
	switch($restrictions) {
		case 'alpha':		preg_match("/([a-zA-Z ,\.]+)/",$temp,$match); break;
		case 'alphanum':	preg_match("/([a-zA-Z0-9 ,\.]+)/",$temp,$match); break;
		case 'num':		preg_match("/([0-9]+)/",$temp,$match); break;
		case 'email':		preg_match("/^(([a-zA-Z0-9_-]*\.*)*[a-zA-Z0-9_-]+@[a-zA-Z0-9_-]+(\.[a-zA-Z0-9_-]+)+)/",$temp,$match); break;
		case 'blob':		$match[1] = $temp; break;
	}
	if($temp!='') {
		global $$variable;
		$$variable = $match[1];
		return true;
	}
	return false;
}


// cleanse variables

assign('username','post','alphanum');

// now $username contains only alphanumeric data

[Sharif]

Never thought about the idea of $username being evil BUT is the code good? You know... the one I posted

[tbach2]

My point was simply that you need to be sure that $username doesn't allow your data to be shown to everybody. Putting the variable in single quotes may help, but some ingenious person may come up with a way to get into your database.

Example: (quoting drawmack)

What happens if the person enters ';SHOW TABLES; for their username?

Other than that, your code looks good and clean.

I'm used to phplib's login, and they have a feature you might like to add--
allow the code to be called from every protected page.

If they are logged in, do nothing and fall through to the page.
If they are not logged in, include the login page and die.
The idea being that you don't have to go to the index page every time.

--just an idea...

[Jeb_]

Originally posted by Sharif
Never thought about the idea of $username being evil BUT is the code good? You know... the one I posted

The code is well structured; good job. I think the design flow needs a bit of work though. There's probably a couple of unnecessary comments in there (but that's a matter of opinion), and I can see two places where you declare an unnecessary variable ($status).

A good rule of thumb is this: don't declare a temporary variable if you're only going to use it once.

There are a few exceptions to this, but in your case here, you don't really "need" the extra $status variable.

Another potential problem is your mysql_num_rows() condition. By saying "greater than zero", and not checking your input, you open yourself up to a huge security hole. You might consider changing that to:

if(mysql_num_rows($result)==1)
{
    // etc...

To ensure that you only ever get one valid user returned.

You can also scrub your input so that's it's safe for your query by using [man]mysql_real_escape_string/man.

Another thing I noticed:

In your first if() statement, you're assuming that $_SESSION['userid'] exists. That will throw a notice error if it doesn't exist. Just wrap it in an isset() or an empty().

[Sharif]

Ok so is my plan good? Like using a session variable as a cookie ($_SESSION['loggedin'] = true) ? Also the way I am using ini_set to make the session last longer, will that work?

[Sharif]

Also, should I just use cookies instead?

[Sharif]

Here is an update version of the code:

<?php
session_start();
header("Cache-control: private"); 
include('required/config.php');
include('required/language.php');
include('required/functions.php');

//Build required variables
$username = $_POST['username'];
$password = $_POST['password'];
$MD5 = md5($password);

//Check and see if a session exists
if($_SESSION['loggedin'] == true && isset($_SESSION['userid']))
{
	//Attempt to login user
	$sql = "SELECT * FROM users WHERE userid = '" . $_SESSION['userid'] . "'";
	$query = mysql_query($sql) or die(sql_error());
	$row = mysql_fetch_array($query);

//If database finds a match
if (mysql_num_rows($query) == 1)
{
	//If user hasn't been activated, turn 'em away!
	if($row['status'] == 1)
	{
		echo $login['not_activated'];
		exit();
	}

	//If they have been activated, welcome them with open webpages!
	else
	{
		//Create session variables
		$_SESSION['loggedin'] = true;
		$_SESSION['type'] = $row['type'];
		$_SESSION['first_name'] = $row['first_name'];
		header('Location: ' . $full_domain . 'index.php');
		exit();
	}
}
}
elseif ($_POST['submit'] == 'Login')
{
	//If the username or password isn't set...
	if(!$username || !$password)
	{
		echo $loginMISSING;
		include('required/login_form.php');
	}
	else 
	{		
		//Attempt to login...
		$sql = "SELECT username, password FROM users WHERE username = '$username' AND password = '$MD5'";
		$query = mysql_query($sql) or die(sql_error());
		$row = mysql_fetch_array($query);

	//If database finds a match...
	if (mysql_num_rows($query) == 1)
	{
		//If user hasn't been activated, turn 'em away!
		if($row['status'] == 1)
		{
			echo $login['not_activated'];
			exit();
		}

		//If they have been activated, welcome them with open webpages!
		else
		{				
			//If they want the website to remember their login information, set session data to last for one week
			if($remember)
			{
				ini_set('session.cookie_lifetime', 604800); 
				ini_set('session.gc_maxlifetime', 604800);  
				$_SESSION['loggedin'] = true;
			}

			//Create session variables
			$_SESSION['userid'] = $row['userid'];
			$_SESSION['type'] = $row['type'];
			$_SESSION['first_name'] = $row['first_name'];

			//Send the user to the index.php page
			header('Location: ' . $full_domain . 'index.php');
			exit();
		}
	}
	//If the database couldn't find a match...
	else
	{
		echo $login['denied'];
		include('required/login_form.php');
		exit();
	}
}
}
//Check and see if the user is ALREADY logged in
elseif(isset($_SESSION['userid']) && isset($_SESSION['type']) && isset($_SESSION['first_name'])) 
{
	header('Location: index.php');
}
else 
{
	echo $login['first'];
	include('required/login_form.php');
}
?>