Closed spring-projects-issues closed 4 years ago
Oliver Drotbohm commented
To be honest, you lost me in the middle of your post. There's a lot of stuff you describe working together and lead to unexpected behavior. But what exactly is it, that you'd like to see?
If I am reading you correctly, you expect the controller declaration to cause a 403 Forbidden
directly as the lookup of the domain type should cause the AccessDeniedException
, right? I see a two ways you can get that to work right now:
Person
as parameter type but rather either use the repository or the service (depending on where you'd like to declare the security constraints on) to lookup the instance manually. This will cause AccessDeniedException
being thrown from the controller and translated into the correct error code.ConversionFailedException
in an @ControllerAdvice
annotated type, inspect the cause and return 403 Forbidden
if you find an AccessDeniedException
.The second item is something that could ultimately end up in Spring Security but would need to be filed in their bug tracker. Rob Winch, Rossen Stoyanchev - Any thoughts?
Ted Bergeron commented
The email for your comment must have gone to my junk folder.
Ok, to boil it down on what would be good to see:
Otherwise, yes, as you stated, the controller should respond with 200, 404 or 403 as appropriate.
Person
as as parameter type is a very nice feature. It seems a shame to give up a feature as a workaround.
AccessDeniedException
is the cause of ConversionFailedException
, but to be defensive in code, I suppose I'd have to walk the cause chain until I either found the security exception or reached the root cause?I do have an extension of ResponseEntityExceptionHandler
with some custom handlers, so I can add solution #2 there.
Oliver Drotbohm commented
Reading up on our conversation I still don't quite understand why you'd like to move the security control down to the repository layer. Compared to the approach of having security aspects configured on the controller you actually get a lot more code executed before eventually finding out someone's not allowed to actually perform the action. I'd argue, the sooner you can find out about sufficient or insufficient permissions to trigger some functionality, the better.
I guess the main reason ConversionUtils
handles Exception
, too, is that without that catch clause you basically loose all context about the conversion attempt that triggered the exception. The reason I think Spring Data is the wrong place to do something about this is that we'd need to introduce a dependency to Spring Security and basically handle a very special case here.
So what if you did the following:
@ControllerAdvice
with @ExceptionHandler
methods to handle ConversionFailedException
instancesAccessDeniedException
and return 403Ted Bergeron commented
For cases where I'd use @PreAuthorize
, I agree, doing that in the controller causes less code execution. Here I needed @PostAuthorize
, so putting that on the findOne(Long id)
method of the repository is the closest to the minimum required amount of code to run.
The other reason I was thinking of security in the repository layer is that I was thinking of moving my boilerplate cases from MVC to Spring Data Rest. In that case, I wouldn't have the MVC source code to modify. I found example code from you and Greg that was putting similar security checks into the repository:
This article about SDR uses @RepositoryEventHandler
to handle security for write operations, but still repositories for securing reads. http://jaxenter.com/rest-api-spring-java-8-112289.html
Ultimately, I'm looking for a best practice recommendation that Rob Winch, Rossen Stoyanchev and yourself would endorse. In an environment of Boot, Spring Data Rest, Spring Security and some custom MVC controllers, how would you choose to implement security? Not only would one want a 403 response where needed, but with HATEOAS revealing links, ideally the OPTIONS command would be smart enough to not tell the user they can send PUT, PATCH or DELETE if the security settings would lead to a 403 for those calls. If a user did not have any access to a resource, due to security, I'd think the HATEOAS information would omit that link completely.
Those features may not exist today, but I'd like to be forward thinking in the approach.
Thank you for the feedback. At the very least, I will have an @ExceptionHandler
method to handle ConversionFailedException
.
Ted Bergeron commented
Referenced in SEC-2975. Unfortunately, I do not have permissions to create an issue link
Ted Bergeron commented
Just a clarification, now that I am implementing this; The actual stack trace is a chain of 3 exceptions:
org.springframework.beans.TypeMismatchException: Failed to convert value of type 'java.lang.String' to required type 'com.xyz.Person';
nested exception is org.springframework.core.convert.ConversionFailedException: Failed to convert from type java.lang.String to type @org.springframework.web.bind.annotation.PathVariable com.xyz.Person for value '28107749';
nested exception is org.springframework.security.access.AccessDeniedException: Access is denied
I have an extension of org.springframework.web.servlet.mvc.method.annotation.ResponseEntityExceptionHandler which is already handling TypeMismatchException. Thus, I overrode the handleTypeMismatch method and added the logic to inspect the rootCause there.
Jan Škach commented
I have same problem, but the #2 solution is not working for me.
I use Spring data not only for repository, but as controllers too. (My service and controller layer is very slim).
When the exception is thrown by RepositoryEntityController, org.springframework.data.rest.webmvc.AbstractRepositoryRestController#handleMiscFailures is used for the ConversionFailedException, no matter if I define my own @ExceptionHandler
in global handler which is @ControllerAdvice
.
There is look searching the exception handler in class where the exception is thrown as first, and its RepositoryEntityController
Jens Schauder commented
The "workaround" described by Oliver and implemented by Ted seems to work fine and seem to be adequate since the conversion from exceptions to http return codes is a decision made by the application developer. Whatever we would decide to do would always just fit some of the use cases.
Regarding the last comment by Jan: while similar this seems to be a different issue and should be filed against Spring Data Rest.
If no new arguments come up, I'll close this issue as Won't fix
Mark Paluch commented
Closing as per comment
Ted Bergeron opened DATACMNS-576 and commented
Using Spring 4.1, Spring Data Evans, Spring Security 3.2.5, Java 8. I spoke with Oliver at the SpringOne 2014 BOF session and am documenting this problem here. The overall concern is how to properly implement security when using Spring Data. Typically, a Service layer will handle transaction and security concerns, but Spring Data bypasses this layer. The specific case here is when using
DomainClassConverter
. Given an MVC method like:and a repository:
The system will work fine, giving a
Person
object on the happy path, 403 when appropriate. It would be nicer to remove the security logic and use@PostAuthorize
instead. Doing so in the controller has no immediate effect, because the controller is implementation, not an interface. One could change the proxy mode or create a controller interface, but those options are not preferred.This blog describes overriding the repository interface methods to use
@PostAuthorize
in the repository layer. This seems better than security in the controllers, though not as clean as securing a service layer. Adding security like this immediately causes integration tests to return 400 instead of 403. I reduced the SPEL to just "false" which still causes the issue.Some debugging finds that security does throw
AccessDeniedException
as desired. The problem stems from the DomainClassConverter, line 73:which leads to
GenericConversionSerivce
, line 189:which leads to
ConversionUtils.invokeConverter(…)
. The try/catch block catches allException
and throwsConversionFailedException
. Thus theAccessDeniedException
is replaced and ultimately becomes aorg.springframework.beans.TypeMismatchException
leading to a 400 response.Affects: 1.9 GA (Evans)
1 votes, 4 watchers