Skip to content

adding custom filters to FilterOrderRegistration #9832

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.

Sign up for GitHub

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

Closed
wants to merge 4 commits into from
Closed

adding custom filters to FilterOrderRegistration #9832

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3bb7745
adding custom filters to FilterOrderRegistration Closes gh-9787
theexiile1305 May 28, 2021
774463a
updated apache license header
theexiile1305 May 30, 2021
a3eed91
adding custom filter in method addFilter(Filter) instead of addFilter…
theexiile1305 Jun 1, 2021
d4f44c6
adding fix for custom filter in method addFilterAtOffsetOf(Filter,int…
theexiile1305 Jun 1, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion .../org/springframework/security/config/annotation/web/builders/FilterOrderRegistration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2002-2020 the original author or authors.
* Copyright 2002-2021 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -114,6 +114,18 @@ final class FilterOrderRegistration {
put(SwitchUserFilter.class, order.next());
}

/**
* Add a particular {@link Filter} class to the latest position to the internal data
* structure of the FilterOrderRegistration.
* @param filter the {@link Filter} class that should be added
*/
void add(Class<? extends Filter> filter) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

int givenOrder = getOrder(filter);
Step step = new Step(givenOrder, ORDER_STEP);
int nextOrder = step.next();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

put(filter, nextOrder);
}

private void put(Class<? extends Filter> filter, int position) {
String className = filter.getName();
this.filterToOrder.put(className, position);
Expand Down
4 changes: 3 additions & 1 deletion ...c/main/java/org/springframework/security/config/annotation/web/builders/HttpSecurity.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2002-2020 the original author or authors.
* Copyright 2002-2021 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -2652,6 +2652,7 @@ public HttpSecurity addFilterBefore(Filter filter, Class<? extends Filter> befor

private HttpSecurity addFilterAtOffsetOf(Filter filter, int offset, Class<? extends Filter> registeredFilter) {
int order = this.filterOrders.getOrder(registeredFilter) + offset;
this.filterOrders.add(filter.getClass());
this.filters.add(new OrderedFilter(filter, order));
return this;
}
Expand All @@ -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());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not required because we know the Filter was already added (otherwise order would be null).

return this;
}

Expand Down