Skip to content
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

HttpSecurityLambdaDsl and ServerHttpSecurityLambdaDsl are overly eager when running against very old Spring Security versions #402

Merged
merged 2 commits into from
Aug 2, 2023

Conversation

shanman190
Copy link
Contributor

Also added/fixed the precondition setup such that the recipe now only runs over JavaSourceFile instances that use the specific HttpSecurity type.

@shanman190
Copy link
Contributor Author

shanman190 commented Aug 2, 2023

Two aspects that I think we should fix in future PRs are:

  • Reliance on JavaType.FullyQualified#getMethods() as this requires reparsing the source code and rerunning the recipe in order to make the desired changes
  • Investigate when Spring Security changes the recommendation of using the standard methods to the Customizer enabled ones. In particular, these methods show up in Spring Security 5.2 (Spring Boot 2.2), but we don't enable them until Spring Security 5.7 (Spring Boot 2.4).

@timtebeek timtebeek added bug Something isn't working spring-security labels Aug 2, 2023
@timtebeek
Copy link
Contributor

Thanks a lot for these fixes! I'd briefly explored if the test you added fails to make changes for the right reasons, which it appears to do. The wrong reasons not to make changes could be a missing type leading to the recipe not executed; I've seen that in the past, and it's tricky to assert & prevent, but the debugger indicates the recipe is executed here.

It looks like Spring Security 7+ will drop the Lambda DSL , so these recipes will surely need to be part of that migration, although they're already part of the chain.

@timtebeek timtebeek merged commit cbc7381 into openrewrite:main Aug 2, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working spring-security
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants