Admin Login Function with custom anti-spam

I made this admin login script a while ago. It works fine in terms of checking the database for username and password and logs me in. What I don't quite understand is how to secure my login once in.

Currently using:

-
Cookie and Session check to show admin pages

if(!isset($_COOKIE['Cckiuas']) && !isset($_SESSION["admin"]) &&
!isset($_SESSION["session"]) && !isset($_SESSION["active"])){ 
     dont show page 
}
else {
    show page
}

If I create a cookie in my browser with the same name with any value in it, it lets me in.

What would be the best way to check if I'm "logged in" or not?

Feel free to suggest improvements, I'm guessing there will be a lot.

Login.php

```

WebCodex - Please Login




">





Please verify you are Human

Using only numbers, what is plus ?  

filter($sum1 + $sum2); ?>" />



filter($_SESSION['errors']);
$error = array_pop($error);
foreach($error as $errors) {
echo ''.$errors.'';
}
$_SESSION['errors'] = '';
}
elseif(isset($_SESSION['errors'])) {
echo ''.$_SESSION['errors'].'';
$_SESSION['errors'] = '';
}
?>

filter($_POST['username']);
$password = $database->filter($_POST['password']);
$securityA = $database->filter($_POST['securityA']);
$sum = $database->filter($_POST['sum']);

$login = $backend->loginCheck($username, $password, $securityA, $sum);

// IF loginCheck returns True
if ($login === true) {
$active = '1';
$lastlogin = date('d / m / y - H:ia');
$session = uniqid(mt_rand(), true);
$update = array('session' => $session, 'lastlogin' => $lastlogin, 'active' => $active);
$where = array('username' => $username, 'password' => $password);
$database->update('wcx_admin', $update, $where, 1);

// REGISTER SESSIONS
$_SESSION["admin"] = $username; // Bad Idea?
$_SESSION["active"] = $active;
$_SESSION["session"] = $session;

Basic login idea

The easiest way is to set a username and password cookie and to check each time if the details are valid. This is, however, not very safe, as one could simply set their cookies as the password's hash and be logged in, without needing to know the clear text password. It's definitely better than just checking for the existence of a cookie, which should be avoided even in trivial scripts.

You can get a quick improvement in security by creating a random ID upon login, storing it and then setting a $_SESSION or cookie parameter to that secret value. It's not bank-grade safety and this will restrict your users to only be logged in at one place only (since logging in a second time will generate a new ID -> old one is not valid anymore).

My advice at this point would be: think big, but do little steps. You probably want to use an iterative process for the development of your script, i.e. start with something basic, make it work, and then add to it bit by bit.

Here are a few concerns I have.
The admin check page

if(!isset($_COOKIE['Cckiuas']) && !isset($_SESSION["admin"]) && 
   !isset($_SESSION["session"]) && !isset($_SESSION["active"])){

You are doing four negative checks and connecting them with && (i.e. the AND operator). That means as soon as the cookie is set, or any one of the session parameters, the admin page will be shown. Did you perhaps mean to use ||?
SQL Injection

$query = "SELECT username, password, salt FROM wcx_admin WHERE username='$username'";

You're adding variables directly into SQL code, which is a big no-no. If you're using MySQLi to do your SQL things, then mysqli_real_escape_string() is your best friend in that context.
Fetching user data from database

$result = $this->get_results($query);
foreach( $result as $row ) {
    $salt = $this->filter($row['salt']);
    $dbpass = $this->filter($row['password']);
}
    $password = hash("sha512", $password . $salt);

(Awkward indentation on the last line), but the foreach() puzzles me here. You probably just want to do

$result = $this->get_results($query);
if(isset($result[0])) {
    $salt = $result[0]['salt'];
    $dbpass = $result[0]['password'];
}
else {  /* Handle login error */ }

Best would be to fetch the row you need from wcx_admin once, and only once you're sure you'll need the data (i.e. the user has supplied a user name and password, and he has positively solved the anti-spam problem).
Output all errors and warnings

At least during development, I strongly suggest you use error_reporting(E_ALL) so that you can be informed about certain inconsistencies. For example, you set $error = '' at the top of loginCheck, but then you add to $error as if it were an array, e.g. $error[] = 'Please enter a Username';. You probably mean to do $error = array();
Flow

• I would check for the validity of the user input password before fetching things from the database. I find your check: if (!$password) to come at a very late time.

• Some people are against using more than one return statement, but I personally like to get out of a function as soon as possible. Note that even when you add "Incorrect Username or Password" to your errors, you will still fetch for the user name and password and validate the password. If an error occurs, you want to stop checking things.

• You're fetching twice the same data from wcx_admin.

• The function loginCheck only has one return true statement. I would suggest return false in the other cases for consistency.

Conceptual questions

• I'm not really sure if the error of "You are already Logged in!" makes sense. What if your database says I'm logged on but I somehow am not logged in anymore – will I just be blocked? What if I switch to another device (or browser)?

• I would suggest separating your check for $sum into another function. This way, you can focus in loginCheck on the login mechanism only and handle the sum elsewhere. This makes it separate and your functions a little slimmer.

• You're using your filter method on data fetched from the database but not on $query (as mentioned above)? I think it should be the other way around.

Good points

Good on you for using salts — clap clap! Also happy to see that you're not giving a user information whether an account exists or not, but only an "incorrect username or password" error for all cases. That's nice.

StackExchange Code Review Q#55487, answer score: 2