Skip to content

Migrate to AuthorizationFilter in Spring Security auto-config #31255

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.

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

Conversation

vpavic
Copy link
Contributor

@vpavic vpavic commented Jun 6, 2022

This commit updates Servlet based Spring Security auto-configuration to use AuthorizationFilter, which is intended to superseed FilterSecurityInterceptor.

See note in Authorize HttpServletRequests with AuthorizationFilter section of Spring Security's reference manual.

Note that SampleActuatorCustomSecurityApplicationTests#testInsecureApplicationPath fails after migrating to the new authorization model, meaning further changes might be needed in either Spring Boot or Spring Security.

@vpavic
Copy link
Contributor Author

vpavic commented Jun 7, 2022

This is effectively blocked on the following issue in Spring Security:

I'll keep the PR in draft state until there's some update.

@wilkinsona
Copy link
Member

Thanks, @vpavic. I've subscribed to spring-projects/spring-security#11337. We can take a more in-depth look at this once the Security team have taken a look. Josh should be back in the office next week.

@wilkinsona wilkinsona added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Jun 7, 2022
@wilkinsona wilkinsona added this to the 3.0.x milestone Jun 7, 2022
@wilkinsona wilkinsona added the status: blocked An issue that's blocked on an external project change label Jun 7, 2022
@wilkinsona
Copy link
Member

spring-projects/spring-security#11360 is now tracking some additions to Spring Security that close some gaps in the functionality offered by AuthorizationFilter.

@vpavic vpavic force-pushed the improve-authorization-config branch from 62d5ee5 to 8a33a2a Compare July 15, 2022 08:22
@vpavic vpavic marked this pull request as ready for review July 15, 2022 08:23
@vpavic
Copy link
Contributor Author

vpavic commented Jul 15, 2022

With spring-projects/spring-security#11360 resolved, this is now ready for review but at the same time apparently blocked by #31703.

@wilkinsona, note that I had to undo some of your changes from 4bd3534 as there's no #filterSecurityInterceptorOncePerRequest available on the new authorizeHttpRequests DSL. Judging by where you had to apply those, I'm now seeing the same test failures as you did when building this PR:

Found test failures in 2 test tasks:

:spring-boot-tests:spring-boot-smoke-tests:spring-boot-smoke-test-web-method-security:test
    smoketest.security.method.SampleMethodSecurityApplicationTests > testManagementProtected()

:spring-boot-tests:spring-boot-smoke-tests:spring-boot-smoke-test-web-secure:test
    smoketest.web.secure.CustomContextPathErrorPageTests > testPublicNotFoundPage()
    smoketest.web.secure.CustomServletPathErrorPageTests > testPublicNotFoundPage()
    smoketest.web.secure.ErrorPageTests > testPublicNotFoundPage()
    smoketest.web.secure.NoSessionErrorPageTests > testCorrectCredentialsWithControllerException()
    smoketest.web.secure.NoSessionErrorPageTests > testPublicNotFoundPage()
    smoketest.web.secure.NoSessionErrorPageTests > testPublicNotFoundPageWithCorrectCredentials()

@wilkinsona
Copy link
Member

Thanks, @vpavic. We'll have a chat with the Security team.

@vpavic vpavic force-pushed the improve-authorization-config branch from 8a33a2a to fb25437 Compare July 21, 2022 17:51
@vpavic
Copy link
Contributor Author

vpavic commented Jul 21, 2022

@wilkinsona I believe this should now be ready meaning I don't think my observation about this being blocked by #31703 was valid.

I've taken a closer look at FilterSecurityInterceptor vs AuthorizationFilter and the latter should have once per request semantics by default as it extends OncePerRequestFilter. However it also filters all dispatch types by default, which I disabled (note the second commit). That uncovered one issue with tests but now everything should build cleanly.

@vpavic
Copy link
Contributor Author

vpavic commented Jul 26, 2022

The FilterSecurityInterceptor vs AuthorizationFilter remarks from the previous comment have been confirmed by the Security team in spring-projects/spring-security#11337 (comment).

@wilkinsona wilkinsona removed the status: blocked An issue that's blocked on an external project change label Jul 27, 2022
@wilkinsona
Copy link
Member

Great stuff. Thanks, @vpavic.

@vpavic
Copy link
Contributor Author

vpavic commented Sep 22, 2022

Is there anything preventing this from making it into today's 3.0.0-M5 release?

@wilkinsona
Copy link
Member

Nothing other than the team being short on time. We'll see what we can do.

@vpavic vpavic force-pushed the improve-authorization-config branch from fb25437 to 217302b Compare September 22, 2022 09:05
@vpavic
Copy link
Contributor Author

vpavic commented Sep 22, 2022

Got it.

I updated the PR to pick up the current main.

Update: I just spotted a couple of new usages of old authorization DSL that have emerged since this PR has last been touched.

This commit updates Servlet based Spring Security auto-configuration to use AuthorizationFilter, which is intended to superseed FilterSecurityInterceptor.
@vpavic vpavic force-pushed the improve-authorization-config branch from 217302b to b3c066f Compare September 22, 2022 09:18
@wilkinsona wilkinsona modified the milestones: 3.0.x, 3.0.0-M5 Sep 22, 2022
wilkinsona pushed a commit that referenced this pull request Sep 22, 2022
This commit updates Servlet based Spring Security auto-configuration
to use AuthorizationFilter, which is intended to supersede
FilterSecurityInterceptor.

See gh-31255
@wilkinsona
Copy link
Member

Thanks very much, @vpavic.

@vpavic vpavic deleted the improve-authorization-config branch September 22, 2022 13:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants