3
\$\begingroup\$

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?

\$\endgroup\$
  • \$\begingroup\$ You're mixing Authentication (which is what the forms ticket does) and Authorization (which is what the Role provider of ASP.NET does. I do not see anything in your model that would require a custom module. What's so special about your security model? \$\endgroup\$ – jessehouwing Oct 25 '14 at 16:38
  • \$\begingroup\$ I can see a handful of attack vectors. This squarely falls into the category of why you should not "roll your own". \$\endgroup\$ – James Snell Oct 25 '14 at 16:40
  • \$\begingroup\$ Yea I agree with James. Like cryptography, the same applies to authorization. Use what the framework can provide, it has had allot more eyes on it and bug fixes applied. \$\endgroup\$ – Piotr Kula Oct 25 '14 at 18:28
  • \$\begingroup\$ @ppumkin My problem with the membership is that it's too bloated and has expectations on how the database should store user information. \$\endgroup\$ – Alternatex Oct 25 '14 at 18:34
  • 1
    \$\begingroup\$ @ppumkin Actually SimpleMemebership is exactly what I've been looking into for this past hour. \$\endgroup\$ – Alternatex Oct 25 '14 at 19:04
2
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Well I guess I can't expect to post on CodeReview and not have someone tutor me over good coding practices :) +1 for you. Since the initial idea of my question was to find a silver bullet for my membership troubles I've decided to seriously look into the SimpleMembership provider. Thank you for you input though, noted. \$\endgroup\$ – Alternatex Oct 26 '14 at 2:33

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.