How can I improve my code?

Question

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 1

Connection string is not prone to sql injection.

you can check both username and password like this:

cmd = new SqlCommand("Select Username FROM [User] WHERE Username =@UsernameValue AND Password =@PasswordValue", loginConn);
loginConn.Open();
cmd.CommandType = CommandType.Text;
cmd.Parameters.Add("@UsernameValue", SqlDbType.VarChar).Value = Main_T_Username.Text;
cmd2.Parameters.Add("@PasswordValue", SqlDbType.VarChar).Value = Main_T_Password.Text;

Answer 2

For the Connection string part use SqlConnectionStringBuilder class wtih conjuction with your App.Config rather using string manipulation straight on the code.

Public string GetConnectionString()
{
    //Read the Application Settings from App.Config 
    SqlConnectionStringBuilder sqlConstrBuilder = new SqlConnectionStringBuilder();
    sqlConstrBuilder.DataSource = dataSource;
    sqlConstrBuilder.InitialCatalog = databaseName;
    sqlConstrBuilder.UserID = ConfigurationManager.AppSettings["UserId"].ToString();
    sqlConstrBuilder.Password = ConfigurationManager.AppSettings["Password"].ToString();

    return sqlConstrBuilder.ConnectionString;
}

Answer 3

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();
    }

Answer 4

Move connection string to config file and read it with ConfigurationManager. Sql Injections is not about connection string, it is about sql command. Writing sql command in the code is not good practice, move it to stored procedure(Actually using orm is better for most cases). If you want to keep sql command in code, replace single quote with double quote to avoid sql injection.

And last, read more before start coding.

Answer 5

You can do 3 things to improve and simplify your code :

1 - store your configuration string in your web.config or App.config

<connectionStrings>
    <add name="ModelDB" connectionString="server=localhost;user id=User;password=Password;port=3333;database=database" providerName="MySql.Data.MySqlClient" />
</connectionStrings>

2 - store the passwords as byte[] in your database and use a hash function (you can use MD5 from System.Security.Cryptography)

using System.Security.Cryptography;

byte[] password = MD5.Create().ComputeHash(Encoding.UTF8.GetBytes(Main_T_Password.Text));

3 - use linq and Ado.Net Entity Data Model to simplify your requests (look for turotials https://msdn.microsoft.com/en-us/data/ff728653.aspx)

You can then generate C# classes from your database and you will be able to use linq and to treat results from your requests as objects which will simplify the code.

For exemple the ModelDB object below will automatically connect to the mysql database using the connectionString from the Web.config or App.config file.

You can use differents providers depending on your database (use providerName="System.Data.SqlClient" if your are using SQLServer)

ModelDB sql = new ModelDB();
var users = from users in sql.Users where Username = Main_T_Username.Text select users;

if(users.Count() == 0)
    MessageBox.Show("WrongPass or Username!");
else if(users.Count() > 0 && users[0].Password.SequenceEqual(password))
    MessageBox.Show("username and Password = Correct");
else
    MessageBox.Show("Password = Wrong");