0

I have two tables in my SQL Server database.

Table #1: Login_User

Columns

Unam, Pswd

Table #2 - Add_User

Columns

fName, lName, mobile, Eid, Unam, Pswd, Yob

Both tables have data.

My query is whenever user wanted to login, login credentials will be taken from either table1 or table2. I have written a SQL query with inner join as shown here:

string s = " select * from Login_User inner join Add_User on Login_User.Unam = Add_User.Unam where Login_User.Unam='" + txtUser.Text + "' and Login_User.Pswd='" + txtPswd.Text + "'";

SqlConnection con = new SqlConnection(@"Data Source=.\SQLEXPRESS;AttachDbFilename=|DataDirectory|\Product.mdf;Integrated Security=True;User Instance=True");
       
SqlDataAdapter da = new SqlDataAdapter(s, con);
DataTable dt = new DataTable();
da.Fill(dt);

if (dt.Rows[0][0].ToString() == "1")
{
    this.Hide();

    Home hm = new Home();
    hm.ShowDialog();
}
else
{
    MessageBox.Show("Invalid User Name or Password!.");
}

I get an error

There is no row at position 0

Please help me

marc_s
  • 732,580
  • 175
  • 1,330
  • 1,459
  • 1
    The error indicates that the query returned no results and `dt` is empty. – JohnG Aug 27 '21 at 09:24
  • 1
    [SQL Injection alert](http://msdn.microsoft.com/en-us/library/ms161953%28v=sql.105%29.aspx) - you should **not** concatenate together your SQL statements - use **parametrized queries** instead to avoid SQL injection - check out [Little Bobby Tables](http://bobby-tables.com/) – marc_s Aug 27 '21 at 09:40
  • 3
    Your main issue: you need `union all` not `inner join`. You have a huge bunch of other issues with your code: SQL injection, you should parameterize. Dispose connection, command and adapter with `using`. Don't store plain-text passwords. [Don't `select *` just select what you need](https://sqlblog.org/2009/10/10/bad-habits-to-kick-using-select-omitting-the-column-list), in this case just the number `1` (then you can use `ExecuteScalar` to get a single result and avoid using an adapter and datatable). Don't assume the datatable actually has any rows ....... – Charlieface Aug 27 '21 at 09:47
  • 2
    ...... [Avoid `AttachDbFilename`](https://stackoverflow.com/questions/11178720/whats-the-issue-with-attachdbfilename). Don't store the same information in two places, have on source of truth for every fact. – Charlieface Aug 27 '21 at 09:47
  • Just one more suggestion: do not send the password with the sql query because it becomes visible for anybody tracing the database server. Leave only the login as search param and compare the password column value you retrieved (if found) with the one you have in your variable. This way the password wont show up anywhere from your app and you have the possibility to differentiate between user not found and wrong password cases too. – cly Aug 27 '21 at 09:57

1 Answers1

0

Strictly avoid creating plain SQL-queries by concatenating input-data with SQL-code. It is highly dangerous for SQL Injections!

You can use some ADO.NET features that allow you better readable code, cleaner memory handling and a safer work with databases.

Executing an SQL-query in ADO.NET could look like this:


void login (string txtUser, string txtPassword) {

// Always hash login-informations (at least the password):
// therefore you hash the input-data and compare it 
// to an also hashed value inside a database
// with this you never transfer 1:1 login-informations from your service to the db

...


  // use "using" - this clears the memory after the object runs out of scope
  using (SqlConnection connection = new SqlConnection(connectionString))
  {
      try
      {
          connection.Open();
  
          using (DbCommand dbCmd = connection.CreateCommand())
          {

              // don't use "*", only select what you need
              string query = @"SELECT t.Unam, t.Pswd FROM 
                               (
                                    SELECT Unam, Pswd FROM User_Login
                                    UNION 
                                    SELECT Unam, Pswd FROM Add_User
                               ) t
                               WHERE t.Unam = @name AND t.Pswd = @pwd";

              dbCmd.CommandText = query;
              // a StringBuilder is possible as well, but always use parameters!

              // hashed txtName and txtPassword!
              dbCmd.Parameters.Add(new SqlParameter("@name", txtName));
              dbCmd.Parameters.Add(new SqlParameter("@pwd", txtPassword));
          
              using (DbDataAdapter adapter = DbProviderFactories.GetFactory(connection).CreateDataAdapter())
              {
                  adapter.SelectCommand = dbCmd;

                  DataTable dt = new DataTable();
                  adapter.Fill(dt);

                  // here you can do your stuff
              }
          }
      }
      catch (Exception)
      {
          throw;
      }
      finally
      {
          connection.Close(); 
          // close the connection after using to minimize database traffic
      }
  }
}

You can read more about an apropriate usage of the database-functionality of ADO.NET in the docs.

Always keep in mind: espacially when working with login-informations and user-data it is important to guarantee a clean and reliable code to provide safety.

Me3nTaL
  • 139
  • 2
  • 11