adding custom filters to FilterOrderRegistration #9832
Conversation
|
@theexiile1305 Please sign the Contributor License Agreement! Click here to manually synchronize the status of this Pull Request. See the FAQ for frequently asked questions. |
|
@theexiile1305 Thank you for signing the Contributor License Agreement! |
|
Hi @theexiile1305, I don't work for pivotal and personally I want to see this issue resolved asap, but I suspect they are going to want a couple of tests as well? |
|
Hi @Stexxen, thank you for the reply. Yea, I give it a try to add some tests 😄 |
spring-projects-issues
added
the
status: waiting-for-triage
.../src/main/java/org/springframework/security/config/annotation/web/builders/HttpSecurity.java
Show resolved
Hide resolved
jzheaux
added
in: web
…AtOffsetOf(Filter,int,Class<? extends Filter>)
…,Class<? extends Filter>)
|
@rwinch Hey there 😃 |
There was a problem hiding this comment.
Thanks for the PR! I've provided feedback inline.
You can add tests to HttpSecurityAddFilterTest. Please be sure to include a test for the original issue and to cover the scenarios I mentioned inline.
| void add(Class<? extends Filter> filter) { | ||
| int givenOrder = getOrder(filter); | ||
| Step step = new Step(givenOrder, ORDER_STEP); | ||
| int nextOrder = step.next(); |
There was a problem hiding this comment.
We don't want to register it at order + ORDER_STEP. If we do this, then registering MyFilter after BasicAuthenticationFilter means that MyFilter is registered in the same spot as RequestCacheAwareFilter which is not what we want.
Instead, MyFilter should end up just before or just after (depending on the offset) the order.
If MyFilter is added in two different slots as is done in gh-9633, then we should fail if a user tries to use MyFilter as reference point. We should also ensure that none of the predefined Filter registrations can be overridden by accident.
NOTE: The reason for the large step is so users have space to insert custom Filters between the predefined Filters.
| * structure of the FilterOrderRegistration. | ||
| * @param filter the {@link Filter} class that should be added | ||
| */ | ||
| void add(Class<? extends Filter> filter) { |
There was a problem hiding this comment.
I'd prefer we refactor this so that using this API does not allow us to get things wrong. Instead of two independent methods, I think we should remove this method and change getOrder to a private method and add findOrder(Class<? extends Filter> filter) which performs this logic and returns the current (or added) order.
| @@ -2664,6 +2665,7 @@ public HttpSecurity addFilter(Filter filter) { | |||
| + " does not have a registered order and cannot be added without a specified order. Consider using addFilterBefore or addFilterAfter instead."); | |||
| } | |||
| this.filters.add(new OrderedFilter(filter, order)); | |||
| this.filterOrders.add(filter.getClass()); | |||
There was a problem hiding this comment.
This is not required because we know the Filter was already added (otherwise order would be null).
|
I am going to work on the PR and clean it up based on @rwinch suggestions. |
|
I created this PR #9898 which was closed for being a "duplicate". It contains unit tests, solves the bug and code modification is really small. Feel free to copy / paste from there if it helps to get this solved quickly, I don't care about having credits but do care this bug is solved as it is a show stopper for me to migrate to spring-boot 2.5 |
|
@theexiile1305 We haven't received any updates based on @rwinch feedback so we went ahead with the changes. I'll close this in favour of gh-9902. |
jgrandja
removed
in: web
jgrandja
added
status: declined
This pull request let custom filters to be added to the FilterOrderRegistration in order to solve the issue mentioned in #9787.
Closes gh-9787