Page 1 of 1

session: login admin and user

Posted: Wed Dec 14, 2011 8:08 am
by ankungen
Hi!

I am trying to make a login script for admins and users and I don't want users to be able to access any of the admin pages and at the moment I don't want admins to be able to access users pages either. I have tried to figure out a way, since I haven't found any good explations on the internet that I can understand, since english isn't my native language.

I have done a script that is close to what I want, but there are some issues. I also think that there is a much better way than mine, because I have only used logic (well, my logic anyway :lol: )with the knowledge I have and I really don't know how to do this right.

Here is what I have done:

Mysql

A database "dbdatabase" and a table "users" with these fields: id, username, password och usertype. In the field usertype I put in either "admin" or "user" without the quotes.

index.php

A loginform that calls for login.php in the action-attribute.

[inline]
<form method="post" action="login.php">
<h4>Logga in</h4>
Anv&auml;ndarnamn:<br> <input type="text" name="username"><br>
L&ouml;senord: <br><input type="password" name="password"><br>
<input type="submit" name="submit" value="Logga in!"><br>
</form>
[/inline]

login.php

Code: Select all

<?php
	if (isset($_POST['submit'])){

	mysql_connect('dbhost', 'dbuser', 'dbpass') or die(mysql_error());
	mysql_select_db('dbdatabase') or die(mysql_error());
	
	$username = $_POST['username'];
	$password = $_POST['password'];
	
	$password = md5($password);
	
$isAdmin = mysql_query("SELECT username FROM users WHERE username='$username' AND password='$password' AND usertype = 'admin'");	
$loginAdmin = mysql_num_rows($isAdmin);

$isUser = mysql_query("SELECT username FROM users WHERE username='$username' AND password='$password' AND usertype = 'user'");
$loginUser = mysql_num_rows($isUser);



	if($loginAdmin == 1){
		session_start();
		$_SESSION['username'] = $username;
		header("Location: admin/index.php");
	}
	
	else if($loginUser == 1){
		session_start();
		$_SESSION['username'] = $username;
		header("Location: user/index.php");
	}
	
	else if($login == 0){
		header("Location: index.php"); //How can I write an error message at the top of the form?
	}

}
?>

admin.php

Code: Select all

<?php

	session_start();
	$lia = $_SESSION['username'];

	mysql_connect('dbhost', 'dbuser', 'dbpass') or die(mysql_error());
	mysql_select_db('dbdatabase') or die(mysql_error());

	$checkAdmin = mysql_query("SELECT username FROM users WHERE username='$lia' AND usertype = 'admin'");	
	$sessionAdmin = mysql_num_rows($checkAdmin);

	if($_SESSION['username'] == ''){
	die("<a href='../?p=index'>Logga in f&ouml;rst</a>"); 
	}	

	else if($sessionAdmin == 'admin'){
	die("<a href='../?p=index'>Logga in som admin f&ouml;rst</a>"); 
	}

?>

user.php

Code: Select all

<?php

	session_start();
	$lia = $_SESSION['username'];

	mysql_connect('dbhost', 'dbuser', 'dbpass') or die(mysql_error());
	mysql_select_db('dbdatabase') or die(mysql_error());

	$checkUser = mysql_query("SELECT username FROM users WHERE username='$lia' AND usertype = user'");	
	$sessionUser = mysql_num_rows($checkUser);

	if($_SESSION['username'] == ''){
	die("<a href='../?p=index'>Logga in f&ouml;rst</a>"); 
	}	

	else if($sessionUser == 'user'){
	die("<a href='../?p=index'>Logga in f&ouml;rst</a>"); 
}

?>
When I use this script, logged in users arrive to user/index.php and can't access admin/index.php and admin arrive to admin/index.php and can't access user/index.php. This is exactly what I want to accomplish, but I also want to block access for users to all the pages in the folder admin and that doesn't work now.

In admin/index.php I have a couple of div tags, "header", "leftmenu" with links and "content" where the links will be shown. I don't know if this is necessary, but here is the code that resides in the content box:

Code: Select all

							<?php
      								
									if(file_exists($_GET['p'].".php")){
         							include($_GET['p'].".php");
									} 							
								
									else{
      								if(empty($_GET['p']) OR $_GET['p'] == ""){
         							include("profil.php");
      								} 
									
									else{
         							include("404.php");
									
      								}
   								
								}
								
							?>		

admin/profil.php is the main file that show up in the content box as you can see above and I tried to put the same code in profil.php as I did in admin/index.php, but I think it should be a simpler way to accomplis this, like a simple code that says: This page can't be shown, without admin/index.php and then put this little code-snippet in all other pages in admin/index.php. How can I do that?

There is another thing that confuses me and is the main reason why I don't understand my own code. This part in admin/index.php:

Code: Select all

	else if($sessionAdmin == 'admin'){
	die("<a href='../?p=index'>Logga in som admin f&ouml;rst</a>"); 
	}
confuses me, because when I figure out this part, I thought like this: else if user isn't = admin, then "die" and a login link. I wrote the code with != and not == at first because that felt right. The above code tells me that if it is true that the user is admin, then don't let the user in and that seem strange. Can anyone explain it to me?

Finally when I press logout when I am logged in as admin I won't be logged out. I have to press the link twice in a sequence. The issue only happens when I am logged in as admin and not for users. I only use session_start(); and session_destroy(); in logout.php.

I know there are a lot of questions here and maybe I can't get an answer to them all, but thanks in advance!

Suzanne

Re: session: login admin and user

Posted: Wed Dec 14, 2011 12:20 pm
by social_experiment
ankungen wrote: I thought like this: else if user isn't = admin, then "die" and a login link. I wrote the code with != and not == at first because that felt right. The above code tells me that if it is true that the user is admin, then don't let the user in and that seem strange. Can anyone explain it to me?
Your first assumption was correct; this code will tell a validly logged in administrator to login. You do want to use != (or not equal to) but this then has to go onto the admin pages, or pages which are for administrators only because if the value is not equal to (!=) 'admin' you don't want the user to access any data if the aren't admins (This approach however isn't a good one, see below for a better explanation). It will be better to change $sessionAdmin to a more generic name, something like $userStatus to indicate that it is the status of the user and not necessarily the status of only an administrator. Also, die() will halt the script; you want to redirect the user with header() instead.

Code: Select all

<?php
 else if($sessionAdmin == 'admin'){
        die("<a href='../?p=index'>Logga in som admin f&ouml;rst</a>"); 
        }
?>
ankungen wrote:Finally when I press logout when I am logged in as admin I won't be logged out. I have to press the link twice in a sequence. The issue only happens when I am logged in as admin and not for users. I only use session_start(); and session_destroy(); in logout.php.
Can you paste the code for this?

There are also other issues with your code which i will briefly highlight
1. No escaping of your username and password when you pass them to your database query. This will lead to SQL injection.
2. Using md5() to hash passwords. Don't. md5() is broken and shouldn't be (solely) relied upon for password hashing. Look at sha1 (at least) or sha256, sha384 as alternate hashing algorithms.

The user.php and admin.php pages do you use them as 'auth' pages, to see if a user is logged in? You shouldn't check if a user is an admin or user but rather set a 'user level' (1 for admin and 2 for user for instance) and determine what is displayed according to those values.

Re: session: login admin and user

Posted: Thu Dec 15, 2011 8:00 pm
by ankungen
social_experiment wrote:
It will be better to change $sessionAdmin to a more generic name, something like $userStatus to indicate that it is the status of the user and not necessarily the status of only an administrator.

I will take a look at that.
Also, die() will halt the script; you want to redirect the user with header() instead.
I really don't know how die() ended up there. I must have been coding in the sleep :)

Can you paste the code for this?
I have found the problem of logging out.
I have all the links for all pages on the site in the header, because I want them all to be accessible all the time. This is just tempararly when I build and test the site. Since the links are different depending on which page I am at, I have three headers, for admin, user and not logged in users. The logout link for admin was like this:

<a href="../?p=logout">Logga ut</a>

Normally I use that syntax for the links for logged in users and admin, because I want the links to open in the content box in admin/index.php or user/index.php, but the logout.php is in the root directory. Therefore, when I pressed the link once the file couldn't be found and I was redirected to the index.php in the root directory. The second time I pressed the link I was redirected to logout.php, because now I didn't press the link from the header of admin anymore. It was the link from the header of not logged in users and it looks like this:

<a href="logout.php">Logga ut</a>

That one is the correct one to use. In the header for user I had the correct syntax already. It was like this:

<a href="../logout.php">Logga ut</a>

So I have now changed to the above for admin and it works.

1. No escaping of your username and password when you pass them to your database query. This will lead to SQL injection.
After I had copied my code to this forum I removed the code block that I had temporarly put between comments. I also have this code in login.php

$username = mysql_real_escape_string('username');
$password = mysql_real_escape_string('password');

I had put it between comments, since the page gets blank otherwise. I didn't want to remove it, since I plan to use it later. I guess this is what you are talking about? I didn't really understand what the code was for, only that it was for security. Now I know and I even have a name for it :D When I seached for the meaning of SQL injections I got a lot of knowledge :D
2. Using md5() to hash passwords. Don't. md5() is broken and shouldn't be (solely) relied upon for password hashing. Look at sha1 (at least) or sha256, sha384 as alternate hashing algorithms.
I have planned to look into security later when I have learned to build a functional site. I have read a little about it and I plan to use salt (I think that is the name) and I guess I will look at sha256 or sha384 since you recommend it before md5. But first I have to build someting to secure :D

The user.php and admin.php pages do you use them as 'auth' pages, to see if a user is logged in?
I did notice that I have written the wrong filename above. The two headlines, admin.php and user.php is admin/index.php and user/index.php as I have written in the rest of my explanation. I have index.php in the root where the loginform is and it calls for login.php on submit. In the root derectory I have two folders, admin and user and both have a index.php. In other words, there is no admin.php or user.php.

Both of these index.php has the same layout, with leftbar.php with links, content box with the php include code above and a header.

When I log in, either admin/index.php or user/index.php is loaded together with the header and leftbar that I have in separate files as leftbar.php and header.php.
You shouldn't check if a user is an admin or user but rather set a 'user level' (1 for admin and 2 for user for instance) and determine what is displayed according to those values.
I don't know if I understand you here?



Re: session: login admin and user

Posted: Fri Dec 16, 2011 1:42 am
by social_experiment
ankungen wrote:You shouldn't check if a user is an admin or user but rather set a 'user level' (1 for admin and 2 for user for instance) and determine what is displayed according to those values.

I don't know if I understand you here?

Code: Select all

<?php
$isAdmin = mysql_query("SELECT username FROM users WHERE username='$username' AND password='$password' AND usertype = 'admin'");        
$loginAdmin = mysql_num_rows($isAdmin);

$isUser = mysql_query("SELECT username FROM users WHERE username='$username' AND password='$password' AND usertype = 'user'");
$loginUser = mysql_num_rows($isUser);
?>
A look at the code above: You are currently doing 2 searches when you can actually do only 1. If you used numeric values to determine whether a user is an admin or just a regular user you can have the following code

Code: Select all

<?php
$query = mysql_query("SELECT username, status FROM users WHERE username = '$username' AND password = '$password' ");
?>
Once you have the status you can determine what to display according to it and redirect to the admin or regular user section. I guess you could also do it using the existing code but it seems a bit inefficient doing two searches when you can do only one and you wouldn't have to check usertype either.

Edit
Have a look at this topic; it illustrates what i have in mind viewtopic.php?f=1&t=133490&p=667753