2

I am trying to create a login window with MDI. It is connected to a SQL Server table test. I have changed the datatypes and deleted and recreated the DB. I have 2 columns: usr and pwd of datatype nvarchar. 

Dim connetionString As String Dim cnn As SqlConnection connetionString = "Data Source=.;Initial Catalog=test;User ID=sa;Password=sasql" cnn = New SqlConnection(connetionString) Dim cmd As SqlCommand Dim myreader As SqlDataReader Dim query As String query = "Select usr From users WHERE (usr =" + TextBox1.Text + " and pwd = " + TextBox2.Text + ")" cmd = New SqlCommand(query, cnn) cnn.Open() myreader = cmd.ExecuteReader() If myreader.Read() Then Else MessageBox.Show("Incorrect username/password !", "LOGIN ERROR", MessageBoxButtons.OK, MessageBoxIcon.Exclamation) End If cnn.Close()

Thank you so much.

Improve this question
3
  • 2
    To fix your specific problem you need to add quotes in your SQL string around the values coming in from the db. But that's the least of your worries. Look up parameterization and SQL injection. Stop and do that NOW, before going any further. Commented Apr 17, 2016 at 8:55
  • Thx sasfrog. I'm a beginner and I do it for fun. I will go deep into it. Commented Apr 17, 2016 at 9:04
  • The best approach for when you're building a string is to inspect it once you've built it. You can see without quotes why you got that message from the db. Commented Apr 17, 2016 at 9:08

2 Answers 2

Reset to default
1

If you are using Tortuga.Chain, the code would look like this:

Dim ds As New SqlServerDataSource(connetionString) Dim user = ds.From("users", new With {.usr = TextBox1.Text, .pwd = TextBox2.Text}).ToString.Execute(); If user Is Not Nothing Then Else MessageBox.Show("Incorrect username/password !", "LOGIN ERROR", MessageBoxButtons.OK, MessageBoxIcon.Exclamation) End If

If you want to stick to raw ADO.NET code, you need to use a parameterized query.

Dim connetionString As String Dim cnn As SqlConnection connetionString = "Data Source=.;Initial Catalog=test;User ID=sa;Password=sasql" cnn = New SqlConnection(connetionString) Dim cmd As SqlCommand Dim myreader As SqlDataReader Dim query As String query = "Select usr From users WHERE (usr = @user and pwd = @pwd )" cmd = New SqlCommand(query, cnn) cmd.Parameters.AddWithValue( "@usr", TextBox1.Text) cmd.Parameters.AddWithValue( "@pwd", TextBox2.Text) cnn.Open() myreader = cmd.ExecuteReader() If myreader.Read() Then Else MessageBox.Show("Incorrect username/password !", "LOGIN ERROR", MessageBoxButtons.OK, MessageBoxIcon.Exclamation) End If cnn.Close()
Improve this answer
Sign up to request clarification or add additional context in comments.

Comments

0

Remove the spaces from the value of connectionString.

Your query compares textual data, which is invalid unless the user enters ' at the start and end of the textboxes. This would be syntactically correct:

"Select usr From users WHERE (usr ='" + TextBox1.Text + "' and pwd = '" + TextBox2.Text + "')"

but logically flawed, since the user might attempt to inject malicious SQL into your textboxes. Your application is extremely unsafe. You must protect it against SQL injection and you need to encrypt the password of the user as well.

Improve this answer

7 Comments

You know it's wrong, so why offer an example that doesn't use a parameterized query?
@JonathanAllen, who "offered" that example? Which part of "but logically flawed" confused you? I described that it would be sntactically correct, but logically flawed. So you disagree with my statement by agreeing to it. Congratulations :)
It isn't just "logically flawed", it flat out doesn't work. If my username is "O'Malley" I'll break your code even without attempting a SQL injection attack.
@JonathanAllen, there are cases when it will not fail. Which means that your statement "it flat out doesn't work" is false. You are pointing out that there is a case when it will fail, which is true. That's what we, developers call bugs. Which are logical flaws. This article will introduce you into the concept of "bug": en.wikipedia.org/wiki/Software_bug
@JonathanAllen, as about your statement that parameterized query must be used, I must tell you, that is flawed as well. If he wants to enter the parameters into the query but escapes them properly or guarantees otherwise that there will be no problems (mentioned in my answer as well). For instance, if foo is a number, then I see no problem with "select name from buyers where id=" + foo. I specified that injection attacks should be prevented and if they are prevented well, then the error mentioned by you will not occur. So, if you will, your statement that parameterized queries are needed is
|

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.