Affiliation change not sent to MUCSUB

mzealey commented 11 months ago

Environment

ejabberd version: master git
Erlang version: Erlang (SMP,ASYNC_THREADS) (BEAM) emulator version 13.2.2.2
Docker container: erlang:25.3.2.3
Installed from: source

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

MUC config:

        mod_muc:
            hosts:
            - groups.user.moya

            access:
            - allow
            access_admin: none
            access_create:
            - allow
            access_persistent:
            - allow

            default_room_options:
              allow_subscription: true
              lang: ""
              persistent: true
              public: false
              public_list: false

            history_size: 0
            max_users: 500
            max_user_conferences: 100
            min_presence_interval: 4
            preload_rooms: false

Bug description

User2 joins a group via mucsub:

<iq type="set" to="random-room-name-1690361951.145076@groups.user.moya" from="user2@user.moya/Moya" id="5be32683cc8a407e9631ef2eb59d2828"><subscribe xmlns="urn:xmpp:mucsub:0" nick="user2"><event node="urn:xmpp:mucsub:nodes:messages" /><event node="urn:xmpp:mucsub:nodes:affiliations" /><event node="urn:xmpp:mucsub:nodes:subject" /><event node="urn:xmpp:mucsub:nodes:config" /><event node="urn:xmpp:mucsub:nodes:subscribers" /></subscribe></iq>

User 1 promotes them to admin:

<iq id="bf33df418c4f43e6b325c34017534cd2" type="set" to="random-room-name-1690361951.145076@groups.user.moya"><query xmlns="http://jabber.org/protocol/muc#admin"><item affiliation="admin" jid="user2@user.moya/Moya" /></query></iq>

However user 2 never receives a presence/affiliation notification because of the following code in src/mod_muc_room.erl:

        {JID, affiliation, A, Reason} when (A == admin) or (A == owner) ->
        SD1 = set_affiliation(JID, A, SD, Reason),
        SD2 = set_role(JID, moderator, SD1),
        send_update_presence(JID, Reason, SD2, SD),
        maybe_send_affiliation(JID, A, SD2),

If we replace maybe_send_affiliation with send_affiliation then all users in the chat group receive:

<message to="user2@user.moya" from="random-room-name-1690361951.145076@groups.user.moya" id="4536439855475222774"><event xmlns="http://jabber.org/protocol/pubsub#event"><items node="urn:xmpp:mucsub:nodes:affiliations"><item id="4536439855475222774"><message xmlns="jabber:client" to="user2@user.moya" from="random-room-name-1690361951.145076@groups.user.moya" id="1435653133147385454"><x xmlns="http://jabber.org/protocol/muc#user"><item jid="user2@user.moya" role="none" affiliation="admin" /></x></message></item></items></event></message>

which is what we expected to happen.

I notice that the code also tries to send presence updates, however we never receive them (probably because it's mucsub rather than muc?). Either way, for mucsub we believe that affiliation is the correct approach because these messages are stored in the offline queue so will be guaranteed to be delivered to the user unlike presence notifications.

prefiks commented 11 months ago

We are using maybe_send_affiliation instead of send_affiliation because send_update_presence called above also can send those notification, so if we would use send_affiliation we could send those notifications twice. What i think is happening here is that notifications should be generated by send_update_presence but most likely is_room_overcrowded check (aka room has more participants that what mod_muc option max_users_presence define), tells that function to not generate presences. I will need to think what we should be doing there...

mzealey commented 11 months ago

I thought this overcrowding may be the case on our production servers, however I can reliably reproduce this bug with just 2 users in a fresh chatroom on a local docker-compose platform, and the config above.

mzealey commented 11 months ago

And in the overcrowded state, I would think the correct approach is to at least send the notification to all admins + the new person as their UI may need to update to give them the extra features available to admins.

processone / ejabberd