3
\$\begingroup\$

I recently asked for a code review to my login system. This login system was fully directed to the user. Now I have added some code lines to print out some links related to the admin.

Basically I only added a condition on profile.php:

if($_SESSION['email'] == 'admin17598@outlook.com'){ 
  echo 'a link to a file that i will use to insert and update my tables'
}

And inside these files that I will use to insert/update data on my table I only added this line:

<?php
require_once '../../db.php';
session_start();

if( $_SESSION['logged_in'] != 1 || $_SESSION['email'] != 'admin17598@outlook.com'){

$_SESSION['message'] = "Algo deu errado :/";
header("location: ../error.php");
exit;

}else {
 $username = $_SESSION['username'];
} 
?>

I want to know if the way I did is correct, and if there's a safer way to do this

Below you can see the full code if you want.

login.php:

<?php

if($_SERVER['REQUEST_METHOD'] == 'POST'){ 

$email = trim($_POST['email']);

try{
  $ckeckEmail = $conn->prepare("SELECT * FROM users WHERE `email` = :email");
  $ckeckEmail->bindValue(':email', $email);
  $ckeckEmail->execute();
  $user = $ckeckEmail->fetch(PDO::FETCH_ASSOC);    
  $RowCount = $ckeckEmail->rowCount();
} catch (PDOerrorInfo $e){}

  if( $RowCount == 0 ){
   // User doesn't exist
    $_SESSION['message'] = "Não existe um usuário com este e-mail.";
    header("location: error.php");
     exit;

  } else{ // User exists

      if( password_verify($_POST['password'], $user['password'])){
        $_SESSION['email']  = $user['email'];
        $_SESSION['user_id'] = $user['user_id'];
        $_SESSION['first_name'] = $user['first_name'];
        $_SESSION['last_name'] = $user['last_name'];
        $_SESSION['username'] = $user['username'];
        $_SESSION['img'] = $user['img'];
        $_SESSION['capa'] = $user['capa'];
        $_SESSION['logged_in'] = true;
        header("location: ../?p=profile");
        exit;
      } else {
          $_SESSION['message'] = "Password is incorrect";
          header("location: error.php");
           exit;
        }      
    }  
}

$conn = NULL;

register.php:

<?php

$img = rand(1,44);
$capa = rand(1,36);
$first_name = trim($_POST['first_name']);
$last_name = trim($_POST['last_name']);
$username = trim($_POST['username']);
$email = trim($_POST['email']);
$password = password_hash($_POST['password'], PASSWORD_BCRYPT);
$hash = md5( rand(0,1000) );

// Check if user with that email already exists
$result = $conn->prepare("SELECT * FROM users WHERE `email` = :email");
$result->bindParam(':email', $email);
$result->execute();
$RowCount = $result->rowCount();

if ( $RowCount > 0 ) { 
    $_SESSION['message'] = 'Já existe um usuário com este e-mail!';
    header("location: error.php");   
     exit;
}
else {
    $sql = "INSERT INTO users (dataR, ultima_atividade, first_name, last_name, username, img, capa, email, password, hash) VALUES (NOW(), NOW(), :first_name, :last_name, :username, :img, :capa, :email, :password, :hash)";
    $sql = $conn->prepare($sql);
    $sql->bindParam(':first_name', $first_name);
    $sql->bindParam(':last_name', $last_name);
    $sql->bindParam(':username', $username);
    $sql->bindParam(':img', $img);
    $sql->bindParam(':capa', $capa);
    $sql->bindParam(':email', $email);
    $sql->bindParam(':password', $password);
    $sql->bindParam(':hash', $hash);
    $sql->execute();
    $_SESSION['message'] = 'Your account was created!';
    header("location: success.php");
}
$conn = NULL;

profile.php:

<li class="pt-1 pb-2 liInfUs">
  <h6>Nome: <?php echo htmlentities($first_name, \ENT_QUOTES, 'UTF-8', false).' '.htmlentities($last_name, \ENT_QUOTES, 'UTF-8', false); ?></h6>
  <h6 class="quicksand300">Email: <?php echo htmlentities($email, \ENT_QUOTES, 'UTF-8', false); ?></h6>
</li>
<?php if($_SESSION['email'] == 'admin17598@outlook.com'){ 
  $countUserOLN = $conn->query("SELECT * FROM users WHERE ultima_atividade >= NOW()-600");
  $rstq = $countUserOLN->rowCount();
  $countUserAll = $conn->query("SELECT * FROM users");
  $rstAll = $countUserAll->rowCount();
?>
  <li class="d-flex">
    <h6 class="mr-3"><a href="verify/create/painel.php" style="color: #21D3FF">Painel</a></h6>
    <h6 class="quicksand300 mr-3">Users: <?php echo $rstAll ?></h6>
    <h6 class="quicksand300">Online: <?php echo $rstq ?></h6>
  </li>
<?php } ?>
\$\endgroup\$
3
\$\begingroup\$

Your question

I want to know if the way i did is correct, and if there's a safer way to do this

You need to ask how secure $_SESSION['email'] is. Could that value be updated, perhaps by a user profile update page?

Would it be simpler to use an attribute that would perhaps be stored in a relational table, to allow multiple users to be denoted as an admin?

Other review points

  • Use consistent indentation for the sake of readability maybe it was consistent before pasting in here but the block below has inconsistent indentation:

    if( $_SESSION['logged_in'] != 1 || $_SESSION['email'] != 'admin17598@outlook.com'){
    $_SESSION['message'] = "Algo deu errado :/";
    header("location: ../error.php");
    exit;
    
    }else {
     $username = $_SESSION['username'];
    } 
    

    Be consistent with the indentation - generally 4 (or sometimes 2) spaces are a convention.

    if( $_SESSION['logged_in'] != 1 || $_SESSION['email'] != 'admin17598@outlook.com'){
        $_SESSION['message'] = "Algo deu errado :/";
        header("location: ../error.php");
        exit;
    
    }else {
        $username = $_SESSION['username'];
    } 
    
  • Store values in constants That way they can be updated in one spot instead of each spot they occur.

    const ADMIN_EMAIL_ADDRESS = 'admin17598@outlook.com';
    

    Then that constant name can be used instead of the value, e.g.:

    if($_SESSION['email'] == ADMIN_EMAIL_ADDRESS){ 
    
  • Don't Repeat Yourself (A.K.A. D.R.Y.). I see the following lines in login.php

    $ckeckEmail = $conn->prepare("SELECT * FROM users WHERE `email` = :email");
    $ckeckEmail->bindValue(':email', $email);
    $ckeckEmail->execute();
    

    And similarly in register.php:

    $result = $conn->prepare("SELECT * FROM users WHERE `email` = :email");
    $result->bindParam(':email', $email);
    $result->execute();
    

    Bearing in mind that the variable names are different, the rest of the code is identical. Those lines could be abstracted out into a function that could be called in both places.

  • use SELECT count(*) instead of SELECT * for a count in profile.php, those queries select all rows, but all you use them for is the count. if there are thousands of relevant rows, those queries could be returning a lot of wasted data.

  • empty try/catch - I know @Your Common Sense already mentioned this in the answer to your previous question but there is still an empty catch in login.php. What should happen when an exception is caught? Should an error message be displayed (and page execution stopped)? This is important to consider, especially if there is a PDO exception like database server offline, credentials changed, database name changed, etc. Read more about this in the answer I just added.
| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ About the catch (PDOerrorInfo $e){} i heard that i should leave it empty to not show users any kind of error that could compromisse my database security. It looks like there's differents opinions about it, and i choosed to leave it in blank. You did not shared your opinion about it, you only made a question. So what you think about it? \$\endgroup\$ – Natalie Dec 7 '18 at 1:55
  • 2
    \$\begingroup\$ @Natalie Sadly, php community is full of rumors and superstitions and a empty try catch is a nasty one. There is a much simpler way to not show users any kind of error that does not require writing ANY try catch operators in the code. See my article that explains the proper error reporting in detail. \$\endgroup\$ – Your Common Sense Dec 7 '18 at 8:45
1
\$\begingroup\$

Another thing I noticed is this:

} catch (PDOerrorInfo $e){}

What is PDOerrorInfo? Did you declare that as a subclass of Exception? If so, when do you expect that to be thrown? Otherwise are you thinking of the PDO method errorInfo()? If that is the case, this catch is basically useless, since a catch can basically only handle instances of Exception. Apparently there won't be any warning about this mistake but if a PDOException is thrown, it won't be caught by the code above.

Generally you would want to have code to notify you or somebody else who can handle the issue of the exception:

try{
    $ckeckEmail = $conn->prepare("SELECT * FROM users WHERE `email` = :email");
    $ckeckEmail->bindValue(':email', $email);
    $ckeckEmail->execute();
    $user = $ckeckEmail->fetch(PDO::FETCH_ASSOC);    
    $RowCount = $ckeckEmail->rowCount();
} 
catch (PDOException $e){ //PDO specific exception
    //call to email or log message, likely using $e->getMessage()
}
catch (Exception $e){ //General exception
    //call to email or log message, likely using $e->getMessage()
}

Though you could just combine those two catch statements into a single for any exception. Just be aware that PDOException uses a string for the $code property instead of an int like Exception uses.

try {
    //database actions
}
catch (Exception $e){ //any instance of Exception, including PDOException
    //call to email or log message, likely using $e->getMessage()
}

After reading your comment:

About the catch (PDOerrorInfo $e){} i heard that i should leave it empty to not show users any kind of error that could compromisse my database security.

It is important to note that an exception could occur for many reasons like database connection issues (e.g. database server offline, incorrect credentials from the PHP code, etc.) so this is more a matter of handling scenarios where things are not correctly setup and less about security. And nothing requires you to show errors to the user - it is recommended that you make it visible to whoever is responsible for handling it (typically you or a team if you have one), e.g. via email, logging it in a log file, etc.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Don't you find this approach quite repetitive? Imagine there are 10 queries to be run, how many calls to email or log message, likely using $e->getMessage() it will be? \$\endgroup\$ – Your Common Sense Dec 7 '18 at 17:10
  • \$\begingroup\$ Yes - I amended the post to mentioning catching any exception in one catch block. \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Dec 7 '18 at 17:16
  • \$\begingroup\$ That's better, and more to the standards but why limit yourself to database actions only? What if there is an wrong argument type exception? \$\endgroup\$ – Your Common Sense Dec 7 '18 at 17:16
  • \$\begingroup\$ Do you mean InvalidArgumentException? Is that not a subclass of LogicException which makes it a subclass of Exception? \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Dec 7 '18 at 17:19
  • \$\begingroup\$ Whatever. InvalidArgumentException OutOfRangeException. It doesn't matter which particular one. Don't you want to email or log the message whatever it is? So I would make it a global try.. catch around the whole code. It will make the error handling DRY and uniform, \$\endgroup\$ – Your Common Sense Dec 7 '18 at 17:24

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.