2
\$\begingroup\$

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 :)

\$\endgroup\$
0
\$\begingroup\$

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.
\$\endgroup\$
  • \$\begingroup\$ Thank you for such an answer! Never expected to get such a full explanation. Regarding the last point: So I need to leave cookie check in the filter and move everything regarding authentication and registration to separate servlets, right? \$\endgroup\$ – LexSav Jan 30 at 8:43
  • \$\begingroup\$ Yes. A Filter changes a request, a servlet handles it. Login and registration are actions, authorization is a change \$\endgroup\$ – Vogel612 Jan 30 at 9:13
  • \$\begingroup\$ One more stupid question :) Why do we need return; statement in the code you provided? \$\endgroup\$ – LexSav Jan 31 at 9:09
  • \$\begingroup\$ And what do you think about thread-safety? Is it more or less enough for such kind of an app to declare variables final in filter and servlets and be done with it?) \$\endgroup\$ – LexSav Jan 31 at 10:19
  • \$\begingroup\$ The return statement is necessary to allow removing the explicit else. If there was no return statement, the code after the closing brace of the if-block would need to be put into an else block to do the same thing. IIRC all application servers out there handle thread safety of Request and Response correctly. Local variables can not have any problems with thread-safety, that's just not how they work. As such the code you have is perfectly threadsafe already \$\endgroup\$ – Vogel612 Jan 31 at 10:41

Your Answer

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

Not the answer you're looking for? Browse other questions tagged or ask your own question.