Is there currently anything wrong with my custom authentication and authorization?

Question

I have my reasons not to use the ASP.NET membership. Though this causes me some issues. I read about a thousand articles on ASP.NET MVC custom authentication and I've found that almost all of them are just too simple to be of any use.

What I need is a good way to login a user, keep some info about him securely in a cookie (info such as id, username, email and role) and later use this info to assure that he's authorized to visit and edit certain pages. The way I have things right now is a bit convoluted and possibly insecure.

Here's the relevant code in my login method:

// Set session cookie
var cookieName = CookieHelper.MergeIntoString(user.Id, user.Username, user.IsAdmin, user.IsStaff);
// Code for the CookieHelper class is to follow below!
var timeout = model.RememberMe ? 525600 : 2880; // 365 days or 2 days
var ticket = new FormsAuthenticationTicket(cookieName, model.RememberMe, timeout);
var encrypted = FormsAuthentication.Encrypt(ticket);
var cookie = new HttpCookie(FormsAuthentication.FormsCookieName, encrypted)
{
    Expires = DateTime.Now.AddMinutes(timeout),
    HttpOnly = true
};

Response.SetCookie(cookie);

Code for the CookieHelper class:

public static class CookieHelper
{
    public static string MergeIntoString(int id, string username, bool isAdmin, bool isStaff)
    {
        return id + "/" + username + "/" + isAdmin + "/" + isStaff;
    }

    public static Dictionary<string, object> DecodeFromString(string cookieName)
    {
        var cookieDictionary = new Dictionary<string, object>();
        var cookieArray = cookieName.Split('/');

        cookieDictionary.Add("Id", cookieArray[0]);
        cookieDictionary.Add("Username", cookieArray[1]);
        cookieDictionary.Add("IsAdmin", cookieArray[2]);
        cookieDictionary.Add("IsStaff", cookieArray[3]);

        return cookieDictionary;
    }
}

Why am I using the CookieHelper class? Because I have no idea how to add more information that will be encrypted in my cookie. So I decided to put the info into the cookie name string and later split it to get my info.

And this is how I later reach into the cookie to get the user id:

var authCookie = HttpContext.Request.Cookies[FormsAuthentication.FormsCookieName].Value;
if (authCookie == null)
    return RedirectToAction("Index", "Home");

var cookieName = FormsAuthentication.Decrypt(authCookie).Name;
var cookie = CookieHelper.DecodeFromString(cookieName);
var userId = Convert.ToInt32(cookie["Id"]);

Now I'm sure there's thousands of issues with this. Can you tell me of a better way of doing what I'm trying to do?

Answer 1

I can't help with the security aspect of it because I don't know enough about it. So just same general code review things:

1. It seems odd that your CookieHelper class contains two methods which seem like they should be complementary operations however they are named inconsistently and their parameter/results don't really match up:

   • The MergeIntoString accepts a bunch of parameters which are all taken from some kind of user object.
   • The DecodeFromString splits up a string which was generated by MergeIntoString but returns a dictionary instead which you then seemingly have to parse back into a user.

   I would suggest to rename the first method to UserToCookie and pass the user as single parameter returning a string and rename the second one to CookieToUser which accepts a string and returns a user object.

   Alternatively keep the dictionary for the second method but then also pass in a dictionary in the first. You could then have helper methods which convert users to/from dictionaries.

2. You have some magic constants without a unit and therefore had to write a comment of what it means.

      var timeout = model.RememberMe ? 525600 : 2880; // 365 days or 2 days
   

   .NET has a TimeSpan type which is ideal to store this kind of information and it become obvious what unit it is and works nice with DateTime as well. So something along these lines:

   var timeout = model.RememberMe ? TimeSpan.FromDays(365) : TimeSpan.FromDays(2);
   
   ....
   
       Expires = DateTime.Now + timeout,
   

   Doesn't require a comment and it's immediately visible what the timeout could be.