2
\$\begingroup\$

When users log in to our website, we store some information in a session to retrieve it across the site. We created a few wrappers to help with data retrieval/storage to lessen duplication of code.

I'm trying to redesign it as I don't like how the UserModel is written. I believe it's dangerous with the static properties, but so far it's been working.

However...

While we haven't noticed any issues, recently someone use something very similar to our code (slightly modified) for another project and had about 20+ people log in at the same time. Some users saw other user's information instead of their own. This brought a huge concern on the original code (below) and would like to get any feedback on how to make it "better" or avoid the same issue they ran into (if it even will happen).

I searched/read around Google/Stack Overflow about the possible issue being at our static method wrappers.

UserModel.cs

public class User {
    private const Boolean UseSessionAsDataStore = true;

    protected const String kUserId = "UserId";
    protected const String kUserName = "UserName";
    protected const String kUserEmail = "UserEmail";

    public static Int32 UserId {
        get { return GetInt32Value(kUserId); }
        set { SaveValue(kUserId, value.ToString()); }
    }

    public static String UserName {
        get { return GetStringValue(kUserName); }
        set { SaveValue(kUserName, value); }
    }

    public static String UserEmail {
        get { return GetStringValue(kUserEmail); }
        set { SaveValue(kUserEmail, value); }
    }

    protected static void SaveValue(String key, String value) {
        UseSessionAsDataStore ?
        SessionHelper.SaveSessionValue(key, value) :
        CookieHelper.SaveCookieValue(key, value);
    }

    protected static void GetStringValue(String key) {
        UseSessionAsDataStore ? SessionHelper.GetSessionStringValue(key) : CookieHelper.GetCookieStringValue(key);
    }

    protected static void GetInt32Value(String key) {
        UseSessionAsDataStore ? SessionHelper.GetSessionInt32Value(key) : CookieHelper.GetCookieInt32Value(key);
    }
}

UserLoginModel.cs

public class UserLoginModel {
    AssignUser (Int32 userId, String userName, String userEmail) {
        UserModel.UserId = userId;
        UserModel.UserName = userName;
        UserModel.UserEmail = userEmail;
    }
}

SessionHelper.cs

(won't provide CookieModel.cs as it's not the issue in question)

public class SessionHelper {
    public static void SaveSessionValue(String key, String value) {
        HttpContext.Current.Session.Add(key, value);
    }

    public static String GetSessionStringValue(String key) {
        if (HttpContext.Current.Session == null || HttpContext.Current.Session[key] == null) { return null; }
        return HttpContext.Current.Session[key] != null ? HttpContext.Current.Session[key].ToString() : String.Empty;
    }

    public static Int32 GetSessionInt32Value(String key) {
        if (HttpContext.Current.Session == null || HttpContext.Current.Session[key] == null) { return 0; }
        Int32 value;
        Int32.TryParse(HttpContext.Current.Session[key].ToString(), out value);
        return value;
    }
}

LoginPage

/* try to log in user and return user data */
...
if (loginSuccessful) {
    UserLoginModel.AssignUser(userId, userName, userEmail);
}
...

MainPage

<html>
    <body>
        Hello <%=UserModel.UserName %>!
    </body>
</html>
...

I have a feeling that this code has the potential to share session data between 2 users, although we have yet to realize it, even with about 5 simultaneous logins.

\$\endgroup\$
8
  • \$\begingroup\$ @Jamal, thanks for the edit, but now the Main Page section doesn't show the greater-than and less-than signs. \$\endgroup\$
    – RoLYroLLs
    Commented May 11, 2016 at 14:16
  • \$\begingroup\$ There aren't any in the post now. Even when I added some myself, I did see them show up. \$\endgroup\$
    – Jamal
    Commented May 11, 2016 at 22:52
  • \$\begingroup\$ @Jamal This is what OP refers to : imgur.com/7slL2fK \$\endgroup\$
    – Xiaoy312
    Commented May 12, 2016 at 1:21
  • \$\begingroup\$ @RoLYroLLs You may want to take a look to this : support.microsoft.com/en-us/kb/917072 \$\endgroup\$
    – Xiaoy312
    Commented May 12, 2016 at 1:59
  • \$\begingroup\$ @Xiaoy312, thanks we do use this in several pages on our site which have dynamic data. I'll check the other site they created to see if they have this or not, as I forgot this can be an issue. \$\endgroup\$
    – RoLYroLLs
    Commented May 12, 2016 at 2:10

2 Answers 2

Reset to default
1
\$\begingroup\$

  1. Do not use Hungarian notation for constants, keep it in PascalCase. If you like to differentiate the names from the properties you can name in form of {PropertyName}Key, like UserIdKey.

  2. Do prefer .net aliases over the full type name, like Int32 -> int, String -> string.

  3. Avoid repetition of word, like User.UserId and SessionHelper.GetSession.... Unless you are worried about mixed up with the foreign key in your DTO, but it is not the case here.

  4. UserModel (if it is User, that is fine)(you weren't really consistent with the code) should be renamed to CurrentUser :

    // which one makes more sense ?
UserModel.Name = userName;
CurrentUser.Name = userName;

  5. HttpContext.Current.Session shows up quite frequently in SessionHelper, you could use a property to shorten it :

    private static HttpSessionState CurrentSession
{
    get { return HttpContext.Current.Session; }
}

The SessionHelper class : 

public class SessionHelper
{
    private static HttpSessionState Session
    {
        get { return HttpContext.Current.Session; }
    }

    public void SaveValue(string key, string value)
    {
        Session.Add(key, value);
    }
    public static string GetStringValue(string key)
    {
        if (Session == null || Session[key] == null)
            return null;

        return HttpContext.Current.Session[key].ToString();
    }
    public static int GetIntValue(string key)
    {
        if (Session == null || Session[key] == null)
            return 0;

        int result;
        return int.TryParse(Session[key].ToString(), out result) ? result : 0;
    }
}

If you can get your hand on C# 6, consider this version :

public class SessionHelper_c6
{
    private static HttpSessionState Session => HttpContext.Current.Session;

    public void SaveValue(string key, string value)
    {
        Session.Add(key, value);
    }
    public static string GetStringValue(string key)
    {
        return Session?[key]?.ToString() ?? null;
    }
    public static int GetIntValue(string key)
    {
        var value = Session?[key]?.ToString() ?? null;
        int result;
        return value != null && int.TryParse(value, out result) ? result : 0;
    }
}
\$\endgroup\$
3
  • \$\begingroup\$ Thanks! I use ReSharper and the defaults for constant are that way and we never changed it and were ok with it. I agree with #3 as I do that with my personal projects and will be using this method when I change the User model where I work. for #2 I actually do not like using the alias, but I guess its just a preference everyone has. Lastly, besides the outputCache do you see any issues with the session and possible return of someone else's session because of the static GET methods? \$\endgroup\$
    – RoLYroLLs
    Commented May 13, 2016 at 14:01
  • \$\begingroup\$ Also, if you don't mind, would you be able to give some insight into this question stackoverflow.com/questions/37197086/… Thanks! \$\endgroup\$
    – RoLYroLLs
    Commented May 13, 2016 at 14:04
  • \$\begingroup\$ @RoLYroLLs I left an answer on your question. Almost missed your comment, had this answer not been upvoted today. Sorry. \$\endgroup\$
    – Xiaoy312
    Commented Jun 2, 2016 at 20:50
0
\$\begingroup\$

A public static property or a public static field is equivalent to a global variable.

What you can do is, create a security token at logon and store it in a session or cookie. Then, create a Sessions class with a static Dictionary and SaveUser and RestoreUser static methods. In SaveUser, you add the UserModel instance to the dictionary using the security token as the key; in RestoreUser, you retrieve the UserModel instance, again using the security token as the key.

Once you have done this, you can add additional features, such as timestamping the user model so that it expires after a set time.

\$\endgroup\$
1
  • \$\begingroup\$ Thanks! I will look ingot this. Could you provide some sample code to get me started? \$\endgroup\$
    – RoLYroLLs
    Commented May 12, 2016 at 15:24

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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