C# Login Form, Login Button , security

Question

So, I am new to VS and C#, I am self-teaching to get a better understanding of the back-end of the product I work with. I have created a small database with some information and a Login form. Everything appears to compile correctly but is that the security way to do that or there is another way, any help is appreciated. Thanks.

 private void button2_Click(object sender, EventArgs e)
    {

        SqlCommand cmd = new SqlCommand("select * from tbladmin where username=@username and password=@password", sqlcon);
        cmd.Parameters.AddWithValue("@username", txtusername.Text);
        cmd.Parameters.AddWithValue("@password", txtpassword.Text);
        SqlDataAdapter sda = new SqlDataAdapter(cmd);
        DataTable dtbl = new DataTable();
        sda.Fill(dtbl);


        try
        {

            if (dtbl.Rows.Count > 0)
            {
                if (dtbl.Rows[0]["role"].ToString() == "Admin")
                {
                    SqlCommand cmd2 = new SqlCommand("select date from tbladmin where username=@username and password=@password", sqlcon);
                    cmd2.Parameters.AddWithValue("@username", txtusername.Text);
                    cmd2.Parameters.AddWithValue("@password", txtpassword.Text);
                    SqlDataAdapter sda2 = new SqlDataAdapter(cmd2);
                    DataTable dss = new DataTable();
                    sda2.Fill(dss);
                    String value2 = dss.Rows[0][0].ToString();
                    DateTime date = DateTime.Parse(dss.Rows[0][0].ToString());
                    Class1.Txtusername = txtusername.Text;
                    Debug.WriteLine("value is :   " + value2);

                    if (date.AddDays(90) < DateTime.Now)
                    {
                        Changpassad obj2 = new Changpassad();
                        this.Hide();
                        obj2.Show();
                    }
                    else
                    {

                        calladmin obj = new calladmin(dss.Rows[0][0].ToString());
                        this.Hide();
                        obj.Show();
                    }
                }
            }
            else if (dtbl.Rows.Count == 0)
            {
                SqlCommand cmd3 = new SqlCommand("select date from tblcallcenter where username=@username and password=@password", sqlcon);
                cmd3.Parameters.AddWithValue("@username", txtusername.Text);
                cmd3.Parameters.AddWithValue("@password", txtpassword.Text);
                SqlDataAdapter sda2 = new SqlDataAdapter(cmd3);
                DataTable dss = new DataTable();
                sda2.Fill(dss);
                String value2 = dss.Rows[0][0].ToString();
                DateTime date = DateTime.Parse(dss.Rows[0][0].ToString());

                Debug.WriteLine("value is :   " + value2);

                if (date.AddDays(90) < DateTime.Now)
                {
                    Changpass obj2 = new Changpass()/;
                    this.Hide();
                    obj2.Show();
                }
                else
                {
                    SqlCommand cmd4 = new SqlCommand("select user_id , username from tblcallcenter where username=@username and password=@password", sqlcon);
                    cmd4.Parameters.AddWithValue("@username", txtusername.Text);
                    cmd4.Parameters.AddWithValue("@password", txtpassword.Text);
                    SqlDataAdapter From_sda = new SqlDataAdapter(cmd4);
                    DataTable From_ds = new DataTable();
                    From_sda.Fill(From_ds);
                    String value1 = From_ds.Rows[0][1].ToString();
                    int id = int.Parse(From_ds.Rows[0][0].ToString());

                    Debug.WriteLine("value is :   " + value1);
                    Class1.Txtusername = txtusername.Text;
                    this.Hide();
                    SqlCommand cmd5 = new SqlCommand("select  [from], Take from tblcallcenter where username=@username and password=@password", sqlcon);
                    cmd5.Parameters.AddWithValue("@username", txtusername.Text);
                    cmd5.Parameters.AddWithValue("@password", txtpassword.Text);
                    SqlDataAdapter sda1 = new SqlDataAdapter(cmd5);
                    DataTable ds = new DataTable();
                    sda1.Fill(ds);
                    Callcenter1 obj = new Callcenter1(ds.Rows[0][0].ToString(), ds.Rows[0][1].ToString());
                    this.Hide();
                    obj.Show();
                }
            }

            else
            {
                MessageBox.Show("Invalid Login try checking Useraname Or Password !", "Error", MessageBoxButtons.OK, MessageBoxIcon.Error);
            }
        }
        catch (Exception)
        {
            MessageBox.Show("Invalid Login try checking Useraname Or Password !", "Error", MessageBoxButtons.OK, MessageBoxIcon.Error);
        }

Answer 1

1. Don't forget the Single responsibility principle

The responsibility of your form is only UI presentation. Should your form know how you store your data? Is your form responsible to know when user should change their password? No.

Each class should have one single responsibility. You could have an AuthenticationService class responsible to authenticate the user, and an UserRepository class to store/retrieve users in the database. For example this could be your button2 click handler:

private void button2_Click(object sender, EventArgs e)
{
    var authenticationService = new AuthenticationService(sqlcon);


    try
    {
        var user = authenticationService.AuthenticateUser(txtusername.Text, txtpassword.Text);

        if (user.Date.AddDays(90) < DateTime.UtcNow)
        {
            Changpassad obj2 = new Changpassad();
            this.Hide();
            obj2.Show();

            return;
        }

        if (user.IsAdmin)
        {
            calladmin obj = new calladmin(user);
            this.Hide();
            obj.Show();
        }
        else
        {
            Callcenter1 obj = new Callcenter1(user);
            this.Hide();
            obj.Show();
        }

    }
    catch (Exception)
    {
        MessageBox.Show("Invalid Login try checking Useraname Or Password !", "Error", MessageBoxButtons.OK, MessageBoxIcon.Error);
    }
}

You could take the principle even further and have your methods also have a single responsibility. For example, in the code above, I'd still extract the code checking if the passwords needs to be changed and the code responsible to open the correct form.

You already uses parameters in you SQL query. Good. You're resistant to SQL injection.

Once you're familiar, I also recommend looking into ORMs like Entity Framework to facilitate the process.

---

2. Don't store plaintext password in database

Seriously. Never do that.

Always try to avoid managing users password. If you absolutely must, there are built-in classes in .Net framework to handle that. Here is some links, but a quick search should yield many more.

• Quickstart: Add sign-in with Microsoft to an ASP.NET web app
• Introduction to Identity on ASP.NET Core

If for some reasons you don't use theses and decide to manage password yourself. You need to find a cryptographically secure hashing algorithm (not md5 like in the other answer). And you need to use an unique "salt" for each password you hash. I'll leave these link here since it's out of scope of your current question:

-Hash passwords in ASP.NET Core -Difference between Encoding, Encryption and Hashing

---

3. Give significant name to your variable.

What is button2? What is obj2? What does ds.Rows[0][0].ToString() represent? You should try to give representative name to your variable. How about btnLogin, callAdminForm, ...

While we're here I recommend you read C# General Naming Conventions. For example, naming convention for classes is PascalCase. It should be CallAdmin not calladmin.

---

4. Why do you have user/password stored in multiple tables?

Why are users stored in both tbladmin and tblcallcenter?

How about a single tblUsers and either a column or a related table specifying the user role? This will be easier to maintain in the long run. Why do something twice when you could do it once?