ThreadStatic and ASP.NET

Question

I've got a requirement to protect my business object properties via a list of separate authorization rules. I want my authorization rules to be suspended during various operations such as converting to DTOs and executing validation rules (validating property values the current user does not have authorization to see).

The approach I'm looking at wraps the calls in a scope object that uses a [ThreadStatic] property to determine whether the authorization rules should be run:

public class SuspendedAuthorizationScope : IDisposable
{
    [ThreadStatic]
    public static bool AuthorizationRulesAreSuspended;

    public SuspendedAuthorizationScope()
    {
        AuthorizationRulesAreSuspended = true;
    }

    public void Dispose()
    {
        AuthorizationRulesAreSuspended = false;
    }
}

Here is the IsAuthorized check (from base class):

public bool IsAuthorized(string memberName, AuthorizedAction authorizationAction)
{
    if (SuspendedAuthorizationScope.AuthorizationRulesAreSuspended)
        return true;

    var context = new RulesContext();

    _rules.OfType<IAuthorizationRule>()
        .Where(r => r.PropertyName == memberName)
        .Where(r => r.AuthorizedAction == authorizationAction)
        .ToList().ForEach(r => r.Execute(context));

    return context.HasNoErrors();
}

Here is the ValidateProperty method demonstrating usage (from the base class):

private void ValidateProperty(string propertyName, IEnumerable<IValidationRule> rules)
{
    using (new SuspendedAuthorizationScope())
    {
        var context = new RulesContext();
        rules.ToList().ForEach(rule => rule.Execute(context));
         if (HasNoErrors(context))
            RemoveErrorsForProperty(propertyName);
        else
            AddErrorsForProperty(propertyName, context.Results);
    }
    NotifyErrorsChanged(propertyName);   
}

I've got some tests around the scoping object that show that the expected/correct value of SuspendedAuthorizationScope.AuthorizationRulesAreSuspended is used as long as a lambda resolves in the scope of the using statement.

Are there any obvious flaws to this design? Is there anything in ASP.NET that I should be concerned with as far as threading goes?

Answer 1

There are two concerns that I see with your proposed approach:

1. One's failure to use using when creating SuspendedAuthorizationScope will lead to retaining open access beyond intended scope. In other words, an easy to make mistake will cause security hole (especially thinking in terms of future-proofing your code/design when a new hire starts digging in unknown code and misses this subtle case).
2. Attaching this magic flag to ThreadStatic now magnifies the previous bullet by having possibility of leaving open access to another page since the thread will be used to process another request after it's done with the current page, and its authorization flag has not been previously reset. So now the scope of authorization lingering longer than it should goes not just beyond missing call to .Dispose(), but actually can leak to another request / page and of completely different user.

That said, the approaches I've seen to solving this problem did involve essentially checking the authorization and marking a magic flag that allowed bypass later on, and then resetting it.

Suggestions: 1. To at least solve the worst variant (#2 above), can you move magic cookie to be a member of your base page class, and have it an instance field that is only valid to the scope of that page and not other instances? 2. To solve all cases, is it possible to use Functors or similar means that you'd pass to authorization function, that would then upon successful authorization will launch your Functor that runs all the logic and then guarantees cleanup? See pseudo code example below:

void myBizLogicFunction()
{
   DoActionThatRequiresAuthorization1();
   DoActionThatRequiresAuthorization2();
   DoActionThatRequiresAuthorization3();
}

void AuthorizeAndRun(string memberName, AuthorizedAction authorizationAction, Func privilegedFunction)
{
    if (IsAuthorized(memberName, authorizationAction))
    {
        try
        {
            AuthorizationRulesAreSuspended = true;

            privilegedFunction();
        }
        finally
        {
            AuthorizationRulesAreSuspended = true;
        }
    }
}

With the above, I think it can be thread static as finally is guaranteed to run, and thus authorization cannot leak beyond call to privilegedFunction. I think this would work, though could use validation and validation by others...

Answer 2

If you have complete control over your code and don't care about hidden dependencies due to magic static value you approach will work. Note that you putting big burden on you/whoever supports your code to make sure there is never asynchronous processing inside your using block and each usage of magic value is wrapped with proper using block.

In general it is bad idea because:

• Threads and requests are not tied one-to one so you can run into cases when you thread local object is changing state of some other request. This will even more likely to happen in you use ASP.Net MVC4+ with async handlers.
• static values of any kind are code smell and you should try to avoid them.

Storing request related information should be done in HttpContext.Items or maybe Session (also session will last much longer and require more careful management of cleaning up state).

Answer 3

My concern would be about the potential delay between the time you leave your using block and the time it takes the garbage collector to get around to disposing of your object. You may be in a false "authorized" state longer than you intend to be.