Skip to content
This repository has been archived by the owner on Dec 8, 2018. It is now read-only.

Passing more than Status Code to the Error action #316

Closed
Ciantic opened this issue Jul 14, 2016 · 7 comments
Closed

Passing more than Status Code to the Error action #316

Ciantic opened this issue Jul 14, 2016 · 7 comments

Comments

@Ciantic
Copy link

Ciantic commented Jul 14, 2016

Currently there is a problem with the way JSON error results are handled e.g.: aspnet/Security#872 and aspnet/Security#699 there is no easy way to pass more than status code to your Error action that shows the error page or JSON result.

Sometimes it would be nice to have a reason in the result e.g. "Insufficient rights, because you are not an employee" or in JSON { "error" : "FORBIDDEN", "requires" : ["EmployeeOnly"] } to show a dialog why you are forbidden to see the page.

But since only thing the error handler gets is the status code it can't determine the extra requirement, claim in this case.

Imagine the situation that you have this:

services.AddAuthorization(opts => {
    // ...
    opts.AddPolicy("EmployeeOnly", policy => {
        policy.AddAuthenticationSchemes(JwtBearerDefaults.AuthenticationScheme);
        policy.RequireClaim("EmployeeNumber");
    });
});

Then this as a Error handler for status code pages:

[HttpGet("/Error"), HttpPost("/Error")]
public IActionResult Error([FromQuery] int status = 400)
{
    if (HttpContext.Request.Headers.ContainsKey("Accept") && 
        HttpContext.Request.Headers["Accept"].Contains("application/json"))
    { 
        if (status == 403) { 
            // PROBLEM: There is no way to get the failing reason (e.g. claim "EmployeeOnly") here? 
            return new JsonResult(new { error = "FORBIDDEN" });
        }
        else
        {
            return new JsonResult(new { error = "UNDEFINED_ERROR" });
        }
    }
    // HTML Version if you wish ...
}
@Tratcher
Copy link
Member

Are you throwing an exception from your authorization check? If so that exception is added to the HttpContext via a Feature:
https://github.com/aspnet/Diagnostics/blob/dev/src/Microsoft.AspNetCore.Diagnostics/ExceptionHandler/ExceptionHandlerMiddleware.cs#L64-L68

@Ciantic
Copy link
Author

Ciantic commented Jul 15, 2016

@Tratcher that is interesting, however I think the problem is the claim authorization check (which is done internally in MVC somewhere) doesn't throw error. That claim is what was also asked in aspnet/Security#872 for the status code pages.

Could there be a workaround to force claim requirement checking done by MVC code to throw an error with claim inside?

@Tratcher
Copy link
Member

We try not to throw exceptions for normal control flow.

@HaoK @blowdart It would be useful for failed requirements to be able to provide explanatory data that could flow to other parts of the application (e.g. the login page or the access-denied page). Thoughts?

@blowdart
Copy link
Member

Could we add something to the context? Exceptions are a horrible idea as you've stated.

@HaoK
Copy link
Member

HaoK commented Jul 15, 2016

Filed aspnet/Security#901 to track figuring out this general issue of how to expose which requirements didn't pass in Security

@Eilon
Copy link
Member

Eilon commented Nov 27, 2016

Due to aspnet/Security#901 being 2.0.0, moving this as well.

@aspnet-hello
Copy link

This issue was moved to dotnet/aspnetcore#2591

@aspnet aspnet locked and limited conversation to collaborators Jan 2, 2018
@aspnet-hello aspnet-hello removed this from the Backlog milestone Jan 2, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants