From 5cf4e200ba4451ceef7b58be089b5aa0b4ff1734 Mon Sep 17 00:00:00 2001 From: Holger Weiss Date: Thu, 9 Nov 2017 00:21:40 +0100 Subject: [PATCH 1/5] mod_mam: Make sure a stanza ID is always added Let mod_mam store incoming messages from a new hook in ejabberd_sm. This makes sure all messages are tagged with a stanza ID, including those that are forked to multiple resources in ejabberd_sm. Closes #1344. --- src/ejabberd_sm.erl | 15 +++++++++++---- src/mod_mam.erl | 47 ++++++++++++--------------------------------- 2 files changed, 23 insertions(+), 39 deletions(-) diff --git a/src/ejabberd_sm.erl b/src/ejabberd_sm.erl index 96dbb4e83..3df1d88e0 100644 --- a/src/ejabberd_sm.erl +++ b/src/ejabberd_sm.erl @@ -137,10 +137,17 @@ route(To, Term) -> -spec route(stanza()) -> ok. route(Packet) -> - try do_route(Packet), ok - catch E:R -> - ?ERROR_MSG("failed to route packet:~n~s~nReason = ~p", - [xmpp:pp(Packet), {E, {R, erlang:get_stacktrace()}}]) + #jid{lserver = LServer} = xmpp:get_to(Packet), + case ejabberd_hooks:run_fold(sm_receive_packet, LServer, Packet, []) of + drop -> + ?DEBUG("hook dropped stanza:~n~s", [xmpp:pp(Packet)]); + Packet1 -> + try do_route(Packet1), ok + catch E:R -> + ?ERROR_MSG("failed to route packet:~n~s~nReason = ~p", + [xmpp:pp(Packet1), + {E, {R, erlang:get_stacktrace()}}]) + end end. -spec open_session(sid(), binary(), binary(), binary(), prio(), info()) -> ok. diff --git a/src/mod_mam.erl b/src/mod_mam.erl index 270cedf47..ddf3b4dab 100644 --- a/src/mod_mam.erl +++ b/src/mod_mam.erl @@ -32,12 +32,12 @@ %% API -export([start/2, stop/1, reload/3, depends/2]). --export([user_send_packet/1, user_send_packet_strip_tag/1, user_receive_packet/1, +-export([user_send_packet/1, user_send_packet_strip_tag/1, sm_receive_packet/1, process_iq_v0_2/1, process_iq_v0_3/1, disco_sm_features/5, remove_user/2, remove_room/3, mod_opt_type/1, muc_process_iq/2, muc_filter_message/3, message_is_archived/3, delete_old_messages/2, get_commands_spec/0, msg_to_el/4, get_room_config/4, set_room_option/3, - offline_message/1, export/1]). + export/1]). -include("xmpp.hrl"). -include("logger.hrl"). @@ -77,14 +77,12 @@ start(Host, Opts) -> Mod:init(Host, Opts), init_cache(Host, Opts), register_iq_handlers(Host, IQDisc), - ejabberd_hooks:add(user_receive_packet, Host, ?MODULE, - user_receive_packet, 88), + ejabberd_hooks:add(sm_receive_packet, Host, ?MODULE, + sm_receive_packet, 50), ejabberd_hooks:add(user_send_packet, Host, ?MODULE, user_send_packet, 88), ejabberd_hooks:add(user_send_packet, Host, ?MODULE, user_send_packet_strip_tag, 500), - ejabberd_hooks:add(offline_message_hook, Host, ?MODULE, - offline_message, 40), ejabberd_hooks:add(muc_filter_message, Host, ?MODULE, muc_filter_message, 50), ejabberd_hooks:add(muc_process_iq, Host, ?MODULE, @@ -140,14 +138,12 @@ cache_opts(Host, Opts) -> stop(Host) -> unregister_iq_handlers(Host), + ejabberd_hooks:delete(sm_receive_packet, Host, ?MODULE, + sm_receive_packet, 50), ejabberd_hooks:delete(user_send_packet, Host, ?MODULE, user_send_packet, 88), - ejabberd_hooks:delete(user_receive_packet, Host, ?MODULE, - user_receive_packet, 88), ejabberd_hooks:delete(user_send_packet, Host, ?MODULE, user_send_packet_strip_tag, 500), - ejabberd_hooks:delete(offline_message_hook, Host, ?MODULE, - offline_message, 40), ejabberd_hooks:delete(muc_filter_message, Host, ?MODULE, muc_filter_message, 50), ejabberd_hooks:delete(muc_process_iq, Host, ?MODULE, @@ -265,12 +261,11 @@ set_room_option(_Acc, {mam, Val}, _Lang) -> set_room_option(Acc, _Property, _Lang) -> Acc. --spec user_receive_packet({stanza(), c2s_state()}) -> {stanza(), c2s_state()}. -user_receive_packet({Pkt, #{jid := JID} = C2SState}) -> - Peer = xmpp:get_from(Pkt), +-spec sm_receive_packet(stanza()) -> stanza(). +sm_receive_packet(#message{from = Peer, to = JID} = Pkt) -> LUser = JID#jid.luser, LServer = JID#jid.lserver, - Pkt2 = case should_archive(Pkt, LServer) of + case should_archive(Pkt, LServer) of true -> Pkt1 = strip_my_archived_tag(Pkt, LServer), case store_msg(Pkt1, LUser, LServer, Peer, recv) of @@ -281,8 +276,9 @@ user_receive_packet({Pkt, #{jid := JID} = C2SState}) -> end; _ -> Pkt - end, - {Pkt2, C2SState}. + end; +sm_receive_packet(Acc) -> + Acc. -spec user_send_packet({stanza(), c2s_state()}) -> {stanza(), c2s_state()}. user_send_packet({Pkt, #{jid := JID} = C2SState}) -> @@ -304,23 +300,6 @@ user_send_packet({Pkt, #{jid := JID} = C2SState}) -> end, {Pkt2, C2SState}. --spec offline_message({any(), message()}) -> {any(), message()}. -offline_message({_Action, #message{from = Peer, to = To} = Pkt} = Acc) -> - LUser = To#jid.luser, - LServer = To#jid.lserver, - case should_archive(Pkt, LServer) of - true -> - Pkt1 = strip_my_archived_tag(Pkt, LServer), - case store_msg(Pkt1, LUser, LServer, Peer, recv) of - {ok, ID} -> - {archived, set_stanza_id(Pkt1, To, ID)}; - _ -> - Acc - end; - false -> - Acc - end. - -spec user_send_packet_strip_tag({stanza(), c2s_state()}) -> {stanza(), c2s_state()}. user_send_packet_strip_tag({Pkt, #{jid := JID} = C2SState}) -> @@ -547,8 +526,6 @@ process_iq(LServer, #iq{from = #jid{luser = LUser}, lang = Lang, should_archive(#message{type = error}, _LServer) -> false; -should_archive(#message{meta = #{sm_copy := true}}, _LServer) -> - false; should_archive(#message{meta = #{from_offline := true}}, _LServer) -> false; should_archive(#message{body = Body, subject = Subject, From 40d725e9c125517b8c1106fd9c107c9a77e7b488 Mon Sep 17 00:00:00 2001 From: Holger Weiss Date: Thu, 9 Nov 2017 00:48:19 +0100 Subject: [PATCH 2/5] mod_mam: Ignore non-message stanzas earlier Let mod_mam's hook callbacks ignore non-message stanzas using pattern matching. --- src/mod_mam.erl | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/src/mod_mam.erl b/src/mod_mam.erl index ddf3b4dab..0ef1d9708 100644 --- a/src/mod_mam.erl +++ b/src/mod_mam.erl @@ -280,9 +280,9 @@ sm_receive_packet(#message{from = Peer, to = JID} = Pkt) -> sm_receive_packet(Acc) -> Acc. --spec user_send_packet({stanza(), c2s_state()}) -> {stanza(), c2s_state()}. -user_send_packet({Pkt, #{jid := JID} = C2SState}) -> - Peer = xmpp:get_to(Pkt), +-spec user_send_packet({stanza(), c2s_state()}) + -> {stanza(), c2s_state()}. +user_send_packet({#message{to = Peer} = Pkt, #{jid := JID} = C2SState}) -> LUser = JID#jid.luser, LServer = JID#jid.lserver, Pkt2 = case should_archive(Pkt, LServer) of @@ -298,19 +298,23 @@ user_send_packet({Pkt, #{jid := JID} = C2SState}) -> false -> Pkt end, - {Pkt2, C2SState}. + {Pkt2, C2SState}; +user_send_packet(Acc) -> + Acc. -spec user_send_packet_strip_tag({stanza(), c2s_state()}) -> {stanza(), c2s_state()}. -user_send_packet_strip_tag({Pkt, #{jid := JID} = C2SState}) -> +user_send_packet_strip_tag({#message{} = Pkt, #{jid := JID} = C2SState}) -> LServer = JID#jid.lserver, - {strip_my_archived_tag(Pkt, LServer), C2SState}. + {strip_my_archived_tag(Pkt, LServer), C2SState}; +user_send_packet_strip_tag(Acc) -> + Acc. -spec muc_filter_message(message(), mod_muc_room:state(), binary()) -> message(). -muc_filter_message(Pkt, #state{config = Config, jid = RoomJID} = MUCState, +muc_filter_message(#message{from = From} = Pkt, + #state{config = Config, jid = RoomJID} = MUCState, FromNick) -> - From = xmpp:get_from(Pkt), if Config#config.mam -> LServer = RoomJID#jid.lserver, NewPkt = strip_my_archived_tag(Pkt, LServer), @@ -323,7 +327,9 @@ muc_filter_message(Pkt, #state{config = Config, jid = RoomJID} = MUCState, end; true -> Pkt - end. + end; +muc_filter_message(Acc, _MUCState, _FromNick) -> + Acc. set_stanza_id(Pkt, JID, ID) -> BareJID = jid:remove_resource(JID), @@ -695,7 +701,7 @@ may_enter_room(From, may_enter_room(From, MUCState) -> mod_muc_room:is_occupant_or_admin(From, MUCState). --spec store_msg(stanza(), +-spec store_msg(message(), binary(), binary(), jid(), send | recv) -> {ok, binary()} | pass. store_msg(Pkt, LUser, LServer, Peer, Dir) -> From 9c174e30b2ece19e804aa6cda8fb0600aaa3f164 Mon Sep 17 00:00:00 2001 From: Holger Weiss Date: Thu, 9 Nov 2017 01:00:15 +0100 Subject: [PATCH 3/5] mod_mam: Fix indentation --- src/mod_mam.erl | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/src/mod_mam.erl b/src/mod_mam.erl index 0ef1d9708..f46b17bc4 100644 --- a/src/mod_mam.erl +++ b/src/mod_mam.erl @@ -82,7 +82,7 @@ start(Host, Opts) -> ejabberd_hooks:add(user_send_packet, Host, ?MODULE, user_send_packet, 88), ejabberd_hooks:add(user_send_packet, Host, ?MODULE, - user_send_packet_strip_tag, 500), + user_send_packet_strip_tag, 500), ejabberd_hooks:add(muc_filter_message, Host, ?MODULE, muc_filter_message, 50), ejabberd_hooks:add(muc_process_iq, Host, ?MODULE, @@ -267,12 +267,12 @@ sm_receive_packet(#message{from = Peer, to = JID} = Pkt) -> LServer = JID#jid.lserver, case should_archive(Pkt, LServer) of true -> - Pkt1 = strip_my_archived_tag(Pkt, LServer), - case store_msg(Pkt1, LUser, LServer, Peer, recv) of + Pkt1 = strip_my_archived_tag(Pkt, LServer), + case store_msg(Pkt1, LUser, LServer, Peer, recv) of {ok, ID} -> - set_stanza_id(Pkt1, JID, ID); + set_stanza_id(Pkt1, JID, ID); _ -> - Pkt1 + Pkt1 end; _ -> Pkt @@ -286,24 +286,24 @@ user_send_packet({#message{to = Peer} = Pkt, #{jid := JID} = C2SState}) -> LUser = JID#jid.luser, LServer = JID#jid.lserver, Pkt2 = case should_archive(Pkt, LServer) of - true -> + true -> Pkt1 = strip_my_archived_tag(Pkt, LServer), case store_msg(xmpp:set_from_to(Pkt1, JID, Peer), - LUser, LServer, Peer, send) of - {ok, ID} -> + LUser, LServer, Peer, send) of + {ok, ID} -> set_stanza_id(Pkt1, JID, ID); - _ -> + _ -> Pkt1 - end; - false -> - Pkt + end; + false -> + Pkt end, {Pkt2, C2SState}; user_send_packet(Acc) -> Acc. --spec user_send_packet_strip_tag({stanza(), c2s_state()}) -> - {stanza(), c2s_state()}. +-spec user_send_packet_strip_tag({stanza(), c2s_state()}) + -> {stanza(), c2s_state()}. user_send_packet_strip_tag({#message{} = Pkt, #{jid := JID} = C2SState}) -> LServer = JID#jid.lserver, {strip_my_archived_tag(Pkt, LServer), C2SState}; From 28661d20bd1b543857cef096fe0444868f11dc08 Mon Sep 17 00:00:00 2001 From: Holger Weiss Date: Thu, 9 Nov 2017 01:10:24 +0100 Subject: [PATCH 4/5] mod_mam: Always strip stanza IDs XEP-0359 v0.5.0 says: "Stanza ID generating entities, which encounter a element where the 'by' attribute matches the 'by' attribute they would otherwise set, MUST delete that element even if they are not adding their own stanza ID." --- src/mod_mam.erl | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/mod_mam.erl b/src/mod_mam.erl index f46b17bc4..18c23b133 100644 --- a/src/mod_mam.erl +++ b/src/mod_mam.erl @@ -265,9 +265,9 @@ set_room_option(Acc, _Property, _Lang) -> sm_receive_packet(#message{from = Peer, to = JID} = Pkt) -> LUser = JID#jid.luser, LServer = JID#jid.lserver, - case should_archive(Pkt, LServer) of + Pkt1 = strip_my_archived_tag(Pkt, LServer), + case should_archive(Pkt1, LServer) of true -> - Pkt1 = strip_my_archived_tag(Pkt, LServer), case store_msg(Pkt1, LUser, LServer, Peer, recv) of {ok, ID} -> set_stanza_id(Pkt1, JID, ID); @@ -275,7 +275,7 @@ sm_receive_packet(#message{from = Peer, to = JID} = Pkt) -> Pkt1 end; _ -> - Pkt + Pkt1 end; sm_receive_packet(Acc) -> Acc. @@ -285,9 +285,9 @@ sm_receive_packet(Acc) -> user_send_packet({#message{to = Peer} = Pkt, #{jid := JID} = C2SState}) -> LUser = JID#jid.luser, LServer = JID#jid.lserver, - Pkt2 = case should_archive(Pkt, LServer) of + Pkt1 = strip_my_archived_tag(Pkt, LServer), + Pkt2 = case should_archive(Pkt1, LServer) of true -> - Pkt1 = strip_my_archived_tag(Pkt, LServer), case store_msg(xmpp:set_from_to(Pkt1, JID, Peer), LUser, LServer, Peer, send) of {ok, ID} -> @@ -296,7 +296,7 @@ user_send_packet({#message{to = Peer} = Pkt, #{jid := JID} = C2SState}) -> Pkt1 end; false -> - Pkt + Pkt1 end, {Pkt2, C2SState}; user_send_packet(Acc) -> @@ -315,9 +315,9 @@ user_send_packet_strip_tag(Acc) -> muc_filter_message(#message{from = From} = Pkt, #state{config = Config, jid = RoomJID} = MUCState, FromNick) -> + LServer = RoomJID#jid.lserver, + NewPkt = strip_my_archived_tag(Pkt, LServer), if Config#config.mam -> - LServer = RoomJID#jid.lserver, - NewPkt = strip_my_archived_tag(Pkt, LServer), StorePkt = strip_x_jid_tags(NewPkt), case store_muc(MUCState, StorePkt, RoomJID, From, FromNick) of {ok, ID} -> @@ -326,7 +326,7 @@ muc_filter_message(#message{from = From} = Pkt, NewPkt end; true -> - Pkt + NewPkt end; muc_filter_message(Acc, _MUCState, _FromNick) -> Acc. From d1df522fd95bd1ca9b1459b5248ee67f26b7f12d Mon Sep 17 00:00:00 2001 From: Holger Weiss Date: Fri, 10 Nov 2017 01:11:24 +0100 Subject: [PATCH 5/5] mod_push: Avoid notification duplicates (again) Now that mod_mam no longer uses the 'offline_message_hook', avoid duplicating notifications for messages written to both MAM and offline storage in another way. --- src/mod_mam.erl | 9 ++++++--- src/mod_push.erl | 20 +++++++++----------- 2 files changed, 15 insertions(+), 14 deletions(-) diff --git a/src/mod_mam.erl b/src/mod_mam.erl index 18c23b133..e24519fc6 100644 --- a/src/mod_mam.erl +++ b/src/mod_mam.erl @@ -270,7 +270,8 @@ sm_receive_packet(#message{from = Peer, to = JID} = Pkt) -> true -> case store_msg(Pkt1, LUser, LServer, Peer, recv) of {ok, ID} -> - set_stanza_id(Pkt1, JID, ID); + xmpp:put_meta(set_stanza_id(Pkt1, JID, ID), + mam_archived, true); _ -> Pkt1 end; @@ -291,7 +292,8 @@ user_send_packet({#message{to = Peer} = Pkt, #{jid := JID} = C2SState}) -> case store_msg(xmpp:set_from_to(Pkt1, JID, Peer), LUser, LServer, Peer, send) of {ok, ID} -> - set_stanza_id(Pkt1, JID, ID); + xmpp:put_meta(set_stanza_id(Pkt1, JID, ID), + mam_archived, true); _ -> Pkt1 end; @@ -321,7 +323,8 @@ muc_filter_message(#message{from = From} = Pkt, StorePkt = strip_x_jid_tags(NewPkt), case store_muc(MUCState, StorePkt, RoomJID, From, FromNick) of {ok, ID} -> - set_stanza_id(NewPkt, RoomJID, ID); + xmpp:put_meta(set_stanza_id(NewPkt, RoomJID, ID), + mam_archived, true); _ -> NewPkt end; diff --git a/src/mod_push.erl b/src/mod_push.erl index c947ecace..0af5df9e3 100644 --- a/src/mod_push.erl +++ b/src/mod_push.erl @@ -199,8 +199,8 @@ register_hooks(Host) -> c2s_stanza, 50), ejabberd_hooks:add(store_mam_message, Host, ?MODULE, mam_message, 50), - ejabberd_hooks:add(offline_message_hook, Host, ?MODULE, - offline_message, 30), + ejabberd_hooks:add(store_offline_message, Host, ?MODULE, + offline_message, 50), ejabberd_hooks:add(remove_user, Host, ?MODULE, remove_user, 50). @@ -218,8 +218,8 @@ unregister_hooks(Host) -> c2s_stanza, 50), ejabberd_hooks:delete(store_mam_message, Host, ?MODULE, mam_message, 50), - ejabberd_hooks:delete(offline_message_hook, Host, ?MODULE, - offline_message, 30), + ejabberd_hooks:delete(store_offline_message, Host, ?MODULE, + offline_message, 50), ejabberd_hooks:delete(remove_user, Host, ?MODULE, remove_user, 50). @@ -343,9 +343,6 @@ c2s_stanza(State, _Pkt, _SendResult) -> -spec mam_message(message() | drop, binary(), binary(), jid(), chat | groupchat, recv | send) -> message(). -mam_message(#message{meta = #{push_notified := true}} = Pkt, - _LUser, _LServer, _Peer, _Type, _Dir) -> - Pkt; mam_message(#message{} = Pkt, LUser, LServer, _Peer, chat, _Dir) -> case lookup_sessions(LUser, LServer) of {ok, [_|_] = Clients} -> @@ -363,9 +360,10 @@ mam_message(#message{} = Pkt, LUser, LServer, _Peer, chat, _Dir) -> mam_message(Pkt, _LUser, _LServer, _Peer, _Type, _Dir) -> Pkt. --spec offline_message({any(), message()}) -> {any(), message()}. -offline_message({Action, #message{to = #jid{luser = LUser, - lserver = LServer}} = Pkt}) -> +-spec offline_message(message()) -> message(). +offline_message(#message{meta = #{mam_archived := true}} = Pkt) -> + Pkt; % Push notification was triggered via MAM. +offline_message(#message{to = #jid{luser = LUser, lserver = LServer}} = Pkt) -> case lookup_sessions(LUser, LServer) of {ok, [_|_] = Clients} -> ?DEBUG("Notifying ~s@~s of offline message", [LUser, LServer]), @@ -373,7 +371,7 @@ offline_message({Action, #message{to = #jid{luser = LUser, _ -> ok end, - {Action, xmpp:put_meta(Pkt, push_notified, true)}. + Pkt. -spec c2s_session_pending(c2s_state()) -> c2s_state(). c2s_session_pending(#{push_enabled := true, mgmt_queue := Queue} = State) ->