ReflectedActionDescriptorProvider does not ignore private types

[pranavkm]

https://github.com/aspnet/Mvc/blob/dev/src/Microsoft.AspNet.Mvc.Core/ReflectedActionDescriptorProvider.cs#L50 does not filter out non-public types (and possibly other non-interesting types). We need to be more restrictive about the things that get filtered by it.

[puncsky]

Potential fix locates at https://github.com/aspnet/Mvc/blob/dev/src/Microsoft.AspNet.Mvc.Core/ReflectedActionDescriptorProvider.cs#L51 and

Mvc/src/Microsoft.AspNet.Mvc.Core/DefaultActionDiscoveryConventions.cs

Line 34 in 03b2723

public virtual bool IsController([NotNull] TypeInfo typeInfo)

[yishaigalatzer]

Investigate, compare MVC & Web API, and come up with a recommednation

[puncsky]

In summary, I would recommend to add typeInfo.IsPublic (consistent with MVC5.2), which is true if the class is public and not nested in other classes. User-defined controllers should not be exposed to the user when they are private. It gives the user more choices on their controllers' visibility.

On the other hand, this kind of hierarchies below is meaningless because hierarchies are achieved by routes and areas.

public class FirstController
{
    public class SecondController
    {
        public class ThirdController
        {
            // ...

To make it even more explicit, we could also add !typeInfo.IsNested.

Repro and Investigation

public class ClassWithNestedController
{
    public class HomeController : [Api]Controller
    {
        // This controller is invalid in MVC5.2 but valid in Web API.
    }
}

public class ClassWithNestedController
{
    public class InnerController : [Api]Controller
    {
        // This controller is invalid in both MVC5.2 and Web API.
    }
}

The inconsistency is caused by:

MVC5.2:

    internal static bool IsControllerType(Type t)
    {
        return
            t != null &&
            t.IsPublic && // false for both public and private InnerController
            t.Name.EndsWith("Controller", StringComparison.OrdinalIgnoreCase) &&
            !t.IsAbstract &&
            typeof(IController).IsAssignableFrom(t);
    }

Web API:

    internal static bool IsControllerType(Type t)
    {
        Contract.Assert(t != null);
        return
            t != null &&
            t.IsClass &&
            t.IsVisible && // false for private InnerController. true for public InnerController. 
            !t.IsAbstract &&
            typeof(IHttpController).IsAssignableFrom(t) &&
            HasValidControllerName(t);
    }

MVC6.0, neither IsPublic nor IsVisible existes.

    public virtual bool IsController([NotNull] TypeInfo typeInfo)
    {
        if (!typeInfo.IsClass ||
            typeInfo.IsAbstract ||
            typeInfo.ContainsGenericParameters)
        {
            return false;
        }

        if (typeInfo.Name.Equals("Controller", StringComparison.OrdinalIgnoreCase))
        {
            return false;
        }

        return typeInfo.Name.EndsWith("Controller", StringComparison.OrdinalIgnoreCase) ||
               typeof(Controller).GetTypeInfo().IsAssignableFrom(typeInfo);
    }

[puncsky]

@yishaigalatzer , what is your idea?

[rynowak]

Decision here is to use MVC5's behavior. Only public-top-level classes will be considered as controllers by default. This is easy to customize by overriding DefaultActionDiscoveryConventions.IsController.

[rynowak]

Fixed by bff94f1