Error Handling in Servlets and JSPs

Question

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

login.jsp:

<strong>LOG IN</strong>
<p>Please enter your credentials.</p>

<form action="process-login.jsp" method="post">
<table>
    <tr>
        <td>Username:</td>
        <td><input type="text" name="username" size="30" required title="Please enter your username."/></td>
    </tr>
    <tr>
        <td>Password:</td>
        <td><input type="password" name="password" size="30" required title="Please enter your password."/></td>
    </tr>
</table>
<br/>
<input type="submit" value="Login"/>
</form>

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:

<strong>LOGIN RESULT</strong>

<%
// retrieve objects from session
Boolean hasExistingAccount = session.getAttribute("hasExistingAccount") != null ? (Boolean) session.getAttribute("hasExistingAccount") : true;
Boolean isPasswordCorrect = session.getAttribute("isPasswordCorrect") != null ? (Boolean) session.getAttribute("isPasswordCorrect") : true;

if(!hasExistingAccount) {
%>

<form action="login.jsp">
<table>
    <tr><td style="color: #FF0000">User does not exist!</td></tr>
</table>
<br/>
<input type="submit" value="Try Again"/>
</form>

<% } else if(!isPasswordCorrect) { %>

<form action="login.jsp">
<table>
    <tr><td style="color: #FF0000">Username or password is incorrect!</td></tr>
</table>
<br/>
<input type="submit" value="Try Again"/>
</form>

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

(Kindly forgive the scriptlets for now, as it is what we have reached in our lessons so far, though I am now aware it is deprecated.)

I use plain if-else statements, where the result page gives the appropriate message. Is this an acceptable way to handle user input errors?

Answer 1

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.