fix: fix role checking when using websocket push (#20679) (#20711)

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
vaadin · Dec 16, 2024 · 0112560 · 0112560
1 parent 500e859
commit 0112560
Show file tree

Hide file tree

Showing 6 changed files with 97 additions and 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
@@ -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-websocket/src/test/java/com/vaadin/flow/spring/flowsecuritywebsocket/AppViewIT.java b/...-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/...ring-security-flow/src/main/java/com/vaadin/flow/spring/flowsecurity/views/AdminView.java b/...ring-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/...ing-security-flow/src/main/java/com/vaadin/flow/spring/flowsecurity/views/PublicView.java b/...ing-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,16 +39,19 @@ 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) {
                         }
                         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/...est-spring-security-flow/src/test/java/com/vaadin/flow/spring/flowsecurity/AppViewIT.java b/...est-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,18 +341,19 @@ 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);
         }
     }
 
     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/...n-spring/src/main/java/com/vaadin/flow/spring/security/SpringNavigationAccessControl.java b/...n-spring/src/main/java/com/vaadin/flow/spring/security/SpringNavigationAccessControl.java
@@ -3,9 +3,16 @@
 import java.security.Principal;
 import java.util.Collection;
 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.server.auth.AccessCheckDecisionResolver;
@@ -55,23 +62,50 @@ public SpringNavigationAccessControl(
 
     @Override
     protected Principal getPrincipal(VaadinRequest request) {
-        if (request == null) {
+        boolean isWebsocketPush = isWebsocketPush(request);
+        if (request == null
+                || (isWebsocketPush && request.getUserPrincipal() == null)) {
             return AuthenticationUtil.getSecurityHolderAuthentication();
         }
         return super.getPrincipal(request);
     }
 
     @Override
     protected Predicate<String> 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<Function<String, Boolean>> 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.
@@ -84,4 +118,11 @@ protected Predicate<String> getRolesChecker(VaadinRequest request) {
         return super.getRolesChecker(request);
     }
 
+    private static boolean isWebsocketPush(VaadinRequest request) {
+        return request != null
+                && HandlerHelper.isRequestType(request,
+                        HandlerHelper.RequestType.PUSH)
+                && "websocket"
+                        .equals(request.getHeader("X-Atmosphere-Transport"));
+    }
 }