This is an automated email from the ASF dual-hosted git repository. dashorst pushed a commit to branch wicket-10.x in repository https://gitbox.apache.org/repos/asf/wicket.git
commit 4a2b39a1e66dd07bf42210e421066614c2ccda5c Author: Emond Papegaaij <[email protected]> AuthorDate: Thu Sep 24 20:29:53 2026 +0200 Ask IAuthorizationStrategy about the WebSocketResource a client connects to isResourceAuthorized() is consulted in one place, the constructor of ResourceRequestHandler, and no route to a WebSocketResource reaches it. AbstractWebSocketProcessor takes the name of a shared resource from a handshake parameter the client chooses and delivers every event to it through WebSocketMessageBroadcastHandler, which resolves the name and calls onPayload() directly. Because WebSocketResource#respond() is final and empty, that is not one transport out of two escaping a check: a connection is the only functional route to such a resource, and the strategy was never asked on it, so an @AuthorizeResource annotation on the class had no effect at all. Ask when the connection is opened, in broadcastMessage(), for the ConnectedMessage that onConnect() broadcasts. That is the point on this transport where the Application, the Session and the RequestCycle are all live and are restored again afterwards; onConnect() itself has only the Application, and a role strategy needs the session. It is also early enough: JavaxWebSocketProcessor registers its message handlers after onConnect() returns, so no frame of the client can reach a resource whose connection was refused. The question is the one ResourceRequestHandler#authorize() puts, with the same empty parameters - the query of a WebSocket URL carries only Wicket's own connection parameters, which the client chooses, and they are not parameters of a request to the resource. The refusal cannot be the same. The default IUnauthorizedResourceRequestListener ends at WebResponse#sendError, which WebSocketResponse only logs, leaving the connection open; so a refused connection is removed from the registry and closed with 1008, the policy violation code a rejected connection already uses. No AbortedMessage is broadcast, and the registry removal means the close that follows finds no connection either, so a refused client reaches none of the callbacks - not onConnect(), not onAbort(), not onClose(). A connection filter that rejects still aborts exactly as before. The decision is taken per connection, not per message. This resource exists to carry many messages without the page lock a WebSocketBehavior takes, and putting a registry lookup and a strategy call in front of every frame would tax what it is for. It also costs little: privileges withdrawn while a socket is open are not noticed, but this transport resolves its session from a copy made during the handshake and does not see a session that was replaced, so a per-frame check would not have seen the common revocation either. Applications that need that must close the connection themselves. The page route is untouched: a page was authorized when it was instantiated and rendered, and its payload is delivered to a behavior of that very page. WebSocketTester gains isConnectionOpen(), getCloseCode() and getCloseReason() so a test can observe a refusal, which is otherwise invisible - the refusal happens inside its own constructor, where an anonymous subclass's fields are not yet initialised, so a callback could not carry it. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> --- .../role/annotations/AuthorizeResource.java | 4 +- .../authorization/IAuthorizationStrategy.java | 12 +- .../wicket/protocol/ws/WebSocketSettings.java | 9 + .../ws/api/AbstractWebSocketProcessor.java | 73 ++++++ .../wicket/protocol/ws/api/WebSocketResource.java | 29 ++ .../wicket-native-websocket-tester/pom.xml | 5 + .../ws/tester/TestWebSocketConnection.java | 20 ++ .../protocol/ws/tester/TestWebSocketProcessor.java | 17 +- .../wicket/protocol/ws/tester/WebSocketTester.java | 31 ++- .../AnnotationsRoleWebSocketResourceTest.java | 200 ++++++++++++++ .../protocol/ws/tester/TestWebSocketResource.java | 21 ++ .../tester/WebSocketResourceAuthorizationTest.java | 291 +++++++++++++++++++++ .../ws/tester/WebSocketTesterProcessorTest.java | 2 +- .../ws/tester/WebSocketTesterResourceTest.java | 3 +- .../nativewebsockets/nativewebsockets_2.adoc | 2 + 15 files changed, 709 insertions(+), 10 deletions(-) diff --git a/wicket-auth-roles/src/main/java/org/apache/wicket/authroles/authorization/strategies/role/annotations/AuthorizeResource.java b/wicket-auth-roles/src/main/java/org/apache/wicket/authroles/authorization/strategies/role/annotations/AuthorizeResource.java index 34cddfe76d..52d6bf8ec8 100644 --- a/wicket-auth-roles/src/main/java/org/apache/wicket/authroles/authorization/strategies/role/annotations/AuthorizeResource.java +++ b/wicket-auth-roles/src/main/java/org/apache/wicket/authroles/authorization/strategies/role/annotations/AuthorizeResource.java @@ -29,7 +29,9 @@ import java.lang.annotation.Target; * <p> * It applies to every request that serves the annotated resource: one requested by the URL of a * mounted or shared {@link org.apache.wicket.request.resource.ResourceReference}, and one served - * through a component that was handed the resource itself. + * through a component that was handed the resource itself. On a WebSocketResource of + * wicket-native-websocket it decides whether a client may open a connection to it, which is the + * only way such a resource is reached. * * @author Carl-Eric Menzel * @see org.apache.wicket.authorization.IAuthorizationStrategy diff --git a/wicket-core/src/main/java/org/apache/wicket/authorization/IAuthorizationStrategy.java b/wicket-core/src/main/java/org/apache/wicket/authorization/IAuthorizationStrategy.java index 11159fe98c..de155daeb7 100644 --- a/wicket-core/src/main/java/org/apache/wicket/authorization/IAuthorizationStrategy.java +++ b/wicket-core/src/main/java/org/apache/wicket/authorization/IAuthorizationStrategy.java @@ -115,16 +115,24 @@ public interface IAuthorizationStrategy * {@link org.apache.wicket.markup.html.form.ImageButton}, or the AjaxDownloadBehavior of * wicket-extensions. * <p> + * It is also asked about a shared WebSocketResource of wicket-native-websocket, once, when a + * client opens a WebSocket connection naming it. That resource serves nothing over HTTP, so a + * connection is the only way to reach it, and the decision is taken for the connection rather + * than for each message on it. + * <p> * Returning {@code false} hands the request to the * {@link IUnauthorizedResourceRequestListener} of the security settings, which decides how it - * is refused; the default one answers 403. + * is refused; the default one answers 403. A refused WebSocket connection is the exception: no + * status code can reach a client over that transport, so the connection is closed instead and + * the listener is not involved. * * @param resource * The resource that should be processed * @param parameters * The request parameters, never {@code null}; they are empty for a resource served * through the component that holds it, because a listener URL carries no resource - * parameters + * parameters, and for a WebSocketResource, because the query of a WebSocket URL + * carries only the connection parameters of the framework itself * @return {@code true} if the request to this resource is allowed. */ boolean isResourceAuthorized(IResource resource, PageParameters parameters); diff --git a/wicket-native-websocket/wicket-native-websocket-core/src/main/java/org/apache/wicket/protocol/ws/WebSocketSettings.java b/wicket-native-websocket/wicket-native-websocket-core/src/main/java/org/apache/wicket/protocol/ws/WebSocketSettings.java index c9320903dd..bec1898b6e 100644 --- a/wicket-native-websocket/wicket-native-websocket-core/src/main/java/org/apache/wicket/protocol/ws/WebSocketSettings.java +++ b/wicket-native-websocket/wicket-native-websocket-core/src/main/java/org/apache/wicket/protocol/ws/WebSocketSettings.java @@ -270,6 +270,15 @@ public class WebSocketSettings /** * Sets the filter for checking the incoming connections + * <p> + * This is the policy of the application over the handshake itself - the origin it came from, the + * headers it carries - and there is room for one filter, so a second call replaces the first; + * use a {@link WebSocketConnectionFilterCollection} to keep both. + * <p> + * It is additional to the authorization of a shared + * {@link org.apache.wicket.protocol.ws.api.WebSocketResource}, which the + * {@link org.apache.wicket.authorization.IAuthorizationStrategy} decides and which installing a + * filter here neither replaces nor switches off. * * @param connectionFilter * the filter for checking the incoming connections diff --git a/wicket-native-websocket/wicket-native-websocket-core/src/main/java/org/apache/wicket/protocol/ws/api/AbstractWebSocketProcessor.java b/wicket-native-websocket/wicket-native-websocket-core/src/main/java/org/apache/wicket/protocol/ws/api/AbstractWebSocketProcessor.java index c9fa0fb4ba..0bd995dd8b 100644 --- a/wicket-native-websocket/wicket-native-websocket-core/src/main/java/org/apache/wicket/protocol/ws/api/AbstractWebSocketProcessor.java +++ b/wicket-native-websocket/wicket-native-websocket-core/src/main/java/org/apache/wicket/protocol/ws/api/AbstractWebSocketProcessor.java @@ -24,6 +24,7 @@ import org.apache.wicket.MarkupContainer; import org.apache.wicket.Page; import org.apache.wicket.Session; import org.apache.wicket.ThreadContext; +import org.apache.wicket.authorization.IAuthorizationStrategy; import org.apache.wicket.markup.IMarkupResourceStreamProvider; import org.apache.wicket.markup.html.WebPage; import org.apache.wicket.page.IPageManager; @@ -58,6 +59,9 @@ import org.apache.wicket.request.cycle.RequestCycle; import org.apache.wicket.request.cycle.RequestCycleContext; import org.apache.wicket.request.http.WebRequest; import org.apache.wicket.request.http.WebResponse; +import org.apache.wicket.request.mapper.parameter.PageParameters; +import org.apache.wicket.request.resource.IResource; +import org.apache.wicket.request.resource.SharedResourceReference; import org.apache.wicket.session.ISessionStore; import org.apache.wicket.util.lang.Args; import org.apache.wicket.util.lang.Checks; @@ -82,6 +86,14 @@ public abstract class AbstractWebSocketProcessor implements IWebSocketProcessor */ static final int NO_PAGE_ID = -1; + /** + * The reason given to a client whose shared {@link WebSocketResource} the + * {@link IAuthorizationStrategy} refused. It is sent with close code + * {@value WebSocketConnectionOriginFilter#POLICY_VIOLATION_ERROR_CODE}, the policy violation + * code of RFC 6455, which is the one a rejected connection already uses. + */ + static final String NOT_AUTHORIZED = "Not authorized"; + private final WebRequest webRequest; private final int pageId; private final String context; @@ -162,6 +174,12 @@ public abstract class AbstractWebSocketProcessor implements IWebSocketProcessor /** * A helper that registers the opened connection in the application-level registry. + * <p> + * The connection may already be closed again when this returns. Broadcasting the + * {@link ConnectedMessage} is where the {@link IAuthorizationStrategy} is asked about a shared + * {@link WebSocketResource}, and a refusal closes the connection there. This happens before the + * transport registers its message handlers, so no frame of the client can be delivered to a + * resource that was refused. * * @param connection * the web socket connection to use to communicate with the client @@ -220,6 +238,16 @@ public abstract class AbstractWebSocketProcessor implements IWebSocketProcessor * components/behaviors write in the WebSocketRequestHandler will be ignored because the protocol * doesn't expect response from the user. * </p> + * <p> + * A ConnectedMessage for a connection that named a shared {@link WebSocketResource} is where + * that resource is authorized. When the {@link IAuthorizationStrategy} refuses it, this + * method removes the connection from the registry and closes it with + * {@value WebSocketConnectionOriginFilter#POLICY_VIOLATION_ERROR_CODE} instead of + * broadcasting, so none of the callbacks of the resource is reached - not + * {@code onConnect()}, and not the {@code onClose()} that follows the close either, because + * the connection is no longer in the registry to be found. The decision is taken once, per + * connection; it is not repeated for the messages that follow on an accepted one. + * </p> * * @param message * the message to broadcast @@ -267,6 +295,15 @@ public abstract class AbstractWebSocketProcessor implements IWebSocketProcessor return; } + if (message instanceof ConnectedMessage && isResourceAuthorized(session) == false) + { + LOG.warn("The IAuthorizationStrategy refused the WebSocketResource named '{}'. " + + "Closing the connection with key '{}'.", resourceName, connection.getKey()); + connectionRegistry.removeConnection(application, sessionId, connection.getKey()); + connection.close(WebSocketConnectionOriginFilter.POLICY_VIOLATION_ERROR_CODE, NOT_AUTHORIZED); + return; + } + IPageManager pageManager = session.getPageManager(); Page page = getPage(pageManager); @@ -340,6 +377,42 @@ public abstract class AbstractWebSocketProcessor implements IWebSocketProcessor return requestCycle; } + /** + * Asks the {@link IAuthorizationStrategy} whether the client that opened this connection may + * use the shared {@link WebSocketResource} it named in the handshake. + * <p> + * A connection to a page is never asked about and always allowed here: a page was authorized + * when it was instantiated and rendered, and the message is delivered to a behavior of that + * very page, out of the page store of the caller's own session. + * <p> + * The question is the one the constructor of + * {@link org.apache.wicket.request.handler.resource.ResourceRequestHandler} puts for the HTTP + * routes to a resource, with the same empty parameters: the query of a WebSocket URL carries + * only Wicket's own connection parameters, which the client chooses, and they are not the + * parameters of a request to the resource. The refusal is not the same, because a status code + * cannot be delivered over this transport - see + * {@link #broadcastMessage(IWebSocketMessage, IWebSocketConnection, boolean, long)}. + * + * @param session + * the session of the client that opened the connection + * @return {@code false} only when a shared resource was named and the strategy refused it + */ + private boolean isResourceAuthorized(Session session) + { + if (Strings.isEmpty(resourceName)) + { + return true; + } + + IAuthorizationStrategy authorizationStrategy = session != null + ? session.getAuthorizationStrategy() + : application.getSecuritySettings().getAuthorizationStrategy(); + + IResource resource = new SharedResourceReference(resourceName).getResource(); + + return authorizationStrategy.isResourceAuthorized(resource, new PageParameters()); + } + /** * @param pageManager * the page manager to use when finding a page by id diff --git a/wicket-native-websocket/wicket-native-websocket-core/src/main/java/org/apache/wicket/protocol/ws/api/WebSocketResource.java b/wicket-native-websocket/wicket-native-websocket-core/src/main/java/org/apache/wicket/protocol/ws/api/WebSocketResource.java index d5bbed966d..ad3a387c80 100644 --- a/wicket-native-websocket/wicket-native-websocket-core/src/main/java/org/apache/wicket/protocol/ws/api/WebSocketResource.java +++ b/wicket-native-websocket/wicket-native-websocket-core/src/main/java/org/apache/wicket/protocol/ws/api/WebSocketResource.java @@ -35,6 +35,27 @@ import org.apache.wicket.request.resource.IResource; /** * An IResource that can be used as WebSocket endpoint + * <p> + * Unlike other resources this one is never served over HTTP - {@link #respond(Attributes)} is final + * and empty - so a WebSocket connection is the only way to reach it. Register it as a shared + * resource and let a {@link BaseWebSocketBehavior} name it, and every callback below is driven by + * the client that opened the connection. + * <p> + * The {@link org.apache.wicket.authorization.IAuthorizationStrategy} of the application is asked + * whether that client may use this resource, through + * {@link org.apache.wicket.authorization.IAuthorizationStrategy#isResourceAuthorized(IResource, org.apache.wicket.request.mapper.parameter.PageParameters)}, + * so a restriction on it - for example the {@code AuthorizeResource} annotation of wicket-auth-roles + * - is enforced. A refused client is disconnected and reaches none of the callbacks, not even + * {@link #onClose(org.apache.wicket.protocol.ws.api.message.ClosedMessage)} or + * {@link #onAbort(org.apache.wicket.protocol.ws.api.message.AbortedMessage)}. + * <p> + * The question is put once, when the connection is opened, and is not repeated for the messages + * that follow: a connection that was allowed stays allowed for as long as it is open, even if the + * privileges of the session change meanwhile. That is deliberate - this resource exists to carry + * many messages cheaply, without the page lock a {@link WebSocketBehavior} takes - and it is what + * this transport can honestly offer, since it resolves its session from a copy made during the + * handshake and does not see a session that was replaced. Close the connection from the + * application when the privileges it was opened with are withdrawn. */ public abstract class WebSocketResource implements IResource { @@ -167,6 +188,14 @@ public abstract class WebSocketResource implements IResource { } + /** + * Does nothing, deliberately: a WebSocketResource has no representation to serve over HTTP, and + * a request for one writes an empty response. Its callbacks are reached over a WebSocket + * connection instead, which is where the + * {@link org.apache.wicket.authorization.IAuthorizationStrategy} is asked about it - so unlike + * the note on {@link IResource#respond(Attributes)}, nothing is served here without being + * authorized, because nothing is served here at all. + */ @Override public final void respond(Attributes attributes) { diff --git a/wicket-native-websocket/wicket-native-websocket-tester/pom.xml b/wicket-native-websocket/wicket-native-websocket-tester/pom.xml index de5908a062..51d7920e8d 100644 --- a/wicket-native-websocket/wicket-native-websocket-tester/pom.xml +++ b/wicket-native-websocket/wicket-native-websocket-tester/pom.xml @@ -53,5 +53,10 @@ <scope>compile</scope> <optional>false</optional> </dependency> + <dependency> + <groupId>org.apache.wicket</groupId> + <artifactId>wicket-auth-roles</artifactId> + <scope>test</scope> + </dependency> </dependencies> </project> diff --git a/wicket-native-websocket/wicket-native-websocket-tester/src/main/java/org/apache/wicket/protocol/ws/tester/TestWebSocketConnection.java b/wicket-native-websocket/wicket-native-websocket-tester/src/main/java/org/apache/wicket/protocol/ws/tester/TestWebSocketConnection.java index 450fe54415..b99f22bcf9 100644 --- a/wicket-native-websocket/wicket-native-websocket-tester/src/main/java/org/apache/wicket/protocol/ws/tester/TestWebSocketConnection.java +++ b/wicket-native-websocket/wicket-native-websocket-tester/src/main/java/org/apache/wicket/protocol/ws/tester/TestWebSocketConnection.java @@ -35,6 +35,8 @@ abstract class TestWebSocketConnection implements IWebSocketConnection private final String sessionId; private final IKey registryKey; private boolean isOpen = true; + private int closeCode = -1; + private String closeReason; public TestWebSocketConnection(WebApplication application, String sessionId, IKey registryKey) { @@ -53,6 +55,24 @@ abstract class TestWebSocketConnection implements IWebSocketConnection public void close(int code, String reason) { isOpen = false; + closeCode = code; + closeReason = reason; + } + + /** + * @return the code this connection was closed with, or {@code -1} when it was never closed + */ + int getCloseCode() + { + return closeCode; + } + + /** + * @return the reason this connection was closed with, or {@code null} when it was never closed + */ + String getCloseReason() + { + return closeReason; } @Override diff --git a/wicket-native-websocket/wicket-native-websocket-tester/src/main/java/org/apache/wicket/protocol/ws/tester/TestWebSocketProcessor.java b/wicket-native-websocket/wicket-native-websocket-tester/src/main/java/org/apache/wicket/protocol/ws/tester/TestWebSocketProcessor.java index 9adf77a75b..cecce065fe 100644 --- a/wicket-native-websocket/wicket-native-websocket-tester/src/main/java/org/apache/wicket/protocol/ws/tester/TestWebSocketProcessor.java +++ b/wicket-native-websocket/wicket-native-websocket-tester/src/main/java/org/apache/wicket/protocol/ws/tester/TestWebSocketProcessor.java @@ -36,6 +36,8 @@ import org.apache.wicket.util.tester.WicketTester; */ abstract class TestWebSocketProcessor extends AbstractWebSocketProcessor { + private TestWebSocketConnection testConnection; + /** * * Constructor. @@ -139,7 +141,7 @@ abstract class TestWebSocketProcessor extends AbstractWebSocketProcessor @Override public void onOpen(Object connection) { - onConnect(new TestWebSocketConnection(getApplication(), getSessionId(), getRegistryKey()) { + testConnection = new TestWebSocketConnection(getApplication(), getSessionId(), getRegistryKey()) { @Override protected void onOutMessage(String message) @@ -170,7 +172,18 @@ abstract class TestWebSocketProcessor extends AbstractWebSocketProcessor { TestWebSocketProcessor.this.broadcastMessage(message); } - }); + }; + + onConnect(testConnection); + } + + /** + * @return the connection this processor opened, or {@code null} when {@link #onOpen(Object)} + * has not been called yet + */ + TestWebSocketConnection getTestConnection() + { + return testConnection; } /** diff --git a/wicket-native-websocket/wicket-native-websocket-tester/src/main/java/org/apache/wicket/protocol/ws/tester/WebSocketTester.java b/wicket-native-websocket/wicket-native-websocket-tester/src/main/java/org/apache/wicket/protocol/ws/tester/WebSocketTester.java index 98e0238cf1..bb86f0268b 100644 --- a/wicket-native-websocket/wicket-native-websocket-tester/src/main/java/org/apache/wicket/protocol/ws/tester/WebSocketTester.java +++ b/wicket-native-websocket/wicket-native-websocket-tester/src/main/java/org/apache/wicket/protocol/ws/tester/WebSocketTester.java @@ -20,7 +20,6 @@ import org.apache.wicket.Application; import org.apache.wicket.Page; import org.apache.wicket.protocol.http.WebApplication; import org.apache.wicket.protocol.ws.WebSocketSettings; -import org.apache.wicket.protocol.ws.api.IWebSocketProcessor; import org.apache.wicket.protocol.ws.api.WebSocketPushBroadcaster; import org.apache.wicket.protocol.ws.api.message.ConnectedMessage; import org.apache.wicket.protocol.ws.api.message.IWebSocketPushMessage; @@ -35,7 +34,7 @@ import org.apache.wicket.util.tester.WicketTester; */ public class WebSocketTester { - private final IWebSocketProcessor socketProcessor; + private final TestWebSocketProcessor socketProcessor; /** * Constructor. @@ -182,6 +181,34 @@ public class WebSocketTester socketProcessor.onClose(0, "Closed by WebSocketTester"); } + /** + * @return {@code true} while the connection this tester opened is still open. A connection the + * application refused - by an {@link org.apache.wicket.protocol.ws.api.IWebSocketConnectionFilter} + * or because the {@link org.apache.wicket.authorization.IAuthorizationStrategy} did not + * allow the {@link org.apache.wicket.protocol.ws.api.WebSocketResource} it named - is + * already closed when the constructor returns. + */ + public boolean isConnectionOpen() + { + return socketProcessor.getTestConnection().isOpen(); + } + + /** + * @return the code the connection was closed with, or {@code -1} while it is still open + */ + public int getCloseCode() + { + return socketProcessor.getTestConnection().getCloseCode(); + } + + /** + * @return the reason the connection was closed with, or {@code null} while it is still open + */ + public String getCloseReason() + { + return socketProcessor.getTestConnection().getCloseReason(); + } + /** * A callback method which may be overritten to receive messages pushed by the server * diff --git a/wicket-native-websocket/wicket-native-websocket-tester/src/test/java/org/apache/wicket/protocol/ws/tester/AnnotationsRoleWebSocketResourceTest.java b/wicket-native-websocket/wicket-native-websocket-tester/src/test/java/org/apache/wicket/protocol/ws/tester/AnnotationsRoleWebSocketResourceTest.java new file mode 100644 index 0000000000..ec04538ba1 --- /dev/null +++ b/wicket-native-websocket/wicket-native-websocket-tester/src/test/java/org/apache/wicket/protocol/ws/tester/AnnotationsRoleWebSocketResourceTest.java @@ -0,0 +1,200 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You 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 org.apache.wicket.protocol.ws.tester; + +import java.io.Serializable; +import java.util.concurrent.atomic.AtomicBoolean; + +import org.apache.wicket.authroles.authorization.strategies.role.IRoleCheckingStrategy; +import org.apache.wicket.authroles.authorization.strategies.role.RoleAuthorizationStrategy; +import org.apache.wicket.authroles.authorization.strategies.role.Roles; +import org.apache.wicket.authroles.authorization.strategies.role.annotations.AuthorizeResource; +import org.apache.wicket.protocol.ws.api.WebSocketRequestHandler; +import org.apache.wicket.protocol.ws.api.WebSocketResource; +import org.apache.wicket.protocol.ws.api.message.ClosedMessage; +import org.apache.wicket.protocol.ws.api.message.ConnectedMessage; +import org.apache.wicket.protocol.ws.api.message.TextMessage; +import org.apache.wicket.util.string.Strings; +import org.apache.wicket.util.tester.WicketTester; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * A WebSocketResource reserved for a role with {@link AuthorizeResource} must not be usable by a + * client who does not hold that role. A WebSocket connection is the only way such a resource is + * reached, so this is where the annotation has to bite. + */ +class AnnotationsRoleWebSocketResourceTest +{ + private static final String ADMIN = "admin-resource"; + private static final String OPEN = "open-resource"; + + private static final String EXPECTED_TEXT = "expected text"; + + private AdminWebSocketResource admin; + private OpenWebSocketResource open; + private WicketTester tester; + + @BeforeEach + public void before() + { + admin = new AdminWebSocketResource(); + open = new OpenWebSocketResource(); + + tester = new WicketTester(); + tester.getApplication().getSharedResources().add(ADMIN, admin); + tester.getApplication().getSharedResources().add(OPEN, open); + } + + @AfterEach + public void after() + { + tester.destroy(); + } + + private void signInAs(String role) + { + tester.getApplication() + .getSecuritySettings() + .setAuthorizationStrategy(new RoleAuthorizationStrategy(new UserRolesAuthorizer(role))); + } + + @Test + public void adminMayOpenAConnectionToTheAdminResource() + { + signInAs("ADMIN"); + + AtomicBoolean echoed = new AtomicBoolean(false); + WebSocketTester webSocketTester = new WebSocketTester(tester, ADMIN) + { + @Override + protected void onOutMessage(String message) + { + assertEquals(Strings.capitalize(EXPECTED_TEXT), message); + echoed.set(true); + } + }; + + assertTrue(admin.connected.get()); + assertTrue(webSocketTester.isConnectionOpen()); + + webSocketTester.sendMessage(EXPECTED_TEXT); + assertTrue(admin.messaged.get()); + assertTrue(echoed.get()); + + webSocketTester.destroy(); + assertTrue(admin.closed.get()); + } + + @Test + public void userMayNotOpenAConnectionToTheAdminResource() + { + signInAs("USER"); + + AtomicBoolean echoed = new AtomicBoolean(false); + WebSocketTester webSocketTester = new WebSocketTester(tester, ADMIN) + { + @Override + protected void onOutMessage(String message) + { + echoed.set(true); + } + }; + + assertFalse(admin.connected.get(), "the connection should have been refused"); + assertFalse(webSocketTester.isConnectionOpen()); + + webSocketTester.sendMessage(EXPECTED_TEXT); + webSocketTester.destroy(); + + assertFalse(admin.messaged.get()); + assertFalse(admin.closed.get()); + assertFalse(echoed.get()); + } + + @Test + public void userMayStillOpenAConnectionToAnUnannotatedResource() + { + signInAs("USER"); + + WebSocketTester webSocketTester = new WebSocketTester(tester, OPEN); + + assertTrue(open.connected.get()); + assertTrue(webSocketTester.isConnectionOpen()); + + webSocketTester.sendMessage(EXPECTED_TEXT); + assertTrue(open.messaged.get()); + } + + private static class RecordingWebSocketResource extends WebSocketResource + { + final AtomicBoolean connected = new AtomicBoolean(false); + final AtomicBoolean messaged = new AtomicBoolean(false); + final AtomicBoolean closed = new AtomicBoolean(false); + + @Override + protected void onConnect(ConnectedMessage message) + { + connected.set(true); + } + + @Override + protected void onClose(ClosedMessage message) + { + closed.set(true); + } + + @Override + protected void onMessage(WebSocketRequestHandler handler, TextMessage message) + { + messaged.set(true); + handler.push(Strings.capitalize(message.getText())); + } + } + + @AuthorizeResource("ADMIN") + private static class AdminWebSocketResource extends RecordingWebSocketResource + { + } + + private static class OpenWebSocketResource extends RecordingWebSocketResource + { + } + + private static final class UserRolesAuthorizer implements IRoleCheckingStrategy, Serializable + { + private static final long serialVersionUID = 1L; + + private final Roles roles; + + UserRolesAuthorizer(String roles) + { + this.roles = new Roles(roles); + } + + @Override + public boolean hasAnyRole(Roles roles) + { + return this.roles.hasAnyRole(roles); + } + } +} diff --git a/wicket-native-websocket/wicket-native-websocket-tester/src/test/java/org/apache/wicket/protocol/ws/tester/TestWebSocketResource.java b/wicket-native-websocket/wicket-native-websocket-tester/src/test/java/org/apache/wicket/protocol/ws/tester/TestWebSocketResource.java index 833511aae0..7036f2b6d7 100644 --- a/wicket-native-websocket/wicket-native-websocket-tester/src/test/java/org/apache/wicket/protocol/ws/tester/TestWebSocketResource.java +++ b/wicket-native-websocket/wicket-native-websocket-tester/src/test/java/org/apache/wicket/protocol/ws/tester/TestWebSocketResource.java @@ -22,6 +22,7 @@ import org.apache.wicket.protocol.ws.api.message.AbortedMessage; import org.apache.wicket.protocol.ws.api.message.BinaryMessage; import org.apache.wicket.protocol.ws.api.message.ClosedMessage; import org.apache.wicket.protocol.ws.api.message.ConnectedMessage; +import org.apache.wicket.protocol.ws.api.message.IWebSocketPushMessage; import org.apache.wicket.protocol.ws.api.message.TextMessage; import org.apache.wicket.util.string.Strings; @@ -36,6 +37,17 @@ public class TestWebSocketResource extends WebSocketResource static final AtomicBoolean ON_CONNECT_CALLED = new AtomicBoolean(false); static final AtomicBoolean ON_CLOSE_CALLED = new AtomicBoolean(false); static final AtomicBoolean ON_ABORT_CALLED = new AtomicBoolean(false); + static final AtomicBoolean ON_MESSAGE_CALLED = new AtomicBoolean(false); + static final AtomicBoolean ON_PUSH_CALLED = new AtomicBoolean(false); + + static void resetCallbacks() + { + ON_CONNECT_CALLED.set(false); + ON_CLOSE_CALLED.set(false); + ON_ABORT_CALLED.set(false); + ON_MESSAGE_CALLED.set(false); + ON_PUSH_CALLED.set(false); + } private final String expectedMessage; @@ -81,10 +93,18 @@ public class TestWebSocketResource extends WebSocketResource super.onAbort(message); } + @Override + protected void onPush(WebSocketRequestHandler handler, IWebSocketPushMessage message) + { + super.onPush(handler, message); + ON_PUSH_CALLED.set(true); + } + @Override protected void onMessage(WebSocketRequestHandler handler, TextMessage message) { super.onMessage(handler, message); + ON_MESSAGE_CALLED.set(true); String text = message.getText(); assertEquals(expectedMessage, text); @@ -95,6 +115,7 @@ public class TestWebSocketResource extends WebSocketResource protected void onMessage(WebSocketRequestHandler handler, BinaryMessage binaryMessage) { super.onMessage(handler, binaryMessage); + ON_MESSAGE_CALLED.set(true); byte[] data = binaryMessage.getData(); int offset = binaryMessage.getOffset(); diff --git a/wicket-native-websocket/wicket-native-websocket-tester/src/test/java/org/apache/wicket/protocol/ws/tester/WebSocketResourceAuthorizationTest.java b/wicket-native-websocket/wicket-native-websocket-tester/src/test/java/org/apache/wicket/protocol/ws/tester/WebSocketResourceAuthorizationTest.java new file mode 100644 index 0000000000..e50d7232a1 --- /dev/null +++ b/wicket-native-websocket/wicket-native-websocket-tester/src/test/java/org/apache/wicket/protocol/ws/tester/WebSocketResourceAuthorizationTest.java @@ -0,0 +1,291 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You 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 org.apache.wicket.protocol.ws.tester; + +import java.util.ArrayList; +import java.util.List; +import java.util.concurrent.atomic.AtomicBoolean; + +import org.apache.wicket.authorization.IAuthorizationStrategy; +import org.apache.wicket.mock.MockApplication; +import org.apache.wicket.protocol.http.WebApplication; +import org.apache.wicket.protocol.ws.WebSocketSettings; +import org.apache.wicket.protocol.ws.api.ConnectionRejected; +import org.apache.wicket.protocol.ws.api.WebSocketConnectionOriginFilter; +import org.apache.wicket.protocol.ws.api.message.IWebSocketPushMessage; +import org.apache.wicket.request.mapper.parameter.PageParameters; +import org.apache.wicket.request.resource.IResource; +import org.apache.wicket.util.string.Strings; +import org.apache.wicket.util.tester.WicketTester; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertSame; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * Tests that the IAuthorizationStrategy decides who may use a shared WebSocketResource. + * + * A WebSocketResource serves nothing over HTTP, so a WebSocket connection is the only way to reach + * it, and the strategy is asked once, when that connection is opened. A refused client is + * disconnected and reaches none of the callbacks. + */ +class WebSocketResourceAuthorizationTest +{ + private static final String EXPECTED_TEXT = "expected text"; + + private RecordingAuthorizationStrategy strategy; + private TestWebSocketResource resource; + private WicketTester tester; + + @BeforeEach + public void before() + { + TestWebSocketResource.resetCallbacks(); + + strategy = new RecordingAuthorizationStrategy(); + resource = new TestWebSocketResource(EXPECTED_TEXT); + + WebApplication application = new MockApplication() + { + @Override + protected void init() + { + super.init(); + + getSecuritySettings().setAuthorizationStrategy(strategy); + getSharedResources().add(TestWebSocketResource.TEXT, resource); + } + }; + tester = new WicketTester(application); + tester.getSession().bind(); + } + + @AfterEach + public void after() + { + tester.destroy(); + TestWebSocketResource.resetCallbacks(); + } + + /** + * The control: an allowed client drives the resource exactly as before, and the strategy was + * asked about the very instance that was registered, with parameters that are empty rather than + * the connection parameters of the handshake. + */ + @Test + public void anAuthorizedCallerDrivesTheResource() + { + AtomicBoolean echoed = new AtomicBoolean(false); + + WebSocketTester webSocketTester = new WebSocketTester(tester, TestWebSocketResource.TEXT) + { + @Override + protected void onOutMessage(String message) + { + assertEquals(Strings.capitalize(EXPECTED_TEXT), message); + echoed.set(true); + } + }; + + assertEquals(1, strategy.resources.size()); + assertSame(resource, strategy.resources.get(0)); + assertNotNull(strategy.parameters.get(0)); + assertTrue(strategy.parameters.get(0).isEmpty()); + + assertTrue(TestWebSocketResource.ON_CONNECT_CALLED.get()); + assertTrue(webSocketTester.isConnectionOpen()); + + webSocketTester.sendMessage(EXPECTED_TEXT); + assertTrue(TestWebSocketResource.ON_MESSAGE_CALLED.get()); + assertTrue(echoed.get()); + + webSocketTester.destroy(); + assertTrue(TestWebSocketResource.ON_CLOSE_CALLED.get()); + } + + /** + * The refusal itself: no callback of the resource is reached, and the connection is closed and + * gone from the registry rather than left open. + */ + @Test + public void anUnauthorizedCallerIsRefusedAtTheHandshake() + { + strategy.allow = false; + + WebSocketTester webSocketTester = new WebSocketTester(tester, TestWebSocketResource.TEXT); + + assertEquals(1, strategy.resources.size()); + assertFalse(TestWebSocketResource.ON_CONNECT_CALLED.get()); + assertFalse(TestWebSocketResource.ON_ABORT_CALLED.get()); + assertFalse(TestWebSocketResource.ON_CLOSE_CALLED.get()); + + assertFalse(webSocketTester.isConnectionOpen()); + assertEquals(WebSocketConnectionOriginFilter.POLICY_VIOLATION_ERROR_CODE, + webSocketTester.getCloseCode()); + assertEquals("Not authorized", webSocketTester.getCloseReason()); + + assertTrue(connections().isEmpty()); + } + + /** + * A refused client that keeps talking is not heard: the connection it would be answered on is + * no longer registered. + */ + @Test + public void aRefusedCallerCannotSendAMessage() + { + strategy.allow = false; + + AtomicBoolean echoed = new AtomicBoolean(false); + WebSocketTester webSocketTester = new WebSocketTester(tester, TestWebSocketResource.TEXT) + { + @Override + protected void onOutMessage(String message) + { + echoed.set(true); + } + + @Override + protected void onOutMessage(byte[] message, int offset, int length) + { + echoed.set(true); + } + }; + + webSocketTester.sendMessage(EXPECTED_TEXT); + webSocketTester.sendMessage(new byte[] { 1, 2, 3 }, 0, 3); + + assertFalse(TestWebSocketResource.ON_MESSAGE_CALLED.get()); + assertFalse(echoed.get()); + } + + /** + * And is not reachable from the server side either, because it was removed from the registry a + * broadcast walks. + */ + @Test + public void aRefusedConnectionIsNotReachableByAPush() + { + strategy.allow = false; + + WebSocketTester webSocketTester = new WebSocketTester(tester, TestWebSocketResource.TEXT); + webSocketTester.broadcastAll(tester.getApplication(), new TestPushMessage()); + + assertFalse(TestWebSocketResource.ON_PUSH_CALLED.get()); + } + + /** + * The decision is taken for the connection, not for every message on it. This pins that choice: + * a later change to re-check per message has to change this test deliberately. + */ + @Test + public void anAuthorizedConnectionIsAskedOnlyOnce() + { + WebSocketTester webSocketTester = new WebSocketTester(tester, TestWebSocketResource.TEXT); + + webSocketTester.sendMessage(EXPECTED_TEXT); + webSocketTester.broadcastAll(tester.getApplication(), new TestPushMessage()); + webSocketTester.destroy(); + + assertTrue(TestWebSocketResource.ON_MESSAGE_CALLED.get()); + assertTrue(TestWebSocketResource.ON_PUSH_CALLED.get()); + assertTrue(TestWebSocketResource.ON_CLOSE_CALLED.get()); + + assertEquals(1, strategy.resources.size()); + } + + /** + * A connection to a page is not a request for a resource, so the strategy is not asked about it + * at all - not even by a strategy that refuses every resource there is. + */ + @Test + public void aPageConnectionIsNotAskedAbout() + { + strategy.allow = false; + + AtomicBoolean echoed = new AtomicBoolean(false); + WebSocketBehaviorTestPage page = new WebSocketBehaviorTestPage(EXPECTED_TEXT); + tester.startPage(page); + + WebSocketTester webSocketTester = new WebSocketTester(tester, page) + { + @Override + protected void onOutMessage(String message) + { + assertEquals(Strings.capitalize(EXPECTED_TEXT), message); + echoed.set(true); + } + }; + + webSocketTester.sendMessage(EXPECTED_TEXT); + + assertTrue(echoed.get()); + assertTrue(webSocketTester.isConnectionOpen()); + assertTrue(strategy.resources.isEmpty()); + } + + /** + * The connection filter of the application is untouched: it still rejects before the resource is + * authorized, and it still tells the resource so with an AbortedMessage. + */ + @Test + public void theConnectionFilterStillAbortsWithAnAbortedMessage() + { + WebSocketSettings.Holder.get(tester.getApplication()) + .setConnectionFilter(servletRequest -> new ConnectionRejected( + WebSocketConnectionOriginFilter.POLICY_VIOLATION_ERROR_CODE, "Rejected")); + + WebSocketTester webSocketTester = new WebSocketTester(tester, TestWebSocketResource.TEXT); + + assertTrue(TestWebSocketResource.ON_ABORT_CALLED.get()); + assertFalse(TestWebSocketResource.ON_CONNECT_CALLED.get()); + assertFalse(webSocketTester.isConnectionOpen()); + assertEquals("Rejected", webSocketTester.getCloseReason()); + } + + private List<?> connections() + { + return new ArrayList<>(WebSocketSettings.Holder.get(tester.getApplication()) + .getConnectionRegistry().getConnections(tester.getApplication())); + } + + private static class RecordingAuthorizationStrategy + extends IAuthorizationStrategy.AllowAllAuthorizationStrategy + { + private boolean allow = true; + + private final List<IResource> resources = new ArrayList<>(); + private final List<PageParameters> parameters = new ArrayList<>(); + + @Override + public boolean isResourceAuthorized(IResource resource, PageParameters pageParameters) + { + resources.add(resource); + parameters.add(pageParameters); + return allow; + } + } + + private static class TestPushMessage implements IWebSocketPushMessage + { + } +} diff --git a/wicket-native-websocket/wicket-native-websocket-tester/src/test/java/org/apache/wicket/protocol/ws/tester/WebSocketTesterProcessorTest.java b/wicket-native-websocket/wicket-native-websocket-tester/src/test/java/org/apache/wicket/protocol/ws/tester/WebSocketTesterProcessorTest.java index 49fcb259b8..aabf300a6d 100644 --- a/wicket-native-websocket/wicket-native-websocket-tester/src/test/java/org/apache/wicket/protocol/ws/tester/WebSocketTesterProcessorTest.java +++ b/wicket-native-websocket/wicket-native-websocket-tester/src/test/java/org/apache/wicket/protocol/ws/tester/WebSocketTesterProcessorTest.java @@ -84,7 +84,7 @@ public class WebSocketTesterProcessorTest public void after() { tester.destroy(); - TestWebSocketResource.ON_ABORT_CALLED.set(false); + TestWebSocketResource.resetCallbacks(); } @Test diff --git a/wicket-native-websocket/wicket-native-websocket-tester/src/test/java/org/apache/wicket/protocol/ws/tester/WebSocketTesterResourceTest.java b/wicket-native-websocket/wicket-native-websocket-tester/src/test/java/org/apache/wicket/protocol/ws/tester/WebSocketTesterResourceTest.java index 23317c93a6..817ea7f467 100644 --- a/wicket-native-websocket/wicket-native-websocket-tester/src/test/java/org/apache/wicket/protocol/ws/tester/WebSocketTesterResourceTest.java +++ b/wicket-native-websocket/wicket-native-websocket-tester/src/test/java/org/apache/wicket/protocol/ws/tester/WebSocketTesterResourceTest.java @@ -52,8 +52,7 @@ public class WebSocketTesterResourceTest @BeforeEach public void before() { - TestWebSocketResource.ON_CONNECT_CALLED.set(false); - TestWebSocketResource.ON_CLOSE_CALLED.set(false); + TestWebSocketResource.resetCallbacks(); ON_OUT_BINARY_CALLED.set(false); ON_OUT_TEXT_CALLED.set(false); diff --git a/wicket-user-guide/src/main/asciidoc/nativewebsockets/nativewebsockets_2.adoc b/wicket-user-guide/src/main/asciidoc/nativewebsockets/nativewebsockets_2.adoc index 96c125bd64..9110779f61 100644 --- a/wicket-user-guide/src/main/asciidoc/nativewebsockets/nativewebsockets_2.adoc +++ b/wicket-user-guide/src/main/asciidoc/nativewebsockets/nativewebsockets_2.adoc @@ -102,6 +102,8 @@ and ---- to any page. This will prepare the JavaScript connection for you. +A _WebSocketResource_ is a shared resource like any other, so the application's _IAuthorizationStrategy_ decides who may use it: _isResourceAuthorized()_ is asked when a client opens a connection to it, and an _@AuthorizeResource_ annotation on the resource class applies. A client that is refused is disconnected with WebSocket close code 1008 and reaches none of the callbacks - there is no 403 to send, because an HTTP status code cannot be delivered over this transport. The decision is t [...] + * *WebSocket connection registry* To push data to one or more clients the application can use the _IWebSocketConnectionRegistry_ to find all registered connections and send data to all/any of them:
