From 1c8243b6ed7ea87aa3444d8d940f8a18e017f8f4 Mon Sep 17 00:00:00 2001 From: Vaadin Bot Date: Wed, 2 Oct 2024 09:57:33 +0200 Subject: [PATCH] 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 --- .../security/AuthenticationContext.java | 74 ++++++++- .../spring/security/UidlRedirectStrategy.java | 6 + .../VaadinSimpleUrlLogoutSuccessHandler.java | 58 +++++++ .../spring/security/VaadinWebSecurity.java | 2 +- .../security/AuthenticationContextTest.java | 152 +++++++++++++++--- .../security/VaadinWebSecurityTest.java | 2 +- 6 files changed, 268 insertions(+), 26 deletions(-) create mode 100644 vaadin-spring/src/main/java/com/vaadin/flow/spring/security/VaadinSimpleUrlLogoutSuccessHandler.java 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 index 31ff25a88fe..1c907d95a15 100644 --- 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 logoutHandlers; + + public CompositeLogoutHandler(List 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 index c4884d8594e..e26863aa696 100644 --- 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/vaadin-spring/src/main/java/com/vaadin/flow/spring/security/VaadinSimpleUrlLogoutSuccessHandler.java b/vaadin-spring/src/main/java/com/vaadin/flow/spring/security/VaadinSimpleUrlLogoutSuccessHandler.java new file mode 100644 index 00000000000..7464ae66c18 --- /dev/null +++ b/vaadin-spring/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}. + *

+ * 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 index 5a78e9ebf40..a80ed7c334e 100644 --- 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 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 index c5d122e63dd..df571284f0c 100644 --- 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,13 +41,17 @@ 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; @@ -55,6 +59,11 @@ 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. 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. 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 resultHandler, + SerializableConsumer 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. 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 index 62de91936a8..a7f86af78fb 100644 --- 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;