0

i've been trying to make this piece of code work work for a while now but seems like it wont. My User inputs Username and Password and then the program compares it to the DB. However it displays Invalid Credentials even though when i output the password it displays the exact same i've input.Just a few points, i know some of you are going to point on things like Using a seperate class for Connection, Check against username for password instead of using Select *, use hashings and all. But right now at the moment i just need to get this code work. Only after that i will be able to work on other aspects to ENHANCE its efficiency. Thank you all in advance.:)

 private void jButton1ActionPerformed(java.awt.event.ActionEvent evt) {//GEN-FIRST:event_jButton1ActionPerformed
    // TODO add your handling code here:
    Username1 = "jTextField1.getText()";
    Password1 = "jPassword1Field2.geText()";
      try 
        {
        Class.forName("com.mysql.jdbc.Driver").newInstance(); 
        String url = "jdbc:mysql://localhost:3306/"; 
        String dbName = "VideoSystem" ;
        String userName = "root"; 
        String passWord = "**mypassword**";
        Connection conn = DriverManager.getConnection(url+dbName, userName, passWord); 
        Statement st = conn.createStatement();
        String sql = ("SELECT * FROM Identification");
        ResultSet rs = st.executeQuery(sql);
        while(rs.next()) 
            { 
            password = rs.getString("Password");
            if( Password1.equals(password)) 
            {
                      MenuSelection obj1 = new MenuSelection();
                      obj1.setVisible(true);
            }
            else
            {
             JOptionPane.showMessageDialog(null, "Invalid Credentials", "System Message", JOptionPane.INFORMATION_MESSAGE);

            }
            }
        conn.close();
        }
      catch (Exception e) 
        { 
      System.err.println("Got an exception! "); 
      System.err.println(e.getMessage()); 
        } 


}
Danny.
  • 363
  • 1
  • 4
  • 23
  • The type for password in my Database is VARCHAR. –  Apr 27 '15 at 08:11
  • 1
    have you tried in debugmode? I think you only check the first password from the db and if this doesn't match you show the errormessage – 0riginal Apr 27 '15 at 08:22
  • Try to debug the code upto the point where you assign the value `password = rs.getString("Password");` and compare the debug value with your `Password1` value. That will help you determine whether you're getting the correct value returned. – Patrick W Apr 27 '15 at 08:32

1 Answers1

3

Those two lines:

Username1 = "jTextField1.getText()";
Password1 = "jPassword1Field2.geText()";

They are string literals, so unless your password is literally jPassword1Field2.geText(), there's not gonna be a match. I think you meant:

Username1 = jTextField1.getText();
Password1 = jPassword1Field2.getText();

Note however, that JPasswordField.getText() is deprecated since Java 2. You should use getPassword() instead:

Password1 = new String(jPassword1Field2.getPassword());

The documentation justifies the deprecation of getText() with "security reasons" that I can't claim to understand, but you still shouldn't use deprecated parts of the API.


And I know you some of the below already and none of that is part of your question, but I really suggest that you

  1. Check against the username. It should be obvious why.
  2. Hash passwords. Always.
  3. Only fetch what you need, mostly for performance reasons. Usually the best way is to use a WHERE clause in SQL (that brings the need to sanitize your input, but you should do that anyway) and building a list of columns instead of fetching *.
  4. Use backticks for database/table/column names in SQL to avoid things like accidentally using a keyword as table/column name or so.

Applying the above, I suggest the following query (assuming the name field is called Username):

PreparedStatement st = conn.prepareStatement("SELECT `Password` FROM `Identification` WHERE `Username` = ?");
st.setString(1, Username1);
ResultSet rs = st.executeQuery();
Siguza
  • 21,155
  • 6
  • 52
  • 89
  • The "security reasons" for using `getPassword()` are hinted at in its javadoc comment: _For stronger security, it is recommended that the returned character array be cleared after use by setting each character to zero._ The best practice of clearing a password (or other sensitive information) after use is something you can't do with Java's immutable Strings as returned by `getText()`. You can find more detailed explanations under [this stackoverflow question](https://stackoverflow.com/questions/8881291/why-is-char-preferred-over-string-for-passwords). – fspinnenhirn Apr 27 '15 at 18:36
  • I read the JavaDoc note, and I get that with an array you have an actual reference to the data and are able to clear it, but I can't think of a case where malicious code could obtain a reference to the char array without being able to use it in time, or where it could not obtain a reference to the password field itself. But if it's only about minimizing a possible attack window, then that's just that. – Siguza Apr 27 '15 at 19:08
  • It's exactly about that: minimizing the risk of exposing sensitive information unnecessarily; the idea is that memory could be swapped out to disk or written out in a core dump, or the variable could accidentally be passed to a logger. – fspinnenhirn Apr 27 '15 at 19:49