Login Problems

[ShawnK]

I am having problems with my login script, it works but its very ineffeciant! Please tell me what you'd do!

<?php

/************************************************************************/
/* KurtzDownloadDB: Download Management System                          */
/* ===========================================                          */
/* Copyright (c) 2005 by Shawn Kurtz                                    */
/*                                                                      */
/* This program is free software. You can redistribute it and/or modify */
/* it under the terms of the GNU General Public License as published by */
/* the Free Software Foundation.                                        */
/************************************************************************/

if(!defined("KDD")) die("<b>Error:</b> You cannot access this file directly!");

$act = stripslashes($_GET['act']);

if(isset($_SESSION['username']))
{
	errorDie("You are already logged in!");
}

if(!isset($_POST['login']))
{
	include("../$config[0]/themes/$config[1]/loginform.tpl");
	die();
}

if(empty($_POST['username']) || empty($_POST['password']))
{
	$error = "<b>KDD Error:</b> You must fill in all fields!";
	include("../$config[0]/themes/$config[1]/loginform.tpl");
}

$uName = stripslashes(strtolower($_POST['username']));
$uPass = md5(stripslashes($_POST['password']));
$userInfo = $mysql->query("SELECT * FROM $db[4]_users WHERE `username` = '$uName'", 0);

if(empty($userInfo) || $uPass !== $userInfo['password'])
{
	$error = "<b>KDD Error:</b> Incorrect Username/Password!";
	include("../$config[0]/themes/$config[1]/loginform.tpl");
}
else
{
	$_SESSION['userid'] = $userInfo['id'];
	$_SESSION['username'] = $userInfo['username'];
	header("Location ../$config[0]/index.php");
}
?>

[ShawnK]

anyone?

[vaaaska]

This is in the wrong forum...this is for improving things that work. It should be in the coding forum.

Plus, it helps to know more information about the problem...like the error messages for instance.

[ShawnK]

There are no problems, and it does work, I just need to optimize it.

[mrhappiness]

and why do you think it's inefficient?

there are only a few (mainly cosmetic) things:

i don't like vars put inside strings

include('../'.$config[0].'/themes/'.$config[1].'/loginform.tpl');

you should do stripslashes only after checking [man]get_magic_quotes_gpc[/man]

why do you have mysql compare the username entered and php the password entered?

$sql = 'SELECT userid
FROM '.$db[4]"._users 
WHERE username = '".$uName."' AND `password` = '".$uPass."'";

(don't know if the backticks are neccessary here but i think so as password() is a mysql-builtin function)

[Weedpacket]

And, um, is that username field indexed?

I can understand doing the MD5 operation out in PHP-land, as MySQL's has been known to generate incorrect hashes. But like mrhappiness suggested, you could still put the hashed password in the query and compare it in the database.

[ShawnK]

ah, thanks that helps! But is there a way to be specific about the errors. Like username doesn't exist, password incorrect, username banned, etc.

[mrhappiness]

Originally posted by ShawnK
ah, thanks that helps! But is there a way to be specific about the errors. Like username doesn't exist, password incorrect, username banned, etc.

$sql = "SELECT
  user_name, (user_pass = '".$md5_pass."') pass_correct, user_status
FROM usertable
WHERE user_name = '".mysql_real_escape_string($username)."'
LIMIT 1";
$r = mysql_query($sql);
$errors = array();
if (!mysql_num_rows($r))
  $errors[] = 'user not found';
else {
  $user_data = mysql_fetch_assoc($r);
  if (!$user_data['pass_correct'])
    $errors[] = 'password incorrect';
  switch ($user_data['user_status']) {
    case CONST_BANNED:
      $errors[] = 'banned'; break;
    case CONST_LOCKED:
      $errors[] = 'locked'; break;
    case CONST_NOT_ACTIVATED:
      $errors[] = 'not activated yet'; break;
  }
}
if (count($errors)) {
  echo 'Errors:<pre>';
  print_r($errors);
  echo '</pre>';
}

i suggest not to display whether the username exists or whether the password is incorrect.
if i get the message "password incorrect" i know that the username exists and only need to crack the password.

out of this reason only check the status if username and password match