Fixes #5133 - Wrong Setting value is broadcasted if interpolation is …

…used

app/graphql/gql/subscriptions/config_updates.rb

@@ -15,13 +15,14 @@ def self.authorize(...)
     end
 
     def update
-      setting = object
+      return no_update if !object.frontend
+      return no_update if object.preferences[:authentication] && !context.current_user?
 
-      if !setting.frontend || (setting.preferences[:authentication] && !context.current_user?)
-        return no_update
-      end
+      # Some setting values use interpolation to reference other settings.
+      # This is applied in `Setting.get`, thus direct reading of the value should be avoided.
+      value = Setting.get(object.name)
 
-      { setting: { key: setting.name, value: setting.state_current[:value] } }
+      { setting: { key: object.name, value: value } }
     end
   end
 end

app/models/setting.rb

@@ -7,9 +7,9 @@ class Setting < ApplicationModel
   store         :preferences
   before_validation :state_check
   before_create :set_initial
-  after_create  :reset_change_id, :reset_cache, :check_broadcast
-  after_update  :reset_change_id, :reset_cache, :check_broadcast
-  after_commit  :check_refresh
+  after_create  :reset_change_id
+  after_update  :reset_change_id
+  after_commit  :reset_cache, :broadcast_frontend, :check_refresh
 
   validates_with Setting::Validator
 
@@ -139,11 +139,9 @@ def self.load(force = false)
   # set initial value in state_initial
   def set_initial
     self.state_initial = state_current
-    true
   end
 
   def reset_change_id
-    @@current[name] = state_current[:value]
     change_id = SecureRandom.uuid
     logger.debug { "Setting.reset_change_id: set new cache, #{change_id}" }
     Rails.cache.write('Setting::ChangeId', change_id, { expires_in: 24.hours })
@@ -152,12 +150,11 @@ def reset_change_id
   end
 
   def reset_cache
-    return true if preferences[:cache].blank?
+    return if preferences[:cache].blank?
 
-    preferences[:cache].each do |key|
+    Array(preferences[:cache]).each do |key|
       Rails.cache.delete(key)
     end
-    true
   end
 
   # check if cache is still valid
@@ -180,21 +177,19 @@ def self.cache_valid?
 
   # convert state into hash to be able to store it as store
   def state_check
-    return true if state.nil? # allow false value
-    return true if state.try(:key?, :value)
+    return if state.nil? # allow false value
+    return if state.try(:key?, :value)
 
     self.state_current = { value: state }
-    true
   end
 
   # Notify clients about config changes.
-  def check_broadcast
-    return true if frontend != true
+  def broadcast_frontend
+    return if !frontend
 
-    value = state_current
-    if state_current.key?(:value)
-      value = state_current[:value]
-    end
+    # Some setting values use interpolation to reference other settings.
+    # This is applied in `Setting.get`, thus direct reading of the value should be avoided.
+    value = self.class.get(name)
 
     Sessions.broadcast(
       {
@@ -203,8 +198,8 @@ def check_broadcast
       },
       preferences[:authentication] ? 'authenticated' : 'public'
     )
+
     Gql::Subscriptions::ConfigUpdates.trigger(self)
-    true
   end
 
   # NB: Force users to reload on SAML credentials config changes

spec/graphql/gql/subscriptions/config_updates_spec.rb

@@ -3,6 +3,7 @@
 require 'rails_helper'
 
 RSpec.describe Gql::Subscriptions::ConfigUpdates, type: :graphql do
+  let(:setting) { build(:setting, name: 'broadcast_test', state: setting_value, frontend: true) }
 
   let(:subscription) do
     <<~QUERY
@@ -23,8 +24,8 @@
         'data' => {
           'configUpdates' => {
             'setting' => {
-              'key'   => 'product_name',
-              'value' => 'subscription_test'
+              'key'   => 'broadcast_test',
+              'value' => expected_value
             }
           }
         }
@@ -33,9 +34,25 @@
     }
   end
 
-  it 'broadcasts config update events' do
-    gql.execute(subscription, context: { channel: mock_channel })
-    Setting.set('product_name', 'subscription_test')
-    expect(mock_channel.mock_broadcasted_messages).to eq([expected_msg])
+  context 'when using static value' do
+    let(:setting_value)  { 'subscription_test' }
+    let(:expected_value) { setting_value }
+
+    it 'broadcasts config update events' do
+      gql.execute(subscription, context: { channel: mock_channel })
+      setting.save
+      expect(mock_channel.mock_broadcasted_messages).to eq([expected_msg])
+    end
+  end
+
+  context 'when using interpolated value' do
+    let(:setting_value)  { 'test #{config.fqdn}' } # rubocop:disable Lint/InterpolationCheck
+    let(:expected_value) { "test #{Setting.get('fqdn')}" }
+
+    it 'broadcasts config update events with interpolated string' do
+      gql.execute(subscription, context: { channel: mock_channel })
+      setting.save
+      expect(mock_channel.mock_broadcasted_messages).to eq([expected_msg])
+    end
   end
 end

spec/models/setting_spec.rb

@@ -12,6 +12,18 @@
           .to change { described_class.get(setting.name) }.to('foo')
       end
     end
+
+    context 'when interpolated value was set and cache is still valid' do
+      it 'stores interpolated value' do
+        create(:setting, name: 'broadcast_test', state: 'test')
+        described_class.send(:load) # prewarm cache
+
+        described_class.set('broadcast_test', 'test #{config.fqdn}') # rubocop:disable Lint/InterpolationCheck
+
+        expect(described_class.get('broadcast_test'))
+          .to eq("test #{described_class.get('fqdn')}")
+      end
+    end
   end
 
   describe '.set' do
@@ -57,34 +69,84 @@
     end
   end
 
-  describe 'check_broadcast' do
+  describe 'broadcast_frontend' do
+    subject(:setting) do
+      build(:setting, name: 'broadcast_test', state: value, frontend: frontend)
+        .tap { |setting| setting.preferences = { authentication: true } if authentication_required }
+    end
+
+    let(:value)                   { 'foo' }
+    let(:frontend)                { true }
+    let(:authentication_required) { false }
+
     context 'when setting is non-frontend' do
-      subject(:setting) { build(:setting, name: 'broadcast_test', state: 'foo', frontend: false) }
+      let(:frontend) { false }
 
       it 'does not broadcast' do
         allow(Sessions).to receive(:broadcast)
         setting.save
         expect(Sessions).not_to have_received(:broadcast)
       end
+
+      it 'does not trigger subscription' do
+        allow(Gql::Subscriptions::ConfigUpdates).to receive(:trigger)
+        setting.save
+        expect(Gql::Subscriptions::ConfigUpdates).not_to have_received(:trigger).with(setting)
+      end
     end
 
     context 'when setting is public' do
-      subject(:setting) { build(:setting, name: 'broadcast_test', state: 'foo', frontend: true) }
-
       it 'broadcasts to public' do
         allow(Sessions).to receive(:broadcast)
         setting.save
-        expect(Sessions).to have_received(:broadcast).with({ data: { name: 'broadcast_test', value: 'foo' }, event: 'config_update' }, 'public')
+        expect(Sessions).to have_received(:broadcast)
+          .with({ data: { name: 'broadcast_test', value: 'foo' }, event: 'config_update' }, 'public')
+      end
+
+      it 'triggers subscription' do
+        allow(Gql::Subscriptions::ConfigUpdates).to receive(:trigger)
+        setting.save
+        expect(Gql::Subscriptions::ConfigUpdates).to have_received(:trigger).with(setting)
       end
     end
 
     context 'when setting requires authentication' do
-      subject(:setting) { build(:setting, name: 'broadcast_test', state: 'foo', frontend: true, preferences: { authentication: true }) }
+      let(:authentication_required) { true }
 
       it 'broadcasts to authenticated only' do
         allow(Sessions).to receive(:broadcast)
         setting.save
-        expect(Sessions).to have_received(:broadcast).with({ data: { name: 'broadcast_test', value: 'foo' }, event: 'config_update' }, 'authenticated')
+        expect(Sessions).to have_received(:broadcast)
+          .with({ data: { name: 'broadcast_test', value: 'foo' }, event: 'config_update' }, 'authenticated')
+      end
+
+      it 'triggers subscription' do
+        allow(Gql::Subscriptions::ConfigUpdates).to receive(:trigger)
+        setting.save
+        expect(Gql::Subscriptions::ConfigUpdates).to have_received(:trigger).with(setting)
+      end
+    end
+
+    context 'when setting uses interpolation' do
+      let(:value) { 'test #{config.fqdn}' } # rubocop:disable Lint/InterpolationCheck
+
+      it 'broadcasts to authenticated only' do
+        allow(Sessions).to receive(:broadcast)
+
+        setting.save
+
+        expect(Sessions)
+          .to have_received(:broadcast)
+          .with(
+            { data: { name: 'broadcast_test', value: "test #{described_class.get('fqdn')}" }, event: 'config_update' },
+            'public'
+          )
+      end
+
+      it 'triggers subscription' do
+        allow(Gql::Subscriptions::ConfigUpdates).to receive(:trigger)
+        setting.save
+        expect(Gql::Subscriptions::ConfigUpdates).to have_received(:trigger).with(setting)
       end
     end
   end