SQL Server : delete user

Question

I'm trying to write a query to delete a user registration from my SQL Server database, but when I try to delete a user, I get this error:

System.InvalidOperationException: 'ExecuteReader: Connection property has not been initialized.'

My code:

Public Class DeleteForm
Private Sub btnDelete_Click(sender As Object, e As EventArgs) Handles btnDelete.Click
    Dim conn = New SqlConnection("Data Source=(localdb)\MSSQLLocalDB;Initial Catalog=dbProject;Integrated Security=True")

    Using cmd = New SqlCommand("SELECT * FROM tblLogin WHERE username = " & txtUsername.Text, conn)
        conn.Open()
        Dim reader As SqlClient.SqlDataReader = cmd.ExecuteReader

        If reader.Read = True Then
            If txtUserPass.Text = txtCheckPass.Text Then
                Dim deleteOk As Integer = MessageBox.Show("This cant be undone!" & vbCrLf & "Are you sure?", "Warning!", MessageBoxButtons.YesNo, MessageBoxIcon.Warning)
                If deleteOk = DialogResult.Yes Then
                    Dim queryDelete As String = "DELETE FROM tblLogin WHERE username = " & txtUsername.Text & " and password = " & txtPassword.Text
                    Dim cmdDelete As New SqlClient.SqlCommand(queryDelete, conn)

                    If conn.State = ConnectionState.Closed Then conn.Open()
                    reader.Close()
                    cmdDelete.ExecuteNonQuery()
                    MsgBox("Cancellazione eseguita correttamente!")
                    cmdDelete.Dispose()
                    conn.Close()
                ElseIf deleteOk = DialogResult.No Then

                End If
            Else
                MsgBox("The passwords arent matching!")
            End If
        Else
            MsgBox("User not found")
            conn.Close()
            txtUsername.Clear()
            txtUsername.Focus()
            txtUserPass.Clear()
            txtCheckPass.Clear()
        End If
    End Using
  End Sub
End Class

Just a suggestion. I would separate this in multiple method. One that look if the user exists, an other that just delete and one that calls them and handle error. — the_lotus, Dec 10 '18 at 15:15
Not using parameters is just awful. This is wide open to sql injection and my friend [Bobby Tables](http://bobby-tables.com/) loves this style of coding. And passwords should be salted and hashed, not stored in clear text. — Sean Lange, Dec 10 '18 at 15:25
Never ever store passwords as plain text either - salt them and hash them — Ňɏssa Pøngjǣrdenlarp, Dec 10 '18 at 17:18

trailmax · Accepted Answer · 2018-12-10T21:42:54.203

5

~~You need to open connection before you can create a command. i.e.~~

Dim conn = New SqlConnection("Data Source=(localdb)\MSSQLLocalDB;Initial Catalog=dbProject;Integrated Security=True")
conn.Open()
Using cmd = New SqlCommand(....

However your current code contains SQL Injection. You should not concatenate strings go get your SQL. You should use parameters. See this answer for better explanation about the application.

Also it is never a good practice to store passwords in plain text. Ever. You should store hash of password only and compare the hashes rather than plain-text. Read this answer for reference. And more background info on why you should hash

edited Dec 10 '18 at 21:42

answered Dec 10 '18 at 15:11

trailmax

34,305
22
140
234

1

"You need to open connection before you can create a command." I know you have way more experience than I but, I do not agree. An instantiated connection can be sent to the Command constructor. It does not need to be open. I never open a connection until the last possible moment; right before the Execute. – Mary Dec 10 '18 at 20:20
@Mary Huh, you are absolutely correct here - I did not realise that, I always opened connection before passing it to commands (though this is a very rare occasion I writ this low-level code) – trailmax Dec 10 '18 at 21:42

SQL Server : delete user

1 Answers1