U4-2232 - UmbracoAuthorizeAttribute breaks Relfection methods

Created by Floris Robbemont 14 May 2013, 12:43:49 Updated by Floris Robbemont 31 Jul 2013, 07:21:54

When using Umbraco's UmbracoAuthorizeAttribute on a class, it breaks the GetAttributes method in the System.Reflection classes (http://umbraco.codeplex.com/SourceControl/latest#src/Umbraco.Web/Mvc/UmbracoAuthorizeAttribute.cs).

This is caused by the Exception that is thrown in the contructor of the attribute.

I think the constructor of an attribute should not be context aware. This should happen only in the AuthorizeCore method.

Comments

Floris Robbemont 14 May 2013, 12:51:03

Maybe when using the constructor for unit tests, do it like this:

public class UmbracoAuthorizeAttribute : AuthorizeAttribute { private bool failedAuthentication = true; private UmbracoContext umbracoContext; private ApplicationContext applicationContext;

    public UmbracoAuthorizeAttribute()
    {
    }

    internal UmbracoAuthorizeAttribute(UmbracoContext umbracoContext)
    {
        this.umbracoContext = umbracoContext;
        this.applicationContext = umbracoContext.Application;
    }

    protected override bool AuthorizeCore(HttpContextBase httpContext)
    {
        if (httpContext == null) throw new ArgumentNullException("httpContext");
        
        EnsureUmbracoContext();

        try
        {
            //we need to that the app is configured and that a user is logged in
            if (!applicationContext.IsConfigured)
                return false;
            var isLoggedIn = umbracoContext.Security.ValidateUserContextId(umbracoContext.Security.UmbracoUserContextId);
            return isLoggedIn;
        }
        catch (Exception)
        {
            return false;
        }
    }

    private void EnsureUmbracoContext()
    {
        if (umbracoContext == null)
        {
            umbracoContext = UmbracoContext.Current;
            applicationContext = umbracoContext.Application;
        }
    }
    /// <summary>
    /// Override to throw exception instead of returning a 401 result
    /// </summary>
    /// <param name="filterContext"></param>
    protected override void HandleUnauthorizedRequest(AuthorizationContext filterContext)
    {
        throw new HttpException((int)global::System.Net.HttpStatusCode.Unauthorized, "You must login to view this resource.");
    }
}


Floris Robbemont 15 May 2013, 18:44:19

Just looked through the codebase, and there are some more attributes that use the UmbracoContext.Current in the constructor. Maybe this needs to be a bigger refactor?


Shannon Deminick 20 Jun 2013, 06:22:21

I've changed this from critical to normal as this is quite an edge case scenario. In what circumstance are you using reflection on this attribute ?


Floris Robbemont 20 Jun 2013, 07:28:58

We're using AOP style for logging. Combining this with Windsor, we're adding a LoggingInterceptor to each registration in the container that the LogAttribute:

if(model.SearchRegistrationForAttribute()) EnableLoggingInterceptor(model); A bit deeper inside the code we're doing reflection on the type that is being registered in the container to get the attribute. We getting all attributes at once (we're also caching the reflection outcome, type.GetCustomAttributes is an expensive method). Once we call GetCustomAttributes without a type parameter for the attribute to get, .NET iterates through all the attributes and creates instances of the attributes found. The creation of the UmbracoAuthorizeAttribute fails on:

public UmbracoAuthorizeAttribute(UmbracoContext umbracoContext) { if (umbracoContext == null) throw new ArgumentNullException("umbracoContext"); _umbracoContext = umbracoContext; _applicationContext = _umbracoContext.Application; }

public UmbracoAuthorizeAttribute()
        : this(UmbracoContext.Current)
{

}

Once .NET creates an instance of this attribute, it looks at the UmbracoContext.Current (which is null at the time), and throws an exception.

This can be easily fixed by putting a try/catch block around my registration code, but I think it's more elegant to fix this in the core.


Shannon Deminick 20 Jun 2013, 07:33:09

Ok no worries, just wanted to see what you were up too :) I'll get around to it soon, the reason i like ctor dependencies is because, well, they are good. I realize attributes are a bit of a different beast though. Instead of making a bigger refactor for each attribute that has this ctor dependency, I'll just change the ctor param to Lazy, then it wont try to resolve until it needs to.


Floris Robbemont 20 Jun 2013, 07:41:40

Awesome! thnx


Shannon Deminick 31 Jul 2013, 06:39:21

Realized there's a larger flaw in this code :(

Total oversight on my part, think my brain turned off regarding how attributes are treated. An attribute is really only ever created once... so by passing in UmbracoContext.Current in the ctor, it will actually hang on to the original context it gets passed... ouch! that's no good.

Gonna commit the changes to these attributes asap.


Shannon Deminick 31 Jul 2013, 06:45:37

Complete in e2eeafcbfcb12934f825b24d7bf5aba0d6f7cfa3


Floris Robbemont 31 Jul 2013, 07:21:54

Thnx!


Priority: Normal

Type: Bug

State: Fixed

Assignee: Shannon Deminick

Difficulty: Very Easy

Category: Architecture

Backwards Compatible: True

Fix Submitted:

Affected versions: 6.0.0, 6.1.1, 6.1.2, 6.1.3

Due in version: 6.1.4

Sprint:

Story Points:

Cycle: