Skip to content

Provide a way to customize the RedirectStrategy only #14061

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
trajano opened this issue Oct 29, 2023 · 5 comments
Closed

Provide a way to customize the RedirectStrategy only #14061

trajano opened this issue Oct 29, 2023 · 5 comments
Assignees
Labels
status: declined A suggestion or change that we don't feel we should currently apply type: enhancement A general enhancement

Comments

@trajano
Copy link

trajano commented Oct 29, 2023

Expected Behavior

http
  .redirectStrategy(ForwardHeadersRedirectStrategy())

Current Behavior

I basically have to recreate the code to rebuild the default login screen

        val reactiveAuthenticationManager = UserDetailsRepositoryReactiveAuthenticationManager(userDetailsService)

        val requestCache = WebSessionServerRequestCache()
        val forwardHeadersRedirectStrategy = ForwardHeadersRedirectStrategy()
        val authenticationEntryPoint =
            ForwardHeadersAuthenticationEntryPoint("/login", requestCache)

        val authenticationSuccessHandler = RedirectServerAuthenticationSuccessHandler("/")
        authenticationSuccessHandler.setRedirectStrategy(forwardHeadersRedirectStrategy)
        authenticationSuccessHandler.setRequestCache(requestCache)

        val authenticationFailureHandler = RedirectServerAuthenticationFailureHandler("/login?error")
        authenticationFailureHandler.setRedirectStrategy(forwardHeadersRedirectStrategy)

        val loginPageGeneratingWebFilter = LoginPageGeneratingWebFilter()
        loginPageGeneratingWebFilter.setFormLoginEnabled(true)
        loginPageGeneratingWebFilter.setOauth2AuthenticationUrlToClientName()

        return http
            .addFilterAt(loginPageGeneratingWebFilter, SecurityWebFiltersOrder.LOGIN_PAGE_GENERATING)
            .addFilterAt(LogoutPageGeneratingWebFilter(), SecurityWebFiltersOrder.LOGOUT_PAGE_GENERATING)
...
            .formLogin {
                it.loginPage("/login")
                it.authenticationFailureHandler(authenticationFailureHandler)
                it.authenticationSuccessHandler(authenticationSuccessHandler)
                it.authenticationEntryPoint(authenticationEntryPoint)
            }

Context
I get X-Forwarded-For headers which are not recognized by Spring Security and such it will redirect without the host name.

class ForwardHeadersRedirectStrategy : ServerRedirectStrategy {
    override fun sendRedirect(exchange: ServerWebExchange, location: URI): Mono<Void> =
        Mono.fromRunnable {
            val response = exchange.response
            response.setStatusCode(HttpStatus.SEE_OTHER)
            val newLocation: URI = createLocation(exchange, location)
            response.headers.location = newLocation

        }

    private fun createLocation(exchange: ServerWebExchange, location: URI): URI {
        val url = location.toASCIIString()
        if (url.startsWith("/") && exchange.request.headers["x-forwarded-proto"] != null) {
            val context = exchange.request.path.contextPath().value()
            val forwardedProto = exchange.request.headers["x-forwarded-proto"]!![0]
            var forwardedPort = exchange.request.headers["x-forwarded-port"]!![0]
            when {
                "https".equals(forwardedProto) && "443".equals(forwardedPort) -> {
                    forwardedPort = null;
                }

                "http".equals(forwardedProto) && "80".equals(forwardedPort) -> {
                    forwardedPort = null;
                }
            }
            return UriComponentsBuilder.newInstance()
                .scheme(forwardedProto)
                .host(exchange.request.headers["x-forwarded-host"]!![0])
                .port(forwardedPort)
                .path(context)
                .path(location.path)
                .query(location.query)
                .build()
                .toUri()
        }
        return location
    }

}
@trajano trajano added status: waiting-for-triage An issue we've not yet triaged type: enhancement A general enhancement labels Oct 29, 2023
@marcusdacoregio
Copy link
Contributor

Hi, @trajano.

I don't think I follow, why you cannot just customize the ServerAuthenticationSuccessHandler or ServerAuthenticationFailureHandler as mentioned here?

@marcusdacoregio marcusdacoregio self-assigned this Nov 1, 2023
@marcusdacoregio marcusdacoregio added status: waiting-for-feedback We need additional information before we can continue and removed status: waiting-for-triage An issue we've not yet triaged labels Nov 1, 2023
@trajano
Copy link
Author

trajano commented Nov 2, 2023

Let me try it out. And to answer your question ... because I didn't see it documented as an option. But looking at the solution

This will override authenticationSuccessHandler so I guess I would also need to change the failure as well, but it's missing authenticationEntryPoint BUT because of

private final class LoginPageSpec {

	protected void configure(ServerHttpSecurity http) {
		if (http.authenticationEntryPoint != null) {
			return;
		}

It destroys the default login forms. So it brings back to my problem where I had to recreate the specs to bring back the original login form capability.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Nov 2, 2023
@marcusdacoregio
Copy link
Contributor

If you override the authenticationEntryPoint, Spring Security assumes that you do not want the default behavior and back-off on generating the default login page. Ideally, the default login page should be used only to give users a starting point for form login and should be replaced with a custom login page.

@marcusdacoregio marcusdacoregio added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels Nov 3, 2023
@trajano
Copy link
Author

trajano commented Nov 3, 2023

That's correct, but I didn't really want to change the entry point either, I just wanted to change the redirect strategy. But there was no facility to just change that, instead I had to recreate all the components down to the authenticationEntryPoint. As per my OP I just wanted

http.redirectStrategy(customRedirectStrategy)

Or to follow the rest of the convention would be something like

http.authenticationEntryPoint { it.setRedirectStrategy(customRedirectStrategy) }
http.authenticationSuccessHandler { it.setRedirectStrategy(customRedirectStrategy) } 
http.authenticationFailureHandler { it.setRedirectStrategy(customRedirectStrategy) }

Another approach is to have that logic I wrote to handle X-Forward... headers be handled by the default redirect strategy.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Nov 3, 2023
@marcusdacoregio
Copy link
Contributor

I am not sure if we want those global strategies since they could become too complex to handle the configuration scenarios in different authentication mechanisms. Furthermore, it seems that you want to handle absolute URIs in redirection, which has been covered by #11656.

That being said, I'll close this as declined since we are not looking into adding that for now.

@marcusdacoregio marcusdacoregio added status: declined A suggestion or change that we don't feel we should currently apply and removed status: feedback-provided Feedback has been provided labels Nov 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: declined A suggestion or change that we don't feel we should currently apply type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

3 participants