From 515bb8f0af6e977d17d66fb2ab18c3eaaaebcea9 Mon Sep 17 00:00:00 2001 From: Nelson Vides Date: Mon, 13 Dec 2021 10:02:08 +0100 Subject: [PATCH 1/6] Clean up order in inbox_SUITE --- big_tests/tests/inbox_SUITE.erl | 74 ++------------------------------- 1 file changed, 3 insertions(+), 71 deletions(-) diff --git a/big_tests/tests/inbox_SUITE.erl b/big_tests/tests/inbox_SUITE.erl index 1b4a258cf79..ea3826140d1 100644 --- a/big_tests/tests/inbox_SUITE.erl +++ b/big_tests/tests/inbox_SUITE.erl @@ -1,81 +1,13 @@ -module(inbox_SUITE). +-compile([export_all, nowarn_export_all]). --include_lib("escalus/include/escalus.hrl"). --include_lib("escalus/include/escalus_xmlns.hrl"). -include_lib("common_test/include/ct.hrl"). +-include_lib("eunit/include/eunit.hrl"). +-include_lib("escalus/include/escalus_xmlns.hrl"). -include_lib("exml/include/exml.hrl"). -include_lib("inbox.hrl"). --include_lib("eunit/include/eunit.hrl"). --export([all/0, - groups/0, - suite/0, - init_per_suite/1, - end_per_suite/1, - init_per_group/2, - end_per_group/2, - init_per_testcase/2, - end_per_testcase/2]). %% tests --export([disco_service/1, - returns_valid_form/1, - returns_error_when_first_bad_form_field_encountered/1, - returns_error_when_bad_form_field_start_sent/1, - returns_error_when_bad_form_field_end_sent/1, - returns_error_when_bad_form_field_order_sent/1, - returns_error_when_bad_form_field_hidden_read_sent/1, - returns_error_when_bad_reset_field_jid/1, - returns_error_when_no_reset_field_jid/1, - returns_error_when_unknown_field_sent/1 - ]). --export([msg_sent_stored_in_inbox/1, - msg_with_no_store_is_not_stored_in_inbox/1, - msg_with_store_hint_is_always_stored/1, - carbons_are_not_stored/1, - user_has_empty_inbox/1, - user_has_two_unread_messages/1, - other_resources_do_not_interfere/1, - reset_unread_counter_with_reset_chat_marker/1, - reset_unread_counter_with_reset_stanza/1, - try_to_reset_unread_counter_with_bad_marker/1, - non_reset_marker_should_not_affect_inbox/1, - user_has_two_conversations/1, - msg_sent_to_offline_user/1, - msg_sent_to_not_existing_user/1, - user_has_only_unread_messages_or_only_read/1, - reset_unread_counter_and_show_only_unread/1, - check_total_unread_count_and_active_conv_count/1, - check_total_unread_count_when_there_are_no_active_conversations/1, - total_unread_count_and_active_convs_are_zero_at_no_activity/1 - ]). --export([simple_groupchat_stored_in_all_inbox/1, - advanced_groupchat_stored_in_all_inbox/1, - groupchat_markers_one_reset/1, - non_reset_marker_should_not_affect_muclight_inbox/1, - groupchat_reset_stanza_resets_inbox/1, - create_groupchat/1, - create_groupchat_no_affiliation_stored/1, - leave_and_remove_conversation/1, - leave_and_store_conversation/1, - groupchat_markers_one_reset_room_created/1, - groupchat_markers_all_reset_room_created/1, - system_message_is_correctly_avoided/1, - no_aff_stored_and_remove_on_kicked/1, - no_stored_and_remain_after_kicked/1, - simple_groupchat_stored_in_all_inbox_muc/1, - simple_groupchat_stored_in_offline_users_inbox_muc/1, - unread_count_is_the_same_after_going_online_again/1, - unread_count_is_reset_after_sending_chatmarker/1, - non_reset_marker_should_not_affect_muc_inbox/1, - unread_count_is_reset_after_sending_reset_stanza/1, - private_messages_are_handled_as_one2one/1 - ]). --export([timestamp_is_updated_on_new_message/1, - order_by_timestamp_ascending/1, - get_by_timestamp_range/1, - get_with_start_timestamp/1, - get_with_end_timestamp/1]). - -import(muc_light_helper, [room_bin_jid/1]). -import(inbox_helper, [ inbox_modules/0, From b9b548733386467e3a306cc6c64c0e59e692111d Mon Sep 17 00:00:00 2001 From: Nelson Vides Date: Mon, 13 Dec 2021 10:02:09 +0100 Subject: [PATCH 2/6] Add regression test for inbox bug --- big_tests/tests/inbox_SUITE.erl | 52 ++++++++++++++++++++++++++++++++- 1 file changed, 51 insertions(+), 1 deletion(-) diff --git a/big_tests/tests/inbox_SUITE.erl b/big_tests/tests/inbox_SUITE.erl index ea3826140d1..6950fce4e50 100644 --- a/big_tests/tests/inbox_SUITE.erl +++ b/big_tests/tests/inbox_SUITE.erl @@ -5,6 +5,7 @@ -include_lib("eunit/include/eunit.hrl"). -include_lib("escalus/include/escalus_xmlns.hrl"). -include_lib("exml/include/exml.hrl"). +-include_lib("jid/include/jid.hrl"). -include_lib("inbox.hrl"). %% tests @@ -90,7 +91,8 @@ groups() -> create_groupchat, leave_and_remove_conversation, groupchat_markers_one_reset_room_created, - groupchat_markers_all_reset_room_created + groupchat_markers_all_reset_room_created, + inbox_does_not_trigger_does_user_exist ]}, {muclight_config, [sequence], [ @@ -140,6 +142,7 @@ init_per_suite(Config0) -> {groupchat, [muclight]}, {markers, [displayed]}]}]), InboxOptions = inbox_opts(), + mongoose_helper:inject_module(?MODULE), escalus:init_per_suite([{inbox_opts, InboxOptions} | Config1]). end_per_suite(Config) -> @@ -963,6 +966,20 @@ groupchat_markers_all_reset_room_created(Config) -> inbox_helper:foreach_check_inbox([Bob, Kate, Alice], 0, AliceRoomJid, Msg) end). +inbox_does_not_trigger_does_user_exist(Config) -> + escalus:fresh_story(Config, [{alice, 1}, {bob, 1}, {kate, 1}], fun(Alice, Bob, Kate) -> + Msg = <<"Mark me!">>, + RoomName = inbox_helper:create_room(Alice, [Bob, Kate]), + RoomJid = room_bin_jid(RoomName), + HookHandlerExtra = start_hook_listener(), + Stanza = escalus_stanza:groupchat_to(RoomJid, Msg), + %% Alice sends message to a room + escalus:send(Alice, Stanza), + [escalus:wait_for_stanza(User) || User <- [Alice, Bob, Kate]], + stop_hook_listener(HookHandlerExtra), + verify_hook_listener(RoomName) + end). + system_message_is_correctly_avoided(Config) -> escalus:story(Config, [{alice, 1}, {alice_bis, 1}, {bob, 1}], fun(Alice, AliceBis, Bob) -> %% Variables @@ -1303,3 +1320,36 @@ get_with_end_timestamp(Config) -> %% TODO: Improve this test to store 3+ conversations in Alice's inbox check_inbox(Alice, [ConvWithBob], #{ 'end' => TimeAfterBob }, #{}) end). + + + +start_hook_listener() -> + TestCasePid = self(), + distributed_helper:rpc(distributed_helper:mim(), ?MODULE, rpc_start_hook_handler, [TestCasePid, domain_helper:host_type()]). + +stop_hook_listener(HookExtra) -> + distributed_helper:rpc(distributed_helper:mim(), ?MODULE, rpc_stop_hook_handler, [HookExtra, domain_helper:host_type()]). + +rpc_start_hook_handler(TestCasePid, HostType) -> + Extra = #{test_case_pid => TestCasePid}, + gen_hook:add_handler(does_user_exist, HostType, fun ?MODULE:hook_handler_fn/3, Extra, 1), + Extra. + +rpc_stop_hook_handler(HookExtra, HostType) -> + gen_hook:delete_handler(does_user_exist, HostType, fun ?MODULE:hook_handler_fn/3, HookExtra, 1). + +hook_handler_fn(Acc, + #{args := [_HostType, User, _Stored]} = _Params, + #{test_case_pid := Pid} = _Extra) -> + Pid ! {input, User#jid.user}, + {ok, Acc}. + +verify_hook_listener(RoomName) -> + receive + {input, RoomName} -> + ct:fail("does_user_exist was called with a room jid"); + {input, _} -> + verify_hook_listener(RoomName) + after 100 -> + ct:pal("OK") + end. From f6d62574d8a525172cc49ed5ffebafb1b01226ad Mon Sep 17 00:00:00 2001 From: Nelson Vides Date: Mon, 13 Dec 2021 10:02:09 +0100 Subject: [PATCH 3/6] Simplify the order of matches in the filter --- src/mam/mod_mam_utils.erl | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/mam/mod_mam_utils.erl b/src/mam/mod_mam_utils.erl index 92c92d2fc68..88eb7fb3421 100644 --- a/src/mam/mod_mam_utils.erl +++ b/src/mam/mod_mam_utils.erl @@ -133,7 +133,7 @@ -define(MAYBE_BIN(X), (is_binary(X) orelse (X) =:= undefined)). --export_type([retraction_id/0, retraction_info/0]). +-export_type([direction/0, retraction_id/0, retraction_info/0]). %% Constants rsm_ns_binary() -> <<"http://jabber.org/protocol/rsm">>. @@ -149,6 +149,7 @@ rsm_ns_binary() -> <<"http://jabber.org/protocol/rsm">>. -type archive_behaviour() :: mod_mam:archive_behaviour(). -type archive_behaviour_bin() :: binary(). % `<<"roster">> | <<"always">> | <<"never">>'. +-type direction() :: incoming | outgoing. -type retraction_id() :: {origin_id | stanza_id, binary()}. -type retraction_info() :: #{retract_on := origin_id | stanza_id, packet := exml:element(), @@ -338,8 +339,7 @@ get_one_of_path(_Elem, [], Def) -> %% It also must include a body or chat marker, as long as it doesn't include %% "result", "delay" or "no-store" elements. %% @end --spec is_archivable_message(Mod :: module(), Dir :: incoming | outgoing, - Packet :: exml:element(), boolean()) -> boolean(). +-spec is_archivable_message(module(), direction(), exml:element(), boolean()) -> boolean(). is_archivable_message(Mod, Dir, Packet=#xmlel{name = <<"message">>}, ArchiveChatMarkers) -> Type = exml_query:attr(Packet, <<"type">>, <<"normal">>), is_valid_message_type(Mod, Dir, Type) andalso @@ -347,11 +347,10 @@ is_archivable_message(Mod, Dir, Packet=#xmlel{name = <<"message">>}, ArchiveChat is_archivable_message(_, _, _, _) -> false. -is_valid_message_type(mod_inbox, _, <<"groupchat">>) -> true; is_valid_message_type(_, _, <<"normal">>) -> true; is_valid_message_type(_, _, <<"chat">>) -> true; +is_valid_message_type(mod_inbox, _, <<"groupchat">>) -> true; is_valid_message_type(_, incoming, <<"groupchat">>) -> true; -is_valid_message_type(_, _, <<"error">>) -> false; is_valid_message_type(_, _, _) -> false. is_valid_message(_Mod, _Dir, Packet, ArchiveChatMarkers) -> From 01142102abd4dabe73bdd98cd57a094c32ca3ac6 Mon Sep 17 00:00:00 2001 From: Nelson Vides Date: Mon, 13 Dec 2021 10:02:10 +0100 Subject: [PATCH 4/6] Move code around and simplify types The part about the inbox owner existence is actually a condition of inbox taking the message or not, so that could be moved into a single filtering function. Also simplify specs by reusing types. --- src/inbox/mod_inbox.erl | 44 +++++++++++++++++++++-------------------- 1 file changed, 23 insertions(+), 21 deletions(-) diff --git a/src/inbox/mod_inbox.erl b/src/inbox/mod_inbox.erl index 69cfa1340c2..93e74ec728a 100644 --- a/src/inbox/mod_inbox.erl +++ b/src/inbox/mod_inbox.erl @@ -41,6 +41,7 @@ -export([config_metrics/1]). +-type message_type() :: one2one | groupchat. -type entry_key() :: {LUser :: jid:luser(), LServer :: jid:lserver(), ToBareJid :: jid:literal_jid()}. @@ -226,9 +227,9 @@ disco_local_features(Acc) -> From :: jid:jid(), To :: jid:jid(), Msg :: exml:element(), - Dir :: outgoing | incoming) -> mongoose_acc:t(). + Dir :: mod_mam_utils:direction()) -> mongoose_acc:t(). maybe_process_message(Acc, From, To, Msg, Dir) -> - case should_be_stored_in_inbox(Msg, Dir) andalso inbox_owner_exists(Acc, From, To, Dir) of + case should_be_stored_in_inbox(Acc, From, To, Msg, Dir) of true -> do_maybe_process_message(Acc, From, To, Msg, Dir); false -> @@ -254,20 +255,9 @@ do_maybe_process_message(Acc, From, To, Msg, Dir) -> Acc end. --spec inbox_owner_exists(Acc :: mongoose_acc:t(), - From :: jid:jid(), - To :: jid:jid(), - Dir :: outgoing | incoming) -> boolean(). -inbox_owner_exists(Acc, From, _To, outgoing) -> - HostType = mongoose_acc:host_type(Acc), - ejabberd_auth:does_user_exist(HostType, From, stored); -inbox_owner_exists(Acc, _From, To, incoming) -> - HostType = mongoose_acc:host_type(Acc), - ejabberd_auth:does_user_exist(HostType, To, stored). - -spec maybe_process_acceptable_message( mongooseim:host_type(), jid:jid(), jid:jid(), exml:element(), - mongoose_acc:t(), outgoing | incoming, one2one | groupchat) -> + mongoose_acc:t(), mod_mam_utils:direction(), message_type()) -> count_res(). maybe_process_acceptable_message(HostType, From, To, Msg, Acc, Dir, one2one) -> process_message(HostType, From, To, Msg, Acc, Dir, one2one); @@ -282,8 +272,8 @@ maybe_process_acceptable_message(HostType, From, To, Msg, Acc, Dir, groupchat) - To :: jid:jid(), Message :: exml:element(), Acc :: mongoose_acc:t(), - Dir :: outgoing | incoming, - Type :: one2one | groupchat) -> count_res(). + Dir :: mod_mam_utils:direction(), + Type :: message_type()) -> count_res(). process_message(HostType, From, To, Message, Acc, outgoing, one2one) -> mod_inbox_one2one:handle_outgoing_message(HostType, From, To, Message, Acc); process_message(HostType, From, To, Message, Acc, incoming, one2one) -> @@ -573,7 +563,7 @@ muclight_enabled(HostType) -> Groupchats = get_groupchat_types(HostType), lists:member(muclight, Groupchats). --spec get_message_type(mongoose_acc:t()) -> groupchat | one2one. +-spec get_message_type(mongoose_acc:t()) -> message_type(). get_message_type(Acc) -> case mongoose_acc:stanza_type(Acc) of <<"groupchat">> -> groupchat; @@ -582,7 +572,19 @@ get_message_type(Acc) -> %%%%%%%%%%%%%%%%%%% %% Message Predicates --spec should_be_stored_in_inbox(Msg :: exml:element(), outgoing | incoming) -> boolean(). -should_be_stored_in_inbox(Msg, Dir) -> - mod_mam_utils:is_archivable_message(?MODULE, Dir, Msg, true) andalso - mod_inbox_entries:should_be_stored_in_inbox(Msg). +-spec should_be_stored_in_inbox( + mongoose_acc:t(), jid:jid(), jid:jid(), exml:element(), mod_mam_utils:direction()) -> + boolean(). +should_be_stored_in_inbox(Acc, From, To, Msg, Dir) -> + mod_mam_utils:is_archivable_message(?MODULE, Dir, Msg, true) + andalso mod_inbox_entries:should_be_stored_in_inbox(Msg) + andalso inbox_owner_exists(Acc, From, To, Dir). + +-spec inbox_owner_exists(mongoose_acc:t(), jid:jid(), jid:jid(), mod_mam_utils:direction()) -> + boolean(). +inbox_owner_exists(Acc, From, _To, outgoing) -> + HostType = mongoose_acc:host_type(Acc), + ejabberd_auth:does_user_exist(HostType, From, stored); +inbox_owner_exists(Acc, _From, To, incoming) -> + HostType = mongoose_acc:host_type(Acc), + ejabberd_auth:does_user_exist(HostType, To, stored). From ed786851cac10c1e77a0559f4e7bb2201b026427 Mon Sep 17 00:00:00 2001 From: Nelson Vides Date: Mon, 13 Dec 2021 10:02:11 +0100 Subject: [PATCH 5/6] Fix a bug where inbox passed rooms into does_user_exist This needs to verify the type of the request at the moment where we need to decide if the inbox owner exists. We can't return false because that would drop the required markers from resetting the count. We can't return `can_access_room` because then the message telling the user that he has been removed from a room would arrive to the user at a time when he's not a member of the room anymore. So we simply must return true from here. --- src/inbox/mod_inbox.erl | 27 +++++++++++++++------------ 1 file changed, 15 insertions(+), 12 deletions(-) diff --git a/src/inbox/mod_inbox.erl b/src/inbox/mod_inbox.erl index 93e74ec728a..5e9385dc2dd 100644 --- a/src/inbox/mod_inbox.erl +++ b/src/inbox/mod_inbox.erl @@ -229,19 +229,19 @@ disco_local_features(Acc) -> Msg :: exml:element(), Dir :: mod_mam_utils:direction()) -> mongoose_acc:t(). maybe_process_message(Acc, From, To, Msg, Dir) -> - case should_be_stored_in_inbox(Acc, From, To, Msg, Dir) of + Type = get_message_type(Acc), + case should_be_stored_in_inbox(Acc, From, To, Msg, Dir, Type) of true -> - do_maybe_process_message(Acc, From, To, Msg, Dir); + do_maybe_process_message(Acc, From, To, Msg, Dir, Type); false -> Acc end. -do_maybe_process_message(Acc, From, To, Msg, Dir) -> +do_maybe_process_message(Acc, From, To, Msg, Dir, Type) -> %% In case of PgSQL we can update inbox and obtain unread_count in one query, %% so we put it in accumulator here. %% In case of MySQL/MsSQL it costs an extra query, so we fetch it only if necessary %% (when push notification is created) - Type = get_message_type(Acc), HostType = mongoose_acc:host_type(Acc), case maybe_process_acceptable_message(HostType, From, To, Msg, Acc, Dir, Type) of ok -> Acc; @@ -573,18 +573,21 @@ get_message_type(Acc) -> %%%%%%%%%%%%%%%%%%% %% Message Predicates -spec should_be_stored_in_inbox( - mongoose_acc:t(), jid:jid(), jid:jid(), exml:element(), mod_mam_utils:direction()) -> + mongoose_acc:t(), jid:jid(), jid:jid(), exml:element(), mod_mam_utils:direction(), message_type()) -> boolean(). -should_be_stored_in_inbox(Acc, From, To, Msg, Dir) -> +should_be_stored_in_inbox(Acc, From, To, Msg, Dir, Type) -> mod_mam_utils:is_archivable_message(?MODULE, Dir, Msg, true) andalso mod_inbox_entries:should_be_stored_in_inbox(Msg) - andalso inbox_owner_exists(Acc, From, To, Dir). + andalso inbox_owner_exists(Acc, From, To, Dir, Type). --spec inbox_owner_exists(mongoose_acc:t(), jid:jid(), jid:jid(), mod_mam_utils:direction()) -> +-spec inbox_owner_exists( + mongoose_acc:t(), jid:jid(), jid:jid(), mod_mam_utils:direction(), message_type()) -> boolean(). -inbox_owner_exists(Acc, From, _To, outgoing) -> +inbox_owner_exists(_, _, _, incoming, groupchat) -> + true; +inbox_owner_exists(Acc, _From, To, incoming, _) -> % filter_local_packet HostType = mongoose_acc:host_type(Acc), - ejabberd_auth:does_user_exist(HostType, From, stored); -inbox_owner_exists(Acc, _From, To, incoming) -> + ejabberd_auth:does_user_exist(HostType, To, stored); +inbox_owner_exists(Acc, From, _To, outgoing, _) -> % user_send_packet HostType = mongoose_acc:host_type(Acc), - ejabberd_auth:does_user_exist(HostType, To, stored). + ejabberd_auth:does_user_exist(HostType, From, stored). From 61d90d015cfb9c2907497769bb5e529ce2747434 Mon Sep 17 00:00:00 2001 From: Nelson Vides Date: Tue, 14 Dec 2021 15:29:25 +0100 Subject: [PATCH 6/6] Fix the double filter_local_packet routing --- src/inbox/mod_inbox.erl | 31 ++++++++++++++++++++++++------- 1 file changed, 24 insertions(+), 7 deletions(-) diff --git a/src/inbox/mod_inbox.erl b/src/inbox/mod_inbox.erl index 5e9385dc2dd..10704a0e765 100644 --- a/src/inbox/mod_inbox.erl +++ b/src/inbox/mod_inbox.erl @@ -580,14 +580,31 @@ should_be_stored_in_inbox(Acc, From, To, Msg, Dir, Type) -> andalso mod_inbox_entries:should_be_stored_in_inbox(Msg) andalso inbox_owner_exists(Acc, From, To, Dir, Type). --spec inbox_owner_exists( - mongoose_acc:t(), jid:jid(), jid:jid(), mod_mam_utils:direction(), message_type()) -> - boolean(). -inbox_owner_exists(_, _, _, incoming, groupchat) -> - true; -inbox_owner_exists(Acc, _From, To, incoming, _) -> % filter_local_packet +-spec inbox_owner_exists(mongoose_acc:t(), + From :: jid:jid(), + To ::jid:jid(), + mod_mam_utils:direction(), + message_type()) -> boolean(). +inbox_owner_exists(Acc, _, To, incoming, groupchat) -> + case is_to_room(To) of + true -> false; + false -> + HostType = mongoose_acc:host_type(Acc), + ejabberd_auth:does_user_exist(HostType, To, stored) + end; +inbox_owner_exists(Acc, _, To, incoming, _) -> % filter_local_packet HostType = mongoose_acc:host_type(Acc), ejabberd_auth:does_user_exist(HostType, To, stored); -inbox_owner_exists(Acc, From, _To, outgoing, _) -> % user_send_packet +inbox_owner_exists(Acc, From, _, outgoing, _) -> % user_send_packet HostType = mongoose_acc:host_type(Acc), ejabberd_auth:does_user_exist(HostType, From, stored). + +%% WHY: filter_local_packet is executed twice in the pipeline of muc messages. in two routing steps: +%% - From the sender to the room: runs filter_local_packet with From=Sender, To=Room +%% - For each member of the room: +%% From the room to each member: runs with From=Room/Sender, To=Member +%% So, as inbox is a per-user concept, it is on the second routing step only when we want to do act. +%% NOTE: ideally for groupchats, we could instead act on `filter_room_packet`, like MAM. +-spec is_to_room(jid:jid()) -> boolean(). +is_to_room(Jid) -> + {error, not_found} =:= mongoose_domain_api:get_domain_host_type(Jid#jid.lserver).