Login Form Critique

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

Post Reply
CoolAsCarlito
Forum Contributor
Posts: 192
Joined: Sat May 31, 2008 3:27 pm
Contact:

Login Form Critique

Post by CoolAsCarlito »

This is for my login process page. I was hoping someone could go through that didn't have anything to do and tell me what they thought of it, if something looked off, out out place, any ideas.

Code: Select all

<?php

session_start();

// Include the database page
require ('../inc/dbconfig.php');
require ('../inc/global_functions.php');

//Login submitted
if (isset($_POST['submit'])) { 

	// Not already logged in
    if(!isset($_SESSION[$loggedinUserDataArray])) { 
    
        // Errors defined as not being any
        $errors = "no";
        
        // Assign variable values if there is values
        $username = empty($_POST['username'])?null:trim($_POST['username']);
        $password = empty($_POST['username'])?null:trim($_POST['password']);
        
        // Error checking, make sure username and password given 
		if (!$username || !$password) {
			
            // No username or password given error
            $errors = "yes";
            $output = "You must enter both username and password!";
            
		} else {
      
            // No errors reported 
            // Escape post data
            $username = mysqli_real_escape_string($dbc,$_POST['username']);
            
            // Query the database for user info with username
            $query = "SELECT * FROM manager_users WHERE username = '".$username."'";
            $result = mysqli_query($dbc,$query);
            
            // Fetch returned data from result set
            $row = mysqli_fetch_array($result);
            
            // Count number of returned results from query
            if (mysqli_num_rows($result) > 0) {
                
                // Assign query array values to variables
                $userID = $row['userID'];
                
                // Find out if the user has been verfied afer they were registered
                if ($row['statusID'] == 1) {
                    
                    // User was not verified error
                    $errors = "yes";
                    $output = array('errorsExist' => true, 'message' => 'Sorry you must verify your email address before logging in. Didn\'t get the verification email? Don\'t worry we can <a href="javascript:void(0);" id="resendVerification">resend it</a>!');
                    
                    if ($row['lockDate'] !== "0000-00-00 00:00:00") {
                        
                        if (($row['statusID'] == 2) AND (strtotime($row['lockDate']) <= time())) {
                            
                            $errors = "yes";
                            $output = array('errorsExist' => true, 'message' => 'Your account is currently locked, we appologize for the inconvienence. This is a security messure implimented by to many failed login\'s!');
                        
                        } else {
                        
                            $data = array('lockDate' => '0000-00-00 00:00:00', 'statusID' => 1);
                            $username = mysqli_real_escape_string($row['username']);
                            $query = "UPDATE manager_users SET '".$data."' WHERE username = '".$username."'";
                            
                            $output = array('errorsExist' => true, 'message' => 'Sorry you must verify your email address before logging in. Didn\'t get the verification email? Don\'t worry we can <a href="javascript:void(0);" id="resendVerification">resend it</a>!');				
                        
                        }
                        
                    }
                    
                } else if ($row['statusID'] == 3) { // Find out if the user was suspended
                    
                    // User is suspended error
                    $errors = "yes";
                    $output = array('errorsExist' => true, 'message' => 'Your account has been suspended and is pending deletion. If you would like to contest this action <a href="javascript:void(0);" id="contestSuspension">click here</a>!');

                } else if ($row['statusID'] == 4) { // Find out if the user is pending deletetion
                    
                    // User is pending deletion error
                    $errors = "yes";
                    $output = array('errorsExist' => true, 'message' => 'Your account is currently pending deletion, would you like to reactivate it? <a href="javascript:void(0);" id="undeleteAccount">Yes, Reactivate</a>!');

                } else {
                    
                    // User is registered, validated and has is active
                    // Retrieve value of the lock out date and find the minute difference
                    $lockDate = empty($row['lockDate'])?0:strtotime($row['lockDate']);
                    $currentDateTime = time();
                    $minutes = floor(($currentDateTime-$lockDate) / 60);
                    
                    // Take minutes and perform tasks
                    if ($lockDate > 0 && $minutes <= 10) {
                        
                        // Calculate time remaining
                        $timeRemaining = 10 - $minutes;
                    
                        // Account locked error
                        $output = array('errorsExist' => true, 'message' => 'Your account is currently locked, we appologize for the inconvienence. You must wait ' .$timeRemaining.' minutes before you can log in again!');
                        
                    } else {
                        
                        // Assign password value from database to variable
                        $password = $row['password'];
                        
                        // Assign password 2 value from database to variable
                        $password2 = $row['password2'];
                        
                        // Assign hashed password to variable
                        $regenFromPostPW = reGenPassHash($_POST['password'], $password2);
                        
                        // Verify there is a value for lockDate greater than 0
                        if ($lockDate > 0) {
                        
                            // Clear the lock
                            $query = "UPDATE manager_users SET lockDate = NULL WHERE username = '".$username."'";
                            $result = mysqli_query($dbc,$query);
                            
                            // Reset the value of number of attempts
                            $_SESSION['numberOfAttempts'] = 0;
                        
                        }
                        
                        // Comparing the database password with the posted password
                        if ($password == $regenFromPostPW) {
                            
                            // Login successful
                            $numberOfLogins = $row['numberOfLogins']+1;
                            
                            // Query the database for user info with userID
                            $query = "SELECT * FROM manager_users WHERE userID = '".$userID."'";
                            $result = mysqli_query($dbc,$query);
                            
                            // Fetch returned data from result set
                            $row = mysqli_fetch_array($result);
                            
                            // Assign query array values to variables
                            $userID = $row['userID'];
                            $firstName = $row['firstName'];
                            $lastName = $row['lastName'];
                            
                            // Assign user data into an array
                            $loggedinUserDataArray = array('userID' => $userID, 'name' => $firstName . " " . $lastName);
                            
                            // Assign user data array to new session
                            $_SESSION['user_data'] = $loggedinUserDataArray;
                        
                            // Update db with new values and run the query
                            $query = "UPDATE manager_users SET numberOfLogins = '".$numberOfLogins."', dateCurrent = CURRENT_TIMESTAMP, lastOnline = CURRENT_TIMESTAMP WHERE userID = '".$userID."'";
                            $result = mysqli_query($dbc,$query);
                            
                            // See if the remember me checkbox was checked
                            if (isset($_POST['remember'])) {
                                
                                // Sets an expiration time for the cookie
                                $myExpiration = time()+60*60*24*100;
                                
                                // Sets the cookie for the username
                                setcookie("username", $username, $myExiration, "/");
                                
                            }

                            // Succesful login complete
                            $output = array('errorsExist' => false, 'message' => 'You have been logged in, please allow a moment while we load your account data!');
                        
                        } else {
                            
                            // Login unsuccessful
                            // Add to number of tries
                            $_SESSION['numberOfAttempts'] = $_SESSION['numberOfAttempts']+1;
                            
                            // Take numberOfAttempts and compare it 
                            if ($_SESSION['numberOfAttempts'] >= 5) {
                             
                                // Retrieve IP Address of user trying to hack into account
                                $hackerIPAddress = $_SERVER['REMOTE_ADDR'];
                                
                                // Assign query array values to variables
                                $userID = $row['userID'];
                                
                                // Update database after account getting hacked and run query
                                $query = "UPDATE manager_users SET lockDate = CURRENT_TIMESTAMP WHERE userID = '".$userID."'";
                                $result = mysqli_query($dbc,$query);
                                
                                // Insert hacker info into database for storage and run query
                                $query2 = "INSERT INTO manager_users_login_hacking (hackerIPAddress, userID, lockDate) VALUES ('".$hackerIPAddress."','".$userID."', CURRENT_TIMESTAMP)";
                                $result2 = mysqli_query($dbc,$query2);
                                
                                // Account locked error
                                $output = array('errorsExist' => true, 'message' => 'Your account is currently locked, we appologize for the inconvienence. This is a security messure implimented by to many failed login\'s! You must wait 10 minutes before you can login again!');         
                                
                                // Email user about locked account
                                function my_domain_name() {
                            		$my_domain = $_SERVER['HTTP_HOST'];
                            		$my_domain = str_replace('www.', '', $my_domain);
                            		return $my_domain;
                            	}
                                $sender_name = "Kansas Outlaw Wrestling";
                                $sender_email = "kowmanagement@kansasoutlawwrestling.com";
                                $recipient_email = $row[ 'emailAddress' ]; 
                                $email_subject = "KOW Manager Account Update";
                                $year = date(Y);
                        
                                $email_body = '
                                <table width="500" border="0" cellspacing="0" cellpadding="0">
                                  <tr>
                                    <td><img src="http://'.my_domain_name().'/images/logo.png" border="0" alt="LOGO" /></td>
                                  </tr>
                                  <tr>
                                    <td> 
                                        <table width="500" border="0" cellspacing="0" cellpadding="0">
                                          <tr>
                                            <td>Hello '.$firstName.' '.$lastName.' This email is to inform you that your account <a href=http://www.kansasoutlawwrestling.com/manager target=_blank>Kansas Outlaw Wrestling</a> was attempted to be hacked into. The user tried to login and failed. If this was you </td>
                                          </tr>
                                        </table>
                                    </td>
                                  </tr>
                                  <tr>
                                    <td>&copy; '.$year.' - '.my_domain_name().'</td>
                                  </tr>
                                </table>
                                ';
                                $mail_template = '
                        		<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN">
                        		<html>
                        		<head>
                        		<title>'.$email_subject.'</title>
                        		</head>
                        		<style type="text/css">
                        			body{background: #EDEBEA;}
                        			#wrapper{background:#FFF;border:4px solid #DDD;width:650px;}
                        		</style>
                        		</head>
                        		<body>
                        		<div id="wrapper">"'.$email_body.'</div>
                        		</body>
                        		</html>
                        		';
                                $headers  = 'MIME-Version: 1.0' . "\r\n";
                        		$headers .= 'Content-type: text/html; charset=iso-8859-1' . "\r\n";
                        		$headers .= 'Content-type: text/html; charset=us-ascii' . "\r\n";
                        		$headers .= 'From:' .$sender_name. "\r\n";
                        		$headers .= 'Reply-To: ' .$sender_name. "\r\n";
                        		$headers .= '1\r\nX-MSMail-Priority: High' . "\r\n";
                        		$headers .= 'X-Mailer: Kansas Outlaw Wrestling Mail Controller v1.0' . "\r\n";
                                mail($recipient_email, $email_subject, $email_body, $headers); 
                                
                            } else {
                                
                                // Calculate how many chances the user has to login before account gets locked
                                $chancesLeft = 5 - $_SESSION['numberOfAttempts'];
                                
                                // Invalid username and password error 
                                $output = array('errorsExist' => true, 'message' => 'Invalid Username and Password combination! You have ' .$chancesLeft.' chances left to login succesfully or the account will be locked!'); 
                                
                            }
                            
                        }
                    
                    }
                    
                }

            } else {
               
               // User doesn't exist in database error
               $output = array('errorsExist' => true, 'message' => 'Sorry we can\'t seem to find you in our system, please check your username and try again!'); 
                
            }
           

        }
        
	} else {
		
        // User alread logged in and reported session exists
        $output = array('errorsExist' => true, 'message' => 'Already logged in!');
        
	}
    
}

//Output the result
$output = json_encode($output);
echo $output;


?>
User avatar
social_experiment
DevNet Master
Posts: 2793
Joined: Sun Feb 15, 2009 11:08 am
Location: .za

Re: Login Form Critique

Post by social_experiment »

Code: Select all

# 66
  $username = mysqli_real_escape_string($row['username']);
  $query = "UPDATE manager_users SET '".$data."' WHERE username = '".$username."'";
This might give an error because mysqli_real_escape_string() requires a connection resource as its first argument. In other places in the script it is added so maybe you forgot to add it here (the 66 is the line number +/-).

Code: Select all

 
 $query = "SELECT * FROM manager_users WHERE username = '".$username."'";
 $result = mysqli_query($dbc,$query);
The script requires a username & password for successful login but i can't see where the password value is used in the script to facilitate login. Usernames are possibly stored in plain text so it's not very secure if the script uses only 1 value (and a plaintext one at that) for login when a possible hashed password is available.
“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