PHP programming forum. Ask questions or help people concerning PHP code. Don't understand a function? Need help implementing a class? Don't understand a class? Here is where to ask. Remember to do your homework!
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()).
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:
/* --- 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;
}
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:
// 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.
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:
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'.
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!