mod_pubsub force_node_config doesn't override details in the database

[mzealey]

Is your feature request related to a problem? Please describe.

When we change something such as

        mod_pubsub:
            force_node_config:
                "urn:xmpp:avatar:*":
                    access_model: open

it only applies to newly created items rather than all items that exist

Describe the solution you'd like

I believe the node_config() merge of data + forced config should happen at retrieval time as well as in create_node.

Describe alternatives you've considered

At the very least this non-modification of existing records should be documented behaviour.

[badlop]

default_node_config sets the node options when it's created. Those options can later be reconfigured by the node owner, etc.

force_node_config, considering the option name, should overwrite in realtime whatever node options are correctly stored. Not only when the node is created, also when it's reconfigured and when it's being used. But right now force_node_config is read in line 3404 and used:

• when creating the node 1533
• when setting configuration 3484

It's not used when accessing the node itself! so those options apparently are not forced...

This is a quick patch, it may not be complete, and may not be the most efficient, but if can give it a try:

diff --git a/src/mod_pubsub.erl b/src/mod_pubsub.erl
index 7b01c4af0..b5241b1ef 100644
--- a/src/mod_pubsub.erl
+++ b/src/mod_pubsub.erl
@@ -3893,7 +3893,11 @@ transaction(Host, Node, Action, Trans) ->
       Host,
       fun() ->
 	      case tree_call(Host, get_node, [Host, Node]) of
-		  N when is_record(N, pubsub_node) ->
+		  NRaw when is_record(NRaw, pubsub_node) ->
+                      OptionsForced = merge_config(
+                                        [node_config(Node, serverhost(Host)),
+                                         NRaw#pubsub_node.options]),
+                      N = NRaw#pubsub_node{options = OptionsForced},
 		      case Action(N) of
 			  {result, Result} -> {result, {N, Result}};
 			  {atomic, {result, Result}} -> {result, {N, Result}};

[edhelas]

+1 on that one :)

[badlop]

do you mean +1 on the feature request? Or do you mean +1 on the patch (did you test it, does it work correctly, and covers the feature request completely)?

[edhelas]

On the feature itself. @weiss told me that the patch is kinda dirty and the behavior not that great. But force_node_config should enforce also things when they are requested from the DB AFAIK. Until now I was manually updating the db to fit with the new freshly set config.

[weiss]

But force_node_config should enforce also things when they are requested from the DB AFAIK.

I totally see how that behavior is probably desired in most cases, but I do think it's hairy to have the code override the node configuration stored in the DB on-the-fly. We'd at least have to be very sure to get the abstractions right so the override mechanism really works as desired no matter the DB query context.

Either way, I think the original behavior was consciously designed the way it is, i.e. "enforce settings while writing the config, not while reading it". I.e. this wasn't an oversight or something.