Otdfbt

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"%>

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.

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.

You've received some good comments on the detailed coding, so I'll focus on the design.

• Follow the single responsibility principle in your controller, it should do one thing, dispatch the commands it receives.


• The SRP 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.


• Create a hierarchy of the access controlled commands to minimise the risk of a security/access holes. So all the controlled commands must inherit from an access controlled command.

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