Check login credentials from a database

Question

I am storing usernames and passwords in a MySql database. I am using the following code to verify the credentials based on the data from my database for a login. The codes works fine. My question is whether this is bad practice and is there a better way to do this.

My approach is to connect to that database, extract and store those information in a List and compare them to the users input coming from a text box input.

//Extracting information from the database and storing it in a List
public void Login()
{
    MySqlCommand cmdReader;
    MySqlDataReader myReader;

    userQuery = "SELECT * FROM User";
    string name = "Name";
    string user = "UserName";
    string pw = "Password";

    string connString = "server=" + server + "; userid=" + userid + "; password=" + password + "; database=" + database;
    try
    {
        conn.ConnectionString = connString;
        conn.Open();
        cmdReader = new MySqlCommand(userQuery, conn);
        myReader = cmdReader.ExecuteReader();
        while (myReader.Read())
        {
            string tempUser, tempPassword;
            if (name != null)
            {
                tempUser = myReader.GetString(user);
                tempPassword = myReader.GetString(pw);
                users.Add(tempUser);
                passwords.Add(tempPassword);
            }
        }
        myReader.Close();

    }
    catch (Exception err)
    {
        MessageBox.Show("Not connected to server. \nTry again later.");
        Application.Current.Shutdown();
    }
}

//Comparing the List data with the users input from textbox1 and textbox2 to verify
private void btn1_Click(object sender, RoutedEventArgs e)
{
    for (int x = 0; x < users.Count; x++)
    {
        for (int y = 0; y < passwords.Count; y++)
        {
            if (users[x] == textbox1.Text && passwords[y] == textbox2.Text)
            {
                MessageBox.Show("Login successful");
            }
        }
    }
}

How can that code work fine? The users is not associated with passwords? You have them in two separate collections. — paparazzo, Feb 24 '15 at 20:27
I am having them in 2 separate collections but when checking, using the nested loop to check them same time same index. Tested and it works. — kar, Feb 24 '15 at 20:38
You have a nested loop on two lists and compare the index? OK it works but that is far from optimal. — paparazzo, Feb 24 '15 at 20:41
The answer from Habid for one. That would be faster than searching two lists. You are making rookie mistake of pre-mature optimization. Often the simplest code is the best code. I will post a Dictionary answer. — paparazzo, Feb 24 '15 at 20:49

score 5 · Accepted Answer · edited May 23 '17 at 10:24

5

Do not store password in plain text in database, store their hashes. See(Best way to store password in database)
Instead of querying and retrieving all the users, send specific user name and password to database and compare the returned result.

As a side note, do not use string concatenation to form SQL queries, instead use parameters, something like:

using (MySqlCommand cmd = new MySqlCommand("SELECT Count(*) FROM User = @userName AND password = @password"),conn)
{
    cmd.Parameters.AddwithValue("@username", username);
    cmd.Parameters.AddwithValue("@password", password);
    ....
    var count = cmd.ExecuteScalar(); //and check the returned value
}

Currently you are retrieving all records from User table and then comparing it with the values at client side, imagine if you have a large number of users, brining that much data to client end would not make sense.

edited May 23 '17 at 10:24

Community

1
1

answered Feb 24 '15 at 20:20

Habib

219,104
29
407
436

Pointer 2: To clarify, you mean instead of pulling information from the database and storing into a List, take the information gathered from the textbox 1 and 2 inputs and query the database using them? – kar Feb 24 '15 at 20:30
@keshk, yes, Imagine if you have millions of users, how would you do it then ? – Habib Feb 24 '15 at 20:30
I like your answer but if I had millions of uses then I would actually be more likely to store that information in a list to keep traffic off SQL. – paparazzo Feb 24 '15 at 20:33
@Habib Just wondering cos either way I have to run through the list to verify. The current method, I going to have to run it a million times on the application to check. The method you mentioned, I have to run it a million times on the database to check. Either way seems to take the same amt of time. Do advice if I am missing a point. Tnks. – kar Feb 24 '15 at 20:35
@keshk You are going to have to run this millions of times on the application? No way you have a million concurrent users. – paparazzo Feb 24 '15 at 20:38
1

@keshk, comparison at the database end, where you would have an index on username, would be much cheaper then bringing all the data to client and then comparing one by one. – Habib Feb 24 '15 at 20:39
@Blam I don't have a million users. Was just mentioning for the example. – kar Feb 24 '15 at 20:39
@Blam, yes, but It was just an example :) – Habib Feb 24 '15 at 20:40

score 1 · Answer 2 · answered Feb 24 '15 at 20:53

Looping on two list and comparing index is not a optimal.
If you want to pre fetch then a Dictionary.
Key lookup on Dictionary is O(1)

Dictionary<string,string> UserIDpw = new Dictionary<string,string>();
while (myReader.Read())
{
    UserIDpw.Add(myReader.GetString(0), myReader.GetString(1));
}

But the answer from Habib is a better approach. You don't have a performance issue the requires you to prefetch and prefetch comes with issues. For one you have the passwords on the web server where thy are easier to hack.

score 0 · Answer 3 · answered Feb 24 '15 at 20:25

0

I would use a stored procedure for this, and then send username and password as parameters. Depending on whether this is intranet app, or something that is out on the internet, I might do the hash thing as Habib suggests.

answered Feb 24 '15 at 20:25

David P

2,027
3
15
27

Check login credentials from a database

3 Answers3