Custom Authentication Attribute

Question

I was trying to find a way to redirect to different pages on authorization and authentication failure. I found this to be a possible solution.

However, I ended with a different solution by myself. It seems to work fine, however, I am not sure if it is the right thing to do.

I created a custom Authorize Attribute that redirects to an action if the request is Authenticated but not Authorized.

public class HandleAuthorizeAttribute : AuthorizeAttribute {
        public static string GlobalUnAuthorizationUrl { get; set; }

        private const string DefaultUnAuthorizationUrl = "~/Account/UnAuthorized";
        private static readonly char[] RolesSeparator = { ',' };

        public string UnAuthorizedUrl { get; set; }

        protected override bool AuthorizeCore(HttpContextBase httpContext) {
            if(httpContext.User.Identity.IsAuthenticated) {
                if(string.IsNullOrEmpty(Roles)) {
                    return true;
                } else {
                    var rolesOfUser = System.Web.Security.Roles.GetRolesForUser(httpContext.User.Identity.Name);
                    var authorizedRoles = Roles.Split(RolesSeparator);

                    var common = rolesOfUser.Intersect(authenticatedRoles);

                    if(common.Count() == 0) {
                        httpContext.Response.Redirect(
                            string.Format("{0}?{1}={2}", ActiveUnAuthorizedUrl, "requestUrl", httpContext.Request.Url.AbsoluteUri));
                        return false;
                    }

                    return true;
                }
            } else {
                return false;
            }
        }

        private string ActiveUnAuthorizedUrl {
            get {
                if(!string.IsNullOrEmpty(UnAuthorizedUrl)) {
                    return UnAuthorizedUrl;
                }
                if(!string.IsNullOrEmpty(GlobalUnAuthorizationUrl)) {
                    return GlobalUnAuthorizationUrl;
                }
                return DefaultUnAuthorizationUrl;
            }
        }
    }

Is it alright to redirect to a URL in the middle of a non-action method? Does it have any potential drawbacks?

Answer 1

There is something wrong with this Method that makes it a bit confusing.

    protected override bool AuthorizeCore(HttpContextBase httpContext) {
        if(httpContext.User.Identity.IsAuthenticated) {
            if(string.IsNullOrEmpty(Roles)) {
                return true;
            } else {
                var rolesOfUser = System.Web.Security.Roles.GetRolesForUser(httpContext.User.Identity.Name);
                var authorizedRoles = Roles.Split(RolesSeparator);

                var common = rolesOfUser.Intersect(authenticatedRoles);

                if(common.Count() == 0) {
                    httpContext.Response.Redirect(
                        string.Format("{0}?{1}={2}", ActiveUnAuthorizedUrl, "requestUrl", httpContext.Request.Url.AbsoluteUri));
                    return false;
                }

                return true;
            }
        } else {
            return false;
        }
    }

first thing that I noticed is that the authorizedRoles variable isn't being used, then I saw that you use a variable that isn't declared or initialized anywhere in your code, authenticatedRoles, I am assuming that these are supposed to be the same variable when I removed the common variable like this

    protected override bool AuthorizeCore(HttpContextBase httpContext) {
        if(httpContext.User.Identity.IsAuthenticated) {
            if(string.IsNullOrEmpty(Roles)) {
                return true;
            } else {
                var rolesOfUser = System.Web.Security.Roles.GetRolesForUser(httpContext.User.Identity.Name);
                var authorizedRoles = Roles.Split(RolesSeparator);

                if((rolesOfUser.Intersect(authorizedRoles)).Count == 0) {
                    httpContext.Response.Redirect(
                        string.Format("{0}?{1}={2}", ActiveUnAuthorizedUrl, "requestUrl", httpContext.Request.Url.AbsoluteUri));
                    return false;
                }
                return true;
            }
        } else {
            return false;
        }
    }

I removed some of the else statements that were making this code a little cluttered as well, I was tempted to remove some of these return statements and replace it with a single bool and then just return at the end of the method, but I think that would have made this code messier and harder to read, so this is what I came up with, and it should do the same thing as your code assuming that those two variables I mentioned earlier are the same variable.

protected override bool AuthorizeCore(HttpContextBase httpContext) {
    if(httpContext.User.Identity.IsAuthenticated) {
        if(string.IsNullOrEmpty(Roles)) {
            return true;
        } 
        var rolesOfUser = System.Web.Security.Roles.GetRolesForUser(httpContext.User.Identity.Name);
        var authorizedRoles = Roles.Split(RolesSeparator);

        if((rolesOfUser.Intersect(authorizedRoles)).Count == 0) {
            httpContext.Response.Redirect(
                string.Format("{0}?{1}={2}", ActiveUnAuthorizedUrl, "requestUrl", httpContext.Request.Url.AbsoluteUri));
            return false;
        }
        return true;
    } 
    return false;
}

Answer 2

By using some early returns, you could eliminate some nesting, and in my opinion, make the code more readable.

In addition, I would ditch the ActiveUnAuthorizedUrl property and make it a function that returns a usable URL. Since "unauthorized" is a single English word, the "A" should not be capitalized. Prefer "unauthorized" to "unauthorization", as the latter is not a word.

Is there any reason why those URLs would be set to an empty string instead of null? I would just use the null coalescing operator.

protected override bool AuthorizeCore(HttpContextBase httpContext)
{
    if (!httpContext.User.Identity.IsAuthenticated) return false;
    if (string.IsNullOrEmpty(Roles)) return true;

    var rolesOfUser = System.Web.Security.Roles.GetRolesForUser(httpContext.User.Identity.Name);
    var authorizedRoles = Roles.Split(RolesSeparator);

    var common = rolesOfUser.Intersect(authenticatedRoles);

    if (common.Count() == 0)
    {
        httpContext.Response.Redirect(UnauthorizedUrlForContext(httpContext));
        return false;
    }

    return true;
}

private string UnauthorizedUrlForContext(HttpContextBase httpContext)
{
    return string.Format("{0}?{1}={2}",
                         UnauthorizedUrl ?? GlobalUnauthorizedUrl ?? DefaultUnauthorizedUrl,
                         "requestUrl", httpContext.Request.Url.AbsoluteUri));
}