From 5ff8cf52cfa511363f5e0969b071c9d470765d88 Mon Sep 17 00:00:00 2001 From: Marco Collovati Date: Fri, 13 Dec 2024 15:54:41 +0100 Subject: [PATCH] fix: fix role checking when using websocket push (#20679) When using PUSH with websocket transport, the atmosphere wrapped request can be a no-op implementation whose isUserInRole method alwasy returns false, causing, for example, wrong access checking during navigation. This change falls back to Spring Securty for role checking when PUSH transport is websocket. It also fixes some tests in order to propagate the Spring Security context when starting Thread that perform UI operations. References psi#123 Part of #11026 --- .../server/auth/NavigationAccessControl.java | 4 +- .../flowsecuritywebsocket/AppViewIT.java | 14 +++++- .../spring/flowsecurity/views/AdminView.java | 11 ++-- .../spring/flowsecurity/views/PublicView.java | 12 +++-- .../flow/spring/flowsecurity/AppViewIT.java | 28 ++++++++--- .../flow/spring/security/SecurityUtil.java | 50 +++++++++++++++++-- 6 files changed, 98 insertions(+), 21 deletions(-) diff --git a/flow-server/src/main/java/com/vaadin/flow/server/auth/NavigationAccessControl.java b/flow-server/src/main/java/com/vaadin/flow/server/auth/NavigationAccessControl.java index 46937421001..f3c21d83df7 100644 --- a/flow-server/src/main/java/com/vaadin/flow/server/auth/NavigationAccessControl.java +++ b/flow-server/src/main/java/com/vaadin/flow/server/auth/NavigationAccessControl.java @@ -454,7 +454,7 @@ public NavigationContext createNavigationContext(Class navigationTarget, Objects.requireNonNull(vaadinService); return new NavigationContext(vaadinService.getRouter(), navigationTarget, new Location(path), RouteParameters.empty(), - vaadinRequest.getUserPrincipal(), - getRolesChecker(vaadinRequest), false); + getPrincipal(vaadinRequest), getRolesChecker(vaadinRequest), + false); } } diff --git a/flow-tests/vaadin-spring-tests/test-spring-security-flow-websocket/src/test/java/com/vaadin/flow/spring/flowsecuritywebsocket/AppViewIT.java b/flow-tests/vaadin-spring-tests/test-spring-security-flow-websocket/src/test/java/com/vaadin/flow/spring/flowsecuritywebsocket/AppViewIT.java index 48832d4aa83..285ec705641 100644 --- a/flow-tests/vaadin-spring-tests/test-spring-security-flow-websocket/src/test/java/com/vaadin/flow/spring/flowsecuritywebsocket/AppViewIT.java +++ b/flow-tests/vaadin-spring-tests/test-spring-security-flow-websocket/src/test/java/com/vaadin/flow/spring/flowsecuritywebsocket/AppViewIT.java @@ -14,7 +14,17 @@ public class AppViewIT extends com.vaadin.flow.spring.flowsecurity.AppViewIT { session expiration handler to redirect to the timeout page instead of the logout view, because the logout process is still ongoing. """) - public void logout_via_doLogin_redirects_to_logout() { - super.logout_via_doLogin_redirects_to_logout(); + public void logout_via_doLogoutURL_redirects_to_logout() { + super.logout_via_doLogoutURL_redirects_to_logout(); + } + + @Test + public void websocket_roles_checked_correctly_during_navigation() { + open("admin"); + loginAdmin(); + navigateTo(""); + assertRootPageShown(); + navigateTo("admin"); + assertAdminPageShown(ADMIN_FULLNAME); } } diff --git a/flow-tests/vaadin-spring-tests/test-spring-security-flow/src/main/java/com/vaadin/flow/spring/flowsecurity/views/AdminView.java b/flow-tests/vaadin-spring-tests/test-spring-security-flow/src/main/java/com/vaadin/flow/spring/flowsecurity/views/AdminView.java index 84cb7c17a7e..9ab120e6635 100644 --- a/flow-tests/vaadin-spring-tests/test-spring-security-flow/src/main/java/com/vaadin/flow/spring/flowsecurity/views/AdminView.java +++ b/flow-tests/vaadin-spring-tests/test-spring-security-flow/src/main/java/com/vaadin/flow/spring/flowsecurity/views/AdminView.java @@ -4,6 +4,9 @@ import java.util.concurrent.TimeUnit; +import org.springframework.security.concurrent.DelegatingSecurityContextRunnable; +import org.springframework.security.core.context.SecurityContextHolder; + import com.vaadin.flow.component.UI; import com.vaadin.flow.component.button.Button; import com.vaadin.flow.component.html.Div; @@ -37,14 +40,16 @@ public AdminView(SecurityUtils securityUtils) { accessRolePrefixedAdminPageFromThread.setId(ROLE_PREFIX_TEST_BUTTON_ID); accessRolePrefixedAdminPageFromThread.addClickListener(event -> { UI ui = event.getSource().getUI().get(); - new Thread(() -> { + Runnable doNavigation = () -> { try { TimeUnit.MILLISECONDS.sleep(100); ui.access(() -> ui.navigate(RolePrefixedAdminView.class)); } catch (InterruptedException e) { } - - }).start(); + }; + Runnable wrappedRunnable = new DelegatingSecurityContextRunnable( + doNavigation, SecurityContextHolder.getContext()); + new Thread(wrappedRunnable).start(); }); add(accessRolePrefixedAdminPageFromThread); } diff --git a/flow-tests/vaadin-spring-tests/test-spring-security-flow/src/main/java/com/vaadin/flow/spring/flowsecurity/views/PublicView.java b/flow-tests/vaadin-spring-tests/test-spring-security-flow/src/main/java/com/vaadin/flow/spring/flowsecurity/views/PublicView.java index 4ec74a40fbc..7b90e804d98 100644 --- a/flow-tests/vaadin-spring-tests/test-spring-security-flow/src/main/java/com/vaadin/flow/spring/flowsecurity/views/PublicView.java +++ b/flow-tests/vaadin-spring-tests/test-spring-security-flow/src/main/java/com/vaadin/flow/spring/flowsecurity/views/PublicView.java @@ -1,5 +1,8 @@ package com.vaadin.flow.spring.flowsecurity.views; +import org.springframework.security.concurrent.DelegatingSecurityContextRunnable; +import org.springframework.security.core.context.SecurityContextHolder; + import com.vaadin.flow.component.UI; import com.vaadin.flow.component.button.Button; import com.vaadin.flow.component.html.H1; @@ -36,7 +39,7 @@ public PublicView() { Button backgroundNavigation = new Button( "Navigate to admin view in 1 second", e -> { UI ui = e.getSource().getUI().get(); - new Thread(() -> { + Runnable navigateToAdmin = () -> { try { Thread.sleep(1000); } catch (InterruptedException e1) { @@ -44,8 +47,11 @@ public PublicView() { ui.access(() -> { ui.navigate(AdminView.class); }); - - }).start(); + }; + Runnable wrappedRunnable = new DelegatingSecurityContextRunnable( + navigateToAdmin, + SecurityContextHolder.getContext()); + new Thread(wrappedRunnable).start(); }); backgroundNavigation.setId(BACKGROUND_NAVIGATION_ID); add(backgroundNavigation); diff --git a/flow-tests/vaadin-spring-tests/test-spring-security-flow/src/test/java/com/vaadin/flow/spring/flowsecurity/AppViewIT.java b/flow-tests/vaadin-spring-tests/test-spring-security-flow/src/test/java/com/vaadin/flow/spring/flowsecurity/AppViewIT.java index b851b7e8437..7e1e2be3dc5 100644 --- a/flow-tests/vaadin-spring-tests/test-spring-security-flow/src/test/java/com/vaadin/flow/spring/flowsecurity/AppViewIT.java +++ b/flow-tests/vaadin-spring-tests/test-spring-security-flow/src/test/java/com/vaadin/flow/spring/flowsecurity/AppViewIT.java @@ -12,6 +12,7 @@ import org.apache.commons.io.IOUtils; import org.junit.Assert; import org.junit.Test; +import org.openqa.selenium.By; import org.openqa.selenium.WebDriver; import com.vaadin.flow.component.button.testbench.ButtonElement; @@ -22,12 +23,12 @@ public class AppViewIT extends AbstractIT { - private static final String LOGIN_PATH = "my/login/page"; - private static final String USER_FULLNAME = "John the User"; - private static final String ADMIN_FULLNAME = "Emma the Admin"; + protected static final String LOGIN_PATH = "my/login/page"; + protected static final String USER_FULLNAME = "John the User"; + protected static final String ADMIN_FULLNAME = "Emma the Admin"; private void logout() { - if (!$(ButtonElement.class).attribute("id", "logout").exists()) { + if (!$(ButtonElement.class).withAttribute("id", "logout").exists()) { open(""); assertRootPageShown(); } @@ -298,7 +299,7 @@ public void navigate_in_thread_with_access() { // https://github.com/vaadin/flow/issues/7323 @Test - public void logout_via_doLogin_redirects_to_logout() { + public void logout_via_doLogoutURL_redirects_to_logout() { open(LOGIN_PATH); loginAdmin(); navigateTo("admin"); @@ -340,11 +341,11 @@ private void navigateToClientMenuList() { assertPathShown("menu-list"); } - private void navigateTo(String path) { + protected void navigateTo(String path) { navigateTo(path, true); } - private void navigateTo(String path, boolean assertPathShown) { + protected void navigateTo(String path, boolean assertPathShown) { getMainView().$("a").attribute("href", path).first().click(); if (assertPathShown) { assertPathShown(path); @@ -352,6 +353,7 @@ private void navigateTo(String path, boolean assertPathShown) { } private TestBenchElement getMainView() { + waitForClientRouter(); return waitUntil(driver -> $("*").id("main-view")); } @@ -406,4 +408,16 @@ private void waitForUploads(UploadElement element, int maxSeconds) { getCommandExecutor().getDriver().executeAsyncScript(script, element); } + /* + * The same driver is used to access both Vaadin views and static resources. + * Static caching done by #isClientRouter can cause some tests to be flaky. + */ + protected void waitForClientRouter() { + boolean hasClientRouter = (boolean) executeScript( + "return !!window.Vaadin.Flow.clients.TypeScript"); + if (hasClientRouter) { + waitForElementPresent(By.cssSelector("#outlet > *")); + } + } + } diff --git a/vaadin-spring/src/main/java/com/vaadin/flow/spring/security/SecurityUtil.java b/vaadin-spring/src/main/java/com/vaadin/flow/spring/security/SecurityUtil.java index 5641c2d7209..46e4a954da7 100644 --- a/vaadin-spring/src/main/java/com/vaadin/flow/spring/security/SecurityUtil.java +++ b/vaadin-spring/src/main/java/com/vaadin/flow/spring/security/SecurityUtil.java @@ -18,9 +18,16 @@ import java.security.Principal; import java.util.Optional; +import java.util.concurrent.atomic.AtomicReference; +import java.util.function.Function; import java.util.function.Predicate; +import org.springframework.security.concurrent.DelegatingSecurityContextRunnable; +import org.springframework.security.core.Authentication; +import org.springframework.security.core.context.SecurityContextImpl; + import com.vaadin.flow.di.Lookup; +import com.vaadin.flow.server.HandlerHelper; import com.vaadin.flow.server.VaadinRequest; import com.vaadin.flow.server.VaadinService; import com.vaadin.flow.spring.AuthenticationUtil; @@ -42,7 +49,9 @@ class SecurityUtil { * if no user is currently logged in */ static Principal getPrincipal(VaadinRequest request) { - if (request == null) { + boolean isWebsocketPush = isWebsocketPush(request); + if (request == null + || (isWebsocketPush && request.getUserPrincipal() == null)) { return AuthenticationUtil.getSecurityHolderAuthentication(); } return request.getUserPrincipal(); @@ -58,15 +67,40 @@ static Principal getPrincipal(VaadinRequest request) { * the user is included in that role */ static Predicate getRolesChecker(VaadinRequest request) { - if (request == null) { - return Optional.ofNullable(VaadinService.getCurrent()) + boolean isWebsocketPush = isWebsocketPush(request); + + // Role checks on PUSH request works out of the box only happen if + // transport is not WEBSOCKET. + // For websocket PUSH, HttServletRequest#isUserInRole method in + // Atmosphere HTTP request wrapper always returns, so we need to + // fall back to Spring Security. + if (request == null || isWebsocketPush) { + AtomicReference> roleCheckerHolder = new AtomicReference<>(); + Runnable roleCheckerLookup = () -> roleCheckerHolder.set(Optional + .ofNullable(request).map(VaadinRequest::getService) + .or(() -> Optional.ofNullable(VaadinService.getCurrent())) .map(service -> service.getContext() .getAttribute(Lookup.class)) .map(lookup -> lookup.lookup(VaadinRolePrefixHolder.class)) .map(VaadinRolePrefixHolder::getRolePrefix) .map(AuthenticationUtil::getSecurityHolderRoleChecker) .orElseGet( - AuthenticationUtil::getSecurityHolderRoleChecker)::apply; + AuthenticationUtil::getSecurityHolderRoleChecker)); + + Authentication authentication = AuthenticationUtil + .getSecurityHolderAuthentication(); + // Spring Security context holder might not have been initialized + // for thread handling websocket message. If so, create a temporary + // security context based on the handshake request principal. + if (authentication == null && isWebsocketPush && request + .getUserPrincipal() instanceof Authentication requestAuthentication) { + roleCheckerLookup = new DelegatingSecurityContextRunnable( + roleCheckerLookup, + new SecurityContextImpl(requestAuthentication)); + } + + roleCheckerLookup.run(); + return roleCheckerHolder.get()::apply; } // Update active role prefix if it's not set yet. @@ -79,4 +113,12 @@ static Predicate getRolesChecker(VaadinRequest request) { return request::isUserInRole; } + private static boolean isWebsocketPush(VaadinRequest request) { + return request != null + && HandlerHelper.isRequestType(request, + HandlerHelper.RequestType.PUSH) + && "websocket" + .equals(request.getHeader("X-Atmosphere-Transport")); + } + }