Login form C# SQL

Question

Basically this is my first login form. I am using SQL and C# WinForms. I made user roles such as "Admin" and others and the user is taken to a specific WinForms, according to his appointed role (appointed by me manually now).

I have also allowed users to create their new accounts, in which they pick their user name and password but the role still needs to be appointed by me. I'd like to know if what I've done is ok or if it needs improvement, and where.

namespace My_PROGRAM
{
    public partial class Login : Form
    {
        SqlConnection loginCon = new SqlConnection("Data Source=******;Initial Catalog=***;Persist Security Info=True;User ID=*****;Password=**********");

        public Login()
        {
            InitializeComponent();
        }


        private void btnLogin_Click(object sender, EventArgs e)
        {
            loginCon.Open();
            SqlDataAdapter loginAdapter = new SqlDataAdapter("SELECT [Role] FROM [dbo].[LOGIN_Tab] WHERE Name ='"+ userNameTextobx.Text +"' and Password='"+ userPasswordTextbox.Text +"'  ", loginCon);
            DataTable result = new DataTable();
            loginAdapter.Fill(result);
            try
            { 
                if (result.Rows.Count == 1)
                {
                    switch(result.Rows[0]["Role"] as string)
                    {
                        case "Admin":
                            {
                                this.Hide();
                                AdminMenu aMenu = new AdminMenu();
                                MessageBox.Show("Login was succesful. Welcome back " + userNameTextobx.Text + " !!");
                                aMenu.Show();
                                break;
                            }
                        case "Planner":
                            {
                                this.Hide();
                                PlannerMenu pMenu = new PlannerMenu();
                                MessageBox.Show("Login was succesful. Welcome back " + userNameTextobx.Text + " !!");
                                pMenu.Show();
                                break;
                            }
                        case "Operator":
                            this.Hide();
                            OperatorMenu oMenu = new OperatorMenu();
                            MessageBox.Show("Login was succesful. Welcome back " + userNameTextobx.Text + " !!");
                            oMenu.Show();
                            break;
                    }
                }
            }
            catch (SqlException ex)
            {
                MessageBox.Show(ex.Message);
                loginCon.Close();
            }
        }

        private void btnNewUser_Click(object sender, EventArgs e)
        {
            try
            { 
                if (userNameTextobx.Text == "" || userNameTextobx.Text.Length < 4)
                    {
                        MessageBox.Show("User name too short, or you didn't write anything !");
                        userNameTextobx.Clear();
                    }
                if (userPasswordTextbox.Text == "" || userPasswordTextbox.Text.Length < 5)
                {
                    MessageBox.Show("Password too short, or you didn't write anything !");
                    userPasswordTextbox.Clear();
                }
                else
                {
                    loginCon.Open();

                    bool exists = false;
                    SqlCommand checkCMD = new SqlCommand("SELECT COUNT(*) FROM [dbo].[LOGIN_Tab] WHERE [Name]= @Name", loginCon);
                    {
                        checkCMD.Parameters.AddWithValue("[Name]", userNameTextobx.Text);
                        exists = (int)checkCMD.ExecuteScalar() > 0;
                    }
                    if (exists)
                    {
                        MessageBox.Show("The name " + userNameTextobx.Text + ", already exists. Pick new name !");
                        userNameTextobx.Clear(); userPasswordTextbox.Clear();
                    }
                    else
                    {
                        SqlCommand insert = loginCon.CreateCommand();
                        insert.CommandType = CommandType.Text;
                        insert.CommandText = "INSERT INTO [dbo].[LOGIN_Tab] ([Name],[Password]) VALUES ('" + userNameTextobx.Text +
                            "','" + userPasswordTextbox.Text + "')";
                        insert.ExecuteNonQuery();
                        loginCon.Close();
                        MessageBox.Show("New user '" + userNameTextobx.Text + "' was created !");
                        userNameTextobx.Clear(); userPasswordTextbox.Clear();
                    }
                }
            }
            catch(SqlException ex)
            {
                MessageBox.Show(ex.Message);
                loginCon.Close();
            } 
        }

        private void btnQuit_Click(object sender, EventArgs e)
        {
            //Environment.Exit(0);
            Application.Exit();
        }

    }
}

Any suggestions are more than welcome and appreciated. Also, is there a good way for me after the user makes a new account, to appoint his role?

Answer 1

SQL Injection alarm

Don't use user passed text in your sql statements if you aren't using parameters.

From: https://stackoverflow.com/a/7505842/2655508

Using parameters helps prevent SQL Injection attacks when the database is used in conjunction with a program interface such as a desktop program or web site.

In your example, a user can directly run SQL code on your database by crafting statements in txtSalary.

For example, if they were to write 0 OR 1=1, the executed SQL would be

SELECT empSalary from employee where salary = 0 or 1=1

whereby all empSalaries would be returned.

Further, a user could perform far worse commands against your database, including deleting it If they wrote 0; Drop Table employee:

SELECT empSalary from employee where salary = 0; Drop Table employee

The table employee would then be deleted.

Answer 2

Cleartext passwords

Not a code-specific issue exactly but it would appear that you are storing the user's passwords in clear text. This means that anyone gaining access to the LOGIN_Tab would be handed a list of valid usernames and passwords for your application. This is especially bad when combined with SQL injection vulnerabilities!

Consider instead passing the passwords through a hashing function and storing this in the database instead. Then when the user logs in you hash their inputted password and compare it to the stored hash in the database. See the answer here: https://stackoverflow.com/questions/16999361/obtain-sha-256-string-of-a-string for an example of how to compute the hashed value.

Answer 3

In addition to Heslacher's answer I would also look at the using statement.

It is very important to keep a handle on resources like Sql Connections to ensure they are closed and cleaned up.

Link to the MSDN about SqlConnection with an example with the using statement.

A SqlConnection object represents a unique session to a SQL Server data source. With a client/server database system, it is equivalent to a network connection to the server. SqlConnection is used together with SqlDataAdapter and SqlCommand to increase performance when connecting to a Microsoft SQL Server database. For all third-party SQL Server products, and other OLE DB-supported data sources, use OleDbConnection.

When you create an instance of SqlConnection, all properties are set to their initial values. For a list of these values, see the SqlConnection constructor.

If the SqlConnection goes out of scope, it won't be closed. Therefore, you must explicitly close the connection by calling Close or Dispose. Close and Dispose are functionally equivalent. If the connection pooling value Pooling is set to true or yes, the underlying connection is returned back to the connection pool. On the other hand, if Pooling is set to false or no, the underlying connection to the server is actually closed.

using (SqlConnection connection = new SqlConnection(connectionString))
{
    connection.Open();
    // Do work here; connection closed on following line.
}

Answer 4

Security issues are covered, so just a bit on the code itself.
You have three nearly identical pieces of code, inside a switch-statement. That seems to be a code-smell. Apart from that, I'm missing a default case. What happens if a user logs in before you have assigned a role? He gets an empty screen, or an ugly error message?

How about creating a(n abstract) UserMenu superclass that your AdminMenu, PlannerMenu and OperatorMenu inherit from, and creating a defaultMenu as well (maybe containg a text that urges the user to contact you to assign a role)?

abstract class Usermenu
{
  static UserMenu CreateMenu(string role)
  {
    switch(role)
    {
      case "Admin":
        return new AdminMenu();
      case "Planner":
        return new PlannerMenu();
      case "Operator":
        return new OperatorMenu();
      default: 
        return new DefaultMenu();
    }
}

Now your whole original switch becomes:

var role = result.Rows[0]["Role"] as string;
this.Hide();
MessageBox.Show("Login was succesful. Welcome back " + userNameTextobx.Text + " !!");
UserMenu.CreateMenu(role).Show();

If you ever want to add a role, you just create the appropriate menu and add a one-line extra case to the switch in your UserMenu class.

Of course, it might be a good idea to change the magic strings to an enum.

Answer 5

using as already covered
SQL injection as already covered

For getting a single row Reader is more efficient than DataTable Since user Name should be unique you should never have more than 1 row. And I think the syntax is cleaner.

You don't need the userNameTextobx.Text == ""

if (userNameTextobx.Text == "" || userNameTextobx.Text.Length < 4)

You should do more sanitation of name and password
Should trim and not allow control characters

Answer 6

General issues

SqlConnection loginCon = new SqlConnection("Data Source=******;Initial Catalog=***;Persist Security Info=True;User ID=*****;Password=**********");

This shows two issues. One is that the connection string is hardcoded into the application. It's a good practice to store it externally, so that any DB changee doesn't require a recompilation. I would suggest for example reading it in the constructor or Load event (from a file, configuration setting or something else):

private readonly string connectionString;

public Login()
{
    InitializeComponent();

    // ReadConnectionString method will get the connection string from somewhere external
    this.connectionString = ReadConnectionString();
}

The second issue is that you're creating a single, global SqlConnection object, which is generally considered a bad practice. Instead, create a new connection for each query and dispose of it immediately after usage. Connection pooling will keep the actual network connection alive for performance gains.
That's why in the previous code block I've just recorded the connection string instead of creating the connection already.

Login method

When the login is successful you call this.Hide() to get rid of the login form. Instead, I suggest to use this.Close(), so that the form is removed from the system and its resources are freed. Hiding it just removes from the screen, but the window is otherwise alive under the hood.

When checking if the user exists you do this:

if (result.Rows.Count == 1)  
{  
    //Truncated form opening here  
}  

And nothing else. This has the problem that, if the login is incorrect (wrong user or password) nothing at all happens, the user is not notified of failure, just presses a button and nothing changes. At the very least, I think you should show a message to the user:

if (result.Rows.Count == 1)
{
    //Truncated form opening here
}
else
{
     MessageBox.Show("Incorrect user name or password");
     //opcionally clear the password field
}

New user method

Initially you validate the entered data:

if (userNameTextobx.Text == "" || userNameTextobx.Text.Length < 4)
{
    MessageBox.Show("User name too short, or you didn't write anything !");
    userNameTextobx.Clear();
}
if (userPasswordTextbox.Text == "" || userPasswordTextbox.Text.Length < 5)
{
    MessageBox.Show("Password too short, or you didn't write anything !");
    userPasswordTextbox.Clear();
}

This is fine, but when you detect an error you should exit the method with a return. Right now, it shows all the errors and can in some cases algo create the user anyway. As an alternative, you can run all the validations and show all the messages together, then exit the method.

To validate if the user exist, you run a query asking for it, then insert. I think this is a bad practice, because it does two roundtrips to the server and has a race condition (the user can be inserted after the SELECT but before the INSERT).
Instead, rely on the database to perform the validation. Add an UNIQUE constraint. Just insert the user directly, and catch the possible constraint violation to know if the user is already there. This solves both problems:

try
{
    SqlCommand insert = loginCon.CreateCommand();
    insert.CommandType = CommandType.Text;
    insert.CommandText = "INSERT INTO [dbo].[LOGIN_Tab] ([Name],[Password]) VALUES ('" + userNameTextobx.Text + "','" + userPasswordTextbox.Text + "')";
    insert.ExecuteNonQuery();
}
catch(SqlException ex)
{
    if(ex.Number == 2627 && ex.message.Contains("ConstraintName")
    {
        MessageBox.Show("The name " + userNameTextobx.Text + ", already exists. Pick new name !");
        userNameTextobx.Clear();
        userPasswordTextbox.Clear();
        return;
    }
}

In the same method, there are two other small issues:

checkCMD.Parameters.AddWithValue("[Name]", userNameTextobx.Text);

Here, the parameter name must be "@Name" instead of "[Name]", which is the column name actually. This probably will cause the query to fail.
Another problem is the usage of AddWithValue, which is a bad practice. Instead explicitly declare the data type according to the types in the database.

The last problem would be, what happens after a correct login? Here I have my doubts, normally I would just login the new user into the system, in the same way that after a correct login, but here we have the role. What role would be the default? If there is one, just assign it when creating the user and login according to it. If not, you can't use the user until a role is assigned, in which case returning to the login form as you're doing seems appropriate.