How can I return these errors?

[TeddyKiller]

I have a class.. which has a login (procLogin)

There are probably many mistakes in this class, although I'm learning.

 

What I need help with, is the errors. The errors are in an array.

$errors[]

I'd like them displayed in a certain place on the page. So what I'm kind of needing to do.. when if $errors then redirect back to $_SERVER['http_referer']

Although http_referer should not be used as it can be faked.. so whatever is best? Though on redirection back to a page.. it must put the errors over..

There is a little method I like to use to display the errors.

if($errors) {
    foreach($errors as $error) {
        echo $error;
    }
}

 

Hope you can help. Code is below.

Thanks

 

<?php
session_start();
include("functions.php");
include("config.php");

class authentication
{
    /* Class constructor */
    function Process(){
        if(isset($_POST['login'])){
            $this->procLogin();
        }
        elseif($_SESSION['logged_in']){
            $this->procLogout();
        } else {
            header("Location: main.php");
        }
    }

    public function procLogin(){
        $username = clean($_POST['username'],1,0,2);
        $password = clean($_POST['password'],1,0,0);
        
        $pwd = md5($username . $password);
    
        if(empty($username) || empty($password)){
            $errors[] = 'You have left empty fields. Please fill them in.';
        } else {
            $query = mysql_query("SELECT * FROM users WHERE username = '$username' AND password = '$pwd' LIMIT 1");
            $row = mysql_fetch_array($query);
            if(mysql_num_rows($query) == 0){
                   $errors[] = 'Your username or password is incorrect. Please try again.';
            } else {
                if($row['activated'] == 0){
                    $errors[] = 'Your account is not activated.';
                }
                
                //Check if the user is banned
                if($this->procBan($username) == true) {
                    $errors[] = 'You are banned until '.date("d/m/Y H:i:s", $row['ExpireDate']).'!';
                } else {
                    $query = mysql_query("DELETE FROM banned WHERE user_id = '".$r['id']."'");
                }

                if(!$errors) {
                    $used = $row['times_logged'] + 1;
                    $loggedn = time();
                    $query = mysql_query("UPDATE users SET times_logged='$used', last_login='$loggedn' WHERE username = '$username'");

                    $hash = sha1($row['id'] . $_SERVER['REMOTE_ADDR'] . $secret_key);
                    $_SESSION['uid'] = $row['id'];
                    $_SESSION['hash'] = $hash;
                    $_SESSION['logged_in'] = true;
        
                    if(isset($_POST['keep']) == checked){
                        $time = time() + 60*60*24*1000;
                        setcookie(HSC5739487932, $hash, $time); 
                    }
        
                    header("location: /main.php");
                }
            }
        }
    }
   
    private function procBan($username) {
        $query = "SELECT expires FROM banned WHERE user_id = '".$r['id']."'";
        $query = mysql_query($query);
    
        if(mysql_num_rows($query) > 0){
            $row = mysql_fetch_array($q);
            if($row['expires'] > time()){ 
                return true;
            } else { 
                return false;
            }
        }
   }
   
   public function procLogout(){
      session_unset();
      session_destroy();
      header("Location: /");
   }
   
};

/* Initialize process */
$authentication = new authentication;
?>

[TeddyKiller]

Slight change.. it works now, just need a way to handle the errors.

<?php
session_start();
include("functions.php");
include("config.php");

class authentication
{
/* Class constructor */
function Process(){
	if(isset($_POST['login'])){
		$this->procLogin();
	}
	elseif($_SESSION['logged_in']){
		$this->procLogout();
	} else {
		header("Location: /design");
	}
}

public function procLogin(){

	global $secret_key;

	$username = clean($_POST['username'],1,0,2);
	$password = clean($_POST['password'],1,0,0);

	$pwd = md5($username . $password);

	if(empty($username) || empty($password)){
		$errors[] = 'You have left empty fields. Please fill them in.';
	} else {
		$query = mysql_query("SELECT * FROM users WHERE username = '$username' AND password = '$pwd' LIMIT 1");
		$row = mysql_fetch_array($query);
		if(mysql_num_rows($query) == 0){
   			$error['error'] = 'Your username or password is incorrect. Please try again.';
		} else {
			if($row['activated'] == 0){
				$error['error'] = 'Your account is not activated.';
			}

			//Check if the user is banned
			if($this->procBan($username) == true) {
				$errors[] = 'You are banned until '.date("d/m/Y H:i:s", $row['ExpireDate']).'!';
			} else {
				$query = mysql_query("DELETE FROM banned WHERE user_id = '".$r['id']."'");
			}

			if(!$errors) {
				$used = $row['times_logged'] + 1;
				$loggedn = time();
        			$query = mysql_query("UPDATE users SET times_logged='$used', last_login='$loggedn' WHERE username = '$username'");

				$hash = sha1($row['id'] . $_SERVER['REMOTE_ADDR'] . $secret_key);
				$_SESSION['uid'] = $row['id'];
				$_SESSION['hash'] = $hash;
				$_SESSION['logged_in'] = true;

				if(isset($_POST['keep']) == checked){
					$time = time() + 60*60*24*1000;
					setcookie(HSC5739487932, $hash, $time); 
				}

				header("location: /design/main.php");
			}
		}
	}
}
   
private function procBan($username) {
	$query = "SELECT expires FROM banned WHERE user_id = '".$r['id']."'";
	$query = mysql_query($query);

	if(mysql_num_rows($query) > 0){
		$row = mysql_fetch_array($q);
		if($row['expires'] > time()){ 
			return true;
		} else { 
			return false;
		}
	}
   }
   
   public function procLogout(){
  session_unset();
      session_destroy();
      header("Location: /");
   }
   
};

/* Initialize process */
$auth = new authentication;
$auth->process();
?>

 

[trq]

Firstly, for good design's sake. Your class should not rely on all these $_POST variables existing. Any data required to be inputted into the class should be done by setting them during instantiation or by passing them into the relevant methods.

 

The way you have written that class makes it very inflexible.

 

As for your errors. I would likely move all those checks into there own methods as well. This way you would (in the client code) do something like....

 

$user = new Auth($_POST['username'], $_POST['password']);
if ($user->isValid()) {
  if ($user->isActive()) {
    if ($user->notBannded()) {
      // do login. This could likely be done with another object, but doesn't really belong in Auth.
    } else {
      echo "Your are banned";
    }
  } else {
    echo "Your account is not active";
  }
} else {
  echo "User does not exist";
}

[TeddyKiller]

Thanks for taking your time to help me.

Whats class like then..

class Auth 
{
    isValid() {
        //Check if login is valid..
    }

    isActive() {
        //Check if user is active
    }

    isBanned() {
        //Check if user is banned
    }
}

Though you have the $username and $password in the class name bit. How would I do the above.. with them?

[trq]

I'm not going to write the class for you but I will show you how to pass arguments into an objects construct.

 

class Auth 
{
    private $_username;
    private $_userpass;

    public function __construct($username, $password)
    {
        $this->_username = $username;
        $this->_userpass = $userpass;
    }

    public function isValid()
    {
        // You  now have $this->_username & $this->_userpass available to use in your checks.
        // Your method should then return true or false depending onb whether on not the user is valid.
    }
}

[TeddyKiller]

I put the handle_login inside auth for now.

Here is what I've got.

 

include("config.php");
include("functions.php");

class Auth 
{   
    private $_username;
    private $_userpass;
   
   public function __construct($username, $password) {
        $this->_username = $username;
        $this->_userpass = $userpass;
    }
   
   public function isFilled() {
      if(empty($this->_username) || empty($this->_userpass)) {
         return false;
      } else {
         if($this->_username == 'username' || $this->_userpass == 'password') {
            return false;
         } else {
            return true;
         }
      }
   }      
   
    public function isValid() {
        $query = mysql_query("SELECT * FROM users WHERE username = '$this->_username' AND password = '$this->_userpass' LIMIT 1");
      if(mysql_num_rows($query) == 1) {
         return true;
      } else {
         return false;
      }
    }

    public function isActive() {
      $user = $this->getUserInfo();
      if($user->activated == '1') {
         return true;
      } else {
         return false;
      }
    }

    public function isBanned() {
      $user = $this->getUserInfo();
        $query = mysql_query("SELECT expires FROM banned WHERE user_id = '".$user->id."'");
      if(mysql_num_rows($query) > 0) {
         return true;
      } else {
         return false;
      }
    }
   
   public function getUserInfo() {
      $query = mysql_query("SELECT * FROM users WHERE username = '$this->_username' AND password = '$this->_userpass' LIMIT 1");
      $row = mysql_fetch_assoc($query);
      
      foreach($row as $key => $value) {
         $user->$key = $value;
      }
      return $user;
   }
   
   public function handle_login() {
      global $secret_key;
      
      $hash = sha1($row['id'] . $_SERVER['REMOTE_ADDR'] . $secret_key);
      $_SESSION['uid'] = $row['id'];
      $_SESSION['hash'] = $hash;
      
      if(isset($_POST['keep']) == checked){   
         $time = time() + 60*60*24*1000;
         setcookie(HSC5739487932, $pwd, $time); 
      }
      
      header("location: /main.php");
   }
}

$user = new Auth(clean($_POST['username'],1,0,2), clean($_POST['password'],1,0,0));
//Are there empty fields?
if ($user->isFilled()) {
   //Are the details valid?
   if ($user->isValid()) {
      //Is the user active?
      if ($user->isActive()) {
         //Is the user banned?
         if ($user->notBanned()) {
             //Display the login
            $user->handle_login();
         } else { echo "Your are current banned"; }
      } else { echo "Your account is not active."; }
   } else { echo "Your username or password is incorrect."; }
} else { echo "You have left empty fields"; }

Added in a few bits here and there..

Haven't tested, it looks as if you've made a typo around here..

    private $_username;
    private $_userpass;
   
   public function __construct($username, $password) {

How I know, is that you use $_username then suddenly turns into $username.

the __construct function isn't being called also.

Is it suppose to be like this? or is that typo you made.

[mrMarcus]

you're misunderstanding how the __construct works.

 

he has declared two private variables to be used as objects throughout the script.

 

$username and $password inside the __construct method are incoming variables, and are then assigned to the two private variables.

 

i do see that:

 

$this->_userpass = $userpass;

 

should be changed to:

 

$this->_userpass = $password;

 

merely a typo.

 

and __construct is called as soon as the class is instantiated.  it is not called like any other function would normally be called.

 

EDIT:  as a pointer for your other question/concern about displaying errors throughout the page.  instead of:

 

$error[] = 'Please enter your first name';

 

or something like that, do this:

 

$error['first_name'] = 'Please enter your first name.';

 

then you can check if these errors are set much easier throughout the script.

[TeddyKiller]

Ok. I made that change. Now the only problem is now.. it states the username or password is incorrect..

I added in the md5 bit.. and it still came as incorrect. I'm really confused?

 

Got any ideas?

    public function isValid() {
        $query = mysql_query("SELECT * FROM users WHERE username = '".$this->_username."' AND password = '".md5($this->_userpass)."' LIMIT 1");
      if(mysql_num_rows($query) == 1) {
         return true;
      } else {
         return false;
      }
    }

 

EDIT: False alarm.. forgot that the password in the database is username + password

 

For isBanned.. if on return false.. what would be the best way to get the results of

$query = mysql_query("SELECT expires FROM banned WHERE user_id = '".$userauth->id."'");

back. So.. it'll be $row. I cannot do return $row; because that'll class as return true.. right?

[mrMarcus]

it depends on what $row is.

 

if $row is:

 

$row = mysql_fetch_array($query);

 

then yes, you can return $row and have access to any available recordset.

[TeddyKiller]

So for this..

    public function notBanned() {
	$userauth = $this->getUserInfo();
        $query = mysql_query("SELECT expires FROM banned WHERE user_id = '".$userauth->id."'");
	if(mysql_num_rows($query) == 0) {
		return true;
	} else {
		$row = mysql_fetch_assoc($query);
		return $row;
	}
    }

return $row; will act as return false; ?

[mrMarcus]

mysql_fetch_assoc will return false only if there are no rows to be displayed as per the query (or $row in this case).  but if there are rows returned, then $row will be an array of those records.

 

can you please, and simply, explain in one (two max) sentences what it is you are trying to achieve.  i fear you are going to over-complicate things.

[TeddyKiller]

    public function notBanned() {
	$userauth = $this->getUserInfo();
        $query = mysql_query("SELECT expires FROM banned WHERE user_id = '".$userauth->id."'");
	if(mysql_num_rows($query) == 0) {
		return true;
	} else {
		return false
	}
    }

This bit works as it is.

It returns that the user is banned (if rows are found)

} else { $reply = "Your are currently banned"; }

Though I'm wanting the reply to be something like..

$reply = 'You are banned until '.date("d/m/Y H:i:s", $row['ExpireDate']).'!';

Though if the only possible way of doing this.. is adding in an extra two lines to the else... then I guess it's what I have to do.

 

I often do over complicate things. Either that.. if I have a bad way of understanding.

[TeddyKiller]

Ok.. how would I do a similar thing for a registration form.. They'll be alot of privates wouldn't there? How can this be fixed?