Jared Barrett Jared Barrett - 7 months ago 9
SQL Question

How can I improve my code?

Yesterday I asked a question about the table you guys helped a lot. Someone suggested that I don't directly store the

strConnectionString
so I changed what I had.

This is my code:

private void main_B_login_Click(object sender, RoutedEventArgs e)
{
//connect to the database
SqlConnection loginConn = null;
SqlCommand cmd = null;
SqlDataAdapter sda = null;
DataTable dt = new DataTable();

loginConn = new SqlConnection("server=localhost;" + "Trusted_Connection=yes;" + "database=Production; " + "connection timeout=30");
cmd = new SqlCommand("Select Username FROM [User] WHERE Username =@UsernameValue", loginConn);
loginConn.Open();
cmd.CommandType = CommandType.Text;
cmd.Parameters.Add("@UsernameValue", SqlDbType.VarChar).Value = Main_T_Username.Text;
sda = new SqlDataAdapter(cmd);
sda.Fill(dt);

if (dt.Rows.Count > 0)
{
//MessageBox.Show("username");

SqlConnection loginConn2 = null;
SqlCommand cmd2 = null;
SqlDataAdapter sda2 = null;
DataTable dt2 = new DataTable();

loginConn2 = new SqlConnection("server=localhost;" + "Trusted_Connection=yes;" + "database=Production; " + "connection timeout=30");
cmd2 = new SqlCommand("Select Password FROM [User] WHERE Password =@PasswordValue", loginConn2);
loginConn2.Open();
cmd2.CommandType = CommandType.Text;
cmd2.Parameters.Add("@PasswordValue", SqlDbType.VarChar).Value = Main_T_Password.Text;
sda2 = new SqlDataAdapter(cmd2);
sda2.Fill(dt2);

if (dt2.Rows.Count > 0)
{
MessageBox.Show("username and Password = Correct");
}
else
{
MessageBox.Show("Password = Wrong");
loginConn2.Close();
}

}
else
{
MessageBox.Show("WrongPass or Username!");
loginConn.Close();
}

}


At the moment it works perfectly. I am not sure about two things.


  1. Is the connection string now as it stands still "bad" in terms of SQL INJECTION?

  2. I have the code basically check first the username then password..? i have stored them both as text values because I don't know how to change it to hashing.



Could I simplify the check to do both username and password? but still give out and error when the username is wrong and when the password is wrong?

Answer

firstly is the connection string now as it stands still "bad" in terms of SQL INJECTION?

The connection string is not relevant.

You are using prepared sql statements , and this is what is good to prevent sql injection.

secondly I have the code basically check first the username then password..? i have stored them both as text values because I don't know how to change it to hashing.

Never save passwords as clear text in a DB

You must save an hash of the password to DB. When a user want to login, compare an hash of the password that the user entered with the hash of the passwords present on DB.

See here for some examples: How to hash a password

Could I simplify the check to do both username and password? but still give out and error when the username is wrong and when the password is wrong?

Yes , you must simplify it.

At the moment you are basically doing 2 hits on DB when you want just one.

You can simply do a select on DB by the username , then if you have 0 rows the username doesn't exists.

Otherwise the username exists and you can compare the password (the hash of the password) the user given to you with the password from the selected row (isn't necessary to do a 2nd select, you have already the row from the previous select).

Something like this (refinement is up to the reader):

   private void main_B_login_Click(object sender, RoutedEventArgs e)
{
    //connect to the database
    SqlConnection loginConn = null;
    SqlCommand cmd = null;
    SqlDataAdapter sda = null;
    DataTable dt = new DataTable();

    loginConn = new SqlConnection("server=localhost;" + "Trusted_Connection=yes;" + "database=Production; " + "connection timeout=30");
    cmd = new SqlCommand("Select Username, Password FROM [User] WHERE Username =@UsernameValue", loginConn);
    loginConn.Open();
    cmd.CommandType = CommandType.Text;
    cmd.Parameters.Add("@UsernameValue", SqlDbType.VarChar).Value = Main_T_Username.Text;
    sda = new SqlDataAdapter(cmd);
    sda.Fill(dt);

    if (dt.Rows.Count > 0)
        {
            if ((string) dt.Rows[0]['Password'] == Main_T_Password.Text)
            {
                MessageBox.Show("username and Password = Correct");
            }
            else
            {
                MessageBox.Show("Password = Wrong");
            }
         }
        else
        {
            MessageBox.Show("WrongPass or Username!");

        }
        loginConn.Close();
    }