Login program using local SQL database

Question

I'm trying to make a login program that will check what the user entered in with usernames and passwords that are saved in a database. for some reason, the "if(dr.HasRows)" part of the program is not working. When i try without the try catch, i get and error "possible mistaken empty statement". What have i done wrong?

SqlConnection Connection = new SqlConnection(@"Data Source=(LocalDB)\v11.0;AttachDbFilename=|DataDirectory|\Logins.mdf;Integrated Security=True");

try
{
    Connection.Open();
    MessageBox.Show("Connection Succesful");

    if (Connection != null && Connection.State == ConnectionState.Closed);

    SqlCommand cmd = new SqlCommand("SELECT Count(*) FROM Logins WHERE Username='" + txtUsername.Text + "' and Password='" + txtPassword.Text + "'", Connection);
    SqlDataReader dr = cmd.ExecuteReader();

    if (dr.HasRows)
    {
        MessageBox.Show("Login Success");
    }
    else
    {
        MessageBox.Show("Incorrect login");
    }
}
catch (Exception)
{
    MessageBox.Show("Connection Unsuccesful");
}

score 7 · Accepted Answer · edited May 23 '17 at 11:48

This line

 if (Connection != null && Connection.State == ConnectionState.Closed);

contains a semicolon that cause the warning, but you have just opened the connection, what is the purpose to add this check?

Then there are the Sql Injection and parsing problems caused by your string concatenation

I would change your code to

using(SqlConnection Connection = new SqlConnection(....))
{
    try
    {
        Connection.Open();
        SqlCommand cmd = new SqlCommand(@"SELECT Count(*) FROM Logins 
                                        WHERE Username=@uname and 
                                        Password=@pass", Connection);
        cmd.Parameters.AddWithValue("@uname", txtUsername.Text);
        cmd.Parameters.AddWithValue("@pass", txtPassword.Text);
        int result = (int)cmd.ExecuteScalar();
        if(result > 0)
            MessageBox.Show("Login Success");
        else
            MessageBox.Show("Incorrect login");
    }
    catch(Exception ex)
    {
        MessageBox.Show("Unexpected error:" + ex.Message);
    }
}

The parameterized approach is safer because it avoids the Sql Injection problem and move the job of properly quoting your values to the framework code. Also the command text is more readable.

I have also changed your code from ExecuteReader to ExecuteScalar because you need to retrieve only the first column of the first row returned by your query and don't need an SqlDataReader for that.

And the last thing that I need to say is: Catching exceptions just to say that something failed is not a good practice. At least, tell to your user what is the error. Use MessageBox.Show("Unexpected error: " + ex.Message)

EDIT OK sorry but I have another one. Storing passwords in clear text is really a big security problem. The right approach is to store an hash of the password and apply the hashing function to the parameter used to pass the password, in this way only the hash result is transmitted along the wire and, if someone manage to stole the database, it will be very difficult to revert back to the clear values

Add a double quote at the end of the query ;) – Abbas Jan 27 '14 at 22:55 — Abbas, Jan 27 '14 at 22:55

score 4 · Answer 2 · answered Jan 27 '14 at 22:51

The possible empty statement is:

if (Connection != null && Connection.State == ConnectionState.Closed);

that ; at the end makes the if statement pointless.

On another note you should be using Parameters to pass your arguments not pasting the strings directly into the query to protect against an SQL injection attack.

Having said all that, your if (dr.HasRows) statement should still be working correctly, assuming the connection is actually valid etc.

Login program using local SQL database

2 Answers2