Login page in asp.net. Issue with query?

Question

I would like to create a simple login page in asp.net. here is my code:

protected void Button1_Click(object sender, EventArgs e)
    {
        SqlConnection conn = new SqlConnection();
        conn.ConnectionString = "Data Source=TEST-PC\\SQLSERVER2012;Initial     Catalog=oncf;Integrated Security=True";
        conn.Open();

        string query = "SELECT COUNT(*) FROM Account WHERE acc_username= '" + TextBox1.Text + "' AND acc_password= '" + TextBox2.Text + "'";
        SqlCommand cmd = new SqlCommand(query, conn);
        SqlDataReader myreader = cmd.ExecuteReader();

        int count = 0;
        while(myreader.Read())
        {

            count = count + 1;
        }

        if(count==1)
        {

            Response.Redirect("page2.aspx");
        }
        else
        {
            Label1.Visible = true;
        }

        conn.Close();

    }

I set a counter in order to know if the credentials entered are present in the DB. If the value of the counter goes to 1, the login is successful. Otherwise, the label with a message error is displayed! However, whatever I enter as input in the username and login textboxes, it always redirect me to the other page ! For now, my concern is not the security aspects, I just want to test this simple code, I don't see any problem with the code, but still it doesnt work, it is driving me crazy...

My advice, even through the fact that this kind of issue is UNDEBUGGABLE knowing we don't know what you enter in your form and without information on your database, you should execute your program, and then execute your query to see if you have the same result. — Anwar
– Anwar, Commented Mar 30, 2015 at 12:28
You should be using parameterised queries, and you should not be storing unencrypted passwords — GarethD
– GarethD, Commented Mar 30, 2015 at 12:35

Community · Accepted Answer · 2017-05-23 10:24:15Z

The reason that you are always redirecting is that your reader always returns 1 row, whether there is a match or not. If there is a match in your database, then the query will return

(no column name)
---------------
1

If there is not a match then it will return:

(no column name)
---------------
0

Either way, myreader.Read() will return true, and you will increment count in this part:

while(myreader.Read())
{

    count = count + 1;
}

if(count==1)
{

    Response.Redirect("page2.aspx");
}

Rather than checking the if the query returns rows you can retrieve the value of the count using SqlCommand.ExecuteScalar(). In addition to this I would make three more changes:

1. Use parameterised queries

This is not just a security concern, parameterised queries are able to use cached plans, whereas if you concatenate the parameters into the query then a new plan is compliled for each new variable value. In addition, parameterised queries are more strongly typed, and you don't need to escape things like O'shea to ensure that your extra quote doesn't mess up the query.

2. Encrypt the passwords

This is directly to do with security so should really be overlooked as per your request to not comment on security, HOWEVER, this answer is not just for your benefit, and a half answer is likely to be read by someone in the future who may or may not be aware of the risks of storing plain text passwords. There is a simple encryption method in this answer.

3. Add using blocks to your code

A minor change, but when you have objects that implement IDisposable it is a good idea to use a using block to esnure they are disposed of properly.

So you might end up with:

string password = SomeStaticClass.Encrypt(TextBox2.Text);
string connectionString = "Data Source=TEST-PC\\SQLSERVER2012;Initial     Catalog=oncf;Integrated Security=True";
string query = "SELECT UserCount = COUNT(*) FROM Account WHERE acc_username= @UserName AND acc_password= @Password";

using (var connection = new SqlConnection(connectionString))
using (var command = new SqlCommand(query, connection))
{
    connection.Open();
    command.Parameters.Add("@UserName", SqlDbType.VarChar, 50).Value = TextBox1.Text;
    command.Parameters.Add("@Password", SqlDbType.VarChar, 50).Value = password;

    int count = Convert.ToInt32(command.ExecuteScalar());

    if(count==1)
    {
        Response.Redirect("page2.aspx");
    }
    else
    {
        Label1.Visible = true;
    }
}

Aheho · Accepted Answer · 2015-03-30 12:47:05Z

2

The problem you are experiencing is because the followinq query Always returns one row even if there isn't a match in the database:

SELECT COUNT(*) FROM Account WHERE acc_username=....

If there is no match, you get a row with one column, value 0. You are checking the number of rows returned when you should just be checking the return value.

Use this instead

    int count = Convert.ToInt32(cmd.ExecuteScalar());

    if(count==1)
    {

        Response.Redirect("page2.aspx");
    }
    else
    {
        Label1.Visible = true;
    }

I know you said you don't want advice on security but just to be sure:

Don't store passwords plain text in a database. Always hash them using a salt.
Don't use string concatenation when building sql. Use parameters.

edited Mar 30, 2015 at 12:47

answered Mar 30, 2015 at 12:36

Aheho

12.8k13 gold badges57 silver badges84 bronze badges

7 Comments

simon Over a year ago

I get this An exception of type 'System.InvalidOperationException' occurred in System.Data.dll but was not handled in user code

Aheho Over a year ago

Use the Convert.ToInt32 instead of the cast.

simon Over a year ago

I still get an InvalidOperationException. But you said, I have a problem in the query, why does it always return a row? How can I fix it?

simon Over a year ago

The code works now! I forgot to delete the datareader I had before. However, can you tell me why my old code always logged me? What is wrong with my query?

Aheho Over a year ago

When you do an aggregate query with no group by clause, it always returns one row. That's just the way sql works. You can take an empty data base table and do a select(*) from tbl and you'll a result. You'll get one row and one column, with a value of 0.

|

void · Accepted Answer · 2015-03-30 12:33:35Z

1

don't use ExecuteReader when you want to return a single value, use ExecuteScalar:

int count = int.Pares(cmd.ExecuteScalar().toString());

if(count >= 1)
{

    Response.Redirect("page2.aspx");
}
else
{
    Label1.Visible = true;
}

answered Mar 30, 2015 at 12:33

void

7,9503 gold badges29 silver badges47 bronze badges

Comments

Community · Accepted Answer · 2017-05-23 12:02:59Z

1

You should always use Paremeterized queries Using parameters in SQL statements

string username=TextBox1.Text;
string password=TextBox2.Text;
SqlConnection conn = new SqlConnection();
conn.ConnectionString = "Data Source=TEST-PC\\SQLSERVER2012;Initial     Catalog=oncf;Integrated Security=True";
conn.Open();
SqlCommand cmd = new SqlCommand("SELECT * FROM Account WHERE acc_username=@username and 
AND acc_password=@password", conn);

cmd.Parameters.AddWithValue("@username",username);
cmd.Parameters.AddWithValue("@password",password);

SqlDataAdapter da = new SqlDataAdapter(cmd);
DataTable dt = new DataTable();
da.Fill(dt);
       if (dt.Rows.Count > 0)
        {
          Response.Redirect("page2.aspx");
        }
        else
        {
         Label1.Visible = true;
        }

edited May 23, 2017 at 12:02

CommunityBot

11 silver badge

answered Mar 30, 2015 at 12:40

Nagaraj S

13.5k6 gold badges35 silver badges53 bronze badges

2 Comments

simon Over a year ago

I get an ArgumentException at the da.Fill(dt)

simon Over a year ago

It still gives me an excpetion at the da.Fill(dt) bro

Microsoft DN · Accepted Answer · 2015-03-30 12:34:03Z

0

Try adding if (myreader.HasRows) before while(myreader.Read())

answered Mar 30, 2015 at 12:34

Microsoft DN

10.1k10 gold badges54 silver badges74 bronze badges

Collectives™ on Stack Overflow

Login page in asp.net. Issue with query?

5 Answers 5

Comments

7 Comments

Comments

2 Comments

Comments

Your Answer

Linked

Hot Network Questions

Collectives™ on Stack Overflow

5 Answers 5

Comments

7 Comments

Comments

2 Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Linked

Related