0

I'm creating a secure login system in PHP.
On each page of the admin area, I want to be sure that the user is correctly logged in, that his account is still enabled, that the user is connected once with his login details (when user loggs in for the first time, the active_user status is set to 1).

function isCookieValid() {
    global $pdo;

    $isValid = false;
    if(isset($_COOKIE["rememberUser"])) {

        $decryptCookie = base64_decode($_COOKIE["rememberUser"]);
        $user_id = explode("mMUa26yB943jRaJl755OM18jgR", $decryptCookie);
        $user_id = $user_id[1];

        $sqlQuery = "SELECT * FROM users WHERE id_user = :id";
        $stmt = $pdo->prepare($sqlQuery);
        $stmt->execute(array(":id" => $user_id));
        if ($row = $stmt->fetch()) {
            if($row["enabled_user"] === 1 && $row["active_user"] === 0) {

                        unset($row["password"], $row["salt"]);
                        $_SESSION["current_user"] = $row;
                        $isValid = true;
            } else {
                logout();
            }
        } else {
            logout();
        }
    }
    return $isValid;
}

I ask the database on every page, for every user, to check if the cookie that is stored in the computer (I automatically activate the cookie, because for my application needs to keep all users logged in for a long while) corresponds to an ID user on the database. If so, I create a variable that stores user details. Then for protecting my pages, I use the function isLoggedin() which is:

function isLoggedin() {

    if(isCookieValid()){
        if(isset($_SESSION["current_user"])) {
            return true;

        }
    }
    return false;
}

Is that system not too heavy ? Because I check the database on every page, for every user. Thanks in advance

Yoyo
  • 21
  • 5
  • Sessions are far better than cookies. Never let sensitive data leave the server. Please [read here](https://stackoverflow.com/questions/2648440/encrypting-cookies-in-php) about cookie encryption. Also [read here](https://stackoverflow.com/questions/328/php-session-security) about session security. – Martin Nov 15 '19 at 09:34

4 Answers4

0

Your system is not heavy, it's not safe. Please don't use a cookie to verify a user, always use session.

function isLoggedin() {
        if(isset($_SESSION["current_user"])) {
            return true;
        } else {
             // here redirect to login page to verify the user id and password..
        }
}
Garry Xiao
  • 232
  • 3
  • 9
  • Adding some reasoning as to **why** here would be useful – Martin Nov 15 '19 at 09:31
  • A cookie is only clientside information. Clientside data can be altered before sending to you. When information comes from clientside, never trust it only because the data was sent by you. – Garry Xiao Nov 15 '19 at 10:31
  • A session is both sides data maintained by clientside and serverside. The only point you should pay more attention to is when you set the session data. Other things are determined by the server and should be safe enough. – Garry Xiao Nov 15 '19 at 10:35
  • Garry, **edit** your answer to add this useful information into the answer itself. Thanks. – Martin Nov 15 '19 at 11:57
0

Maybe you could do something like this. Hope it'll help.

<?php
// Initialize the session
session_start();

// Check if the user is logged in, if not then redirect him to login page
if(!isset($_SESSION["current_user"]) || $_SESSION["current_user"] !== true){
header("location: login.php");
exit;
}
?>
0

Like already said, use sessions for login handling.

Also a general tip: Mixing up responsibilities will lead to problems you have now.

An indicator for example is your function name isCookieValid() and the call logout() in this function

The isCookieValid() call should just do what it supposed to do - checking if the cookie is valid and nothing more.

Following this, you are more flexible doing conditional stuff like

if(!isCookieValid()) {
    logout();
} 

Without running implicit unwanted stuff like you are doing now.

Jim Panse
  • 2,220
  • 12
  • 34
0

To the question "Is that system not too heavy ? Because I check the database on every page, for every user." I just say "it depends". If your system is slow is too heavy. If not, .. I dont care. I mean, ... if something works, doesnt really matter how it is done.

Post Scriptum. While you care about the heavy of the system, .. you are not watching about the security. You just shared the way to login to your system. For example creating a string encoded like this:

    $_COOKIE['rememberUser'] = base64_encode(
        'mMUa26yB943jRaJl755OM18jgR42'
    );

Do not share sensitive data ... In this answer I will not talk about security, but I suggest you to look for jwt solutions. JWT (Javascript Web Token) is a standard de facto and can help you in your use case: there are a lot of solution already implemented. You here are reinventing the wheel. but

I've just refactored your isCookieValid method:

function isCookieValid() {
    global $pdo;
    $isValid = false;

    if(isset($_COOKIE["rememberUser"])) {
        $decryptCookie = base64_decode($_COOKIE["rememberUser"]);
        $user_id = explode("mMUa26yB943jRaJl755OM18jgR", $decryptCookie);
        $user_id = $user_id[1];

        $sqlQuery = "SELECT * FROM users WHERE id_user = :id";
        $stmt = $pdo->prepare($sqlQuery);
        $stmt->execute(array(":id" => $user_id));

        if ($row = $stmt->fetch()) {
            if($row["enabled_user"] === 1 && $row["active_user"] === 0) {
                unset($row["password"], $row["salt"]);
                $_SESSION["current_user"] = $row;
                return true;
            }
        }
    }

    return false;
}

And then u can use:

function isLoggedin() {
    if (isCookieValid()) {
        logout();
    }
}
sensorario
  • 20,262
  • 30
  • 97
  • 159
  • Please explain your refactoring. There's quite a few things in there that look like they are not best practise from a security point of view (*salts*, etc.) – Martin Nov 15 '19 at 09:36
  • I've not fixed the security. I've simplified existing code. I think an'edit is necessary. I want to say "security is more important than heaviness". Also, ... my response want to be focused on the question. – sensorario Nov 15 '19 at 09:51