Login module with JDBC

Question

I made simple login module that is responsible for handling registration, and signing in my application. How can I improve this code? Are there any conventions for this case?

The idea is to create siteUser and link him to his unique Account with cash info. Then let him login to app.

package auctionsite.login;
import java.sql.Connection;
import java.sql.DriverManager;
import java.sql.PreparedStatement;
import java.sql.ResultSet;
import java.sql.SQLException;
import auctionsite.stuff.IRegister;
public class LoginHandler implements IRegister {
 public static LoginHandler instance = null;
 private static final String DB_URL = "jdbc:derby:auction_site;create=true";
 private Connection conn = null;
 public static LoginHandler getInstance() {
 return instance == null ? new LoginHandler() : instance;
 }
 private int getUserID(String username) {
 try {
 Connection conn = DriverManager.getConnection(DB_URL);
 PreparedStatement prepStmt = conn.prepareStatement("SELECT userId FROM SiteUser WHERE name =?");
 prepStmt.setString(1, username);
 ResultSet rs = prepStmt.executeQuery();
 rs.next();
 return rs.getInt(1);
 } catch (SQLException e) {
 e.printStackTrace();
 }
 return 0;
 }
 private void insertNewUser(String username, String pass) {
 try {
 Connection conn = DriverManager.getConnection(DB_URL);
 PreparedStatement prepStmt = conn.prepareStatement("INSERT INTO SiteUser (name,password) VALUES (?,?)");
 prepStmt.setString(1, username);
 prepStmt.setString(2, pass);
 prepStmt.executeUpdate();
 } catch (SQLException e) {
 e.printStackTrace();
 }
 }
 private void createAccountForUser(String username) {
 try {
 Connection conn = DriverManager.getConnection(DB_URL);
 PreparedStatement prepStmt = conn.prepareStatement("INSERT INTO PaymentAccount (cash,userId) VALUES (?,?)");
 prepStmt.setDouble(1, 0.00);
 prepStmt.setInt(2, getUserID(username));
 } catch (SQLException e) {
 e.printStackTrace();
 }
 }
 @Override
 public int register(String username, String pass) {
 PreparedStatement prepStmt = null;
 try {
 conn = DriverManager.getConnection(DB_URL);
 prepStmt = conn.prepareStatement("SELECT COUNT(*) FROM SiteUser WHERE name=?");
 prepStmt.setString(1, username);
 ResultSet rs = prepStmt.executeQuery();
 rs.next();
 if (rs.getInt(1) <= 0) {
 if (validatePass(pass) && validateUsername(username)) {
 insertNewUser(username, pass);
 createAccountForUser(username);
 return 1;
 } else {
 return 0;
 }
 }
 } catch (SQLException e) {
 e.printStackTrace();
 }
 return -1;
 }
 private boolean validateUsername(String username) {
 return username.length() > 3;
 }
 private boolean validatePass(String pass) {
 return pass.length() >= 10 & pass.matches(".*\\d+.*");
 }
 @Override
 public void delete() {
 }
 @Override
 public int login(String userName, char[] pass) {
 try {
 conn = DriverManager.getConnection(DB_URL);
 PreparedStatement prepStmt = conn
 .prepareStatement("SELECT name,password FROM SiteUser WHERE name=? AND password=?");
 prepStmt.setString(1, userName);
 prepStmt.setString(2, new String(pass));
 ResultSet rs = prepStmt.executeQuery();
 if (rs.next()) {
 return 1;
 }
 } catch (SQLException e) {
 e.printStackTrace();
 }
 return 0;
 }
}

• Ankit Soni: Is this a working code? Because I see method createAccountForUser preparing the statement and do nothing after that. Also, you aren't closing the connections or resultset anywhere.
• jas97: Thanks, I overlooked this method... Works well right now after adding executeQuery

Answer 1

Checking return values

A mistake I see in multiple places is not checking the return values of operations.

Take for example the part of getting the user ID:

PreparedStatement prepStmt = conn.prepareStatement("SELECT userId FROM SiteUser WHERE name =?");
prepStmt.setString(1, username);
ResultSet rs = prepStmt.executeQuery();
rs.next();
return rs.getInt(1);

Before getting values from a ResultSet instance, you should always check the return value of rs.next(), as explained in the official JDBC tutorial.

Without that check, the rs.getInt(1) statement will raise SQLException when there is no matching user. Since you wrapped the block of code in try { ... } catch (SQLException e) { ... }, the exception will be handled, but this is not appropriate use of exceptions.

The purpose of the try-catch is not to handle the case of no matching user. Its purpose is to handle unexpected errors during the database operation. If you rely on this try-catch to handle the case of no matching user, that's effectively using exceptions for flow control. That's not the intended use, and it's also inefficient, as it's slower than using a conditional to check the value of rs.next().

I suggest to review the entire code, look for statements where a function returns a value but you don't use it. Some IDEs for example IntelliJ will give you a warning on statements like that.

---

Another form of not checking the return value is in the process of registering a user. Consider this snippet from the register method:

insertNewUser(username, pass);
createAccountForUser(username);

The insertNewUser doesn't return anything, but probably it should. Inserting a new user might not succeed, and it would be incorrect to create an account if the user could not be created.

Potentially unsafe register

The implementation looks prone to a race condition: in between the moment of checking if the username exists, and the moment of inserting the user, another process or program may have inserted a user with the same name. In fact, unless you wrap these two operations within a transaction, there's no way to safely guarantee that the insert operation will succeed.

I'm not sure if you have a unique index on the username column. If you do, then you could skip checking if the user exists, attempt to insert. If the insert is successful, the user did not exist, it was successfully inserted, and you can continue creating the account. If the insert fails, whether because the user already exists, or due to other database errors, you can abort further processing.

Connection management

Creating connections is an expensive operation. Avoid creating new connections unnecessarily as much as possible. It might be a good idea to manage the connection outside this class. Or if you really want to create it in this class, make sure that a chain of operations reuses the same connection, rather than recreating it in each step.

Ordering of operations

The register method first checks if the specified username exists, and then validates the username and the password against some built-in rules. It would be better to perform these operations in the opposite order. The validation logic is a cheap operation, database lookups are expensive.

Confusing member and local variables

Some methods reuse the conn field, others create it as a local variable. This is confusing.

Answer 2

Apart from the feedback by Janos.

1. The value of instance is never set in the method getInstance, which will return a new LoginHandler() every time getInstance() is called as instance is always null.

2. You should always close the Connection, PreparedStatement and ResultSet in finally block

3. Instead of e.printStackTrace(), use Logger and log the exception with proper message.

Answer 3

Security: You need to hash passwords before saving them to the database.

See https://en.wikipedia.org/wiki/Scrypt

Java implementation: https://github.com/amdelamar/jhash