Keep the Pid-based API in mod_muc_room

big_tests/tests/graphql_muc_SUITE.erl

@@ -87,6 +87,7 @@ admin_muc_handler() ->
      admin_try_set_nonexistent_room_user_affiliation,
      admin_set_user_role,
      admin_try_set_nonexistent_room_user_role,
+     admin_try_set_user_role_in_room_without_moderators,
      admin_make_user_enter_room,
      admin_make_user_enter_room_with_password,
      admin_make_user_enter_room_bare_jid,
@@ -452,6 +453,16 @@ admin_set_user_role(Config, Alice, Bob) ->
     assert_success(?SET_ROLE_PATH, Res2),
     assert_user_role(RoomJID, Bob, moderator).
 
+admin_try_set_user_role_in_room_without_moderators(Config) ->
+    muc_helper:story_with_room(Config, [], [{alice, 1}, {bob, 1}], fun admin_try_set_user_role_in_room_without_moderators/3).
+
+admin_try_set_user_role_in_room_without_moderators(Config, _Alice, Bob) ->
+    RoomJID = jid:from_binary(?config(room_jid, Config)),
+    BobNick = <<"Boobek">>,
+    enter_room(RoomJID, Bob, BobNick),
+    Res = execute_auth(set_user_role_body(RoomJID, BobNick, visitor), Config),
+    ?assertNotEqual(nomatch, binary:match(get_err_msg(Res), <<"not found">>)).
+
 admin_try_set_nonexistent_room_user_role(Config) ->
     Res = execute_auth(set_user_role_body(?NONEXISTENT_ROOM, <<"Alice">>, moderator), Config),
     ?assertNotEqual(nomatch, binary:match(get_err_msg(Res), <<"not found">>)).

src/mod_muc_api.erl

@@ -71,30 +71,25 @@ get_rooms(MUCServer, From, Limit, Index) ->
 -spec get_room_config(jid:jid(), jid:jid()) ->
     {ok, mod_muc_room:config()} | {not_allowed | room_not_found, iolist()}.
 get_room_config(RoomJID, UserJID) ->
-    case mod_muc:room_jid_to_pid(RoomJID) of
-        {ok, Pid} ->
-            case gen_fsm_compat:sync_send_all_state_event(Pid, {is_room_owner, UserJID}) of
-                {ok, true} ->
-                    {ok, Config} = gen_fsm_compat:sync_send_all_state_event(Pid, get_config),
-                    {ok, Config};
-                {ok, false} ->
-                    {not_allowed, "Given user does not have permission to read config"}
-            end;
+    case mod_muc_room:is_room_owner(RoomJID, UserJID) of
+        {ok, true} ->
+            {ok, Config} = mod_muc_room:get_room_config(RoomJID),
+            {ok, Config};
+        {ok, false} ->
+            {not_allowed, "Given user does not have permission to read config"};
         {error, not_found} ->
             ?ROOM_NOT_FOUND_RESULT
     end.
 
 -spec get_room_config(jid:jid()) -> {ok, mod_muc_room:config()} | {room_not_found, iolist()}.
 get_room_config(RoomJID) ->
-    case mod_muc:room_jid_to_pid(RoomJID) of
-        {ok, Pid} ->
-            {ok, Config} = gen_fsm_compat:sync_send_all_state_event(Pid, get_config),
+    case mod_muc_room:get_room_config(RoomJID) of
+        {ok, Config} ->
             {ok, Config};
         {error, not_found} ->
             ?ROOM_NOT_FOUND_RESULT
     end.
 
-
 -spec create_instant_room(jid:lserver(), binary(), jid:jid(), binary()) ->
     {ok, short_room_desc()} | {internal | not_found, iolist()}.
 create_instant_room(MUCDomain, Name, OwnerJID, Nick) ->
@@ -124,14 +119,11 @@ create_instant_room(MUCDomain, Name, OwnerJID, Nick) ->
 -spec modify_room_config(jid:jid(), jid:jid(), room_conf_mod_fun()) ->
     {ok, mod_muc_room:config()} | {room_not_found, iolist()}.
 modify_room_config(RoomJID, UserJID, Fun) ->
-    case mod_muc:room_jid_to_pid(RoomJID) of
-        {ok, Pid} ->
-            case gen_fsm_compat:sync_send_all_state_event(Pid, {is_room_owner, UserJID}) of
-                {ok, true} ->
-                    modify_room_config_raw(Pid, Fun);
-                {ok, false} ->
-                    {not_allowed, "Given user does not have permission to change the config"}
-            end;
+    case mod_muc_room:is_room_owner(RoomJID, UserJID) of
+        {ok, true} ->
+            {ok, modify_room_config_raw(RoomJID, Fun)};
+        {ok, false} ->
+            {not_allowed, "Given user does not have permission to change the config"};
         {error, not_found} ->
             ?ROOM_NOT_FOUND_RESULT
     end.
@@ -140,8 +132,8 @@ modify_room_config(RoomJID, UserJID, Fun) ->
     {ok, mod_muc_room:config()} | {room_not_found, iolist()}.
 modify_room_config(RoomJID, Fun) ->
     case mod_muc:room_jid_to_pid(RoomJID) of
-        {ok, Pid} ->
-            modify_room_config_raw(Pid, Fun);
+        {ok, _} ->
+            {ok, modify_room_config_raw(RoomJID, Fun)};
         {error, not_found} ->
             ?ROOM_NOT_FOUND_RESULT
     end.
@@ -191,41 +183,46 @@ send_private_message(RoomJID, SenderJID, ToNick, Message) ->
 kick_user_from_room(RoomJID, Nick, ReasonIn) ->
     %% All the machinery which is already deeply embedded in the MUC
     %% modules will perform the neccessary checking.
-    SenderJID = room_moderator(RoomJID),
-    Reason = #xmlel{name = <<"reason">>,
-                    children = [#xmlcdata{content = ReasonIn}]
-                   },
-    Item = #xmlel{name = <<"item">>,
-                  attrs = [{<<"nick">>, Nick},
-                           {<<"role">>, <<"none">>}],
-                  children = [ Reason ]
-                 },
-    IQ = iq(<<"set">>, SenderJID, RoomJID, [ query(?NS_MUC_ADMIN, [ Item ]) ]),
-    ejabberd_router:route(SenderJID, RoomJID, IQ),
-    {ok, "Kick message sent successfully"}.
+    case room_moderator(RoomJID) of
+        {ok, SenderJID} ->
+            kick_user_from_room_raw(RoomJID, SenderJID, Nick, ReasonIn);
+        {error, moderator_not_found} ->
+            ?MODERATOR_NOT_FOUND_RESULT;
+        {error, not_found} ->
+            ?ROOM_NOT_FOUND_RESULT
+    end.
+
+-spec kick_user_from_room(jid:jid(), jid:jid(), binary(), binary()) -> {ok, iolist()}.
+kick_user_from_room(RoomJID, UserJID, Nick, ReasonIn) ->
+    %% All the machinery which is already deeply embedded in the MUC
+    %% modules will perform the neccessary checking.
+    case try_add_role_resource(RoomJID, UserJID, moderator) of
+        {ok, #jid{lresource = <<>>}} ->
+            {moderator_not_found, "Resource with moderator role not found"};
+        {ok, SenderJID} ->
+            kick_user_from_room_raw(RoomJID, SenderJID, Nick, ReasonIn);
+        {error, not_found} ->
+            ?ROOM_NOT_FOUND_RESULT
+    end.
 
 -spec delete_room(jid:jid(), binary()) -> {ok | room_not_found, iolist()}.
 delete_room(RoomJID, Reason) ->
-    case mod_muc:room_jid_to_pid(RoomJID) of
-        {ok, Pid} ->
-            gen_fsm_compat:send_all_state_event(Pid, {destroy, Reason}),
+    case mod_muc_room:delete_room(RoomJID, Reason) of
+        ok ->
             ?ROOM_DELETED_SUCC_RESULT;
         {error, not_found} ->
             ?DELETE_NONEXISTENT_ROOM_RESULT
     end.
 
 -spec delete_room(jid:jid(), jid:jid(), binary()) ->
-    {ok | room_not_found, iolist()}.
+    {ok | not_allowed | room_not_found, iolist()}.
 delete_room(RoomJID, OwnerJID, Reason) ->
-    case mod_muc:room_jid_to_pid(RoomJID) of
-        {ok, Pid} ->
-            case gen_fsm_compat:sync_send_all_state_event(Pid, {is_room_owner, OwnerJID}) of
-                {ok, true} ->
-                    gen_fsm_compat:send_all_state_event(Pid, {destroy, Reason}),
-                    ?ROOM_DELETED_SUCC_RESULT;
-                {ok, false} ->
-                    {not_allowed, "Given user does not have permission to delete this room"}
-            end;
+    case mod_muc_room:is_room_owner(RoomJID, OwnerJID) of
+        {ok, true} ->
+            ok = mod_muc_room:delete_room(RoomJID, Reason),
+            ?ROOM_DELETED_SUCC_RESULT;
+        {ok, false} ->
+            {not_allowed, "Given user does not have permission to delete this room"};
         {error, not_found} ->
             ?DELETE_NONEXISTENT_ROOM_RESULT
     end.
@@ -240,16 +237,13 @@ get_room_users(RoomJID) ->
 
 -spec get_room_users(jid:jid(), jid:jid()) -> {ok, [user_map()]} | {room_not_found, iolist()}.
 get_room_users(RoomJID, UserJID) ->
-    case mod_muc:room_jid_to_pid(RoomJID) of
-        {ok, Pid} ->
-            case gen_fsm_compat:sync_send_all_state_event(Pid, {can_access_room, UserJID}) of
-                {ok, true} ->
-                    {ok, Users} = gen_fsm_compat:sync_send_all_state_event(Pid, get_room_users),
-                    WithJID = can_access_identity(Pid, UserJID),
-                    {ok, [to_user_map(U, WithJID) || U <- Users]};
-                {ok, false} ->
-                    ?USER_CANNOT_ACCESS_ROOM_RESULT
-            end;
+    case mod_muc_room:can_access_room(RoomJID, UserJID) of
+        {ok, true} ->
+            {ok, Users} = mod_muc_room:get_room_users(RoomJID),
+            {ok, WithJID} = mod_muc_room:can_access_identity(RoomJID, UserJID),
+            {ok, [to_user_map(U, WithJID) || U <- Users]};
+        {ok, false} ->
+            ?USER_CANNOT_ACCESS_ROOM_RESULT;
         {error, not_found} ->
             ?ROOM_NOT_FOUND_RESULT
     end.
@@ -337,31 +331,34 @@ set_affiliation(RoomJID, FromJID, UserJID, Affiliation) ->
 -spec set_role(jid:jid(), binary(), mod_muc:role()) ->
     {ok | room_not_found | not_allowed, iolist()}.
 set_role(RoomJID, Nick, Role) ->
-    case mod_muc:room_jid_to_pid(RoomJID) of
-        {ok, Pid} ->
-            ModJID = room_moderator(RoomJID),
-            set_role(Pid, ModJID, Nick, Role);
+    case room_moderator(RoomJID) of
+        {ok, ModJID} ->
+            case mod_muc_room:set_admin_items(RoomJID, ModJID, [role_item(Nick, Role)]) of
+                ok ->
+                    {ok, "Role set successfully"};
+                {error, Error} ->
+                    format_xml_error(Error, Role, <<"role">>)
+            end;
+        {error, moderator_not_found} ->
+            ?MODERATOR_NOT_FOUND_RESULT;
         {error, not_found} ->
             ?ROOM_NOT_FOUND_RESULT
     end.
 
--spec set_role(jid:jid() | pid(), jid:jid(), binary(), mod_muc:role()) ->
+-spec set_role(jid:jid(), jid:jid(), binary(), mod_muc:role()) ->
     {ok | room_not_found | not_allowed, iolist()}.
-set_role(#jid{} = RoomJID, ModJID, Nick, Role) ->
-    case mod_muc:room_jid_to_pid(RoomJID) of
-        {ok, Pid} ->
-            set_role(Pid, ModJID, Nick, Role);
+set_role(RoomJID, UserJID, Nick, Role) ->
+    RoleItem = role_item(Nick, Role),
+    case try_add_role_resource(RoomJID, UserJID, moderator) of
+        {ok, ModJID} ->
+            case mod_muc_room:set_admin_items(RoomJID, ModJID, [RoleItem]) of
+                ok ->
+                    {ok, "Role set successfully"};
+                {error, Error} ->
+                    format_xml_error(Error, Role, <<"role">>)
+            end;
         {error, not_found} ->
             ?ROOM_NOT_FOUND_RESULT
-    end;
-set_role(Pid, ModJID, Nick, Role) when is_pid(Pid) ->
-    RoleItem = role_item(Nick, Role),
-    ModJIDRes = try_add_role_res(Pid, ModJID, moderator),
-    case gen_fsm_compat:sync_send_event(Pid, {set_admin_items, ModJIDRes, [RoleItem]}) of
-        ok ->
-            {ok, "Role set successfully"};
-        {error, Error} ->
-            format_xml_error(Error, Role, <<"role">>)
     end.
 
 -spec enter_room(jid:jid(), jid:jid(), binary() | undefined) -> {ok, iolist()}.
@@ -415,18 +412,11 @@ format_xml_error(#xmlel{name = <<"error">>}, Aff, Op) ->
     Msg = io_lib:format("Given user does not have permission to set the ~p ~s", [Aff, Op]),
     {not_allowed, Msg}.
 
--spec can_access_identity(pid(), jid:jid()) -> boolean().
-can_access_identity(Pid, UserJID) ->
-    {ok, WithJID} = gen_fsm_compat:sync_send_all_state_event(Pid, {can_access_identity, UserJID}),
-    WithJID.
-
--spec modify_room_config_raw(pid(), room_conf_mod_fun()) -> {ok, mod_muc_room:config()}.
-modify_room_config_raw(Pid, Fun) ->
-    {ok, Config} = gen_fsm_compat:sync_send_all_state_event(Pid, get_config),
-    NewConfig = Fun(Config),
-    {ok, NewConfig2} =
-        gen_fsm_compat:sync_send_all_state_event(Pid, {change_config, NewConfig}),
-    {ok, NewConfig2}.
+-spec modify_room_config_raw(jid:jid(), room_conf_mod_fun()) -> mod_muc_room:config().
+modify_room_config_raw(RoomJID, Fun) ->
+    {ok, Config} = mod_muc_room:get_room_config(RoomJID),
+    {ok, NewConfig2} = mod_muc_room:change_room_config(RoomJID, Fun(Config)),
+    NewConfig2.
 
 -spec to_user_map(mod_muc_room:user(), boolean()) -> user_map().
 to_user_map(#user{role = Role, nick = Nick}, false = _WithJID) ->

src/mod_muc_room.erl

@@ -38,6 +38,10 @@
 %% API exports
 -export([get_room_users/1,
          get_room_affiliations/1,
+         set_admin_items/3,
+         get_room_config/1,
+         change_room_config/2,
+         delete_room/2,
          is_room_owner/2,
          can_access_room/2,
          can_access_identity/2]).
@@ -229,6 +233,49 @@ is_room_owner(RoomJID, UserJID) ->
             {error, Reason}
     end.
 
+-type error_xml() :: #xmlel{}.
+-type item_xml() :: #xmlel{}.
+
+-spec set_admin_items(jid:jid(), jid:jid(), [item_xml()]) ->
+    ok | {error, not_found | error_xml()}.
+set_admin_items(RoomJID, ModJID, Items) ->
+    case mod_muc:room_jid_to_pid(RoomJID) of
+        {ok, Pid} ->
+            gen_fsm_compat:sync_send_event(Pid, {set_admin_items, ModJID, Items});
+        {error, Reason} ->
+            {error, Reason}
+    end.
+
+-spec get_room_config(jid:jid()) ->
+    {ok, config()} | {error, not_found}.
+get_room_config(RoomJID) ->
+    case mod_muc:room_jid_to_pid(RoomJID) of
+        {ok, Pid} ->
+            gen_fsm_compat:sync_send_all_state_event(Pid, get_config);
+        {error, Reason} ->
+            {error, Reason}
+    end.
+
+-spec change_room_config(jid:jid(), config()) ->
+    {ok, config()} | {error, not_found}.
+change_room_config(RoomJID, NewConfig) ->
+    case mod_muc:room_jid_to_pid(RoomJID) of
+        {ok, Pid} ->
+            gen_fsm_compat:sync_send_all_state_event(Pid, {change_config, NewConfig});
+        {error, Reason} ->
+            {error, Reason}
+    end.
+
+-spec delete_room(jid:jid(), binary()) ->
+    ok | {error, not_found}.
+delete_room(RoomJID, ReasonIn) ->
+    case mod_muc:room_jid_to_pid(RoomJID) of
+        {ok, Pid} ->
+            gen_fsm_compat:send_all_state_event(Pid, {destroy, ReasonIn});
+        {error, Reason} ->
+            {error, Reason}
+    end.
+
 %% @doc Return true if UserJID can read room messages
 -spec can_access_room(RoomJID :: jid:jid(), UserJID :: jid:jid()) ->
             {ok, boolean()} | {error, not_found}.