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;
}
3
Answers
There are lot of issues in your existing code.
*
in the select query. You should use the columns as aliases.As suggested by @Larnu, please use parameterized query to prevent the sql injection. You can refer this article for the concept.
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:
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:
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.