Report more friendly errors on invalid stanza id

big_tests/tests/mam_SUITE.erl

@@ -81,6 +81,7 @@
          pagination_offset5_opt_count_all/1,
          server_returns_item_not_found_for_before_filter_with_nonexistent_id/1,
          server_returns_item_not_found_for_after_filter_with_nonexistent_id/1,
+         server_returns_item_not_found_for_after_filter_with_invalid_id/1,
          %% complete_flag_cases tests
          before_complete_false_last5/1,
          before_complete_false_before10/1,
@@ -500,7 +501,8 @@ rsm_cases() ->
        pagination_offset5_opt_count_all,
        %% item_not_found response for nonexistent message ID in before/after filters
        server_returns_item_not_found_for_before_filter_with_nonexistent_id,
-       server_returns_item_not_found_for_after_filter_with_nonexistent_id].
+       server_returns_item_not_found_for_after_filter_with_nonexistent_id,
+       server_returns_item_not_found_for_after_filter_with_invalid_id].
 
 complete_flag_cases() ->
     [before_complete_false_last5,
@@ -2587,6 +2589,13 @@ server_returns_item_not_found_for_after_filter_with_nonexistent_id(Config) ->
     Condition = [<<"cancel">>, <<"item-not-found">>],
     server_returns_item_not_found_for_nonexistent_id(Config, RSM, StanzaID, Condition).
 
+server_returns_item_not_found_for_after_filter_with_invalid_id(Config) ->
+    NonexistentID = <<"bef3a242-99ce-402a-9ffc-2f3c20da92d4">>,
+    RSM = #rsm_in{max = 5, direction = 'after', from_id = NonexistentID},
+    StanzaID = <<"AV25E9SCO50K">>,
+    Condition = [<<"modify">>, <<"not-acceptable">>],
+    server_returns_item_not_found_for_nonexistent_id(Config, RSM, StanzaID, Condition).
+
 server_returns_item_not_found_for_nonexistent_id(Config, RSM, StanzaID, Condition) ->
     P = ?config(props, Config),
     F = fun(Alice) ->

src/mam/mod_mam.erl

@@ -106,7 +106,6 @@
 -include("mongoose.hrl").
 -include("jlib.hrl").
 -include("amp.hrl").
--include_lib("exml/include/exml.hrl").
 -include("mod_mam.hrl").
 
 %% ----------------------------------------------------------------------
@@ -432,13 +431,29 @@ handle_get_prefs_result({error, Reason}, IQ) ->
 -spec handle_set_message_form(From :: jid:jid(), ArcJID :: jid:jid(),
                               IQ :: jlib:iq(), Acc :: mongoose_acc:t()) ->
     jlib:iq() | ignore | {error, term(), jlib:iq()}.
-handle_set_message_form(#jid{} = From, #jid{} = ArcJID,
-                        #iq{xmlns=MamNs, sub_el = QueryEl} = IQ,
-                        Acc) ->
+handle_set_message_form(#jid{} = From, #jid{} = ArcJID, #iq{} = IQ, Acc) ->
     HostType = acc_to_host_type(Acc),
     ArcID = archive_id_int(HostType, ArcJID),
+    try iq_to_lookup_params(HostType, IQ) of
+        Params0 ->
+            do_handle_set_message_form(Params0, From, ArcID, ArcJID, IQ, HostType)
+    catch _C:R:S ->
+            report_issue({R, S}, mam_lookup_failed, ArcJID, IQ),
+            return_error_iq(IQ, R)
+    end.
+
+
+-spec do_handle_set_message_form(Params :: mam_iq:lookup_params(),
+                                 From :: jid:jid(),
+                                 ArcId :: archive_id(),
+                                 ArcJID :: jid:jid(),
+                                 IQ :: jlib:iq(),
+                                 HostType :: mongooseim:host_type()) ->
+    jlib:iq() | ignore | {error, term(), jlib:iq()}.
+do_handle_set_message_form(Params0, From, ArcID, ArcJID,
+                           #iq{xmlns=MamNs, sub_el = QueryEl} = IQ,
+                           HostType) ->
     QueryID = exml_query:attr(QueryEl, <<"queryid">>, <<>>),
-    Params0 = iq_to_lookup_params(HostType, IQ),
     Params = mam_iq:lookup_params_with_archive_details(Params0, ArcID, ArcJID, From),
     case lookup_messages(HostType, Params) of
         {error, Reason} ->
@@ -683,6 +698,9 @@ return_error_iq(IQ, {Reason, {stacktrace, _Stacktrace}}) ->
 return_error_iq(IQ, timeout) ->
     E = mongoose_xmpp_errors:service_unavailable(<<"en">>, <<"Timeout">>),
     {error, timeout, IQ#iq{type = error, sub_el = [E]}};
+return_error_iq(IQ, invalid_stanza_id) ->
+    Text = mongoose_xmpp_errors:not_acceptable(<<"en">>, <<"Invalid stanza id provided">>),
+    {error, invalid_stanza_id, IQ#iq{type = error, sub_el = [Text]}};
 return_error_iq(IQ, item_not_found) ->
     {error, item_not_found, IQ#iq{type = error, sub_el = [mongoose_xmpp_errors:item_not_found()]}};
 return_error_iq(IQ, not_implemented) ->
@@ -698,6 +716,8 @@ report_issue({Reason, {stacktrace, Stacktrace}}, Issue, ArcJID, IQ) ->
 report_issue(Reason, Issue, ArcJID, IQ) ->
     report_issue(Reason, [], Issue, ArcJID, IQ).
 
+report_issue(invalid_stanza_id, _Stacktrace, _Issue, _ArcJID, _IQ) ->
+    expected;
 report_issue(item_not_found, _Stacktrace, _Issue, _ArcJID, _IQ) ->
     expected;
 report_issue(not_implemented, _Stacktrace, _Issue, _ArcJID, _IQ) ->

src/mam/mod_mam_muc.erl

@@ -78,7 +78,6 @@
          result_set/4,
          result_query/2,
          result_prefs/4,
-         make_fin_message/5,
          make_fin_element/7,
          parse_prefs/1,
          borders_decode/1,
@@ -388,7 +387,23 @@ handle_set_message_form(HostType, #jid{} = From, #jid{} = ArcJID, IQ) ->
     ResLimit = mod_mam_params:max_result_limit(?MODULE, HostType),
     DefLimit = mod_mam_params:default_result_limit(?MODULE, HostType),
     ExtMod = mod_mam_params:extra_params_module(?MODULE, HostType),
-    Params0 = mam_iq:form_to_lookup_params(IQ, ResLimit, DefLimit, ExtMod),
+    try mam_iq:form_to_lookup_params(IQ, ResLimit, DefLimit, ExtMod) of
+        Params0 ->
+            do_handle_set_message_form(HostType, From, ArcID, ArcJID, IQ, Params0)
+    catch _C:R:S ->
+            report_issue({R, S}, mam_lookup_failed, ArcJID, IQ),
+            return_error_iq(IQ, R)
+    end.
+
+
+-spec do_handle_set_message_form(HostType :: mongooseim:host_type(),
+                                 From :: jid:jid(),
+                                 ArcId :: mod_mam:archive_id(),
+                                 ArcJID :: jid:jid(),
+                                 IQ :: jlib:iq(),
+                                 Params :: mam_iq:lookup_params()) ->
+    jlib:iq() | ignore | {error, term(), jlib:iq()}.
+do_handle_set_message_form(HostType, From, ArcID, ArcJID, IQ, Params0) ->
     Params = mam_iq:lookup_params_with_archive_details(Params0, ArcID, ArcJID, From),
     Result = lookup_messages(HostType, Params),
     handle_lookup_result(Result, HostType, From, IQ, Params).
@@ -566,6 +581,9 @@ return_error_iq(IQ, {Reason, {stacktrace, _Stacktrace}}) ->
     return_error_iq(IQ, Reason);
 return_error_iq(IQ, timeout) ->
     {error, timeout, IQ#iq{type = error, sub_el = [mongoose_xmpp_errors:service_unavailable(<<"en">>, <<"Timeout in mod_mam_muc">>)]}};
+return_error_iq(IQ, invalid_stanza_id) ->
+    Text = mongoose_xmpp_errors:not_acceptable(<<"en">>, <<"Invalid stanza id provided">>),
+    {error, invalid_stanza_id, IQ#iq{type = error, sub_el = [Text]}};
 return_error_iq(IQ, item_not_found) ->
     {error, item_not_found, IQ#iq{type = error, sub_el = [mongoose_xmpp_errors:item_not_found()]}};
 return_error_iq(IQ, not_implemented) ->
@@ -600,6 +618,8 @@ report_issue({Reason, {stacktrace, Stacktrace}}, Issue, ArcJID, IQ) ->
 report_issue(Reason, Issue, ArcJID, IQ) ->
     report_issue(Reason, [], Issue, ArcJID, IQ).
 
+report_issue(invalid_stanza_id, _Stacktrace, _Issue, _ArcJID, _IQ) ->
+    expected;
 report_issue(item_not_found, _Stacktrace, _Issue, _ArcJID, _IQ) ->
     expected;
 report_issue(missing_with_jid, _Stacktrace, _Issue, _ArcJID, _IQ) ->

src/mam/mod_mam_utils.erl

@@ -225,7 +225,9 @@ maybe_external_binary_to_mess_id(BExtMessID) ->
 %% @doc Decode a message ID received from the user.
 -spec external_binary_to_mess_id(binary()) -> integer().
 external_binary_to_mess_id(BExtMessID) when is_binary(BExtMessID) ->
-    binary_to_integer(BExtMessID, 32).
+    try binary_to_integer(BExtMessID, 32)
+    catch error:badarg -> throw(invalid_stanza_id)
+    end.
 
 %% -----------------------------------------------------------------------
 %% XML