Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

We have a User class which handles all things related to users within a Content Management System. As you'd expect, the class contains a function login(). This function accepts three arguments: $username, $password and $remember (the latter defaulting to true).

My question is: what is the best way to handle login errors so that they can be displayed to the user? We're using a custom MVC model here, so we have controllers and views that need to remain abstract from the class itself. I have identified the following possible solutions:

  1. Return signed integers from the login() function that can be interpreted inside the controller.
  2. Add a variable to the User object containing the error message in plain text
  3. Output the error directly from within the function call

Point 3 is moot, since we won't be doing that (coupling output with a class == bad).

Point 2 seems to be the most logical, but will make multi-language (a feature that may well be added later) difficult. For example, we might do something like this:

class User
{
    public $loginError;

    public function login($username, $password, $remember = true)
    {
        if($password !== $ourStoredPassword)
        {
            $this->loginError = 'Password does not match';
            return false;
        }
    }
}

I think the best option is #1. For example, we could do this inside the Class:

class User
{
    public function login($username, $password, $remember = true)
    {
        if($password !== $ourStoredPassword)
        {
            return -1;
        }
    }
}

Then inside our controller, just look at the return value from login(), and handle it therein. What are people's thoughts? Is there a better way to tackle this?

share|improve this question
    
I'd go with #2. You can always have a translation module that uses the messages as keys. #1 introduces another problem, what do you do with the switch that was introduced in the controller? –  devnull Feb 19 at 14:08
    
Very good point, @devnull. I guess we could have an assoc. array that contains the language-related verbose error message, and then handle it within a tx module. Thanks for the input (hand't thought of doing it that way). –  BenM Feb 19 at 14:11

2 Answers 2

up vote 6 down vote accepted

Why not create a series of Exception objects for Login Errors such as UserNotFoundException and PasswordIncorrectException.

You can then throw the exceptions within your login method when something fails.

You should then catch this Exception within your Controller and show the output to the user as appropriate. This approach removes the coupling between the error and the message you wish to display.

http://en.wikipedia.org/wiki/Exception_handling

share|improve this answer

When it comes to user login errors I recommend a simple true/false return value.

By giving additional information above and beyond either success or failure, you tell potential malicious users what names are associated with the system (by continuously guessing names and logging the error results). If email addresses can be used as a login you'd risk letting spambots scan your system for valid email addresses.

Additionally, this helps adhere to the KISS principle; the less moving parts you have the less likely things are to break later. If you program several things around a verbose error system and you decide to change it later, it could break in other areas.

By having the system return "username or password incorrect" people know to double-check their information. And since it should wipe the password between attempts users will only get to double-check the name anyway.

share|improve this answer
    
Whilst I agree that the reason for failure should not be transparent to the user in the majority of cases, I disagree with only the use of a true/false return statement. What if we wanted to handle multiple failed password attempts for a user, or as you suggested detect that a bot is repeatedly attempting email addresses that do not exists in the system, perhaps obtained from a data leak elsewhere. At least exceptions provide a simple way to handle these scenarios cleanly. –  Andy Feb 22 at 5:27

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.