From 4e7bf9207e552ccaf9d016cfc9c9429c2a14e675 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20Chmielowski?= Date: Mon, 6 May 2019 12:21:49 +0200 Subject: [PATCH 1/8] Do not declare mod_muc as dependency of mod_mam to prevent loop in deps --- src/mod_mam.erl | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/mod_mam.erl b/src/mod_mam.erl index ba00d74e5..0f06d9823 100644 --- a/src/mod_mam.erl +++ b/src/mod_mam.erl @@ -251,10 +251,7 @@ reload(Host, NewOpts, OldOpts) -> end. depends(_Host, Opts) -> - case proplists:get_bool(user_mucsub_from_muc_archive, Opts) of - true -> [{mod_muc, hard}, {mod_muc_admin, hard}]; - false -> [] - end. + []. -spec register_iq_handlers(binary()) -> ok. register_iq_handlers(Host) -> From 3d434cfcef3356319f3fdfe94cc34f848ade51c4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20Chmielowski?= Date: Mon, 6 May 2019 19:15:48 +0200 Subject: [PATCH 2/8] Handle get_subscribed_rooms call from mod_muc_room pid Previously sometimes we tried to post message to all online rooms, and if that was called from muc room pid, we were not able to process that message for that room and send response, and this did lead to timeout. --- src/mod_muc.erl | 14 +++++++++++++- src/mod_muc_room.erl | 2 ++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/src/mod_muc.erl b/src/mod_muc.erl index 76fce1f8e..edc570eed 100644 --- a/src/mod_muc.erl +++ b/src/mod_muc.erl @@ -758,6 +758,10 @@ get_subscribed_rooms(Host, User) -> ServerHost = ejabberd_router:host_of_route(Host), get_subscribed_rooms(ServerHost, Host, User). +-record(subscriber, {jid :: jid(), + nick = <<>> :: binary(), + nodes = [] :: [binary()]}). + -spec get_subscribed_rooms(binary(), binary(), jid()) -> {ok, [{jid(), [binary()]}]} | {error, any()}. get_subscribed_rooms(ServerHost, Host, From) -> @@ -768,7 +772,15 @@ get_subscribed_rooms(ServerHost, Host, From) -> false -> Rooms = get_online_rooms(ServerHost, Host), {ok, lists:flatmap( - fun({Name, _, Pid}) -> + fun({Name, _, Pid}) when Pid == self() -> + USR = jid:split(BareFrom), + case erlang:get(muc_subscribers) of + #{USR := #subscriber{nodes = Nodes}} -> + [{jid:make(Name, Host), Nodes}]; + _ -> + [] + end; + ({Name, _, Pid}) -> case p1_fsm:sync_send_all_state_event( Pid, {is_subscribed, BareFrom}) of {true, Nodes} -> diff --git a/src/mod_muc_room.erl b/src/mod_muc_room.erl index f972a5feb..94c430197 100644 --- a/src/mod_muc_room.erl +++ b/src/mod_muc_room.erl @@ -538,6 +538,7 @@ handle_sync_event({change_config, Config}, _From, {reply, {ok, NSD#state.config}, StateName, NSD}; handle_sync_event({change_state, NewStateData}, _From, StateName, _StateData) -> + erlang:put(muc_subscribers, NewStateData#state.subscribers), {reply, {ok, NewStateData}, StateName, NewStateData}; handle_sync_event({process_item_change, Item, UJID}, _From, StateName, StateData) -> case process_item_change(Item, StateData, UJID) of @@ -4373,6 +4374,7 @@ store_room(StateData) -> store_room(StateData, []). store_room(StateData, ChangesHints) -> % Let store persistent rooms or on those backends that have get_subscribed_rooms + erlang:put(muc_subscribers, StateData#state.subscribers), ShouldStore = case (StateData#state.config)#config.persistent of true -> true; From 83b790c7c913e5af4ada5f01f7550a633ecf2d1f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20Chmielowski?= Date: Mon, 6 May 2019 19:17:30 +0200 Subject: [PATCH 3/8] Do not store mucsub wrapped messages with no-store hint in offline storage We already don't store those messages in mam and we don't store messages that aren't wrapped with that hint in offline, so it make sense to extend it also to mucsub messages. --- src/mod_offline.erl | 55 +++++++++++++++++++++++++-------------------- 1 file changed, 31 insertions(+), 24 deletions(-) diff --git a/src/mod_offline.erl b/src/mod_offline.erl index 0c70ae065..3e240dbfb 100644 --- a/src/mod_offline.erl +++ b/src/mod_offline.erl @@ -405,31 +405,38 @@ need_to_store(_LServer, #message{type = error}) -> false; need_to_store(LServer, #message{type = Type} = Packet) -> case xmpp:has_subtag(Packet, #offline{}) of false -> - case check_store_hint(Packet) of - store -> - true; - no_store -> - false; - none -> - Store = case Type of - groupchat -> - gen_mod:get_module_opt( - LServer, ?MODULE, store_groupchat); - headline -> - false; - _ -> - true - end, - case {Store, gen_mod:get_module_opt( - LServer, ?MODULE, store_empty_body)} of - {false, _} -> - false; - {_, true} -> + case misc:unwrap_mucsub_message(Packet) of + #message{type = groupchat} = Msg -> + need_to_store(LServer, Msg#message{type = chat}); + #message{} = Msg -> + need_to_store(LServer, Msg); + _ -> + case check_store_hint(Packet) of + store -> true; - {_, false} -> - Packet#message.body /= []; - {_, unless_chat_state} -> - not misc:is_standalone_chat_state(Packet) + no_store -> + false; + none -> + Store = case Type of + groupchat -> + gen_mod:get_module_opt( + LServer, ?MODULE, store_groupchat); + headline -> + false; + _ -> + true + end, + case {Store, gen_mod:get_module_opt( + LServer, ?MODULE, store_empty_body)} of + {false, _} -> + false; + {_, true} -> + true; + {_, false} -> + Packet#message.body /= []; + {_, unless_chat_state} -> + not misc:is_standalone_chat_state(Packet) + end end end; true -> From 5b863c25ae2563af77b2070f01f910e80c5b01bf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20Chmielowski?= Date: Mon, 6 May 2019 19:19:10 +0200 Subject: [PATCH 4/8] Test offline:use_mam_for_storage, mam:user_mucsub_from_muc_archive used together --- test/offline_tests.erl | 81 ++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 79 insertions(+), 2 deletions(-) diff --git a/test/offline_tests.erl b/test/offline_tests.erl index 179a2415c..97d9ee58b 100644 --- a/test/offline_tests.erl +++ b/test/offline_tests.erl @@ -27,7 +27,8 @@ -compile(export_all). -import(suite, [send/2, disconnect/1, my_jid/1, send_recv/2, recv_message/1, get_features/1, recv/1, get_event/1, server_jid/1, - wait_for_master/1, wait_for_slave/1]). + wait_for_master/1, wait_for_slave/1, + connect/1, open_session/1, bind/1, auth/1]). -include("suite.hrl"). %%%=================================================================== @@ -147,7 +148,9 @@ master_slave_cases(DB) -> master_slave_test(send_all)] ++ case DB of riak -> []; - _ -> [master_slave_test(from_mam)] + _ -> [ + master_slave_test(from_mam), + master_slave_test(mucsub_mam)] end }. @@ -194,6 +197,80 @@ from_mam_slave(Config) -> C4 = lists:keydelete(mam_enabled, 1, C3), mam_tests:clean(C4). +mucsub_mam_master(Config) -> + Room = suite:muc_room_jid(Config), + Peer = ?config(peer, Config), + wait_for_slave(Config), + ct:comment("Joining muc room"), + ok = muc_tests:join_new(Config), + + ct:comment("Enabling mam in room"), + CfgOpts = muc_tests:get_config(Config), + %% Find the MAM field in the config + ?match(true, proplists:is_defined(mam, CfgOpts)), + ?match(true, proplists:is_defined(allow_subscription, CfgOpts)), + %% Enable MAM + [104] = muc_tests:set_config(Config, [{mam, true}, {allow_subscription, true}]), + + ct:comment("Subscribing peer to room"), + ?send_recv(#iq{to = Room, type = set, sub_els = [ + #muc_subscribe{jid = Peer, nick = <<"peer">>, + events = [?NS_MUCSUB_NODES_MESSAGES]} + ]}, #iq{type = result}), + + ?match(#message{type = groupchat}, + send_recv(Config, #message{type = groupchat, to = Room, body = xmpp:mk_text(<<"1">>)})), + ?match(#message{type = groupchat}, + send_recv(Config, #message{type = groupchat, to = Room, body = xmpp:mk_text(<<"2">>), + sub_els = [#hint{type = 'no-store'}]})), + ?match(#message{type = groupchat}, + send_recv(Config, #message{type = groupchat, to = Room, body = xmpp:mk_text(<<"3">>)})), + + ct:comment("Cleaning up"), + suite:put_event(Config, ready), + ready = get_event(Config), + muc_tests:leave(Config), + mam_tests:clean(clean(disconnect(Config))). + +mucsub_mam_slave(Config) -> + Server = ?config(server, Config), + gen_mod:update_module_opts(Server, mod_offline, [{use_mam_for_storage, true}]), + gen_mod:update_module_opts(Server, mod_mam, [{user_mucsub_from_muc_archive, true}]), + + Room = suite:muc_room_jid(Config), + MyJID = my_jid(Config), + MyJIDBare = jid:remove_resource(MyJID), + ok = mam_tests:set_default(Config, always), + #presence{} = send_recv(Config, #presence{}), + send(Config, #presence{type = unavailable}), + + wait_for_master(Config), + ready = get_event(Config), + ct:sleep(100), + + ct:comment("Receiving offline messages"), + + ?match(#presence{}, suite:send_recv(Config, #presence{})), + + lists:foreach( + fun(N) -> + Body = xmpp:mk_text(integer_to_binary(N)), + Msg = ?match(#message{from = Room, type = normal} = Msg, recv_message(Config), Msg), + PS = ?match(#ps_event{items = #ps_items{node = ?NS_MUCSUB_NODES_MESSAGES, items = [ + #ps_item{} = PS + ]}}, xmpp:get_subtag(Msg, #ps_event{}), PS), + ?match(#message{type = groupchat, body = Body}, xmpp:get_subtag(PS, #message{})) + end, [1, 3]), + + % Unsubscribe yourself + ?send_recv(#iq{to = Room, type = set, sub_els = [ + #muc_unsubscribe{} + ]}, #iq{type = result}), + suite:put_event(Config, ready), + mam_tests:clean(clean(disconnect(Config))), + gen_mod:update_module_opts(Server, mod_offline, [{use_mam_for_storage, false}]), + gen_mod:update_module_opts(Server, mod_mam, [{user_mucsub_from_muc_archive, false}]). + send_all_master(Config) -> wait_for_slave(Config), Peer = ?config(peer, Config), From 8207ea18bf9c6d4d57f0edee14038cd1ced97c67 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20Chmielowski?= Date: Mon, 6 May 2019 20:03:10 +0200 Subject: [PATCH 5/8] Remove compiler warnings --- src/mod_mam.erl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/mod_mam.erl b/src/mod_mam.erl index 0f06d9823..f050128cd 100644 --- a/src/mod_mam.erl +++ b/src/mod_mam.erl @@ -250,7 +250,7 @@ reload(Host, NewOpts, OldOpts) -> ok end. -depends(_Host, Opts) -> +depends(_Host, _Opts) -> []. -spec register_iq_handlers(binary()) -> ok. From 7d23cd28991c7e0a0aa3cd49047f07fa2ac1c566 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20Chmielowski?= Date: Tue, 7 May 2019 09:58:14 +0200 Subject: [PATCH 6/8] When applying limit of max msgs in spool check only spool size --- src/mod_offline.erl | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/mod_offline.erl b/src/mod_offline.erl index 3e240dbfb..63a0d6763 100644 --- a/src/mod_offline.erl +++ b/src/mod_offline.erl @@ -195,7 +195,7 @@ store_offline_msg(#offline_msg{us = {User, Server}, packet = Pkt} = Msg) -> infinity -> Mod:store_message(Msg); Limit -> - Num = count_offline_messages(User, Server), + Num = count_messages_in_db(User, Server), if Num < Limit -> Mod:store_message(Msg); true -> @@ -1053,10 +1053,14 @@ count_offline_messages(User, Server) -> Res = read_db_messages(LUser, LServer), count_mam_messages(LUser, LServer, Res); _ -> - Mod = gen_mod:db_mod(LServer, ?MODULE), - Mod:count_messages(LUser, LServer) + count_messages_in_db(LUser, LServer) end. +-spec count_messages_in_db(binary(), binary()) -> non_neg_integer(). +count_messages_in_db(LUser, LServer) -> + Mod = gen_mod:db_mod(LServer, ?MODULE), + Mod:count_messages(LUser, LServer). + -spec add_delay_info(message(), binary(), undefined | erlang:timestamp()) -> message(). add_delay_info(Packet, LServer, TS) -> From 4dc85497385e33f163aedccb38c5145115521bda Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20Chmielowski?= Date: Tue, 7 May 2019 11:02:53 +0200 Subject: [PATCH 7/8] Make anonymous auth don't {de}register user when there are other resources This should fix issue reported in #2878 --- src/ejabberd_auth_anonymous.erl | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/src/ejabberd_auth_anonymous.erl b/src/ejabberd_auth_anonymous.erl index 21aecc341..767d99bf2 100644 --- a/src/ejabberd_auth_anonymous.erl +++ b/src/ejabberd_auth_anonymous.erl @@ -114,10 +114,16 @@ anonymous_user_exist(User, Server) -> %% Register connection -spec register_connection(ejabberd_sm:sid(), jid(), ejabberd_sm:info()) -> ok. register_connection(_SID, - #jid{luser = LUser, lserver = LServer}, Info) -> + #jid{luser = LUser, lserver = LServer, lresource = LResource}, Info) -> case proplists:get_value(auth_module, Info) of ?MODULE -> - ejabberd_hooks:run(register_user, LServer, [LUser, LServer]); + % Register user only if we are first resource + case ejabberd_sm:get_user_resources(LUser, LServer) of + [LResource] -> + ejabberd_hooks:run(register_user, LServer, [LUser, LServer]); + _ -> + ok + end; _ -> ok end. @@ -128,7 +134,13 @@ unregister_connection(_SID, #jid{luser = LUser, lserver = LServer}, Info) -> case proplists:get_value(auth_module, Info) of ?MODULE -> - ejabberd_hooks:run(remove_user, LServer, [LUser, LServer]); + % Remove user data only if there is no more resources around + case ejabberd_sm:get_user_resources(LUser, LServer) of + [] -> + ejabberd_hooks:run(remove_user, LServer, [LUser, LServer]); + _ -> + ok + end; _ -> ok end. From a6f7d7ce23e9e510bca678aa95f2160ec83eb1d6 Mon Sep 17 00:00:00 2001 From: Christophe Romain Date: Tue, 7 May 2019 11:46:04 +0200 Subject: [PATCH 8/8] Raise api hook right before performing the call --- src/rest.erl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/rest.erl b/src/rest.erl index 05d1b9d4a..9c1b28068 100644 --- a/src/rest.erl +++ b/src/rest.erl @@ -100,6 +100,7 @@ request(Server, Method, Path, Params, Mime, Data) -> {URI, Hdrs} end, Begin = os:timestamp(), + ejabberd_hooks:run(backend_api_call, Server, [Server, Method, Path]), Result = try httpc:request(Method, Req, HttpOpts, [{body_format, binary}]) of {ok, {{_, Code, _}, RetHdrs, Body}} -> try decode_json(Body) of @@ -118,7 +119,6 @@ request(Server, Method, Path, Params, Mime, Data) -> exit:Reason -> {error, {http_error, {error, Reason}}} end, - ejabberd_hooks:run(backend_api_call, Server, [Server, Method, Path]), case Result of {error, {http_error, {error, timeout}}} -> ejabberd_hooks:run(backend_api_timeout, Server,