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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
