Should I be testing for an AdalException inside an ActionFilter?

Question

In some Azure Samples on GitHub like this one we have an example of using ADAL to access a protected Web API resource. The effort is protected by a try/catch looking for an AdalException

I'll summarize the code as thus:

 try 
 { 
     //pseudo code...  configure a client to use an access token..
     var token = ADAL.AcquireTokenAsync...
     var httpClient = new HttpClient(baseUri, token);

     // use the token for querying some protected API
     var result = //await HttpClient to return data...

     return View(result); 
 } 
 catch (AdalException) 
 { 
     // do something important with the exception, 
     // e.g. return an error View w/login link
 } 

So, as I start to flesh out my MVC controllers to use ADAL-access_token'd requests, do I really want all this try/catch business in every controller?

Does it make sense to create an ActionFilter? This snippet was inspired by code I saw at this Azure Sample

public class AdalErrorAttribute : FilterAttribute, IExceptionFilter
{
    void IExceptionFilter.OnException(ExceptionContext filterContext)
    {
        if(filterContext.Exception is AdalException)
        {
            if (filterContext.RequestContext.HttpContext.Request.QueryString["reauth"] == "True")
            {
                //
                // Send an OpenID Connect sign-in request to get a new set of tokens.
                // If the user still has a valid session with Azure AD, they will not be prompted for their credentials.
                // The OpenID Connect middleware will return to this controller after the sign-in response has been handled.
                //
                filterContext.RequestContext.HttpContext.GetOwinContext().Authentication.Challenge(
                    new AuthenticationProperties(),
                    OpenIdConnectAuthenticationDefaults.AuthenticationType);
            }
        }
    }
}

My Context: I'm taking a rather homogenous set of scaffolded MVC controllers that were EntityFramework-centric at the time they were generated.. but now need to be re-tooled to access my Web API (via my new AutoRest client)

Answer 1

@vibronet made a good point - don't do that for your WebAPI endpoints. They're called with bearer auth, and shouldn't be automatically redirected to a sign-in process. Give back a 401 indicating the credentials are invalid and let it go.

But for an MVC app the user is using interactively, this is a reasonable idea to follow, subject to a couple of constraints.

1. Make sure your filter is very tight about what it matches. Ie. make sure it's only handling exceptions you can reasonably-certainly draw to an authentication issue (vs. being an access-denied issue). Perhaps a custom exception that gets thrown by your wrapping logic, or some extra conditions beyond it being an ADALException to make sure it's an issue solveable by logging in again.
2. Unless you really want every request have this handling, look at attaching it at the controller or action layer instead of globally.
3. Look out for potential "loop" issues, where you get an error, tell the user to log in again, get the error, make them log in again, etc. Maybe set some data in the session when triggering the login, or in the login process, or something like that. Better to give an error message than get the user's browser stuck in an infinite-redirect loop. (of course, this applies to the manually-handle-the-exception case too)

Hope that helps.