Issues with email activation

[Pezmo]

I am building a site that requires users to register and login to view and use certain parts of the site.

When a user registers, an email is sent to them with a link that they need to click to activate their account.

 

The link in the email contains the users email and an activation code and takes them to a page named 'activate' and checks the following conditions...

1. If the email in the url exists in the database (This works)
2. If the activation code in the url exists in the database (This works)
3. If both of the previous conditions are true and the account active state is N then UPDATE `active` in database to Y thus allowing the user to login. (This works but not correctly)

What I want to happen is...

User registers > User gets email > *** (Anything before this point users cannot login. They will get a message telling them that their account has not been activated) *** > User clicks link > Users account is activated (`active` is changed from N to Y) > User can log in

 

What I have is...

User registers  > *** (If the user logs in anytime after registration they can log in and the account is activated without clicking on the link in the email)  > *** User gets email and clicks on the link >  Goes to page and gets a message saying the account has already been activated.

 

I have tested the conditions and if the email does or does't exist I get the correct messages and the same goes for the activation code, but the third seems to happen as soon as the email is sent to the user allowing them to log in before they click the link.

 

Here is the where the email is sent...

if ($OK) { // If OK insert data to database
	mysqli_query($db_connect, "INSERT INTO `users` (`uname`, `password`, `email`, `fname`, `lname`, `contact`, `house`, `street` `postcode`, `regdate`, `activationCode`, `active`) VALUES ('".$uname."', '".$passHash."', '".$email."', '".$fname."', '".$lname."', '".$contact."', '".$house."', '".$street."', '".$postcode."', '".$regdate."', '".$activationCode."', 'N')") or die(mysqli_connect_error());
	
	// Set up email to send
	function email($to, $subject, $body) {
		@mail($to, $subject, $body, 'From: Example <info@example.com>');
	}	
	
	// Send email to user for account activation 
	email($email, 'Activate your account', "Hello ".$uname."\nYou have recently created an account using the credentials...\n\nUsername - ".$uname."\nPassword - ".$password."\n\nPlease click the link below to activate your account.\nhttp://www.example.com/activate?email=".$email."&activationCode=".$activationCode."\n\nThank You.\n\n");
	echo "<p>A message has been sent to ".$email.", please check your emails to activate your account.<p>";
	echo "<p>If you do not receive the message in your inbox please be sure to check your junk mail too.</p>";
	session_destroy();
} else {
	back();
	exit();
} 

Here is the ACTIVATE page...

if (isset($_GET['email'], $_GET['activationCode']) === true) { // If email and email code exist in URL
    $email = mysqli_real_escape_string($db_connect, trim($_GET['email']));
    $activationCode = mysqli_real_escape_string($db_connect, trim($_GET['activationCode']));
    
    $query = mysqli_query($db_connect, "SELECT * FROM `users` WHERE `email` = '".$email."' ") or die(mysqli_connect_error());
    $result = (mysqli_num_rows($query) > 0);
    
    if ($result) { // Check email exists in database
        while($row = mysqli_fetch_assoc($query)) {
            if ($row['activationCode'] == $activationCode) { // Check activation code exists in database

    
// THIS IS THE PART NOT WORKING CORRECTLY -----------------------------------------------------------------------------------------------------------------------------------------------------

                if ($row['active'] == 'Y') { // Account is active
                    echo $failed."<p>Your account has already been activated. You may <a href='/login'>Log In</a></p>";                
                } else { // Account not active
                    mysqli_query($db_connect, "UPDATE `users` SET `active` = 'Y' WHERE `email` = '".$email."' LIMIT 1"); // Activate account
echo $success."<p>Your account is now activated. You may <a href='/login'>Log In</a></p>";
                }

// --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------

            } else { // Activation code is invalid
                echo $failed."<p>Hmmm, the activation code seems to be invalid!</p>";    
            }
        }                        
    } else { // Email does not exist
        echo $failed."<p>Hmmm, ".$email." does not seem to exist in our records!</p>";        
    }    

} else {
    header("Location: /login");
    exit();
}

Here is the LOGIN page...

if (isset($_POST['login'])) { // Create variables from submitted data
	$uname = mysqli_real_escape_string($db_connect, $_POST['uname']); 
	$password = mysqli_real_escape_string($db_connect, $_POST['loginPassword']);
	$password = md5($password); // Encrypt password
	
	$query = mysqli_query($db_connect, "SELECT * FROM `users` WHERE `uname` = '".$uname."' AND `password` = '".$password."' ") or die(mysqli_connect_error()); // Check if uname and password match
	$result = (mysqli_num_rows($query) > 0);

	if ($result) { // If uname and password match
		while($row = mysqli_fetch_assoc($query)) {
			if ($row['active'] == 'N') { // Account is not active
				echo "<p>Your account has not been activated! Please check your email inbox.</p><br />";		
			} else if ($row['active'] == 'Y') { // Account is active
				$_SESSION['uname'] = $_POST['uname'];
				header("Location: /profile");		
			}
		}
	} else { // If uname and password do not match
		echo "<p>The combination of username and password is incorrect!</p><br />";
	}
	
} else { // Default
	login(); 
	register();
	exit();		
} 

Why it is not working correctly or what I am doing wrong?

All help appreciated in advance.

Edited September 8, 2015 by Pezmo

[hansford]

I would be checking this part of the code based on some known username, password and status of both Y and N.

echo out the value of $row['active'];

Personally, I wouldn't use a character for this column to compare. I would use a Boolean value of 1 or 0;

if ($result) { // If uname and password match
		while($row = mysqli_fetch_assoc($query)) {
			if ($row['active'] == 'N') { // Account is not active
				echo "<p>Your account has not been activated! Please check your email inbox.</p><br />";		
			} else if ($row['active'] == 'Y') { // Account is active
				$_SESSION['uname'] = $_POST['uname'];
				header("Location: /profile");		
			}

[Pezmo]

I was using 0 and 1 before and it still wasn't working so just trying out different things. It was set up that when a user registers, `active` was set to 0 by default and when activated via activate page (from email link) it would turn to 1 but thought that may have been causing the issue.

If I remove the 3rd condition it works as it should but user will never be able to activate account (obviously) but when its there it automatically changes the active state as soon as the user registers.

Edited September 9, 2015 by Pezmo

[QuickOldCar]

Not quite sure why your Y/N not working...

Do as hansford suggested and echo $row['active'] in your while loop to see what it is

 

md5 is not secure to use and should look into password_hash() and password_verify() instead

 

Also why are you adding a Limit 1 to the update statement?

mysqli_query($db_connect, "UPDATE `users` SET `active` = 'Y' WHERE `email` = '".$email."' LIMIT 1");

 

If you would so happen to have more than one of the same email should also be doing an additional check, I personally use autoincrement id's or make a unique constraint on emails only allowing one in the entire database.

mysqli_query($db_connect, "UPDATE `users` SET `active` = 'Y' WHERE `email` = '".$email."' AND `uname` = '".$row['uname']."'");

Don't see the need for an if else check I also like checking positively

if ($row['active'] == 'Y') { // Account is active
    $_SESSION['uname'] = $_POST['uname'];
    header("Location: /profile");
    exit;   
} else  { // Account is not active
    echo "<p>Your account has not been activated! Please check your email inbox.</p><br />"; 
}

[hansford]

It's difficult to tell where the logical error is from the information we are presented with.  Can more than one user potentially have the same username and password ?  Users can have the same password, but there should only be one user with that name.

[Pezmo]

No user can have same email or username. That is checked in the registration page and will reject if the username or email already exists in the database.

[mac_gyver]

your symptom of a user being able to log in (or perhaps it means they are already logged in when visiting the login page) without needing to go through the email activation step sounds like the email address was already present and activated from previous testing. are you deleting the row(s) from your database table and logging out between tests?

 

are you sure your registration code that's checking if an email address/username is already present is actually doing what you expect? what is the full registration code?

 

is your login code checking that the visitor is NOT already logged in before allowing them to log in again?

 

the code you have posted has at least one header() redirect that doesn't have an exit/die statement after it. if you have code later in the same file that's doing something with the database table, that could account for the symptom. other than that, there's nothing in the snippets of posted code that would allow what you are stating as a symptom.

 

and while this has nothing to do with the problem, why are you looping over the result from your queries. for a query that will at most match only one row, just fetch the row.

Edited September 9, 2015 by mac_gyver

[scootstah]

 

$password = md5($password); // Encrypt password

 

Do not use MD5 for hashing passwords. Use password_hash and password_verify.

[Pezmo]

I have changed some of the areas you have all mentioned (md5 to password_hash()) and a few other pointers and kept looking for the cause and then I looked at where the email was sent and added single quotes to the link and it now works perfectly. Cheers for all your help and tips, mucho appreciated.