question with my login system

alright, ive written a login system where it checks ur input (duh) and then gets a number, 0 or 1 from an Administrator column in the db, and if its 1 it takes you to admin.php and if its 0 it takes you to user.php

now the admin one if working fine, but when i login to the user.php one, i can see my content. i think its something to do with my if statements, ill post the code for both login.php and user.php

( index.php?id=7 is a login form )

include 'config.php';
$username = $_POST['username'];
$password = md5($_POST['password']);
$query = mysql_query("SELECT * FROM users") or die(mysql_error());
while($row=mysql_fetch_array($query)){
if(($username==$row['Username'])&&($password==$row['Password'])){
if($row['Administrator'] == 1){
$_SESSION['isrobbieloggedin'] = 1;
$_SESSION['adminname'] = $row['Name'];
header("Location: admin.php"); } else {
$_SESSION['username'] = $row['Name'];
$_SESSION['isuserloggedin'] = 1;
$_SESSION['userid'] = $row['id'];
header("Location: user.php");
}
}
}
if(!$username == NULL){ echo $username.', y'; } else { echo 'Y'; }
echo 'our username or password was incorrect. Please hit the back button and try again.';
( login.php )


$id = $_SESSION['userid'];
$query = mysql_query("SELECT * FROM users WHERE id = '$id'") or die(mysql_error());
while($row = mysql_fetch_array($query)){

if(isset($_POST['Submit'])){
$id = $_SESSION['userid'];
$name = $_SESSION['username'];
$website = $_POST['website'];
$testemonial = $_POST['testemonial'];
mysql_query("INSERT INTO testemonials(Testemonial, Author, Website, UserID) VALUES('$testemonial','$name','$website','$id')") or die(mysql_error());
mysql_query("UPDATE users SET HasPosted = '1' WHERE id = '$id'") or die(mysql_error());
echo 'Your testimonial has been added. You <b>cannot</b> edit or post another or delete your testimonial.';
} else {
if($row['HasPosted']==0){ ?>
<div align="center">
<form action="user.php" method="post" name="post" id="post">
<p>&nbsp;</p>
<table width="500" border="0">
<tr>
<td width="95" align="center" valign="middle"><div align="center">Your
testemonial:</div></td>
<td width="395"> <div align="center">
<textarea name="testemonial" cols="60" rows="10" id="textarea"></textarea>
</div></td>
</tr>
<tr>
<td align="center" valign="middle"><div align="center">Posted by:</div></td>
<td><div align="center"><? echo $_SESSION['username']; ?> </div></td>
</tr>
<tr>
<td align="center" valign="middle"><div align="center">Your URL:</div></td>
<td> <div align="center">
<input type="text" name="website">
</div></td>
</tr>
<tr>
<td colspan="2" align="center" valign="middle"> <div align="center">
<input type="submit" name="Submit" value="Submit">
</div></td>
</tr>
</table>
<p>&nbsp;</p>
</form>
</div>

</td>
<?

} else {
echo 'You have already posted your testimonial.';
}
}



}

( user.php )

i think it's where my if statement says if($condition== ... im not sure why but i get different results when i change the two == to one =.. which should i use and how to get this script working?

btw, instead of seeing what i should on user.php i just see blank.

I'll have to check the code @ work (where i have sessions enabled). In the meantime, I wonder if you should change this line:

if(!$username == NULL){ echo $username.', y'; } else { echo 'Y'; }

to this:
if($username != NULL){ echo $username.', y'; } else { echo 'Y'; }

I'd also put exit() statements after your header() statements.


Here's the other problem....

Your while() statement continues on and on and on...
even after it's done, it will continue until user...


Change your sql statement.



"SELECT * FROM users WHERE Username = '$username')"


That way, it only pulls from the database what's needed. If no rows are returned, then the username doesn't exist.





An Example:

<?
include 'config.php';
$username = $_POST['username'];
$password = md5($_POST['password']);
$sql = "SELECT * FROM users WHERE Username = '$username'";
$result = mysql_query($sql) or die(print mysql_error());

if ($result) {
while ($row = mysql_fetch_assoc($result)) {

/* catch bad password */
if ($password != $row["Password"]) {
print "Bad Password";
break 2;
}


/* check administrator password */
if ($row["Administrator"] == "1") {
print "Houston, we have a winner.";
break 2;
} else {
print "Houston, we have a loser, er...I mean....USER...";
break 2;
}


} // while
} // if


?>



I typed that here in this post, so if it doesn't work, uhm...i'll revise it. ;)


Hope That helps, John

I don't understand that code. Why isn't the password included in the where clause?
And why break 2 ? There is only one structure you can break out of in that code.
I also don't understand why you don't check if mysql_num_rows == 1 You should, on succesful login, only get 1 row and there is no need for a while loop (even in your present code)
die(print mysql_error()); --> why the print? Die will automatically print what's between the brackets (unledd it's an integer)

You'd better also check your inputted values agains SQL-injections and cerainly use addslashes() on the string-formvalues you use inside the sql-statement.


I would also recommend using sha1() or sha2() instead of md5(). (md5() seams to be compromised and is apparently explicitely refused by VISA).

Also, I personaly would store the pageadress to redirect to after a succesfull login, inside the db, and use that inside your code --> more flexability at no cost. Users could then pick their own startpage and your code gets quit a bit shorter.
Because if you incule both username and pwd inside the where clause, you only need

if (mysql_num_row($result)== 1) {
$row= mysql_fetch_assoc($result);
header("Location: " . $row['url]' . ".php");
} else {
echo 'Invalid login. Try again';
}

Hmmmm. raf, thou hast made excellent points. The break didn't need 2, you're correct. as well on the print mysql_error(). I realized both upon posting, but didn't have time to edit (I had a party to go to!!!:D)

I like the idea of just checking the num_rows returned. Hadn't thought of that. THAT is what happens when you code php for years in a little hole and never actually DISCUSS it with anyone.... lol... thanks for the pointer, I think I'll switch my login functions ;)

Although, I must add, if I can find a reason NOT to use the num_rows_returned, I'll tell ya ;) hee hee.

AH-HA! I just thought of a reason....

What if you store the password somewhere else...like in an LDAP server! HA-HA! Of course, you'd prolly store the username there too....but that's beside the point!! MUA HAHHAA.. ok... so you're right. In this particular case, there is ABSOLUTELY no point in making things more complicated than they need to be.

Good advice!

And I'll totally agree with the encryption (hashing) of the password. I didn't even look at that part of the code, just accepted it.....lol

excellent suggestions....thanks for pointing them out!

i'm glad you agree.

by the way, inside the original code, there was the "select * "you copied ---> NEVER use "select * " Always specify the variables you actually need (even if you do actually need all variables --> if you later on add a column to your db, it will also be returned while you don't us it).

the sessionvariable of the orgiginal code are also a bit to complicated.
Instead of having seperate sessionvariables for the admin and the regular user, i woul use one sessionvariable, and store a security-profile value inthere. Admin = 10, regular user = 4 or so., not logged in = 0 or Empty. Then , for each page, you can compaire the securityprofile with the minimum required securityprofile for that page (or, for that part of the pages content or whatever.) For bigger apps, i always keep a pages-table to dynamically build the html-header, and where i store the minimum security info and navigationoption is.

So for all page-profile compinations, you can do with one check
if ($_SESSION['secprof'] >= $pagesecprof) {
--> less code; less sessionvariables to manage; the code actually expresses what you logically want to do; completey dynamic etc.