coding the right way

[I-AM-OBODO]

Hi all.

 

My code runs fine but one problem: i want the "user is invalid" to validate last. i don't know why when i lleave the security code empty and click submit, it brings out two validation error (1) security number missing (2) invalid user, but the idea is for the invalid user to be validated last. what could be the problem. (hope i am getting my question right.

 

secondly, i believe my coding needs prunning (not the right way) i wouldnt mind a correction on how to do it rightly.

 

thanks

 

code below

<?php

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

   //post variables
   $sec_no = mysql_real_escape_string(stripslashes($_POST['sec_no']));
   $uname = mysql_real_escape_string(stripslashes($_POST['uname']));

   //validate entry
   if($sec_no == ''){
       //$er1 = "<font color=red><b>Security Number Missing</b></font><br>";
       echo "<font color=red><b>Security Number Missing</b></font><br>";
   }
   if($uname == ''){
       //$er2 = "<font color='red'><b>Username Missing</b></font>";
       echo "<font color='red'><b>Username Missing</b></font>";

   }else{

   // check for valid user
   $q = "SELECT * FROM reg_users WHERE username = '$uname' AND Security_no = '$sec_no'";
   $r = mysql_query($q);

   if(mysql_num_rows($r)==1){

       $row = mysql_fetch_array($r);

       if($row){

           $p = substr ( md5(uniqid(rand(),1)), 3, 10);


           $q = "UPDATE reg_users SET password = '".md5('$p')."' WHERE username = '$uname' AND Security_no = '$sec_no'";
           $r = mysql_query($q);

           if($r){
               echo "<font color='blue'><b>password changed</b></font>";
           }else{
               echo "<font color='red'><b>password not changed this time</b></font>";
           }
       }

   }    else{
       echo "<font color='red'><b>user is invalid</b></font>";
   }
   }
   }
?>

[kicken]

 i don't know why when i lleave the security code empty and click submit, it brings out two validation error 

Because you're running two separate and independent checks.  Your check for the user is not connected in any way to your check for the security number.

 if($sec_no == ''){
   ...
 }

 if($uname == ''){
 }

 

Those are separate independent if conditions, both will run.

 

You could change the second if to an else if which would cause the checks to be serialized so you'd only get back a single error at time, as in:

 - if the security # is empty, show just that error

 - otherwise if the username is empty show just that error

 - otherwise check the valid user

 

 

Alternativly you could only check the valid user if the first two validations pass.  Use a flag to keep track of that.  eg:

$hasErrors=false;
if($sec_no == ''){ $hasErrors=true; ...}
if($uname == ''){ $hasErrors=true; ...}

if (!$hasErrors){
  //check valid user
}

[denno020]

I'm pretty sure you can just make your user validation an else if of your security number validation. That will ensure only one of each will run, IF they're incorrect. If they're both correct, then your else to user validation will run as per usual.

 

if($sec_no == ''){
 //$er1 = "<font color=red><b>Security Number Missing</b></font><br>";
 echo "<font color=red><b>Security Number Missing</b></font><br>";
}else if($uname == ''){
 //$er2 = "<font color='red'><b>Username Missing</b></font>";
 echo "<font color='red'><b>Username Missing</b></font>";                
}else{
 // check for valid user
 //etc....

 

That's all you need, just add that else before your user validation 'if', and the user validation will only be checked if the security code validates and passes.

[I-AM-OBODO]

Thanks all.

Maybe i am not making myself clear enough. I know both the security number and username will validate at the same time and that is exactly how i want it to run. The problem however is:

echo "<font color='red'><b>user is invalid</b></font>

 

I actually want it to validate if all other conditions are false but it validates even when the security number is empty which is not what i want.

 

thanks

[Jessica]

Alternativly you could only check the valid user if the first two validations pass.  Use a flag to keep track of that.  eg:

$hasErrors=false;
if($sec_no == ''){ $hasErrors=true; ...}
if($uname == ''){ $hasErrors=true; ...}

if (!$hasErrors){
  //check valid user
}

[I-AM-OBODO]

Thanks all.

[I-AM-OBODO]

What's the difference btw my method ( if($uname == ""){

echo " user empty ";

}

and

$hasError = false;

if($uname == ""){

 

$hasError = true;

})

is there any security threat or anything that mars the code or mine is a bad practice? just curious to know so as to refrain from the practice.

 

thanks

[kicken]

Your code didn't accomplish what you wanted.  Other than that, it's fine.

[Christian F.]

That said, your code showcases a symptom of something which is very likely to cause you some issues. Not necessarily security related, but functional challenges which you don't need to have.

 

In short by echoing out content like that in the middle of the validation, indicates that you're mixing your (processing) PHP code with your code. So that you have some lines of HTML which starts to build a section of the page, then some lines of PHP which does some processing of the data, then some more HTML code, and so forth.

Problem with this approach is mainly two-fold:

1. It prevents you from sending any more HTTP headers, or any other content which has been sent to the client. Meaning that stuff like redirects, cookies (and thus sessions) and so forth becomes a lot more difficult to implement.
   
2. By mixing the presentation logic and the processing logic, and not to mention two different languages with two different levels of depth logic (indentation), you are making your script a lot more difficult to read and thus maintain. This is especially true if you're echoing content from within functions, which means you cannot alter the PHP code without also altering your layout (or vice versa).

 

What you should do is to put all of the (processing) PHP code at the top of the page, use variables to store the output, and not send any HTML code to the client before you've completed all of the processing. The dynamically generated content you can easily echo out wherever you want it, and you will not need to re-arrange your PHP code at all to move stuff around.

[I-AM-OBODO]

well said. thanks.