6
\$\begingroup\$

I have two tables in the database:

  1. Credentials (userid, password, usertype)

  2. Customer (customername, userid(foreign key))

I need to validate username,password and usertype and create a session variable for customername.

I have organized the code and it works fine, but how can I minimize the code and make it efficient?

// Function that enables LOGON for a registered User into site
public string LogOn(UserInformation objLogOnUserInformation)
{
    ConnectionManager conCm = new ConnectionManager();
    try
    {
        SqlConnection con = conCm.OpenConnection();
        SqlCommand cmd,cmdOne;
        SqlDataReader dr;
        string _str = "U";
        cmd = new SqlCommand("select UserID  from Credentials", con);
        dr = cmd.ExecuteReader();

        while (dr.Read())
        {
            //checking user name is present in database
            if (dr.GetValue(0).ToString() == objLogOnUserInformation.UserId)
            {
                dr.Close();
                //retriving Password for the existing user
                cmd = new SqlCommand("select Password  from Credentials where UserId='" + objLogOnUserInformation.UserId + "'", con);
                dr = cmd.ExecuteReader();

                while (dr.Read())
                {
                    //checking the password is matching with the database
                    if (dr.GetValue(0).ToString() == objLogOnUserInformation.Password)
                    {
                        dr.Close();
                        cmd = new SqlCommand("select UserType  from Credentials where userid='" + objLogOnUserInformation.UserId + "'", con);
                        dr = cmd.ExecuteReader();

                        while (dr.Read())
                        {
                            //checking user type 
                            if (dr.GetValue(0).ToString() == _str)
                            {
                                dr.Close();
                                cmdOne = new SqlCommand("select customername from customer where userid='" + objLogOnUserInformation.UserId + "'", con);
                                dr = cmdOne.ExecuteReader();
                                dr.Read();
                                objLogOnUserInformation.SessionUserName = dr.GetValue(0).ToString();
                                dr.Close();
                                //if usertype is customer, U is returned
                                return _str;
                            }
                            else
                            {
                                dr.Close();
                                cmdOne = new SqlCommand("select customername from customer where userid='" + objLogOnUserInformation.UserId + "'", con);
                                dr = cmdOne.ExecuteReader();
                                dr.Read();
                                objLogOnUserInformation.SessionUserName = dr.GetValue(0).ToString();
                                dr.Close();
                                //If usertype is Admin, A is returned
                                return "A";

                            }
                        }
                    }

                    else
                    {
                        return " Either User Name or Password is not Valid";
                    }
                }
            }
        } 
\$\endgroup\$
  • 5
    \$\begingroup\$ Are you save your password as a plain text? Use parameterized queries. Use using statement to dispose your database connections. \$\endgroup\$ – Soner Gönül Jul 4 '14 at 7:43
  • \$\begingroup\$ @SonerGönül that should not be a comment, it should be an answer. \$\endgroup\$ – ANeves Jul 4 '14 at 16:15
13
\$\begingroup\$

Here are some observations:

  • you first iterate all rows in Credentials and then select the Password yet again - why not just select the pasword?
  • seems like your password is plain-text - BAD idea - hash your password together with a salt please
  • don't concat strings to get SELECT queries - use parameters instead
  • you are using C#/ASP.NET so why don't you use the provided login mechanisms?
  • you return a string "A" if the login succeeds and the error message if not - this is IMO bad design - return a simple struct/class with more information instead
\$\endgroup\$
  • 4
    \$\begingroup\$ +1 for the 'Why don't you use the provided login mechanisms?' Getting security right is a non-trivial exercise. Using an industry standard mechanism is 99 times out of 100 better than a roll-your-own. There are a number of access points to customize the ASP.NET mechanisms if you have non-standard requirements. \$\endgroup\$ – AlanT Jul 4 '14 at 8:13
9
\$\begingroup\$
if (dr.GetValue(0).ToString() == _str)
{
   dr.Close();
   cmdOne = new SqlCommand("select customername from customer where userid='" + objLogOnUserInformation.UserId + "'", con);
   dr = cmdOne.ExecuteReader();
   dr.Read();
   objLogOnUserInformation.SessionUserName = dr.GetValue(0).ToString();
   dr.Close();
   //if usertype is customer, U is returned
   return _str;
}
else
{
   dr.Close();
   cmdOne = new SqlCommand("select customername from customer where userid='" + objLogOnUserInformation.UserId + "'", con);
   dr = cmdOne.ExecuteReader();
   dr.Read();
   objLogOnUserInformation.SessionUserName = dr.GetValue(0).ToString();
   dr.Close();
   //If usertype is Admin, A is returned
   return "A";
}

I'll just start with all the things I see in this code.

Naming:

dr, con, objLogOnUserInformation, cmdOne

All these names are bad for various reasons.

dr (and con) is waay too short. You can allow yourself to name your variables after what they do and not some cryptic unpronounciable names. --> use dataReader (and connection) instead

objLogOnUserInformation is bad because ad 1 it's not clear, and ad 2 it uses hungarian notation. Never use hungarian notation, instead:

BUUUURN --> use: loggingInUserInfo

cmdOne doesn't tell you what it is and it is numbered! If you have no better idea than numbering equal variables, you are probably doing something wrong. I'd use sqlCommand

User-type:

Your user type is a String. THIS IS DANGEROUS and also quite prone to bugs. C# has a nice "feature" called enums. It looks like your UserType is one:

public enum UserType 
{
    CUSTOMER, ADMIN, //maybe others
}

And now the coolest thing happens.. Your if-else is useless. you do exactly the same either way you return the UserType. why not remove the if-statement completely (as well as the outer data reader and merge it with your "select usertype" statement:

dataReader.close(); 
// if you use dataReader with a using statement early on, you can leave that out
sqlCommand = new SqlCommand("select customername, usertype from customer where userId='" + loggingInUserInfo.UserId + "'", connection);
using (dataReader = sqlCommand.ExecuteReader())
{
    dataReader.read();

    loggingInUserInformation.SessionUserName = dataReader.Item["customername"];
    return dataReader.Item["usertype"];
}
\$\endgroup\$
  • 1
    \$\begingroup\$ While I agree with 99% of this I think dr and conn are so commonly used for DataReaders and Connections it's hardly worth worrying about \$\endgroup\$ – Liath Jul 4 '14 at 14:03
8
\$\begingroup\$

To make the code efficient you should select only that data that you need.

Your code:

cmd = new SqlCommand("select UserID  from Credentials", con);

This selects

  1. ids of all users but you need only the one that will match to objLogOnUserInformation!
  2. userid but userid is already known!

Must be

cmd = new SqlCommand("select UserID  from Credentials WHERE UserID=@UserID", con);
cmd.Parameters.AddWithValue("@UserID", objLogOnUserInformation.UserId);

This will select only the one which has required id and will not require to perform a loop.

Your code

cmd = new SqlCommand("select Password  from Credentials where UserId='" + objLogOnUserInformation.UserId + "'", con);

Same thing as above. You should select only required user and not all users.

Combine the query which we already have with a new condition

cmd = new SqlCommand(@"select UserID  from Credentials WHERE UserID=@UserID
              AND Password=@Password", con);
cmd.Parameters.AddWithValue("@UserID", objLogOnUserInformation.UserId);
cmd.Parameters.AddWithValue("@Password", objLogOnUserInformation.Password);

Do the same change for all further conditions. At the end you should have only one SELECT-statement that should select data in one shot.

\$\endgroup\$
1
\$\begingroup\$

Well, you can start by removing obvious copy-paste:

dr.Close();
cmd = new SqlCommand("select Password  from Credentials where UserId='" + objLogOnUserInformation.UserId + "'", con);
dr = cmd.ExecuteReader();

and

dr.Close();
cmdOne = new SqlCommand("select customername from customer where userid='" + objLogOnUserInformation.UserId + "'", con);
dr = cmdOne.ExecuteReader();
dr.Read();
objLogOnUserInformation.SessionUserName = dr.GetValue(0).ToString();
dr.Close();
\$\endgroup\$
1
\$\begingroup\$

In addition to the current answers, I would suggest you to not reuse the dr object like that. Use one DataReader for each query that you need and scope it correctly with a using statement. You may not be able to nest the while statements that deeply if your SqlConnection does not allow MultipleActiveResultSets, but that may be a good thing. By reusing the dr object the way you did, it is hard to see which da data is available and how long the resource is being used.

\$\endgroup\$
0
\$\begingroup\$

colleagues have pointed out all the major mistakes. i want to stress the deadly mistake of storing the plain-text password in the DB. check this out as a starter. This as an advanced implementation.

in any case the way you build your code is prone to side channel attacks (especially timing attack). In the web, you may find solutions to password hash comparison timing attacks but none towards username timing attacks. In many contexts such as records/document management users are horrible in password decisions. They have words from their usernames in their passwords. 2FA authentication is not an option. I have seen examples like username = sarah.johnson@xxxx.gov password = sarah1 when pwd is limited to 6 digit alpha-numeric. You would get Sarah123 in case of 8 digit uppercase+lowercase+numeric constraint. Therefore, timing attack against username and then dictionary/birthday attack on password works just fine on such contexts. Hacking valid usernames (which are mostly real mail addresses) is often overlooked. In your case + you have password hashing if the userId matches, your code exits much earlier if userId is invalid. Remember password hashing algorithms should be slow enough to fight brute attacks. This opens the back door to the timing attack. The attacker checking the response time difference can easily pick the valid usernames from invalid ones. In fact, the attacker would get most if not all of your usernames in one attack episode.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.