[ejabberd] [PATCH] mod_shared_roster: Sanity check in remove_user_from_group() and add_user_to_group()

Badlop badlop at gmail.com
Tue Apr 21 14:46:21 MSD 2009


2009/4/17  <martin.langhoff at gmail.com>:
> When handling special users ('@all@', '@online', '@recent@', '@nearby@'),
> ensure we are dealing with the localhost.
>
> --- a/src/mod_shared_roster.erl
> +++ b/src/mod_shared_roster.erl
> @@ -623,6 +623,10 @@ add_user_to_group(Host, US, Group) ->
>
>     case regexp:match(LUser, "^@.+@$") of
>        {match,_,_} ->
> +
> +           % Sanity check - magic groups can only be manipulated locally
> +           Host = LServer,
> +
>            GroupOpts = mod_shared_roster:get_group_opts(Host, Group),
>            AllUsersOpt =
>                case LUser == "@all@" of

This is what I understood:

This patch forces a process crash when the administrator tries to add
as member of a shared roster group a JID which is not of the local
domain.

The admin will see in the Web Admin: "Not Found" [1]

And in ejabberd.log: [2]
=ERROR REPORT==== 21-Apr-2009::12:11:15 ===
E(<0.936.0>:ejabberd_hooks:248) : {{badmatch,"localhost2"},
                                   [{mod_shared_roster,add_user_to_group,3},
                                    {lists,foreach,2},
...

The reason to make this verification is that you don't want ejabberd
to allow dumb admins to add as members JIDs that are not local. [3]

Notice that a dumb admin still can add non-existent local JIDs [4].
So, if your idea is to make shared-roster dumb-proof, there is still
work to do [5].

These are my objections:

[1] When the administrator uses the WebAdmin, he does not receive any
indication about what was wrong with his web form submission.

[2] If the admin checks the logs for some hint of the problem, he sees
an error message that does not indicate what is the problem.

[3] It is not dumb that an admin adds as member a JID that is not
local. There are rare cases in which this is useful. Of course
presence will not be received from that remote JID. Instead of
reducing the possibilities of shared rosters, it is preferable to
document that only local accounts will get proper presence, and let
the admin decide what he wants.

[4] There are also valid reasons to add as member of a group a local
JID that doesn't exist. Similar to 3, I prefer to document, and let
the admin decide.

[5] If it's accepted that there are valid reasons to allow as members
non-existent and non-local JID (3 and 4), then there is not more work
to do in that field.

In your OLPC-ejabberd, this patch may be useful, as you know what
exact usage mod_shared_roster will get from your admins. However, the
generic ejabberd has a wide range of usages.

In summary, I propose to not apply this patch in the main ejabberd.
The problem is not the misfeatures of this patch (which could be
solved). My main objection is that it restricts the possible usages of
shared rosters.


PD: Regarding [1] and [2], you may notice that, unfortunately, some
parts of ejabberd do not yet provide human-readable error messages.


---
Badlop,
ProcessOne


More information about the ejabberd mailing list