Inte trådsäkert

7 svar · 770 visningar · startad av doggelito 26 maj 2008

doggelitoMedlem sedan juni 20003 076 inlägg

Frågan26 maj 2008 12:35#1

Har stött på patrull!
En inloggad användare har lyckats få en annan inloggad användares användarinfo.

Jag använder en statisk användarklass med bla en sessionvariabel och ett cacheobjekt. Felet bör ligga där i.

Är inte denna kod trådsäker? Hur kan den förändras/förbättras?

public static class UserInfo
{
    static string currentUserName = String.Empty;

    //Sessionvariabel som håller en användarens användarnamn
    public static string CurrentUserName
    {
        get
        {
            if (HttpContext.Current.Session["UserName"] != null)
            {
                currentUserName = HttpContext.Current.Session["UserName"].ToString();
            }
            return currentUserName;
        }
        set { HttpContext.Current.Session["UserName"] = value; }
    }

    static Entities.User currentUser;

    //Cache objekt som håller en användare
    public static Entities.User CurrentUser
    {
        get
        {
            if (CurrentUserName.Length > 0)
            {
                if (HttpContext.Current.Cache["UserWork" + CurrentUserName] != null)
                    currentUser = (Entities.User)HttpContext.Current.Cache["UserWork" + CurrentUserName];
                else
                {
                    //CurrentUser kan/får aldrig vara tom
                    HttpContext.Current.Response.Redirect("/?Msg=Din session har avslutats och du behöver logga in igen.");
                }
            }
            else
                HttpContext.Current.Response.Redirect("/?Msg=Din session har avslutats och du behöver logga in igen.");

            return currentUser;
        }
        set
        {
            currentUser = value;
            HttpContext.Current.Cache["UserWork" + CurrentUserName] = currentUser;
        }
    }
}

Erik JuhlinMedlem sedan maj 200010 687 inlägg

26 maj 2008 12:57#2

Jag kan inte se att currentUserName deklareras nånstans. Jag som är blind?

doggelitoMedlem sedan juni 20003 076 inlägg

26 maj 2008 13:12#3

Sorry, glömde visst den raden. Koden uppdaterad!

spangoMedlem sedan juni 20008 205 inlägg

26 maj 2008 13:34#4

Nej, det finns ju bara en currentUserName, så använder fler den samtidigt krockar det.Vad behöver du den till? Ta bort den och gör så här istället:

    public static string CurrentUserName
    {
        get { return (string)HttpContext.Current.Session["UserName"]; }
        set { HttpContext.Current.Session["UserName"] = value; }
    }

Om du verkligen skulle vilja ha den lagrad i ett fält skulle du få dekorera fältet med [ThreadStatic], men som sagt; du behöver den inte.

red. Sen får du ändra på ditt villkor också:

// byt 
if (CurrentUserName.Length > 0)
// mot
if (CurrentUserName != null)

red. potentiellt nullfel

doggelitoMedlem sedan juni 20003 076 inlägg

27 maj 2008 11:30#5

Tack! :bire
Jag återkommer om det mot förmodan fortfarande inte funkar!

spangoMedlem sedan juni 20008 205 inlägg

27 maj 2008 11:56#6

Förresten, du vill ta bort currentUser också.

    public static Entities.User CurrentUser
    {
        get
        {
            string userName = CurrentUserName;
            if (userName != null)
            {
                Entities.User user = HttpContext.Current.Cache["UserWork" + userName];
                if (user == null)
                {
                    //CurrentUser kan/får aldrig vara tom
                    HttpContext.Current.Response.Redirect("/?Msg=Din session har avslutats och du behöver logga in igen.");
                }
                return user;
            }
            else
            {
                HttpContext.Current.Response.Redirect("/?Msg=Din session har avslutats och du behöver logga in igen.");
                return null;
            }
        }
        set
        {
            HttpContext.Current.Cache["UserWork" + CurrentUserName] = currentUser;
        }
    }

doggelitoMedlem sedan juni 20003 076 inlägg

27 maj 2008 12:25#7

Mm, jag var inne på lite samma sak, jag återskapade dock CurrentUser om CurrentUserName inte är tom, så här:

public static Entities.User CurrentUser
{
    get
    {
        if (CurrentUserName != null)
        {
            if (HttpContext.Current.Cache["UserWork" + CurrentUserName] != null)
                return (Entities.User)HttpContext.Current.Cache["UserWork" + CurrentUserName];
            else
            {
                IGenericDao<Entities.User> UserDao = new GenericDao<Entities.User>();
                return UserDao.GetObject(Expression.Eq("UserName", CurrentUserName));
            }
        }
        else
            HttpContext.Current.Response.Redirect("/?Msg=Din session har avslutats och du behöver logga in igen.", true);

        return null;
    }
    set
    {
        HttpContext.Current.Cache["UserWork" + CurrentUserName] = value;
    }
}

Är min kod överflödig?
Kommer cacheobjektet CurrentUser någonsin att kunna vara tomt om CurrentUserName inte är tom?

För jag sätter dem samtidigt när användaren loggar in:

UserInfo.CurrentUserName = "arne";
UserInfo.CurrentUser = användare från databas;

Sedan rör jag dem inte mer.
Så då måste väl cachen finnas minst lika länge som sessionvariabeln? Eller kan cachen på något sätt avslutas när inte session gör det?

spangoMedlem sedan juni 20008 205 inlägg

27 maj 2008 13:19#8

Det är inte helt otroligt att cachen kan tömmas (har inte koll på exakt hur det går till i ASP.NET dock) så nej, din kod är nog inte helt onödig, nej.