Print ignore message DefaultSecurityFilterChain

[manueljordan]

When either web.ignoring().mvcMatchers(...) or
web.ignoring().antMatchers(...) methods are used, for all their
variations, the DefaultSecurityFilterChain class now indicates
correctly through its ouput what paths are ignored according the
ignoring() settings.

Closes gh-9334

[jzheaux]

Thanks for the PR, @manueljordan!

I'm hesitant to add this much code to support logging. In this PR, we should not:

1. Increase the visibility of existing APIs
2. Introduce new interfaces

I've left some additional feedback inline.

[jzheaux]

Maybe we could start with something simpler like this:

if (filters.isEmpty()) {
    logger.info(LogMessage.format("Will not secure %s", requestMatcher));
} else {
    logger.info(LogMessage.format("Will secure %s with %s", requestMatcher, filters));
}

If Spring Security has no filters assigned to a given request matcher, then it's true that Spring Security will not be securing that request.

[jzheaux]

This warning message may need to wait as there are numerous other scenarios to cover in order for that to be complete, e.g. regexMatchers, anyRequest(), and requestMatcher(AnyRequestMatcher.INSTANCE). It may be best to begin with the simplest case and do:

@Override
public IgnoredRequestConfigurer anyRequest() {
	WebSecurity.this.logger.warn("Applying explicit instruction to ignore all paths");
	WebSecurity.this.logger.warn("This disables Spring Security and is not recommended");
	return super.anyRequest();
}

This aligns with other configured code that guards usage of anyRequest() but does not also attempt to parse strings to discern the intent of the application. Later on, it may be possible for the code to be more intelligent without committing to an increased public API.

[manueljordan]

Hello @jzheaux

Thanks for the quick reply

Increase the visibility of existing APIs

If I do that it breaks the DRY principle. The changes of visibility applies mostly for utils methods (static methods), here the situation is about the nested static class, it is tricky, I thought to move it to an outer/external class but I could be more problematic

Introduce new interfaces

I thought to work based on hierarchy (subclass), but since I did realize it applies only to 2 subclasses of the whole tree hierarchy, I took that decision, it opens the same option to ignore something if something new appears in the future, such as: somethingMatchers()

About

if (filters.isEmpty()) {
    logger.info(LogMessage.format("Will not secure %s", requestMatcher));
} else {
    logger.info(LogMessage.format("Will secure %s with %s", requestMatcher, filters));
}

I thought the same, but - if my memory does not fail me in some scenarios the filters appears empty and not empty (yes, for ignoring()), so that approach was discarded, so the key is work with the RequestMatcher type, where now two of them are of the IgnoreRequestMatcher type.

This warning message may need to wait as there are numerous other scenarios to cover in order for that to be complete, e.g. regexMatchers, anyRequest(), and requestMatcher(AnyRequestMatcher.INSTANCE). It may be best to begin with the simplest case and do:

That working scenario only applies to web.ignoring().mvcMatchers(...) and web.ignoring().antMatchers(...), if my memory does not fail me web.ignoring().X applies only to mvcMatchers(...) and antMatchers(...). Furthermore I confirmed through the @Test methods that configure(WebSecurity) overrides by complete the configure(HttpSecurity) settings, so that warning message is important.

For the other methods mentioned, they do not have a common interface to identity quickly if is for an ignore scenario or not. It is the problem from the beginning, there is no a quick way to know if a URL (path) is for ignoring or not.

I am open to follow your guidance, you are the expert here.

Consider remember, I am starting to play with this API practically from the scratch, and your vision is bigger than my current knowledge, but I explained the experiences and the decisions taken according each case

[jzheaux]

@manueljordan, sorry for the delay. I'm getting back to this PR now.

If I do that it breaks the DRY principle.

Either that, or we need to simplify what we are trying to do. If we are changing the public API in this PR, we are probably trying to go too far too fast.

I thought the same, but - if my memory does not fail me in some scenarios the filters appears empty and not empty

Either way, if there are no filters for a given request matcher, then requests that match are being effectively ignored.

[manueljordan]

Hello @jzheaux

Either that, or we need to simplify what we are trying to do. If we are changing the public API in this PR, we are probably trying to go too far too fast.

What do you suggest do here? You are the expert here - and I always take your suggestions friendly. I hope you take in the same way my thoughts. But according with my research in fix this, fall in break the DRY would be an option but you know ... break something like that is not wise ... it seems you want that I should split my current PR in more PRs covering each case by separate. Am I correct?

Either way, if there are no filters for a given request matcher, then requests that match are being effectively ignored.

Your approach even if is valid - with my PR I am giving an important warning to the developer about security constraints that are being overriding and therefore exposing private things. Do you want split that warning messages how other PR? - something I like wrote in the previous sentence...

Let me know your thoughts

[jzheaux]

I always take your suggestions friendly

Thanks! The feeling is mutual -- I appreciate the effort you are putting in here.

What do you suggest here?

I'd prefer we go with the comments I made in my review.

with my PR I am giving an important warning to the developer

Agreed. I think it's reasonable to add an even stronger message:

// ...
for (RequestMatcher ignoredRequest : this.ignoredRequests) {
    WebSecurity.this.logger.warn("You are asking Spring Security to ignore " + matcher + ". This is not recommended -- please use permitAll via HttpSecurity#authorizeHttpRequests instead.");
    securityFilterChains.add(new DefaultSecurityFilterChain(ignoredRequest));
}
// ...

My point is not that we shouldn't warn, but that this PR should not introduce so much infra to facilitate logging. We should be able to add some sensible warnings without copy-pasting and without affecting the public API.

[jzheaux]

@manueljordan Do you feel like you can make the changes requested? If not, no worries, I can add a polish commit.

[manueljordan]

Due lack of time, please do it. Thanks for your understanding

[jzheaux]

This is now merged into 5.7.x via 01ed617 and as well as the discussed polish commit cbd87fa. The main thrust of the polish commit is to still alert the user to this bad practice while leaving the public API intact.