Merge pull request #4392 from esl/muc-light-multiple-owners

Multiple owner support in muc_light

big_tests/tests/muc_light_SUITE.erl

@@ -53,7 +53,9 @@
          deny_config_change_that_conflicts_with_schema/1,
          assorted_config_doesnt_lead_to_duplication/1,
          remove_and_add_users/1,
+         multiple_owner_change/1,
          explicit_owner_change/1,
+         explicit_owner_handover/1,
          implicit_owner_change/1,
          edge_case_owner_change/1,
          adding_wrongly_named_user_triggers_infinite_loop/1
@@ -179,6 +181,8 @@ groups() ->
                               assorted_config_doesnt_lead_to_duplication,
                               remove_and_add_users,
                               explicit_owner_change,
+                              explicit_owner_handover,
+                              multiple_owner_change,
                               implicit_owner_change,
                               edge_case_owner_change,
                               adding_wrongly_named_user_triggers_infinite_loop
@@ -298,6 +302,8 @@ muc_light_opts(block_user) ->
     #{all_can_invite => true};
 muc_light_opts(blocking_disabled) ->
     #{blocking => false};
+muc_light_opts(multiple_owner_change) ->
+    #{allow_multiple_owners => true};
 muc_light_opts(_) ->
     #{}.
 
@@ -792,6 +798,14 @@ remove_and_add_users(Config) ->
             assert_cache_hit_event(TS, 1, room_bin_jid(?ROOM))
         end).
 
+multiple_owner_change(Config) ->
+    escalus:story(Config, [{alice, 1}, {bob, 1}, {kate, 1}], fun(Alice, Bob, Kate) ->
+            AffUsersChanges2 = [{Bob, owner}],
+            escalus:send(Alice, stanza_aff_set(?ROOM, AffUsersChanges2)),
+            verify_aff_bcast([{Alice, owner}, {Bob, owner}, {Kate, member}], AffUsersChanges2),
+            escalus:assert(is_iq_result, escalus:wait_for_stanza(Alice))
+        end).
+
 explicit_owner_change(Config) ->
     escalus:story(Config, [{alice, 1}, {bob, 1}, {kate, 1}], fun(Alice, Bob, Kate) ->
             AffUsersChanges1 = [{Bob, none}, {Alice, none}, {Kate, owner}],
@@ -800,6 +814,16 @@ explicit_owner_change(Config) ->
             escalus:assert(is_iq_result, escalus:wait_for_stanza(Alice))
         end).
 
+explicit_owner_handover(Config) ->
+    % check that Alice looses ownership when Kate is set to owner
+    escalus:story(Config, [{alice, 1}, {bob, 1}, {kate, 1}], fun(Alice, Bob, Kate) ->
+        ReqAffUsersChanges = [{Kate, owner}],
+        escalus:send(Alice, stanza_aff_set(?ROOM, ReqAffUsersChanges)),
+        ExpectedAffUserChanges = [{Alice, member} | ReqAffUsersChanges],
+        verify_aff_bcast([{Kate, owner}, {Alice, member}, {Bob, member}], ExpectedAffUserChanges),
+        escalus:assert(is_iq_result, escalus:wait_for_stanza(Alice))
+    end).
+
 implicit_owner_change(Config) ->
     escalus:story(Config, [{alice, 1}, {bob, 1}, {kate, 1}], fun(Alice, Bob, Kate) ->
             AffUsersChanges1 = [{Bob, none}, {Alice, member}],

doc/modules/mod_muc_light.md

@@ -109,6 +109,21 @@ Specifies maximal number of rooms returned for a single Disco request.
 
 When enabled, rooms the user occupies are included in their roster.
 
+### `modules.mod_muc_light.allow_multiple_owners`
+* **Syntax:** boolean
+* **Default:** `false`
+* **Example:** `allow_multiple_owners = true`
+
+When enabled, owner can add one or more people as owners.
+If disabled, there can only be one owner.
+This option may be useful for creating a subset of users with admin rights, instead of giving rights for all members,
+which can be done with the [all_can_configure](#modulesmod_muc_lightall_can_configure) and
+[all_can_invite](#modulesmod_muc_lightall_can_invite) options.
+
+!!! Warning
+    This is a custom option, not compatible with our [MUC Light XEP](../open-extensions/muc_light.md).
+    If a client is adhering to the XEP, its behaviour may be unexpected, and this option should not be enabled.
+
 ### `modules.mod_muc_light.config_schema`
   * **Syntax:** an array of `config_schema` items, as described below
   * **Default:**

include/mod_muc_light.hrl

@@ -15,6 +15,7 @@
 -define(DEFAULT_MAX_OCCUPANTS, infinity).
 -define(DEFAULT_ROOMS_PER_PAGE, 10).
 -define(DEFAULT_ROOMS_IN_ROSTERS, false).
+-define(DEFAULT_ALLOW_MULTIPLE_OWNERS, false).
 
 -type aff() :: owner | member | none.
 -type aff_user() :: {jid:simple_bare_jid(), aff()}.

src/muc_light/mod_muc_light.erl

@@ -231,6 +231,7 @@ config_spec() ->
                  <<"rooms_per_page">> => #option{type = int_or_infinity,
                                                  validate = positive},
                  <<"rooms_in_rosters">> => #option{type = boolean},
+                 <<"allow_multiple_owners">> => #option{type = boolean},
                  <<"config_schema">> => #list{items = config_schema_spec(),
                                               process = fun ?MODULE:process_config_schema/1}
                 },
@@ -245,6 +246,7 @@ config_spec() ->
                     <<"max_occupants">> => ?DEFAULT_MAX_OCCUPANTS,
                     <<"rooms_per_page">> => ?DEFAULT_ROOMS_PER_PAGE,
                     <<"rooms_in_rosters">> => ?DEFAULT_ROOMS_IN_ROSTERS,
+                    <<"allow_multiple_owners">> => ?DEFAULT_ALLOW_MULTIPLE_OWNERS,
                     <<"config_schema">> => default_schema()}
       }.
 
@@ -658,19 +660,19 @@ process_create_aff_users_if_valid(HostType, Creator, AffUsers) ->
     case lists:any(fun ({User, _}) when User =:= Creator -> true;
                        ({_, Aff}) -> Aff =:= none end, AffUsers) of
         false ->
-            process_create_aff_users(Creator, AffUsers, equal_occupants(HostType));
+            process_create_aff_users(HostType, Creator, AffUsers, equal_occupants(HostType));
         true ->
             {error, bad_request}
     end.
 
 equal_occupants(HostType) ->
     gen_mod:get_module_opt(HostType, ?MODULE, equal_occupants).
 
--spec process_create_aff_users(Creator :: jid:simple_bare_jid(), AffUsers :: aff_users(),
+-spec process_create_aff_users(HostType :: host_type(), Creator :: jid:simple_bare_jid(), AffUsers :: aff_users(),
                                EqualOccupants :: boolean()) ->
     {ok, aff_users()} | {error, bad_request}.
-process_create_aff_users(Creator, AffUsers, EqualOccupants) ->
-    case mod_muc_light_utils:change_aff_users([{Creator, creator_aff(EqualOccupants)}], AffUsers) of
+process_create_aff_users(HostType, Creator, AffUsers, EqualOccupants) ->
+    case mod_muc_light_utils:change_aff_users(HostType, [{Creator, creator_aff(EqualOccupants)}], AffUsers) of
         {ok, FinalAffUsers, _ChangedAffUsers, _JoiningUsers, _LeavingUsers} -> {ok, FinalAffUsers};
         Error -> Error
     end.

src/muc_light/mod_muc_light_db_mnesia.erl

@@ -126,9 +126,9 @@ get_user_rooms_count(_HostType, UserUS) ->
                   UserUS :: jid:simple_bare_jid(),
                   Version :: binary()) ->
     mod_muc_light_db_backend:remove_user_return() | {error, term()}.
-remove_user(_HostType, UserUS, Version) ->
+remove_user(HostType, UserUS, Version) ->
     mnesia:dirty_delete(muc_light_blocking, UserUS),
-    {atomic, Res} = mnesia:transaction(fun remove_user_transaction/2, [UserUS, Version]),
+    {atomic, Res} = mnesia:transaction(fun remove_user_transaction/3, [HostType, UserUS, Version]),
     Res.
 
 -spec remove_domain(mongooseim:host_type(), jid:lserver(), jid:lserver()) -> ok.
@@ -210,9 +210,9 @@ get_aff_users(_HostType, RoomUS) ->
                        ExternalCheck :: external_check_fun(),
                        Version :: binary()) ->
     mod_muc_light_db_backend:modify_aff_users_return().
-modify_aff_users(_HostType, RoomUS, AffUsersChanges, ExternalCheck, Version) ->
-    {atomic, Res} = mnesia:transaction(fun modify_aff_users_transaction/4,
-                                       [RoomUS, AffUsersChanges, ExternalCheck, Version]),
+modify_aff_users(HostType, RoomUS, AffUsersChanges, ExternalCheck, Version) ->
+    {atomic, Res} = mnesia:transaction(fun modify_aff_users_transaction/5,
+                                       [HostType, RoomUS, AffUsersChanges, ExternalCheck, Version]),
     Res.
 
 %% ------------------------ Misc ------------------------
@@ -311,14 +311,18 @@ destroy_room_transaction(RoomUS) ->
             mnesia:delete({muc_light_room, RoomUS})
     end.
 
--spec remove_user_transaction(UserUS :: jid:simple_bare_jid(), Version :: binary()) ->
+-spec remove_user_transaction(HostType :: mongooseim:host_type(),
+                              UserUS :: jid:simple_bare_jid(),
+                              Version :: binary()) ->
     mod_muc_light_db_backend:remove_user_return().
-remove_user_transaction(UserUS, Version) ->
+remove_user_transaction(HostType, UserUS, Version) ->
     lists:map(
-      fun(#muc_light_user_room{ room = RoomUS }) ->
-              {RoomUS, modify_aff_users_transaction(
-                         RoomUS, [{UserUS, none}], fun(_, _) -> ok end, Version)}
-      end, mnesia:read(muc_light_user_room, UserUS)).
+        fun(#muc_light_user_room{room = RoomUS}) ->
+            Res = modify_aff_users_transaction(
+                HostType, RoomUS, [{UserUS, none}], fun(_, _) -> ok end, Version),
+            {RoomUS, Res}
+        end,
+        mnesia:read(muc_light_user_room, UserUS)).
 
 %% ------------------------ Configuration manipulation ------------------------
 
@@ -345,17 +349,18 @@ dirty_get_blocking_raw(UserUS) ->
 
 %% ------------------------ Affiliations manipulation ------------------------
 
--spec modify_aff_users_transaction(RoomUS :: jid:simple_bare_jid(),
+-spec modify_aff_users_transaction(HostType :: mongooseim:host_type(),
+                                   RoomUS :: jid:simple_bare_jid(),
                                    AffUsersChanges :: aff_users(),
                                    ExternalCheck :: external_check_fun(),
                                    Version :: binary()) ->
     mod_muc_light_db_backend:modify_aff_users_return().
-modify_aff_users_transaction(RoomUS, AffUsersChanges, ExternalCheck, Version) ->
+modify_aff_users_transaction(HostType, RoomUS, AffUsersChanges, ExternalCheck, Version) ->
     case mnesia:wread({muc_light_room, RoomUS}) of
         [] ->
             {error, not_exists};
         [#muc_light_room{ aff_users = AffUsers } = RoomRec] ->
-            case mod_muc_light_utils:change_aff_users(AffUsers, AffUsersChanges) of
+            case mod_muc_light_utils:change_aff_users(HostType, AffUsers, AffUsersChanges) of
                 {ok, NewAffUsers, _, _, _} = ChangeResult ->
                     verify_externally_and_submit(
                       RoomUS, RoomRec, ChangeResult, ExternalCheck(RoomUS, NewAffUsers), Version);

src/muc_light/mod_muc_light_db_rdbms.erl

@@ -752,7 +752,7 @@ modify_aff_users_transaction(HostType, RoomUS, RoomID, AffUsersChanges,
                              CheckFun, PrevVersion, Version) ->
     {selected, AffUsersDB} = select_affs_by_room_id(HostType, RoomID),
     AffUsers = decode_affs(AffUsersDB),
-    case mod_muc_light_utils:change_aff_users(AffUsers, AffUsersChanges) of
+    case mod_muc_light_utils:change_aff_users(HostType, AffUsers, AffUsersChanges) of
         {ok, NewAffUsers, AffUsersChanged, JoiningUsers, _LeavingUsers} ->
             case CheckFun(RoomUS, NewAffUsers) of
                 ok ->

src/muc_light/mod_muc_light_utils.erl

@@ -24,7 +24,7 @@
 -author('[email protected]').
 
 %% API
--export([change_aff_users/2]).
+-export([change_aff_users/3]).
 -export([b2aff/1, aff2b/1]).
 -export([light_aff_to_muc_role/1]).
 -export([room_limit_reached/2]).
@@ -57,21 +57,22 @@
 %% API
 %%====================================================================
 
--spec change_aff_users(CurrentAffUsers :: aff_users(), AffUsersChangesAssorted :: aff_users()) ->
+-spec change_aff_users(HostType :: mongooseim:host_type(), CurrentAffUsers :: aff_users(), AffUsersChangesAssorted :: aff_users()) ->
     change_aff_success() | {error, bad_request()}.
-change_aff_users(AffUsers, AffUsersChangesAssorted) ->
+change_aff_users(HostType, AffUsers, AffUsersChangesAssorted) ->
     case {lists:keyfind(owner, 2, AffUsers), lists:keyfind(owner, 2, AffUsersChangesAssorted)} of
         {false, false} -> % simple, no special cases
             apply_aff_users_change(AffUsers, AffUsersChangesAssorted);
         {false, {_, _}} -> % ownerless room!
             {error, {bad_request, <<"Ownerless room">>}};
         _ ->
+            MultipleOwners = allow_multiple_owners(HostType),
             lists:foldl(fun(F, Acc) -> F(Acc) end,
                         apply_aff_users_change(AffUsers, AffUsersChangesAssorted),
-                        [fun maybe_demote_old_owner/1,
-                         fun maybe_select_new_owner/1])
+                        change_aff_functions(MultipleOwners))
     end.
 
+
 -spec aff2b(Aff :: aff()) -> binary().
 aff2b(owner) -> <<"owner">>;
 aff2b(member) -> <<"member">>;
@@ -156,6 +157,12 @@ filter_out_loop(_HostType, _FromUS, _MUCServer, _BlockingQuery, _RoomsPerUser, [
 
 %% ---------------- Affiliations manipulation ----------------
 
+-spec change_aff_functions(AllowMultipleOwners :: boolean()) -> [fun(), ...].
+change_aff_functions(_AllowMultipleOwners = false)->
+    [fun maybe_demote_old_owner/1, fun maybe_select_new_owner/1];
+change_aff_functions(_AllowMultipleOwners = true)->
+    [fun maybe_select_new_owner/1].
+
 -spec maybe_select_new_owner(ChangeResult :: change_aff_success() | {error, bad_request()}) ->
     change_aff_success() | {error, bad_request()}.
 maybe_select_new_owner({ok, AU, AUC, JoiningUsers, LeavingUsers} = _AffRes) ->
@@ -317,3 +324,6 @@ run_forget_room_hook({Room, MucHost}) ->
             ?LOG_ERROR(#{what => run_forget_room_hook_skipped,
                          room => Room, muc_host => MucHost})
     end.
+
+allow_multiple_owners(HostType) ->
+    gen_mod:get_module_opt(HostType, mod_muc_light, allow_multiple_owners).

test/common/config_parser_helper.erl

@@ -976,6 +976,7 @@ default_mod_config(mod_muc_light) ->
       max_occupants => infinity,
       rooms_per_page => 10,
       rooms_in_rosters => false,
+      allow_multiple_owners => false,
       config_schema => [{<<"roomname">>, <<"Untitled">>, roomname, binary},
                         {<<"subject">>, <<>>, subject, binary}]};
 default_mod_config(mod_muc_log) ->

test/config_parser_SUITE.erl

@@ -2344,6 +2344,8 @@ mod_muc_light(_Config) ->
           T(#{<<"rooms_per_page">> => 10})),
     ?cfgh(P ++ [rooms_in_rosters], true,
           T(#{<<"rooms_in_rosters">> => true})),
+   ?cfgh(P ++ [allow_multiple_owners], true,
+         T(#{<<"allow_multiple_owners">> => true})),
     ?errh(T(#{<<"backend">> => <<"frontend">>})),
     ?errh(T(#{<<"host">> => <<"what is a domain?!">>})),
     ?errh(T(#{<<"host">> => <<"invalid..com">>})),
@@ -2359,6 +2361,7 @@ mod_muc_light(_Config) ->
     ?errh(T(#{<<"all_can_invite">> => #{}})),
     ?errh(T(#{<<"max_occupants">> => <<"seven">>})),
     ?errh(T(#{<<"rooms_per_page">> => false})),
+    ?errh(T(#{<<"allow_multiple_owners">> => <<"true">>})),
     ?errh(T(#{<<"rooms_in_rosters">> => [1, 2, 3]})).
 
 mod_muc_light_config_schema(_Config) ->

test/muc_light_SUITE.erl

@@ -164,7 +164,7 @@ filter_room_packet_handler(Acc, _Params, _Extra) ->
 prop_aff_change_success() ->
     ?FORALL({AffUsers, Changes, Joining, Leaving, WithOwner}, change_aff_params(),
             begin
-                case mod_muc_light_utils:change_aff_users(AffUsers, Changes) of
+                case mod_muc_light_utils:change_aff_users(host_type(), AffUsers, Changes) of
                     {ok, NewAffUsers0, AffUsersChanged, Joining0, Leaving0} ->
                         Joining = lists:sort(Joining0),
                         Leaving = lists:sort(Leaving0),
@@ -178,7 +178,7 @@ prop_aff_change_success() ->
                         % are there no owners or there is exactly one?
                         true = validate_owner(NewAffUsers0, false, WithOwner),
                         % changes list applied to old list should produce the same result
-                        {ok, NewAffUsers1, _, _, _} = mod_muc_light_utils:change_aff_users(AffUsers, AffUsersChanged),
+                        {ok, NewAffUsers1, _, _, _} = mod_muc_light_utils:change_aff_users(host_type(), AffUsers, AffUsersChanged),
                         NewAffUsers0 = NewAffUsers1,
                         true;
                     _ ->
@@ -198,7 +198,7 @@ prop_aff_change_bad_request() ->
     ?FORALL({AffUsers, Changes}, bad_change_aff(),
             begin
                 {error, {bad_request, _}} =
-                    mod_muc_light_utils:change_aff_users(AffUsers, Changes),
+                    mod_muc_light_utils:change_aff_users(host_type(), AffUsers, Changes),
                 true
             end).