0

I need some help with my login script. I am just trying to self teach php and mysqli and stuck with this so any advice/help would be much appreciated.

The actual part that checks the login details works and directs the user a page if the match is found or back to login page if unsuccessful.

What I want to happen is once the username has been checked and the sessions set use the session 'user' to then find the same user from either the pupil or instructor table depending on the session 'member_type'. The session 'user' is set as the 'user_id' which is the same in the login table and the table the rest of the details are stored in and is the primary key of both tables.

<?php 

include_once ("includes/dbconnect.php");
session_start();

$error="";

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

// username and password received from loginform 
$username=mysqli_real_escape_string($conn,$_POST['username']);
$password=mysqli_real_escape_string($conn,$_POST['user_password']);

$sql_checklogin="SELECT * FROM user_logins WHERE username='$username' and password='$password'";
$result=mysqli_query($conn,$sql_checklogin);
$login=mysqli_fetch_array($result,MYSQLI_ASSOC);
$count=mysqli_num_rows($result);


// If result matched $username and $password, table row must be 1 row
if($count==1)
{
// Set Sessions
$_SESSION['logged_in']=TRUE;
$_SESSION['user']=$login['user_id'];
$_SESSION['member_type']=$login['reg_type'];

} else {
  header("location:login.php");
  $error = "Invalid Username or Password!";
}
}
//This part to be replaced with code that checks both tables to find the user details
if ( isset($_SESSION['member_type'])) {
header("Location:mydashboard.php");
}

?>
//The above code works and allows user to login but sends both member types to same page

Just now this works fine but I want to replace the last IF part of the script with something like this (or a better suggestion?).

//if ( isset($_SESSION['member_type'])=='pupil' { 
//$sql_findpupil="SELECT * FROM pupils WHERE user_id='$_SESSION['user']'";
//$result=mysqli_query($conn,$sql_findpupil);
//$pupil=mysqli_fetch_array($result,MYSQLI_ASSOC);

Should I be putting another count function at this part?

//header("location:mydashboard.php");

//}elseif ( isset($_SESSION['member_type'])=='instructor' {
//$sql_findinstructor="SELECT * FROM instructors WHERE user_id='$_SESSION['user']'";
//$result=mysqli_query($conn,$sql_findinstructor);
//$instructor=mysqli_fetch_array($result,$MYSLQI_ASSOC);

And here?

//header("location:control_panel.php");
//}
//}

As I said, I am trying to teach myself some new skills at home so have probably made some basic school boy errors here and missed something obvious

Brian
  • 1
  • because you are missing your exit; after your header() method. – Jaquarh Oct 02 '16 at 18:53
  • Looks like you are storing passwords in plain text PHP provides [`password_hash()`](http://php.net/manual/en/function.password-hash.php) and [`password_verify()`](http://php.net/manual/en/function.password-verify.php) please use them. And here are some [good ideas about passwords](https://www.owasp.org/index.php/Password_Storage_Cheat_Sheet) If you are using a PHP version prior to 5.5 [there is a compatibility pack available here](https://github.com/ircmaxell/password_compat) – RiggsFolly Oct 02 '16 at 19:21
  • Your script is at risk of [SQL Injection Attack](http://stackoverflow.com/questions/60174/how-can-i-prevent-sql-injection-in-php) Have a look at what happened to [Little Bobby Tables](http://bobby-tables.com/) Even [if you are escaping inputs, its not safe!](http://stackoverflow.com/questions/5741187/sql-injection-that-gets-around-mysql-real-escape-string) Use [prepared parameterized statements](http://php.net/manual/en/mysqli.quickstart.prepared-statements.php) – RiggsFolly Oct 02 '16 at 19:21
  • What is the point of selecting the pupil or instructor in the login checker, you dont do anything with this information. Do it in the `control_panel.php` or `mydashboard.php` using the session information you saved in this script. Then you will already know which table to select it from – RiggsFolly Oct 02 '16 at 19:26
  • Cheers RiggsFolly, I do intend to use the password hash and try prevent SQL injections etc.. right now everything is on localhost as I am just trying to make sure I can get pages etc to work with the limited knowledge I have but I suppose these should become a habbit! – Brian Oct 02 '16 at 19:52

1 Answers1

0

isset() checks if a variable is set or not, but you want to check the value. So your if ( isset($_SESSION['member_type'])) is always true, for pupils and instructors. Try

if ($_SESSION['member_type'] == 'pupil') {
    header("Location:mydashboard.php");
}

instead.

And another suggestion, refactor your database-design. Better don't use an instructor and a pupil table containing userIds, but a userType-Table containing only two rows (pupil / instructor) and then add a userTypeId to your user-table. And also add a foreign key from user.userTypeId to userType.id to make sure every user is linked to an existing userType.

Using that design you won't need another query, to determine if a user is a pupil or an instructor.

simon.ro
  • 2,984
  • 2
  • 22
  • 36