Custom System.Web.Http.AuthorizeAttribute

Question

I've hacked together what feels like a mess for an authorize attribute to secure web api methods. The code appears to be functionally correct; passing the unit tests in place. The code has me worried because it will be executing with every call to any web api method and there is a lot going on, including string allocations and database access. I'm sure that a lot of my misgivings are due to premature optimization.

I am looking for a general review of the code; looking for obvious problems, possible performance bottlenecks etc.

The use of this attribute provides for three different scenarios. First, specifying the attribute with no users or roles means that the attribute should simply validate that the supplied user/password authenticate properly. Second, the attribute supports a comma separated list of usernames... so in addition to authenticating the user, it should also verify the user is in the permitted list. Finally, the attribute allows a comma separated list to denote allowed roles... this requires the user to authenticate and to be a member of one of the provided roles. The semantics are such with regard to user or role lists, passing either condition in additional to authenticating successfully should return a true result.

using System;
using System.Collections.Generic;
using System.Linq;
using System.Threading;
using System.Web;
using System.Web.Http;
using System.Web.Http.Controllers;
using System.Web.Security;

using WebMatrix.WebData;

using Appsomniacs.Web.Filters;

namespace Appsomniacs.Web.Handlers
{
    public class BasicAuthorizeAttribute : AuthorizeAttribute
    {
        protected override bool IsAuthorized(HttpActionContext actionContext)
        {
            var identity = Thread.CurrentPrincipal.Identity;

            if (identity == null &&
                HttpContext.Current != null)
            {
                identity = HttpContext.Current.User.Identity;
            }

            if (identity != null &&
                identity.IsAuthenticated)
            {
                var basicAuth = identity as BasicAuthenticationIdentity;

                if (basicAuth != null)
                {
                    if (WebSecurity.Login(basicAuth.Name, basicAuth.Password))
                    {
                        if (this.Users.Length > 0)
                        {
                            var allowedUsers = this.Users.ToUpperInvariant()
                                .Split(',');

                            if (allowedUsers.Contains(basicAuth.Name.ToUpperInvariant()))
                            {
                                return true;
                            }
                        }

                        if (this.Roles.Length > 0)
                        {
                            var allowedRoles = Roles.Split(',');

                            var userRoles = System.Web.Security.Roles.GetRolesForUser(basicAuth.Name);

                            var matches = userRoles.Intersect(allowedRoles).ToArray();

                            if (matches.Length > 0)
                            {
                                return true;
                            }
                        }
                        else
                        {
                            return true;
                        }
                    }
                }
            }

            return false;
        }
    }
}

Answer 1

How about something like this. It's pretty much what you have but all I have done is seperated a couple of the parts into methods

using System;
using System.Collections.Generic;
using System.Linq;
using System.Threading;
using System.Web;
using System.Web.Http;
using System.Web.Http.Controllers;
using System.Web.Security;
using WebMatrix.WebData;

using Appsomniacs.Web.Filters;

namespace Appsomniacs.Web.Handlers
{
    public class BasicAuthorizeAttribute : AuthorizeAttribute
    {
        protected override bool IsAuthorized(HttpActionContext actionContext)
        {
            var identity = Thread.CurrentPrincipal.Identity;

            if (identity == null && HttpContext.Current != null)
            {
                identity = HttpContext.Current.User.Identity;
            }

            if (identity != null && identity.IsAuthenticated)
            {
                var basicAuth = identity as BasicAuthenticationIdentity;

                if (basicAuth != null)
                {
                    if (WebSecurity.Login(basicAuth.Name, basicAuth.Password))
                    {
                        return IsAllowedUser(basicAuth.Name) || IsAutenticatedInRole(basicAuth.Name);
                    }
                }
            }

            return false;
        }

        private bool IsAllowedUser(string name) 
        {
            if(this.Users.Length == 0) return false;

            var allowedUsers = this.Users.ToUpperInvariant()
                .Split(',');

            return allowedUsers.Contains(basicAuth.Name.ToUpperInvariant());            
        }

        private bool IsAutenticatedInRole(string name) 
        {
            if (this.Roles.Length > 0)
            {
                var userRoles = System.Web.Security.Roles.GetRolesForUser(basicAuth.Name);

                var allowedRoles = Roles.Split(',');
                var matches = userRoles.Intersect(allowedRoles).ToArray();

                return matches.Length > 0;
            }

            return true;        
        }
    }   
}