Describe the bug

12307 is now closed. So I created this issue.

In this comment it suggests to use this to configure PortMapper this way:

    @Bean
    public SecurityFilterChain securityFilterChain(HttpSecurity http) throws Exception {
        http
            // ...
            .oauth2Login(Customizer.withDefaults())
            .exceptionHandling((exceptions) -> exceptions
                .authenticationEntryPoint(authenticationEntryPoint())
            );
...
...
    private AuthenticationEntryPoint authenticationEntryPoint() {
        PortMapperImpl portMapper = new PortMapperImpl();
        portMapper.setPortMappings(Map.of("8080", "8080"));

        PortResolverImpl portResolver = new PortResolverImpl();
        portResolver.setPortMapper(portMapper);

        LoginUrlAuthenticationEntryPoint authenticationEntryPoint =
                new LoginUrlAuthenticationEntryPoint(
                        "/oauth2/authorization/spring-addons-public");
        authenticationEntryPoint.setPortMapper(portMapper);
        authenticationEntryPoint.setPortResolver(portResolver);

        return authenticationEntryPoint;
    }

However once the authenticationEntryPoint is overridden then the code below prevents registration of DefaultLoginPageGeneratingFilter :

org\springframework\security\config\annotation\web\configurers\DefaultLoginPageConfigurer.java

    @Override
    @SuppressWarnings("unchecked")
    public void configure(H http) {
        AuthenticationEntryPoint authenticationEntryPoint = null;
        ExceptionHandlingConfigurer<?> exceptionConf = http.getConfigurer(ExceptionHandlingConfigurer.class);
        if (exceptionConf != null) {
            authenticationEntryPoint = exceptionConf.getAuthenticationEntryPoint();
        }
        if (this.loginPageGeneratingFilter.isEnabled() && authenticationEntryPoint == null) { // <--------------------- NOTE
            this.loginPageGeneratingFilter = postProcess(this.loginPageGeneratingFilter);
            http.addFilter(this.loginPageGeneratingFilter);
            LogoutConfigurer<H> logoutConfigurer = http.getConfigurer(LogoutConfigurer.class);
            if (logoutConfigurer != null) {
                http.addFilter(this.logoutPageGeneratingFilter);
            }
        }
    }

because authenticationEntryPoint is not null.

And then if there is failure during the flow there is redirection to http.://.../login?error and that results in 404 error.

I implemented the configuration of PortMapper this way and that works:

        httpSecurity
...
                .oauth2Login((OAuth2LoginConfigurer<HttpSecurity> httpSecurityOAuth2LoginConfigurer) -> {

                    httpSecurityOAuth2LoginConfigurer.withObjectPostProcessor(authenticationEntryPointFilterPostProcessor);
                })
...
...
        ObjectPostProcessor<AuthenticationEntryPoint> authenticationEntryPointFilterPostProcessor = new ObjectPostProcessor<>() {
            @Override
            public <O extends AuthenticationEntryPoint> O postProcess(O authenticationEntryPoint) {
                if (authenticationEntryPoint instanceof DelegatingAuthenticationEntryPoint delegatingAuthenticationEntryPoint) {
                    // Reflectively access LoginUrlAuthenticationEntryPoint via DelegatingAuthenticationEntryPoint
                    Field entryPointsField = ReflectionUtils.findField(DelegatingAuthenticationEntryPoint.class, "entryPoints");
                    assert entryPointsField != null;
                    entryPointsField.setAccessible(true);
                    @SuppressWarnings("unchecked")
                    LinkedHashMap<RequestMatcher, AuthenticationEntryPoint> entryPoints =
                            (LinkedHashMap<RequestMatcher, AuthenticationEntryPoint>) ReflectionUtils.getField(entryPointsField,
                                    delegatingAuthenticationEntryPoint);
                    Objects.requireNonNull(entryPoints).values().forEach(authenticationEntryPointValue -> {
                        if (authenticationEntryPointValue instanceof LoginUrlAuthenticationEntryPoint loginUrlAuthenticationEntryPoint) {
                            PortMapperImpl portMapper = new PortMapperImpl();
                            portMapper.setPortMappings(Map.of(
                                    "80", "80",
                                    "8080", "8080"));
                            loginUrlAuthenticationEntryPoint.setPortMapper(portMapper);
                            // Also need to set PortResolver
                            PortResolverImpl portResolver = new PortResolverImpl();
                            portResolver.setPortMapper(portMapper);
                            loginUrlAuthenticationEntryPoint.setPortResolver(portResolver);
                        }
                    });
                }

                return authenticationEntryPoint;
            }
        };

The key idea is to not disturb the config of entryPoint but reflectively set the PortMapper.

Ideally if the LoginUrlAuthenticationEntryPoint was postProcessed the reflection could be avoided.

In addition, LoginUrlAuthenticationEntryPoint uses both PortMapper and PortResolver (which in turn has delegate PortMapper) which causes additional confusion.

public class LoginUrlAuthenticationEntryPoint implements AuthenticationEntryPoint, InitializingBean {

    private PortMapper portMapper = new PortMapperImpl();

    private PortResolver portResolver = new PortResolverImpl();

In any case, what is the correct/official way to handle this?

Comment From: sandipchitale

BTW this PortMapper business is nothing but trouble and I am glad there is an effort to get rid of it.

Comment From: marcusdacoregio

Hi, @sandipchitale. Thanks for the report.

The default login page is meant to be used as a temporary solution until you add your login page to use it in production, if you want it to be enabled again you can register the DefaultLoginPageGeneratingFilter manually. Post-processing the LoginUrlAuthenticationEntryPoint might be a good idea at first, but I don't think that you should be relying on the default generated login page, therefore you do not need to do that reflection stuff to keep it registered.

Comment From: sandipchitale

In one of our use case we only have one OAuth2 Client. Therefore as such the login page never shown - due to special logic in OAuth2LoginConfigurer:

org\springframework\security\config\annotation\web\configurers\oauth2\client\OAuth2LoginConfigurer.java:

  320:          Map<String, String> loginUrlToClientName = this.getLoginLinks();
  321:          if (loginUrlToClientName.size() == 1) {

during normal processing of login. It is only when there is an error during the flow, and when there is a need to show the error (/login?error case), then this login page comes into play. In any case the point is taken that we should have our own page for this case as well and deal with the PortMapper ourselves. In any case, when implementing a custom login page is it still recommended to subclass LoginUrlAuthenticationEntryPoint ?

Comment From: sjohnr

@sandipchitale thanks for the extra details and conversation. We're taking challenges that users have with configuration very seriously and hope to continue improving things.

Having said that, I would generally recommend that you take control of all aspects of the configuration that you rely on, such as the login page, redirects (via AuthenticationEntryPoint), error handling (whether via AuthenticationFailureHandler, AccessDeniedHandler or general Spring error handling with @ExceptionHandler etc.). Whether you subclass LoginUrlAuthenticationEntryPoint or not is your choice, but we prefer encapsulation and delegation over inheritance whenever possible. You should also feel free to completely roll your own redirect-based AuthenticationEntryPoint as the API contract is quite easy to implement. You can choose which parts of Spring Security to re-use in that implementation (or not) as needed.

In one of our use case we only have one OAuth2 Client. Therefore as such the login page never shown - due to special logic in OAuth2LoginConfigurer:

``` org\springframework\security\config\annotation\web\configurers\oauth2\client\OAuth2LoginConfigurer.java:

320: Map loginUrlToClientName = this.getLoginLinks(); 321: if (loginUrlToClientName.size() == 1) { ```

The defaults in Spring Security like this one (as well as the default login page) are generally for the getting started experience only and I would recommend that you override and take full control of all related configuration. I hope that answers your remaining questions.

For now, I'm going to go ahead and close this issue. If you feel I've jumped the gun on that, let me know what we've missed and we can re-open if there's anything additional to consider here.

Comment From: sandipchitale

@sjohnr Thanks for the detailed response. I got the gist and will proceed to implement based on the above discussion.