Exceptions for Authorized Objects should propagate when returned from a Controller #17074
Conversation
spring-projects-issues
added
the
status: waiting-for-triage
evgeniycheban
force-pushed
the
gh-16058
branch
4 times, most recently
from
5e6afe3 to
fb85470
Compare
There was a problem hiding this comment.
Nice solution, @evgeniycheban! I've left a piece of feedback inline.
|
|
||
| @Override | ||
| public void extendHandlerExceptionResolvers(List<HandlerExceptionResolver> resolvers) { | ||
| resolvers.add(0, new HttpMessageNotWritableAccessDeniedExceptionResolver()); |
There was a problem hiding this comment.
One thing that concerns me is that this might break applications that have an @ExceptionHandler for AccessDeniedException-caused HttpMessageNotWritableException already.
Further, it would be nice if there were a clear way for applications to override Security's default handling of this scenario.
There was a problem hiding this comment.
I implemented a fix for this scenario by finding the DefaultHandlerExceptionResolver's index and adding Security's ExceptionResolver before the default resolver. This way user-defined @ExceptionHandler will always take precedence and try to resolve an exception first since the ExceptionHandlerExceptionResolver is added before the extendHandlerExceptionResolvers method call. I think we could also consider providing a configuration option to control wether Spring Security should handle this type of scenario or not.
There was a problem hiding this comment.
I imagine that the way to override would be to add an exception handler or add their own exception resolver. Is there another way that you had in mind?
There was a problem hiding this comment.
I think then it's possible now to override it by defining a custom @ExceptionHandler, I have added a test covering that.
There was a problem hiding this comment.
Btw, nice polish @jzheaux! I forgot to remove an instanceof check for HttpMessageNotWritableException that was initially added to allow users to handle AccessDeniedExceptions that happened in the application code, not during serialization, but since we changed the registration of Security's ExceptionResolver adding it after user-defined @ExceptionHandlers we can now handle all AccessDeniedExceptions that the user left uncaught those might happen not only during serialization but for example during deserialization via @PreAuthorize setters.
jzheaux
added
in: web
evgeniycheban
force-pushed
the
gh-16058
branch
2 times, most recently
from
230dc85 to
f89970e
Compare
|
Hi @jzheaux I updated the PR, according to your feedback, thanks. |
…a Controller Closes spring-projectsgh-16058 Signed-off-by: Evgeniy Cheban <[email protected]>
Any time a response handler throws an exception, we want to propagate an underlying AccessDeniedException if their is one. Issue spring-projectsgh-16058
|
Thanks, @evgeniycheban! This will merge to |
3 participants
Closes gh-16058