Java Servlet & JSP simple login

Question

I am currently learning Java Servlets and JSP at university. As a little practice I created a project where I want to be able to login into a protected area. Now, I got it working by using a RequestDispatcher. However, after I have been forwarded to the protected area, the URL in the browser shows http://localhost:8080/jspractice02/FrontController. I think it would make more sense to be http://localhost:8080/jspractice02/protected/dashboard.jsp. Should I use sendRedirect instead of a Dispatcher? Another thing is that I don't know if my code structure is somewhat senseful, especially how I handle the login. I would love to get some feedback on that.

Directory Structure

+-- src
|   +-- main
|       +-- java
|       |   +-- de.practice.PresentationLayer
|           |   +-- FrontController.java
|       +-- webapp
|       |   +-- home.jsp
|       |   +-- protected
|           |   +-- dashboard.jsp
|           |   +-- add-student.jsp

FrontController.java

package de.practice.Presentation;

import java.io.IOException;
import java.util.ArrayList;

import javax.servlet.RequestDispatcher;
import javax.servlet.ServletException;
import javax.servlet.annotation.WebServlet;
import javax.servlet.http.HttpServlet;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
import javax.servlet.http.HttpSession;

/**
 * Servlet implementation class FrontController
 */
@WebServlet("/FrontController")
public class FrontController extends HttpServlet {
    private static final long serialVersionUID = 1L;


    private static String username = "";
    private static String password = "";
    private static String queryString = "";


    /**
     * Constructor
     */
    public FrontController() {
        super();
    }


    /**
     * doGet
     */
    protected void doGet(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException {

        String jsp = "";
        queryString = request.getParameter("action");

        if(queryString != null) {

            switch(queryString) {
                case "addStudent":
                    jsp = "protected/student.jsp";
            }

            RequestDispatcher rd = request.getRequestDispatcher(jsp);
            rd.forward(request, response);
        }
    }



    /**
     * doPost
     */
    protected void doPost(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException {

        doGet(request, response);
        login(request, response);

    }



    /**
     * validPassword
     * @param request
     * @param response
     * @param password
     * @return
     */
    private boolean validPassword(HttpServletRequest request, HttpServletResponse response, String password) {

        if(password.length() >= 3) {
            return true;
        }
        return false;
    }


    /**
     * validUsername
     * @param request
     * @param response
     * @param username
     * @return
     */
    private boolean validUsername(HttpServletRequest request, HttpServletResponse response, String username) {

        if(username.length() >= 3) {
            return true;
        }
        return false; 
    }


    /**
     * 
     * @param request
     * @param response
     * @param password
     * @param username
     * @throws IOException
     * @throws ServletException
     */
    private void login(HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException {
        // Get username and password
        username = request.getParameter("username");
        password = request.getParameter("password");


        // check if they are valid
        if(validUsername(request, response, username) && validPassword(request, response, password)) {

            // If valid create session
            HttpSession session = request.getSession();
            session.setAttribute("username", username);

            // and redirect to protected area
            RequestDispatcher rd = request.getRequestDispatcher("protected/dashboard.jsp");
            rd.forward(request, response);

        } else {
            // Code for invalid login
            RequestDispatcher rd = request.getRequestDispatcher("login-failed.jsp");
            rd.forward(request, response);

        }
    }




}

<%@ include file="partials/header.jsp"%>

<section id="start" class="panel">
    <div class="container">
        <h1>Home</h1>

        <form class="form" method="post" action="FrontController">
            <div class="form-group">
                <label for="username">Username</label>
                <input type="text" name="username" class="form-control">
            </div>
            <div class="form-group">
                <label for="password">Password</label>
                <input type="password" name="password" class="form-control">
            </div>

            <input type="submit" name="submitBtn" class="btn btn-primary">
        </form>

    </div>
</section>

<%@ include file="partials/footer.jsp"%>

Answer 1

Critical error

private static String username = "";
private static String password = "";
private static String queryString = "";

You should never keep request-scoped data in a field in a servlet. Servlet is created only once during application startup, and shared among all requests. So if several clients connect to your servlet at the same time, they might not reach the protected page, even if they provide correct password. It will also lead to setting wrong username attribute in the session object. If you had customized pages for each user, this could also lead to people seeing protected pages of other people.

Solution - extract the sensitive request-scoped data in doGet method only. Also, simply removing the static keyword will not resolve the issue.

Code style

1. Get rid of empty JavaDoc that only makes the code harder to read
2. Get rid of obvious comments that only make the code harder to read, like // Get username and password
3. Do not introduce redunant code (constructor that only calls super())
4. Improve method ordering. When you call validUsername() in login(), then validUsername() should be below login() in the class. this improves readability.
5. Get rid of redundant logic. For example, the validUsername() method body can be elegantly expressed in one line - return username.length() >= 3. By the way, I think this method should be named isUsernameValid() in my opinion.
6. Do not pass unnecessary arguments to methods. For example, validUsername() does not need request and response.

Other

The browser shows http://localhost:8080/jspractice02/FrontController because you've annotated the class with @WebServlet("/FrontController"). Please read the documentation on how that works.

I'm not sure if calling doGet() from doPost() is necessary (and I'm not sure if it's best practice). Your doPost() should just login the user and redirect them if the credentials provided are valid.

Answer 2

package de.practice.Presentation

That's not a valid package name under the Java convention. Packages are all lowercase with maybe underscores.

---

private static String username = "";

You don't want these static, actually, you don't want them as fields at all. When dealing with requests, you have to consider that one instance of the same class might handle multiple requests simultaneously. Having a state in the class (f.e. fields) has to be carefully considered.

---

protected void doGet(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException {

    String jsp = "";
    queryString = request.getParameter("action");

    if(queryString != null) {

        switch(queryString) {
            case "addStudent":
                jsp = "protected/student.jsp";
        }

        RequestDispatcher rd = request.getRequestDispatcher(jsp);
        rd.forward(request, response);
    }
}

Consider mapping your requests and actions in a way which allows you to refer to locations directly without having to resort to a big switch statement.

---

private boolean validPassword(HttpServletRequest request, HttpServletResponse response, String password) {

    if(password.length() >= 3) {
        return true;
    }
    return false;
}

We all know that's just a placeholder, but you could return directly:

return password.length() >= 3;

---

RequestDispatcher rd = request.getRequestDispatcher("login-failed.jsp");
rd.forward(request, response);

You're repeating this action multiple times, consider a helper function. But you could also write that in one line.

---

/**
 * Constructor
 */

You might as well omit the Javadoc if it's not going to be useful.

---

Answer 3

You've received some good comments on the coding style, so I'll focus on the best practice principles of structure and design.

• Follow the single responsibility principle in your controller, it should do one thing, dispatch the commands it receives. A class should do one thing, the front controller shows how this can be decomposed in a flexible and expansive way.
• Applying the SRP with a FrontController will make it unnecessary to change the controller to add new commands. Replace your switch with a command mapping, see the Command Pattern. A good approach to this is to load the commands from a property file into a Hashmap; so Command=ClassName that maps the command-name to a class name.
• Use the Class.forName(className).newInstance() idiom to construct new instances of the commands on demand. Commands should qualify as light weight classes for immediate construction.
• Create a hierarchy of commands, this will allow controlled and uncontrolled commands. The access controlled commands will to minimise the risk of a security/access holes when all the controlled commands inherit from an access controlled command.

See this answer for a more elaborate explanation of this approach : https://softwareengineering.stackexchange.com/a/345714/241947