From 80813cb4a6f8c58c1b1df962b79b83f7c8cf20cc Mon Sep 17 00:00:00 2001 From: Marco Collovati Date: Tue, 12 Nov 2024 11:52:15 +0100 Subject: [PATCH] fix: delay session expiration handling to prevent canceling ongoing navigation (#19983) Attempts to fix the synchronization issue related to the usage of the Login reported in #12640. The Login component sends the UIDL request for the login event to the server and concurrently submits the form. If processing the form submission performs a session ID change and a request redirect, the UIDL requests might fail with a session expiration response. The Flow client then can cancel the first redirect because it reloads the page due to the session expiration. Lastly, the beacon request hits again a valid session, but a resynchronization is triggered because the previous UIDL request was rejected. This change delays a bit the session expiration handling on Flow client, to allow a potential redirect to complete without being cancelled. However, the client application is immediately set in TERMINATED state. --- .../client/communication/MessageHandler.java | 9 +++-- .../vaadin/client/GwtMessageHandlerTest.java | 33 ++++++++++--------- .../communication/ServerRpcHandler.java | 8 +++++ .../com/vaadin/viteapp/BasicComponentIT.java | 20 +++++------ .../com/vaadin/viteapp/BasicComponentIT.java | 19 +++++------ .../com/vaadin/viteapp/BasicComponentIT.java | 19 +++++------ .../com/vaadin/viteapp/BasicComponentIT.java | 18 ++++++---- .../flow/uitest/ui/InternalErrorIT.java | 2 ++ .../uitest/ui/RouterSessionExpirationIT.java | 8 +++-- 9 files changed, 79 insertions(+), 57 deletions(-) diff --git a/flow-client/src/main/java/com/vaadin/client/communication/MessageHandler.java b/flow-client/src/main/java/com/vaadin/client/communication/MessageHandler.java index c71628ea39a..98533e45c1e 100644 --- a/flow-client/src/main/java/com/vaadin/client/communication/MessageHandler.java +++ b/flow-client/src/main/java/com/vaadin/client/communication/MessageHandler.java @@ -456,9 +456,14 @@ assert getServerId(valueMap) == -1 if (nextResponseSessionExpiredHandler != null) { nextResponseSessionExpiredHandler.execute(); } else if (uiState != UIState.TERMINATED) { - registry.getSystemErrorHandler() - .handleSessionExpiredError(null); registry.getUILifecycle().setState(UIState.TERMINATED); + // Delay the session expiration handling to prevent + // canceling potential ongoing page redirect/reload + Scheduler.get().scheduleFixedDelay(() -> { + registry.getSystemErrorHandler() + .handleSessionExpiredError(null); + return false; + }, 250); } } else if (meta.containsKey("appError") && uiState != UIState.TERMINATED) { diff --git a/flow-client/src/test-gwt/java/com/vaadin/client/GwtMessageHandlerTest.java b/flow-client/src/test-gwt/java/com/vaadin/client/GwtMessageHandlerTest.java index bbde6eeb649..4f37931e8d0 100644 --- a/flow-client/src/test-gwt/java/com/vaadin/client/GwtMessageHandlerTest.java +++ b/flow-client/src/test-gwt/java/com/vaadin/client/GwtMessageHandlerTest.java @@ -159,12 +159,13 @@ public void handleSessionExpiredError(String details) { @Override public void handleUnrecoverableError(String caption, String message, - String details, String url, String querySelector) { + String details, String url, String querySelector) { unrecoverableErrorHandled = true; } } - private static class TestApplicationConfiguration extends ApplicationConfiguration { + private static class TestApplicationConfiguration + extends ApplicationConfiguration { @Override public String getApplicationId() { return "test-application-id"; @@ -230,8 +231,7 @@ public void testMessageProcessing_moduleDependencyIsHandledBeforeApplyingChanges assertEquals(ResourceLoader.class.getName(), eventsOrder.sources.get(0)); // the second one is applying changes to StatTree - assertEquals(StateTree.class.getName(), - eventsOrder.sources.get(1)); + assertEquals(StateTree.class.getName(), eventsOrder.sources.get(1)); }); } @@ -308,11 +308,13 @@ public void testHandleJSON_uiTerminated_sessionExpiredMessageNotShown() { doAssert(() -> { // then: no session expire and unrecoverable error handling expected - assertFalse("Session Expired Message handling is not expected " + - "when the page is being redirected", + assertFalse( + "Session Expired Message handling is not expected " + + "when the page is being redirected", getSystemErrorHandler().sessionExpiredMessageHandled); - assertFalse("Unrecoverable Error Message handling was not " + - "expected when the page is being redirected", + assertFalse( + "Unrecoverable Error Message handling was not " + + "expected when the page is being redirected", getSystemErrorHandler().unrecoverableErrorHandled); assertEquals(UILifecycle.UIState.TERMINATED, getUILifecycle().getState()); @@ -340,11 +342,13 @@ public void testHandleJSON_uiTerminated_unrecoverableErrorMessageNotShown() { doAssert(() -> { // then: no session expire and unrecoverable error handling expected - assertFalse("Session Expired Message handling is not expected " + - "when the page is being redirected", + assertFalse( + "Session Expired Message handling is not expected " + + "when the page is being redirected", getSystemErrorHandler().sessionExpiredMessageHandled); - assertFalse("Unrecoverable Error Message handling was not " + - "expected when the page is being redirected", + assertFalse( + "Unrecoverable Error Message handling was not " + + "expected when the page is being redirected", getSystemErrorHandler().unrecoverableErrorHandled); assertEquals(UILifecycle.UIState.TERMINATED, getUILifecycle().getState()); @@ -376,7 +380,7 @@ public void testHandleJSON_sessionExpiredAndUIRunning_sessionExpiredMessageShown getSystemErrorHandler().unrecoverableErrorHandled); assertEquals(UILifecycle.UIState.TERMINATED, getUILifecycle().getState()); - }); + }, 300); } public void testHandleJSON_unrecoverableErrorAndUIRunning_unrecoverableErrorMessageShown() { @@ -423,8 +427,7 @@ private void doAssert(Runnable assertions) { doAssert(assertions, 100); } - private void doAssert(Runnable assertions, - int assertDelayInMillis) { + private void doAssert(Runnable assertions, int assertDelayInMillis) { delayTestFinish(500); new Timer() { @Override diff --git a/flow-server/src/main/java/com/vaadin/flow/server/communication/ServerRpcHandler.java b/flow-server/src/main/java/com/vaadin/flow/server/communication/ServerRpcHandler.java index fdca0765831..ff8b1ad47d7 100644 --- a/flow-server/src/main/java/com/vaadin/flow/server/communication/ServerRpcHandler.java +++ b/flow-server/src/main/java/com/vaadin/flow/server/communication/ServerRpcHandler.java @@ -298,6 +298,14 @@ public void handleRpc(UI ui, Reader reader, VaadinRequest request) getLogger().info( "Ignoring old duplicate message from the client. Expected: " + expectedId + ", got: " + requestId); + } else if (rpcRequest.isUnloadBeaconRequest()) { + getLogger().debug( + "Ignoring unexpected message id from the client on UNLOAD request. " + + "This could happen for example during login process, if concurrent requests " + + "are sent to the server and one of those changes the session identifier, " + + "causing an UIDL request to be rejected because of session expiration. " + + "Expected sync id: {}, got {}.", + expectedId, requestId); } else { /* * If the reason for ending up here is intermittent, then we diff --git a/flow-tests/test-frontend/vite-embedded-webcomponent-resync-longpolling/src/test/java/com/vaadin/viteapp/BasicComponentIT.java b/flow-tests/test-frontend/vite-embedded-webcomponent-resync-longpolling/src/test/java/com/vaadin/viteapp/BasicComponentIT.java index 071c7382da2..b8c26a148d9 100644 --- a/flow-tests/test-frontend/vite-embedded-webcomponent-resync-longpolling/src/test/java/com/vaadin/viteapp/BasicComponentIT.java +++ b/flow-tests/test-frontend/vite-embedded-webcomponent-resync-longpolling/src/test/java/com/vaadin/viteapp/BasicComponentIT.java @@ -28,11 +28,14 @@ import org.junit.Assert; import org.junit.Before; import org.junit.Test; +import org.openqa.selenium.By; import org.openqa.selenium.Keys; import org.openqa.selenium.StaleElementReferenceException; +import org.openqa.selenium.support.ui.ExpectedConditions; import com.vaadin.flow.server.InitParameters; import com.vaadin.flow.testutil.ChromeDeviceTest; +import com.vaadin.testbench.TestBenchElement; public class BasicComponentIT extends ChromeDeviceTest { @@ -63,19 +66,16 @@ public void session_resynced_webcomponent_is_active() throws Exception { Assert.assertEquals("Authentication failure", getAuthenticationResult()); + TestBenchElement input = $("login-form").first().$("input").first(); + // simulate expired session by invalidating current session session.invalidate(); - waitForWebComponent("login-form"); - - // init request to resynchronize expired session and recreate components - clickButton(); + // Wait for web component to be detached, session expiration message + // should be delivered by PUSH long polling connection + waitUntil(ExpectedConditions.stalenessOf(input)); - try { - // it seems WebDriver needs also sync to new session - setUsername(""); - } catch (StaleElementReferenceException ex) { - // NOP - } + waitForElementPresent(By.tagName("login-form")); + waitUntil(d -> "".equals(getAuthenticationResult())); // check if web component works again setUsername("admin"); diff --git a/flow-tests/test-frontend/vite-embedded-webcomponent-resync-ws/src/test/java/com/vaadin/viteapp/BasicComponentIT.java b/flow-tests/test-frontend/vite-embedded-webcomponent-resync-ws/src/test/java/com/vaadin/viteapp/BasicComponentIT.java index 071c7382da2..337ab52c64f 100644 --- a/flow-tests/test-frontend/vite-embedded-webcomponent-resync-ws/src/test/java/com/vaadin/viteapp/BasicComponentIT.java +++ b/flow-tests/test-frontend/vite-embedded-webcomponent-resync-ws/src/test/java/com/vaadin/viteapp/BasicComponentIT.java @@ -28,11 +28,14 @@ import org.junit.Assert; import org.junit.Before; import org.junit.Test; +import org.openqa.selenium.By; import org.openqa.selenium.Keys; import org.openqa.selenium.StaleElementReferenceException; +import org.openqa.selenium.support.ui.ExpectedConditions; import com.vaadin.flow.server.InitParameters; import com.vaadin.flow.testutil.ChromeDeviceTest; +import com.vaadin.testbench.TestBenchElement; public class BasicComponentIT extends ChromeDeviceTest { @@ -63,19 +66,15 @@ public void session_resynced_webcomponent_is_active() throws Exception { Assert.assertEquals("Authentication failure", getAuthenticationResult()); + TestBenchElement input = $("login-form").first().$("input").first(); // simulate expired session by invalidating current session session.invalidate(); - waitForWebComponent("login-form"); - - // init request to resynchronize expired session and recreate components - clickButton(); + // Wait for web component to be detached, session expiration message + // should be delivered over websocket + waitUntil(ExpectedConditions.stalenessOf(input)); - try { - // it seems WebDriver needs also sync to new session - setUsername(""); - } catch (StaleElementReferenceException ex) { - // NOP - } + waitForElementPresent(By.tagName("login-form")); + waitUntil(d -> "".equals(getAuthenticationResult())); // check if web component works again setUsername("admin"); diff --git a/flow-tests/test-frontend/vite-embedded-webcomponent-resync-wsxhr/src/test/java/com/vaadin/viteapp/BasicComponentIT.java b/flow-tests/test-frontend/vite-embedded-webcomponent-resync-wsxhr/src/test/java/com/vaadin/viteapp/BasicComponentIT.java index 071c7382da2..337ab52c64f 100644 --- a/flow-tests/test-frontend/vite-embedded-webcomponent-resync-wsxhr/src/test/java/com/vaadin/viteapp/BasicComponentIT.java +++ b/flow-tests/test-frontend/vite-embedded-webcomponent-resync-wsxhr/src/test/java/com/vaadin/viteapp/BasicComponentIT.java @@ -28,11 +28,14 @@ import org.junit.Assert; import org.junit.Before; import org.junit.Test; +import org.openqa.selenium.By; import org.openqa.selenium.Keys; import org.openqa.selenium.StaleElementReferenceException; +import org.openqa.selenium.support.ui.ExpectedConditions; import com.vaadin.flow.server.InitParameters; import com.vaadin.flow.testutil.ChromeDeviceTest; +import com.vaadin.testbench.TestBenchElement; public class BasicComponentIT extends ChromeDeviceTest { @@ -63,19 +66,15 @@ public void session_resynced_webcomponent_is_active() throws Exception { Assert.assertEquals("Authentication failure", getAuthenticationResult()); + TestBenchElement input = $("login-form").first().$("input").first(); // simulate expired session by invalidating current session session.invalidate(); - waitForWebComponent("login-form"); - - // init request to resynchronize expired session and recreate components - clickButton(); + // Wait for web component to be detached, session expiration message + // should be delivered over websocket + waitUntil(ExpectedConditions.stalenessOf(input)); - try { - // it seems WebDriver needs also sync to new session - setUsername(""); - } catch (StaleElementReferenceException ex) { - // NOP - } + waitForElementPresent(By.tagName("login-form")); + waitUntil(d -> "".equals(getAuthenticationResult())); // check if web component works again setUsername("admin"); diff --git a/flow-tests/test-frontend/vite-embedded-webcomponent-resync/src/test/java/com/vaadin/viteapp/BasicComponentIT.java b/flow-tests/test-frontend/vite-embedded-webcomponent-resync/src/test/java/com/vaadin/viteapp/BasicComponentIT.java index 071c7382da2..e7f6fb36670 100644 --- a/flow-tests/test-frontend/vite-embedded-webcomponent-resync/src/test/java/com/vaadin/viteapp/BasicComponentIT.java +++ b/flow-tests/test-frontend/vite-embedded-webcomponent-resync/src/test/java/com/vaadin/viteapp/BasicComponentIT.java @@ -28,11 +28,14 @@ import org.junit.Assert; import org.junit.Before; import org.junit.Test; +import org.openqa.selenium.By; import org.openqa.selenium.Keys; import org.openqa.selenium.StaleElementReferenceException; +import org.openqa.selenium.support.ui.ExpectedConditions; import com.vaadin.flow.server.InitParameters; import com.vaadin.flow.testutil.ChromeDeviceTest; +import com.vaadin.testbench.TestBenchElement; public class BasicComponentIT extends ChromeDeviceTest { @@ -63,19 +66,20 @@ public void session_resynced_webcomponent_is_active() throws Exception { Assert.assertEquals("Authentication failure", getAuthenticationResult()); + TestBenchElement input = $("login-form").first().$("input").first(); + // simulate expired session by invalidating current session session.invalidate(); - waitForWebComponent("login-form"); // init request to resynchronize expired session and recreate components clickButton(); - try { - // it seems WebDriver needs also sync to new session - setUsername(""); - } catch (StaleElementReferenceException ex) { - // NOP - } + // Wait for web component to be detached, session expiration message + // should be delivered by PUSH long polling connection + waitUntil(ExpectedConditions.stalenessOf(input)); + + waitForElementPresent(By.tagName("login-form")); + waitUntil(d -> "".equals(getAuthenticationResult())); // check if web component works again setUsername("admin"); diff --git a/flow-tests/test-root-context/src/test/java/com/vaadin/flow/uitest/ui/InternalErrorIT.java b/flow-tests/test-root-context/src/test/java/com/vaadin/flow/uitest/ui/InternalErrorIT.java index 79940757cd4..997203a9a51 100644 --- a/flow-tests/test-root-context/src/test/java/com/vaadin/flow/uitest/ui/InternalErrorIT.java +++ b/flow-tests/test-root-context/src/test/java/com/vaadin/flow/uitest/ui/InternalErrorIT.java @@ -90,6 +90,8 @@ public void enableSessionExpiredNotification_sessionExpired_notificationShown() // Just click on any button to make a request after killing the session clickButton(CLOSE_SESSION); + waitUntil(d -> isSessionExpiredNotificationPresent()); + Assert.assertTrue("After enabling the 'Session Expired' notification, " + "the page should not be refreshed " + "after killing the session", isMessageUpdated()); diff --git a/flow-tests/test-root-context/src/test/java/com/vaadin/flow/uitest/ui/RouterSessionExpirationIT.java b/flow-tests/test-root-context/src/test/java/com/vaadin/flow/uitest/ui/RouterSessionExpirationIT.java index addcb2d266e..c053f71654f 100644 --- a/flow-tests/test-root-context/src/test/java/com/vaadin/flow/uitest/ui/RouterSessionExpirationIT.java +++ b/flow-tests/test-root-context/src/test/java/com/vaadin/flow/uitest/ui/RouterSessionExpirationIT.java @@ -41,11 +41,13 @@ public void should_HaveANewSessionId_when_NavigationAfterSessionExpired() { navigateToSesssionExpireView(); // expired session causes page reload, after the page reload there will // be a new session - Assert.assertNotEquals(sessionId, getSessionId()); - sessionId = getSessionId(); + // Assert.assertNotEquals(sessionId, getSessionId()); + waitUntil(d -> !sessionId.equals(getSessionId())); + + String newSessionId = getSessionId(); navigateToAnotherView(); // session is preserved - Assert.assertEquals(sessionId, getSessionId()); + Assert.assertEquals(newSessionId, getSessionId()); } @Test