Skip to content

Conversation

ysavchen
Copy link
Contributor

Closes gh-10481

Comparing to AuthorizeRequestsDslTests, the newly added AuthorizeHttpRequestsDslTests does not include the following test:

@Test
fun `request when secured by mvc path variables then responds based on path variable value`() {
    this.spring.register(MvcMatcherPathVariablesConfig::class.java).autowire()
    this.mockMvc.get("/user/user")
        .andExpect {
            status { isOk() }
        }

    this.mockMvc.get("/user/deny")
        .andExpect {
            status { isForbidden() }
        }
}

@EnableWebSecurity
@EnableWebMvc
open class MvcMatcherPathVariablesConfig {
    @Bean
    open fun securityFilterChain(http: HttpSecurity): SecurityFilterChain {
        http {
            authorizeHttpRequests {
                authorize("/user/{userName}", "#userName == 'user'")
            }
        }
        return http.build()
    }

    @RestController
    internal class PathController {
        @RequestMapping("/user/{user}")
        fun path(@PathVariable user: String) {
        }
    }
}

As far as I understand authorizeHttpRequests API does not provide SpEL support and this test seems obsolete.
But due to default value for access parameter, there is a mismatch between authorizeRequests and authorizeHttpRequets API:

autorhizeRequests {
    authorize("/user/{userName}", "#userName == 'user'")   //invokes authorize with pattern and access
}

authorizeHttpRequests {
    authorize("/user/{userName}", "#userName == 'user'")   //invokes authorize with pattern and servletPath as access type is not String anymore
}

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Feb 21, 2022
@sjohnr sjohnr added in: config An issue in spring-security-config status: duplicate A duplicate of another issue type: enhancement A general enhancement labels Feb 23, 2022
@jzheaux jzheaux removed the status: waiting-for-triage An issue we've not yet triaged label Feb 24, 2022
Copy link
Contributor

@jzheaux jzheaux left a comment

Choose a reason for hiding this comment

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

Thanks, @ysavchen! I've left some feedback inline.

I'd also like to have @eleftherias take a look since she is quite a bit more familiar with the Kotlin DSL than I am.

@jzheaux jzheaux requested a review from eleftherias February 24, 2022 18:46
Copy link
Contributor

@eleftherias eleftherias left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @ysavchen!
I would like to have a test for what happens when we use both authorizeRequests and authorizeHttpRequests in the Kotlin DSL.
I've also left one commend inline.

@jzheaux
Copy link
Contributor

jzheaux commented Mar 10, 2022

Hi, @ysavchen. Are you able to make the requested changes?

@ysavchen
Copy link
Contributor Author

Sorry for delay @jzheaux. I needed some time to analyze the options.
I've added a comment regarding the used solution.
Tests are updated.

@ysavchen ysavchen requested review from eleftherias and jzheaux March 10, 2022 21:07
@jzheaux jzheaux added this to the 5.7.0-RC1 milestone Mar 22, 2022
@jzheaux jzheaux merged commit ca00b14 into spring-projects:main Mar 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

in: config An issue in spring-security-config status: duplicate A duplicate of another issue type: enhancement A general enhancement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add authorizeHttpRequests to Kotlin DSL

5 participants