spring-projects / spring-security

Spring Security
http://spring.io/projects/spring-security
Apache License 2.0
8.84k stars 5.91k forks source link

Improve Integration between Authorized Objects and Spring Data #15746

Open jzheaux opened 2 months ago

jzheaux commented 2 months ago

If an authorized object is sent to Spring Data, for example using CrudRepository#save, the call fails since it tries to look up model metadata by the class name, a CGLIB name in this case.

Moreover, if an authorized object is sent to CrudRepository#save (and the call succeeded), then the associated masks and other authorization handling would apply if its methods are called.

Consider the following sample controller method:

@PutMapping("/{id}")
public Message update(@PathVariable("id") Long id, @RequestBody String body) {
    Message message = this.messageRepository.findById(id); // authorized, if using `@AuthorizeReturnObject`
    // ... 
    // only authorized operations on the object
    // ...
    return this.messageRepository.save(message); // if still wrapped, then unwanted masking or other error handling could ensue when persisting
}

Because a proxied object could be used as a method parameter anywhere in the application, Security can't know on its own any circumstances where it should unwrap the object.

One way to address this could be for Spring Data to detect AuthorizationProxy-implementing domain objects and unwrap them. The following sample illustrates the issue in its updateMessage method.

mp911de commented 2 months ago

Calling a Spring Data repository with an authorized return object requires unwrapping. Spring Data repositories are proxies already hence the advisor would be a good place to unwrap the target object.

You can identify Spring Data repositories as these implement org.springframework.data.repository.Repository.

jzheaux commented 2 months ago

Calling a Spring Data repository with an authorized return object requires unwrapping. Spring Data repositories are proxies already hence the advisor would be a good place to unwrap the target object.

@mp911de I think this is a good point. Truly, it's better for Spring Security to unproxy its own proxies.

I'm not certain yet how generally applicable such a heuristic would be (outside of repositories), but I reason it's likely every proxied method parameter in a Repository class would want to be unproxied.

A more general-purposes way we could do it is add a companion annotation like @PermitParameter that applications can use to identify which parameters should no longer be advised:

@Repository 
public class MessageRepository ... {
    @AuthorizeReturnObject
    Optional<Message> findById(Long id);

    void save(@PermitParameter Message message);
}

Ostensibly, it could be added at the method or class level to reduce duplication.

Down the road, an application could more generally apply this to repositories though its proposed mixin support.

noshua commented 1 month ago

@jzheaux Can you give me a hint please how I can unwrap a CGLib proxy to call save() on my entity using Spring 6.3 as I'm currently suffering from this issue? Thanks in advance.

If anyone interested, here is my solution to unwrap a proxy under Spring 6.3:

    @SuppressWarnings("unchecked")
    public static <T> T unwrap(T proxy) {
        try {
            return proxy instanceof Advised advised ? (T) advised.getTargetSource().getTarget() : proxy;
        } catch (Exception e) {
            throw new RuntimeException(e);
        }
    }
noshua commented 1 month ago

@jzheaux I ran into another problem. When I try to fetch an pageable search results of authorized entities I get:

java.lang.ClassCastException: class org.springframework.security.authorization.method.AuthorizationAdvisorProxyFactory$ContainerTypeVisitor$$Lambda/0x00007518415f66a0 cannot be cast to class org.springframework.data.domain.Page (org.springframework.security.authorization.method.AuthorizationAdvisorProxyFactory$ContainerTypeVisitor$$Lambda/0x00007518415f66a0 and org.springframework.data.domain.Page are in unnamed module of loader 'app')

jzheaux commented 1 month ago

Thanks, @noshua, can you please open a separate ticket with a reproducer, and we can link that ticket to this one?

noshua commented 1 month ago

@jzheaux Sure, here you go: https://github.com/spring-projects/spring-security/issues/15994

noshua commented 1 week ago

@jzheaux dunno if you have already thought about that so I wanted to mention. If you have 2 entities A and @AuthorizeReturnObject B with any relationship like @OneToOne. When you save A it also fails because the Spring Data stack thinks B$Cglib is an unsaved transient instance.

org.hibernate.TransientPropertyValueException: object references an unsaved transient instance - save the transient instance before flushing : com.example.A.b -> com.example.B

So a solution like you supposed would also need to unwrap relations recursively even if A isn't wrapped itself. void save(@PermitParameter A a);