Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Would you have any suggestions improvements for the below functions classes ?

Ok here how do i make a registered member login

    HttpCookie LoginInfo = new HttpCookie("LoginInfo");
    LoginInfo.Values["UserName"] = srUserName;
    LoginInfo.Values["Password"] = srPassword;
    LoginInfo.Values["selectedLanguage"] = srSelectedLanguage;
    Response.Cookies.Add(LoginInfo);

Here how do i check visitor is logged in or not

public static void controlOfLoginStatus()
{
    string srQuery = "";
    string srUserName = "";
    string srPassword = "";
    string srLang = "";

    if (HttpContext.Current.Session["UserId"] == null)
    {
        if (HttpContext.Current.Request.Cookies["LoginInfo"] != null)
        {
            try
            {
                srUserName = HttpContext.Current.Request.Cookies["LoginInfo"]["UserName"].ToString();
                srPassword = HttpContext.Current.Request.Cookies["LoginInfo"]["Password"].ToString();
                srLang = HttpContext.Current.Request.Cookies["LoginInfo"]["selectedLanguage"].ToString();
            }
            catch
            {

            }
        }
        string srUserIdTemp = csPublicFunctions.ReturnUserIdUsernamePassword(srUserName, srPassword);
        if (srUserIdTemp == "0")
        {
            HttpContext.Current.Session.Clear();
            HttpContext.Current.Session.Abandon();
            HttpContext.Current.Response.Redirect("Login");
        }
        else
        {
            csPublicFunctions.insertIntoOnlineUsers(srUserIdTemp, HttpContext.Current.Session.SessionID);
            HttpContext.Current.Session["UserId"] = srUserIdTemp;
            if (HttpContext.Current.Session["lang"] == null)
                HttpContext.Current.Session["lang"] = srLang;
        }
    }

    srQuery = "SELECT UserId " +
     " FROM BannedUsers" +
     " WHERE UserId = " + HttpContext.Current.Session["UserId"].ToString();
    using (DataTable dtTemp = DbConnection.db_Select_DataTable(srQuery))
    {
        if (dtTemp.Rows.Count > 0)
        {
            HttpContext.Current.Response.Redirect("exit.aspx");
        }
    }
}

Here how do i log-out

public static void exitLogout()
{
    string srQuery = "delete from OnlineUsers where UserId=" + HttpContext.Current.Session["UserId"].ToString();
    DbConnection.db_Update_Delete_Query(srQuery);

    try
    {
        HttpContext.Current.Session["UserId"] = "0";
        HttpContext.Current.Session.Clear();
        HttpContext.Current.Session.Abandon();
    }
    catch
    {

    }

    try
    {
        HttpCookie LoginInfo = new HttpCookie("LoginInfo");
        LoginInfo.Values["UserName"] = "21412zxcvzxc343245243vvc";
        LoginInfo.Values["Password"] = "21412zxcvzxc343245243vvc";
        LoginInfo.Values["selectedLanguage"] = "en";
        HttpContext.Current.Response.Cookies.Add(LoginInfo);
    }
    catch
    {            
    }
}

csPublicFunctions.ReturnUserIdUsernamePassword uses parametrized queries so no possible risk of SQL injection

share|improve this question
1  
Lose the "Hungarian Notation" variable names: msdn.microsoft.com/en-us/library/vstudio/ms229045.aspx –  Jesse C. Slicer Dec 31 '12 at 2:11
    
Does ReturnUserIdUsernamePassword use proper salted and expensive password hashing? –  CodesInChaos Feb 4 '13 at 15:10
add comment

2 Answers 2

In addition to losing the hungarian notation there are a few things worth mentioning:

  1. Don't use try-catch construction with empty 'catch' clause. It is almost always a bad idea to hide an exception if it was thrown.
  2. It would be better to check explicitly if the key is in the collection using appropriate methods. BTW exception handling is quite expensive speedwise and explicit checking will work faster.
  3. Try to refactor your controlOfLoginStatus() method into several distinct functions with more meaningful purpose.
  4. Refactor your database code into separate class.
  5. Use parameterized queries while working with database. It will save you from sql injections and it will make your queries run faster.
share|improve this answer
add comment

Don't store the users password in a cookie. That's an unnecessary security risk.

Create a new random session id to put there instead. That session id needs to be invalidated on the server on logout. Or if you want auto-login that crosses sessions, you still should create a token that doesn't depend on the user password.

Additionally you should set the Secure and HttpOnly flags.

  • Secure prevents sending of the cookie over unencrypted connections i.e. it enforces https over http. This flag is essential to prevent an attacker on your network from sniffing the cookie.
  • HttpOnly prevents javascript from reading the cookie value. This reduces the impact of XSS vulnerabilities a bit.
share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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