0
\$\begingroup\$

How can I improve/secure my login script and how to check for any possible injection?

PS. the script must run on multiple platforms, so I need empty arrays for cases such as the Android ones.

UPDATES

  1. mysql structure
  2. php switch cases instead of UNION

user_table:

CREATE TABLE `user_table` (
`user_id` int(11) NOT NULL AUTO_INCREMENT,
`user_type` varchar(255) NOT NULL,
`user_email` varchar(255) NOT NULL,
`user_pass ` varchar(255) NOT NULL,
 PRIMARY KEY (`user_id`)
) ENGINE=InnoDB AUTO_INCREMENT=63 DEFAULT CHARSET=latin1
Field Type Null Key Default Extra
user_id int(11) NO PRI NULL auto_increment
user_type varchar(255) NO NULL
user_email varchar(255) NO NULL
user_pass varchar(255) NO NULL

student_table:

CREATE TABLE `student_table` (
`user_id` int(11) NOT NULL,
`user_type` varchar(255) NOT NULL,
PRIMARY KEY (`user_id`)
) ENGINE=InnoDB DEFAULT CHARSET=latin1
Field Type Null Key Default Extra
user_id int(11) NO PRI NULL
user_type varchar(255) NO NULL

teacher_table:

 CREATE TABLE `teacher_table` (
`user_id` int(11) NOT NULL,
`Class` varchar(255) NOT NULL,
`DEPARTMENT` varchar(255) NOT NULL,
PRIMARY KEY (`user_id`)
) ENGINE=InnoDB DEFAULT CHARSET=latin1
Field Type Null Key Default Extra
user_id int(11) NO PRI NULL
Class varchar(255) NO NULL
DEPARTMENT varchar(255) NO NULL

management_table:

CREATE TABLE `management_table` (
`user_id` int(11) NOT NULL,
`Class` varchar(255) NOT NULL,
`DEPARTMENT` varchar(255) NOT NULL,
`HEAD` varchar(255) NOT NULL,
PRIMARY KEY (`user_id`)
) ENGINE=InnoDB DEFAULT CHARSET=latin1
Field Type Null Key Default Extra
user_id int(11) NO PRI NULL
Class varchar(255) NO NULL
DEPARTMENT varchar(255) NO NULL
HEAD varchar(255) NO NULL

user_status table:

CREATE TABLE `user_status` (
`user_id` int(11) NOT NULL,
`Login_id` int(11) NOT NULL,
`user_token` varchar(255) NOT NULL,
`user_status` varchar(255) NOT NULL,
`time_stamp` timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP,
PRIMARY KEY (`user_id`)
) ENGINE=InnoDB DEFAULT CHARSET=latin1
Field Type Null Key Default Extra
user_id int(11) NO NULL
Login_id int(11) NO PRI NULL auto_increment
user_token varchar(255) NO NULL
user_status varchar(255) NO NULL
time_stamp timestamp NO CURRENT_TIMESTAMP on update CURRENT_TIMESTAMP

login script:

 <?php
     //filter email var before connecting to database
     function validateEmail($email) {
         if(filter_var($email, FILTER_VALIDATE_EMAIL)) {
         //echo "email is valid";        
         }
         else {
         //echo "Email not valid";
         exit;
         }
     }
     //connect to database
     function db_connect($db_name, $db_username, $db_password) 
     {
         $conn = new PDO($db_name, $db_username, $db_password);
         $conn->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION); 
         return $conn; 
     }
     // check login credentials 
     function userLogin($email, $password,$PDO)
     {
         $stmt = $PDO->prepare("
             SELECT user_table.user_id ,user_table.user_pass
             FROM user_table
             WHERE user_table.user_email = :EMAIL");

         $stmt->bindParam(':EMAIL', $email);
         $stmt->execute();
         $row = $stmt->fetch(PDO::FETCH_ASSOC); 
         $hash = $row['user_pass'];             
         $returnApp = array( 'LOGIN' => 'Wrong_password_email');

         if (!empty($row) && password_verify($password, $hash)) 
         {
             $user_id = $row['user_id'];
             return $user_id;
         }
         else{
             return $returnApp;                 
         }
     }
     //guidv4
     function guidv4($data = null) {
         $data = $data ?? random_bytes(16);
         assert(strlen($data) == 16);
         $data[6] = chr(ord($data[6]) & 0x0f | 0x40);
         $data[8] = chr(ord($data[8]) & 0x3f | 0x80);
         return vsprintf('%s%s-%s-%s-%s-%s%s%s', str_split(bin2hex($data), 4));
     }
     //create token
     function createtoken($user_id,$user_online,$user_token,$PDO){

         $sql_insert = "INSERT INTO user_status 
                (user_id, user_token,user_status) 
                VALUES 
                (:ID,:TOKEN,:ONLINE );";

         $stmt = $PDO->prepare($sql_insert);
         $stmt->bindParam(':ID', $user_id);
         $stmt->bindParam(':ONLINE', $user_online);
         $stmt->bindParam(':TOKEN', $user_token);
     
         if ($stmt->execute()){
  
         }else{

         }
     }
     //getting user type
     function usertype($user_id,$PDO){
         $sql_select ="SELECT user_table.user_type AS user
             FROM user_table 
             WHERE user_table.user_id = :USER_ID";
         $stmt = $PDO->prepare($sql_select);
         $stmt->bindParam(':USER_ID', $user_id);
                
         if ($stmt->execute()) {
             $row = $stmt->fetch(PDO::FETCH_ASSOC); 
             return $row;
         }else{                          
         }                         
     }
     //specifyuserquery
     function specifyuser($user_type){

         foreach ($user_type as $key) {
  
             switch ($key) {

                 case 'STUDENT':
                         $query = "
                             SELECT
                                 student_table.user_type AS type,
                                 user_status.login_id AS id,
                                 user_status.user_token AS Token
                             FROM  student_table
                             LEFT JOIN user_status ON user_status.user_id = student_table.user_id
                             WHERE  student_table.user_id = :USERID ";

                     return $query;

                 case 'TEACHER':
                         $query = "
                             SELECT
                                 teacher_table.Class AS CL,
                                 teacher_table.DEPARTMENT AS DEP,
                                 user_status.login_id AS ID,
                                 user_status.user_token AS TOKEN
                             FROM  teacher_table
                             LEFT JOIN user_status ON user_status.user_id = teacher_table.user_id
                             WHERE  teacher_table.user_id = :USERID ";

                     return $query;

                 case 'MANAGEMENT':
                         $query = "
                              SELECT
                                  management_table.CLASS AS CL,
                                  management_table.DEPARTMENT AS DEP,
                                  management_table.HEAD AS HEAD,
                                  user_status.login_id AS ID,
                                  user_status.user_token AS TOKEN 
                              FROM  management_table
                              LEFT JOIN user_status ON user_status .user_id = management_table.user_id
                              WHERE  management_table.user_id = :USERID"; 

                     return $query;
             }
         }
     }
     //getdata
     function getdata($query,$user_id,$PDO){

         $stmt = $PDO->prepare($query);
         $stmt->bindParam(':USERID', $user_id);
                    
         if ($stmt->execute()) {
             $row = $stmt->fetch(PDO::FETCH_ASSOC); 
             $data = array( 'LOGIN' => 'Log_In_Success');
             $final = array_merge($data, $row);
             return $final;
         }else{

         }                      
     }

     $email = $_POST['email'];

     //validate email 
     validateEmail($email);

    // connect to data base
    try 
    {
        $PDO=db_connect("mysql:host=localhost;dbname=DATABASE", "root", "");
        //echo "connection success";
    }
    catch (PDOException $e) {
        //echo "Database error! " . $e->getMessage();
    }

    $password =$_POST['pass'];
    //getting either user_id or email/pass dont match database
    $result = userLogin($email,$password,$PDO); 
       
    $user_online = 'ONLINE';
    $user_token = guidv4();

    //if checks if email/pass is correct or error
    if (ctype_digit($result)) {
        //create token
        $token = createtoken($result,$user_online,$user_token,$PDO);
        //get type
        $user_type = usertype($result,$PDO);
        //specifyuser
        $query =specifyuser($user_type);
        //getdata
        $data = getdata($query,$result,$PDO);
        // merge data with type
        $final = array_merge($data,$user_type);

        echo json_encode($final);

    }else{
        echo json_encode($result);
        exit;
    }
?>
\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

I'm still not a fan of this code.

For instance, the userLogin() function will return the user id, when an user was found. That's fine. However, when no user was found, it returns something completely different: An array with a error status and message. I prefer functions that always return the same type, or null/false. I would write something like this:

function getUserIdFromCredentials($database, $userEmail, $userPassword)
{
    $query = "SELECT user_table.user_id ,user_table.user_pass " .
             "FROM user_table " .
             "WHERE user_table.user_email = :email";
    $statement = $database->prepare($query);
    $statement->execute([":email" => $email]);
    if (($row = $statement->fetch(PDO::FETCH_ASSOC)) &&
        password_verify($password, $row["user_pass"])) {
        return $row["user_id"];
    }
    return null; // no user was found - credentials were not valid
}

This function returns null when no user id was found. The only thing we need to know is whether the credentials were valid or not. Error messages, if any, should be handled at another level of the code.

I also renamed the function from userLogin() to getUserIdFromCredentials(). Now this name might seem a bit long, but it does accurately describe what the function does. It doesn't log an user in, it gets the user id given the credentials entered by a visitor. In your code an user is only really "logged in" after you've created a token, I hope. Longer names are preferred, when they more accurately describe what they refer to. Longer names do not impact the speed at which the code executes.

Note how I don't access the $row array, until I know it exists.

You also use the login script to get information about the specific type of user. You store this information in separate tables. I feel that this doesn't belong in this login script. It's not needed for a valid login, and you need to retrieve this information elsewhere anyway. I would get rid of it here.

Now I cannot escape the feeling that this code isn't written for a web site, it seems to be written for an App or API. You don't tell us, but there's this very suspicious $returnApp variable, suggesting it's an App. An App might need to authenticate on every access to a script, when it is working stateless.

It would explain the absence of a logout function, and the lack of a cookie or session variable. On the other hand, you do generate a token, which is only useful when the script is not stateless. I don't know.

I would like you to actually make this code work. Finish it, so you can actually run and test it. Handle all errors properly. It's time to leave any theoretical discussions behind and get your hands dirty.

\$\endgroup\$
6
  • \$\begingroup\$ you are right its should work on different os/platfroms i could change the $returnapp to be later as for later ill seperate the functions from the control code and add the returnApp final execution .. i still need user type and limitation for different UX/UI so its being stored on shared preferences on android side for example thats why i dont want to stored direct info for user as shared preference not secure enough/ also to let the user know sessions that are open .. i had to cut the code much to make my spaghetti code readable .. \$\endgroup\$
    – Taa Lee
    Commented Feb 20, 2022 at 21:42
  • \$\begingroup\$ so ill spereate the function from control code .. ill move errors messages to control and return null instead of error .. its seems like a school but its should be for uni .. and i have to tell you.. your instructions really helped understand php better and made php not a pain anymore .. you dont know how much i hated php but its much simpler now \$\endgroup\$
    – Taa Lee
    Commented Feb 20, 2022 at 21:47
  • \$\begingroup\$ and one last thing the function you gave me in my spaghetti code took me 2 days to understand and you dont know how eaiser php is now for me after i understood it \$\endgroup\$
    – Taa Lee
    Commented Feb 20, 2022 at 21:54
  • 1
    \$\begingroup\$ @TaaLee From what I have seen, while teaching others to program, is that most problems stem from over-complicating things. If you stick to a few golden rules, life becomes a lot easier. We already had spagetti code and database normalization, here I talked about separation of concerns and simply choosing good descriptive names. I also find that a good preparation is crucial. Exactly define in advance what the code should do. \$\endgroup\$ Commented Feb 20, 2022 at 22:09
  • \$\begingroup\$ thats exactly what i needed .. and thats what i have learned.. past failures on serverside/appside had me redo the whole project as small mistake was fatal to the whole system "butterfly effect" is huge in programming but after your instructions i can separate the server from frontend and now i can separate the fixed functions connected to the database from control code that connected to frontend .. ill read the articles do my research of both and update my code .. i know that thanks not allowed here but you should know how much i appreciate your instructions.. very useful and on point .. \$\endgroup\$
    – Taa Lee
    Commented Feb 20, 2022 at 22:25

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.