Merge pull request #4475 from esl/pep-disco-fix

Properly handle Account Owner Service Discovery request

Author: arcusfelis

big_tests/tests/pep_SUITE.erl

@@ -21,7 +21,9 @@
 -export([
          disco_test/1,
          disco_sm_test/1,
+         disco_sm_node_test/1,
          disco_sm_items_test/1,
+         disco_sm_items_node_test/1,
          pep_caps_test/1,
          publish_and_notify_test/1,
          auto_create_with_publish_options_test/1,
@@ -68,7 +70,9 @@ groups() ->
           [
            disco_test,
            disco_sm_test,
+           disco_sm_node_test,
            disco_sm_items_test,
+           disco_sm_items_node_test,
            pep_caps_test,
            publish_and_notify_test,
            auto_create_with_publish_options_test,
@@ -148,20 +152,39 @@ disco_test(Config) ->
       end).
 
 disco_sm_test(Config) ->
+    disco_sm_test(Config, undefined).
+
+disco_sm_node_test(Config) ->
+    disco_sm_test(Config, random_node_ns()).
+
+disco_sm_test(Config, Node) ->
     escalus:fresh_story(
-      Config,
-      [{alice, 1}],
-      fun(Alice) ->
-              AliceJid = escalus_client:short_jid(Alice),
-              escalus:send(Alice, escalus_stanza:disco_info(AliceJid)),
-              Stanza = escalus:wait_for_stanza(Alice),
-              ?assertNot(escalus_pred:has_identity(<<"pubsub">>, <<"service">>, Stanza)),
-              escalus:assert(has_identity, [<<"pubsub">>, <<"pep">>], Stanza),
-              escalus:assert(has_feature, [?NS_PUBSUB], Stanza),
-              escalus:assert(is_stanza_from, [AliceJid], Stanza)
-      end).
+        Config,
+        [{alice, 1}],
+        fun(Alice) ->
+            AliceJid = escalus_client:short_jid(Alice),
+            Disco =
+                case Node of
+                    undefined ->
+                        escalus_stanza:disco_info(AliceJid);
+                    _ ->
+                        escalus_stanza:disco_info(AliceJid, Node)
+                end,
+            escalus:send(Alice, Disco),
+            Stanza = escalus:wait_for_stanza(Alice),
+            ?assertNot(escalus_pred:has_identity(<<"pubsub">>, <<"service">>, Stanza)),
+            escalus:assert(has_identity, [<<"pubsub">>, <<"pep">>], Stanza),
+            escalus:assert(has_feature, [?NS_PUBSUB], Stanza),
+            escalus:assert(is_stanza_from, [AliceJid], Stanza)
+        end).
 
 disco_sm_items_test(Config) ->
+    disco_sm_items_test(Config, false).
+
+disco_sm_items_node_test(Config) ->
+    disco_sm_items_test(Config, true).
+
+disco_sm_items_test(Config, UseNode) ->
     NodeNS = random_node_ns(),
     escalus:fresh_story(
       Config,
@@ -170,7 +193,11 @@ disco_sm_items_test(Config) ->
               AliceJid = escalus_client:short_jid(Alice),
 
               %% Node not present yet
-              escalus:send(Alice, escalus_stanza:disco_items(AliceJid)),
+              DiscoStanza = case UseNode of
+                                true -> escalus_stanza:disco_items(AliceJid, NodeNS);
+                                false -> escalus_stanza:disco_items(AliceJid)
+                            end,
+              escalus:send(Alice, DiscoStanza),
               Stanza1 = escalus:wait_for_stanza(Alice),
               Query1 = exml_query:subelement(Stanza1, <<"query">>),
               ?assertEqual(undefined, exml_query:subelement_with_attr(Query1, <<"node">>, NodeNS)),
@@ -180,7 +207,7 @@ disco_sm_items_test(Config) ->
               pubsub_tools:publish(Alice, <<"item1">>, {pep, NodeNS}, []),
 
               %% Node present
-              escalus:send(Alice, escalus_stanza:disco_items(AliceJid)),
+              escalus:send(Alice, DiscoStanza),
               Stanza2 = escalus:wait_for_stanza(Alice),
               Query2 = exml_query:subelement(Stanza2, <<"query">>),
               Item = exml_query:subelement_with_attr(Query2, <<"node">>, NodeNS),

src/pubsub/mod_pubsub.erl

@@ -650,34 +650,14 @@ disco_local_features(Acc, _, _) ->
     Acc :: mongoose_disco:identity_acc(),
     Params :: map(),
     Extra :: gen_hook:extra().
-disco_sm_identity(Acc = #{from_jid := From, to_jid := To, node := Node}, _, _) ->
-    Identities = disco_identity(jid:to_lower(jid:to_bare(To)), Node, From),
+disco_sm_identity(Acc = #{from_jid := From, to_jid := To}, _, _) ->
+    Identities = disco_identity(jid:to_lower(jid:to_bare(To)), From),
     {ok, mongoose_disco:add_identities(Identities, Acc)}.
 
-disco_identity(error, _Node, _From) ->
+disco_identity(error, _From) ->
     [];
-disco_identity(_Host, <<>>, _From) ->
-    [pep_identity()];
-disco_identity(Host, Node, From) ->
-    Action = fun (#pubsub_node{id = Nidx, type = Type, options = Options, owners = Owners}) ->
-                     case get_allowed_items_call(Host, Nidx, From, Type, Options, Owners) of
-                         {result, _} ->
-                             {result, [pep_identity(), pep_identity(Options)]};
-                         _ ->
-                             {result, []}
-                     end
-             end,
-    case dirty(Host, Node, Action, ?FUNCTION_NAME) of
-        {result, {_, Result}} -> Result;
-        _ -> []
-    end.
-
-pep_identity(Options) ->
-    Identity = pep_identity(),
-    case get_option(Options, title) of
-        false -> Identity;
-        [Title] -> Identity#{name => Title}
-    end.
+disco_identity(_Host, _From) ->
+    [pep_identity()].
 
 pep_identity() ->
     #{category => <<"pubsub">>, type => <<"pep">>}.
@@ -686,39 +666,26 @@ pep_identity() ->
     Acc :: mongoose_disco:feature_acc(),
     Params :: map(),
     Extra :: gen_hook:extra().
-disco_sm_features(Acc = #{from_jid := From, to_jid := To, node := Node}, _, _) ->
-    Features = disco_features(jid:to_lower(jid:to_bare(To)), Node, From),
+disco_sm_features(Acc = #{from_jid := From, to_jid := To}, _, _) ->
+    Features = disco_features(jid:to_lower(jid:to_bare(To)), From),
     {ok, mongoose_disco:add_features(Features, Acc)}.
 
--spec disco_features(error | jid:simple_jid(), binary(), jid:jid()) -> [mongoose_disco:feature()].
-disco_features(error, _Node, _From) ->
+-spec disco_features(error | jid:simple_jid(), jid:jid()) -> [mongoose_disco:feature()].
+disco_features(error, _From) ->
     [];
-disco_features(_Host, <<>>, _From) ->
-    [?NS_PUBSUB | [feature(F) || F <- plugin_features(<<"pep">>)]];
-disco_features(Host, Node, From) ->
-    Action = fun (#pubsub_node{id = Nidx, type = Type, options = Options, owners = Owners}) ->
-                     case get_allowed_items_call(Host, Nidx, From, Type, Options, Owners) of
-                         {result, _} ->
-                             {result, [?NS_PUBSUB | [feature(F) || F <- plugin_features(<<"pep">>)]]};
-                         _ ->
-                             {result, []}
-                     end
-             end,
-    case dirty(Host, Node, Action, ?FUNCTION_NAME) of
-        {result, {_, Result}} -> Result;
-        _ -> []
-    end.
+disco_features(_Host, _From) ->
+    [?NS_PUBSUB | [feature(F) || F <- plugin_features(<<"pep">>)]].
 
 -spec disco_sm_items(Acc, Params, Extra) -> {ok, Acc} when
     Acc :: mongoose_disco:item_acc(),
     Params :: map(),
     Extra :: gen_hook:extra().
-disco_sm_items(Acc = #{from_jid := From, to_jid := To, node := Node}, _, _) ->
-    Items = disco_items(jid:to_lower(jid:to_bare(To)), Node, From),
+disco_sm_items(Acc = #{from_jid := From, to_jid := To}, _, _) ->
+    Items = disco_items(jid:to_lower(jid:to_bare(To)), From),
     {ok, mongoose_disco:add_items(Items, Acc)}.
 
--spec disco_items(mod_pubsub:host(), mod_pubsub:nodeId(), jid:jid()) -> [mongoose_disco:item()].
-disco_items(Host, <<>>, From) ->
+-spec disco_items(mod_pubsub:host(), jid:jid()) -> [mongoose_disco:item()].
+disco_items(Host, From) ->
     Action = fun (#pubsub_node{nodeid = {_, Node},
                                options = Options, type = Type, id = Nidx, owners = Owners},
                   Acc) ->
@@ -741,20 +708,6 @@ disco_items(Host, <<>>, From) ->
     case mod_pubsub_db_backend:dirty(NodeBloc, ErrorDebug) of
         {result, Items} -> Items;
         _ -> []
-    end;
-disco_items(Host, Node, From) ->
-    Action = fun (#pubsub_node{id = Nidx, type = Type, options = Options, owners = Owners}) ->
-                     case get_allowed_items_call(Host, Nidx, From, Type, Options, Owners) of
-                         {result, Items} ->
-                             {result, [disco_item(Host, ItemId) ||
-                                          #pubsub_item{itemid = {ItemId, _}} <- Items]};
-                         _ ->
-                             {result, []}
-                     end
-             end,
-    case dirty(Host, Node, Action, ?FUNCTION_NAME) of
-        {result, {_, Result}} -> Result;
-        _ -> []
     end.
 
 disco_item(Node, Host, Options) ->
@@ -765,10 +718,6 @@ disco_item(Node, Host, Options) ->
         [Title] -> Item#{name => Title}
     end.
 
-disco_item(Host, ItemId) ->
-    #{jid => jid:to_binary(Host),
-      name => ItemId}.
-
 %% -------
 %% callback that prevents routing subscribe authorizations back to the sender
 %%