rewrite-spring icon indicating copy to clipboard operation
rewrite-spring copied to clipboard

Incorrect change by `ServerHttpSecurityLambdaDsl`

Open eocantu opened this issue 1 year ago • 1 comments

What version of OpenRewrite are you using?

I am using

  • OpenRewrite v8.28.1
  • rewrite-spring v5.13.2
  • Recipe: org.openrewrite.java.spring.boot3.UpgradeSpringBoot_3_2

How are you running OpenRewrite?

I am using the Maven plugin, and my project is a multi-module project: https://github.com/eocantu/spring-boot-demo

What is the smallest, simplest way to reproduce the problem?

class SecurityConfig {
    @Bean
    fun securityWebFilterChain(
        http: ServerHttpSecurity,
        tokenAuthFilter: AuthenticationWebFilter,
        authHeaderWebExchangeMatcher: AuthHeaderWebExchangeMatcher
    ): SecurityWebFilterChain {
        return http
            .csrf().disable()
            .addFilterAt(tokenAuthFilter, SecurityWebFiltersOrder.AUTHENTICATION)
            .authorizeExchange()
            .pathMatchers("/playground").permitAll()
            .pathMatchers("/warmup").permitAll()
            .pathMatchers("/isActive").permitAll()
            .pathMatchers("/actuator/**").permitAll()
            .matchers(authHeaderWebExchangeMatcher).permitAll()
            .anyExchange().authenticated()
            .and()
            .build()
    }
}

What did you expect to see?

class SecurityConfig {
    @Bean
    fun securityWebFilterChain(
        http: ServerHttpSecurity,
        tokenAuthFilter: AuthenticationWebFilter,
        authHeaderWebExchangeMatcher: AuthHeaderWebExchangeMatcher
    ): SecurityWebFilterChain {
        return http
            .csrf { csrf -> csrf.disable() }
            .addFilterAt(tokenAuthFilter, SecurityWebFiltersOrder.AUTHENTICATION)
            .authorizeExchange { exchange -> exchange
                .pathMatchers("/playground").permitAll()
                .pathMatchers("/warmup").permitAll()
                .pathMatchers("/isActive").permitAll()
                .pathMatchers("/actuator/**").permitAll()
                .matchers(authHeaderWebExchangeMatcher).permitAll()
                .anyExchange().authenticated() }
            .build()
    }
}

What did you see instead?

class SecurityConfig {
    @Bean
    fun securityWebFilterChain(
        http: ServerHttpSecurity,
        tokenAuthFilter: AuthenticationWebFilter,
        authHeaderWebExchangeMatcher: AuthHeaderWebExchangeMatcher
    ): SecurityWebFilterChain {
        return http
            .csrf({csrf ->csrf
                .build()})
    }
}

Are you interested in contributing a fix to OpenRewrite?

Perhaps, if the fix isn't too involved.

eocantu avatar Jun 24 '24 21:06 eocantu

Oh wow, that's pretty destructive; definitely not what we'd hope to see. I wonder if the use of Kotlin's { .. } factors in here. A quick way to verify would be to add a unit test in ServerHttpSecurityLambdaDslTest.java such that we can test variants more easily.

The code changes themselves are in this visitor, which is doing it's best in the face of complicated API changes on the part of Spring Security, which had worked well in other cases so far, but this is the first time I see them applied to Kotlin.

timtebeek avatar Jun 25 '24 07:06 timtebeek