Mysql Query Help

[jlindsey]

I am stuck on this one little mysql query. I have two columns named FamilyID and username.

My first query is easy because it searches through the whole table for the Family ID that a user is trying to create. If the Family ID that they are trying to create has been created, an errormsg will come up saying, "Family Id has already been created". That works great! My second query is the one I cannot get to work and I know it is because of my very limited skills. What I need is for the query to see if the column FamilyID has data, or if it has no data and I need the search narrowed to the user that is logged in. The variable for the user logged in is $user and the column name is username. So if the the user that is logged in has data in their FamilyID column, I want an errormsg that says "You have already created a Family ID.". If it does not then I obviously want the script to continue. Also FamilyID column is varchar and not init inside the database. Below is what I have so far for my script and the problem area is in bold.

 

 

<?php

 

 

 

if ($_POST['postgift']) {

$Family = $_POST['FamilyID'];

$password = $_POST['password'];

$getretypepass = $_POST['retypepass'];

 

if ($Family) {

if ($password) {

if ($getretypepass) {

if ( $password === $getretypepass ) {

 

require("./connect-mysql.php");

 

 

$query = mysql_query("SELECT * FROM users WHERE FamilyID='$Family'");

$numrows = mysql_num_rows($query);

if ($numrows == 0) {

$query = mysql_query("SELECT FamilyID FROM users WHERE username='$user'");

$numrows = mysql_num_rows($query);

if ($numrows == 0) {

 

$password = md5(md5("j3k4k3".$password."j4j4lk"));

 

$query = ("UPDATE users SET FamilyID='$_POST[FamilyID]', fampassword='$password' WHERE username='$user'");

 

 

if (!mysql_query($query)) {

die('Error: ' . mysql_error());

}

echo 'You Have Created a Family ID. <a href="member.php" style=color:#000000>Click here</a> to go back to Family Christmas Lists';

}

else

$errormsg = "You Have Already Created A Family.";

}

else

$errormsg = "That Family ID has already been Created.";

 

mysql_close();

}

else

$errormsg = "Your passwords did not match.";

}

else

$errormsg = "You must re-enter your password.";

}

else

$errormsg = "You Must Create a password for the Family ID.";

}

else

$errormsg = "You Must Enter a Family ID.";

}

 

echo "<form action='./createfamilyid.php' method='post'>

<table>

<tr>

<td></td>

<td><font color='red'>$errormsg</font></td>

</tr>

<tr>

<td><b>Family ID:</b></td>

<td><input type='text' size='25' name='FamilyID'></td>

</tr>

<tr>

<td><b>Password:</b></td>

<td><input type='password' name='password' value='' /></td>

</tr>

<tr>

 

<td><b>Retype Password:</b></td>

<td><input type='password' name='retypepass' value='' /></td>

 

</tr>

<tr>

<td></td>

<td><input type='submit' name='postgift' value='Create Family ID' /></td>

</tr>

 

</table>

</form>";

 

 

?>

[Muddy_Funster]

Ok, what's your full table layout for the login and family?

What's the Actual problem here? You know that you shouldn't query the database to see if a family id already exists and then error if it does? make familyId a Unique Index and the database will produce an error for you if you try to enter a duplicate.

Why are you running md5() twice?

Don't use select *

[Christian F.]

In addition to the points already raised by Muddy_Funster, I would like to introduce you to two very useful pieces of information.

 

First is I this article about secure login systems, which I recommend quite highly. It'll help you strengthen your login scripts, so that your users' passwords remain secure.

 

Second piece of information, is the "exit early" principle.

What that implies is that whenever you get an error condition in your code, you should detect it and exit from the normal flow as soon as possible. This helps reduce nesting (blocks within blocks within blocks) a lot, and makes the code a whole lot easier to read and maintain. So instead of testing if something succeeded, like you've done in your code above, you should test if something failed.

 

I've rewritten your script a bit to show you the exit early principle in effect, and how you can use functions to make your scripts more user-friendly.

 

 

<?php

if ($_POST['postgift']) {
// TODO: Add some proper validation. 
$family = $_POST['FamilyID'];

$password = $_POST['password'];
$getretypepass = $_POST['retypepass'];

$errorMsg = array ();

if (!$family) {
	$errorMsg[] = "You Must Enter a Family ID.";
}

if (!$password) {
	$errorMsg[] = "You Must Create a password for the Family ID.";
}

if (!$getretypepass) {
	$errorMsg[] = "You must re-enter your password.";
}

if ($password !== $getretypepass) {
	$errorMsg[] = "Your passwords did not match.";
}

// CF: If errors occured with the validation, show the form anew and stop processing.
if (!empty ($errorMsg)) {
	return gen_form ($errorMsg, $family);
}

require ("./connect-mysql.php");

// CF: Added mysql_real_escape_string () to protect against SQL injections.
$query = "SELECT * FROM users WHERE FamilyID='%s'";
$query = sprintf ($query, mysql_real_escape_string ($family));
$res = mysql_query ($query);
$numrows = mysql_num_rows ($res);
if ($numrows != 0) {
	// CF: Generate the proper error message, and show the form anew.
	$errorMsg = "That Family ID has already been Created.";
	return gen_form ($errorMsg, $family);
}

// CF: Added mysql_real_escape_string () to protect against SQL injections.
$query = sprintf ("SELECT FamilyID FROM users WHERE username='%s'", mysql_real_escape_string ($user));
$res = mysql_query ($query);
$numrows = mysql_num_rows ($res);
if ($numrows != 0) {
	// CF: Generate the proper error message, and show the form anew.
	$errorMsg = "You Have Already Created A Family.";
	return gen_form ($errorMsg, $family);
}

// FIXME: This is extremely bad security, please read the article linked to in the post above:
$password = md5 ("j3k4k3" . $password . "j4j4lk");

// CF: Added mysql_real_escape_string () to protect against SQL injections.
$query = "UPDATE users SET FamilyID='%s', fampassword='%s' WHERE username='%s'";
$query = sprintf ($query, mysql_real_escape_string ($family), $password, mysql_real_escape_string ($user));
if (!mysql_query ($query)) {
	// FIXME: Remove the mysql_error () call before putting into production, to prevent giving
	//		valuable information about the system to any potential attackers.
	return ('Error: ' . mysql_error ());
}

// CF: Send the browser to the confirmation page, to prevent the F5-resubmit problem.
header ("Location: {$_SERVER['SCRIPT_NAME']}?send=ok");
die ();
}

// CF: If a family ID has been successfully created.
if (isset ($_GET['send']) && $_GET['send'] == 'ok') {
$message = 'You Have Created a Family ID.  <a href="member.php" style=color:#000000>Click here</a> to go back to Family Christmas Lists';
}

// CF: Generate the form for first-time execution.
$form = gen_form ();

/**
* Generates the family ID form, and prepoluates it with any previous ID or error messages. If given.
* 
* @param mixed $errorMsg = ''
* @param string $id = ''
* @return string
*/
function gen_form ($errorMsg = '', $id = '') {
// If one or more error messages have been set, create a finished string for them.
if (!empty ($errorMsg) && is_array ($errorMsg)) {
	$errorMsg = implode ("<br />\n", $errorMsg);
}

// Escape the ID to prevent XSS and other HTML injection attacks.
$id = htmlspecialchars ($id);

// Generate and return the completed form.
return <<<OutHTML
<p class="error">$errormsg</p>
<form action="" method="post">
<fieldset>
	<label for="inp_id">Family ID:</label>
	<input id="inp_id" type="text" size="25" name="FamilyID" value="$id" />

	<label for="inp_pass">Password:</label>
	<input id="inp_pass" type="password" name="password" value="" />

	<label for="inp_rep">Retype Password:</label>
	<input id="inp_rep"type="password" name="retypepass" value="" />
</fieldset>

<fieldset class="buttonline">
	<input type="submit" name="postgift" value="Create Family ID" />
</fieldset>
</form>

OutHTML;
}

 

 

Notice how I've used the gen_form () function to show the form anew, with the family ID already populated, if there are any errors during the validation of the family generation.

[mrMarcus]

if (!$family) {
   $errorMsg[] = "You Must Enter a Family ID.";
}

 

This will always return true, regardless of whether $family contains a value or not.

 

The ! operator is checking if !$family is NOT TRUE (Layman's terms: FALSE)... which it isn't.  It's also not TRUE.  It's not of boolean type.  Use empty() when checking form posted values (isset is not necessary since every $_POST'ed form input is sent on form submission.  Because of that, you need not check whether they are set; simply check their value's).

 

if (isset($_POST['submit'])) {

$foo = $_POST['bar'];
if (!$foo) {
	die('I will always return...');
}
}
...
<form action="" method="post">
<input type="text" name="bar" value="Go Leafs, Go."/> <input type="submit" name="submit"/>
</form>

Edited December 19, 2012 by mrMarcus

[Christian F.]

Actually, as long as the family ID provided isn't 0 or an empty string, it'll evaluate to "true". I'm taking advantage of the PHP type autocasting rules, in which an non-empty string (which isn't "0") evaluates to true.

 

php > var_dump (!"0");
bool(true)
php > var_dump (!"");
bool(true)
php > var_dump (!"test");
bool(false)

 

I know what I'm doing, but thanks for being on the lookout.

[mrMarcus]

Actually, as long as the family ID provided isn't 0 or an empty string, it'll evaluate to "true". I'm taking advantage of the PHP type autocasting rules, in which an non-empty string (which isn't "0") evaluates to true.

 

php > var_dump (!"0");
bool(true)
php > var_dump (!"");
bool(true)
php > var_dump (!"test");
bool(false)

 

I know what I'm doing, but thanks for being on the lookout.

 

Had a typo in my test code.