-3

I use this method in my database class which checks the password and yahooId ,if they were correct it allows the user to go to the next frame .I have added a lot of yahooId and password in my sql but this method just checks the last row and allows the last person to go to the next frame.would you please help me? thanks.

  public static boolean Test(String userName, String password) {
    boolean bool = false;
    Statement stmt = null;
    try {
        stmt = conn.createStatement();

        ResultSet rst = null;

        rst = stmt.executeQuery("SELECT yahooId , password FROM clienttable");


        while (rst.next()) {
            if (rst.getString(1).equals(userName) && rst.getString(2).equals(password)) {
                bool = true;
                break;
            } else {
                bool = false;
            }
        }
    } catch (SQLException ex) {
        Logger.getLogger(Manager.class.getName()).log(Level.SEVERE, null, ex);
    }
    System.out.println(bool);
    return bool;



}
Johanna
  • 27,036
  • 42
  • 89
  • 117
  • Is this server-side (JSP), or a client-side applet? – outis Jan 18 '10 at 13:44
  • 1
    Not related, but `bool` is a terrible name for the variable. Something descriptive of purpose, such as `authenticUser` or `validCredentials`, would be better. – outis Jan 18 '10 at 13:48
  • Also, I've a sneaking suspicion `password` is plaintext. Read http://chargen.matasano.com/chargen/2007/9/7/enough-with-the-rainbow-tables-what-you-need-to-know-about-s.html – outis Jan 18 '10 at 13:48
  • its not stated anywhere that the password is plain, maybe the method is passed an already hashed password and check it against hashed passwords in the table – mohdajami Jan 18 '10 at 13:54
  • 2
    Duplicate: http://stackoverflow.com/questions/2059279/jdbc-always-tests-the-last-row-of-mysql-table – BalusC Jan 18 '10 at 13:54
  • @medopal: which is why it's merely a suspicion. Given the level of the code, it's very likely that `password` isn't hashed. – outis Jan 18 '10 at 13:56
  • 2
    Depending on your JDBC driver you can also do things like `rs.getString("username")` which avoids having to keep the horrible magic numbers in the code. – pjp Jan 18 '10 at 14:02
  • 1
    One more thing - You're not closing the result set or statement. You should do this in a finally block. – Adamski Jan 18 '10 at 14:11
  • @Johanna: the reason I was asking about server-side vs. client side is that if this runs client-side but the DB runs on a central server, it opens two gaping security holes. – outis Jan 18 '10 at 14:24

2 Answers2

7

Don't select all the rows when you're interested in just one of them. Use a WHERE clause, which is its raison d'etre:

SELECT yahooId , password FROM clienttable WHERE yahooId=? AND password=?

If the result set is empty, authentication fails. If there's a single result, authentication succeeds. If there's more than one result, your dataset is munged (a UNIQUE index on yahooID is the proper way of preventing this).

The question marks, by the way, come from using prepared statements, if you haven't seen them before.

outis
  • 75,655
  • 22
  • 151
  • 221
  • +1 for emphasizing that more. That was already told her in one of her previous topics. It's also *much more* memory efficient. You really don't want to copy the entire DB in Java's memory if you're interested in only one row. – BalusC Jan 18 '10 at 13:55
  • 1
    The behaviour may be different between the SQL and Java if the username and password is supposed to be case sensitive. Need to ensure that the password and yahooId columns in the database have the correct collation. – pjp Jan 18 '10 at 14:00
  • +1 :D i was looking at the SQL in the question and saying something is missing its shouldn't be this way – mohdajami Jan 18 '10 at 14:01
0

The problem is you're reading in the whole clienttable just to find a match for a specific user.

Instead, be VERY specific with your query to only look for a specific record that matches:

SELECT yahooId, password FROM clienttable WHERE yahooid = "UserName"

If that query returns a record, then you know the user exists. You can then check the password matches what is supplied to your method (I'm hoping you're not storing the password in plain text...).

This approach, enables you if you wanted to in the future, keep track of unsuccessful logon attempts to a user's account. And is much much more performant/scalable than loop round every record to find one match.

AdaTheDev
  • 142,592
  • 28
  • 206
  • 200