Inbox filter bug that puts unnecessary load on the auth backend

big_tests/tests/inbox_SUITE.erl

@@ -1,81 +1,14 @@
 -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("jid/include/jid.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,
@@ -158,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],
       [
@@ -208,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) ->
@@ -1031,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
@@ -1371,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.

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,21 +227,21 @@ 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
+    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;
@@ -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,39 @@ 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(), message_type()) ->
+    boolean().
+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, Type).
+
+-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, _, 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).

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,20 +339,18 @@ 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
         is_valid_message(Mod, Dir, Packet, ArchiveChatMarkers);
 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) ->