processone / ejabberd

Robust, Ubiquitous and Massively Scalable Messaging Platform (XMPP, MQTT, SIP Server)
https://www.process-one.net/en/ejabberd/
Other
6.01k stars 1.5k forks source link

default_node_config is ignored when new nodes are created #4070

Closed alinradut closed 11 months ago

alinradut commented 11 months ago

Environment

Configuration (only if needed): grep -Ev '^$|^\s*#' ejabberd.yml

  mod_pubsub:
    db_type: sql
    access_createnode: pubsub_createnode
    plugins:
      - flat
      - pep
    force_node_config:
      ## Avoid buggy clients to make their bookmarks public
      storage:bookmarks:
        access_model: whitelist
    default_node_config:
      send_last_published_item: never
      max_items: 4

Errors from error.log/crash.log

No errors

Bug description

I have a fairly standard ejabberd install backed by MySQL and I want to pre-configure the PubSub nodes to set send_last_published_item to never or on_sub. Currently, I have users who can be subscribed to as many as 50 PubSub nodes which flood the client after logging in and I want to dial that down a bit as I am expecting that number to go up significantly in the future.

I updated ejabberd.yml and set a few values in default_node_config and then I created a new node via

<iq type='set' id='some-id'>
  <pubsub xmlns='http://jabber.org/protocol/pubsub'>
    <create node='123'/>
  </pubsub>
</iq>

However, when I checked the database, the new node ignored all defaults from default_node_config. This node has not been modified by someone else, it is created entirely anew with a configuration that ignores the defaults:

SELECT * FROM `pubsub_node_option` WHERE nodeid=9;

nodeid  name  val 
9 access_model  presence  
9 deliver_notifications true  
9 deliver_payloads  true  
9 itemreply none  
9 max_items 1 
9 max_payload_size  250000  
9 notification_type headline  
9 notify_config false 
9 notify_delete false 
9 notify_retract  false 
9 persist_items true  
9 presence_based_delivery true  
9 publish_model publishers  
9 purge_offline false 
9 roster_groups_allowed []  
9 rsm true  
9 send_last_published_item  on_sub_and_presence 
9 sql true  
9 subscribe true  

These values are suspiciously similar to the defaults from ejabberd/src/node_flat.erl.

This is what the configuration looks like:

  mod_pubsub:
    db_type: sql
    access_createnode: pubsub_createnode
    plugins:
      - flat
      - pep
    force_node_config:
      ## Avoid buggy clients to make their bookmarks public
      storage:bookmarks:
        access_model: whitelist
    default_node_config:
      send_last_published_item: never
      notification_type: normal
      notify_retract: false
      max_items: 4
      delivers_notifications: false
      notify_delete: true
      max_payload_size: 100

I've read the documentation so many times and the description for default_node_config is confusing to me:

default_node_config

To override default node configuration, regardless of node plugin. Value is a list of key-value definition. Node configuration still uses default configuration defined by node plugin, and overrides any items by value defined in this configurable list.

I read this as [This is to be used] to override default node configuration, regardless of node plugin. But then it says that the Node configuration still uses default configuration defined by the node plugin, and overrides any items by value defined in this configurable list.

This can be interpreted as default_node_config having precedence, but the second sentence is weird to me.

Am I doing something wrong or is this a bug?

Is there a way to configure the defaults for the node plugin? I am using the "flat" type for nodes.

Thanks!

badlop commented 11 months ago

There is a bug in mod_pubsub.erl: only the first plugin in the option plugins is checked for default_node_config. In practice, this initial configuration:

modules:
  mod_pubsub:
    plugins:
      - flat
      - pep
    force_node_config:
      a123:
        send_last_published_item: forced
    default_node_config:
      send_last_published_item: defaulted

produces those results depending on what options are provided or removed:


Quick and dirty solution: put the first in the list of pluginsthe one that you want to get options from default_node_config.

modules:
  mod_pubsub:
    plugins:
      - pep
      - flat

Definitive solution: this simple fix:

diff --git a/src/mod_pubsub.erl b/src/mod_pubsub.erl
index b8f4c4905..8563ba2d1 100644
--- a/src/mod_pubsub.erl
+++ b/src/mod_pubsub.erl
@@ -3367,9 +3367,10 @@ get_option(Options, Var, Def) ->
 -spec node_options(host(), binary()) -> [{atom(), any()}].
 node_options(Host, Type) ->
     DefaultOpts = node_plugin_options(Host, Type),
-    case config(Host, plugins) of
-   [Type|_] -> config(Host, default_node_config, DefaultOpts);
-   _ -> DefaultOpts
+    case lists:member(Type, config(Host, plugins)) of
+   true ->
+            config(Host, default_node_config, DefaultOpts);
+   false -> DefaultOpts
     end.

 -spec node_plugin_options(host(), binary()) -> [{atom(), any()}].

with the fix: