You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@unomi.apache.org by "Copilot (via GitHub)" <gi...@apache.org> on 2026/08/07 16:42:31 UTC
Re: [PR] Require authentication for GraphQL WebSocket upgrade (master) [unomi]
Copilot commented on code in PR #843:
URL: https://github.com/apache/unomi/pull/843#discussion_r3737321879
##########
graphql/cxs-impl/src/test/java/org/apache/unomi/graphql/servlet/auth/GraphQLServletSecurityValidatorTest.java:
##########
@@ -138,6 +138,40 @@ void validate_withoutAuthorizationHeader_isRejected() throws IOException {
verify(response).sendError(HttpServletResponse.SC_UNAUTHORIZED);
}
+ @Test
+ void validateWebSocketUpgrade_withoutAuthorization_isRejected() throws IOException {
+ when(request.getHeader("Authorization")).thenReturn(null);
+
+ boolean authenticated = validator.validateWebSocketUpgrade(request, response);
+
+ assertFalse(authenticated);
+ verify(response).sendError(HttpServletResponse.SC_UNAUTHORIZED);
+ verify(securityService, never()).setCurrentSubject(any());
+ }
+
+ @Test
+ void validateWebSocketUpgrade_withBasicAuth_isAccepted() throws IOException {
+ when(request.getHeader("Authorization")).thenReturn(BASIC_AUTH);
+ when(tenantService.getTenantByApiKey(any(), eq(ApiKey.ApiKeyType.PRIVATE))).thenReturn(null);
+
+ boolean authenticated = validator.validateWebSocketUpgrade(request, response);
+
+ assertTrue(authenticated);
+ verify(securityService).setCurrentSubject(any(Subject.class));
+ verify(response, never()).sendError(any(Integer.class));
+ }
+
+ @Test
+ void validateWebSocketUpgrade_rejectsPublicApiKeyOnly() throws IOException {
+ // No Authorization header — public API key alone must not open subscriptions
+ when(request.getHeader("Authorization")).thenReturn(null);
+
+ boolean authenticated = validator.validateWebSocketUpgrade(request, response);
Review Comment:
This test is named "rejectsPublicApiKeyOnly" but it never sets the public API key header, so it doesn't actually cover the intended scenario (it duplicates the "withoutAuthorization" case). Stub X-Unomi-Api-Key to ensure public-key-only upgrades are rejected.
##########
itests/src/test/java/org/apache/unomi/itests/graphql/GraphQLWebSocketIT.java:
##########
@@ -52,6 +56,7 @@ public void testWebSocketConnectionSegment() throws Exception {
URI echoUri = new URI("ws://localhost:" + getHttpPort() + "/graphql");
ClientUpgradeRequest request = new ClientUpgradeRequest();
+ request.setHeader("Authorization", basicAuthHeader(BASIC_AUTH_USER_NAME, BASIC_AUTH_PASSWORD));
Future<Session> onConnected = client.connect(socket, echoUri, request);
RemoteEndpoint remote = onConnected.get().getRemote();
Review Comment:
This integration test uses onConnected.get() without a timeout, which can hang the entire IT suite if the upgrade never completes (e.g., auth regression). Use a bounded get(...) like the other WebSocket tests in this class.
##########
graphql/cxs-impl/src/main/java/org/apache/unomi/graphql/servlet/GraphQLServlet.java:
##########
@@ -125,6 +126,9 @@ protected void service(HttpServletRequest request, HttpServletResponse response)
LOGGER.debug("GraphQLServlet service called with request: {}", request.getRequestURI());
if (factory.isUpgradeRequest(request, response)) {
try {
+ if (!validator.validateWebSocketUpgrade(request, response)) {
+ return;
+ }
Review Comment:
When a WebSocket upgrade is rejected (missing/invalid credentials), the method returns early without clearing thread-local security/execution context. If the servlet container reuses the same thread, a stale subject/context from a previous request could leak into later processing. Clear the context before returning on authentication failure.
##########
graphql/cxs-impl/src/main/java/org/apache/unomi/graphql/servlet/websocket/SubscriptionWebSocket.java:
##########
@@ -106,33 +125,50 @@ private void unsubscribe(GraphQLMessage message) {
private void subscribe(GraphQLMessage message) {
final Map<String, Object> payload = message.getPayload();
- ExecutionInput executionInput = ExecutionInput.newExecutionInput()
- .query((String) payload.get("query"))
- .variables((Map<String, Object>) payload.get("variables"))
- .operationName((String) payload.get("operationName"))
- .context(serviceManager)
- .build();
-
- ExecutionResult executionResult = this.graphQL.execute(executionInput);
- if (executionResult.getErrors() != null && !executionResult.getErrors().isEmpty()) {
- sendMessage(GraphQLMessage.create(message.getId())
- .errors(executionResult.getErrors())
- .build());
- closeConnection(message, "Error executing graphQL query");
- return;
- } else if (!(executionResult.getData() instanceof Publisher)) {
- final String error = "Fetched value should be instance of Publisher, was: " + executionResult.getClass().getName();
- sendMessage(GraphQLMessage.create(message.getId())
- .errors(Collections.singletonList(error))
- .build());
- closeConnection(message, error);
- return;
+ try {
+ securityService.setCurrentSubject(subject);
+ executionContextManager.setCurrentContext(executionContext);
+
+ Map<String, Object> variables = (Map<String, Object>) payload.get("variables");
+ if (variables == null) {
+ variables = new HashMap<>();
+ }
+
+ ExecutionInput executionInput = ExecutionInput.newExecutionInput()
+ .query((String) payload.get("query"))
+ .variables(variables)
+ .operationName((String) payload.get("operationName"))
+ .context(serviceManager)
+ .build();
+
+ ExecutionResult executionResult = this.graphQL.execute(executionInput);
+ if (executionResult.getErrors() != null && !executionResult.getErrors().isEmpty()) {
+ sendMessage(GraphQLMessage.create(message.getId())
+ .errors(executionResult.getErrors())
+ .build());
+ closeConnection(message, "Error executing graphQL query");
+ return;
+ } else if (!(executionResult.getData() instanceof Publisher)) {
+ final String error = "Fetched value should be instance of Publisher, was: " + executionResult.getClass().getName();
+ sendMessage(GraphQLMessage.create(message.getId())
Review Comment:
The error message reports executionResult's class rather than the fetched data's class. This makes debugging misleading when the returned data isn't a Publisher.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@unomi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org