...

View Full Version : Write the perfect activation script



guvenck
02-28-2007, 11:08 PM
Hello,

I wrote this code below as an activation script. Sometimes it causes problems with users, giving a "You have already activated your account!" message at first click on the activation link in the email.

Then I went through the code and did not like my logic. I think it could have been written better and simpler. Can you please look at it and make suggestions? Thanks.



<?php

$uniqueid = $_GET['uniqueid'];
$usrid = $_GET['usrid'];

include ("db.php");

// Check if already activated
$check1 = mysql_query("SELECT * FROM members WHERE ID='$usrid' AND uniqid='$uniqueid' AND activated='1'");
if(mysql_num_rows($check1) == 1) {
include ("header.php");
echo '<p class="error">You have already activated your account!</p>';
include ("footer.php");
exit();
}

// Check if not activated yet
$check2 = mysql_query("SELECT * FROM members WHERE ID='$usrid' AND uniqid='$uniqueid' AND activated='0'");
if(mysql_num_rows($check2) == 0) {
include ("header.php");
echo '<p class="error">Activation failed!</p>';
include ("footer.php");
exit();
}

if(mysql_num_rows($check2) == 1) {
$sql = mysql_query("UPDATE members SET activated='1' WHERE ID='$usrid' AND uniqid='$uniqueid'");
$doublecheck = mysql_query("SELECT * FROM members WHERE ID='$usrid' AND uniqid='$uniqueid' AND activated='1'");
if (mysql_num_rows($doublecheck) == 0) {
include ("header.php");
echo '<p class="error">Activation failed!</p>';
include ("footer.php");
exit();
}
if (mysql_num_rows($doublecheck) == 1) {
include ("header.php");
echo '<p class="success">You have successfully activated your account! Click <a href="login.php">here</a> to login.</p>';
include ("footer.php");
}
}

?>

marek_mar
02-28-2007, 11:24 PM
The doublecheck is not needed. You only need to see if the update query has affected any rows.

guvenck
03-01-2007, 12:09 AM
Hi Marek, updated the code:
Apart from mysql_affected_rows, does the logic look OK?




<?php

$uniqueid = $_GET['uniqueid'];
$usrid = $_GET['usrid'];

include ("db.php");

// Check if already activated
$check1 = mysql_query("SELECT * FROM members WHERE ID='$usrid' AND uniqid='$uniqueid' AND activated='1'");
if(mysql_num_rows($check1) == 1) {
include ("header.php");
echo '<p class="error">You have already activated your account!</p>';
include ("footer.php");
exit();
}

// Check if not activated yet
$check2 = mysql_query("SELECT * FROM members WHERE ID='$usrid' AND uniqid='$uniqueid' AND activated='0'");
if(mysql_num_rows($check2) == 0) {
include ("header.php");
echo '<p class="error">Activation failed!</p>';
include ("footer.php");
exit();
}

if(mysql_num_rows($check2) == 1) {
$sql = mysql_query("UPDATE members SET activated='1' WHERE ID='$usrid' AND uniqid='$uniqueid'");
if(mysql_affected_rows() == 1) {
include ("header.php");
echo '<p class="success">You have successfully activated your account! Click <a href="login.php">here</a> to login.</p>';
include ("footer.php");
exit();
} else {
include ("header.php");
echo '<p class="error">Activation failed!</p>';
include ("footer.php");
}
}

?>

meth
03-01-2007, 12:42 AM
There's a gaping hole in your security.

What happens when a user loads this URL (change the domain/page to yours):


http://www.myDomain.com/activationPage.php?uniqueid=1;SHOW%20TABLES;#&usrid=1

Never trust external data, whether it comes from a form POST or a URL GET.

guvenck
03-01-2007, 02:10 AM
Hi,

It happens nothing. Gives me a "Activation Failed!" message.

uniqueid is a 32 char randomly generated string based on timestamp.

7ebbefdf38b0fe16547ea4cb725f632b is a uniqueid.

This is an email activation. The unique goes to the inbox of the user. Another uniqueid is a bit difficult to guess. What other option do I have other than trusting this external data?

koyama
03-01-2007, 02:42 AM
Do you need to send the $usrid ?

guvenck
03-01-2007, 11:15 AM
No, I guess not. But don't want to rely on uniqueid alone. I send the usrid under a different variable (e.g. "confirm") and expect the careful user not to understand this.

meth
03-01-2007, 01:30 PM
Just ensure the token string is unique before sending the url. Query your db for the same string; if true, send the email; if false, generate another and check again. Perhaps concatenate the userid to the timestamp before md5. This way you can eliminate userid from the url and ensure the token is unique the first time it's generated.

You also may want to sanitize $_GET['uniqueid'] before using it in a query.

aedrin
03-01-2007, 04:19 PM
Base your uniqid on the user ID, that way you know it is a unique number (assuming that your user's ID is generated through an auto_increment column).

You shouldn't need 4 queries to do activation.

You just select where activated equals 0 and it has the right uniqid. If it has a row (only 1), then update the table to set activated.

If it doesn't have any rows, print something like "Activation failed or you have already activated your account."

There is no need to do queries like that. If you really needed to know whether they are activated already (versus it failing), just select without the where activated. And do the logic in PHP (if row['activated'] == 0, etc.).

guvenck
03-02-2007, 02:42 PM
I wrote the following to generate the uniqid, could be done with a single function?





function CheckToken($token) {
include("db.php");
$result = mysql_query("SELECT uniqid FROM members WHERE uniqid='$token'") OR die(mysql_error());
if (mysql_num_rows ($result) == 0) { // does not exist
return FALSE;
} else { // exists
return TRUE;
}
}


function GenerateToken() {
$token = md5(uniqid(rand(), TRUE));
while(CheckToken($token)) {
GenerateToken();
}
return $token;
}


// generate uniqid
$uniqid = GenerateToken();

aedrin
03-02-2007, 05:49 PM
That's a pretty bad setup.

Although uniqid is generally pretty unique and similar numbers don't occur often, you are setting yourself up for a really bad situation where a simple script will drag down the database and server.

Like I said, your best bet is to generate it from the User ID.



query("INSERT INTO users(id, name) VALUES(NULL, 'John Doe')");

$id = mysql_insert_id();

$uniqid = uniqid($id);

query("UPDATE users SET uniqid = '{$uniqid}' WHERE id = {$id}");

guvenck
03-02-2007, 06:52 PM
So you say I should add the User ID into the uniqid as a prefix? Hmmm. Can't say I like that that much :confused:

Inigoesdr
03-02-2007, 07:37 PM
So you say I should add the User ID into the uniqid as a prefix? Hmmm. Can't say I like that that much :confused:

You don't need the user ID to make it random, there are literally hundreds of reasonable possibilities. This should work fine:

$uniqid = sha1(uniqid(time(),1));
md5() will work fine instead of sha1() too. You could substr() it if you wanted to a smaller string, or even generate a random number from the strlen() to substr it to.

guvenck
03-02-2007, 08:42 PM
Hi, Inigoesdr, I am trying to make it unique rather than random.

I decided to use this:



$uniqid = uniqid(md5($userid));


That generates 45 characters in total. I've also added a check in case the number exists, it exits and emails me :)

Couldn't write something to regenerate it if it exists :(

Inigoesdr
03-02-2007, 08:44 PM
Using that code will always be unique. You don't need to check.

marek_mar
03-03-2007, 12:46 PM
You should adda unique index to the activation key column and insert/update it as long as the affected rows are 1. The chance of uniqueid() generating exactly the same number is extreeemly small, though it depends on the amounts of users you will be having (but I don't think you'll be getting anywhere close in user count).

Inigoesdr
03-03-2007, 05:23 PM
You should adda unique index to the activation key column and insert/update it as long as the affected rows are 1. The chance of uniqueid() generating exactly the same number is extreeemly small, though it depends on the amounts of users you will be having (but I don't think you'll be getting anywhere close in user count).

If he prefixes the userid in uniqid() like he has then it will always be unique. ;)

guvenck
03-04-2007, 05:21 PM
Hi Inigoesdr,


$uniqid = sha1(uniqid(time(),1));

MD5 is reported to have collisions. I don't know about sha1 though.



$uniqid = uniqid(md5($userid));

I don't prefix the userid, I prefix the MD5 of userid. I don't know if that would be unique, but as md5 of the same string is always the same, probably it would be unique.

Inigoesdr
03-04-2007, 05:29 PM
Hi Inigoesdr,


$uniqid = sha1(uniqid(time(),1));

MD5 is reported to have collisions. I don't know about sha1 though.



$uniqid = uniqid(md5($userid));

I don't prefix the userid, I prefix the MD5 of userid. I don't know if that would be unique, but as md5 of the same string is always the same, probably it would be unique.
Yes, it would be unique.



EZ Archive Ads Plugin for vBulletin Copyright 2006 Computer Help Forum