PDO and Login

[Adem]

Hello,
I’m trying to make a PDO login system,

What are the mistakes here?

private function dologinWithPostData()
{
    if (empty($_POST['user_name'])) {
        $this->errors[] = "Username field was empty.";
    } elseif (empty($_POST['user_password'])) {
        $this->errors[] = "Password field was empty.";
    } elseif (!empty($_POST['user_name']) && !empty($_POST['user_password'])) {

        $user_name = $_POST['user_name'];

        try {
            $conn = new PDO("mysql:host=".DB_HOST.";dbname=".DB_NAME.";charset=utf8", DB_USER, DB_PASS);
            $conn->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
            $stmt = $conn->prepare("SELECT user_name, user_email, user_password_hash FROM users WHERE user_name=:user_name");
            $stmt->bindParam("user_name", $user_name, PDO::PARAM_STR);
            $stmt->execute();
          
            $result = $stmt->fetch(PDO::FETCH_ASSOC);

            if (!$result) {
                $this->errors[] = "Username password combination is wrong!";
            }else{
                if (password_verify($_POST['user_password'], $result['user_password_hash'])) {
                    $_SESSION['user_name'] = $result['user_name'];
                    $_SESSION['user_email'] = $result['user_email'];
                    $_SESSION['user_login_status'] = 1;
                }else{
                    $this->errors[] = "Username password combination is wrong!";
                }
            }

          } catch(PDOException $e) {
            $this->errors[] = "Error: " . $e->getMessage();
          }
          $conn = null;
    }
}

[ErnieAlex]

Well, how have you debugged this so far? Have you made sure that you are posting data to the code?
I mean, since you are in the beginners section, this is just the PHP code for the login. It is not the rest of
the page with the HTML form. Are you sure you are getting data to start with.

Also, we assume you already set up the database and actually have data in it for testing.

What errors are you getting? More info please!

[Adem]

Create member, Login, Forgot password, PDO and class and password reset token
I’m trying to code with your help

HTML ok
META ok
Login ok
There is no problem, it works
Is there incorrect coding?
Better coding, secure coding
Any suggestions anywhere?

This is a member creation, no problem, any suggestions?

private function registerNewUser()
{
    if (empty($_POST['user_name'])) {
        $this->errors[] = "Empty Username";
    } elseif (empty($_POST['user_password_new']) || empty($_POST['user_password_repeat'])) {
        $this->errors[] = "Empty Password";
    } elseif ($_POST['user_password_new'] !== $_POST['user_password_repeat']) {
        $this->errors[] = "Password and password repeat are not the same";
    } elseif (strlen($_POST['user_password_new']) < 6) {
        $this->errors[] = "Password has a minimum length of 6 characters";
    } elseif (strlen($_POST['user_name']) > 64 || strlen($_POST['user_name']) < 2) {
        $this->errors[] = "Username cannot be shorter than 2 or longer than 64 characters";
    } elseif (!preg_match('/^[a-zA-ZÇŞĞÜÖİçşğüöı \d]{2,64}$/i', $_POST['user_name'])) {
        $this->errors[] = "Username does not fit the name scheme: only a-Z and numbers are allowed, 2 to 64 characters";
    } elseif (empty($_POST['user_email'])) {
        $this->errors[] = "Email cannot be empty";
    } elseif (strlen($_POST['user_email']) > 64) {
        $this->errors[] = "Email cannot be longer than 64 characters";
    } elseif (!filter_var($_POST['user_email'], FILTER_VALIDATE_EMAIL)) {
        $this->errors[] = "Your email address is not in a valid email format";
    } elseif (!empty($_POST['user_name'])
        && strlen($_POST['user_name']) <= 64
        && strlen($_POST['user_name']) >= 2
        && preg_match('/^[a-zA-ZÇŞĞÜÖİçşğüöı \d]{2,64}$/i', $_POST['user_name'])
        && !empty($_POST['user_email'])
        && strlen($_POST['user_email']) <= 64
        && filter_var($_POST['user_email'], FILTER_VALIDATE_EMAIL)
        && !empty($_POST['user_password_new'])
        && !empty($_POST['user_password_repeat'])
        && ($_POST['user_password_new'] === $_POST['user_password_repeat'])
    ) {
        $user_name = filter_input(INPUT_POST,'user_name');
        $user_email = filter_input(INPUT_POST,'user_email');
        $user_password = $_POST['user_password_new'];

        $user_password_hash = password_hash($user_password, PASSWORD_DEFAULT);

        try {
            $conn = new PDO("mysql:host=".DB_HOST.";dbname=".DB_NAME.";charset=utf8", DB_USER, DB_PASS);
            $conn->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);

            $stmt = $conn->prepare("SELECT user_email FROM users WHERE user_email=:user_email");
            $stmt->bindParam("user_email", $user_email, PDO::PARAM_STR);
            $stmt->execute();
         
            if ($stmt->rowCount() > 0) {
                $this->errors[] = '<p class="error">The email address is already registered!</p>';
            }

            if ($stmt->rowCount() == 0) {
                $stmt = $conn->prepare("INSERT INTO users(user_name, user_email, user_password_hash) VALUES (:user_name,:user_email,:user_password_hash)");
                $stmt->bindParam("user_name", $user_name, PDO::PARAM_STR);
                $stmt->bindParam("user_password_hash", $user_password_hash, PDO::PARAM_STR);
                $stmt->bindParam("user_email", $user_email, PDO::PARAM_STR);
                $result = $stmt->execute();
         
                if ($result) {
                    $this->errors[] = '<p class="success">Your registration was successful!</p>';
                } else {
                    $this->errors[] = '<p class="error">Something went wrong!</p>';
                }
            }

          } catch(PDOException $e) {
            $this->errors[] = "Error: " . $e->getMessage();
          }
          $conn = null;
    }
}

}

PDO connecting is correct?

[benanamen]

First issue is stuffing all that validation code into the method. At minimum, it should be pulled up to it’s own method, at best, create a validation class.

The next problem is you are not doing anything with the validation and just calling the query regardless. You are also outputting internal system errors to the user. That info is only good to hackers and useless to the user.

There are only a few cases to use try/catch. This is not one of them.

The DB connections should ultimately be passed to the Class using Dependency Injection.

Do not check if an email is available. Set a unique constraint on the column, attempt the insert, and catch the duplicate error if any. This would be a valid case for try/catch.

There is no need to manually close the db connection. Php does it automatically.

Do not create variables for nothing.

Chaining a bunch of validations together is a very poor practice. How will you know which one failed? You have also duplicated validations.

[phdr]

Create member, Login, Forgot password, PDO and class and password reset token

This appears to be a course assignment. Whoever has given this assignment should have covered proper OOP design. The code you have shown is your main application code surround by a class/method definition. This is not OOP. All this has done is add an unnecessary layer to what you are doing, creating more work for you, not less work. OOP classes/methods should do general-purpose, reusable, useful things that help you to write applications, i.e. they are building blocks. And as @benanamen has stated, your class methods should NOT be responsible for creating/destroying the database connection. It is the responsibility of your main code to create the database connection and to supply it to any class that needs it.

Ignoring the incorrect OOP usage, the code should/should-not do the following (hopefully not repeating too many that @benanamen has given) -

1. Trim all input data before validating it. You can do this with a single array_map() call.
2. Validate all independent inputs at one time. By stringing together all the validation tests with elseif statements, the code will ‘stop’ validating inputs on the first error. The user will have to repeatedly submit the form to find all the validation errors. The only time you should have elseif/else statements in the validation logic are for dependent validation steps for the same input.
3. Use the field name as the errors array index. This is so that dependent validation steps can test if there is not already an error for the input and so that you can test/output the validation error messages separately at specific places in the html document.
4. One of the points of using an array to hold the validation errors is that this array is also an error flag. If the array is empty, there are no errors and you can use the submitted form data. This will save you from writing out and repeating the same tests twice in your code.
5. Don’t write out line after line of code copying variables to other variables. This is just a waste of time. Just keep the input data as an array and use elements of that array throughout the rest of the code.
6. Using filter_input() AFTER you have already tested and validated the inputs is pointless and not testing the filter_input() return value will mean that your code doesn’t even know if the input existed, which for ‘always set’ inputs indicates a programming mistake. Just forget about using filter_input() for the time being.
7. Once you have more than 2-3 inputs, you should consider dynamically validating and processing the data, by defining an array that lists the validation and processing steps for each field. This would be a good place to create OOP code to do general-purpose, reusable, useful things that help you to write applications.
8. Your database connection code should also set emulated prepared queries to false (you want to use real prepared queries) and set the default fetch mode to assoc, so that you don’t need to specify it in every fetch statement.
9. You should build the sql query statement in a php variable. This will help with debugging and will separate the sql syntax as much as possible from the php code, helping to prevent errors.
10. Use ? prepared query place-holders and just supply an array of input values to the execute([…]) call.
11. Any styling of the validation error messages should be handled in the html document, not in the validation code.
12. Since you are using exceptions for database statement errors, the current logic testing the result from the ->execute() call for the INSERT query will never be executed upon an error. However, this is a case where your code should catch the exception, test if the sql error number is for a duplicate key, then setup an error message for the user telling them what was wrong with the data that they submitted. For all other sql error numbers, just re-throw the database exception and let php handle it.
13. Your current exception catch logic, that’s storing the raw sql error message in the errors array, implies that you are doing to display this to the user. As @benanamen has already stated, don’t do this. The only time you should display the raw database errors is when you, as the developer/programmer, are debugging problems. At all other times, you should log this information. This will happen ‘automatically’ if you let php handle the database exceptions.
14. The only user value you store in a session variable upon successful login should be the user_id (auto-increment integer index.) This also serves as a login flag. If it is set, there is a logged in user. If it is not set, there is no logged in user. You should query on each page request to get any other user information. This insures that any changes made to the other user information will take effect without needing the user to log in again.
15. Don’t use defined constants for the database connection credentials. This limits your application to a single database connection.

[Strider64]

I just like to add most methods (functions in Classes) are usually pretty short and even some Classes are short as they are way to create your own PHP library that can be used over and over again. In a sense you’re creating your own mini PHP framework - well kind of. Here’s a method that I did for a Calendar Class that I have been using over the last 2 to 3 years. When I first started out writing the Calendar Class it was very long, but as I’m getting better in writing the PHP OOP it’s getting shorter and shorter (as well as my other PHP Classes). It saves time in writing PHP and it helps in concentrating solely on HTML/CSS.

Here’s the example of a method from the Calendar Class:

protected function display() {

    /* Grab Holiday from Holiday Class (if there is one) */
    $holidayCheck = new Holiday($this->current->format("F j, Y"), 1);

    /* Assign Holiday to a Variable/Argument */
    $this->holiday = $holidayCheck->holidays(); 
    $this->controls(); // Create Buttons for Previous/Next Calendars: 

    /*
     * Month Being Displayed Variable
     */
    $this->calendar[$this->index]['month'] = $this->current->format('F Y');

    /* Generate last Sunday of previous Month */
    $this->current->modify("last sun of previous month");

    /*
     * Output 6 rows (42 days) guarantees an even calendar that will
     * display nicely.
     */
    $num = 1;
    while ($num <= 6) {
        $this->drawDays(); // Grab the Current Row of Dates:
        $num += 1;
    }

    return $this->calendar;
} // End of Display Method:

Using an autoloader (I suggest using composer’s autoloader), namespace to avoid conflicts with arguments (variables) / methods (functions - they not truly “functions”, but I looked at them this way) and comment the code will save you a lot of time, plus headaches.

[benanamen]

@Strider64, I agree with the text of your post.

As for the method, Holiday should not be instantiated in whatever class this is. You should pass the instance to the class using Dependency Injection. As it is you are locked into a specific date format. You would have to edit the class to have a different date format which again will still limit you to one format at a time. This code is what would be called “Tightly Coupled” which is not what you want.

[Adem]

I know PHP very very little
I’m trying to adapt it by taking examples from existing codes.
First of all, the database connection format is wrong let me change it

$host = 'localhost';
$db   = 'login';
$user = 'root';
$pass = '';
$charset = 'utf8mb4';

$dsn = "mysql:host=$host;dbname=$db;charset=$charset";
$options = [
    PDO::ATTR_ERRMODE            => PDO::ERRMODE_EXCEPTION,
    PDO::ATTR_DEFAULT_FETCH_MODE => PDO::FETCH_ASSOC,
    PDO::ATTR_EMULATE_PREPARES   => false,
];
try {
     $pdo = new PDO($dsn, $user, $pass, $options);
} catch (\PDOException $e) {
     throw new \PDOException($e->getMessage(), (int)$e->getCode());
}

Let’s define it inside the class

private $pdo = null;

I give up on “try {}” I’ll search another connection format.

[phdr]

I’m trying to adapt it by taking examples from existing codes.
First of all, the database connection format is wrong let me change it

All you are accomplishing by using this method is to go from one bad example to another bad example. This is not learning, it’s just wasting time trying different things have you found.

So, what’s wrong with the current connection code. The most serious is this -

} catch (\PDOException $e) {
     throw new \PDOException($e->getMessage(), (int)$e->getCode());
}

Did you read that code? It’s catching the PDOException, then throwing a new PDOException with two of the pieces of information from the PDOException you just caught.

This is ridiculous nonsense. It’s not doing anything useful. It’s just a waste of typing time, program memory, and execution time.

Unless your application code can do something to recover from a database statement error, there’s no point in having try/catch logic in your code. For a connection error, there’s nothing your application can do about it.

Some other points about the code and just trying different things you have seen -

1. You have changed the character set from utf8 (the first posted code) to utf8mb4 (the last posted code.) Which one of those matches your database table(s) character set? If you don’t set the character set of the connection to match your database table(s), you will get a character set conversion and end up with symbols displayed on your web site, rather than the correct characters.
2. While it is a minor point, variables should be named as to the meaning of the data in them. The variables holding the database connection parameters should be uniquely named to indicate they are for the database connection. What if your code already has $user and $pass variables at the point you add this connection code? This would overwrite those exiting variables with different values. When you only have a small amount of code, keeping track of variable names is easy. When you have applications with 10,000’s of lines of code, variable naming becomes critical to avoid unexplained problems in the code.

[Adem]

The next problem is you are not doing anything with the validation and just calling the query regardless. You are also outputting internal system errors to the user. That info is only good to hackers and useless to the user.

I don’t understand exactly what you mean here
Can you tell in more detail

Do not check if an email is available. Set a unique constraint on the column, attempt the insert, and catch the duplicate error if any. This would be a valid case for try/catch.

Is there an example of how I can do this?

Do not create variables for nothing.

I’ve never heard of it anywhere

Chaining a bunch of validations together is a very poor practice. How will you know which one failed? You have also duplicated validations.

How do I check for blank or incorrect data

[ErnieAlex]

Here are some basic ideas on validation. This site is not be best source for code, but, it will give you a lot of basic info to get started with. Once you create some code post it and we can help you further…
Validation Basics

[Pedroski55]

You guys should slow down, he. like me is a beginner, but even beginners can make it work.

Step 1: make a conn.php to connect to your db. i think you have that already.
Step 2: make an index.php as a form with the inputs you need.
Step 3: make a login.php to work with index.php. Click the button in index.php and login.php takes over.
Step 4: make a register_form.php and register.php to work together.
Step 5: make a change_pw_form.php and a change_pw.php to work together.

Save all these 7 files 1 folder, to start with at least.

Any time $_SESSION[‘loginerror’] is set, bump the user back to the login form ‘index.php’ and display the error, like ‘incorrect password’ or ‘incorrect email’:

<div id="div-blue">
	你好！Hi! If there is a problem, you will see a message here.<br>	
			<?php
				if(isset($_SESSION['loginerror'])){
				echo  "A login error occurred: " . $_SESSION['loginerror'] . "<br>";

					//unset error
					unset($_SESSION['loginerror']);					
				}				
			?>
		</div><br>

See the first comment from anonymous here. You can’t use variables for the table or column names, apparently prepare won’t like that.

In my case, to display a student’s scores and attendance, I need variables for $class and $week and $studentnum from the input form, because it is inconvenient to lump all classes together.

But I know what my input should be, so I can limit the possibilities for say, $class by defining an array:

$classes = array('19BE1', '19BE2', '19BE3');

and use html to limit the size of the input:

<input type="text" name="getclass" id="getclass" maxlength="5" size="35" placeholder="Write the name of your class here" required /><br>

As far as I know, an sql injection attack will need to be longer than 5 characters and would not be in my array. You could write DROP there in the field for “your class name”.

I like to keep this as simple as possible, so I can understand what is going on.

I put php timers on top of my files, because they should not be available outside of class time. Outside of class time, users will be lead to a javascript clock and a message.

If you would like some help beginner to beginner, I can explain it in a simple fashion.

[JimL]

As far as I know, an sql injection attack will need to be longer than 5 characters and would not be in my array. You could write DROP there in the field for “your class name”.

It’s important to note though that any limitation in HTML/JS is for user convenience only. A malicious user won’t care what limitations is set by the front end - in fact these limitations are often a good place to start poking at a system. Your back end code (PHP) will always have to validate all the data it receives.

[Pedroski55]

You are right of course, although I would not know how to alter the ‘maxlength=5’ on the fly, but if the hacker can hack into my webhost server and alter the php, I’ve lost the fight anyway!

I was just faced with the problem of variables for table names and week names. You can’t use pdo :placeholders there.

I’m glad I just run a homework page and not a bank!

[Adem]

Here are some basic ideas on validation. This site is not be best source for code, but, it will give you a lot of basic info to get started with. Once you create some code post it and we can help you further…
Validation Basics

I used this suggestion
The function in the example is at the bottom, but for me it worked when the fonction was on the top
What is the reason of this?

[ErnieAlex]

That was an example just to get you started. You should look into input filters. They are used in most cases to protect inputs from posted forms. You can file information for this all over the internet. Here is one that explains how to filter your inputs. PHP filter_input()

Basically, you want to make sure that visitors to your site can not enter code to erase your database or worse. Since code can be placed into input fields, you need to remove it. It is quite easy with the filter_input() function. Remember that a hacker can post to your site from a program and does not actually need to be on your site in a browser. Also, if something did or did not work for you, please show us the code and we can help. Can’t help if we don’t see the code…

[phdr]

The function in the example is at the bottom, but for me it worked when the fonction was on the top
What is the reason of this?

Php is a parsed, tokenized, interpreted language. As long as the function definition is in the main file and is not conditionally defined, it doesn’t matter where the function definition is placed at. If the function definition is in a ‘required’ file, it must come before the point where you call the function.

[Adem]

I tried to filter here with the example given above

    function clear($data) {
        $data = trim($data);
        $data = stripslashes($data);
        $data = htmlspecialchars($data);
        return $data;
      }

    $adi                    = clear($_POST["adi"]);
    $yetkili_adi_soyadi     = clear($_POST['yetkili_adi_soyadi']);
    $gsm                    = clear($_POST['gsm']);
    $telefon                = clear($_POST['telefon']);
    $websitesi              = clear($_POST['websitesi']);
    $sehir_ilce_semt        = clear($_POST['sehir_ilce_semt']);
    $filigran_metni         = clear($_POST['filigran_metni']);
    $adresi                 = clear($_POST['adresi']);
    $vergi_dairesi          = clear($_POST['vergi_dairesi']);
    $vergi_no               = clear($_POST['vergi_no']);

    $sql = "INSERT INTO kullanici (emaili, hash_user_password, adi, yetkili_adi_soyadi, gsm, telefon, websitesi, sehir_ilce_semt, filigran_metni, adresi, vergi_dairesi, vergi_no, group, logo) 
    VALUES (:emaili, :hash_user_password, :adi, :yetkili_adi_soyadi, :gsm, :telefon, :websitesi, :sehir_ilce_semt, :filigran_metni, :adresi, :vergi_dairesi, :vergi_no, :group, :logo)";
    $result = $PDO->prepare($sql);
    $values = array(':emaili'                 => filter_var(filter_input(INPUT_POST, 'emaili'), FILTER_VALIDATE_EMAIL),
                    ':hash_user_password'     => pas_hash(filter_input(INPUT_POST, 'user_password'), PASSWORD_DEFAULT),
                    ':adi'                    => $adi,
                    ':yetkili_adi_soyadi'     => $yetkili_adi_soyadi,
                    ':gsm'                    => $gsm ?? null,
                    ':telefon'                => $telefon ?? null,
                    ':websitesi'              => $websitesi ?? null,
                    ':sehir_ilce_semt'        => $sehir_ilce_semt,
                    ':filigran_metni'         => $filigran_metni ?? null,
                    ':adresi'                 => $adresi,
                    ':vergi_dairesi'          => $vergi_dairesi,
                    ':vergi_no'               => $vergi_no,
                    ':group'             => 0,
                    ':logo'                   => $_COOKIE['logo'] ?? null);

[ErnieAlex]

$data = stripslashes($data); $data = htmlspecialchars($data);

These two are handled by the filter_input() function. Therefore, it is a waste of time to call those in a fuction. It just adds extra code to handle on the server. I mean that this line:

$telefon                = clear($_POST['telefon']);

Calls a function which trims the data and clears it. But, this does the same thing without the function.

$telefon                = trim(filter_input(INPUT_POST, 'telefon');

It would make the server work less because functions must be located in the file each time it is called. And, that might make a lot of server processing if you have a large number of users online at the same time. Just something to think about.

[Adem]

Thank you very much, I understand the subject very well