Fixes #5401 - GraphQL operations made over ActionCable may mix up current user

app/channels/application_cable/connection.rb

@@ -11,24 +11,25 @@ module ApplicationCable
     # Therefore, on login/logout, a new web socket connection must be made to
     #   reflect the changes within GraphQL.
     def connect
-      find_verified_user
-      UserInfo.current_user_id = current_user&.id
+      return if session_id.blank?
+
+      self.current_user = find_verified_user
+      self.sid          = session_id
     end
 
     private
 
     def find_verified_user
-      session_id = cookies[Zammad::Application::Initializer::SessionStore::SESSION_KEY]
-      return if session_id.blank?
-
       private_id = Rack::Session::SessionId.new(session_id).private_id
-      return if private_id.blank?
 
       session = ActiveRecord::SessionStore::Session.find_by(session_id: private_id)
-      return if session.blank?
+      return if !session
 
-      self.current_user = User.find_by(id: session.data['user_id'])
-      self.sid          = session_id
+      User.find_by(id: session.data['user_id'])
+    end
+
+    def session_id
+      @session_id ||= cookies[Zammad::Application::Initializer::SessionStore::SESSION_KEY]
     end
   end
 end

app/channels/graphql_channel.rb

@@ -18,7 +18,9 @@ class GraphqlChannel < ApplicationCable::Channel
       channel:      self,
     }
 
-    result = Gql::ZammadSchema.execute(query:, context:, variables:, operation_name:)
+    result = UserInfo.with_user_id(current_user&.id) do
+      Gql::ZammadSchema.execute(query:, context:, variables:, operation_name:)
+    end
 
     payload = {
       result: result.to_h,

spec/channels/application_cable/connection_spec.rb

@@ -0,0 +1,53 @@
+# Copyright (C) 2012-2024 Zammad Foundation, https://zammad-foundation.org/
+
+require 'rails_helper'
+
+RSpec.describe ApplicationCable::Connection, type: :channel do
+  context 'when user session is present' do
+    let(:session_id) { '123_456' }
+
+    before do
+      private_session_id = Rack::Session::SessionId.new(session_id).private_id
+
+      create(:active_session, session_id: private_session_id, user: user)
+
+      cookies[Zammad::Application::Initializer::SessionStore::SESSION_KEY] = session_id
+    end
+
+    context 'when session contains a user' do
+      let(:user)       { create(:agent) }
+
+      it 'sets current user on connecting' do
+        connect
+
+        expect(connection).to have_attributes(current_user: user, sid: session_id)
+      end
+
+      it 'connects but sets no user or sid if user in session no longer exists' do
+        user.destroy!
+
+        connect
+
+        expect(connection).to have_attributes(current_user: be_nil, sid: session_id)
+      end
+    end
+
+    context 'when session contains no user' do
+      let(:user) { nil }
+
+      it 'connects but sets no user or sid if user in session no longer exists' do
+        connect
+
+        expect(connection).to have_attributes(current_user: be_nil, sid: session_id)
+      end
+    end
+  end
+
+  context 'when no user session present' do
+    it 'connects but sets no user or sid' do
+      connect
+
+      expect(connection).to have_attributes(current_user: be_nil, sid: be_nil)
+    end
+  end
+end

spec/channels/graphql_channel_spec.rb

@@ -0,0 +1,60 @@
+# Copyright (C) 2012-2024 Zammad Foundation, https://zammad-foundation.org/
+
+require 'rails_helper'
+
+RSpec.describe GraphqlChannel, type: :channel do
+  # https://github.com/zammad/zammad/issues/5401
+  describe 'setting UserInfo.current_user_id thread variable' do
+    class TestQuery < Gql::Queries::BaseQuery # rubocop:disable RSpec/LeakyConstantDeclaration, Lint/ConstantDefinitionInBlock
+      type Gql::Types::UserType, null: true
+
+      def resolve
+        User.find_by id: UserInfo.current_user_id
+      end
+    end
+
+    let(:query) do
+      <<~QUERY
+        query testQuery {
+          testQuery {
+            id
+          }
+        }
+      QUERY
+    end
+
+    context 'when connected with a session with a user' do
+      let(:user) { create(:agent) }
+
+      it 'sets UserInfo.current_user_id for the operation' do
+        stub_connection sid: '123_456', current_user: user
+
+        subscribe
+
+        perform :execute, operationName: 'testQuery', query: query
+
+        expect(transmissions.last).to include(
+          result: include(
+            data: include(testQuery: include(id: user.to_global_id.to_s))
+          )
+        )
+      end
+    end
+
+    context 'when connected with a userless session' do
+      it 'sets UserInfo.current_user_id for the operation' do
+        stub_connection sid: '123_456', current_user: nil
+
+        subscribe
+
+        perform :execute, operationName: 'testQuery', query: query
+
+        expect(transmissions.last).to include(
+          result: include(
+            data: include(testQuery: be_nil)
+          )
+        )
+      end
+    end
+  end
+end