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.

user_table:

CREATE TABLE `user_table` (
`user_id` int(11) NOT NULL AUTO_INCREMENT,
`user_type` 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

student_table:

CREATE TABLE `student_table` (
`user_id` int(11) NOT NULL,
`user_type` varchar(255) NOT NULL,
`user_email` varchar(255) NOT NULL,
`user_pass ` 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
user_email varchar(255) NO NULL
user_pass 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,
`user_email` varchar(255) NOT NULL,
`user_pass ` 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
user_email varchar(255) NO NULL
user_pass 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,
`user_email` varchar(255) NOT NULL,
`user_pass ` 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_email varchar(255) NO NULL
user_pass 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,
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

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 student_table.user_id ,student_table.user_pass
   FROM student_table
   WHERE student_table.user_email = :EMAIL
   UNION 
   SELECT teacher_table.user_id ,teacher_table.user_pass
   FROM teacher_table
   WHERE teacher_table.user_email = :EMAIL
   UNION 
   SELECT management_table.user_id,management_table.user_pass
   FROM management_table
   WHERE management_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{                          
  }                         
}
//getting specific data for user limitation
function getdata($user_id,$PDO){

  $stmt = $PDO->prepare("
                     SELECT
                           student_table.user_type AS type,
                           user_status.login_id AS id,
                           user_status.user_token AS Token,
                           null,
                           null
                     FROM  student_table
                     LEFT JOIN user_status ON user_status.user_id = student_table.user_id
                     WHERE  student_table.user_id = :USERID
                     UNION 
                     SELECT
                           teacher_table.Class AS CL,
                           teacher_table.DEPARTMENT AS DEP,
                           user_status.login_id AS ID,
                           user_status.user_token AS TOKEN,
                           null 
                     FROM  teacher_table
                     LEFT JOIN user_status ON user_status.user_id = teacher_table.user_id
                     WHERE  teacher_table.user_id = :USERID 
                     UNION
                     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 ");

$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);
// get data
 $data = getdata($result,$PDO);
// merge data with type
 $final = array_merge($data,$user_type);

 echo json_encode($final);

 }else{
  echo json_encode($result);
  exit;
 }
?>
\$\endgroup\$
2
  • 1
    \$\begingroup\$ Wasn't this code already posted for review? Didn't someone already tell you to improve the tabbing (the easiest editing technique to perform in software development)? \$\endgroup\$ Commented Feb 19, 2022 at 7:18
  • \$\begingroup\$ @mickmackusa yeah but this is the updated version and i was asked to ask a new question for updated version not update the question and i did but when i post here sometimes spaces fail .. so i have to redo it .. \$\endgroup\$
    – Taa Lee
    Commented Feb 19, 2022 at 7:27

1 Answer 1

1
\$\begingroup\$

You do have a user_table containing your users, but it is almost empty. You then put the user_id, user_email and user_pass fields in the student_table, teacher_table and management_table, requiring you to gather them together like this:

   SELECT student_table.user_id ,student_table.user_pass
   FROM student_table
   WHERE student_table.user_email = :EMAIL
   UNION 
   SELECT teacher_table.user_id ,teacher_table.user_pass
   FROM teacher_table
   WHERE teacher_table.user_email = :EMAIL
   UNION 
   SELECT management_table.user_id,management_table.user_pass
   FROM management_table
   WHERE management_table.user_email = :EMAIL

This makes no sense. Why not put these fields in the user_table? The user table should contain all fields that are directly related to the user. That includes all the fields from the user_status table.

This would make for a much simpler database design, and shorter database queries. Keeping things simple will increase the security of the code because it will be easier to spot security holes.

This suggestion falls under the term: database normalization

\$\endgroup\$
8
  • \$\begingroup\$ i have 14 different type of user each one have different limitation usage .. so i had a problem with unique id's when different types of user interact with eachother .. and someone recommended to do table for auto increment and type then add generated id with their limitation and credentials \$\endgroup\$
    – Taa Lee
    Commented Feb 19, 2022 at 7:32
  • 2
    \$\begingroup\$ @TaaLee An user is an user, regardless of the type or limitation. Each user should indeed have an unique id, and an auto increment field is a good way to achieve that. \$\endgroup\$ Commented Feb 19, 2022 at 7:34
  • 1
    \$\begingroup\$ @TaaLee: Yes, indeed, that is the idea. \$\endgroup\$ Commented Feb 19, 2022 at 7:40
  • 1
    \$\begingroup\$ @TaaLee: Please note that if you are already using the database, for real users, you might get conflicts: A student might have the same user id as a teacher. Of course that would create chaos. In that case you have to assign everyone a new, and unique, user id. \$\endgroup\$ Commented Feb 19, 2022 at 7:43
  • 1
    \$\begingroup\$ I tackled the biggest problem your code has, that doesn't mean the rest is fine. Security problems are often tiny things you forgot or did. It might be the way you handle errors. Since you're going to change the code quite drastically I cannot say it is fine. You do bind parameters to queries, use tokens, and hash passwords, those are all good things. \$\endgroup\$ Commented Feb 19, 2022 at 7:56

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.