spring-projects / spring-boot

Spring Boot helps you to create Spring-powered, production-grade applications and services with absolute minimum fuss.
https://spring.io/projects/spring-boot
Apache License 2.0
75.15k stars 40.68k forks source link

@MockBean "ignores" @PreAuthorize annotation on interface method while Mockito.mock(...) does not #28036

Closed drahkrub closed 2 years ago

drahkrub commented 3 years ago

spring-boot 2.5.4

I don't know if this is a bug - if it's not, then it's a feature request:

If some interface method is annotated with @PreAuthorize and a bean implementing the interface is generated with @MockBean then the @PreAuthorize annotation does not have any effect. In contrary the @PreAuthorize annotation works as expected if the implementing bean is created with Mockito.mock(...).

@SpringBootTest
class MockTests {

    @Autowired
    Service service;

    @Autowired
    Service mockBeanService;

    @Test
    @WithMockUser(authorities = "ADMIN")
    void succeeds() {
        service.doit();
    }

    @Test
    @WithMockUser
    void succeedsAsExceptionIsThrown() {
        Assertions.assertThrows(AccessDeniedException.class, () -> {
            service.doit();
        });
    }

    @Test
    @WithMockUser
    void failsAsNoExceptionIsThrown() {
        Assertions.assertThrows(AccessDeniedException.class, () -> {
            mockBeanService.doit();
        });
    }

    @Configuration
    @EnableGlobalMethodSecurity(prePostEnabled = true)
    static class Config {

        @MockBean(name = "mockBeanService")
        Service mockBeanService;

        @Bean(name = "service")
        Service service() {
            return Mockito.mock(Service.class);
        }
    }

    static interface Service {

        @PreAuthorize("hasAuthority('ADMIN')")
        void doit();
    }
}
drahkrub commented 3 years ago

Just found https://github.com/spring-projects/spring-boot/issues/7243 - is it mentioned in the docs that beans mocked with @MockBean are intentionally unproxied?

wilkinsona commented 3 years ago

is it mentioned in the docs that beans mocked with @MockBean are intentionally unproxied?

I'm not sure that it is. We could use this issue to add something if we feel that your use case is sufficiently common to warrant mentioning in the reference documentation.

To that end, why are you using @MockBean in this case? It doesn't seem to be necessary based on what you've shared thus far. Don't you want to test that access is denied to your concrete Service implementation rather than a mock derived from the Service interface? It would theoretically be possible for the latter to be denied while the former's allowed, lulling you into a false sense of security.

drahkrub commented 3 years ago

is it mentioned in the docs that beans mocked with @MockBean are intentionally unproxied?

I'm not sure that it is. We could use this issue to add something if we feel that your use case is sufficiently common to warrant mentioning in the reference documentation.

At least I spent quite a long time trying to figure out why my test wasn't working as expected. Thought at first I had a bug in the setup.

To that end, why are you using @MockBean in this case?

Cause it's more comfortable (shorter). ;-)

[...] It would theoretically be possible for the latter (the mocked service) to be denied while the former (the concrete implementation) is allowed, lulling you into a false sense of security.

How should this be possible? The method in the interface is annotated with @PreAuthorize therefore if accessing the mocked service is denied, the access of the concrete implementation must be denied too. Or am I wrong?

drahkrub commented 3 years ago

(in my real project I'm doing integration tests for several controllers which utilize several services implementing interfaces whose methods are annotated with @PreAuthorize and so on)

wilkinsona commented 3 years ago

How should this be possible? The method in the interface is annotated with @PreAuthorize therefore if accessing the mocked service is denied, the access of the concrete implementation must be denied too. Or am I wrong?

It depends on how the mock is generated, what it does with annotations on interface methods that it implements, and how that compares to what you've done with your own implementation. If they're not the same and the former means that Spring Security finds the annotation while the latter does not, your test will pass while your actual service isn't secured.

For something as important as security, I would avoid using mocks (and perhaps even spies) so that you're testing the actual implementation and know it's secured as required.

drahkrub commented 3 years ago

It depends on how the mock is generated, what it does with annotations on interface methods that it implements, and how that compares to what you've done with your own implementation. If they're not the same and the former means that Spring Security finds the annotation while the latter does not, your test will pass while your actual service isn't secured.

Hm, that's interesting. My implementation "does nothing" with the @PreAuthorize annotation, I simply rely on Spring Security. And it would be nice if any implementation, especially mocked ones, behave the same wrt this annotation. Or the other way round, Spring Security should behave the same wrt to any implementation of interfaces annotated with @PreAuthorize. Anything else I would consider as quite strange (but I would like to see an example).

For something as important as security, I would avoid using mocks (and perhaps even spies) so that you're testing the actual implementation and know it's secured as required.

Ok, that's a point.

wilkinsona commented 3 years ago

My implementation "does nothing" with the @PreAuthorize annotation, I simply rely on Spring Security. And it would be nice if any implementation, especially mocked ones, behave the same wrt this annotation.

It (almost certainly) will. However, there have been issues in the past around "inheriting" annotations from interface methods. I have no idea what Mockito does when it mocks an annotated method. Does it, for example, also annotate the implementing method? I'd rather not have to know. To do that safely, testing the actual implementation is your best option, IMO.

wilkinsona commented 2 years ago

This doesn't seem to have affected many other people so I don't think we should do anything at this time. If it becomes apparent that it is causing more widespread problems, we can consider some updates to the documentation.