Skip to content

Passing more than Status Code to the Error action #2591

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
aspnet-hello opened this issue Jan 2, 2018 · 11 comments
Closed

Passing more than Status Code to the Error action #2591

aspnet-hello opened this issue Jan 2, 2018 · 11 comments
Labels
area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlewares area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates feature-diagnostics Diagnostic middleware and pages (except EF diagnostics)
Milestone

Comments

@aspnet-hello
Copy link

From @Ciantic on Thursday, July 14, 2016 4:19:35 AM

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 ...
}

Copied from original issue: aspnet/Diagnostics#316

@aspnet-hello aspnet-hello added this to the Backlog milestone Jan 2, 2018
@aspnet-hello
Copy link
Author

From @Tratcher on Thursday, July 14, 2016 3:40:34 PM

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

@aspnet-hello
Copy link
Author

From @Ciantic on Thursday, July 14, 2016 11:34:35 PM

@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?

@aspnet-hello
Copy link
Author

From @Tratcher on Friday, July 15, 2016 9:37:11 AM

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?

@aspnet-hello
Copy link
Author

From @blowdart on Friday, July 15, 2016 9:38:35 AM

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

@aspnet-hello
Copy link
Author

From @HaoK on Friday, July 15, 2016 11:22:40 AM

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

@aspnet-hello
Copy link
Author

From @Eilon on Saturday, November 26, 2016 7:53:41 PM

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

@mkArtakMSFT
Copy link
Member

@Tratcher, @HaoK, @blowdart should this be closed already?

@blowdart
Copy link
Contributor

blowdart commented Oct 8, 2018

Move it to 3, it's still often requested

@Eilon Eilon added area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates feature-diagnostics Diagnostic middleware and pages (except EF diagnostics) and removed repo:Diagnostics labels Nov 26, 2018
@pranavkm
Copy link
Contributor

@blowdart \ @HaoK, does IAuthorizationMiddlewareResultHandler that was added in 5.x help with this issue?

@HaoK
Copy link
Member

HaoK commented Oct 14, 2020

I think so, see https://github.com/dotnet/aspnetcore/blob/master/src/Security/samples/CustomAuthorizationFailureResponse/Authorization/SampleAuthorizationMiddlewareResultHandler.cs for an example of how to inspect the authorization failure and switch on which requirement failed and do whatever custom logic as desired.

@pranavkm
Copy link
Contributor

Doc article that shows it being used: dotnet/AspNetCore.Docs#20193

@ghost ghost locked as resolved and limited conversation to collaborators Nov 13, 2020
@amcasey amcasey added the area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlewares label Jun 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlewares area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates feature-diagnostics Diagnostic middleware and pages (except EF diagnostics)
Projects
None yet
Development

No branches or pull requests

7 participants