Allow non-AuthorizationDecision results in WebSecurity#19284
Open
seonwooj0810 wants to merge 1 commit into
Open
Allow non-AuthorizationDecision results in WebSecurity#19284seonwooj0810 wants to merge 1 commit into
seonwooj0810 wants to merge 1 commit into
Conversation
WebSecurity#addAuthorizationManager registered a lambda that cast the AuthorizationManager#authorize result to AuthorizationDecision when forwarding the call to the privilege evaluator. The cast was added when migrating away from the deprecated AuthorizationManager#check method but narrowed AuthorizationResult to AuthorizationDecision. With multi-factor authentication, the manager produced by AuthorizationManagerFactories returns a FactorAuthorizationDecision, which implements AuthorizationResult directly and is not assignable to AuthorizationDecision, so WebInvocationPrivilegeEvaluator#isAllowed failed with ClassCastException for any URL guarded by a multi-factor manager. The lambda is registered through Builder#add as an AuthorizationManager whose authorize method already returns AuthorizationResult, so the cast is unnecessary as well as unsound. Closes spring-projectsgh-19282 Signed-off-by: seonwoo_jung <laborlawseon@kap.kr>
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-19282
Root cause
WebSecurity#addAuthorizationManagerregisters, for everyAuthorizationFilterin aSecurityFilterChain, a lambda that delegates to the filter'sAuthorizationManager#authorizeso thatWebInvocationPrivilegeEvaluatorcan use the same authorization rules. The lambda casts the returnedAuthorizationResulttoAuthorizationDecision:The cast was added in
0ab01eac1while migrating from the deprecatedAuthorizationManager#check(which returnedAuthorizationDecision) to#authorize(which returns the broaderAuthorizationResult). The narrowing was carried over unconditionally, which violates the new return-type contract for anyAuthorizationResultthat is not anAuthorizationDecision.In Spring Security 7.0 this is reachable through the multi-factor authorization manager produced by
AuthorizationManagerFactories. Its underlyingAllRequiredFactorsAuthorizationManager#authorizereturns aFactorAuthorizationDecision, whichimplements AuthorizationResultdirectly and is a sibling ofAuthorizationDecision, not a subtype. The stack trace in the issue ends in this lambda:The cast is also unnecessary:
RequestMatcherDelegatingAuthorizationManager.Builder#addaccepts anAuthorizationManager<? super RequestAuthorizationContext>, whoseauthorizealready returnsAuthorizationResult.Change
Remove the cast. The lambda now returns the
AuthorizationResultdirectly, matching the contract ofAuthorizationManager#authorizeand the type expected byBuilder#add.Tests
Added
WebSecurityTests.privilegeEvaluatorWhenAuthorizationManagerReturnsFactorDecisionThenIsAllowedReturnsFalseWithoutClassCastException. It registers aSecurityFilterChainwhose authorization manager returns aFactorAuthorizationDecisionand asserts thatWebInvocationPrivilegeEvaluator#isAllowedreportsfalsewithout throwing. With the cast still present, this test fails withClassCastException at WebSecurityTests.java:131; with the cast removed it passes../gradlew :spring-security-config:test --tests 'org.springframework.security.config.annotation.web.builders.WebSecurityTests'is green, and./gradlew :spring-security-config:checkstyleMain :spring-security-config:checkstyleTestreports no violations.Verification done: (1)
gh pr list --search "FactorAuthorizationDecision in:title,body"returns no in-flight PRs; (2) the linked issue has zero comments and no self-claims; (3) the fix is in.javafiles only; (4) the buggy cast is present onorigin/mainatWebSecurity.java:383; (5) the issue is a stand-alone bug report, not a sub-issue of an umbrella; (6) the stack trace in the issue terminates inside the framework lambda this PR rewrites.