Why doesn't the if statement work within this loop? PHP

Question

I have a while loop that goes through the rows returned from an SQL query. The values of a particular column from that row are stored in an array. The array is then iterated through and each element is compared with the input from the user. If the input matches an array element then a boolean becomes true. I'm trying to do this so that the user can enter a password to access a particular page. However it just doesn't work. I have printed all of the values from the array as well as the input, so I know that there isn't a problem there. But for some reason, the if statement just doesn't compare them. Here is the code:

if (isset( $_POST['ok'])) {
  $password = $_POST['pass'];
  $matched = false;
  $pw = array();
  mysql_connect("localhost", "xxx", "xxx")or die("Error");
  mysql_select_db("details")or die("Error");
  $query="SELECT * FROM members";
  $result=mysql_query($query);
  while ($row = mysql_fetch_assoc($result) ){
    $pw[] = $row["pass"];
  }
  foreach($pw as $p){
    if(strcmp($p, $password) == 0){
      $matched = true;
    }
  }
  if ($matched==true) {
    //Membership page
  } else {
    //Error message
  }
} else {
  ....

try change your loop to foreach($pw as $p){ if($p == $password){ $matched = true;break; } } — Itay Moav -Malimovka
– Itay Moav -Malimovka, Commented Nov 17, 2012 at 14:13
And don't forget to NEVER store plain passwords, use a hashing algorithm on them ! — HamZa
– HamZa, Commented Nov 17, 2012 at 14:15
@HamZaDzCyberDeV - This is just a basic version for testing, the real one will have a hashing algorithm — M9A
– M9A, Commented Nov 17, 2012 at 14:18

Michel Feldheim · Accepted Answer · 2012-11-17 14:46:30Z

1

It would be much easier and efficient to change your query to something like this

$dbh = mysql_connect("localhost", "xxx", "xxx") or die("Error");
mysql_select_db("details", $dbh ) or die("Error");

$pass = mysql_real_escape_string( $_POST['pass'], $dbh );
$user = mysql_real_escape_string( $_POST['user'], $dbh );

$sqlQuery = <<< EOQ
    SELECT
        *
    FROM
        `members`
    WHERE
        `user` COLLATE utf8_bin = '{$user}' COLLATE utf8_bin
        AND
        `password` COLLATE utf8_bin = '{$pass}' COLLATE utf8_bin
EOQ;

$result = mysql_query( $sqlQuery );
if ( $result and ( mysql_num_rows( $result ) === 1 ) {
       echo "success";
       $userDetails = mysql_fetch_assoc( $result );
} else {
       echo "username or password wrong";
}

Edit: updated the password and username check to be case sensitive in any case

Edit2: above comments remind not to store passwords plaintext. To change to hashed passwords

UPDATE members SET pass = SHA1( pass );

Then change your check to

... AND pass = SHA1( '{$pass}' )

edited Nov 17, 2012 at 14:46

answered Nov 17, 2012 at 14:19

Michel Feldheim

18.3k5 gold badges64 silver badges77 bronze badges

Sign up to request clarification or add additional context in comments.

4 Comments

M9A Over a year ago

The reason I am not doing it that way is because I will need to store the column values in an array as they will be used again later on

Michel Feldheim Over a year ago

if you need all user records later fetch them in addition. especially in this case keep it clean and simple and separate things which do not belong here ( separate authentication and other user data processing )

leemes Over a year ago

I don't understand why people still use the outdated mysql module with escaping functions instead of the mysqli module or PDO with prepared statements...

Michel Feldheim Over a year ago

Mysqli indeed is the better adapter. Prepared statements are misused for escaping commonly. Escaping only is a positive side effect of prepared statements but not the original idea.

Richard Chambers · Accepted Answer · 2012-11-17 14:19:22Z

0

You need a break after finding a match so that $matched will be equal to true.

if ( isset( $_POST['ok'] ) ) {

$password = $_POST['pass'];
$matched = false;
$pw = array();

mysql_connect("localhost", "xxx", "xxx")or die("Error");
mysql_select_db("details")or die("Error");
$query="SELECT * FROM members";
$result=mysql_query($query);

while ($row = mysql_fetch_assoc($result) ){
$pw[] = $row["pass"];
}

foreach($pw as $p){
  if(strcmp($p, $password) == 0){
  $matched = true;    // found match so break out and do the membership.
  break;
}
}

    if ($matched==true) {

      //Memebrship page

    } else {

      //Error message
    }

} else {

....

answered Nov 17, 2012 at 14:19

Richard Chambers

17.9k4 gold badges96 silver badges121 bronze badges

2 Comments

Michel Feldheim Over a year ago

that would execute the script faster but not change anything in the resulting behaviour since he doesn't have an else { $matched = false } block in the second (unnecessary) loop

M9A Over a year ago

Unfortunatley this doesnt do anything

ro0ter · Accepted Answer · 2012-11-17 14:26:27Z

0

Sugestions:

1) Replace the direct mysql function calls with PDO: ( this will not require any escaping, since the PDO will handle everything )

$mysql_host = "localhost";
$mysql_user = "xxx";
$mysql_password = "xxx";
$mysql_database = "details";
$dbLink = new PDO("mysql:host=$mysql_host;dbname=$mysql_database;charset=utf8", $mysql_user, $mysql_password, array(PDO::ATTR_PERSISTENT => true));
$query = db()->prepare("select * from members WHERE pass = ? limit 1");
$query->execute(array($_POST['pass']));
$query->setFetchMode(PDO::FETCH_ASSOC);
$myMember = $query->fetch();
$query->closeCursor();

2) If you want to stick with your code, you could use $pwd = mysql_real_escape_string($_POSt['pass']) for the posted password and then select the row containing the escaped received password $pwd. Also, do not forget mysql_free_result($result);!!!

3) Make a hash of the password therefore you will not need to use mysql_real_escape_string. use $pwHash = md5($_POST['pass']) or $pwHash = sha1($_POST['pass']) or any combination.

4) Please align your code. It will make it more readable for people answering your questions (offering help) and also for future maintenance (you or someone else; believe me, you`ll forget the code in 2-3 years).

5) Your code should work, I'm not sure why it doesn't. Try adding var_dump for $pw and also write something on the screen when the password matches. Maybe you swapped the pages (members with error)

answered Nov 17, 2012 at 14:26

ro0ter

4392 silver badges11 bronze badges

3 Comments

M9A Over a year ago

I added var_dump for $pw, and I also did var_dump for the input text....and from that I could see that the array contains the input string. So I know that there a match, it just seems like it isnt doing the actual comparison and then changing the boolean to true

ro0ter Over a year ago

what character encoding are you using when connecting to DB? will it be possible that the character encoding to alter the comparison?

ro0ter Over a year ago

also, try echoing the parameters you are comparing with strcmp, inside the for loop. see where you get. you could add a echo inside the for where you set the matched to true to convince yourself that the code works well

HamZa · Accepted Answer · 2012-11-17 14:44:02Z

0

Why the foreach loop ? You can do it like this:

if (isset( $_POST['ok'])) {
  $password = $_POST['pass'];
  $matched = false;
  $pw = array();
  mysql_connect("localhost", "xxx", "xxx")or die("Error");
  mysql_select_db("details")or die("Error");
  $query="SELECT * FROM members";
  $result=mysql_query($query);
  while ($row = mysql_fetch_assoc($result) ){
    $pw[] = $row["pass"];
  }
   $pw_tmp = flip_array($pw);

   if(isset($pw_tmp[$password])){
      //Membership page
   }else{
      //Error message
   }
}else{
  // something else ...
}

edited Nov 17, 2012 at 14:44

answered Nov 17, 2012 at 14:38

HamZa

15k11 gold badges56 silver badges75 bronze badges

Collectives™ on Stack Overflow

Why doesn't the if statement work within this loop? PHP

4 Answers 4

4 Comments

2 Comments

3 Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

4 Answers 4

4 Comments

2 Comments

3 Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related