My login script

PHP programming forum. Ask questions or help people concerning PHP code. Don't understand a function? Need help implementing a class? Don't understand a class? Here is where to ask. Remember to do your homework!

Moderator: General Moderators

Post Reply
Roxon
Forum Newbie
Posts: 3
Joined: Sat Sep 05, 2009 1:24 pm

My login script

Post by Roxon »

Ok so I'm pretty new at PHP, I know my stuff regarding a few other language which is probably the only reason I made it this far. I started creating a login script a couple of days ago and it worked/did everything I expected it to do, but since I've managed to break it and I have no idea what I've done to cause this so I'm admitting defeat and asking for help. It will go as far to process/verify the log in and create the session, but right at the end the session is called again and then all the session variables are wiped causing it to forget everything. Oh and the html is ripped from a tutorial site as I didn't really need to practice creating that.

Index.php

Code: Select all

<?php
include("include/session.php");
session_start();
?>
<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8" />
<title>Test</title>
<style type="text/css" media="all">@import "css/master.css";</style>
</head>
 
<body>
 
 
<div id="page-container">
 
    <div id="main-nav">
    </div>
 
 
    
    <div id="header">
    <h1><img src="images/general/logo_enlighten.gif" 
        width="236" height="36" alt="Enlighten Designs" border="0" /></h1>
    </div>
    
    <div id="sidebar-a">
    <div class="padding">
 
 
<?php
      
   //If logged in show to user   
   if($session->logged_in == true){
       
       echo "<h1>Logged In</h1>";
       echo "Welcome <b>"+ $session->username +"</b>, you are logged in. <br><br>";
       echo "<tr><input type=\"submit\" value=\"Logout\"></tr>";
       echo "<input type=\"hidden\" name=\"attemptlogout\" value\"1\">";
   }
   
   //User not logged in
   else{
 
       echo "Login";
       echo"<br/>";
    
       echo "<form action=\"processLogin.php\" method=\"POST\">";
       echo "<tr><td>Username:</td><td><input type=\"text\" name=\"user\" maxlength=\"20\" value=\"\"></td></tr>";
       echo "<tr><td>Password:</td><td><input type=\"password\" name=\"pass\" maxlength=\"20\" value\"\"></td></tr>";
       echo "<tr><input type=\"submit\" value=\"Login\"></tr>";
       echo "<input type=\"hidden\" name=\"attemptlogin\" value\"1\">";
       echo "</form>";
   }
?>
    
    </div>
    </div>
    
    <div id="content">
    <div class="padding">
    <h2><img src="images/headers/about.gif" width="54" height="14" alt="About" /></h2>
    <p><strong>Enlighten Designs</strong> is an Internet solutions provider that specialises in 
       front and back end development. To view some of the web sites we have created view our 
       portfolio.</p>
    <p>We are currently undergoing a 'face lift', so if you have any questions or would 
       like more information about the services we provide please feel free to contact us.</p>
    
    <h2><img src="images/headers/contact.gif" width="98" height="14" alt="Contact Us" /></h2>
    <p>Phone:   (07) 853 6060<br />
    Fax:     (07) 853 6060<br />
    Email:   <a href="mailto:info@enlighten.co.nz">info@enlighten.co.nz</a><br />
    P.O Box: 14159, Hamilton, New Zealand</p>
    <p><a href="#">More contact information...</a></p>
    </div>
    </div>
    
    <div id="footer">
        <div id="altnav">
        <a href="#">About</a> - 
        <a href="#">Services</a> - 
        <a href="#">Portfolio</a> - 
        <a href="#">Contact Us</a> - 
        <a href="#">Terms of Trade</a>
        </div>
    
    Copyright &copy; Enlighten Designs
 
    Powered by <a href="http://www.enlightenhosting.com/">Enlighten Hosting</a> and
    <a href="http://www.vadmin.co.nz/">Vadmin 3.0 CMS</a>
    </div>
 
</div>
 
</body>
</html>

ProcessLogin.php

Code: Select all

<?php
/*
*prcoessLogin
*process' login/logout functions
*/
include("include/constants.php");
include("include/session.php");
 
class processLogin{
 
 var $connection;
 
 /**
    * Handles input from form
    * 
    */
 function processLogin(){
  
      //Points to necessary function depending on user action
      if(isset($_POST['attemptlogin'])){
         $sucess = $this->login($_POST['user'], $_POST['pass']);
        
      }
      else if($_POST['attemptlogout']){
         $sucess = $session->Logout();
      }
 
      //Redirects to index if sucessful (will later be dynamic to point at referal page)
      if($sucess){            
      header("Location: http://localhost/cms/index.php");
      }
 }
 
  /**
    * Confirms login details and proccess' if correct
    * 
    */
 function login($subuser, $subpass){
 
     global $session;
 
 
     //Creates connection and points to database
     $this->connection = mysql_connect(DB_SERVER, DB_USER, DB_PASS) or die(mysql_error());
     mysql_select_db(DB_NAME, $this->connection) or die(mysql_error());
 
     //make safe the input
     $subuser = $this->makeSafe($subuser);
     $subpass = $this->makeSafe($subpass);
 
     //query to select user from database which is used to populate array
     $q = "SELECT * FROM ".UserTable." WHERE Username = '$subuser'";
     $result = mysql_query($q, $this->connection);
    
     //Username does not exist
     if(!$result || (mysql_numrows($result) < 1)){
        return NULL;
     }
 
      //prepares posted password for comparison
      $dbarray = mysql_fetch_array($result);     
      $subpass = md5($subpass);
      
      //if password not correct
      if($dbarray['Password'] != $subpass){
       return NULL;
      }
 
      //Set up session with uset details
      $session->setSession($dbarray);
      
      return true;
   }
 
    /**
    * Cleans and inputted string for potential SQL injection attacks
    * 
    */
   function makeSafe($input){
 
       $input = addslashes($input);
       $input = mysql_real_escape_string($input);
 
       return $input;
   }
   
 }
 $processLogin = new processLogin;
?>
Session.php

Code: Select all

<?php
/*
*session.php
*
*Creates sessions for logged in users
*/
 
include("constants.php");
 
class Session
{
   var $username;              //Username given on sign-up
   var $logged_in = False;    //True if user is logged in, false otherwise
   var $userinfo = array();  //The array holding all user info
   var $referrer;           //Defines refferer page to point back to
   var $SessionID;         //Session ID to confirm user is authentic
   var $connection;       //Defines connection used for database entry
 
 
   /* Class constructor */
   function Session(){
       
      $this->startSession();
   
      $this->connection = mysql_connect(DB_SERVER, DB_USER, DB_PASS) or die(mysql_error());
      mysql_select_db(DB_NAME, $this->connection) or die(mysql_error());
   }
 
   /**
    * Starts session and handles various tasks such as checking the login
    * status of the user 
    */
     
   function startSession(){
       session_start();   
   
      // Determines login status 
       if(isset($_Session['Username'])){
         $this->logged_in = $this->checkLogin();
       }      
             
      //Sets referrer page
       if(isset($_SESSION['url'])){
         $this->referrer = $_SESSION['url'];
       }else{
         $this->referrer = "/";
       }
 
      //sets url
      $this->url = $_SESSION['url'] = $_SERVER['PHP_SELF'];
   }
 
   //Sets up session on log in
   function setSession($userArray){ 
   
      //stores userArray to be ussed Sessionwide 
      $userinfo  = $userArray;
      
      //Stores info needed for session recognition
      $username  = $_SESSION['Username'] = $userinfo['Username'];
      $SessionID    = $_SESSION['SessionID']   = $this->generateHash();
      $this->updateUserField($username, "SessionID", $SessionID);
 
      //Checks if logged in and then sets flag
      $this->logged_in = $this->checkLogin();
   
      return;
   }
 
   /**
    * Checks login status
    * */
   function checkLogin(){
         
   
      // Username and userid is set
      if(isset($_SESSION['Username']) && isset($_SESSION['SessionID'])){
        
         //Checks if session matches user 
         if($this->confirmUserID($_SESSION['Username'], $_SESSION['SessionID']) != 0){
 
            //User not logged in
            unset($_SESSION['Username']);
            unset($_SESSION['SessionID']);
            return false;
         }
 
         return true;
      }
      //Not logged in
      else{
         return false;
      }
   }
 
 
   /**
    *  Logs out user
    */
   function logout(){
      global $database;  //The database connection
 
      //Unsets session
      unset($_SESSION['Username']);
      unset($_SESSION['SessionID']);
      //Destroy session?
 
      //set login flaf to false
      $this->logged_in = false;
   }
 
 
   /**
    * Generates random hash
    */
   function generateHash(){
      return md5($this->generateStr(16));
   }
 
   /**
    * Generates random string 
    * Seperate for possible future creation of automated mailing
    */
   function generateStr($length){
      $randstr = "";
      for($i=0; $i<$length; $i++){
         $randnum = mt_rand(0,61);
         if($randnum < 10){
            $randstr .= chr($randnum+48);
         }else if($randnum < 36){
            $randstr .= chr($randnum+55);
         }else{
            $randstr .= chr($randnum+61);
         }
      }
      return $randstr;
   }
 
 
   /**
    * Compares session to DB
    * 
    */
   function confirmUserID($username, $SessionID){
     
     //Add lashes for DB comparison  
     $username = addslashes($username);
     
      //Checks user exists
     $q = "SELECT * FROM users WHERE Username = '$username'";
     $result = mysql_query($q, $this->connection);
     $dbarray = mysql_fetch_array($result);
 
     //If user does not exist
     if(!$result || (mysql_numrows($result) < 1)){
 
         return 1; 
      }
 
      //Extract Session ID from DB results
      $dbarray['SessionID'] = stripslashes($dbarray['SessionID']);
      $SessionID = stripslashes($SessionID);
      
      //Validates SessionID
      if($SessionID == $dbarray['SessionID']){
         return 0; 
      }
      else{
         return 2; 
      }
   }
   
   /**
    * Update field in the user table
    * 
    */
    function updateUserField($username, $field, $value){
      $q = "UPDATE users SET ".$field." = '$value' WHERE username = '$username'";
      
      return mysql_query($q, $this->connection);
   }
 
}
 
/**
 * Initialise session
 */
$session = new Session;
 
?>
Any help at all would be appreciated, and any tips regarding security, efficiency etc as I'm pretty new to this and have a lot to learn!
Last edited by Roxon on Sun Sep 06, 2009 10:12 am, edited 1 time in total.
Roxon
Forum Newbie
Posts: 3
Joined: Sat Sep 05, 2009 1:24 pm

Re: My login script

Post by Roxon »

No one? Any help at all would be appreciated. I'm pretty sure the session is being wiped when the processLogin page redirects the user to index and the session is then called again.
peterjwest
Forum Commoner
Posts: 63
Joined: Tue Aug 04, 2009 1:06 pm

Re: My login script

Post by peterjwest »

Had a look through your code, can't see anything in particular wrong with it.

Make sure you have error reporting on: http://www.bradino.com/php/error-reporting/ and check for php errors. Then echo all of your MYSQL queries to check they are behaving correctly. Print out your arrays (e.g. $_SESSION) and disable the redirect so that you can see what's going on.

In terms of tips, I would suggest merging your two session/login classes, they are effectively doing the same thing. You may want to encapsulate the database behaviour into a separate class and then pass that into the session object.

e.g
$database = new DatabaseHandler($dbUser,$dbPass,$etc);
$session = new Session($database);
$session->login($_POST);

Another point is you don't need to use a global here. Globals and statics should be used sparingly, its completely uneccessary here, you can just create an instance of session wherever you need it.

Be more strict with your indentation, its useful. And finally: COMMENT LESS. All of your code is completely obvious to an average php developer. You only really need to comment things which a programmer might find obscure or confusing. I see no reason for you to keep any of your current comments.

As an example here's one of your classes without the faff:

Code: Select all

 
class processLogin {
    var $connection;
 
    function processLogin(){
        if(isset($_POST['attemptlogin']))
            $sucess = $this->login($_POST['user'], $_POST['pass']);
        else if($_POST['attemptlogout'])
            $sucess = $session->Logout();
        if($sucess)          
            header("Location: http://localhost/cms/index.php");
    }
 
    function login($subuser, $subpass){
        global $session;
        $this->connection = mysql_connect(DB_SERVER, DB_USER, DB_PASS) or die(mysql_error());
        mysql_select_db(DB_NAME, $this->connection) or die(mysql_error());
        $subuser = $this->makeSafe($subuser);
        $subpass = $this->makeSafe($subpass);
        $q = "SELECT * FROM ".UserTable." WHERE Username = '$subuser'";
        $result = mysql_query($q, $this->connection);
        if(!$result || (mysql_numrows($result) < 1)) return;
        $dbarray = mysql_fetch_array($result);    
        $subpass = md5($subpass);
        if($dbarray['Password'] != $subpass) return;
        $session->setSession($dbarray); 
        return true;
    }
 
    function makeSafe($input){
        return mysql_real_escape_string(addslashes($input));
    }
}
 
I could have made it smaller if I had time. I would suggest reducing temporary variables [see makeSafe()] but keep it readable!
User avatar
Ollie Saunders
DevNet Master
Posts: 3179
Joined: Tue May 24, 2005 6:01 pm
Location: UK

Re: My login script

Post by Ollie Saunders »

To encourage others forum contributors advice change your "code" tags to "php" tags in the bbcode of your post.

Finding the fault with your code is difficult largely because of the way you've written it. The best way to tackle this problem is probably to go about improving your design and style until it becomes manageable enough that the error is evident. I can't fault any of peterjwest's advice, do what he says.

The other thing that would help a lot is test coverage. Besides that there are probably 20 (no exaggeration) little stylistic improvements I could suggest. Let me know if you're interested in hearing them.

The only functional error that was obvious to me:

Code: Select all

     $input = addslashes($input);
      $input = mysql_real_escape_string($input);
 
You only need one of these, the second one. Otherwise you're double escaping.
Roxon
Forum Newbie
Posts: 3
Joined: Sat Sep 05, 2009 1:24 pm

Re: My login script

Post by Roxon »

Thanks, that's sound advice from both of you. I realise the quality of my code isn't the best right now, I tend to go for something that works and clean up later.

I found the error:

Code: Select all

if($session->logged_in == true){
changed to:

Code: Select all

if($session->checkLogin()){
But yea if your willing to make suggestions please do if you have the time.

Thanks again!
User avatar
jackpf
DevNet Resident
Posts: 2119
Joined: Sun Feb 15, 2009 7:22 pm
Location: Ipswich, UK

Re: My login script

Post by jackpf »

Also, I noticed that you're using stripslashes() on data from the database. You must be escaping it twice.
User avatar
Ollie Saunders
DevNet Master
Posts: 3179
Joined: Tue May 24, 2005 6:01 pm
Location: UK

Re: My login script

Post by Ollie Saunders »

But yea if your willing to make suggestions please do if you have the time.
Actually to fully explain each of the points would take some time even if we were sound side-by-side in front of the computer and should be done in a teaching style, not a critical one. If I'm to adopt a critical stance I prefer to be criticising things that are obviously wrong, not things that aren't quite as good. Otherwise it sounds like I'm just showcasing why I'm better than you, which is just mean.

I would love to teach it properly but it takes too much time. If you were here in person I would make the effort but to type it is an impractical ask.

It's a shame because I've spent over an hour writing what would have been a post.
Post Reply