Add getName to SecurityFilterChain for trace logging#19162
Open
LivingLikeKrillin wants to merge 6 commits into
Open
Add getName to SecurityFilterChain for trace logging#19162LivingLikeKrillin wants to merge 6 commits into
LivingLikeKrillin wants to merge 6 commits into
Conversation
Closes spring-projectsgh-6274 Signed-off-by: Jooyoung Jung <143606756+LivingLikeKrillin@users.noreply.github.com>
Returns the bean name captured via existing BeanNameAware integration. Issue spring-projectsgh-6274 Signed-off-by: Jooyoung Jung <143606756+LivingLikeKrillin@users.noreply.github.com>
No behavior change. Adds a private matchChain(HttpServletRequest) that returns the matched SecurityFilterChain rather than just its filters. The existing private getFilters(HttpServletRequest) (reflectively invoked by WebTestUtils.findFilter) is preserved and now delegates to matchChain. Issue spring-projectsgh-6274 Signed-off-by: Jooyoung Jung <143606756+LivingLikeKrillin@users.noreply.github.com>
Appends '[<chainName>]' suffix to the 'Securing' and 'Secured' DEBUG log lines when the matched chain's getName() returns a non-null value. The suffix is omitted when getName() returns null, preserving the existing log format for unnamed chains. Adds ch.qos.logback:logback-classic as a testImplementation dependency so FilterChainProxyTests can use logback's ListAppender to assert the captured log output. Issue spring-projectsgh-6274 Signed-off-by: Jooyoung Jung <143606756+LivingLikeKrillin@users.noreply.github.com>
Locks down the reflective contract that WebTestUtils relies on (invoking private FilterChainProxy.getFilters(HttpServletRequest) via ReflectionTestUtils). Prevents accidental rename in future refactors. Issue spring-projectsgh-6274 Signed-off-by: Jooyoung Jung <143606756+LivingLikeKrillin@users.noreply.github.com>
…rChainProxy Locks the reflective contract that WebTestUtils.findFilter relies on (invoking the package-private subclass's private getFilters method via ReflectionTestUtils). Mirrors the FilterChainProxy regression test at the subclass level. Bypasses WebTestUtils.findFilter directly because that method is package-private and CompositeFilterChainProxy lives in a different package. Issue spring-projectsgh-6274 Signed-off-by: Jooyoung Jung <143606756+LivingLikeKrillin@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes gh-6274
Background
This issue has had two prior attempts at resolution:
DefaultSecurityFilterChain) — closed, not mergedgetId()onRequestMatcher) — closed, not mergedSince then, in commit f46e56d (gh-15874),
BeanNameAwarewas added toDefaultSecurityFilterChainto improve error messages. The bean name is nowcaptured and surfaced through
toString(), but theSecurityFilterChaininterface itself still has no name accessor, so consumers like
FilterChainProxycannot include the matched chain identity in"Securing/Secured" debug logs without
instanceofchecks.Change
default @Nullable String getName()toSecurityFilterChain(returns
nullby default — fully backward compatible).DefaultSecurityFilterChainto return the bean namecaptured by the existing
BeanNameAwareintegration.FilterChainProxy.doFilterInternalto include[<name>]inDEBUG logs when a name is available; omit when null.
Commit-by-commit reviewability
The PR is split into six small commits so each can be reviewed in isolation:
Add getName default to SecurityFilterChain— interface change onlyOverride getName in DefaultSecurityFilterChain— bean-name overrideExtract matchChain helper in FilterChainProxy— pure refactor, no behavior changeInclude chain name in FilterChainProxy DEBUG log— the visible behavioral changeAdd WebTestUtils.findFilter regression test for FilterChainProxyAdd getFilters(HttpServletRequest) regression test for CompositeFilterChainProxyHappy to squash if the team prefers a single commit, but kept separate so the
"no behavior change" refactor (3) can be verified independently from the
behavioral change (4).
Preservation of the reflective
getFilters(HttpServletRequest)contractThe existing private
FilterChainProxy.getFilters(HttpServletRequest)ispreserved verbatim (name and signature). It is invoked reflectively by
WebTestUtils.findFilteratWebTestUtils.java:158viaReflectionTestUtils.invokeMethod(springSecurityFilterChain, "getFilters", request),and an analogous private method exists on
CompositeFilterChainProxyfor thesame reason. The new
matchChainhelper is added alongside it, andgetFilters(HttpServletRequest)now delegates tomatchChain. Two regressiontests lock this contract for both
FilterChainProxyandCompositeFilterChainProxy.Why
@Nullable Stringrather thanOptional<String>Return type is
@Nullable Stringrather thanOptional<String>forconsistency with the existing
@Nullable beanNamefield onDefaultSecurityFilterChain, and following Brian Goetz's guidance thatOptionalis awkward as a property-style return type. The new method'sJavadoc explicitly marks the returned value as for diagnostic use only.
Out of scope (deliberately)
BeanNameAwarecovers the common path;custom implementations can return their own identity by overriding
getName().SecurityWebFilterChainlacksBeanNameAwaregroundwork (no equivalent of
f46e56de78on the reactive side yet), sothat change is meaningfully different. If this lands, I will open a
separate issue for
SecurityWebFilterChainparity.DebugFilterorDefaultFilterChainValidator. Thoseconsumers can adopt
getName()in follow-up PRs once the interfacechange lands.
Alternative considered
If the team prefers to avoid an interface change, the same DEBUG output
can be achieved via an
instanceof DefaultSecurityFilterChaincast inFilterChainProxy. Happy to switch to that diff if a cast-based approachis preferred.
Testing
SecurityFilterChainTests— interface default returns null.DefaultSecurityFilterChainTests—getName()returns the bean name whenset; null otherwise.
FilterChainProxyTests— DEBUG log includes[<name>]suffix when thematched chain has a name; suffix omitted when null. Uses logback
ListAppenderfor log capture.WebTestUtilsTests— regression test for the reflective contract onplain
FilterChainProxy.CompositeFilterChainProxyTests— regression test for the same contracton
WebSecurityConfiguration.CompositeFilterChainProxy(the package-privatecomposite path used by Spring Boot annotation-based config).
Notes
getName()inFilterChainProxy, consistent with howmatches()andgetFilters()are already invoked. Diagnostics shouldnot silently swallow exceptions.
startsWith("Securing"))are unaffected. End-anchored regexes (
^Securing .* /\S+$) may needadjustment.