7
\$\begingroup\$

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?

\$\endgroup\$

migrated from stackoverflow.com Jul 19 '12 at 13:15

This question came from our site for professional and enthusiast programmers.

  • \$\begingroup\$ If you are confused with how the URL to redirect is formed, just ignore it. Its NOT important! \$\endgroup\$ – Mohayemin Jun 7 '12 at 12:30
5
\$\begingroup\$

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

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));
}
\$\endgroup\$
  • 1
    \$\begingroup\$ you could take this one step farther if you invert the other if statement, make it if (common.Count() != 0) { return true;} doesn't really add or take away though. \$\endgroup\$ – Malachi Aug 29 '14 at 18:17

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.