Skip to content

Conversation

@heruan
Copy link
Member

@heruan heruan commented Jun 6, 2025

This fixes #21401 by providing a SecurityContextHolderStrategy bean as part of Spring Security auto-configuration and replaces static invocations of SecurityContextHolder.getContext() by using the strategy bean instead.

  • Provide SecurityContextHolderStrategy in SpringSecurityAutoConfiguration
  • Remove conflicting VaadinAwareSecurityContextHolderStrategyConfiguration
  • Set the strategy on filters during VaadinSecurityConfigurer build lifecycle
  • Set the strategy statically when using VaadinWebSecurity for backwards compatibility
  • Inject the strategy bean in AuthenticationContext and SpringAccessPathChecker
  • Deprecate constructors that obtain the strategy statically
  • Avoid static access in AuthenticationUtil methods
  • Update tests

Breaking changes

  • VaadinAwareSecurityContextHolderStrategyConfiguration has been removed — mild since it was purely for internal use
  • SpringSecurityAutoConfiguration::accessPatchChecker signature has changed to include the strategy parameter — mild since this class shouldn't be extended (better have package-private bean methods)
  • Applications that have set a custom strategy statically after VaadinAwareSecurityContextHolderStrategyConfiguration might expect that custom strategy to be used by Flow, instead of the bean — those apps should now provide the custom strategy as a bean (if they expect Flow to use it)

DRAFT Tests setting the strategy statically must be updated (some already are)

@heruan heruan self-assigned this Jun 6, 2025
@github-actions
Copy link

github-actions bot commented Jun 6, 2025

Test Results

1 281 files  ± 0  1 281 suites  ±0   1h 17m 14s ⏱️ -1s
8 877 tests + 1  8 810 ✅ + 1  67 💤 ±0  0 ❌ ±0 
9 322 runs   - 12  9 247 ✅  - 10  75 💤  - 2  0 ❌ ±0 

Results for commit 0e328e8. ± Comparison against base commit ee5139e.

♻️ This comment has been updated with latest results.

@mshabarov mshabarov requested a review from mcollovati June 9, 2025 11:35
@mcollovati mcollovati force-pushed the feat/security-context-strategy-bean branch from 95f2ac4 to 06f63c2 Compare June 17, 2025 06:17
@sonarqubecloud
Copy link

* @param evaluator
* evaluator to check path permissions.
* @deprecated Use
* {@link #SpringAccessPathChecker(WebInvocationPrivilegeEvaluator, String)}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
* {@link #SpringAccessPathChecker(WebInvocationPrivilegeEvaluator, String)}
* {@link #SpringAccessPathChecker(SecurityContextHolderStrategy, WebInvocationPrivilegeEvaluator)}

Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be good to add a note mentioning usage of SecurityContextHolder#getContextHolderStrategy() in the deprecated constructors to explain the deprecation (similar to AuthenticationContext)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if we should just remove the @Configuration annotation and deprecate the class for 24.8

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that would be better. I initially removed the class to see what failed without it, but we can keep it and deprecate before complete removal.

@mcollovati mcollovati force-pushed the feat/security-context-strategy-bean branch from 06f63c2 to 5eb99f8 Compare October 6, 2025 08:38
@mcollovati mcollovati changed the title Obtain SecurityContext from the SecurityContextHolderStrategy bean refactor: Obtain SecurityContext from the SecurityContextHolderStrategy bean Oct 8, 2025
@mcollovati mcollovati force-pushed the feat/security-context-strategy-bean branch from 9d38154 to 4d566a5 Compare October 8, 2025 05:49
@mcollovati
Copy link
Collaborator

One missing part: restore and deprecate VaadinAwareSecurityContextHolderStrategyConfiguration
This PR can be revised after VaadinWebSecurity removal gets merged.

@CLAassistant
Copy link

CLAassistant commented Oct 22, 2025

CLA assistant check
All committers have signed the CLA.

@sonarqubecloud
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

Status: 🔎Iteration reviews

Development

Successfully merging this pull request may close these issues.

Refactor Spring utilities to avoid static access to SecurityContextHolder

5 participants