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")); + } + }