7
\$\begingroup\$

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;
        }
    }
}
\$\endgroup\$
6
\$\begingroup\$

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;        
        }
    }   
}
\$\endgroup\$
  • \$\begingroup\$ That is a much cleaner implementation, I appreciate the review and improvement suggestions. Thank you! \$\endgroup\$ – user28056 Aug 4 '13 at 3:44
  • \$\begingroup\$ No problem. One suggestion though. If you leave your question open for a day or so (i.e. not accepted) you are more likely to get some other people offering answers. Just might give you some more ideas or perspectives. \$\endgroup\$ – dreza Aug 4 '13 at 4:14
  • \$\begingroup\$ Fair enough :) I wanted to make sure you got credit for taking the time. I think your refactor is pretty solid and I'm glad I decided to post here instead of rolling with the hacked up version... much clearer. We'll see if anyone has something else to contribute and if not I'll make sure to hit the answer button again. \$\endgroup\$ – user28056 Aug 4 '13 at 6:21
  • \$\begingroup\$ In the meantime you can almost upvote the answer instead :) \$\endgroup\$ – dreza Aug 4 '13 at 7:14

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