Stupid mistake #371

CromeDome on 2005-02-16T03:10:14

Valuable bonus points* will be awarded to the first person to detect the stupid error found in this code snippet:

# Check the login credentials my $username = untaint_string($self->query->param("username")); my $password = untaint_string($self->query->param("password"));

if($username ne "" and $password ne "") { use KW::Users; my $user = KW::Users->retrieve($username);

if($user and $user->login_password eq encrypt($password)) { # Last check! User info matches, but are they active? if($user->active_yn eq "Y") { # Log the user in $self->session->param("username", $username); $self->session->param("is_logged_in", 1); } } }

$self->param("message", "Please enter a valid username and password combination.") unless $self->session->param("is_logged_in", 1);

# Return the user to their chosen destination return $self->redirect($redirect);


Stupid mistake aside, there's a bigger issue above that I am unsure as to how to deal with. Traditionally, I've used the session parameter message when I've wanted to tell my output function to throw a message at the top of the page (to indicate an error, etc.). I've always done this in the context of a single script before - for example, user is trying to login (as above), fails the login (and therefore sets a message), and I call the function to redisplay the login form again, and the message gets displayed. Nowadays, I'm potentially redirecting to another script in another process, and so my message parameter goes out of scope. Any suggestions on how to do what I want?

Thanks in advance!

* Valuable Bonus Points have no real or imaginary value. They do, however, give you bragging rights (for whatever THAT is worth!) ;)


A few guesses

dws on 2005-02-16T05:08:52

You don't show a session becoming "un logged in" when a password doesn't match (nor do you show anything about session lifetime). And I would expect to stay on whatever the login page was rather than get redirected on a failed login attempt.

Re:A few guesses

CromeDome on 2005-02-16T05:27:40

Stupid error #372 is posting the corrected code without the bug removed. I meant to post this:

$self->param("message", "Please enter a valid username and password combination.")
                                unless $self->session->param("is_logged_in", 1);

when I originally posted this:

$self->param("message", "Please enter a valid username and password combination.")
                                unless $self->session->param("is_logged_in");


Oops. Perhaps I've worked too long tonight. . .

To address your other guesses, when a visitor hits the site, we check for an existing session, and if they don't have one, we start a new session (regardless of whether or not they are logged in, because users of this site may or may not have an account) and hand them a cookie with their session ID. When the browser closes, the cookie expires, and when they come back, they get a new session and cookie. I'm not sure if that's good or bad, but so far, it's worked out ok.

I can see where explicitly marking someone as being not logged in would be a good thing, and I thank you for pointing that out to me. I'm tired and starting to miss stuff.

As for redirection. . . my login and logout functionality is in one module, and maintenance functions are spread out in a few different modules. These modules check to see whether someone is logged in, and if not redirects them to the login page, and stashes the URL to get back the module in the session. If the login fails, they get redirected back to the maintenance module and back to login. . . I really need to make that less hackish. . .

Thanks again! :)