Simple login with jsp

Question

I made a simple login page with Java EE, jsp, servlet, tomcat and jdbc.

It does following:

1. login user
2. register user
3. after login it creates token for the session so ya can be directed from start page if ya have already login
4. you can remember your login: then email and token is store in cookies
5. logout: clear cookies and session

Here is whole app: https://github.com/JulianRNajlepszy/simplelogin/tree/master/simplelogin

and here is the Controller class and Account class for review :)

package main;

import java.io.IOException;
import java.io.PrintWriter;
import java.sql.Connection;
import java.sql.SQLException;

import javax.naming.Context;
import javax.naming.InitialContext;
import javax.naming.NamingException;
import javax.servlet.ServletConfig;
import javax.servlet.ServletException;
import javax.servlet.http.Cookie;
import javax.servlet.http.HttpServlet;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
import javax.servlet.http.HttpSession;
import javax.sql.DataSource;

import bean.User;
import db.Account;

/**
 * Servlet implementation class Controller
 */
public class Controller extends HttpServlet {
    private static final long serialVersionUID = 1L;
    private DataSource ds;
    Account account;

    /**
     * @see HttpServlet#HttpServlet()
     */
    public Controller() {
        super();
    }

    /**
     * @see HttpServlet#HttpServlet()
     */
    public void init(ServletConfig config) throws ServletException {
        try {
            InitialContext initContext = new InitialContext();

            Context env = (Context) initContext.lookup("java:comp/env");

            ds = (DataSource) env.lookup("jdbc/loginjspjdbcDB");

            Connection conn = null;
            try {
                conn = ds.getConnection();
            } catch (SQLException e) {
                // TODO Auto-generated catch block
                e.printStackTrace();
                return;
            }
            this.account = new Account(conn);
        } catch (NamingException e) {
            throw new ServletException();
        }
    }

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

        HttpSession session = request.getSession();
        String page = (String) request.getParameter("page");

        request.setAttribute("email", "");
        request.setAttribute("message", "");

        if (session.getAttribute("email") == null) {
            String cookieEmail = getCookie(request, "email");
            if(cookieEmail != null) {
                session.setAttribute("email", cookieEmail);
            }
        }
        if (session.getAttribute("token") == null) {
            String cookieToken = getCookie(request, "token");
            if(cookieToken != null) {
                session.setAttribute("token", cookieToken);
            }
        }

        if (page == null) {
            try {
                String email = (String) session.getAttribute("email");
                String token = (String) session.getAttribute("token");
                if (account.isLoginNow(email, token)) {
                    request.setAttribute("email", session.getAttribute("email"));
                    request.getRequestDispatcher("/succes.jsp").forward(request, response);
                    return;
                }
            } catch (SQLException e1) {
                // TODO Auto-generated catch block
                e1.printStackTrace();
            }
            request.getRequestDispatcher("/index.jsp").forward(request, response);
        } else if (page.equals("login")) {
            request.getRequestDispatcher("/login.jsp").forward(request, response);
        } else if (page.equals("register")) {
            request.getRequestDispatcher("/register.jsp").forward(request, response);
        } else {
            PrintWriter out = response.getWriter();
            out.print("<html><h1>404</h1></html>");
        }
    }

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

        HttpSession session = request.getSession();
        PrintWriter out = response.getWriter();

        String action = request.getParameter("action");
        request.setAttribute("email", request.getParameter("email"));

        if (action.equals("login")) {
            String email = request.getParameter("email");
            String password = request.getParameter("password");
            String tmpRemember = request.getParameter("remember");
            boolean remember = tmpRemember != null && tmpRemember.equals("true");

            try {
                if (!account.isLoginExist(email)) {
                    request.setAttribute("message", "email doesn't exist");
                    request.getRequestDispatcher("/login.jsp").forward(request, response);
                    return;
                }
                if (!account.login(email, password)) {
                    request.setAttribute("message", "bad password");
                    request.getRequestDispatcher("/login.jsp").forward(request, response);
                    return;
                }
            } catch (SQLException e) {
                // TODO Auto-generated catch block
                e.printStackTrace();
            }

            String token = TokenGenerator.generate(20);
            session.setAttribute("email", email);
            session.setAttribute("token", token);
            try {
                account.remember(email, token);
            } catch (SQLException e) {
                // TODO Auto-generated catch block
                e.printStackTrace();
            }

            if (remember) {
                response.addCookie(new Cookie("email", email));
                response.addCookie(new Cookie("token", token));
            } else {
                eraseCookie(request, response);
            }

        } else if (action.equals("register")) {
            User user = new User();
            String email = request.getParameter("email");
            String password = request.getParameter("password");
            String password2 = request.getParameter("password2");
            user.setEmail(email);
            user.setPassword(password);
            user.setPassword2(password2);

            if (!user.isValid(email, password, password2)) {
                request.setAttribute("message", user.getValidationMessage());
                request.getRequestDispatcher("/register.jsp").forward(request, response);
                return;
            }

            try {
                if (account.isLoginExist(email)) {
                    request.setAttribute("message", "The email is already in use. Change it.");
                    request.getRequestDispatcher("/register.jsp").forward(request, response);
                    return;
                }
            } catch (SQLException e) {
                out.println("Problem with database, cannot check if the email is already in use.");
                e.printStackTrace();
            }

            try {
                account.register(email, password);
                request.getRequestDispatcher("succesregister.jsp").forward(request, response);
                return;
            } catch (SQLException e) {
                out.println("Cannot register.");
                // TODO Auto-generated catch block
                e.printStackTrace();
            }

        } else if (action.equals("logout")) {
            String toRemove = (String) session.getAttribute("email");
            if (toRemove != null) {
                session.removeAttribute("email");
                session.removeAttribute("token");
                try {
                    account.removeToken(toRemove);
                } catch (SQLException e) {
                    // TODO Auto-generated catch block
                    e.printStackTrace();
                }
            }
            eraseCookie(request, response);
            request.getRequestDispatcher("/index.jsp").forward(request, response);
            return;
        }
        request.getRequestDispatcher("/succes.jsp").forward(request, response);
    }

    /**
     * @author Gray
     *         https://stackoverflow.com/questions/890935/how-do-you-remove-a-cookie-in-a-java-servlet
     *
     */
    private void eraseCookie(HttpServletRequest req, HttpServletResponse resp) {
        Cookie[] cookies = req.getCookies();
        if (cookies != null)
            for (Cookie cookie : cookies) {
                cookie.setValue("");
                cookie.setPath("/");
                cookie.setMaxAge(0);
                resp.addCookie(cookie);
            }
    }

    private String getCookie(HttpServletRequest request, String name) {
        Cookie[] cookies = request.getCookies();
        if(cookies != null) {
            for(Cookie cookie : cookies) {
                if(cookie.getName().equals("name")) {
                    return cookie.getValue();
                }
            }
        }
        return null;
    }

}

.

package db;

import java.sql.Connection;
import java.sql.PreparedStatement;
import java.sql.ResultSet;
import java.sql.SQLException;

public class Account {

    public Connection conn;

    public Account(Connection conn) {
        this.conn = conn;
    }

    public boolean login(String login, String password) throws SQLException {
        String passwordInDb = "";

        String sql = "select password as password from users where email = ?";
        PreparedStatement stmt = conn.prepareStatement(sql);
        stmt.setString(1, login);
        ResultSet rs = stmt.executeQuery();
        if (rs.next()) {
            passwordInDb = rs.getString(1);
        }
        return passwordInDb.equals(password);
    }

    public void register(String login, String password) throws SQLException {
        if (isLoginExist(login)) {
            throw new RuntimeException("Login already in use!");
        }
        String sql = "insert into users (email, password) values(?, ?)";
        PreparedStatement stmt = conn.prepareStatement(sql);
        stmt.setString(1, login);
        stmt.setString(2, password);
        stmt.executeUpdate();
    }

    public boolean isLoginExist(String login) throws SQLException {
        String sql = "select count(*) as no from users where email = ?";
        PreparedStatement stmt = conn.prepareStatement(sql);
        stmt.setString(1, login);
        ResultSet rs = stmt.executeQuery();

        int count = 0;
        if (rs.next()) {
            count = rs.getInt("no");
        }

        rs.close();

        return count > 0;
    }

    public void remember(String email, String token) throws SQLException {
        removeToken(email);

        String sql = "insert into active_users (email, token) values(?, ?)";
        PreparedStatement stmt = conn.prepareStatement(sql);
        stmt.setString(1, email);
        stmt.setString(2, token);
        stmt.executeUpdate();
    }

    public boolean isLoginNow(String email, String token) throws SQLException {
        String sql = "select token from active_users where email = ?";
        PreparedStatement stmt = conn.prepareStatement(sql);
        stmt.setString(1, email);
        ResultSet rs = stmt.executeQuery();
        String validToken = "";
        if (rs.next()) {
            validToken = rs.getString("token");
        }
        rs.close();
        return validToken != null && validToken.length() > 0 && validToken.equals(token);
    }

    public void removeToken(String email) throws SQLException {
        String sql = "delete from active_users where email = ?";
        PreparedStatement stmt = conn.prepareStatement(sql);
        stmt.setString(1, email);
        stmt.executeUpdate();
    }
}

Answer 1

Object Oriented

So, from a object oriented perspective, stuff is a bit chaotic.

Servlet

What bothers me the most is that there's "too much code" in the servlet. Just think about that: What if, let's say, you want to introduce a presentation framework (because servlet programming is a bit year ~2003)? Which parts would you have to move 'to somewhere else', or which parts aren't reusable?

Account

First, I thought it's a type which represents an actual account, but it's actually ... well a mixture of business-object/domain model and data access object. It does do some sort of logic, like validating an existing login, and it does execute queries on the backend. "Usually", or "a common used pattern is", you have a layer which is dedicated to read and write data. A dedicated layer which does the 'business logic', and the dedicated layer which displays the data and takes data from the user. That's called a 'three tier architecture', which helps you to 'separate the concerns'.

Readability

Servlet.doGet

The servlet has a bit of a readability problem. It starts with the problem, that you have code in your 'doPost' and code in your 'doGet' and it's a bit hard to understand what happens where, or rather what should happen where.

Let's look at that 'set-empty-string-to-that-attribute-in-request-method':

    request.setAttribute("email", "");
    request.setAttribute("message", "");

What are you clearing the email and the message? Looks important. I think it's only in the doGet. Maybe wrap it in a separate method which describes what you are doing here.

And then the 'cookie-to-session-stuff'

    if (session.getAttribute("email") == null) {
        String cookieEmail = getCookie(request, "email");
        if(cookieEmail != null) {
            session.setAttribute("email", cookieEmail);
        }
    }

So, you're checking if the email attribute is not in the session, then get the cookie, and then set it to the session. Maybe wrap it in a initializeEmail-Method or something? Same goes for the token. And I'm pretty sure you can write one method which works for the email and for the token.

This if is also confusing

    if (page == null) {

Why does the code have to be run, when no page is set?

And this method

if (account.isLoginNow(email, token)) {

... is badly named. I don't understand what it does, until I go and check the implementation of it. This is bad, because it's like having to open another tab, read a text, and go back again, it messes with the initial train of thought.

Servlet.doPost

This method is waaaaay too long. And you could easily take that a part. For every "if(action)" statemend, you can move the code within in a separate method, like "performAction" method. If you'd do that, a reader of the code sees in a few lines what the doPost is responsible for. And if you want to know what a certain part does, it's isolated in a separate method.

The attribute remember: Remember what?

This is a bit redundant:

        user.setEmail(email);
        user.setPassword(password);
        user.setPassword2(password2);

        if (!user.isValid(email, password, password2)) {

First, you set the attribute to the user. And the isValid method needs those attributes?

Account

• login: Well, this one is not well named either. It actually does not login. It validates a user's password.
• in general, you use 'login' as your parameter name, but not always, sometimes email. Either use one or the other.
• isLoginExist: It's "loginExists".
• remember: Yeah that could use a proper name, too. rememberWhat()?
• isLoginNow: Yeaaaah, that one, too. I'd say 'verifyToken' or something would be more helpful.
• A rule is, that a method name must contain a verb. But a noun usually helps the readability, too. For instance, you have a method name removeToken, that's much more clear than remember().

Other stuff

• Use constants! The string action, login, etc. are all over the place
• request.getRequestDispatcher("/login.jsp").forward(request, response): This code is used a lot. Maybe dedicated methods 'forwardToLogin' would be helpful.
• You must not save passwords in clear text. Never ever do that.
• Empty catch blocks / e.printStackTrace: Never ever leave that. Not even if it's for a review. It's offensive! :P
• Your connections / statement handling will lead to out of memory exceptions. Always use a "try-with-resource" block. But working with java.sql api is a subject of its own.
• You have a JavaDoc for instance at the doPost method. That's just clutter.

Hope this helps,

slowy