0

I have a ComboBox for username and a TextBox for password. I'm trying to create the usual login form where in the user needs to input the correct username and password or else access is denied. MsgBox("Welcome") is working and MsgBox("Failed") is not.

Private Sub btnlogin_Click(sender As Object, e As EventArgs) Handles btnlogin.Click
    sSql = "Select * from tblusers where username = '" & cmbusers.Text & "' and password = '" & txtpass.Text & "'"
    execSQL(sSql, False)
    If RD.Read Then
        If cmbusers.Text = RD(1) And txtpass.Text = RD(2) Then
            MsgBox("Welcome")
        Else
            MsgBox("Failed", MsgBoxStyle.Critical)
        End If
    End If
End Sub
Blackwood
  • 4,504
  • 16
  • 32
  • 41
  • `AndAlso` instead of `And` ? – muffi Sep 14 '17 at 13:26
  • "not working" is not a helpful explanation. How does it not work? – A Friend Sep 14 '17 at 13:33
  • 1
    Never ever concat strings to make SQL queries. Use SQL Parameters. Also, never ever store passwords as plain text - hash them. Reusing global DB provider objects is also a very bad idea. Please read [ask] and take the [tour] – Ňɏssa Pøngjǣrdenlarp Sep 14 '17 at 14:40
  • I tried to input a wrong password to test the if else statement but when i click the button ,no output was shown but when i inputted a correct password,an output was shown – Bhergie Calsada Estabillo Sep 14 '17 at 15:53
  • I would avoid using index properties when referencing a field. Use the column name instead of RD(1) and RD(2), etc. And yeah, use parameters to avoid that SQL injection problem you have. – LarsTech Sep 14 '17 at 16:03

2 Answers2

0

Your query won't return anything if the credentials aren't correct, Your messageBox Failed is in the wrong position as it will appear only if data was read.

Private Sub btnlogin_Click(sender As Object, e As EventArgs) Handles btnlogin.Click
    sSql = "Select * from tblusers where username = '" & cmbusers.Text & "' and password = '" & txtpass.Text & "'"
    execSQL(sSql, False)
    If RD.Read Then
        If cmbusers.Text = RD(1) And txtpass.Text = RD(2) Then
            MsgBox("Welcome")
        End If
    Else
        MsgBox("Failed")
    End If
End Sub
Youssef13
  • 3,836
  • 3
  • 24
  • 41
0

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
u8it
  • 3,956
  • 1
  • 20
  • 33