4
\$\begingroup\$

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()
\$\endgroup\$
6
  • \$\begingroup\$ Rather than reinventing, try to find how it's solved in PHP frameworks. I would be more critical then Alex, I would say you are going wrong direction. And $GLOBALS['adminPage'] == 1 is just plain wrong. \$\endgroup\$ – Marek Jun 20 '14 at 8:18
  • \$\begingroup\$ Thank you for reading and replying to my post, could you please elaborate on what you mean by "just plain wrong."? Should I store the global in a session variable instead? \$\endgroup\$ – user3586322 Jun 20 '14 at 15:28
  • \$\begingroup\$ You shouldn't use global variables. Also $GLOBALS["isAdmin"] = TRUE;. This function also mixes incontroller code (decision to do POST, redirection, exit()). Security problem is that is saves cleartext password into session. \$\endgroup\$ – Marek Jun 23 '14 at 11:27
  • \$\begingroup\$ You mention you use some UUID() function. Have you checked it is designed to generate random and unpredictable strings with enough entropy? Else you have another security issue. \$\endgroup\$ – Marek Jun 23 '14 at 11:34
  • \$\begingroup\$ @Marek, could you post a link to a good article on using UUID() correctly? cause there's a lot of them out there but they are different and its hard to know which is right. \$\endgroup\$ – user3586322 Jun 23 '14 at 15:16
4
\$\begingroup\$

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...

\$\endgroup\$
7
  • \$\begingroup\$ Finally an answer i can work with, thank you. I will study and fine-tune the code from you suggestions and shall return once more to dig deeper. hope to see you around again. =) \$\endgroup\$ – user3586322 Jun 20 '14 at 0:01
  • \$\begingroup\$ Sounds like you had a bad experience somewhere, that's too bad! The guys here at Code Review are generally pretty nice :) \$\endgroup\$ – Alex L Jun 20 '14 at 0:02
  • \$\begingroup\$ quick question, How else should i store my password? cause when you navigate to the protected page it calls this method to keep you logged in: return databaseContainsUser($_SESSION['user'], $_SESSION['password']); once you click log-out though it unset() the sessions \$\endgroup\$ – user3586322 Jun 21 '14 at 19:23
  • \$\begingroup\$ Typically, you shouldn't need to log in a user on every page request, thats a bit over the top! Having just a unique session should be okay. Check for that instead of logging in each time. \$\endgroup\$ – Alex L Jun 21 '14 at 19:52
  • \$\begingroup\$ Dude you are awesome, thank you! last quick question, when i post my updated code should I start a new thread or just re-post on this thread? \$\endgroup\$ – user3586322 Jun 21 '14 at 20:26

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.