Support for secure repository methods when using DomainClassConverter via @PathVariable in Spring MVC [DATACMNS-576]

[spring-projects-issues]

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:

@RestController
@RequestMapping(value = "/person", produces = MediaType.APPLICATION_JSON_VALUE)
public class PersonController {

    @RequestMapping(value = "/{id}", method = RequestMethod.GET)
    @ResponseStatus(HttpStatus.OK)
    public Person personInfo(@PathVariable("id") Person person, @AuthenticationPrincipal CustomUser customUser) {

        if (person == null) {
            throw new InvalidPathVariableException();
        } else if (!customUser.hasRole(Service.ROLE_ADMIN) && !(person.getOrg().equals(customUser.getOrg()))) {
            throw new AccessDeniedException("You are not authorized to view this person.");
        }
        return person;
    }
}

and a repository:

public interface PersonRepository extends CrudRepository<Person, Long> {
}

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.

public interface PersonRepository extends CrudRepository<Person, Long> {
    @Override
    @PostAuthorize("false")
    Person findOne(Long id);
}

Some debugging finds that security does throw AccessDeniedException as desired. The problem stems from the DomainClassConverter, line 73:

return invoker.invokeFindOne(conversionService.convert(source, info.getIdType()));

which leads to GenericConversionSerivce, line 189:

Object result = ConversionUtils.invokeConverter(converter, source, sourceType, targetType);

which leads to ConversionUtils.invokeConverter(…). The try/catch block catches all Exception and throws ConversionFailedException. Thus the AccessDeniedException is replaced and ultimately becomes a org.springframework.beans.TypeMismatchException leading to a 400 response.

---

Affects: 1.9 GA (Evans)

1 votes, 4 watchers

[spring-projects-issues]

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:

1. don't use 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.
2. Write an exception handler for 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?

[spring-projects-issues]

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:

• Having ConversionUtils.invokeConverter(…) catch all Exceptions seems wrong. Shouldn't it only catch exceptions that relate to the conversion process itself, not everything? I was trying to explain that, while it seemed like Spring Security was not working on the Spring Data Repos, it is actually working perfectly there.

Otherwise, yes, as you stated, the controller should respond with 200, 404 or 403 as appropriate.

1. This solution does work, but using Person as as parameter type is a very nice feature. It seems a shame to give up a feature as a workaround.
   • Also, while we are discussing Spring MVC here, the recommended solution should also be something to use with Spring Data Rest.
2. Perhaps, this is the best solution for MVC and SDR? The chaining of cause seems tricky. In my case, the 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.

[spring-projects-issues]

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:

• have a @ControllerAdvice with @ExceptionHandler methods to handle ConversionFailedException instances
• in that method, inspect the exception to see whether the target type is a Spring Data managed domain class. If so, inspect the cause of the exception for an AccessDeniedException and return 403

[spring-projects-issues]

Ted 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:

https://github.com/spring-projects/spring-data-examples/blob/master/rest/security/src/main/java/example/company/ItemRepository.java

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.

[spring-projects-issues]

Ted Bergeron commented

Referenced in SEC-2975. Unfortunately, I do not have permissions to create an issue link

[spring-projects-issues]

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.

[spring-projects-issues]

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

[spring-projects-issues]

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

[spring-projects-issues]

Mark Paluch commented

Closing as per comment