-
Notifications
You must be signed in to change notification settings - Fork 597
Custom JSON response from Policy-Based Authorization is being overwritten #872
Comments
The repro is very MVC specific, it may be an issue with the flow there. |
In RC1 I was using AuthorizationFilterAttribute to check my requirements and create a custom JSON response with a message and 403 status code. In RC2 the AuthorizationFilterAttribute has been removed (aspnet/Mvc#4233) because authorization policies and requirements are now the recommended way to handle authorization. However, I can't figure out how to use policies to return a custom message/status code. If any of my policy handlers fail then whatever middleware (I think its the middleware?) I use seems to intercept the response and overwrite it with a raw 403 response. I've tried the following middleware: |
@blowdart for his thoughts as well. but its not the Policy's job to return custom message or status code. you should be able to handle authZ failures gracefully at a higher level. |
What higher level should I be handling this at? I don't want to fill each controller action with authorization logic and the AuthorizationFilterAttribute has been removed... It makes sense to me that I can decorate a controller action with something like:
Especially as I add more requirements to my policies... A client could receive a 403 for any number of reasons and its helpful if the client can know why it was forbidden. I understand not wanting to set custom status codes from requirement handlers but what about a custom message? That way we can optionally tell a client/user what requirement they failed on. |
Its not unreasonable, but it'd be hard to make generic so it flows over all different types of responses, headers, JSON or HTML. That's certainly outside of the scope for authorization itself, as it has no notion of presentation. Worth thinking about in 1.1. |
@levitatejay I took the liberty to close the related ticket you opened on the OpenIddict repository, since OpenIddict is not responsible of applying the authorization policies.
As mentioned, the scenario you're trying to achieve will likely require important infrastructure changes in ASP.NET Core to make the authorization stack less opaque than it is currently (at least, to allow returning an error message). Unfortunately, this is not something I can easily implement in the OAuth2 validation middleware (we'll port the Definitely a scenario to support in the next version, IMHO. |
Could someone please recommend a workaround for the meantime? In RC1 I was creating a custom I have a few ideas but I don't like any of them:
|
Also where is the response body being overwritten? I can see in
But where is the body overwritten? |
It's probably not overwritten so much as merely ignored and never sent from |
@Tratcher Thank you! I just discovered that I can change the response from the requirement handler by using: It seems that writing to the response body causes the auth middleware to ignore the challenge... (HandleForbiddenAsync isn't getting called). I'm not sure why though? Is this okay as a workaround until there is a supported way to return messages for requirements? I'm nervous about interfering too much with the authentication pipeline |
HandleForbiddenAsync can't run after you write to the body because the headers have already been sent. I don't recommend this approach as it can flush the headers before MVC is finished and may cause errors if MVC tries to set more headers. |
If the authorization middleware has no notion of presentation then it should be possible for us to put our own middleware in the pipeline that can inspect what requirements failed for the request and modify the response accordingly. |
Where is this handler? I also would like to return different JSON messages when different claim requirements fail. Right now I'm getting empty result with status code 403. Supppose I have this now: services.AddAuthorization(opts => {
// ...
opts.AddPolicy("EmployeeOnly", policy => {
policy.AddAuthenticationSchemes(JwtBearerDefaults.AuthenticationScheme);
policy.RequireClaim("EmployeeNumber");
});
}); Update I found a way to get generic errors by using Status Code Pages extension. The problem still is there is no way to get the claim name in the status code pages and use it in the JSON results e.g.: But if you don't need the EmployeeOnly in your results, it works like this: // This line must be before app.UserMvc() in the Configure
app.UseStatusCodePagesWithReExecute("/Error", "?status={0}"); Then having this: [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) {
// TODO: There is no way to get claim here!
return new JsonResult(new { error = "FORBIDDEN" });
}
else if (status == 401)
{
return new JsonResult(new { error = "LOGIN_REQUIRED" });
}
else
{
return new JsonResult(new { error = "UNDEFINED_ERROR" });
}
}
// HTML Version if you wish ...
} Only thing needed is a way to pass more than status code (e.g. claim) from the StatusCodePages extension to the custom error handler. |
Apologies but I think this would be more nicely handled by status code pages extension, I opened an issue there: aspnet/Diagnostics#316 |
Will be tracked via #901 as well |
I was able to send a custom authorization failure message by adding a custom error handling Middleware and let the AuthorizationHandler throw the exception (with the proper message) I am expecting in the Middleware and write to the httpContext.Response. |
I would double on that. Need a way to indicate the reason why policy is failed. In our case it is when user need to accept new "privacy policy". |
I would also like to be able to set a response body depending on the policy that failed. Right now I'm using the events from
|
Even though
Still, it would be nice to be able to manually generate the response from an authorization handler; using a filter for this is a bit of a hack, and doesn't support DI. |
aspnet/Identity#858
The text was updated successfully, but these errors were encountered: