OPINIONS WANTED

[shane18]

This is my login page code, and I want your opinion on it please!

 

  // Login ~ CHECKS THE (USERNAME/PASSWORD) ENTERED BY THE USER THEN EITHER GRANTS ACCESS OR DENIEDS ACCESS
  case "Login":
    if(!preg_match("/^[a-z0-9]{2,20}$/i", $_POST["F_1_Login_Username"]) || !preg_match("/^[a-z0-9]{2,20}$/i", $_POST["F_1_Login_Password"])){
  $Login_Error = "INCORRECT ACCOUNT INFORMATION";
}
if(!$Login_Error){
      $Login_Username = $DB->real_escape_string($_POST["F_1_Login_Username"]);
      $Login_Password = $DB->real_escape_string(md5($_POST["F_1_Login_Password"]));
      $Login_Check = $DB->query("SELECT id,username,account_status,suspended_timestamp FROM members WHERE username='$Login_Username' && password='$Login_Password'");
      $Login_Status = $Login_Check->num_rows;
      $Login_Information = $Login_Check->fetch_object();
      if($Login_Status){
        if($Login_Information->account_status == 0){
          if(!QUICK_STATUS_CHECK($DB,$Login_Information->id,$TIMESTAMP)){
            $_SESSION["USER_ID"] = $Login_Information->id;
            $USER_ID = $Login_Information->id;
            $DB->query("UPDATE members SET last_action='$TIMESTAMP' WHERE id='$USER_ID'");
          }else{
            $Login_Error = "YOU ARE CURRENTLY LOGGED IN ALREADY";
          }
        }elseif($Login_Information->account_status == 1){
          if($Login_Information->suspended_timestamp < $TIMESTAMP){
            if(!QUICK_STATUS_CHECK($DB,$Login_Information->id,$TIMESTAMP)){
              $_SESSION["USER_ID"] = $Login_Information->id;
              $USER_ID = $Login_Information->id;
              $DB->query("UPDATE members SET account_status='0' WHERE id='$USER_ID'");
              $DB->query("UPDATE members SET last_action='$TIMESTAMP' WHERE id='$USER_ID'");
            }else{
              $Login_Error = "YOU ARE CURRENTLY LOGGED IN ALREADY";
            }
          }else{
            $Login_Error = "ACCOUNT SUSPENDED FOR 24 HOURS";
          }
        }else{
          $Login_Error = "ACCOUNT BANNED";
        }
      }else{
        $Login_Error = "INCORRECT ACCOUNT INFORMATION";
      }
}
if($USER_ID){
  TEMPLATE(0,0);
      SMOOTH_REDIRECT("Redirecting","index.php");
  TEMPLATE(1,0);
    }else{
      LOGIN_FORCE($USER_ID, $Login_Error);
    }
  break;

 

The code above works 100%, I just want to see if I missed any security things or programmed it crappy.

[mikesta707]

Looks fine to me. One thing tho, when you create an MD5 hash of a password, you don't need to use real escape strings on it, because you could get some unwanted results. if someone puts a ' in the password, then its md5 hash will be different \'. Besides that, and stylistic differences I might have had, it looks good

 

 

[shane18]

Doesn't it MD5 first before its escaped?

[oni-kun]

Doesn't it MD5 first before its escaped?

$DB->real_escape_string(md5($_POST["F_1_Login_Password"]));

 

You're right, but it's wasting a call to an instantiated class, and function, MD5 will never return a quotation!

[shane18]

Thanks for all of your opinions! I made the needed changes.

[ignace]

Don't perform a real_escape_string on an md5 it will mess up the hash. Hashing it alone is sufficient.

[Baseball]

Don't perform a real_escape_string on an md5 it will mess up the hash. Hashing it alone is sufficient.

 

Did u not read the other replies?

[ignace]

First off it's none of your concerns wether or not I read the other replies I'm free to post whatever I want just like mods/admins are free to moderate my post/ban my ass. Mods/Admins not you

Second your answer is off-topic and has no value for the OP which is worse?

Third I said the same as oni-kun only simpler

Fourth after a second look I realize I was confused and mysql_real_escape_string does actually nothing to the md5 string, my apologies (to the OP)

[JREAM]

This is very messy!

 

1. I would look up Naming Conventions, you have very hard to read variables, no need to use Login_User, when you are using the _ for a spacer, you could just use login_user; alternatively without the _ spacer you could do loginUser (camelCase).

2. Too many nested if/else statements!

3. Possibly break things into a few more functions

4. Make comments in your code, when you come back to this in a few months I think you will be very confused!

5. Your naming is confusing, perhaps rename your database logic to differentiate between the two (SysCheck rather than LoginCheck, and space out your query to be readable)

      $SysCheck = $DB->query("
		SELECT 	id,username,account_status,suspended_timestamp 
		FROM 	members 
		WHERE 	username='$Login_Username' 
		&& 		password='$Login_Password'");

  /** Login Status counts if a user exists with user/password combo */
      $Login_Status = $SysCheck->num_rows;
      $Login_Information = $SysCheck->fetch_object();

 

6. $DB->real_escape_string is unnecessarily long to type out, why not just name the method $DB->clean or something, I can't imagine typing those underscores so much is that enjoyable!

 

7. An example function

  function ValidUser($str)
  {
/** Returns a Boolean */
return preg_match("/^[a-z0-9]{2,20}$/i", $str) || preg_match("/^[a-z0-9]{2,20}$/i", $str);
  }
  
  
case "Login":

if(!ValidUser($_POST["F_1_Login_Username"]) {
	$Login_Error = "INCORRECT ACCOUNT INFORMATION";
}

[shane18]

Thanks for your opinion!

[shane18]

BTW, I name my variables in a certain way... if a variable is created in a page.. it always goes $PAGENAME_variablename..... like functions go like $F_FUNCTION#_variablename.....