Create user script

[TomasV]

I can't find out whats the problem here, would appreciate some input in how to think building my "if".

The problem is that I don't seem to catch if an email exists, nor if user exists and neither can I create a new user :/.

 

Appreciate your help alot!

<?php

	// Start the session in case of errors to display within the page of user creation
	session_start();
	$err_msg = array();
	$errflag = false;



	// Check if the submit button was pressed
	if ($_SERVER['REQUEST_METHOD'] === 'POST'
		&& $_POST['submit'] === 'Skapa')
	{
		// Crypt password
		$options = ['cost' => 10];

		$username = strip_tags($_POST['uname']);
		$password = strip_tags(password_hash($_POST['pword'], PASSWORD_DEFAULT, $options));
		$email = strip_tags($_POST['uname'], '@');

		// Check so all the fields are filled
		if ($_POST['uname'] == '' || $_POST['pword'] == '' || $_POST['pwordcheck'] == '')
		{
			$err_msg[] = 'Please enter all fields<br>';
			$errflag = true;
		}

		// See if passwords and confirm matches
		if ($_POST['pword'] !== $_POST['pwordcheck'])
		{
			$err_msg[] = 'Passwords doesn\'t match!<br>';
			$errflag = true;
		}

		// Check password length, atleast 8 characters
		if (strlen($_POST['pword']) < 7)
		{
			$err_msg[] = 'Password must be atleast 8 characters long';
			$errflag = true;
		}

		// Check if email exists
		include_once('../includes/db.inc.php');
		$db = new PDO(DB_INFO, DB_USER, DB_PASS);

		$sql = "SELECT COUNT(*) AS count FROM movies WHERE email = :emailadress";
			$stmt = $db->prepare($sql);
			$stmt->bindParam(':emailadress', $email);
			$stmt->execute();
			$row = $stmt->fetch(PDO::FETCH_ASSOC);

		if ($row > 0)
		{
			$err_msg[] = 'Email already taken!';
			$errflag = true;
			$db = NULL;
		}
		

		// Check if user exists
		include_once('../includes/db.inc.php');
		$db = new PDO(DB_INFO, DB_USER, DB_PASS);

		$sql = "SELECT uname FROM users WHERE uname = :username";
			$stmt = $db->prepare($sql);
			$stmt->bindParam(':username', $username);
			$stmt->execute();
			$row = $stmt->fetch(PDO::FETCH_ASSOC);

		if ($row > 0)
		{
			$err_msg[] = 'User already exists';
			$errflag = true;
			$db = NULL;
		}

		if ($errflag = false)
		{
			// Everything passed, create the user!
			include_once('../includes/db.inc.php');
			$db = new PDO(DB_INFO, DB_USER, DB_PASS);

			$sql = "INSERT INTO users (uname, pword, email) VALUES (:username, :password, :emailadress)";
				$stmt = $db->prepare($sql);
				$stmt->bindParam(':username', $username);
				$stmt->bindParam(':password', $password);
				$stmt->bindParam(':emailadress');
				$stmt->execute();

			$_SESSION['uname'] = $username;
			header('Location: ../template/header.php');
			exit;
		}
				
		// If any error, send the user back and display messages
		if ($errflag == true)
		{
			$_SESSION['err_msg'] = $err_msg;
			session_write_close();
			header('Location: ../user/create.php');
			exit;
		}
	}

	else
	{
		$_SESSION['err_msg'] = $err_msg;
			session_write_close();
			header('Location: ../user/create.php');
		exit;
	}

?>

[QuickOldCar]

if ($_POST['pword'] !== $_POST['pwordcheck'])

 

Is this checking a password in the form to an unhashed password saved from database?

 

Reason I ask is because the $_POST['pword'] isn't hashed until you made it $password

$password = strip_tags(password_hash($_POST['pword'], PASSWORD_DEFAULT, $options));

Edited June 5, 2014 by QuickOldCar

[Jacques1]

@ QuickOldCar: The code doesn't check password hashes at all. It's the registration procedure, and this simply compares the password field with the “Please retype your password” field.

 

 

 

@ TomasV:

 

First of all: Congratulations for being one of the very, very few members who actually uses up-to-date code with PDO and bcrypt. Normally, we have to start every reply with a long explanation of why the mysql_* functions shouldn't be used, why plaintext passwords or MD5 hashes are not appropriate etc. Looks like this is one of the rare occasions where we can skip that part.

 

There are several problems, though.

 

Are you sure you want to query a table called “movies” to check if the e-mail address is in use? Why movies? Also note that you do a COUNT(*) query and then check if the result set is empty. A COUNT(*) query always yields a row, so this check doesn't work and might be the bug you're looking for.

 

But this isn't really the right approach, anyway. You cannot enforce unique database entries using PHP. Just because there were no rows when you checked doesn't mean this is still true when you actually insert a new row. What if two people try to register with the same name at almost the same time? Then they're both allowed to have it:

 user A      | user B    
-------------+-------------
             |
check name:  |
not used yet |
             |
             | check name:
             | not used yet
             |
insert name  |
             |
             | insert name

The solution is to let the database system take care of uniqueness checks: Add a UNIQUE constraint to all columns that should have unique values. In your application, simply try to insert the row, and if that fails due to a constraint violation, you know the name is already taken.

 

You shouldn't establish a new database connection for every single query. This makes no sense and is extremely inefficient. Simply connect to the database once on top of the script and then use this connection for all queries.

 

It's also important that you configure PDO correctly. Currently, you have no error reporting at all. You'd have to manually check for errors, but you don't do that either. The default configuration is actually downright dangerous, because PDO will use client-side escaping instead of actual prepared statement.

 

So start by creating a separate script for the database connection (and one for configuration values):

 

config.php

<?php


define('DATABASE_HOST', 'localhost');
define('DATABASE_USERNAME', 'myapp_default_user');
define('DATABASE_PASSWORD', '823da81d4ce2b7c4a0c11bbc9343aa73');
define('DATABASE_CHARSET', 'utf8mb4');
define('DATABASE_NAME', 'myapp');

database.php

<?php


function connect_to_database()
{
    static $connection;

    // Make sure to only connect once, even if the function is called multiple times
    if (!$connection)
    {
        $db_options = array(
            // use actual prepared statements instead of client-side escaping
            PDO::ATTR_EMULATE_PREPARES => false,
            // throw an exception when a query fails
            PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION,
            // optional: fetch associative arrays by default
            PDO::ATTR_DEFAULT_FETCH_MODE => PDO::FETCH_ASSOC,
        );

        $connection = new PDO('mysql:host='.DATABASE_HOST.';dbname='.DATABASE_NAME.';charset='.DATABASE_CHARSET, DATABASE_USERNAME, DATABASE_PASSWORD, $db_options);
    }

    return $connection;
}

Include those files and establish a database connection on top of your script:

<?php

require_once __DIR__.'/config.php';
require_once __DIR__.'/database.php';


$database = connect_to_database();

Then add the UNIQUE constraints and check for violations as explained above:

// MySQL error codes
define('MYSQL_ER_DUP_ENTRY', 1062);



$regstration_stmt = $database->prepare('
    INSERT INTO
        users (uname, pword)
    VALUES
        (:username, :password)
');

/*
 * Try to insert the user. If that fails, check if it's due to a violation of the
 * unique constraint. In that case, we know that the username is already taken.
 */
try
{
    $regstration_stmt->execute(array(
        'username' => $username,
        'password' => $password,
    ));

}
catch (PDOException $registration_error)
{
    // check the error code to see what went wrong
    if ($registration_error->errorInfo[1] == MYSQL_ER_DUP_ENTRY)
    {
        echo 'Sorry, this name is already taken.';
    }
    else
    {
        // nothing to do for us, pass on the exception
        throw $registration_error;
    }
}

There are some other things, but I guess you do it step by step.

[TomasV]

Thanks a bunch for your reply!

 

Taken me two weeks to get the script working somewhat and your input really gave me some thoughts .

 

I'm up and running now with some improvements. Trying to get a hold of a good way of letting users upload profile pictures.

Is there any recommendations to a tutorial you seem fit for a newbie like me?

 

There are other things, but I'll try solve them the best I can on my own for now. I do appreciate your help alot, explained it very detailed and easy to understand!

[Jacques1]

I'm up and running now with some improvements. Trying to get a hold of a good way of letting users upload profile pictures.

Is there any recommendations to a tutorial you seem fit for a newbie like me?

 

I don't have a concrete tutorial at hand, but if you search for something like “secure file upload”, you'll get a good overview.

 

Unfortunately, file uploads are tricky. It seems trivial to save some files on your server, but doing this correctly is pretty hard. Even experienced developers often get this wrong. You're facing two major risks: People may upload server-side scripts and use them to attack you. Or they might upload client-side scripts and attack your users.

 

To prevent attackers from uploading and executing server-side scripts, you need to take the following steps:

• Do not let the user choose the filename. Make a whitelist of acceptable extensions (e. g., “.jpg”, “.jpeg”, “.gif” and “.png”) and validate the uploaded file against this list. If it's valid, then you assemble the filename from the user ID and the extension. Otherwise, you reject the file.
• Make sure your webserver will never execute scripts within the upload folder.
• As a more robust alternative, save the uploaded files outside of the document root and serve them through an extra script.

Preventing attacks against your users is even harder. What's important to understand is that even a perfectly valid image can contain malicious JavaScript code. For example, the JPEG format supports arbitrary text comments. If an attacker creates such an image, every validator and even manual inspection will tell you that it's fine. Yet still it's a script waiting to be executed by the victim.

 

Take the following steps:

• If there's any chance you can get a separate domain, use that to serve the uploaded images. It's one of the few reliable counter-measures, because the same-origin policy will isolate the attack on this domain and not let JavaScript access your actual site.
• Make sure the files are served with the correct MIME type (like image/jpeg). This is what makes the difference between a harmless image and malicious JavaScript code.
• Use Content Security Policy to tell modern browsers that no scripts should be executed.

Note that some particularly shitty browsers like IE before version 8 will still execute embedded scripts, even if you explicitly tell them to interpret the file as an image. The only barrier in this case would be the same-origin policy, so definitely try to get an extra domain.

Edited June 10, 2014 by Jacques1