5

I'm a student and this is my first time writing a piece of software. It's a web application on a LAMP stack, and as part of that web application, I wrote the following function to interact with the database when creating a new user:

public function CreateUser($username, $password, $email){
  global $DBHandler, $SQLStatement;
  $SQLStatement = $DBHandler->prepare("SELECT id FROM users WHERE username = :username AND verified > 0");
  $SQLStatement->bindParam(':username', $username);
  $SQLStatement->execute();
  $check = $SQLStatement->fetch();
  if ($check['id']){
    return 2;
  }else{
    $SQLStatement = $DBHandler->prepare("SELECT id FROM users WHERE email = :email AND verified > 0");
    $SQLStatement->bindParam(':email', $email);
    $SQLStatement->execute();
    $check = $SQLStatement->fetch();
    if ($check['id']){
      return 3;
    }else{
      /* Edited out code that generates a random verification code, a random salt, and hashes the password. */
      $SQLStatement = $DBHandler->prepare("INSERT INTO users (username, email, passwordhash, salt, verifycode) VALUES (:username, :email, :passwordhash, :salt, :verifycode)");
      $SQLStatement->bindParam(':username', $username);
      $SQLStatement->bindParam(':email', $email);
      $SQLStatement->bindParam(':passwordhash', $passwordhash);
      $SQLStatement->bindParam(':salt', $salt);
      $SQLStatement->bindParam(':verifycode', $verifycode);
      $SQLStatement->execute();
      return 1;
    }
  }
}

$DBHandler is initiated elsewhere as a PHP Data Object.

It follows these basic steps:

  1. Query the database to see if someone has already verified an account with the desired username.
  2. If the username is available, do another query and do the same check for email.
  3. If both the username and the email are available, generate verify code, salt, hash the password, do a third query to write everything to the database, and return 1 (meaning success).

The idea is that usernames and emails aren't reserved until your account is verified (verified = '1'). Elsewhere there's a script that changes your verified column to '1' if you click an emailed verification link.

My first question is this:

Person A is proposing username "Lorem", and person B has an unverified account that also proposes the username "Lorem". Is it possible for the queries to be executed in the following order?

  1. Person A's script determines the username "Lorem" isn't taken by a verified user
  2. Person B's script verifies their account with username "Lorem"
  3. Person A's script creates their unverified account with username "Lorem"

My second question is:

Is it possible to combine the 3 queries in this script into 1 query with the same if/else logic expressed in SQL instead of PHP, and if so, would that improve performance and/or prevent the above scenario from occurring?

ch7527
  • 203
  • 1
  • 6
  • What you're doing here is writing a very primitive Object Relationship Manager (ORM). Would using an [existing ORM](http://stackoverflow.com/questions/108699/good-php-orm-library) help or is this an academic exercise? – tadman Sep 16 '12 at 07:02
  • You should also not be using a global SQL statement object. Always create a new one as part of the function. – Tomalak Sep 16 '12 at 07:10
  • And I'd think storing the salt in the database kind of gives it away? – Andomar Sep 16 '12 at 07:19
  • I'd say the project is academic, but not an exercise. Part of the grade is creating a website that we personally want to create, that fulfills some need in our lives outside of class. Most of the other students are creating blogs and photo albums and things like that, but I'm writing a communications platform similar to a forum. – ch7527 Sep 16 '12 at 07:23
  • I did a ton of research on the salting and hashing because security fascinates me, and the general impression I got was that salts aren't meant to be secret, they're just meant to be unique for every user. So someone can get all the salts and the hashes, but because every salt is unique, they can't use rainbow tables and they still have to brute force every account individually. – ch7527 Sep 16 '12 at 07:28
  • I've heard of Doctrine and Propel for ORMs but I'm not really sure what they do... They were over my head a few weeks ago, but I think I might be able to understand them now. I'll definitely look into that. – ch7527 Sep 16 '12 at 07:32
  • I don't see the benefit of allowing multiple people to request the same username just because none of them have clicked the confirmation link yet. If this happened among a group of ten people, each of whom made it all the way to opening the confirmation email, I'd imagine the nine of them who weren't the fastest clicker would be a bit irritated for thinking they had an account, but were really just competitors in a race for the same username. – Fred Sobotka Sep 16 '12 at 07:48
  • I think you're right Fred. I wrote it this way to prevent scenarios where someone could reserve someone else's email without verifying it, just to prevent them from signing up. Or reserve a bunch of usernames just to prevent people from having them. This probably isn't the best way though, and I think I'll rework the system. – ch7527 Sep 16 '12 at 07:58

1 Answers1

2

For a more concurrent solution, you could make the insert conditional:

insert  into users 
        (username, email, ...)
select  :username, :email, ...
where   not exists
        (
        select  *
        from    users
        where   verified > 0
                and (username = :username
                     or email = :email)
        )

This is still not 100% safe in MySQL, but should suffice in practice.

Note that the problem of concurrency in user creation is a luxury problem. I'd just set up a basic version like the one you have, and focus on the more interesting aspects of your website!

Andomar
  • 232,371
  • 49
  • 380
  • 404
  • So just to be clear, the scenario I described could actually happen? I know it's really unlikely, but doesn't the possibility mean my code is definitely the wrong way of doing things? If I keep doing it this way, a concurrency like this could do all flavors of nasty stuff, couldn't it? – ch7527 Sep 16 '12 at 07:44
  • Yes that can happen, and it does in a lot of websites. It doesn't really matter because few websites have a large number of signups, and users can always retry using a different name. Rather than write user management code that tries to account for a lot of rare cases, you could consider using OpenID, like StackOverflow does. – Andomar Sep 16 '12 at 08:21