Require authentication for GraphQL WebSocket upgrade (master)

[sergehuber]

Summary

• Validate credentials before accepting a GraphQL WebSocket upgrade.
• Attach the authenticated subject to the subscription socket and clear security context after subscribe.
• Add unit and integration coverage for missing/invalid credentials and successful private-key upgrade.

Test plan

• GraphQLServletSecurityValidatorTest WebSocket upgrade cases
• GraphQLWebSocketIT no-auth / public-key / wrong-password / private-key cases

[Copilot]

Pull request overview

This PR tightens GraphQL subscriptions security by requiring authentication during the WebSocket upgrade, carrying the authenticated Subject into the subscription socket, and clearing thread-local security/execution context after subscription setup. It also expands unit + integration test coverage around the new upgrade-auth behavior.

Changes:

• Enforce authentication during GraphQL WebSocket upgrade and reject unauthenticated upgrades with HTTP 401.
• Bind the authenticated Subject (and ExecutionContext) to the created SubscriptionWebSocket, and clear thread-local context after handling GQL_START.
• Add unit tests for validateWebSocketUpgrade(...) and IT coverage for unauthenticated/invalid credential upgrade attempts and private-key success.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
itests/src/test/java/org/apache/unomi/itests/graphql/GraphQLWebSocketIT.java	Adds IT coverage for rejected/accepted WebSocket upgrades and updates existing test to include auth.
graphql/cxs-impl/src/test/java/org/apache/unomi/graphql/servlet/auth/GraphQLServletSecurityValidatorTest.java	Adds unit tests for WebSocket upgrade authentication behavior.
graphql/cxs-impl/src/main/java/org/apache/unomi/graphql/servlet/websocket/SubscriptionWebSocketFactory.java	Ensures a WebSocket is only created when a subject is present and passes security context into the socket.
graphql/cxs-impl/src/main/java/org/apache/unomi/graphql/servlet/websocket/SubscriptionWebSocket.java	Sets/clears security + execution context around subscription execution and handles missing variables map.
graphql/cxs-impl/src/main/java/org/apache/unomi/graphql/servlet/GraphQLServlet.java	Wires WebSocket upgrade validation before accepting upgrades and passes dependencies into the WebSocket factory.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Suppressed comments (2)

graphql/cxs-impl/src/main/java/org/apache/unomi/graphql/servlet/auth/GraphQLServletSecurityValidator.java:84

• A malformed Basic value (for example Authorization: Basic !!!) makes the Base64 decoder in isAuthenticatedUser throw IllegalArgumentException, so this upgrade returns a server error instead of the promised 401 for invalid credentials. Treat decoding failures as authentication failures.

        if (isAuthenticatedUser(req)) {
            return true;
        }
        res.sendError(HttpServletResponse.SC_UNAUTHORIZED);
        return false;

graphql/cxs-impl/src/main/java/org/apache/unomi/graphql/servlet/websocket/SubscriptionWebSocket.java:130

• The added upgrade tests only send connection_init and connection_terminate, so they never exercise this new subject/context binding or its cleanup. Add a start/subscription test whose resolver requires the authenticated tenant (and verify the thread-local context is cleared afterward), otherwise the core authorization behavior can regress while all new tests still pass.

            securityService.setCurrentSubject(subject);
            executionContextManager.setCurrentContext(executionContext);

[Copilot]

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

graphql/cxs-impl/src/main/java/org/apache/unomi/graphql/servlet/GraphQLServlet.java:185

• This negotiates protocols the socket does not implement. graphql-transport-ws is accepted by this prefix check (and explicitly expected by the new test), but SubscriptionWebSocket only handles the legacy start/stop/data messages; transport-ws clients send subscribe/next instead. Such a connection upgrades successfully and then silently ignores subscriptions. Advertise only graphql-ws, or implement the transport-ws message state machine and align the UI/tests.

            for (String part : headerValue.split(",")) {
                String subProtocol = part.trim();
                if (subProtocol.startsWith("graphql")) {
                    response.addHeader("Sec-WebSocket-Protocol", subProtocol);

itests/src/test/java/org/apache/unomi/itests/graphql/GraphQLWebSocketIT.java:179

• This assertion can pass before the server has processed start, so it does not prove that subscription setup succeeded or that the captured security context works. It also never triggers an event, which is when GraphQL resolves the subscription selection set. Replace the sleep with a deterministic event emission and await/assert the resulting WebSocket data message before stopping the subscription.

            remote.sendString(resourceAsString("graphql/socket/out/start.json"));
            // Successful subscribe() registers a publisher and does not emit until events arrive.
            // Give the server a moment; an auth/context failure would close the socket with an error.
            Thread.sleep(500);
            Assert.assertFalse("Subscription start should not close the socket", closeFuture.isDone());

[Copilot]

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

itests/src/test/java/org/apache/unomi/itests/graphql/GraphQLWebSocketIT.java:179

• This can pass before the server has processed the start frame: after a fixed 500 ms delay, an open socket proves neither that subscribe() ran nor that the captured subject/context was accepted. That leaves the core authenticated-subscription regression untested and makes the result timing-dependent. Trigger a matching event and await/assert its subscription data (or add another deterministic server acknowledgement) before stopping the subscription.

            remote.sendString(resourceAsString("graphql/socket/out/start.json"));
            // Successful subscribe() registers a publisher and does not emit until events arrive.
            // Give the server a moment; an auth/context failure would close the socket with an error.
            Thread.sleep(500);
            Assert.assertFalse("Subscription start should not close the socket", closeFuture.isDone());