Trying to create a login page but it doesn't match username against password [Learning purpose]-CodePudding

If user puts any username or password from the database it logs in. It doesn't crossmatch between the index of them. I'm working on this as a beginner learner.

if(txtusername.Text != null && txtpassword.Text != string.Empty)
{
    sql = string.Format(@" select * from idpass where username ='{0}' ", txtusername.Text );
    DataTable dtForNameAndRole = LoadDataByQuery(sql); 
    if(dtForNameAndRole.Rows.Count > 0)
    {

        sql = string.Format(@" select * from idpass where password ='{0}' ", txtpassword.Text);
        DataTable dtForNameAndRole2 = LoadDataByQuery(sql);
        if (dtForNameAndRole2.Rows.Count > 0)
        {
            sql = string.Format(@" select * from idpass where username = '{0}' and password ='{0}' ", txtusername.Text, txtpassword.Text);
            DataTable dtForNameAndRole3 = LoadDataByQuery(sql);
         
            Response.Redirect("Dashboard.aspx");
        }
        else
        {
            lblMessage.Text = "No Such Password";
        }
    }//end of if
    else
    {
        lblMessage.Text = "no such user";
    }
}
else
{

    msgtr.Visible = true;
    lblMessage.Text = "Sorry! Invalid user name or password.";
    lblMessage.ForeColor = Color.Red;
    return;
}

CodePudding user response：

There are lot of issues in your existing code.

You should not store the plain password in the database.
There is no need of calling the query 3 times. Only one time is sufficient.
You should not query only on password because passwords can be same for many users.
Don’t use * in the select query. You should use the columns as aliases.
Put the breakpoint and debug your code to see what is the query you’re calling. This way you will get to know whether your query is correct or not. And you can run this query in the database as well to check whether it’s working as expected or not.

CodePudding user response：

as noted, this is for learning. However, VERY important to realize that you want to use the "built in system" for logons. The reasons are too long for this post. But by using the defined built in logon system?

Then you can use the pre-made templates for logons.

You can setup security using IIS (internet services), and NOT have to hand code out a WHOLE web based security system. This will save I can figure about a whole month of solid work. And if you use the built in logon system?

Then you can even drop into a page the asp.net logon control - and it will do all the magic and coding for you - in other words, there is a HUGE system working behind the scenes to manage security and logons for you. In a nutshell?

Don't try and roll your own security system and logon system - it is FAR too much work.

Ok, now that I outlined the above, we are of course still here to learn. So, lets fix up your code you have.

So, first up, I assume that you gone project->(your project properties), and under settings have setup a connection string (don't type those in manually - set that up in settings so you don't have to code out connection strings in code. You want ONE common connecting setting, since then you have one place to change this when you get around to publishing to a actual web site.

That setting is the ones you will find here:

Ok, now our code. Like everyone, it gets a bit tiring to wear out a few keyboards ever time we need to pull some data. And eventually, you want to consider a data framework like EF (entity framework) to class out and "abstract" your data base operations.

However, when starting out - I think it is GREAT idea to try test and play with some basic and simple database operations - such as your example sql queries.

So, first up, lets build that save world poverty's and the keyboards (so we don't have to re-code over and over some simple SQL queries)

So, lets drop in this code:

    DataTable LoadDataByQuery(SqlCommand cmdSQL)
    {
        DataTable rstData = new DataTable();
        using (SqlConnection conn = new SqlConnection(Properties.Settings.Default.TEST4))
        {
            cmdSQL.Connection = conn;
            conn.Open();
            rstData.Load(cmdSQL.ExecuteReader());
        }
        return rstData;
    }

So, now your code can become this:

As pointed out we CAN NOT and NEVER just check password alone, since many people might have the same password.

So we can check for user - or give message

Or THEN check for user password - and give bad password message.

Also, try to code without such a lot of nesting - its hard to debug and hard to follow. I tend to like reverse the conditions, and BAIL OUT of the code when things go wrong and then keep going if code is ok. That way you remove a boatload of nested if /else.

Try this code:

       if (txtusername.Text == string.Empty)
        {
            msgtr.Visible = true;
            lblMessage.Text = "Please enter a user name";
            return;
        }
        // check/get user
        SqlCommand sql = new SqlCommand();
        sql.CommandText = @" select * from idpass where username = @user";
        sql.Parameters.Add("@user", SqlDbType.NVarChar).Value = txtusername.Text;
        DataTable dtForNameAndRole = LoadDataByQuery(sql);

        if (dtForNameAndRole.Rows.Count == 0)
        {
            msgtr.Visible = true;
            lblMessage.Text = "no such user";
            return;
        }

        // if we get this far, then user name = ok
        // user ok, try for pass word
        sql.CommandText  = " AND password = @Pass";
        sql.Parameters.Add("@Pass", SqlDbType.NVarChar).Value = txtpassword.Text;
        DataTable dtForNameAndRole2 = LoadDataByQuery(sql);

        if (dtForNameAndRole2.Rows.Count == 0)
        {
            msgtr.Visible = true;
            lblMessage.Text = "Sorry! Invalid user password";
            lblMessage.ForeColor = Color.Red;
            return;
        }
        //get this far, user   password = ok
        Response.Redirect("Dashboard.aspx");
    }

Note also how our human minds work. For example, we read this post on SO? you read from top to bottom - and your brain is thus wired to work this way.

As a result, note how easy the above code is not only to read, but follow.

Hum, check for user name? no good, setup message - exit - we are done!!

If user ok, hey, lets keep working our way though this problem - lets try user password? no good, setup message - exit - we are done!

Hey, if we get this far? then we are done!!!

Give the above approach a try - I think you like this idea and style.

Note also, while we DID chew up about 2 extra lines of code to setup parameters?

Those parameters are sql injection safe. We were able to "additive" to the sql and the parameters (thus saving even more code).

We did not have to worry about single quotes - again string concatenations are error prone.

So my lesson and point? We used parameters NOT ONLY to prevent sql injection, but we in fact wound up with again more readable and maintainable code, and did not have to mess with string concatenate into the sql (well, ok, we did some concatenation - but NOT with a messy mix of single and double quotes). I point this out, since not only did we gain sql injection code, but the efforts to write the code was ALSO less. I never liked people just banging the drum to not use sql concatenate strings based on user input - but ALSO one should then at least provide a coding approach that reduces the pain and suffering for having suggested code to prevent sql injection.

I often use the above EVEN WHEN there is no chance of sql injection, since you can even on the fly add to the sql, and as above shows, on the fly even build up and add more parameters as we did above.

Good luck - all the best in the new year.