How to better login script

Coding Critique is the place to post source code for peer review by other members of DevNetwork. Any kind of code can be posted. Code posted does not have to be limited to PHP. All members are invited to contribute constructive criticism with the goal of improving the code. Posted code should include some background information about it and what areas you specifically would like help with.

Popular code excerpts may be moved to "Code Snippets" by the moderators.

Moderator: General Moderators

Grinsa
Forum Newbie
Posts: 11
Joined: Thu Jan 06, 2011 7:03 pm

Re: How to better login script

Post by Grinsa »

Ok, I will need to read up further on defining your own functions, but I understand what this script aims to do. Unfortunately, I still have the same problem as stated previously. I had already changed the alphanum (as well as the checkusername --> checkUsername and checkpassword --> checkPassword) to no avail. I changed this form to checkregister.php and the html form to register.php and it seems like everything is being accepted as successful registration with no checks :?
MichaelR
Forum Contributor
Posts: 148
Joined: Sat Jan 03, 2009 3:27 pm

Re: How to better login script

Post by MichaelR »

The problem I can see is with the scope of the $errorMsg variable. It's not being defined as 'global', so is_array($errorMsg), which is called in the global scope, is always going to return false. Try adding "global $errorMsg" to the functions, as shown below:

Code: Select all

  function checkPassword($value) {

    global $errorMsg;

    if (strlen($value) > 7 || strlen($value) < 33)  {
      return $value;
    }

   else {
     $errorMsg[] = "Your password must be between 8 and 32 characters long!";
   }

 }
However, because you're not using a return value, this can be simplified to the following:

Code: Select all

  function checkPassword($value) {

    global $errorMsg;

    if (strlen($value) < 8 || strlen($value) > 32)  {
      $errorMsg[] = "Your password must be between 8 and 32 characters long!";
    }

 }
Grinsa
Forum Newbie
Posts: 11
Joined: Thu Jan 06, 2011 7:03 pm

Re: How to better login script

Post by Grinsa »

Thanks for your reply. Unfortunately the script does not seem to function after that I added those lines (when I click the register button a white screen is returned). Here are the register form (html file) and the checkregister.php files:

Code: Select all

<html>
<body>
<table width="300" border="0" align="center" cellpadding="0" cellspacing="1" bgcolor="#CCCCCC">
<tr>
<form name="input" action="checkregister.php" method="post">
<td>
<table width="100%" border="0" cellpadding="3" cellspacing="1" bgcolor="#FFFFFF">
<tr>
<td colspan="3"><strong>Register</strong></td>
</tr>
<tr>
<td width="78">Username</td>
<td width="6">:</td>
<td width="294"><input name="username" type="text" id="username"></td>
</tr>
<tr>
<td>Password</td>
<td>:</td>
<td><input name="password" type="password" id="password"></td>
</tr>
<tr>
<td>Confirm Password</td>
<td>:</td>
<td><input name="confirm" type="password" id="confirm"></td>
</tr>
<tr>
<td>Email</td>
<td>:</td>
<td><input name="email" type="text" id="email"></td>
</tr>
<tr>
<td>&nbsp;</td>
<td>&nbsp;</td>
<td><input type="submit" name="register" value="Register"></td>
</tr>
</table>
</td>
</form>
</tr>
</table>
</body>
</html>

Code: Select all

<?php
include('database.php');

 // check empty value
 function checkEmpty($value) {
  global $errorMsg
  if ($value != '') {
   $clean_val = htmlentities($value);
   return $clean_val;
  }
  else {
   $errorMsg[] = "One or more fields are missing!";
  }
 }

 function checkUsername($value) {
  global $errorMsg
  if (strlen($value) < 6 || strlen($value) > 16) {
   $errorMsg[] = "Your username must be between 6 and 16 characters long!";
  }
 }

 function alphanumUsername($value) {
  global $errorMsg
  if (ctype_alnum($value) == false) {
   $errorMsg[] = "Your username must consist of numbers and letters only!";
  }
 }

 function confirm($value, $value1) {
  global $errorMsg
  if ($value != $value1) {
   $errorMsg[] = "Your passwords do not match!";
  }
 }

 function checkPassword($value) {
  global $errorMsg
  if (strlen($value) < 8 || strlen($value) > 32) {
   $errorMsg[] = "Your password must be between 8 and 32 characters long!";
  }
 }

 //---
 checkempty($_POST['username']);
 checkempty($_POST['password']);
 checkempty($_POST['confirm']);
 checkempty($_POST['email']);

 checkUsername($_POST['username']);
 
 alphanumUsername($_POST['username']);
 
 confirm($_POST['password'], $_POST['confirm']);
 
 checkPassword($_POST['password']);
 
 // ----
 
 if (is_array($errorMsg)) {
  foreach ($errorMsg as $key) {
   echo 'Error : '. $key .'<br />';
  }
 }
 else {
  $password = hash('sha512', $_POST['password']);
  $username = $_POST['username'];
  $email = $_POST['email'];

  $query = "INSERT INTO members (username, password, email)
  VALUES ('". mysql_real_escape_string($username) ."', '". $password ."',
  '". mysql_real_escape_string($email) ."')";

  $sql = mysql_query($query);

  if ($sql) {
   echo 'Registration successful';
  }
  else {
   echo 'Registration unsuccessful';
  }
 }
?>
User avatar
greyhoundcode
Forum Regular
Posts: 613
Joined: Mon Feb 11, 2008 4:22 am

Re: How to better login script

Post by greyhoundcode »

Nothing to do with previous comments, but something I always do after a login/logout is regenerate the session ID.
MichaelR
Forum Contributor
Posts: 148
Joined: Sat Jan 03, 2009 3:27 pm

Re: How to better login script

Post by MichaelR »

Grinsa, remember to add a semi-colon (;) after "global $errorMsg".
Grinsa
Forum Newbie
Posts: 11
Joined: Thu Jan 06, 2011 7:03 pm

Re: How to better login script

Post by Grinsa »

Ah yes, sorry about that. This fixed the issue and the script seems to work great. I have just two other minor comments/questions:

1) The first function sanitizes the value inputs and returns a "cleanvalue." Should this be done for the password since it is hashed anyways which eliminates the risk for sql injection?

Code: Select all

 
// check empty value
 function checkEmpty($value) {
  global $errorMsg;
  if ($value != '') {
   $clean_val = htmlentities($value);
   return $clean_val;
  }
  else {
   $errorMsg[] = "One or more fields are missing!";
  }
 }
2) I seem to have some sort of issue with my sessions. They are correctly being set I think but the script won't display the username in the welcome statement:

checklogin.php

Code: Select all

//this is at the very beginning of script
session_start();
session_regenerate_id();

// this is later when password/username combo is verified and if correct:
if ($count==1)
	{
	$_SESSION['login'] = array('username' => $myusr);
	header("location:menu.php");
	}
This is the page that the user is sent to: menu.php

Code: Select all

<?php
session_start();
session_regenerate_id();

if (!isset($_SESSION['login']['username']))
	{
	echo "You must be <a href='index.html'>logged in</a> to view this page!";
	}
else 
	{
?>

	<html>
	<body>

<?php 
	$username = $_SESSION['username'];
	echo "Welcome " . $username . " !";
?>

	<br />
	<a href = logout.php>Log out</a>
	</body>
	</html>

<?php
	}
?>
The message only reads "Welcome !" when I would like for it to include the name.
This is logout.php

Code: Select all

<?php
session_start();
session_regenerate_id();

$_SESSION['login'] = array();
header("location:index.html")
?>
Thanks for all the help! it's almost complete now and i've learned a fair bit along the way!
User avatar
greyhoundcode
Forum Regular
Posts: 613
Joined: Mon Feb 11, 2008 4:22 am

Re: How to better login script

Post by greyhoundcode »

Grinsa wrote:2) I seem to have some sort of issue with my sessions. They are correctly being set I think but the script won't display the username in the welcome statement:
Could it be that you need to replace $_SESSION['username'] with $_SESSION['login']['username']?
User avatar
social_experiment
DevNet Master
Posts: 2793
Joined: Sun Feb 15, 2009 11:08 am
Location: .za

Re: How to better login script

Post by social_experiment »

Grinsa wrote:The first function sanitizes the value inputs and returns a "cleanvalue." Should this be done for the password since it is hashed anyways which eliminates the risk for sql injection?
It depends, you are correct in saying that the password gets hashed and if additional code is included it will not match your existing password. The other part is that eliminate some additional characters from being used in a password (&, >, <). Better to be safe than sorry ;)
“Don’t worry if it doesn’t work right. If everything did, you’d be out of a job.” - Mosher’s Law of Software Engineering
Grinsa
Forum Newbie
Posts: 11
Joined: Thu Jan 06, 2011 7:03 pm

Re: How to better login script

Post by Grinsa »

social_experiment wrote:It depends, you are correct in saying that the password gets hashed and if additional code is included it will not match your existing password. The other part is that eliminate some additional characters from being used in a password (&, >, <). Better to be safe than sorry ;)
Yeah i suppose, but the user would not be aware of the fact that he is not able to use these characters and his password would be different than what he thought he had entered?
greyhoundcode wrote:Could it be that you need to replace $_SESSION['username'] with $_SESSION['login']['username']?
That worked! Thanks.
User avatar
social_experiment
DevNet Master
Posts: 2793
Joined: Sun Feb 15, 2009 11:08 am
Location: .za

Re: How to better login script

Post by social_experiment »

Grinsa wrote:Yeah i suppose, but the user would not be aware of the fact that he is not able to use these characters and his password would be different than what he thought he had entered?
I doubt it would tax your system if you added a simple "Passwords can only contain alphanumeric characters" message somewhere (and then test it with a function ;) ). Users are fickle things, you have to spoonfeed them.
“Don’t worry if it doesn’t work right. If everything did, you’d be out of a job.” - Mosher’s Law of Software Engineering
Post Reply