Alexandra Alexandra - 6 months ago 29
SQL Question

ASP.NET registration page: Can't insert data in sql database

I'm working on an ASP.NET project which requires, a registration page. The following code verifies if the username or email address already exist, but if everything is ok it doesn't insert data into the database. What am I doing wrong?

using System;
using System.Collections.Generic;
using System.Linq;
using System.Web;
using System.Web.UI;
using System.Web.UI.WebControls;
using System.Data.SqlClient;
using System.Configuration;
using System.Drawing;

namespace BootstrapRegisterLogin
{
public partial class SignUp : System.Web.UI.Page
{
bool flag = false;

protected void Page_Load(object sender, EventArgs e)
{
}

private String EncryptPassword(string pass)
{
pass = tbPass.Text;
byte[] bytes = System.Text.Encoding.Unicode.GetBytes(pass);
string EncryptedPassword = Convert.ToBase64String(bytes);
return EncryptedPassword;
}

protected void btSignup_Click(object sender, EventArgs e)
{
if (tbName.Text != "" && tbEmail.Text != "" && tbUname.Text != "" && tbPass.Text != "" && tbCPass.Text != "" && tbHoneypot.Text == "")
{
if (tbPass.Text == tbCPass.Text)
{
String CS = ConfigurationManager.ConnectionStrings["MyDatabaseConectionString1"].ConnectionString;
using (SqlConnection con = new SqlConnection(CS))
{
con.Open();

SqlCommand verifica = new SqlCommand();
verifica.CommandText = "select * from [Users]";
verifica.Connection = con;

SqlDataReader rd = verifica.ExecuteReader();
while (rd.Read())
{
if (rd[1].ToString() == tbUname.Text || rd[3].ToString() == tbEmail.Text)
{
flag = true;
break;
}
}

if (flag == true)
{
if (rd[1].ToString() == tbUname.Text)
{
lblMsg.Text = "Username already exists!";
}

if (rd[3].ToString() == tbEmail.Text)
{
lblMsg.Text = "Email already exists in the database!";
}
}
else
{
SqlCommand cmd = new SqlCommand("insert into Users values('" + tbUname.Text + "','" + EncryptPassword(tbPass.Text) + "','" + tbEmail.Text + "','" + tbName.Text + "','" + tbHoneypot.Text + "','U')", con);
//cmd.ExecuteNonQuery();
lblMsg.Text = "Congratulations! Registration complete!!";
lblMsg.ForeColor = Color.Green;
Response.Redirect("~/Login.aspx");
}
}
}
else
{
lblMsg.Text = "The two passwords don't match!";
}
}
else if (tbHoneypot.Text != "")
{
lblMsg.Text = "Sorry! Bots are not allowed!!";
}
else
{
lblMsg.Text = "All fields are required!!";
}
}
}
}

Answer

Uncomment the line

//cmd.ExecuteNonQuery();

Without this line it can't work.

By the way, this:

SqlCommand verifica = new SqlCommand();
verifica.CommandText = "select * from [Users]";
verifica.Connection = con;

SqlDataReader rd = verifica.ExecuteReader();
while (rd.Read())
{
    if (rd[1].ToString() == tbUname.Text || rd[3].ToString() == tbEmail.Text)
    {
        flag = true;
        break;
    }
}

if (flag == true)
{
    if (rd[1].ToString() == tbUname.Text)
    {
        lblMsg.Text = "Username already exists!";
    }

    if (rd[3].ToString() == tbEmail.Text)
    {
        lblMsg.Text = "Email already exists in the database!";
    }
}

is a query that would be a lot more performant if it ran on the DBMS. You might want to change that to something like this:

SELECT [Username], [Email] 
FROM [Users] 
WHERE [Username] = @Username OR [Email] = @Email

And please use SqlCommand.Prepare() to prepare your queries instead of concating strings. The way you do this now, you are prone to SQL Injection

UPDATE

Based on your update, your real problem is that verifica and rd are still using con. You need to dispose those, for example like this:

using (SqlConnection con = new SqlConnection(CS))
{
    con.Open();

    bool userNameExists = false;
    bool emailExists = false;

    using(SqlCommand verifica = new SqlCommand())
    {       
        //Update this query with the hints above
        verifica.CommandText = "select * from [Users]";
        verifica.Connection = con;

        using(SqlDataReader rd = verifica.ExecuteReader())
        {           
            while (rd.Read())
            {
                if (rd[1].ToString() == tbUname.Text)
                { 
                    userNameExists = true;
                    break;
                }
                else if (rd[3].ToString() == tbEmail.Text)
                {
                    emailExists = true;
                    break;
                }
            }
        }
    }

    if (userNameExists)
    {
        lblMsg.Text = "Username already exists!";
    }
    else if (emailExists)
    {
        lblMsg.Text = "Email already exists in the database!";
    }
    else
    {
        //You should update this part too, someone could choose the username 
        // "x','','U'); DROP TABLE [Users]; --" and do a lot of damage this way
        SqlCommand cmd = new SqlCommand("insert into Users values('" + tbUname.Text + "','" + EncryptPassword(tbPass.Text) + "','" + tbEmail.Text + "','" + tbName.Text + "','" + tbHoneypot.Text + "','U')", con);
        cmd.ExecuteNonQuery();
        lblMsg.Text = "Congratulations! Registration complete!!";
        lblMsg.ForeColor = Color.Green;
        Response.Redirect("~/Login.aspx");
    }
}

WARNINGS

Image a user chooses the name x','','U'); DROP TABLE [Users]; --. What would happen in your query the way it is now?

Convert.ToBase64String is NOT Encryption. It is Encoding and can be reversed very easily. Use a Hash instead. In combination with the problem mentioned above, one could easily read the email address and passwords of all your users (and they certainly have other accounts with exactly that combination).

Final Note

If simply uncommenting a line solves a problem but causes another, then you didn't fix it - you just caused another one.

In this case your original problem was

There is already an open DataReader associated with this Command which must be closed first.

Food for thought: Your SqlDataReader has a Close method.