BetterPHP

You have ob_start(); in the init file and the login page file. one of those should be removed (hit remove the login page one)

In the add_user function, you should not be using all of those $_POST variables directly, they should be passed as function arguments, the way you have it limits the use of that function to the specific case. Also you need to escape them all as you do with the $user variable to prevent SQL injection.

You need to create a function to check if the account is active, something like this...

// checks if the given username and password is valid
function account_active($user){
        $user = mysql_real_escape_string($user);
       
        $total = mysql_query("SELECT COUNT(`acc_id`) FROM `account` WHERE `acc_email` = '{$user}' AND `acc_active` = 1");
       
        return (mysql_result($total, 0) == '1') ? true : false;
}

If that returns true it means the account is active. this check should be performed only id the credentials have been accepted to cut down on queries, so

        if (valid_credentials($_POST['username'], $_POST['password']) === false){
                $errors[] = 'Email or password is incorect.';
        }else if (is_active($_POST['username']) === false){
             $errors[] = 'Your account is not active.';
        }

Start with that and see how far you get.

$total = mysql_query("SELECT COUNT(`acc_admin`) FROM `account` WHERE `acc_email` = '{$user}'");

I believe that will always return 1?

Maybe store the value as a Boolean (1 or 0, 1 for if true, 0 if false) and then simply remove COUNT() and it should work

SELECT COUNT(`acc_admin`) FROM `account` WHERE `acc_email` = '{$user}'

Should be

SELECT `acc_admin` FROM `account` WHERE `acc_email` = '{$user}'

And similar for the other one, if you use a value of 1 and 0.

        if (valid_credentials($_POST['username'], $_POST['password']) === false){
                $errors[] = 'Email or password is incorect.';
        }
       
        if (active($_POST['username'] == true)){
                $errors[] = 'Your account is not active yet.'; 
        }

Should be

        if (valid_credentials($_POST['username'], $_POST['password']) === false){
                $errors[] = 'Email or password is incorect.';
        }else if (active($_POST['username'] == true)){
                $errors[] = 'Your account is not active yet.'; 
        }

so you check to make sure the user exists before checking if their account is active.

I think you have the condition backwards too

return (mysql_result($total, 0) == '1') ? false : true;

is the same as

if (mysql_result($result, 0) == '1'){
    return false;
}else{
    return true;
}

so you will get false if the column is set to 1, generally 1 means true.

You misplaced a bracket on this line

}else if (active($_POST['username']) == false){

that could be the problem

You are still returning true if the user is not an admin and not active too by the looks of it.

uhshosting wrote:does the above == true mean if it turns out true it will create the error?

It does, but because your active() function actually returns false if the account is active it would give the desired effect. However you should make the active function return true if the account is active not false.

you can try adding a

var_dump(active($_POST['username']));

somewhere to see what value you are actually getting.

        if (empty($errors)){
                $_SESSION['username'] = htmlentities($_POST['username']

this has a missing closing }

SELECT `acc_active` FROM `account` WHERE `acc_active` = '{$user}'

the column in the WHERE should probably be acc_username or something.

uhshosting wrote:ok i corrected the issues you said. still no luck, no changes. just to make sure should this be a boolean? because i thats what i had a few days ago but changed it to INT? its INT right now.

}
function active($user){
	$user = mysql_real_escape_string($user);
	
	$total = mysql_query("SELECT `acc_active` FROM `account` WHERE `acc_email` = '{$user}'");
	
	
	return (mysql_result($total, 0) == '1') ? true : false;
}
function admin($user){
	$user = mysql_real_escape_string($user);
	
	$total = mysql_query("SELECT `acc_admin` FROM `account` WHERE `acc_email` = '{$user}'");
	
	
	return (mysql_result($total, 0) == '1') ? true : false;
	
}

<?php
include('init.inc.php');
    var_dump(active($_POST['username']));
$errors = array();

if (isset($_POST['username'], $_POST['password'])){
	if (empty($_POST['username'])){
		$errors[] = 'You must have an email.';	
	}
	
	if (empty($_POST['password'])){
		$errors[] = 'the password cannot be empty.';
	}
	
	if (valid_credentials($_POST['username'], $_POST['password']) === false){
		$errors[] = 'Email or password is incorect.';
	}else if (active($_POST['username']) == true){
		$errors[] = 'Your account is not active yet.';	
	}
	
	
	if (empty($errors)){
		$_SESSION['username'] = htmlentities($_POST['username']);
	}
	
	if (admin($_POST['username'])){
		 header('location: admin/');	
	}
	else
		header('location: user/');
		die();
	}

Is your $user variable containing the users email address or the username?

Because "where acc_email = username" won't work very well. You will have to either change acc_email to acc_username or change the content of the $user variable to an email address.

uhshosting wrote:just to make sure should this be a boolean? because i thats what i had a few days ago but changed it to INT? its INT right now.

should what be a boolean The column should be INT(1) or BOOL.

Temor wrote:Is your $user variable containing the users email address or the username?

It contains the email address, as $_POST['email'] is passed to the function. the variable names used don't help really.

        if (empty($errors)){
                $_SESSION['email'] = htmlentities($_POST['email']);
        }
       
        if (admin($_POST['email'])){
                 header('location: admin/');   
        }
        else
                header('location: user/');
                die();
        }

Here, you seem to redirect all of the time, you should only redirect if no errors have happened. you also need to die() after both header lines (or after the if else statement which would have the same effect.

Here is that block of code with consistent indenting

if (isset($_POST['email'], $_POST['password'])){
	if (empty($_POST['email'])){
		$errors[] = 'You must have an email.';	
	}
	
	if (empty($_POST['password'])){
		$errors[] = 'the password cannot be empty.';
	}
	
	if (valid_credentials($_POST['email'], $_POST['password']) === false){
		$errors[] = 'Email or password is incorect.';
	}else if (active($_POST['email']) == true){
		$errors[] = 'Your account is not active yet.';	
	}
	
	if (empty($errors)){
		$_SESSION['email'] = htmlentities($_POST['email']);
	}
	
	if (admin($_POST['email'])){
		header('location: admin/');	
	}else
		header('location: user/');
	
	die();
}

Or am I missing something ?

jacek wrote:

uhshosting wrote:just to make sure should this be a boolean? because i thats what i had a few days ago but changed it to INT? its INT right now.

should what be a boolean The column should be INT(1) or BOOL.

Temor wrote:Is your $user variable containing the users email address or the username?

It contains the email address, as $_POST['email'] is passed to the function. the variable names used don't help really.

        if (empty($errors)){
                $_SESSION['email'] = htmlentities($_POST['email']);
        }
       
        if (admin($_POST['email'])){
                 header('location: admin/');   
        }
        else
                header('location: user/');
                die();
        }

Here, you seem to redirect all of the time, you should only redirect if no errors have happened. you also need to die() after both header lines (or after the if else statement which would have the same effect.

In the one I quoted he passed $_POST['username'].

    
}else if (active($_POST['username']) == true){
                $errors[] = 'Your account is not active yet.'; 
        }
       
       
        if (empty($errors)){
                $_SESSION['username'] = htmlentities($_POST['username']);
        }
       
        if (admin($_POST['username'])){

Passing the username into a function that will try and match it with an email address won't get you far.

I see that in the post after mine he changed the $_POST value to email.

It makes more sense to have the function return true or false, you could have

return (bool)$admin;

you also need to fetch the result of the query.

return mysql_result((bool)$admin);

This line should be given you an error, if it is not you need to enable error_reporting.

Also, what you are doing here is performing the query, then destroying the result of the query by casting it to a bool. And finally trying to fetch the result of something which is not a query result anymore.

if (admin($admin == 1)){

You have a ) in the wrong place on this line.