Asp .Net Core Why is my authorisation handler called even though the user wasn't authenticated?

Question

I have a problem, if i manually go to a page past login one i get an exception inside one of my policy handles because it cannot find a claim.

Why does this happen instead of redirectting the user to login page because he is not authenticated? I even set the authentication scheme because all my pages have

[Authorize("scheme"), Policy = "policy"]

Here is my whole startup code

public class Startup
{
    public Startup(IConfiguration configuration)
    {
        Configuration = configuration;
    }

    public IConfiguration Configuration { get; }

    // This method gets called by the runtime. Use this method to add services to the container.
    public void ConfigureServices(IServiceCollection services)
    {
        services.AddAuthentication()
         .AddCookie("ProductionAuth", options =>
         {
             options.ExpireTimeSpan = TimeSpan.FromDays(1);
             options.LoginPath = new PathString("/Production/Index");
             options.LogoutPath = new PathString("/Production/Logout");
             options.AccessDeniedPath = new PathString("/Production/AccessDenied/");
             options.SlidingExpiration = true;
         })
        .AddCookie("AdministrationAuth", options =>
        {
            options.ExpireTimeSpan = TimeSpan.FromDays(1);
            options.LoginPath = new PathString("/Administration/Index");
            options.LogoutPath = new PathString("/Administration/Logout");
            options.AccessDeniedPath = new PathString("/Administration/AccessDenied/");
            options.SlidingExpiration = true;
        });

        services.AddAuthorization(options =>
        {
            options.AddPolicy("HasArranqueActivo", policy =>
                policy.RequireAuthenticatedUser()
                .AddAuthenticationSchemes("ProductionAuth")
                .Requirements.Add(new HasArranqueActivoRequirement()
            ));

            options.AddPolicy("HasArranqueInactivo", policy =>
                 policy.RequireAuthenticatedUser()
                 .AddAuthenticationSchemes("ProductionAuth")
                .Requirements.Add(new HasArranqueInactivoRequirement()
            ));
                 options.AddPolicy("IsParagemNotOnGoing", policy =>
                 policy.RequireAuthenticatedUser()
                 .AddAuthenticationSchemes("ProductionAuth")
                 .Requirements.Add(new IsParagemNotOnGoingRequirement()
            ));
                 options.AddPolicy("IsParagemOnGoing", policy =>
                 policy.RequireAuthenticatedUser()
                 .AddAuthenticationSchemes("ProductionAuth")
                 .Requirements.Add(new IsParagemOnGoingRequirement()
            ));
        });

        services.AddMemoryCache();

        services.AddMvc()
         .AddRazorPagesOptions(options =>
         {
             options.AllowAreas = true;
             options.Conventions.AuthorizeAreaFolder("Administration", "/Account");
             options.Conventions.AuthorizeAreaFolder("Production", "/Account");
         })
         .AddNToastNotifyToastr(new ToastrOptions()
         {
             ProgressBar = true,
             TimeOut = 3000,
             PositionClass = ToastPositions.TopFullWidth,
             PreventDuplicates = true,
             TapToDismiss = true
         })
        .SetCompatibilityVersion(CompatibilityVersion.Version_2_2);
    }

    // This method gets called by the runtime. Use this method to configure the HTTP request pipeline.
    public void Configure(IApplicationBuilder app, IHostingEnvironment env)
    {
        if (env.IsDevelopment())
        {
            app.UseDeveloperExceptionPage();
        }

        app.UseNToastNotify();
        app.UseAuthentication();

        app.UseMvc(routes =>
        {
            routes.MapRoute(
                name: "default",
                template: "{controller=Home}/{action=Index}/{id?}");
        });
    }
}

This is my handler, every handler is basically the same. The requirements are mepty, they have nothing in the body.

public class IsParagemNotOnGoingHandler : AuthorizationHandler<IsParagemNotOnGoingRequirement>
{
    private readonly DatabaseContext _context;

    public IsParagemNotOnGoingHandler(DatabaseContext context)
    {
        _context = context;
    }

    protected override Task HandleRequirementAsync(AuthorizationHandlerContext context, IsParagemNotOnGoingRequirement requirement)
    {
        // Get the context       
        var redirectContext = context.Resource as AuthorizationFilterContext;

        var registoId = Convert.ToInt32(context.User.FindFirst(c => c.Type == ClaimTypes.PrimarySid).Value);
        bool IsRegistoOnGoing = _context.ParagensRegistos.Any(pr => pr.RegistoId == registoId && pr.HoraFim == null);

        if (!IsRegistoOnGoing)
        {
            context.Succeed(requirement);
        }
        else
        {
            redirectContext.Result = new RedirectToPageResult("/Paragem");
            context.Succeed(requirement);
        }
            
        return Task.CompletedTask;
    }
}

and this is the exception

An unhandled exception occurred while processing the request. NullReferenceException: Object reference not set to an instance of an object. NoPaper.Policies.IsParagemNotOnGoingHandler.HandleRequirementAsync(AuthorizationHandlerContext context, IsParagemNotOnGoingRequirement requirement) in IsParagemNotOnGoingHandler.cs, line 27

Stack Query Cookies Headers NullReferenceException: Object reference not set to an instance of an object. NoPaper.Policies.IsParagemNotOnGoingHandler.HandleRequirementAsync(AuthorizationHandlerContext context, IsParagemNotOnGoingRequirement requirement) in IsParagemNotOnGoingHandler.cs + var registoId = Convert.ToInt32(context.User.FindFirst(c => c.Type == ClaimTypes.PrimarySid).Value); Microsoft.AspNetCore.Authorization.AuthorizationHandler.HandleAsync(AuthorizationHandlerContext context) Microsoft.AspNetCore.Authorization.DefaultAuthorizationService.AuthorizeAsync(ClaimsPrincipal user, object resource, IEnumerable requirements) Microsoft.AspNetCore.Authorization.Policy.PolicyEvaluator.AuthorizeAsync(AuthorizationPolicy policy, AuthenticateResult authenticationResult, HttpContext context, object resource) Microsoft.AspNetCore.Mvc.Authorization.AuthorizeFilter.OnAuthorizationAsync(AuthorizationFilterContext context) Microsoft.AspNetCore.Mvc.Internal.ResourceInvoker.InvokeFilterPipelineAsync() Microsoft.AspNetCore.Mvc.Internal.ResourceInvoker.InvokeAsync() Microsoft.AspNetCore.Routing.EndpointMiddleware.Invoke(HttpContext httpContext) Microsoft.AspNetCore.Routing.EndpointRoutingMiddleware.Invoke(HttpContext httpContext) Microsoft.AspNetCore.StaticFiles.StaticFileMiddleware.Invoke(HttpContext context) Microsoft.AspNetCore.Authentication.AuthenticationMiddleware.Invoke(HttpContext context) NToastNotify.NtoastNotifyAjaxToastsMiddleware.InvokeAsync(HttpContext context, RequestDelegate next) Microsoft.AspNetCore.Builder.UseMiddlewareExtensions+<>c__DisplayClass5_1+<b__1>d.MoveNext() Microsoft.AspNetCore.StaticFiles.StaticFileMiddleware.Invoke(HttpContext context) Microsoft.AspNetCore.Diagnostics.DeveloperExceptionPageMiddleware.Invoke(HttpContext context)

Answer 1

In your AuthorizationHandler, you can check if the user is authenticated by looking at the context.User.Identity.IsAuthenticated property like so:

protected override Task HandleRequirementAsync(AuthorizationHandlerContext context, YourRequirementType requirement)
{
    if (context.User.Identity == null || !context.User.Identity.IsAuthenticated)
    {
        _logger.LogDebug("Authorization Failed: User not authenticated.");
        context.Fail(new AuthorizationFailureReason(this, $"User not authenticated"));
        return Task.CompletedTask;
    }

Answer 2

I see a few cases where uncaught exceptions could cause you headaches. They can be prevented as follows.

• The as operator performs a safe cast that returns null on an invalid cast. Your attempted member access on it, redirectContext.Result may throw. You can use a C# 7.0 pattern matching assignment to do a safe cast with a validity check in a single statement. If you cannot use C# 7, add a simple null check prior to access.

if (context.Resource is AuthorizationFilterContext filterContext)

• The User may not have the claim. In addition to what is mentioned in the comments, the user may or may not have the claim even if they are authenticated. My preferred access pattern for this is null-conditional access with first or default linq.

var primarySid = user.Claims.FirstOrDefault(c => c.Type == ClaimTypes.PrimarySid)?.Value

• The PrimarySid claim may or may not be an int. You might not set a string as the PrimarySid on purpose but bugs happen or a malicious user might.

if (!int.TryParse(primarySid, out var registoId))

• Optional, since the method returns a Task, you can perform an asynchronous database lookup which scales better and has the added side-benefit of not needing to return Completed Tasks every time you return.

Putting this all together,

protected override async Task HandleRequirementAsync(AuthorizationHandlerContext context, IsParagemNotOnGoingRequirement requirement)
{
    var user = context.User;
    if (!user.Identity.IsAuthenticated)
    {
        return;
    }

    var primarySid = user.Claims.FirstOrDefault(c => c.Type == ClaimTypes.PrimarySid)?.Value
    if (string.IsNullorWhiteSpace(primarySid)) 
    {
        return;
    }

    int registoId;
    if (!int.TryParse(rawUserId, out registoId)) 
    {
        return;
    }

    bool IsRegistoOnGoing = await _context.ParagensRegistos
                  .AnyAsync(pr => pr.RegistoId == registoId && pr.HoraFim == null);

    if (!IsRegistoOnGoing)
    {
        context.Succeed(requirement); // success!!
    }
    else
    {
        if (context.Resource is AuthorizationFilterContext filterContext)
        {
             filterContext.Result = new RedirectToPageResult("/Paragem");
        }     
        context.Succeed(requirement); // success here again?
    }
}

Also, your requirement succeeds regardless of whether the condition is true or false and you have performed a side-effect inside your authorisation handler which is to fire a redirect. This is not how it is supposed to work. You are supposed to fail or succeed requirement which in turn leads to a redirect that you have configured elsewhere. When your conditions mean nothing and you mix concerns and violate the separation of concerns principle, your application is going to be hard to maintain.

Provided that you stick with this architecture, you can shorten the final if to,

if (IsRegistoOnGoing && context.Resource is AuthorizationFilterContext filterContext)
{
    filterContext.Result = new RedirectToPageResult("/Paragem");
}
context.Succeed(requirement);

I highly recommend a code review once you straighten out the possible exceptions.

Answer 3

There's an important note in the docs that addresses this:

Authorization handlers are called even if authentication fails.

In your case, authentication has failed but your IsParagemNotOnGoingHandler's HandleRequirementAsync is still being called. To resolve the problem, you can just make your handler implementation more resilient to the claim being missing. Here's an example for completeness:

protected override Task HandleRequirementAsync(AuthorizationHandlerContext context, IsParagemNotOnGoingRequirement requirement)
{
    if (!context.User.HasClaim(c => c.Type == ClaimTypes.PrimarySid))
        return Task.CompletedTask;

    ...
}

You might want to protect against Convert.ToInt32 failing too, for the case where the claim's value isn't convertible to an int.