[kiasta]

I'm trying to finish my login system, but I hit a snag. It was suggested to me to use structured statements, and PDO was the best way to access mysql databases (though, the issue with terminating connections make me think otherwise). I'm having a little issue, since I just started learning PDO about an hour ago, I'm still not 100% confident on how to go about doing things properly. Anyways let me explain my problem, first the code:

private function verify_db()
    {
        $dbhost = 'host';
        $dbname = 'dbname';
        $dbuser = 'user';
        $dbpass = 'pass';
        try
        {
            $db = new PDO("mysql:host=$dbhost;dbname=$dbname", $dbuser, $dbpass );
        }
        catch(PDOException $err)
        {
            $err->getMessage();
        }
        foreach($db->query('SELECT * FROM username') as $row)
        {
            if ($row == 'username')
            {
                $this->set_priv('errCode', $row, EXISTS_USER);
                $db = NULL;
                return false;
            }
            if ($row == 'email')
            {
                $this->set_priv('errCode', $row, EXISTS_EMAIL);
                $db = NULL;
                return false;
            }
        }
        $db = NULL;
        return true;
    }

To my understanding, that looks correct. I'm trying to find a comparison between usernames and emails in the database, if there is one than the user exists and registration cannot continue. If not than registration can continue and the data will be stored on the database. Now while doing the comparison I am getting this error on the register.php page before even submitting the form:

Warning: Invalid argument supplied for foreach() in /hermes/waloraweb024/b1976/moo.mywebsite/development/authenticate.class.php on line 363
getVerification: 1

the getVerification is just a bool variable that is assigned if the user exists or not:

$getVerification = $this->verify_db();
return $getVerification;

That is the only time it is used, anywhere.

I can see that it has to do with the foreach statement but I really can't see what the problem is. It looks to me to be fine, I have a table called username in the db and according to the many tutorials (however generic they are) it looks perfectly fine to me.

Perhaps because I've been working on this for 15 hours straight my brain is melting away and I just can't see it, so any ideas? Suggestions or tips? I'm going to sleep on it and hopefully dream of a solution, but in the meantime I hope someone can offer a solution.

Oh and I know that I don't have the mysql_get_real_string() function, I took it out for debugging's sake. Thank you for your time

This post has been edited by kiasta: 20 October 2012 - 01:58 AM

[Dormilich]

there are several problems:
- line #11. what do you do if there was an exception? you dump the error message into the void and then proceed as if nothing would’ve happened.

- line #15, what if the query fails? PDO::query() returns false on failure, which is illegal as argument for foreach() (hence the error)

- line #15, why do you assume $row to be a string?

- what about DB error handling?


tips:
- always catch exception where you can handle the problem. that is usually not where the exception occurs. better enclose the complete PDO code with a single try…catch block

- enable PDO’s error handling (set to none by default)

// in the constructor
$options = array(
    PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION,
    PDO::ATTR_EMULATE_PREPARES => false
);
$pdo = new PDO($dsn, $user, $pass, $options);

// through a setter
$pdo = new PDO($dsn, $user, $pass);
$pdo->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
$pdo->setAttribute(PDO::ATTR_EMULATE_PREPARES, false);

- it may help to set a fetch mode (PDO has a lot of them).

This post has been edited by Dormilich: 20 October 2012 - 02:44 AM

[kiasta]

Dormilich, on 20 October 2012 - 05:43 AM, said:

there are several problems:
- line #11. what do you do if there was an exception? you dump the error message into the void and then proceed as if nothing would’ve happened.

Good point, I put the code inside the try/catch.

Quote

- line #15, what if the query fails? PDO::query() returns false on failure, which is illegal as argument for foreach() (hence the error)

Good point, bad coding practice on my part (I'm still new to php)

Quote

- line #15, why do you assume $row to be a string?

To be honest, I have no idea why I thought that. I must have been too tired, I was coding all night.

Quote

- what about DB error handling?


tips:
- always catch exception where you can handle the problem. that is usually not where the exception occurs. better enclose the complete PDO code with a single try…catch block

- enable PDO’s error handling (set to none by default)

// in the constructor
$options = array(
    PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION,
    PDO::ATTR_EMULATE_PREPARES => false
);
$pdo = new PDO($dsn, $user, $pass, $options);

// through a setter
$pdo = new PDO($dsn, $user, $pass);
$pdo->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
$pdo->setAttribute(PDO::ATTR_EMULATE_PREPARES, false);

- it may help to set a fetch mode (PDO has a lot of them).

Thanks for all the tips and for your question about db error handling, I still didn't know much about PDO nor the way it handles errors.

Here is my revised code, hopefully it doesn't make your eyes bleed as much, heh.

private function write_db ()
    {
        $dbHost   = 'host';
        $dbName   = 'name';
        $dbUser   = 'user';
        $dbPass   = 'pass';
        $DSN      = "mysql:host=$dbHost;dbname=$dbName"; //will need to add port later, not working for now
        $username = $this -> get_priv ( 'finData' , 'username' );
        $email    = $this -> get_priv ( 'finData' , 'email' );
        $salt     = $this -> create_salt ();
        $password = $this -> hash_password ( $this -> get_priv ( 'finData' , 'password' ), $salt );
        try
        {
            $db = new PDO ( $DSN , $dbUser , $dbPass );
            $db -> setAttribute ( PDO::ATTR_ERRMODE , PDO::ERRMODE_EXCEPTION );
            $db -> setAttribute ( PDO::ATTR_EMULATE_PREPARES , false );

            $stmt = $db -> prepare ( "SELECT username, email FROM users" );
            while ( $row  = $stmt -> fetch () )
            {
                for ( $i = 0; $i < count ( $row ); $i ++  )
                {
                    if ( $row[ $i ] == $username )
                    {
                        $this -> set_priv ( 'errCode' , 'username' , EXISTS_USER );
                    }
                    if ( $row[ $i ] == $email )
                    {
                        $this -> set_priv ( 'errCode' , 'email' , EXISTS_EMAIL );
                    }
                }
            }
            if ( $this -> get_priv ( 'errCode' , 'username' ) != EXISTS_USER ||
                    $this -> get_priv ( 'errCode' , 'email' ) != EXISTS_EMAIL )
            {
                $sql = "INSERT INTO `users` (username, email, password, salt)
                VALUES ('$username', '$email', '$password', '$salt')";
                $db -> exec ( $sql );
                return true;
            }
            else
            {
                return false;
            }
            $db = NULL; //to close connection
        }
        catch ( PDOException $err )
        {
            $err -> getMessage ();
        }
    }

Now I do have a small question: How would I implement this function...

mysql_real_escape_string()

...into my code? When I try to use it I get errors. I have made my own custom function to catch bad characters for the username, only the email is open for sql injection. How would I avoid that since I can't use that function? E-mails are handled a bit differently and I can't control what characters are allowed in an email so to code only alphanumeric for emails is not probable.

[JackOfAllTrades]

You don't use mysql_real_escape_string(), you use the prepared statements functionality provided by PDO.

[Atli]

Here are two things to consider:

• In that last code, you forget to execute() the SELECT query. You go straight from prepare() to fetch(). The execute() step should come between those two.
  
  
• Your SELECT query is rather wasteful. You're looking to see if a specific username and email exists, so why not ask the database that? There is no reason to fetch ALL the data from the database and look through it yourself when the database can look for you much faster.
  
  With that said, if you want to count both the username and the email in one query, it complicates matters a bit. But not much. You've got a couple of options, actually. I'd suggest just doing something like this:
  

  SELECT
      (SELECT COUNT(*) FROM users WHERE username = ?) AS 'user_exists',
      (SELECT COUNT(*) FROM users WHERE email = ?) AS 'email_exists';
  
  

  
  Then you can just read the two fields it returns as boolean values.