Trouble with admin login


#1

I wonder if anyone can help. I have just really started on php and I am having trouble with an admin login script. I have learnt off w3schools and I have tried to piece my script together but it still isn’t working. I have taken everything through the MySQLi Object-Oriented way as w3schools have shown. I know I still have a lot of work to do on the script such as password_hash and mysql injection.

I have my database connect

    <?

    /* DATEBASE CONFIGURATION */

    $servername = "";
    $dbName = "";  
    $username = "";
    $password = "";

    // Create connection
    $conn = new mysqli($servername, $username, $password);

    // Check connection
    if ($conn->connect_error) {
        die("Connection failed: " . $conn->connect_error);
    } 

    ?>

I have my admin_login.php

<?php 
session_start();
if (isset($_SESSION["manager"])) {
    header("location: index.php"); 
    exit();
}
?>
<?php 
// Parse the log in form if the user has filled it out and pressed "Log In"
if (isset($_POST["username"]) && isset($_POST["password"])) {

    $manager = preg_replace('#[^A-Za-z0-9]#i', '', $_POST["username"]); // filter everything but numbers and letters
    $password = preg_replace('#[^A-Za-z0-9]#i', '', $_POST["password"]); // filter everything but numbers and letters
// Connect to the MySQL database  
    include "../storescripts/connect_to_mysql.php"; 
    $sql = mysqli_query("SELECT id FROM admin WHERE username='$manager' AND password='$password' LIMIT 1"); // query the person
    // ------- MAKE SURE PERSON EXISTS IN DATABASE ---------
    $existCount = $conn->query($sql);
    if ($existCount->mysqli_num_rows == 1) {
    while($row = mysqli_fetch_array($sql)){ 
             $id = $row["id"];
         }
         $_SESSION["id"] = $id;
         $_SESSION["manager"] = $manager;
         $_SESSION["password"] = $password;
         header("location: index.php");
         exit();
    } else {
        echo 'That information is incorrect, try again <a href="index.php">Click Here</a>';
        exit();
    }
}

?>

And i have my the page in which it is suppose to go to after it passes through the code called index.php

<?php 
session_start();
if (!isset($_SESSION["manager"])) {
    header("location: admin_login.php"); 
    exit();
}
// Be sure to check that this manager SESSION value is in fact in the database
$managerID = preg_replace('#[^0-9]#i', '', $_SESSION["id"]); // filter everything but numbers and letters
$manager = preg_replace('#[^A-Za-z0-9]#i', '', $_SESSION["manager"]); // filter everything but numbers and letters
$password = preg_replace('#[^A-Za-z0-9]#i', '', $_SESSION["password"]); // filter everything but numbers and letters
// Run mySQL query to be sure that this person is an admin and that their password session var equals the database information
// Connect to the MySQL database  
include "../storescripts/connect_to_mysql.php"; 
$sql = mysqli_query("SELECT * FROM admin WHERE id='$managerID' AND username='$manager' AND password='$password' LIMIT 1"); // query the person
// ------- MAKE SURE PERSON EXISTS IN DATABASE ---------
$existCount = $conn->query($sql);
$existCount = mysqli_num_rows($sql); // count the row nums
if ($existCount == 0) { // evaluate the count
     echo "Your login session data is not on record in the database.";
     exit();
}
?>

After it passes through the script it goes to the else in admin_login

else {
        echo 'That information is incorrect, try again <a href="index.php">Click Here</a>';
        exit();

Can someone please tell me where i am going wrong. i am only a beginner in php and really would appreciate some help, The more people explain it to me the more i can understand other bits of code i am trying to put together, many thanks, Gary


#2

I’ll take it easy on you. First, go here: https://www.phptherightway.com/

You are presently doing a few things, that worry me from a security standpoint, like storing a password.

The whole code in one piece with that it IS doing and what it SHOULD be doing will help more, for me.


#3

hi astonecipher, thank you very much for your reply, so what you are saying is that the password should not be session and should be taken out. am i right? i think the problem that i am having is in the if statement but i just work it out. sorry i am only a beginner. i do appreciate any help you can give me, many thanks


#4

Correct, you do not want to store passwords, even hashed ones in the session array.

Loose this:

if ($existCount->mysqli_num_rows == 1) {
while($row = mysqli_fetch_array($sql)){ 
         $id = $row["id"];
     }
     $_SESSION["id"] = $id;
     $_SESSION["manager"] = $manager;
     $_SESSION["password"] = $password;
     header("location: index.php");
     exit();
} else {
    echo 'That information is incorrect, try again <a href="index.php">Click>';
    exit();
}

That is your issue. You just want to fetch a single row, so you don’t need a while loop to get the data back for one. To test if there is a row, just see if that returned array is empty.


#5

hi astonecipher, again thank you very much for your reply. this php is hard work lol. should i not even session the id and the manager which is the username or even the id on it’s own? many thanks for your help, top man


#6

The most likely reason your code isn’t working is because you are not selecting a database (see item #4 in the following list) and the queries are failing with an error, but you have no error handling for the queries, so you don’t know when and why they are failing (see items #5 and #6 in the following list.)

A bunch of points (some of which have already been mentioned in the thread) -

  1. Don’t use php code you find at w3schools. It is filled with bad programming practices.

  2. Always use full opening php tags - <?php. You have one short opening php tag <? in your connection code that could be causing the connection code to not even be seen as being php code. Also, be consistent. You have full opening php tags elsewhere, use them everywhere. Also, don't put a closing php tag ?> and immediately follow it with an opening php tag.

  3. You should learn and use the php PDO extension instead of the mysqli extension. It is much simpler and more consistent than the mysqli extension, especially when using prepared queries.

  4. When you make the database connection, you need to select the database.

  5. You should NOT unconditionally output raw database errors onto a web page. If you use exceptions to handle database statement (connection, query, prepare, and execute) errors, and in most cases let php catch the exception, it will use its error_reporting, display_errors, and log_errors settings to control what happens with the actual error information. Error_reporting should always be set to E_ALL. When learning, developing, and debugging code/queries, you should display all errors. When on a live/public server, you should log all errors.These settings should be set in the php.ini on your system.

  6. You should have error handling for ALL the database statements. Enabling exceptions for the database extension you are using will give you error handling for all the statements, without having to add code at each statement (you can also eliminate the existing error handling code for the connection.)

  7. The only value your login system should store in a session variable is the user id and the session variable should be uniquely named to indicate the meaning of the data, such as $_SESSION[‘user_id’].

  8. Don’t use isset() statements for form fields that will be set (only unchecked checkbox/radio form fields won’t be set and are the only place in your form processing code that you should have any isset() statements.) Your form processing code should first detect that a post method form has been submitted, then all the expected form fields will be set (if they are not, you have a mistake somewhere and you would want to see php errors, which the isset() statements hide.)

  9. Except for trimming submitted data, so that you can detect if all white-space characters were entered, don’t modify or filter user submitted data. You want to validate the submitted data and then only use the data if it is valid. You don’t want to alter/change the user submitted data. I know of another php help site that got its user database copied, because the individuals who wrote the forum software inconsistently filtered/changed the submitted email addresses, which mapped multiple values to the same user. This allowed a hacker to do a password recovery on an administrator account and login as the administrator. Don’t alter/filter/change user submitted data.

  10. You can trim all the submitted data at once as a set, using one array statement. If someone has time, they will post an example.

  11. You should store validation errors in a php array. The array then becomes an error flag. If the array is empty, there are no validation errors. If the array is not empty, there are errors. You can also display the contents of the array to display the errors to the user.

  12. Don’t create unnecessary variables, by copying variables to other variables, and if you do modify the meaning of data in a variable, such as when you trim it, don’t change the name/index of the variable. In your code, the username data should be called username throughout the code. You should not have the $manager variable at all.

  13. You should use prepared queries when supply data value(s) to an sql query statement, with place-holder(s) in the sql statement for each data value, then supply the data when you execute the query.

  14. As already mentioned, by you, you should use password_verify() and password_hash() when testing/storing hashed passwords.

  15. When a query is expected to match at most one row, just fetch the row of data and detect if there was a row.

  16. You should have all user login data stored in one users table. An administrator is just a user that has specific permissions.

  17. Your form and form processing code should be on the same page. This will eliminate the need for links to go back and try again, and you will be able to display any validation errors when you re-display the form. It will also let you re-populate the form fields with the submitted values so that the user doesn’t need to keep typing in the same information, which will increase the chance of typo errors.

  18. The code on any login ‘protected page’ should query for the user permissions. This will do what the current code is doing (verify that the user id exists) and it will get the current permissions for the user. This will mean that any changes to the user’s permissions will take effect immediately (on the next page request.) If you store the user permissions in a session variable, any change won’t take effect unless the user logs out and back in again.

  19. You only need to validate external user submitted data ($_POST, $_GET, $_COOKIE, $_FILES, and some $_SERVER.) The user id in the session variable doesn’t need to be validated before using it since it is an internal value.

  20. When dealing with database queries, only the final result that your application code expects needs to be stored in uniquely named variables. The common steps leading up to the result should use generic, simple named variables. This will save typing and typo errors, but you will also begin to see that the common steps that you are repeating for each query are the same and can be converted to use function/class-method calls instead.


#7

thank you very much for everything you have said phdr, i will do a php pdo course on saturday as i have to do a business plan to do tomorrow. I like this site, people take the time to explain things to you and not just make you feel small for not knowing things. thank you very much astonecipher and phdr for your help and you will be seeing me again on this site. this site gets a number 1 ranking from me.