Jump to content


Photo

Whats wrong?


  • Please log in to reply
3 replies to this topic

#1 jwk811

jwk811
  • Members
  • PipPipPip
  • Advanced Member
  • 714 posts

Posted 17 October 2006 - 06:52 PM

I am creating a membership and got it so that someone can register and the info goes to a database.. after that they recieve an email and are given a link to click to activate their account. Heres the acivate.php file.
<? 
/* Account activation script */ 

// Get database connection 
include 'db.php'; 

// Create variables from URL. 

$userid = $_REQUEST['id']; 
$code = $_REQUEST['code']; 

$sql = mysql_query("UPDATE users SET activated='1' WHERE userid='$userid' AND password='$code'"); 

$sql_doublecheck = mysql_query("SELECT * FROM users WHERE userid='$userid' AND password='$code' AND activated='1'"); 
$doublecheck = mysql_num_rows($sql_doublecheck); 

if($doublecheck == 0){ 
    echo "<strong><font color=red>Your account could not be activated!</font></strong>"; 
} elseif ($doublecheck > 0) { 
    echo "<strong>Your account has been activated!</strong> You may login below!<br />"; 
    include 'login.php'; 
} 

?>
For some reason everytime I do it, it will say "Your account could not be activated!". And I can see that in the if else statment its double checking to make sure that the activated cell in the table on my database is set to 1, but why isn't is setting it to 1? That's why I'm getting the error? Would you happen to know why? I'm connecting to the database and everything okay, is there something wrong with this script? Any help would be great!

#2 alpine

alpine
  • Members
  • PipPipPip
  • Advanced Member
  • 756 posts
  • LocationNorway

Posted 17 October 2006 - 07:07 PM

I've adjusted it a bit, see what you get out of this one:

<?php

if(!empty($_GET['id'] && !empty($_GET['code'])))
{
include 'db.php';

$userid = $_GET['id'];
settype($id,"integer");
$code = htmlspecialchars($_GET['code']);

$sql_check = mysql_query("SELECT * FROM users WHERE userid='$userid' AND password='$code'") or die(mysql_error());
if(mysql_num_rows($sql_check)==1)
{
$sql_update = mysql_query("UPDATE users SET activated='1' WHERE userid='$userid' AND password='$code'") or die(mysql_error());
if(mysql_affected_rows()==1)
{
echo "<strong>Your account has been activated!</strong> You may login below!<br />";
include 'login.php';
}
else
{
echo "<strong><font color=red>Your account could not be activated at this time!</font></strong>";
}
}
else
{
echo "<strong><font color=red>No account found matching the submitted activation data</font></strong>";
}
}
else
{
echo "<strong><font color=red>Missing nessesary activation data</font></strong>";
}

?>


But, i wouldn't recomend you using the userid AND the password so wide open like that, if you want to use the password i would at least encrypt it with md5() or sha1() in the table AND in the activation email etc.

#3 printf

printf
  • Staff Alumni
  • Advanced Member
  • 889 posts

Posted 17 October 2006 - 07:22 PM

Your not testing the query for errors, plus your not stopping anyone from reavtivating that already activated. Plus your not validating the dangerous inputs!

If you want to do it your way then do something like this!

<?

/* Account activation script */

// Get database connection

include ( './db.php' );

// Create variables from URL.

// first check if it's already been activated

$sql = mysql_query ( "SELECT COUNT(*) AS total FROM users WHERE userid = '" . mysql_real_escape_string ( $_REQUEST['id'] ) . "' AND password = '" . mysql_real_escape_string ( $_REQUEST['code'] ) . "' AND activated = 1" ) or die ( 'Query Error: ' . mysql_error );

$found = mysql_ftech_assoc ( $sql );

if ( $found['total'] == 0 )
{
	$sql = mysql_query ( "UPDATE users SET activated = 1 WHERE userid = '" . mysql_real_escape_string ( $_REQUEST['id'] ) . "' AND password = '" . mysql_real_escape_string ( $_REQUEST['code'] ) . "'" ) or die ( 'Query Error: ' . mysql_error );

	if ( mysql_affected_rows ( $sql ) == 0 )
	{
		echo "<strong><font color='red'>Your account could not be activated, no user found by that id or password!</font></strong>";
	}
	else
	{
		echo "<strong>Your account has been activated!</strong> You may login below!<br />";

    		include ( './login.php' );
	}
}
else
{
	echo "<strong>You have already activated your account!</strong> You may login below!<br />";

    	include ( './login.php' );
}

?>


But I wouldn't do that, I would have (2) tables, one that holds the activated users and the other that holds the users awaiting activation, this way you don't add user to the user table that may never activate. But more importantly you would need only (1) query for activation, instead of this way which needs (2). It all about control logic!

me!

#4 jwk811

jwk811
  • Members
  • PipPipPip
  • Advanced Member
  • 714 posts

Posted 18 October 2006 - 01:36 AM

okay thanks, ill just something like that.. how could i combine the two to make it so it does all that stuff? and what did you mean about the password thing? ill fairly new to php.




0 user(s) are reading this topic

0 members, 0 guests, 0 anonymous users