Login Validation Is Skipping A Check.

[White_Lily]

Hi, i wrote a login form with some validation - problem is it is skipping the first part of the validation where it checks if the form had any data entered in the first place...

 

$sub = $_POST["submit"];
$username = $_POST["username"];
$password = sha1($_POST["password"]);

if($sub){
if(!$username && !$password){
$err .= "You tried to login without providing the informtion required.";
}else{
if(!$username){
$err .= "Please enter a username.";
}else{
if(!$password){
$err .= "Please enter a password.";
}else{
$getLogin = select("*", "users", "username = '$username'");
$get = mysql_fetch_assoc($getLogin);

if($get["username"] != $username){
$err .= "The username didn't match the database.";
}else{
if($get["password"] != $password){
$err .= "The password didn't match the database.";
}else{
session_start();
$_SESSION["username"] = $username;
$_SESSION["password"] = $password;
$_SESSION["user_level"] = $get["user_level"];

header("Location: main.php");
}
}
}
}
}

echo '<div class="error">ERROR: '.$err.'</div>';
}

 

can anyone see why?

 

(it goes straight to checking the username field rather then checking the entire form)

[Jessica]

Even if every field is blank, if a form was submitted at all, $_POST will exist. It always contains the submit field at least, and the input fields will contain null/blank values.

[White_Lily]

So how come all the other login forms ive written check if the form is empty, yet this one skips it completely?

[Jessica]

Magic.

[White_Lily]

Cool.

[Pikachu2000]

You aren't checking whether the form had any data entered. You're checking whether $sub evaluates to FALSE, then whether $username and $password evaluate to FALSE. At least one of them does not evaluate to FALSE.

[darkfreaks]

if(!isset($username) && !isset($password)) { //code here
}

[Christian F.]

The problem lies in the third line. sha1 () always produces a salt, even for an empty string ('da39a3ee5e6b4b0d3255bfef95601890afd80709', btw).

 

That said, you really should check for isset () directly on the $_POST values. By assigning them to variables before you check whether they're set or not, will always produce a notice about undefined indices if nothing has been posted. There is really no need to assign them to individual variables either, unless you've modified (or after validating) them. And then only to give a clear divide on what's the unmodified user-inputted data, and what's "safe" to use in the script.

 

You definitely should salt and hash the password as well, storing plain-text passwords is really bad. Not only because you will then be able to see your users' passwords, which they're almost certainly using for other sites as well, but also because anyone who gains access to your database (by legit means or not) can do the same.

I'm sure you've seen me post the link to this article about secure login systems before, and I still strongly recommend following its guidelines.

 

PS: You also should have a die () after any header ('Location: ....'); calls, to prevent the PHP parser from executing any remaining code in the script.

[White_Lily]

As I said in a different topic ive never killed my script after a header - since not killing has worked for me so far i will continue not killing scripts until something goes wrong.

Edited October 13, 2012 by White_Lily

[White_Lily]

Christian is right, it generates a value whether theres a post input or not, also - just writing if($variable) does check whether there is a value or not. Its just easier than writing if(isset($variable))

[Psycho]

You should NOT tell the user that the username or password do not match. Doing that gives a malicious user information that they can use to exploit the system. If you are not going to track invalid login attempts against usernames then you can simply do a query usign both the username AND the password. If there is no match then prevent login. You would only do a SELECT using just the username THEN compare the password if you want to keep track of invalid logins.

 

Also, do NOT store the password in session data. There is no (good) reason to do so. There are several other problems as wll: not using a salt, not escaping the username, etc.

 

Here is a much more logical flow:

 

Edit: removed code since OP is not appreciative of help given

Edited October 14, 2012 by Psycho

[Psycho]

Christian is right, it generates a value whether theres a post input or not, also - just writing if($variable) does check whether there is a value or not. Its just easier than writing if(isset($variable))

 

Just because somethign works and is easier does not mean it is the correct way. There are plenty of un-strict behavior that is changes in later versions of PHP. Best to use proper coding now than to have a lot of code break with newer versions of PHP.

[White_Lily]

The way I work is this:

 

Write the code in the easiest way for me,

 

If it works, leave it alone.

If it does NOT work, change it.

[White_Lily]

BTW - Your re-written code would return an error since the $get variable for $get["user_level"] doesnt exist without $get = mysql_fetch_assoc($getLogin);

[Jessica]

The way I work is this:

 

Write the code in the easiest way for me,

 

If it works, leave it alone.

If it does NOT work, change it.

And you've been doing this how long?

 

There's a difference between code that just barely works, and code that is built correctly from the ground up. When you have professionals giving you advice on how to do things the right way, and you choose to ignore it, you look like a child.

[White_Lily]

Since ive been coding PHP. Which is probably... 2-3 years... i think

[Jessica]

I think you missed the point of the rhetorical question.

[White_Lily]

Coding at 1:10am doesn't help.

[Psycho]

BTW - Your re-written code would return an error since the $get variable for $get["user_level"] doesnt exist without $get = mysql_fetch_assoc($getLogin);

 

OK, I rewrote your code in about 5 minutes in a much more logical and sound format. I don't have your database or other files needed to even test the code. So, I missed changing one single variable name. You sure put me in my place.

 

If you are not interested in learning good standard coding practices, that's your perogative. I'll just remember not to provide any help to you in the future.

Edited October 14, 2012 by Psycho

[White_Lily]

As I said, if my code doesn't work (which it didn't otherwise I wouldn't of posted this topic in the first place) then I change it. So I changed it to something that does work (which is your re-written code with the mysql_fetch_assoc() line where its needed). and it works fine now.

 

No need to get in a huff about it.

 

I'm interested in learning code otherwise I wouldn't have a job as a web developer. I just don't see why I should change something that works.

Edited October 14, 2012 by White_Lily

[Pikachu2000]

I just don't see why I should change something that works.

 

And therein lies the problem. You've been given reasons you should make changes, but instead of doing some research or testing on your own, you just hand wave sound advice away because you seem to think the mere fact that something "works" means it's the right way to do it. That's what they thought about the Hindenburg, too.

[White_Lily]

I don't remember saying its 'the right way' to do it if it works the way I do things O.o

 

All I said is that if my way works, then I leave it alone - Nothing about it being 'the right way'.

[Pikachu2000]

So you just plain don't care about anything past "it works". Got it.

[White_Lily]

I only care if it doesn't display whats its supposed to display, or it doesn't display at all. Thats the only time i care.

[Mahngiel]

So you just plain don't care about anything past "it works". Got it.

 

You haven't realized this is DD reincarnated with an attitude yet, have ya?