Authorization/authentication errors thrown before feature flag is checked

[ianpaul10]

If you have a ASP.Net Core app with a controller with the following method:

[HttpGet]
[FeatureGate(FeatureFlags.Foo)]
[Authorize]
public IEnumerable<string> Get()
        {
            return new string[] { "value1", "value2" };
        }

Currently if:

1. A request is made with correct scopes and the feature is turned on -> 200 returned (expected)
2. A request is made with incorrect scopes and the feature is turned on -> 401 returned (expected)
3. A request is made with correct scopes and the feature is turned off -> 404 returned (expected)
4. A request is made with incorrect scopes and the feature is turned off -> 401 returned (unexpected)

In scenario 4 (or any scenario when the feature is turned off) I would have imagined that it would always return a 404 so that a client wouldn't be made aware of new features it doesn't have access to.

[rjgotten]

@ianpaul10
Don't use the [FeatureGate] attribute.
Its design is, frankly, broken.

Just implement an IActionConstraint which checks the IFeatureManagerSnaphot and register that on your controller action.
IActionConstraint acts at the routing level and if the feature your constraint is checking, is disabled - then the constraint can report as violated and cause the routing system to completely skip the action as a valid routable endpoint. Then it's the routing system itself which will fall through to its fallback endpoint, most likely a 404 handler, and you don't need to futz about with any custom handlers needing to be wedged into the correct spot in the stack of MVC filters.

An action constraint is always how the entire thing should have been designed, but sadly wasn't.

Alternatively - and a bit more advanced - implement the behavior as an IEndpointSelectorPolicy and you can apply feature gating to more types of endpoints than just the MVC controllers...

[jimmyca15]

@rjgotten

Using IActionConstraint is a valid suggestion to handle routing based off of feature state. IActionConstraint doesn't provide asynchronous evaluation so that might be a factor to limit adoption.

I don't consider the design of [FeatureGate] to be broken. It solves the problem differently. IActionConstraint will make the action not routable, as you said. FeatureGate will route to the action and provide a chance to customize the response using IDisabledFeaturesHandler with the original action's context available.

[jimmyca15]

@ianpaul10

It looks to me like in cases (2) and (4) both requests are coming from an unauthenticated user. If 404 was returned for case 4, wouldn't your issue exist based off of the difference between (2) and (4)?

[jimmyca15]

@ianpaul10

After taking a second look I see the issue here. It seems that you do want this action to be not routable if the feature is disabled. @rjgotten's approach would be better here but there is no integration point for that in the feature management library. The FeatureGateAttribute is better suitable if you do want an action to still be routed to even when a feature is disabled, you just want the response to be customized, like maybe a custom page.

We can look to see how we can best integrate with the routing system to provide a better solution for your scenario where you don't want the action to be routable.

[zhenlan]

@ianpaul10 just want to add another perspective here. You don't want an unauthorized user to poke around and see what exists (401) and what doesn't (404). So from a security perspective, it's a good idea you always respond 401/403 for unauthenticated/unauthorized users regardless of the state of a feature.

[rjgotten]

@zhenlan
You don't want an unauthorized user to poke around and see what exists (401) and what doesn't (404).

Conversely, you may also not want e.g. a tenant in a multi-tenant application to be aware certain features used for other tenants, exist - regardless of whether they have the necessary permission level to access them or not.
Both are valid scenarios.

For completeness sake: the 401 could also be returned from whatever is your configured fallback route. If you route to an error controller, that controller could also be set up to require authentication and authorization, only showing the error response to authenticated users and showing a 401 Unauthorized for others. (Actually; in that case a 403 may be more appropriate even.)

I'd also argue that that is actually the correct way to handle it, as it properly separates concerns; does not rely on weird quirks in the order of filter evaluation on MVC actions; and can also cover any other scenario. E.g. if you want a consistent 401/403 instead of a 404, you'll also have to cover e.g. non-existent controllers or action methods -- and solving this at the routing level within a fallback error controller accomplishes just that.

@jimmyca15
Using IActionConstraint is a valid suggestion to handle routing based off of feature state. IActionConstraint doesn't provide asynchronous evaluation so that might be a factor to limit adoption.

IEndpointSelectorPolicy though, does. 😉