Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Without AspectJ, using @EnableGlobalMethodSecurity causes JDK proxies to be used by default #25413

Closed
chrylis opened this issue Feb 24, 2021 · 5 comments
Assignees
Labels
type: bug A general bug
Milestone

Comments

@chrylis
Copy link
Contributor

chrylis commented Feb 24, 2021

This is a followon to #16398, which was closed due to lack of reproducability, based on research for spring-projects/spring-framework#26603. In short, interoperability between Spring Security and Spring MVC requires class-based proxying to be in effect, but spring.aop.proxy-target-class does not apply its settings until after the controller beans are already created.

Starting from a blank (Groovy) project from Initializr with Boot 2.4.3, I have these classes:

@SpringBootApplication
@EnableGlobalMethodSecurity(prePostEnabled = true)
class DemoApplication {
  static void main(String[] args) {
    SpringApplication.run(DemoApplication, args)
  }
}
@RestController
class DemoController implements Supplier<String> {
  // Supplier is here to get the infrastructure to decide it can use a JDK proxy

  @RequestMapping('/demo')
  @PreAuthorize('true')
  def demo() {
    'Hello'
  }

  @Override
  String get() {
    demo()
  }
}
@SpringBootTest
@AutoConfigureMockMvc
class DemoApplicationTests {

  @Autowired
  MockMvc mockMvc

  @Test
  @WithMockUser
  void hello() {
    mockMvc.perform(get('http://localhost/hello')).andExpect(status().isOk())
  }
}

and this line in application.properties:

spring.aop.proxy-target-class: true

The auto-config report happily indicates that ClassProxyingConfiguration is active, but the constructor for that class, which applies its rules by side-effect, is not called until after the controller is already instantiated using the then-configuration of "use JDK proxy". This in turn triggers the cascade of looking at the wrong class (the proxy interface) for mappings, failing to find anything, and returning a 404.

The "simple" workaround of @EnableAspectJAutoProxy(proxyTargetClass = true) requires pulling in the whole AspectJ infrastructure instead of using CGLib.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Feb 24, 2021
@mdeinum
Copy link
Contributor

mdeinum commented Feb 25, 2021

The spring.aop.proxy-target-class is applied to the AspectJ infrastructure beans, it isn't applied to the proxy mechanism of @EnableGlobalMethodSecurity. You need to enable that explicitly by setting proxyTargetClass to true of @EnableGlobalMethodSecurity.

Also @EnableAspectJAutoProxy(proxyTargetClass = true) doesn't require you pulling in the whole aspectj stuff, that is already included as that is used to parse the pointcuts etc. Nor do I believe that will solve your issue as that enables class bases proxying for a different part of the AOP mechanism.

The same happens with @EnableAsync and @EnableScheduling (see spring-projects/spring-framework#26535).

So all in all enabling class-based proxies application wide seems (still) to be a struggle.

@wilkinsona
Copy link
Member

wilkinsona commented Feb 25, 2021

This is a follow-on to #16398, which was closed due to lack of reproducibility

#16398 was closed because the user was configuring their own DefaultAdvisorAutoProxyCreator which caused Boot's auto-configuration to back off.

The auto-config report happily indicates that ClassProxyingConfiguration is active, but the constructor for that class, which applies its rules by side-effect, is not called until after the controller is already instantiated

Thanks for the analysis. This, I think, is the crux of the problem. ClassProxyingConfiguration is changing a bean definition at a point where the bean has already been created. It needs to make the change at a point where bean creating hasn't begun. For example, a BeanFactoryPostProcessor would be a better mechanism to do it.

You can work around the problem by defining a BFPP of your own that forces the use of class-based proxies:

@Bean
static BeanFactoryPostProcessor forceAutoProxyCreatorToUseClassProxying() {
	return (beanFactory) -> {
		AopConfigUtils.forceAutoProxyCreatorToUseClassProxying((BeanDefinitionRegistry) beanFactory);
	};
}

With this in place, an app similar to the one described above creates a CGLib proxy for DemoController.

I think we need to make a similar change to AopAutoConfiguration's ClassProxyingConfiguration although I'm a little wary of what that may break. For example, your DemoController could be declared final today and it would work but a change in the proxying configuration would lead to a failure as CGLib proxies cannot be created for final classes. Flagging for team attention so that we can agree on what the fix should be and the branch that it should go in.

@wilkinsona wilkinsona added type: bug A general bug for: team-attention An issue we'd like other members of the team to review and removed status: waiting-for-triage An issue we've not yet triaged labels Feb 25, 2021
@chrylis
Copy link
Contributor Author

chrylis commented Feb 25, 2021

@mdeinum

Both the documentation and code suggest that proxy-target-class should apply to all AOP proxies: The core AOP configuration is set to proxyTargetClass, and if AspectJ is not present, that normally produces CGLib proxies; see AopAutoConfiguration, which explicitly consumes proxy-target-class even without AspectJ.

This is especially surprising because as far as I remember, @EnableAspectJAutoProxy does apply to all of the add-on AOP such as Spring Security and @Transactional; am I mistaken in this? I expect, as a Boot user, that spring.aop.proxy-target-class would produce equivalent behavior.

@EnableAspectJAutoProxy(proxyTargetClass = true) does in fact require AspectJ on the classpath, or subsequent bean instantiation triggers NoClassDefFoundError on AspectJ types.

@chrylis
Copy link
Contributor Author

chrylis commented Feb 25, 2021

@wilkinsona I understand the concern, but that is in fact buggy behavior that some applications might be accidentally relying on. In the case of controllers specifically, interface-based proxies are nearly always going to fail mysteriously anyhow (but of course this doesn't apply to services implementing interfaces generally).

I wonder whether it might be worthwhile as a framework improvement to be able to annotate classes as "always use a class-based proxy for this" so that regardless of global config individual beans could be so instructed.

@mdeinum
Copy link
Contributor

mdeinum commented Mar 1, 2021

Both the documentation and code suggest that proxy-target-class should apply to all AOP proxies: The core AOP configuration is set to proxyTargetClass, and if AspectJ is not present, that normally produces CGLib proxies; see AopAutoConfiguration, which explicitly consumes proxy-target-class even without AspectJ.

AFAIK AspectJ is only used to parse the expressions and it is always cglib that is used to generate the class-based proxies, not AspectJ. I do agree that documentation (sometimes) shows this but not always (as was the case with the @EnableAsync and @EnableAspectJAutoProxy.

This is especially surprising because as far as I remember, @EnableAspectJAutoProxy does apply to all of the add-on AOP such as Spring Security and @Transactional; am I mistaken in this? I expect, as a Boot user, that spring.aop.proxy-target-class would produce equivalent behaviour.

I do agree that I also would expect that when enabling class based proxies that it would be enabled for everything (even without Spring Boot) as mentioned in (spring-projects/spring-framework#26535). But in reality this sometimes doesn't happen and it hasn't for as long as I remember. Which lead to my remark that enabling class-based-aop is in a bit of a sorry state IMHO and fixes need to be done both in Spring and Spring Boot.

@EnableAspectJAutoProxy(proxyTargetClass = true) does in fact require AspectJ on the classpath, or subsequent bean instantiation triggers NoClassDefFoundError on AspectJ types.

It is needed to parse pointcut expressions, and I generally have spring-boot-starter-aop on the classpath which pulls the needed dependencies in already.

@philwebb philwebb added this to the 2.5.x milestone Mar 17, 2021
@philwebb philwebb removed the for: team-attention An issue we'd like other members of the team to review label Mar 17, 2021
@wilkinsona wilkinsona changed the title proxy-target-class applied too late to matter Without AspectJ, using @EnableGlobalMethodSecurity causes JDK proxies to be used by default Mar 31, 2021
@wilkinsona wilkinsona self-assigned this Mar 31, 2021
@wilkinsona wilkinsona modified the milestones: 2.5.x, 2.5.0-RC1 Mar 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug A general bug
Projects
None yet
Development

No branches or pull requests

5 participants