Login system security

Question

Here is a login system I am working on. I just want an opinion on it and whether I am going in the right direction or am just completely missing something.

The main thing I would like to know is: am I initializing my user session IDs correctly? Basically once the user enters their credentials, the system goes into the database to see if there is a match. If there is, then it adds the session_id variable if it's not already set.

Now if it is set, then it checks your value against the one in the database. If they match, then you are entered into the system and then the session_ids are changed on both sides (server/client), otherwise it returns false. Is this a correct or incorrect method?

function userIsLoggedIn()
{
if ($_SERVER["REQUEST_METHOD"] == "POST") {
  if (isset($_POST['action']) and $_POST['action'] == 'login') {
  if (!isset($_POST['user']) or $_POST['user'] == '' or !isset($_POST['password']) or $_POST['password'] == '') {
        return FALSE;
  }

  array_push($_SESSION['mintime'], time());
  array_shift($_SESSION['mintime']);

  // minimum time (in seconds) between valid form submissions
  if ($_SESSION['mintime'][1] - $_SESSION['mintime'][0] < 2) {

        echo "to quick";
        return FALSE;

  } else {

        $user = test_input($_POST['user']);

        if ($GLOBALS['adminPage'] == 1 and $user != 'admin') {
        return FALSE;
        }

        $passtemp = test_input($_POST['password']);

        $password = hash('sha512', $passtemp . $user);

        if ($user == 'admin') {
        $GLOBALS["isAdmin"] = TRUE;
        }

        if (databaseContainsUser($user, $password)) {            

            if (!isset($_SESSION['session_id'])) {
            setSessionID($user);
        $_SESSION['loggedIn'] = TRUE;
        $_SESSION['user']     = $user;
        $_SESSION['password'] = $password;
        return TRUE;
        }
        if ((isset($_SESSION['session_id'])) and (checkSessionID($user))) {

        updateSessionID($user);
        setSessionID($user);
        $_SESSION['loggedIn'] = TRUE;
        $_SESSION['user']     = $user;
        $_SESSION['password'] = $password;
        return TRUE;
        } else {
                unset($_SESSION['session_id']);
            unset($_SESSION['loggedIn']);
                unset($_SESSION['user']);
                unset($_SESSION['password']);
                return FALSE;
        }    

        } else {
        unset($_SESSION['loggedIn']);
        unset($_SESSION['user']);
        unset($_SESSION['password']);
        return FALSE;
        }
  } //end else time check
  } //end if login check
  if (isset($_POST['action']) and $_POST['action'] == 'logout') {
  unset($_SESSION['loggedIn']);
  unset($_SESSION['user']);
  unset($_SESSION['password']);
  header('Location: ' . $_POST['goto']);
  exit();
  }

  if (isset($_POST['action']) and $_POST['action'] == 'admin') {
  header('Location: ' . $_POST['gotoadmin']); // gotoadmin = '..'
  exit();
  }
 } //end if POST check

if (isset($_SESSION['loggedIn'])) {
  return databaseContainsUser($_SESSION['user'], $_SESSION['password']);
}

} //end function

//validate input
function test_input($data)
{
$data = trim($data);
$data = stripslashes($data);
$data = htmlspecialchars($data, ENT_QUOTES, 'UTF-8');
return $data;
}

I store all passwords like this:

sha($password + salt, 512)

and the session IDs are made like this:

UUID()

Answer 1

First off, when posting a question, try to give it some natural IDE formatting if you can. this improves readability and you're more likely to get more/better answers!

Code

Technically, you can call

$_SERVER["REQUEST_METHOD"] == "POST"

however, my personal preference is just

isset($_POST)

I find that's it's easier to understand and is more intuitive.

This first two conditions encompass basically the entire function. Why not just reduce them to be negative and return false?

Such as

if (!isset($_POST['action']) or $_POST['action'] != 'login') {
    return FALSE;
}

It's reduces unnecessary nesting. Apply the same for if ($_SERVER["REQUEST_METHOD"] == "POST").

---

if (!isset($_POST['user']) or $_POST['user'] == '' or !isset($_POST['password']) or $_POST['password'] == '')

could be simplified to

if (!isset($_POST['user'], $_POST['password']) or empty($_POST['user']) or empty($_POST['password']))

It's just easier to read.

---

In your minimum time check between form submissions, you've added an else, but because the initial condition would return false, an else is completely unnecessary. It also nests your code one more layer, which isn't too great if it can be avoided.

---

$user = test_input($_POST['user']);

The variable here is very ambiguous. I suggest calling something more appropriate such as $username or something that describes what the value is.

---

hash('sha512', $passtemp . $user);

This not as secure as it could be! First off, if your PHP version supports it, I suggest you use bcrypt instead of hash. It take scares of the salting for you, so that you don't use bad salts such as you have. The salt should typically be a long string composed of random characters. Not just a username.

---

if ((isset($_SESSION['session_id'])) and (checkSessionID($user)))

Why so many parentheses! It just makes things unclear!

---

$_SESSION['password'] = $password;

It's generally bad practice to store a password in any place but the database.

This condition returns true, again the else after this is not needed. Remove it to reduce nesting.

---

Overall, you're on your way, but there's a lot that could be improved.

• Your code is strictly procedural which is becoming less and less neccessary.

• Your code is reinventing something that doesn't need to be made again.

• Your code is hard to read and understand. Partly because of all the nesting you have (which is easy to avoid with an OO design). Here's an article about arrow code written by StackExchange's very own Jeff Atwood.

There're hundreds of frameworks and community run scripts that do the logging in/out for you. Don't think that using one of these is limiting you or making your coding skills worse.

If you insist on using this code, it will suffice, but it will be hard to maintain and expand...