2
\$\begingroup\$

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);
    }
\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review. Code needs to be working before we review it. In the question you say it compiles, does this code execute properly? \$\endgroup\$ – pacmaninbw Apr 9 '19 at 12:05
  • \$\begingroup\$ @pacmaninbw yes it working \$\endgroup\$ – ahmed_eltot93 Apr 9 '19 at 12:06
  • 2
    \$\begingroup\$ Aside from the code, which is addressed below, it seems as if you're storing passwords as plaintext in the database. This is a big no-no in security circles. Search for salting/hashing passwords for any number of good implementations of securing stored passwords. \$\endgroup\$ – Jesse C. Slicer Apr 9 '19 at 13:30
1
\$\begingroup\$

The code looks like it is coming from Windows Forms, I can't be certain, it could be WPF.

The code does not seem to be in a MVVM or MVC design pattern. Either of these design patterns would allow data abstraction from the user interface.

The code might be less complex and easier to maintain if there was some separation between the UI (view) and the data (Model). The code might also be less complex and easier to maintain if it was broken up into smaller functions that did only one thing (Single Responsibility Principle).

All SQL Queries can throw errors

The first SQL query in the file is not in a try/catch block, if the SQL query fails the program will fail.

The error message in the catch block "Invalid Login try checking Username Or Password !" doesn't represent what actually happened. If this catch block executes it means that the database that is being queried threw the exception and that there is an error either in the SQL code or the table doesn't exist in the database.

Use Using Blocks

A using statement provides additional error checking as well as cleaning up after the SQL call. There are some examples of using statements in this stackoverflow question.

You can wrap connections and SQL commands in using statements. For an SQL connection wrapped in a using statement the database the connection will be closed automatically. An SQL command will be properly disposed of it is wrapped in a using statement.

    private DataRow GetRawSeriesData(uint seriesId)
    {
        DataRow rawData = null;

        if (seriesId > 0)
        {
            try
            {
                using (MySqlConnection conn = new MySqlConnection(_dbConnectionString))
                {
                    string queryString = "SELECT * FROM series WHERE idSeries = '" + seriesId.ToString() + "';";
                    int ResultCount = 0;
                    DataTable Dt = new DataTable();
                    conn.Open();
                    using (MySqlCommand cmd = new MySqlCommand())
                    {
                        cmd.Connection = conn;
                        cmd.CommandType = CommandType.Text;
                        cmd.CommandText = queryString;

                        MySqlDataAdapter sda = new MySqlDataAdapter(cmd);
                        ResultCount = sda.Fill(Dt);
                        if (ResultCount > 0)
                        {
                            rawData = Dt.Rows[0];
                        }
                    }
                }
            }
            catch (Exception ex)
            {
                string errorMsg = "Database Error: " + ex.Message;
                MessageBox.Show(errorMsg);
            }
        }

        return rawData;
    }

User Input Error Checking

There doesn't seem to be any error checking on the username or password, in some cases this can allow SQL Injection attacks.

On more secure websites the password might be checked against a set of rules to make it more secure, such as a requirement for 2 lowercase characters, 2 uppercase characters, 2 integers, and special characters, as well as a minimum length.

There might also be a count of the number of times the button was pushed to prevent a bot attack.

\$\endgroup\$
  • 4
    \$\begingroup\$ Why do you use string concatenation in your query? that is primo territory for an injection attack. The OP uses a standard parameter injection mechanism that lets the SQL/DB code take care of escaping/sanitizing input precisely to avoid this. \$\endgroup\$ – D. Ben Knoble Apr 9 '19 at 13:52
  • \$\begingroup\$ @D.BenKnoble I agree with your analysis based on what you see of the code. If this was user input I wouldn't do it this way. seriesId is a uint value that has been assigned as a primary key by SQL. This code is buried in about 4 layers of code that performs error checking. \$\endgroup\$ – pacmaninbw Apr 9 '19 at 14:37
  • \$\begingroup\$ @D.BenKnoble updated the code example to show the full function. \$\endgroup\$ – pacmaninbw Apr 9 '19 at 14:41
  • 1
    \$\begingroup\$ thanks—didnt want to give the wrong impression to future readers. \$\endgroup\$ – D. Ben Knoble Apr 9 '19 at 14:45
  • \$\begingroup\$ @D.BenKnoble Thanks for the prompt to fix my code, you'll find a new and hopefully improved version here codereview.stackexchange.com/questions/217150/… \$\endgroup\$ – pacmaninbw Apr 9 '19 at 17:18

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.