Error Handling in Servlets and JSPs

I have an HTML login form that is processed by a servlet, which then redirects the user to a result page.

login.jsp:

LOG IN
Please enter your credentials.

    
        Username:
        
    
    
        Password:
        
    

Servlet (process-login.jsp):

protected void doPost(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException {
    String userId = request.getParameter("username");
    String userPassword = request.getParameter("password");

    if(userId != null && userPassword != null) {
        DatabaseManipulator dm = new DatabaseManipulator();
        String[] usernameArray = dm.dbEntriesToArray("user_id");
        String registeredPassword = dm.getRegisteredPassword(userId);

        LoginModule lm = new LoginModule();
        boolean hasExistingAccount = lm.hasExistingAccount(usernameArray, userId);
        boolean isPasswordCorrect = lm.isPasswordCorrect(userPassword, registeredPassword);

        // returns new session if it does not exist
        HttpSession session = request.getSession(true);

        // bind objects to session
        session.setAttribute("hasExistingAccount", hasExistingAccount);
        session.setAttribute("isPasswordCorrect", isPasswordCorrect);

        if(!hasExistingAccount) {
            response.sendRedirect("login-result.jsp");

        } else if(!isPasswordCorrect) {
            response.sendRedirect("login-result.jsp");

        // login success
        } else {
            String currentUserId = dm.getCurrentUserId(userId);
            int currentUserType = dm.getCurrentUserType(userId);

            session.setAttribute("currentUserId", currentUserId);
            session.setAttribute("currentUserType", currentUserType);

            response.sendRedirect("index.jsp");
        }

    } else {
        // redirect on illegal access of servlet
        response.sendRedirect("index.jsp");
    }
}

login-result.jsp:

```
LOGIN RESULT

User does not exist!

This doesn't look good:

DatabaseManipulator dm = new DatabaseManipulator();
String[] usernameArray = dm.dbEntriesToArray("user_id");
String registeredPassword = dm.getRegisteredPassword(userId);

LoginModule lm = new LoginModule();
boolean hasExistingAccount = lm.hasExistingAccount(usernameArray, userId);
boolean isPasswordCorrect = lm.isPasswordCorrect(userPassword, registeredPassword);

The interaction between the current class, DatabaseManipulator and LoginModule is not natural and inefficient.
Why get an array of usernames from a DatabaseManipulator and pass that to LoginModule?
These classes are too tightly coupled: they know too much about each other.
The current class gets something from DatabaseManipulator and passes it on to LoginModule.
A system with too many connections between its elements like this is complex and rigid.

The responsibilities are also not well separated.
The current class shouldn't need to know the details of validating users,
but delegate to a dedicated UserManager class.
Consider these methods and responsibilities:

• UserManager.isValidUser(String username): check if the user exists. The class will hide how this check is performed. Maybe it loads an array of all usernames, like the original code. Hopefully it doesn't, but to outsiders it won't matter. You can extend it later: you might add the concept of disabled users. This class and method will be the single place to make that change, the rest of your program doesn't have to know how it works.

• UserManager.isPasswordCorrect(String username, String password): check if the password is correct

The original code would then become:

UserManager userManager = new UserManager();
boolean hasExistingAccount = userManager.isValidUsername(userId);
boolean isPasswordCorrect = userManager.isPasswordCorrect(userId, userPassword);

This is more intuitive, the details of user validation are now correctly hidden from the current class, and it's shorter too.

Security

I'm not sure it's such a good idea to tell the user when a username doesn't exist.
One can discover the valid usernames on the site by a brute-force attack.
It might be somewhat better to give the same error message whether the username exists or not, something:


The username or password you entered is incorrect.

Minor things

These conditions can be joined:

if(!hasExistingAccount) {
    response.sendRedirect("login-result.jsp");

} else if(!isPasswordCorrect) {
    response.sendRedirect("login-result.jsp");

like this:

if (!hasExistingAccount || !isPasswordCorrect) {
    response.sendRedirect("login-result.jsp");
}

When you have code like this:

if (userId != null && userPassword != null) {

    // many, many, many lines of code...

} else {
    // redirect on illegal access of servlet
    response.sendRedirect("index.jsp");
}

... by the time you read until the else block, it can be difficult to remember what the if was about... In such situations it's better to make the shortest block come first by reverting the condition:

if (userId == null || userPassword == null) {
    // redirect on illegal access of servlet
    response.sendRedirect("index.jsp");
} else {
    // many, many, many lines of code...
}

or, it might be even more natural to return early and drop the else block,
reducing one level of indentation:

if (userId == null || userPassword == null) {
    // redirect on illegal access of servlet
    response.sendRedirect("index.jsp");
    return;
}

// many, many, many lines of code...

This is especially useful with more complex, arrow-shaped code, with many nested conditions.

StackExchange Code Review Q#67274, answer score: 2