3
\$\begingroup\$

I would like to ask your advice on my simple code to login and registration sessions.

In the User class, login function:

public function login($username, $password){   
    $this->db->query("SELECT * FROM users WHERE username = :username AND status = :status LIMIT 1");
    $this->db->bind(':username', $username);
    $this->db->bind(':status', 1);
    $row = $this->db->single();
    $count = $this->db->rowCount();
        if ($count > 0) {
            if (password_verify($password, $row['password'])) {
                $_SESSION['session'] = [
                    'id' => $row['id'],
                    'username' => $row['username'],
                    'email' => $row['email'],
                    ];
                return true;
            } else {
                return false;
            }
        }
}

check logged:

public function isLoggedIn() {
    if (isset($_SESSION['session'])) {
    return true;
    }
}

And login page with check logged:

if ($user->isLoggedIn()) {
    header('Location: index.php');
    exit();
}

if(isset($_POST['login'])){

    //Retrieve the field values from our login form.
    $username = !empty($_POST['username']) ? trim($_POST['username']) : null;
    $password = !empty($_POST['password']) ? trim($_POST['password']) : null;

    if($user->login($username, $password)) {
        header('Location: index.php');
        exit();
    } else {
        $message[] = 'We found problems with the login, please try again.';
    }
}
\$\endgroup\$
  • \$\begingroup\$ Whatever you do with the session, simply create a random session token and save it to your db. Just checking for the existence of a session is as secure as an open door. \$\endgroup\$ – GiantTree May 14 '16 at 10:11
  • \$\begingroup\$ So, when you register a new user step a token with a random code, and then when you check the validity of the login token? Do you have any examples? \$\endgroup\$ – Marco May 14 '16 at 11:12
  • \$\begingroup\$ Take a look at this. You can see in login() on line 100 it creates a new session. In newsession() (line 248 ff.) it creates a new session and saves it to the db. This session is tied to the user and IP address. Although session generation is not optimal (microtime() is not a good example), it generates unique sessions and invalidates them, if they expire (checksession(), line 370 ff.). I wouldn't recommend the whole class because of weak security but the base is very robust. \$\endgroup\$ – GiantTree May 14 '16 at 11:19
  • \$\begingroup\$ Instead of reinventing the wheel, use already made solutions for your problem: phpauth.tk. Remember to look over the code and customize everything you need. \$\endgroup\$ – GiantTree May 14 '16 at 11:24
  • \$\begingroup\$ I'm having a problem, everything worked, until it is night I slightly modified the code in the login () function by inserting setcookie etc., and this morning the login does not work, that seems to properly switch sessions without feeding the username and then the 'user account is not updated with your data, you know why? \$\endgroup\$ – Marco May 14 '16 at 11:49
1
\$\begingroup\$

The could looks like it would work properly, and I think it's clearly written and easy to understand.

When you check that a user is logged in, you should check for something in the session that clearly states they are logged in. For instance you could check that $_SESSION['user']['loggedIn'] === true. It's risky to simply test for isset($_SESSION['session']) as another programmer, or even you in six months time, could write code to set that when the user is not logged in. Perhaps they will want to store a message in the session, like "sorry your password didn't match try again".

I would replace $_SESSION['session'] with $_SESSION['User']. You might then want to only access $_SESSION['User'] from the User class of your application. Other parts of the application may want to store other things in the session - messages to display on next request, or a basket of goods, or whatever the app deals with.

I assume you have called session_start() before the code we see runs.

Using password_verify() is good. Far too many apps implement their own insecure password algorithms.

I don't see the 'new' keyword, which is good - that means you may be doing dependency injection by having the constructor of User take a db as a parameter. If that's true you can easily write a test for the login function that uses a mock db.

\$\endgroup\$

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.