Page 1 of 1

Login with md5

Posted: Sun Jul 23, 2006 6:20 am
by thiscatis
The Error :

Warning: mysql_num_rows(): supplied argument is not a valid MySQL result resource in......

The Script :

Code: Select all

session_start();
if(!isset($_SESSION['logged_in'])){
if($_POST){
$username = $_POST['username'];
$password = $_POST['password'];
$query = "SELECT * FROM $tblUsers WHERE username='$username' AND password = '".md5($_POST['password'])."' ";
$result = mysql_num_rows(mysql_query($query));
if($result==1){
$_SESSION['logged_in'] = TRUE;
}
header("location: " . $_SERVER['PHP_SELF']);
exit();
}else{
echo "<form action=\"".$_SERVER['PHPSELF']."\" method=\"POST\">";
echo "username: <input type=text name=username><BR>";
echo "password: <input type=password name=password><BR>";
echo "<input type=submit name=Login></form>";
exit();
}
}
I can't find error in the script :s

Posted: Sun Jul 23, 2006 6:39 am
by Charles256
is it supposed to be $tblusers or just tblusers?

Re: Login with md5

Posted: Sun Jul 23, 2006 8:04 am
by aerodromoi
thiscatis wrote:The Error :

Warning: mysql_num_rows(): supplied argument is not a valid MySQL result resource in......

The Script :

Code: Select all

session_start();
if(!isset($_SESSION['logged_in'])){
if($_POST){
$username = $_POST['username'];
$password = $_POST['password'];
$query = "SELECT * FROM $tblUsers WHERE username='$username' AND password = '".md5($_POST['password'])."' ";
$result = mysql_num_rows(mysql_query($query));
if($result==1){
$_SESSION['logged_in'] = TRUE;
}
header("location: " . $_SERVER['PHP_SELF']);
exit();
}else{
echo "<form action="".$_SERVER['PHPSELF']."" method="POST">";
echo "username: <input type=text name=username><BR>";
echo "password: <input type=password name=password><BR>";
echo "<input type=submit name=Login></form>";
exit();
}
}
I can't find error in the script :s
btw:
You might want to validate the username input - using either regular expressions or a combination of htmlentities/mysql_real_escape_string and stripslashes (depending on get_magic_quotes_gpc()).

Not a valid resource

Posted: Sun Jul 23, 2006 8:56 am
by ronverdonk
The mysql_num_rows function works only on a result resource (see PHP manual). Your statement does not create a result resource. So you must code:

Code: Select all

$x        = mysql_query($query); 
$result = mysql_num_rows($x);
Ronald :mrgreen:

Re: Not a valid resource

Posted: Sun Jul 23, 2006 9:14 am
by aerodromoi
ronverdonk wrote:The mysql_num_rows function works only on a result resource (see PHP manual). Your statement does not create a result resource. So you must code:

Code: Select all

$x        = mysql_query($query); 
$result = mysql_num_rows($x);
Ronald :mrgreen:

Code: Select all

$result = mysql_num_rows(mysql_query($query));
is perfectly valid.

I'd stick to Charles256's idea - as no value has been assigned to $tblUsers, it looks like a typo to me.

Posted: Sun Jul 23, 2006 10:04 am
by thiscatis
I created a function and here's the result,
what do you think?

Code: Select all

/* --- LOGIN FUNCTIONS ----------------------------------------------------------- */

// verifies the users accountname and password
function UserLogin($sUserAccount, $sUsername, $sPassword) {
	$bValid = false;
	if ($sUserAccount == $sUsername) {
		$sResult = mysql_query("SELECT * FROM tblUsers WHERE tblUsers.username = '" . $sUsername . "'");
		if ($row = mysql_fetch_object($sResult)) {
			$sDbPass = $row->password;
			$bValid = (md5($sPassword) === $sDbPass);
		} else
			die('Invalid username');
		mysql_free_result($sResult);
	} else
		echo "<p style='color: white;'>You can only login to your own account</p>";
	return $bValid;
}

// displays a form to log in
function DisplayLoginForm($sUserAccount) {
	// my form is here but too much html output for the board 
}

// fires the UserLogin function with the parameters from the form
function AttemptLogin($sUserAccount) {
	$bValid = false;
	$sUsername = (isset($_POST['txtLoginUser']) ? $_POST['txtLoginUser'] : '');
	$sPassword = (isset($_POST['txtLoginPwd']) ? $_POST['txtLoginPwd'] : '');
	$bValid = UserLogin($sUserAccount, $sUsername, $sPassword);
	$_SESSION['account_permission'] = (($bValid == true) ? $sUserAccount : '');
	return $bValid;
}

// checks if the user is logged in in the session
function IsLoggedIn($sUserAccount) {
	if (isset($_SESSION['account_permission']))
		$bValid = ($_SESSION['account_permission'] == $sUserAccount);
	else
		$bValid = false;
	return $bValid;
}

Posted: Sun Jul 23, 2006 12:05 pm
by bdlang
thiscatis wrote:
I created a function and here's the result,
what do you think?
Does it work?

What is the value of $sUserAccount and where does it come from? Why is that extra data required other than simply passing the username and password value from the login form?


Comments:
  • You still don't validate or escape any data coming from the form, as I can see.
  • The 'typical' way to validate login is to perform a COUNT(*) on the table that matches that particular username and password. If the resultset value is 1, then the login is valid. This method is far more simple and uses less resources, takes less time; you don't need to perform a SELECT * on the table (thus selecting the username and password you presumably already have) and pulling down all that data for a comparison.
    Example:

    Code: Select all

    // example only, please filter data better than this
    // username escaped and value trimmed
    $username= mysql_real_escape_string(trim($_POST['username']));
    // password hashed, escaped and trimmed
    $password= md5(mysql_real_escape_string(trim($_POST['password'])));
    // sql statement
    $sql= sprintf("SELECT COUNT(*) AS valid_login FROM user_table WHERE username=%s AND password=%s", $username, $password);
    // result and query
    $valid_login= mysql_result(mysql_query($sql), 0, 'valid_login');
    // comparing the result
    if ( $valid_login == 1 ) {
        // user logged in
    } else {
        // nice try
    }
  • IMHO, all three functions 'UserLogin()', 'AttemptLogin()' and 'IsLoggedIn()' could be combined in one function, 'UserLogin()' that returns either the user data required for your script, or BOOL FALSE if the user cannot be logged in.
  • While everyone has a personal preference concerning coding styles, your function 'UserLogin()' is especially 'messy' and hard to follow without clear demarcation between logical code blocks.

    Code: Select all

    if ($row = mysql_fetch_object($sResult)) {
                            $sDbPass = $row->password;
                            $bValid = (md5($sPassword) === $sDbPass);
                    } else
                            die('Invalid username');
                    // where does the if-else statement end? here?
                    mysql_free_result($sResult);
            } else
                    echo "<p style='color: white;'>You can only login to your own account</p>";
            // same thing. It's not clear where your if-else block ends and the function returns.
            return $bValid;
  • In that last code block there, you perform this comparison:

    Code: Select all

    $bValid = (md5($sPassword) === $sDbPass);
    Note that '===' compares value and type. I would leave it as '==', unless you want to specifcally cast $sDbPass as a string. Sometimes comparisons can get tricky...
  • Not a comment on your code necessarily, just a tip. Don't use the exact same names in your form as in your table, e.g. username and username. Change your form name to simply 'userInput' or something different so the end user can't glean the exact table information simply from your form. It also makes it more clear which data is which; the data coming from the form is input, so it is more clear with the name 'userInput'.

Posted: Sun Jul 23, 2006 1:29 pm
by thiscatis
Thanks!
I'm rather new at PHP, so not everything is super from the first try, but i'll see if I can turn some of your comments into better parts in my code!