fix: allow null response in logout (#20057) (#20112)

Don't throw NullPointerException in case of null VaadinServletResponse in AuthenticationContext#logout. Tolerate null response better in case when running with @Push(transport = Transport.WEBSOCKET), or when response is null for some other reason. Makes logout also work in WEBSOCKET mode by automatically switching to WEBSOCKET_XHR for one additional request that executes logout. Fixes: #20017 Co-authored-by: Tomi Virtanen <[email protected]>
vaadin · Oct 2, 2024 · 1c8243b · 1c8243b
1 parent 06c7707
commit 1c8243b
Show file tree

Hide file tree

Showing 6 changed files with 268 additions and 26 deletions.
diff --git a/vaadin-spring/src/main/java/com/vaadin/flow/spring/security/AuthenticationContext.java b/vaadin-spring/src/main/java/com/vaadin/flow/spring/security/AuthenticationContext.java
@@ -20,6 +20,7 @@
 import jakarta.servlet.http.HttpServletResponse;
 
 import java.io.IOException;
+import java.io.Serializable;
 import java.security.Principal;
 import java.util.Collection;
 import java.util.Collections;
@@ -38,13 +39,15 @@
 import org.springframework.security.core.GrantedAuthority;
 import org.springframework.security.core.context.SecurityContext;
 import org.springframework.security.core.context.SecurityContextHolder;
-import org.springframework.security.web.authentication.logout.CompositeLogoutHandler;
 import org.springframework.security.web.authentication.logout.LogoutHandler;
 import org.springframework.security.web.authentication.logout.LogoutSuccessHandler;
+import org.springframework.security.web.authentication.logout.SecurityContextLogoutHandler;
+import org.springframework.util.Assert;
 
 import com.vaadin.flow.component.UI;
 import com.vaadin.flow.server.VaadinServletRequest;
 import com.vaadin.flow.server.VaadinServletResponse;
+import com.vaadin.flow.shared.ui.Transport;
 
 /**
  * The authentication context of the application.
@@ -127,14 +130,42 @@ public boolean isAuthenticated() {
      * {@link org.springframework.security.web.authentication.logout.LogoutHandler}.
      */
     public void logout() {
+        final UI ui = UI.getCurrent();
+        if (ui.getPushConfiguration().getTransport() == Transport.WEBSOCKET
+                && ui.getInternals().getPushConnection().isConnected()) {
+            // WEBSOCKET transport mode would not log out properly after session
+            // invalidation. Switching to WEBSOCKET_XHR for a single request
+            // to do the logout.
+            ui.getPushConfiguration().setTransport(Transport.WEBSOCKET_XHR);
+            ui.getPage().executeJs("return true").then(ignored -> {
+                LOGGER.debug(
+                        "Switched to WEBSOCKET_XHR transport mode successfully for logout operation.");
+                ui.getPushConfiguration().setTransport(Transport.WEBSOCKET);
+                doLogout(ui);
+            }, exception -> {
+                LOGGER.debug(
+                        "Failed to switch to WEBSOCKET_XHR transport mode for logout operation. "
+                                + "Logout is performed anyway even if browser shows 'disconnected' message and browser console has WebSocket errors. "
+                                + "Received exception: {}",
+                        exception);
+                ui.getPushConfiguration().setTransport(Transport.WEBSOCKET);
+                doLogout(ui);
+            });
+        } else {
+            doLogout(ui);
+        }
+    }
+
+    private void doLogout(UI ui) {
         HttpServletRequest request = VaadinServletRequest.getCurrent()
                 .getHttpServletRequest();
-        HttpServletResponse response = VaadinServletResponse.getCurrent()
-                .getHttpServletResponse();
+        HttpServletResponse response = Optional
+                .ofNullable(VaadinServletResponse.getCurrent())
+                .map(VaadinServletResponse::getHttpServletResponse)
+                .orElse(null);
         Authentication auth = SecurityContextHolder.getContext()
                 .getAuthentication();
 
-        final UI ui = UI.getCurrent();
         logoutHandler.logout(request, response, auth);
         ui.accessSynchronously(() -> {
             try {
@@ -435,4 +466,39 @@ public static void applySecurityConfiguration(HttpSecurity httpSecurity,
                 logoutConfigurer.getLogoutHandlers());
     }
 
+    // package protected for testing purposes
+    static class CompositeLogoutHandler implements LogoutHandler, Serializable {
+
+        private final List<LogoutHandler> logoutHandlers;
+
+        public CompositeLogoutHandler(List<LogoutHandler> logoutHandlers) {
+            Assert.notEmpty(logoutHandlers, "LogoutHandlers are required");
+            this.logoutHandlers = logoutHandlers;
+        }
+
+        @Override
+        public void logout(HttpServletRequest request,
+                HttpServletResponse response, Authentication authentication) {
+            for (LogoutHandler handler : this.logoutHandlers) {
+                try {
+                    handler.logout(request, response, authentication);
+                } catch (IllegalStateException e) {
+                    // Tolerate null response when Session is already
+                    // invalidated
+                    if (response != null
+                            || !isContinueToNextHandler(request, handler)) {
+                        throw e;
+                    }
+                }
+            }
+        }
+
+        private boolean isContinueToNextHandler(HttpServletRequest request,
+                LogoutHandler handler) {
+            return handler instanceof SecurityContextLogoutHandler
+                    && (request.getSession() == null
+                            || !request.isRequestedSessionIdValid());
+        }
+    }
+
 }
diff --git a/vaadin-spring/src/main/java/com/vaadin/flow/spring/security/UidlRedirectStrategy.java b/vaadin-spring/src/main/java/com/vaadin/flow/spring/security/UidlRedirectStrategy.java
@@ -32,6 +32,12 @@ public void sendRedirect(HttpServletRequest request,
                                 + "but it was not possible to get the UI instance to perform the action.",
                         url);
             }
+        } else if (response == null) {
+            LoggerFactory.getLogger(UidlRedirectStrategy.class)
+                    .warn("A redirect to {} was request, "
+                            + "but it has null HttpServletResponse and can't perform the action. "
+                            + "Performing logout during a Vaadin request with @Push(transport = Transport.WEBSOCKET) would cause this.",
+                            url);
         } else {
             super.sendRedirect(request, response, url);
         }

diff --git a/...ng/src/main/java/com/vaadin/flow/spring/security/VaadinSimpleUrlLogoutSuccessHandler.java b/...ng/src/main/java/com/vaadin/flow/spring/security/VaadinSimpleUrlLogoutSuccessHandler.java
@@ -0,0 +1,58 @@
+/*
+ * Copyright 2000-2024 Vaadin Ltd.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License"); you may not
+ * use this file except in compliance with the License. You may obtain a copy of
+ * the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package com.vaadin.flow.spring.security;
+
+import jakarta.servlet.ServletException;
+import jakarta.servlet.http.HttpServletRequest;
+import jakarta.servlet.http.HttpServletResponse;
+
+import java.io.IOException;
+import java.io.Serializable;
+
+import org.springframework.security.core.Authentication;
+import org.springframework.security.web.authentication.logout.SimpleUrlLogoutSuccessHandler;
+
+/**
+ * Default logout success handler for {@link VaadinWebSecurity}.
+ * <p>
+ * For internal use only. May be renamed or removed in a future release.
+ */
+class VaadinSimpleUrlLogoutSuccessHandler extends SimpleUrlLogoutSuccessHandler
+        implements Serializable {
+
+    @Override
+    public void onLogoutSuccess(HttpServletRequest request,
+            HttpServletResponse response, Authentication authentication)
+            throws IOException, ServletException {
+        handle(request, response, authentication);
+    }
+
+    @Override
+    protected void handle(HttpServletRequest request,
+            HttpServletResponse response, Authentication authentication)
+            throws IOException, ServletException {
+        if (response == null) {
+            // tolerate null response without failing
+            String targetUrl = determineTargetUrl(request, response,
+                    authentication);
+            getRedirectStrategy().sendRedirect(request, response, targetUrl);
+        } else {
+            super.handle(request, response, authentication);
+        }
+    }
+
+}
diff --git a/vaadin-spring/src/main/java/com/vaadin/flow/spring/security/VaadinWebSecurity.java b/vaadin-spring/src/main/java/com/vaadin/flow/spring/security/VaadinWebSecurity.java
@@ -630,7 +630,7 @@ protected void addLogoutHandlers(Consumer<LogoutHandler> registry) {
 
     private void configureLogout(HttpSecurity http, String logoutSuccessUrl)
             throws Exception {
-        SimpleUrlLogoutSuccessHandler logoutSuccessHandler = new SimpleUrlLogoutSuccessHandler();
+        VaadinSimpleUrlLogoutSuccessHandler logoutSuccessHandler = new VaadinSimpleUrlLogoutSuccessHandler();
         logoutSuccessHandler.setDefaultTargetUrl(logoutSuccessUrl);
         logoutSuccessHandler.setRedirectStrategy(new UidlRedirectStrategy());
         http.logout(cfg -> cfg.logoutSuccessHandler(logoutSuccessHandler));

diff --git a/vaadin-spring/src/test/java/com/vaadin/flow/spring/security/AuthenticationContextTest.java b/vaadin-spring/src/test/java/com/vaadin/flow/spring/security/AuthenticationContextTest.java
@@ -41,20 +41,29 @@
 import org.springframework.security.core.userdetails.User;
 import org.springframework.security.test.context.support.WithAnonymousUser;
 import org.springframework.security.test.context.support.WithMockUser;
-import org.springframework.security.web.authentication.logout.CompositeLogoutHandler;
 import org.springframework.security.web.authentication.logout.LogoutHandler;
 import org.springframework.security.web.authentication.logout.LogoutSuccessHandler;
 import org.springframework.test.context.ContextConfiguration;
 import org.springframework.test.context.junit4.SpringRunner;
 
+import com.vaadin.flow.component.PushConfiguration;
 import com.vaadin.flow.component.UI;
+import com.vaadin.flow.component.internal.UIInternals;
+import com.vaadin.flow.component.page.Page;
+import com.vaadin.flow.component.page.PendingJavaScriptResult;
+import com.vaadin.flow.function.SerializableConsumer;
 import com.vaadin.flow.internal.CurrentInstance;
 import com.vaadin.flow.server.Command;
 import com.vaadin.flow.server.VaadinRequest;
 import com.vaadin.flow.server.VaadinResponse;
 import com.vaadin.flow.server.VaadinServletRequest;
 import com.vaadin.flow.server.VaadinServletResponse;
 
+import com.vaadin.flow.server.communication.PushConnection;
+import com.vaadin.flow.shared.ui.Transport;
+import com.vaadin.flow.spring.security.AuthenticationContext.CompositeLogoutHandler;
+import elemental.json.JsonValue;
+
 @RunWith(SpringRunner.class)
 @ContextConfiguration(classes = ObjectPostProcessorConfiguration.class)
 public class AuthenticationContextTest {
@@ -402,9 +411,104 @@ public void hasAllAuthorities_noAuthorities_false() {
                 .hasAllAuthorities(List.of("AUTH_READ", "AUTH_WRITE")));
     }
 
+    @Test
+    @WithMockUser()
+    public void logout_allowNullResponse() {
+        authContext.setLogoutHandlers(Mockito.mock(LogoutSuccessHandler.class),
+                List.of(Mockito.mock(LogoutHandler.class)));
+        try {
+            CurrentInstance.set(VaadinRequest.class,
+                    Mockito.mock(VaadinServletRequest.class));
+            UI.setCurrent(Mockito.mock(UI.class));
+            mockPush(UI.getCurrent(), Transport.WEBSOCKET_XHR);
+            try {
+                authContext.logout();
+            } catch (NullPointerException e) {
+                Assert.fail("Should not throw NPE");
+            }
+        } finally {
+            CurrentInstance.clearAll();
+        }
+    }
+
     @Test
     @WithMockUser()
     public void logout_handlersEngaged() throws Exception {
+        SetupForLogoutTest setup = getSetupForLogoutTest();
+
+        UI ui = Mockito.mock(UI.class);
+        Mockito.doAnswer(i -> {
+            i.<Command> getArgument(0).execute();
+            return null;
+        }).when(ui).accessSynchronously(ArgumentMatchers.any());
+        mockPush(ui);
+        try {
+            CurrentInstance.set(VaadinRequest.class, setup.vaadinRequest());
+            CurrentInstance.set(VaadinResponse.class, setup.vaadinResponse());
+            UI.setCurrent(ui);
+            authContext.logout();
+
+            Mockito.verify(setup.successHandler()).onLogoutSuccess(
+                    setup.request(), setup.response(), setup.authentication());
+            Mockito.verify(setup.handler2()).logout(setup.request(),
+                    setup.response(), setup.authentication());
+            Mockito.verify(setup.handler1()).logout(setup.request(),
+                    setup.response(), setup.authentication());
+        } finally {
+            CurrentInstance.clearAll();
+        }
+    }
+
+    @Test
+    @WithMockUser()
+    public void logout_pushWithWebsocket_handlersEngaged() throws Exception {
+        SetupForLogoutTest setup = getSetupForLogoutTest();
+
+        UI ui = Mockito.mock(UI.class);
+        Mockito.doAnswer(i -> {
+            i.<Command> getArgument(0).execute();
+            return null;
+        }).when(ui).accessSynchronously(ArgumentMatchers.any());
+        mockPush(ui, Transport.WEBSOCKET);
+        Page page = Mockito.mock(Page.class);
+        Mockito.when(ui.getPage()).thenReturn(page);
+        Mockito.when(page.executeJs(Mockito.anyString()))
+                .thenReturn(new PendingJavaScriptResult() {
+                    @Override
+                    public boolean cancelExecution() {
+                        return false;
+                    }
+
+                    @Override
+                    public boolean isSentToBrowser() {
+                        return true;
+                    }
+
+                    @Override
+                    public void then(
+                            SerializableConsumer<JsonValue> resultHandler,
+                            SerializableConsumer<String> errorHandler) {
+                        resultHandler.accept(null);
+                    }
+                });
+        try {
+            CurrentInstance.set(VaadinRequest.class, setup.vaadinRequest());
+            CurrentInstance.set(VaadinResponse.class, setup.vaadinResponse());
+            UI.setCurrent(ui);
+            authContext.logout();
+
+            Mockito.verify(setup.successHandler()).onLogoutSuccess(
+                    setup.request(), setup.response(), setup.authentication());
+            Mockito.verify(setup.handler2()).logout(setup.request(),
+                    setup.response(), setup.authentication());
+            Mockito.verify(setup.handler1()).logout(setup.request(),
+                    setup.response(), setup.authentication());
+        } finally {
+            CurrentInstance.clearAll();
+        }
+    }
+
+    private SetupForLogoutTest getSetupForLogoutTest() {
         Authentication authentication = SecurityContextHolder.getContext()
                 .getAuthentication();
 
@@ -425,26 +529,15 @@ public void logout_handlersEngaged() throws Exception {
                 .mock(VaadinServletResponse.class);
         Mockito.when(vaadinResponse.getHttpServletResponse())
                 .thenReturn(response);
+        return new SetupForLogoutTest(authentication, successHandler, handler1,
+                handler2, request, vaadinRequest, response, vaadinResponse);
+    }
 
-        UI ui = Mockito.mock(UI.class);
-        Mockito.doAnswer(i -> {
-            i.<Command> getArgument(0).execute();
-            return null;
-        }).when(ui).accessSynchronously(ArgumentMatchers.any());
-
-        try {
-            CurrentInstance.set(VaadinRequest.class, vaadinRequest);
-            CurrentInstance.set(VaadinResponse.class, vaadinResponse);
-            UI.setCurrent(ui);
-            authContext.logout();
-
-            Mockito.verify(successHandler).onLogoutSuccess(request, response,
-                    authentication);
-            Mockito.verify(handler2).logout(request, response, authentication);
-            Mockito.verify(handler1).logout(request, response, authentication);
-        } finally {
-            CurrentInstance.clearAll();
-        }
+    private record SetupForLogoutTest(Authentication authentication,
+            LogoutSuccessHandler successHandler, LogoutHandler handler1,
+            LogoutHandler handler2, HttpServletRequest request,
+            VaadinServletRequest vaadinRequest, HttpServletResponse response,
+            VaadinServletResponse vaadinResponse) {
     }
 
     @Test
@@ -512,4 +605,23 @@ public void supportsCustomRolePrefixes() {
         Assert.assertTrue(roles.contains("USER"));
         Assert.assertTrue(roles.contains("ADMIN"));
     }
+
+    private static void mockPush(UI ui) {
+        mockPush(ui, null);
+    }
+
+    private static void mockPush(UI ui, Transport pushTransport) {
+        UIInternals internals = Mockito.mock(UIInternals.class);
+        PushConnection pushConnection = Mockito.mock(PushConnection.class);
+        PushConfiguration pushConfiguration = Mockito
+                .mock(PushConfiguration.class);
+
+        Mockito.when(ui.getPushConfiguration()).thenReturn(pushConfiguration);
+        Mockito.when(pushConfiguration.getTransport())
+                .thenReturn(pushTransport == null ? Transport.WEBSOCKET_XHR
+                        : pushTransport);
+        Mockito.when(ui.getInternals()).thenReturn(internals);
+        Mockito.when(internals.getPushConnection()).thenReturn(pushConnection);
+        Mockito.when(pushConnection.isConnected()).thenReturn(true);
+    }
 }
diff --git a/vaadin-spring/src/test/java/com/vaadin/flow/spring/security/VaadinWebSecurityTest.java b/vaadin-spring/src/test/java/com/vaadin/flow/spring/security/VaadinWebSecurityTest.java
@@ -34,7 +34,7 @@
 import org.springframework.security.config.annotation.web.builders.HttpSecurity;
 import org.springframework.security.config.annotation.web.builders.WebSecurity;
 import org.springframework.security.core.Authentication;
-import org.springframework.security.web.authentication.logout.CompositeLogoutHandler;
+import com.vaadin.flow.spring.security.AuthenticationContext.CompositeLogoutHandler;
 import org.springframework.security.web.authentication.logout.LogoutHandler;
 import org.springframework.test.context.ContextConfiguration;
 import org.springframework.test.context.junit4.SpringRunner;