Implementation of login and registration functions in servlet filter

Question

I'm a newbie in servlet API. I have a test task to create a very simple web app with one home page and an authorization form. The requirement is to use filter somewhere in my app.

Given that it's a super simple app, I implemented basic login and registration functions right in my filter (it was a much simpler way for me to do it).

Here is the question: can I do this at all according to the best practices? Or should I only leave the cookie check in the filter and move all other functions to separate servlets? Is it necessary?

The filter itself:

@WebFilter("/") 
public class Filter implements javax.servlet.Filter {

    @Override
    public void init(FilterConfig filterConfig) throws ServletException {
    }

    @Override
    public void doFilter(final ServletRequest request,
                         final ServletResponse response,
                         final FilterChain filterChain)

            throws IOException, ServletException {

        final HttpServletRequest req = (HttpServletRequest) request;
        final HttpServletResponse resp = (HttpServletResponse) response;

        @SuppressWarnings("unchecked")
        final AtomicReference<UserDAO> dao = (AtomicReference<UserDAO>) req.getServletContext().getAttribute("dao");

        final String username = req.getParameter("username");
        final String password = req.getParameter("password");
        final String action = req.getParameter("action");
        final String remember = req.getParameter("remember");

        //Just in case
        final String path = req.getRequestURI().substring(req.getContextPath().length());

        if (path.startsWith("/resources/")) {
            filterChain.doFilter(request, response); // Goes to default servlet.
        }

        final Cookie[] cookies = req.getCookies();

        if (cookies != null) {
            for (Cookie cookie : cookies) {

                if (dao.get().checkUserById(cookie.getName()) &&
                    !cookie.getValue().isEmpty())
                {
                    req.setAttribute("username", dao.get().getUsernameById(cookie.getName()));
                    req.getRequestDispatcher("/WEB-INF/view/home.jsp").forward(req, resp);
                }
            }
        }

        if ((username != null) && (password != null) && (action != null)) {

            req.setAttribute("username", username);

            if (action.equals("registration")) {

                if (!dao.get().checkUserByUsername(username)) {

                    final String id = Integer.toString(dao.get().getSize() + 1);
                    final User user = new User(id, username, password);

                    dao.get().add(user);

                    if (remember != null && remember.equals("on")) {
                        Cookie cookie = getRememberMeCookie(id, username, password);
                        resp.addCookie(cookie);
                    }

                    req.getRequestDispatcher("/WEB-INF/view/home.jsp").forward(req, resp);

                } else {
                    req.getRequestDispatcher("/WEB-INF/view/login-form.jsp?registration").forward(req, resp);
                }

            } else if (action.equals("login")) {

                if (dao.get().checkUserByUsernameAndPassword(username, password)) {

                    String id = dao.get().getIdByUsername(username);

                    if (remember != null && remember.equals("on")) {
                        Cookie cookie = getRememberMeCookie(id, username, password);
                        resp.addCookie(cookie);
                    }

                    req.getRequestDispatcher("/WEB-INF/view/home.jsp").forward(req, resp);

                } else {
                    req.getRequestDispatcher("/WEB-INF/view/login-form.jsp?login").forward(req, resp);
                }
            }

        } else {
            req.getRequestDispatcher("/WEB-INF/view/login-form.jsp").forward(req, resp);
        }
    }

    @Override
    public void destroy() {
    }

    private String md5Hash(String username, String password) throws NoSuchAlgorithmException {

        String entryData = username + "md5Cookie" + password;
        MessageDigest m = MessageDigest.getInstance("MD5");

        byte[] data = entryData.getBytes();
        m.update(data,0,data.length);
        BigInteger i = new BigInteger(1,m.digest());

        return String.format("%1$032X", i);
    }

    private Cookie getRememberMeCookie(String id, String username, String password) {

        String md5CookieValue = null;

        try {
            md5CookieValue = md5Hash(username, password);
        } catch (NoSuchAlgorithmException e) {
            e.printStackTrace();
        }

        final Cookie cookie = new Cookie(id, md5CookieValue);
        cookie.setMaxAge(60 * 60 * 24 * 15);

        return cookie;
    }

}

I think almost all code after line 47 can be moved to separate servlets, just need a confirmation.

P.S. Logic is awful, I know. I'm newbie after all :)

Answer 1

Quickfire opinions:

• Don't use MD5 for anything. MD5 is cryptographically broken. Especially do not put passwords through it.
• Don't use passwords to generate an authentication token. Use a SecureRandom in conjunction with the claimed ID. Keep a server-side copy of the salt and check the authentication token on every request.
• Only require the password once and try to purge it from memory as soon as possible. This implies removing it from the req

• Use "guard clauses" to reduce the level of nesting in your methods and to return early. This allows you to reduce the amount of context you need to keep in your head when reading a given piece of code. Note that since java7 switch-case statements can also operate on Strings.
  This simplifies the checks for content:

  if (username == null || password == null || action == null) {
      req.getRequestDispatcher("/WEB-INF/view/login-form.jsp").forward(req, resp);
      return;
  }
  req.setAttribute("username", username);
  switch (action) {
      case "registration":
          if (dao.get().checkUserByUsername(username)) {
              req.getRequestDispatcher("/WEB-INF/view/login-form.jsp?registration").forward(req, resp);
              return;
          }
          // [...]
  

• Extract the technicalities of generating an AuthenticationToken into a separate class. Usually that class would either be static or injected through some kind of dependency injection mechanism. It can also be used to provide facilities for checking a given token.

• A Filter would make sense to check for authentication. To authenticate a request or to create a new user is something more suited to a Servlet.