More PHP woes

[Schmoopy]

Ok so I'm still trying to code a newsletter system, and am at the registration stage, and am trying to achieve the following:

Validate user id and hash.
Check email is not already active.
Update table if details are correct.

At the moment, when the user first signs up to the newsletter, their details are stored in a table with the following fields : ID, email and Active.

When the user first signs up the Active field is 0 by default, and an email is sent out to their account to verify they want to join.

Here is the code I'm currently using (and yes it's probably terrible, but there you go...):



<?php
$title = "Bristol DNB - Register";
include("page_header.php");
$hash = $_GET['ver'];
$id = $_GET['id'];

if ($id == "" || $hash == "")
{
die("Invalid information");
}


$con = @mysql_connect("$host","$user","$password");
mysql_select_db("bristoldnb_mail",$con);

if (!$con)
{
die("Looks like something went wrong while connecting to the server, please try again later<p></p>You will be redirected back to the home page in 10 seconds, or click <a href=\"$server_url\">here</a>");
}

$active = mysql_query("SELECT `email` FROM `subscribers` WHERE `ID` = '$id' AND `Active` = '1'");
if($srow = mysql_fetch_row($active))
{
die("Subscription already active");
}

elseif($sql = mysql_query("SELECT `email` FROM `subscribers` WHERE `ID` = '$id' AND `Active` = '0'")) {

if (mysql_fetch_row($sql))
{
die("Oh noes");
}

$row = mysql_fetch_row($sql);
$email = $row[0];

if ($hash == sha1($email . "bristoldnb"));
{
mysql_query("UPDATE `subscribers` SET `Active` = '1' WHERE `id` = '$id'");
echo "Registration Successul";
}

}

?>
</div>

<div id="content">
<div id="unsubscribe">

</div>
</div>

<?php
include("page_footer.php");
?>




I'm pretty sure some of the code doesn't make sense but I've just been playing around with values to get some sort of response from the php.

Sort of lost as to what to do, I've tried using invalid ids and hash keys but still not working.

Thanks for any help,

Jack.

[JasonDFR]

My instinct is always to keep things as simple as possible. I would do away with your hash, use the email address and a unique "id" to build the url for the email validation, and just test to see if an email / id combination exists in your db. If so, activate that subscriber.

You can do this because in order to activate an account, someone will need the "id" from the email you send out. If they don't have access to the id in the email, they can't activate the account. If they do have access to the email, well, that's the point.

Testing to see if an account is already active might just complicate matters. Is the outcome going to be any different if an account is already active? No, the end result is that the subscriber has an active account, so you can do away with that step.

As long as unsubscribe deletes the email address, id, or both from your table, you are in good shape.

Example URL: your_script.php?email=email@email.com&id=56456




// Validate the information being passed in the URL; this is a good regex for any email validation btw
if ( preg_match('/^[A-Za-z0-9](([_\.\-]?[a-zA-Z0-9]+)*)@([A-Za-z0-9]+)(([\.\-]?[a-zA-Z0-9]+)*)\.([A-Za-z]{2,})$/', $_GET['email']) && is_numeric($_GET['id']) ) {

// Get your values to test against the db
$email = $_GET['email'];
$id = $_GET['id'];

// db connection here

// Build query to match an existing email address with it's corresponding id
$q = "SELECT *
FROM `subscribers`
WHERE `subscribers`.`email` = '$email'
AND `subscribers`.`id` = '$id'
LIMIT 1 ";

$r = mysql_query($q) or die ("Query Failed: " . mysql_error());

if ( mysql_num_rows($r) == 1 ) { // There is a legitimate match

// Query db and set active to 1

$q = "UPDATE `subscribers` SET `active` = '1'
WHERE `users`.`email` = '$email' LIMIT 1 ";

if ( mysql_query($q) ) {

echo '<p>Your account is activated.</p>';

}

} else { // mysql_affected_rows does not equal 1

// There is no match

}

} else { // validation failed

// bad information is being passed in the URL

}


Good luck,

Jason

[Schmoopy]

Ah, much simpler in the approach. thanks for the help.