Check Username and Password Separate
As Youssef13 pointed out, your query specifies both username and password, and then you check the result for both as well. If either the password or username is wrong, the query won't return any results and RD.Read
will be false
. I would recommend verifying the user name first, and then verifying the password. The user should be notified which one is wrong because that's critical to log-in usability.
Private Sub btnlogin_Click(sender As Object, e As EventArgs) Handles btnlogin.Click
sSql = "Select * from tblusers where username = '" & cmbusers.Text & "'"
execSQL(sSql, False)
'Assume we have one record because usernames are unique, can we assume this?
If RD.Read Then
If txtpass.Text = RD(2) Then
MsgBox("Welcome")
Else
MsgBox("Bad Password", MsgBoxStyle.Critical)
End If
Else
MsgBox("Bad Username", MsgBoxStyle.Critical)
End If
End Sub
SQL Injection, Disposal, and Misc Improvements
The code below shows how to use parameters to avoid sql injection, using
statements for proper object disposal, try/catch
for reader exceptions, and column name instead of index (less easy to inadvertently break).
Private Sub btnlogin_Click(sender As Object, e As EventArgs) Handles btnlogin.Click
Using MySQLConnection As New SqlConnection("<Connection String Here>")
MySQLConnection.Open()
Using cmd As New SqlCommand("Select * from tblusers where username = @Username", MySQLConnection)
cmd.Parameters.Add("Username", SqlDbType.Text).Value = cmbusers.Text
Try
Using RD = cmd.ExecuteReader()
If RD.Read Then
If RD("<NameOfPasswordColumnHere>") = txtpass.Text Then
MsgBox("Welcome")
Else
MsgBox("Bad Password", MsgBoxStyle.Critical)
End If
Else
MsgBox("Bad Username", MsgBoxStyle.Critical)
End If
End Using
Catch ex As InvalidCastException
'Handling Not implemented, throw exception
Throw
Catch ex As SqlException
'Handling Not implemented, throw exception
Throw
Catch ex As InvalidOperationException
'Handling Not implemented, throw exception
Throw
Catch ex As ObjectDisposedException
'Handling Not implemented, throw exception
Throw
Catch ex As IOException
'Handling Not implemented, throw exception
Throw
Catch ex As NullReferenceException
'Handling Not implemented, throw exception
Throw
End Try
End Using
End Using
End Sub
Password Security
Also, it seems like you're storing plain-text passwords, which should be avoided. Even a simple XOR cypher such as the one demonstrated below is an improvement. Some may argue that this gives a false sense of security, but it's better than nothing. You can follow-up with SQLhashing as well as many other improvements, but don't be intimidated to start off with some small security steps. For instance, basic precautions like TextBox1.UseSystemPasswordChar = True
and SecureString
.
Private Function XORString(Text As String, Key As String, Enc As System.Text.Encoding) As String
Dim TextBytes() As Byte
Dim KeyBytes() As Byte
Dim TextByteCount As Long
Dim KeyByteCount As Long
Dim KeyIdx As Long
Dim TextIdx As Long
TextBytes = Enc.GetBytes(Text)
KeyBytes = Enc.GetBytes(Key)
TextByteCount = UBound(TextBytes)
KeyByteCount = UBound(KeyBytes)
For TextIdx = 0 To TextByteCount
TextBytes(TextIdx) = TextBytes(TextIdx) Xor KeyBytes(KeyIdx)
If KeyIdx < KeyByteCount Then
KeyIdx += KeyIdx
Else
KeyIdx = 0
End If
Next TextIdx
XORString = Enc.GetString(TextBytes)
End Function
Use like this...
'Other code here ...
Using RD = cmd.ExecuteReader()
If RD.Read Then
'RD("NameOfPasswordColumn") must contain an XORed value set from XORString("<UsersSavedPassword>", "ThisIsBetterThanNothing", <Correct Encoding Here>)
If XORString(RD("NameOfPasswordColumn"), "ThisIsBetterThanNothing", System.Text.Encoding.Unicode) = txtpass.Text Then
MsgBox("Welcome")
Else
MsgBox("Bad Password", MsgBoxStyle.Critical)
End If
Else
MsgBox("Bad Username", MsgBoxStyle.Critical)
End If
End Using
'... rest of code