spring-projects / spring-boot

Spring Boot
https://spring.io/projects/spring-boot
Apache License 2.0
75.01k stars 40.65k forks source link

Add an annotation to exclude Filter @Beans from registration #16500

Open OrangeDog opened 5 years ago

OrangeDog commented 5 years ago

Sometimes you want to declare Filters as @Beans in order to have them managed and dependencies injected etc. but do not want them to be automatically registered. This can be a problem particularly when working with Spring Security. Though this will apply in any situation with multiple filter chains.

Currently you have to declare a FilterRegistrationBean for every filter with setEnabled(false), which can get very tedious and can be a maintenance problem.

I propose a new annotation-based method to exempt filter beans from autoregistration. For maximum utility, perhaps by implementing FilterRegistrationBean (or just RegistrationBean) wholly as an annotation?

@Bean
@FilterRegistration(enabled=false)
public Filter manualFilter() { ... }

@Bean
@FilterRegistration(order=200, urlPatterns={"/path/**"})
public Filter automaticFilter() { ... }

Related: #2173

philwebb commented 5 years ago

Having a dedicated annotation just to disable the bean wouldn't be great, but I quite like the idea of offering all the FilterRegistrationBean properties in the annotation. I could see that being quite useful. I do wonder how many people are registering custom filters.

@OrangeDog how many filters are you typically registering in your application? I'm trying to think about how we balance the amount of boilerplate needed with FilterRegistrationBean vs the additional overhead of introducing a new annotation for people to learn.

OrangeDog commented 5 years ago

Right now just configuring spring-security-saml2-core involves nine filters, all of which are supposed to be beans, and none of which should go in the default filter chain. That's ~60 lines of FilterRegistrationBean declarations in order to turn them off.

I'm likely to add a couple more security filters at some point for additional authentication methods, e.g. persistent API tokens.

vpavic commented 5 years ago

I've also faced this situation in a few projects, most of the times in the very same scenario as @OrangeDog's:

Having FilterRegistrationBean properties exposed in annotation would be very helpful there. This shouldn't be an uncommon use case.

drtrang commented 5 years ago

We have a messy dependencies, so filters by automatically registery is a risk for us, because it's not under control with filter list (they are register to application context already).

So we skipped all of subclasses for javax.servlet.Filter when scanning packages, use custom TypeFilter, but i don't think it's a good idea.

Is there a better way to resolve this?

raffig commented 2 years ago

I've run on this issue today, after 4 hrs of research. Honestly, I spent some time on thinking about designed solution (instead of those hacks with disabled FilterRegistrationBean that you have to unwrap in place where the filter is needed) and I came to following conculsion:

Implementors of Filter should not be registered automatically at all.

Why? Well, since we have a FilterRegistrationBean then everyone can use it to register filter in main container chain on purpose. Other Filters should be ignored, because they may be used as part of some subchains (like Spring Security filter chain) etc. Currently automatic registration of all Filters causes pretty random setup. There is a very little control over things like order or dispatcher of filter. If an author wants to include filter in basic chain they can create appropriate FilterRegistrationBean. If it is a starter, there could even be some standard mechanism to provide order or dispatcher setup through eg. properties for better control over particular filters. Otherwise authors are encouraged to simply create Filter bean and that often causes problems, because it is completely out of control how the filter is registered.

Yes, I know that is a breaking change, but since Spring Boot 3.0 is coming maybe it is a good moment for such change?

chubbard commented 11 hours ago

Ok so it's been 10 years since this was brought up, and 5 years on discussing an annotation to take the place of FilterRegistrationBean. I need to add some web filters to the normal web filters chain, and I need to add other security filters to Spring Security's chain. These are rather mundane exercises, but under the current status quo it's very very difficult when all Filters are automatically registered, Autoconfiguration, order issues, etc. The current workaround is a verbose hack that should be as easy as just setting up an array of objects.

I do think direct configuration of the jakarta web filter chain would make things much simpler. Annotations to control enable functionality would be a modest improvement, but the problem is you can't see what the chain order is in one place with annotations so while order can be controlled now it's hard to see the full chain all at once. The Spring Security Chain configures that chain directly in the code in one method. I think a similar configuration would be easier to handle setting up the web filter chain when you need to control it.

If a method could be added in a @Configuration that allows you to configure the web filter chain (similar to Spring Security's HttpSecurity class). The default of that injected customizer would be auto registration, but by invoking these methods to configure the overall chain; it would disable auto-registration of filters.

Something like this:

public WebFilterChain webFilterChain( WebFilterChain web, MyFilter1 filter1, MyFilter2 filter2, MyFilter3 filter3, SecurityFilterChain securityChain ) {
   return web.configureChain().addFilters( filter1, filter2, securityChain, filter3 );
}

The default configuration might be the following and it would do the same thing that happens today:

public WebFilterChain webFilterChain( WebFilterChain web ) {
    return web.configureChain().autoRegisterFilters();
}

I think it's time to make a decision on this, and address it because it's a real mess right now.