Posted by shimmyshack on 03/29/07 10:49

On 28 Mar, 16:40, "Nosferatum" <John.Ola...@gmail.com> wrote:
> This script is meant to limit access by sessions, using username and
> password from mysql db and redirect users after login according to a
> given value belonging to each user in the db (10,20,30,40).
>
> (the included config is just server settings, the login is just a
> login form).
>
> The script appear to connect but will not redirect users, it seems
> that even with correct login details, it won't validate.
>
> this code is in top of each protected page granting access to users
> with user level 10:
> <?php $allow = array (10);include ("../protect/protect.php"); ?>
>
> THE SCRIPT (protect.php):
>
> <?php
>
> session_start ();
>
> // --------------------------------THE
> VARIABLES---------------------------------- //
>
> @include ("config.php");
>
> // ----------------------------------THE CODE
> ------------------------------------ //
>
> function clearance ($user_value, $pass_value, $level_value,
> $userlevel_value, $table_value, $column1, $column2, $path) { //
> Function to see if user can login
>
> $check = mysql_query ("SELECT $userlevel_value FROM $table_value
> WHERE username='$user_value' AND password='$pass_value'"); // Query to
> see if user exists
>
> $verify = mysql_num_rows ($check);
>
> $get = mysql_fetch_array ($check);
>
> if (count ($level_value) != 0) { // If the allow array contains
> userlevels
>
> if (in_array ($get[$userlevel_value], $level_value) && $verify > 0)
> { // Search allow to see if userlevels match
>
> $_SESSION['username'] = $user_value; // Register sessions
> $_SESSION['password'] = $pass_value; // password
> $_SESSION['userlevel'] = $get[$userlevel_value];
>
> }
> //redirect users according to user level
> if ($verify > 0); {
> $row = mysql_fetch_array($check);
> }
>
> switch($row['userlevel_value']) {
> case '10':
> header("location:/hidden/folder1/index.php");
> break;
> case '20':
> header("location:/hidden/folder2/index.php");
> break;
> case '30':
> header("location:/hidden/folder3/index.php");
> break;
> case '40':
> header("location:/hidden/folder4/index.php");
> break;
> default:
> printf("Invalid username and password<br>\n");
> }
> //end redirect
>
> } else {
>
> if ($verify == 0) { // If attempt fails then redirect to login page
>
> $_SESSION = array();
>
> $error = "Sorry, invalig login";
>
> @include ("login.php");
>
> exit;
>
> }
>
> if ($verify > 0) { // If attempt is good then register the user
>
> $_SESSION['username'] = $user_value;
> $_SESSION['password'] = $pass_value;
>
> }
>
> }
>
> }
>
> function protect ($level_value, $password_value, $userlevel_value,
> $table_value, $column1, $path) { // Function to keep pages secure
>
> if (!isset ($_SESSION['username'])) { // If session doesn't exist
> then get user to login
>
> if (isset ($_POST['username']) && isset ($_POST['password'])) {
>
> $error = "Sorry, username or password doesnt fit";
>
> }
>
> $_SESSION = array();
>
> @include ("login.php");
>
> exit;
>
> } else { // If user is logged in check to see if session is valid and
> that they have the required userlevel
>
> $check = mysql_query ("SELECT $password_value, $userlevel_value FROM
> $table_value WHERE $column1='$_SESSION[username]'"); // Query to see
> if user exists
>
> $verify = mysql_num_rows ($check);
>
> $get = mysql_fetch_array ($check);
>
> if ($verify == 0) {
>
> $_SESSION = array();
>
> $error = "Something wrong with your login";
>
> @include ("login.php");
>
> exit;
>
> }
>
> if ($verify > 0 && count ($level_value) != 0) {
>
> if (!in_array ($get[$userlevel_value], $level_value)) { // Check to
> see if the users userlevel allows them to view the page
>
> $error = "Sorry, no access";
>
> @include ("login.php");
>
> exit; // Ensure no other data is sent
>
> }
>
> }
>
> }
>
> }
>
> if (isset ($_POST['username']) && isset ($_POST['password'])) { // If
> user submits login information then validate it
>
> clearance ($_POST['username'], $_POST['password'], $allow,
> $userlevel, $table, $username, $password, $path);
>
> }
>
> protect ($allow, $password, $userlevel, $table, $username, $path);
>
> mysql_close ($link); // Close the database connection for security
> reasons
>
> // -----------------------------------THE END
> ------------------------------------ //
>
> ?>

It's just a bit confused right now, with $userlevel_value,
$level_value, $get['userlevel_value'] and $userlevel - I might have
forgotten one or made one up.

Try sticking to a convention, eg. I would have $arrAllowedLevels for
$level_value.
I'm sure you get bored of passing so many things to each function, if
you can logically group the variables passed in it would be easier to
keep track of, can you either use an array of "query details"
$arrQuery =
array('columns_selected'=>array('username','password'),'table_name'=>'my_table');
or table details, or just form the query outside and pass it in as a
string, wrapping the logic inside 2 functions when you are including a
file called protect.php seems uneeded for this purpose. You could
start by writing the whole lot as a nice clean script, and think about
wrapping it up later. It might help you get the logic straight.

because of the way you have coded the clearance() query, at the moment
it allows anyone to authenticate without a correct username or
password, and then for this to persist inside the session, allowing
unrestricted access (and other nasties). Remember to use
$var=mysql_real_escape_string((string)$var[,$link]) before you pass a
value into a query, see the manual for more (I am assuming that config
handles the db link as well)

a couple of other points.
what is $level_value, it seems to be an array and the logic suggests
that you want to use it as "if the user has [at least?] this level,
then let them in" - but what if your users have a greater level than
this? You will have to add each level into the array to allow them
access too.
Why could you not use an integer called $intMinClearanceLevel, if the
user's level is at least equal to it, they can pass, this is a minor
change but simplifies your logic and the need to add each level into
the array.
Call exit() after a call to header( 'Location: ' . $strAbsoluteUri );
and use the absolute uri, although browsers tend to do well, it could
be ambiguous in certain circumstances (probably more so for the coder
than the browser!)
If you do want to "be efficient" and only include files if and when
they are needed - which as Jerry points out - can make for over
inclusion, use include_once so that php will not include multiple
times!
Don't be disheartened though, it will all come together

[Back to original message]