4

I am developing a login form in C#. This form connects to the database to match the username and password and also to find any duplicate. What I was trying to do is to implement a loop to accept only three tries then it will close. The code is: 

namespace RoyalCollegeApp
{
    public partial class Login : Form
    {
        public Login()
        {
            InitializeComponent();
        }

        private void Login_Load(object sender, EventArgs e)
        {

        }

        private void btn_login_Click(object sender, EventArgs e)
        {
            if (string.IsNullOrEmpty(txt_user.Text))
            {
                MessageBox.Show("Please type your Username");
                txt_user.Focus();
                return;
            }

            if (string.IsNullOrEmpty(txt_pass.Text))
            {
                MessageBox.Show("Please type your Password");
                txt_pass.Focus();
                return;
            }

            try
            {
                string constring = @"Provider=Microsoft.ACE.OLEDB.12.0;
                Data Source=C:\...\Auth_Credentials.accdb;";
                OleDbConnection conDataBase = new OleDbConnection(constring);
                OleDbCommand cmdDataBase = new OleDbCommand("Select * from Auth_Credentials where
                    Username='"
                    + this.txt_user.Text
                    + "' and Password='"
                    + this.txt_pass.Text
                    + "';", conDataBase);
                OleDbDataReader myReader;

                conDataBase.Open();
                myReader = cmdDataBase.ExecuteReader();

                int count = 0;

                while (myReader.Read())
                {
                    count = count + 1;
                }
                if (count == 1)
                {
                    MessageBox.Show("Login Successful");
                    this.Hide();
                    RCM RCM = new RCM();
                    RCM.Show();
                    this.Hide();
                    RCM.FormClosing += RCM_Closing;
                }
                else if (count > 1)
                {
                    MessageBox.Show("Duplicate Username or Password");
                }
                else
                {
                    MessageBox.Show("Username or Password do not match");
                }
            }
            catch (Exception ex)
            {
                MessageBox.Show(ex.Message);
            }
        }

        private void RCM_Closing(object sender, FormClosingEventArgs e)
        {
            Application.Exit();
        }
    }
}

I have tried many solutions but I am still groping in the dark. Any suggestions will be appreciated, cheers.

Improve this question

3 Answers 3

Reset to default
3

There are a few helpfull answers, but I prefered to have one correct example. Your code has a few problems

  • It's easy to hack your database using SQL Injection
  • There are some issues with IDisposable, which can create memory leaks.
  • The code is very tight coupled to the frontend, which is almost always the case in winforms. Nevertheless I prefer to limit the calls to the UI Components.

The code I've is the following: (it has inline comments)

private int _failedLoginCounter = 0;

private void btnLogin_Click(object sender, EventArgs e)
{
    var username = txtUsername.Text;
    var password = txtPassword.Text;

    if (string.IsNullOrEmpty(username))
    {
        MessageBox.Show("Please type your Username");
        txtUsername.Focus();
        return;
    }

    if (string.IsNullOrEmpty(password))
    {
        MessageBox.Show("Please type your Password");
        txtPassword.Focus();
        return;
    }

    // Seperate the login check and make it lously coupled from the UI (= do not refer to the UI elements, instead pass the values to a method)
    CheckLogin(username, password);
}

private void CheckLogin(string username, string password)
{
    try
    {
        string constring = @"Provider=Microsoft.ACE.OLEDB.12.0; Data Source=C:\...\Auth_Credentials.accdb;";

        // You need to use a using statement since OleDbConnection implements IDisposable
        // more inf: http://msdn.microsoft.com/en-us/library/system.data.oledb.oledbconnection(v=vs.110).aspx
        using (OleDbConnection conDataBase = new OleDbConnection(constring))
        {
            // You need to use a using statement since OleDbCommand implements IDisposable
            // more info: http://msdn.microsoft.com/en-us/library/system.data.oledb.oledbcommand(v=vs.110).aspx
            using (OleDbCommand cmdDataBase = conDataBase.CreateCommand())
            {
                cmdDataBase.CommandText =
                    "SELECT * FROM Auth_Credentials WHERE Username=@username AND Password = @password";

                cmdDataBase.Parameters.AddRange(new OleDbParameter[]
                {
                    new OleDbParameter("@username", username),
                    new OleDbParameter("@password", password)
                });

                // Open database if not open
                if (conDataBase.State != ConnectionState.Open)
                    conDataBase.Open();

                var numberOrResults = 0;

                // You need to use a using statement since OleDbDataReader inherits DbDataReader which implements IDisposable
                // more info: http://msdn.microsoft.com/en-us/library/system.data.common.dbdatareader(v=vs.110).aspx
                using (OleDbDataReader myReader = cmdDataBase.ExecuteReader())
                {
                    while (myReader != null && myReader.Read())
                    {
                        numberOrResults++;
                    }
                }

                // If only one result was returned by the database => Succesful login
                if (numberOrResults == 1)
                {
                    MessageBox.Show("Login Successful");
                    this.Hide();
                }

                // If more than 1 result was returned by the database => Failed login
                // This is not a good idea, this situation should never occor.
                // Always make sure username + pass (or whatever you use for authentication) is unique.
                else if (numberOrResults > 1)
                {
                    MessageBox.Show("Duplicate Username or Password");
                    // increment the failed login counter
                    _failedLoginCounter++;
                }
                // No match was found in te database => Failed login
                else if (numberOrResults == 0)
                {
                    MessageBox.Show("Username or Password do not match");
                    // increment the failed login counter
                    _failedLoginCounter++;
                }
            }

        }

        // If the user has 3 failed login attempts on a row => close.
        if (_failedLoginCounter >= 3)
            this.Close();
    }
    catch (Exception ex)
    {
        MessageBox.Show(ex.Message);
    }
}

For your initial question I've finished the answer of Selman22, basicly I've used a private field which keeps track of the number of failed tries. Each time the user tries to login, we check if its the 3th time. If so we close the form.

This still isn't the best approach imho, but I didnt want to change your context ;-)

I've also removed the SQL Injection possibilities by adding Parameters to the query.

To work with IDisposable implementations, you have to Dispose the object correctly. This is done in the finally block of the try/catch statement (or by a USING statement, like I did).

I hope this is helpfull, if not freel free to comment.

Improve this answer
Sign up to request clarification or add additional context in comments.

3 Comments

Your solution works really well and thanks for your suggestions about the IDisposable implementations. The application I am developing it is for one my final assessments and I am just a beginner. Would like to tell me what the best approach is in your opinion? It is just for my personal knowledge. Cheers.
There isn't one correct way, there are only a few principles which you should always take in mind. The biggest remark I have on this little piece of code, is that it's not seperated enough. You should seperate your logic in classes so that one class does only have one responsibility, this also means the view(and it's co7ndebehind) is only for visualisation. You should have a seperate classes which handles the database actions (checking username pass, adding new user, ...). Each class should contain actions about a specific object in your application. Have a look at the Repository pattern.
This is an example of what I mean: pastebin.com/RXeU1Qm4 Basicly I moved all the database code to a new class calles "AuthenticationRepository". The rest of the code is still the same... There are still alot of things that could be better, if you realy want to get the best out of your code I advice you to do some reading on: Dependency Injection, Inversion Of Control and Inversion Of Control Containers, SOLID principles, Common Design Patterns, Interfaces, ... These are pretty basic but powerful things.
2

Define a counter to store user's attempts:

public partial class Login : Form
{
    int counter = 0;
    ...
}

Then increment it each time user type invalid password or username:

else if (count > 1)
{
      MessageBox.Show("Duplicate Username or Password");
      counter++;
}
else
{
     MessageBox.Show("Username or Password do not match");
     counter++;
}

Then before you check whether or not the user exists, first check the counter and perform appropriate action:

private void btn_login_Click(object sender, EventArgs e)
{
       if(counter == 3) this.Close();
       ...
}
Improve this answer

3 Comments

I have tried your solution right now and it closes when I click the login button the fourth time. It should close automatically after the third attempt.
@JohnReed okey then check counter immediately after you increment it.
Marked as useful answer, but @Selman22 maybe edit the example to match his requirement (I know it's a simply cut/copy of one line :P). Makes it more usefull for the OP. Nevertheless it should be pretty clear with your anser.
1

Your current problem is quite simple, you need to maintain the number of login attempts outside of the btn_login_click method. Create a private field outside of the method and initialize it to 0, increment it in the click handler and stop when it's X. But I'm not sure that is sufficient--typically, you want something to not only limit the number of attempts sequentially, but to limit the number of attempts in total and/or for a specific username.

Also your current code is subject to SQL injection and is poorly structured (the validation and query code should not be in the click event handler).

NOTE: Your code is storing the password directly in the database, this is a BIG "No-NO". You always store just a hash of the password, never the password itself. Even if you need the set the password automatically and then send it to the user -- there's really no reason to store the password directly instead of storing a hash of the password.

Improve this answer

Comments

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.