php login script - remember me

Question

Can anyone see anything wrong with this login script:

public function login($username, $pass, $remember) {
// check username and password with db
    // else throw exception

        $connect = new connect(); 
$conn = $connect->login_connect();

        // check username and password
        $result = $conn->query("select * from login where
                               username='".$username."' and
                               password=sha1('".$pass."')");
        if (!$result) {
            throw new depException('Incorrect username and password combination. Please try again.');
        } else {
            echo $username, $pass;
        }

To explain:

At the moment the script is allowing anything through. In other words the query is returning true for any username and password that are passed to it. I've put the echo statement just as a check - obviously the script would continue in normal circumstances!

I know that the connect class and login_connect method are working because I use them in a register script that is working fine. depException is just an extension of the Exception class.
The function login() is part of the same class that contains register() that is working fine.
I know that the two variables ($username and $pass) are getting to the function because the echo statement is outputting them accurately. (The $remember variable is not needed for this part of the script. It is used later for a remember me process).
I'm stumped. Please help!

UPDATE

Thanks for those responses. I was getting confused with what the query was returning. The complete script does check for how many rows are returned and this is where the checking should have been done. Everything is now working EXCEPT for my remember me function. Perhaps someone could help with that?!?! Here is the full script:

    public function login($username, $pass, $remember) {
    // check username and password with db
        // else throw exception

        $connect = new connect(); 
$conn = $connect->login_connect();

        // check username and password
        $result = $conn->query("select * from login where
                               username='".$username."' and
                               password=sha1('".$pass."')");
        if (!$result) {
            throw new depException('Incorrect username and password combination. Please try again.');
        } 

       if ($result->num_rows>0) {
            $row = $result->fetch_assoc();
            //assign id to session
            $_SESSION['user_id'] = $row[user_id];

            // assign username as a session variable
            $_SESSION['username'] = $username;

           // start rememberMe
    $cookie_name = 'db_auth';

    $cookie_time = (3600 * 24 * 30);*/ // 30 days

    // check to see if user checked box
    if ($remember) {
    setcookie ($cookie_name, 'username='.$username, time()+$cookie_time);
    }


            // If all goes well redirect user to their homepage.
    header('Location: http://localhost/v6/home/index.php'); 

        } else {
           throw new depException('Could not log you in.);
        }
}

Thanks very much for your help.

UPDATE 2!

Thanks to your help I've got the main part of this script working. However, the remember me bit at the end still doesn't want to work. Could someone give me a hand to sort it out? $username, $pass and $remember are all short variable names that I assigned before passing them to the function to save writing $_POST['username'] etc. everytime. $remember refers to a checkbox.

Answer 1

Your query is open for sql-injections...

SELECT *
FROM users
WHERE username = '' OR 'a' = 'a'
AND password = sha1('guessAnyPassword')

I'd also check your result, and base the action on how many records were returned.

if (mysql_num_rows($result) > 0)

Answer 2

What does $conn->query() return, a MySQL resource object like mysql_query() does? If so then it'll always compare "true". mysql_query() only returns FALSE if the query completely fails, like it has a syntax error or a table doesn't exist.

To check if you got any results you need to try to fetch a row from the result set and see if you get anything, via whatever your equivalent of mysql_fetch_row() is.

Important: Your script is vulnerable to SQL injection attacks, or even just odd usernames like o'neil with an apostrophe. You should escape all variables in a query with mysql_real_escape_string() (or equivalent) to make sure your query doesn't get messed up by special characters. Or, even better, use prepared statements which look like

select * from login where username=? and password=sha1(?)

---

Re: UPDATE

Variables from a form are available via either $_GET or $_POST, depending on which method was used to submit the form. Try if (isset($_POST['remember'])) to see if that check box was checked.

Important: I see that you tried to use a bare $remember to see if the check box was checked. That suggests to me that you are trying to take advantage of the register_globals feature in PHP which makes your GET and POST variables accessible via regular variable names. If that is the case you should heed the warning in the PHP manual!

WARNING

[register_globals] has been DEPRECATED as of PHP 5.3.0 and REMOVED as of PHP 6.0.0. Relying on this feature is highly discouraged.

Use $_GET and $_POST instead. I could tell you how to make if ($remember) work, actually, but given the inherent evil-ness of register_globals I'm not gonna! ;-)

Answer 3

In php most queries only return False if there was an error executing them. Your query is returning a value, probably an empty array of values. This is not a false value as far as your if statement is concerned.

Check how many rows are returned. The function to do this will depend on your abstraction layer (connect class etc...)