Bug 13157

The relevant section is now at <http://xmpp.org/extensions/xep-0045.html#disco-roominfo>.

I have a branch for this. Not tested against a real server yet...

Comments in the branch describe how this is actually implemented in servers, in practice (I looked at the source code of recent versions of Openfire, Tigase, ejabberd and mu-conference; plus Prosody, which currently lets absolutely anyone set the subject).

muc#roominfo_changesubject isn't actually implemented anywhere as far as I can tell.

muc#roomconfig_changesubject is widely implemented, but it's ambiguous what it should mean. In the branch I assumed that the server is Openfire, Tigase or ejabberd, in which this setting is the difference between "you must be a moderator" and "you must be a participant or moderator".

In mu-conference this setting is actually the difference between "you must be an admin" and "anyone can set it"; due to us assuming the other interpretation, users will find that Gabble won't let them change the subject with role 'visitor', only with 'participant' or 'moderator', even though it would actually work.

This is not a regression, at least, since Gabble previously had a bug in which it only ever allowed moderators to set the subject, despite a special case designed to avoid that restriction...

I'm not sure whether this is a candidate for 0.8 or not, so to be safe, I developed the branch against 0.8.

So, XEP-0045 §5.1.1 has a footnote that configuration settings MAY affect whether visitors can change the subject. We should seek clarification from some XMPP list. :)

Taking this bug to review the branch.

+            assert prop_flags[name] == cs.PROPERTY_FLAG_WRITE, \
+                    (name, prop_flags[name])
+        else:
+            assert prop_flags[name] == 0, (name, prop_flags[name])

Use assertEquals please.

+        assert (props['subject-contact'], cs.PROPERTY_FLAG_READ) in e.args[0]
+        assert (props['subject-timestamp'], cs.PROPERTY_FLAG_READ) in e.args[0]

assertContains.

+    x = query.addElement(('jabber:x:data', 'x'))

ns.py

Do no servers implement muc#roominfo_subject?

Why do you join a different room for the second test in 50c1556? Surely it should work just as well if you re-join the same room.

     for change_subject in (None, True, False):
         for send_first in (True, False):
-            test_subject(q, bus, conn, stream, change_subject, send_first)
+            for moderator in (True, False):
+                test_subject(q, bus, conn, stream, change_subject, send_first,
+                        moderator)

One problem with this is that python backtraces don't show the values of arguments, so it's harder to see which test is failing. While 3×2×2 duplicated calls to test_subject offend a small part of me, it'd just be 10 lines rather than 5.

+  if (self->priv->self_role >=
+      (self->priv->mortals_can_change_subject
+       ? ROLE_PARTICIPANT
+       : ROLE_MODERATOR))
+    {

Maybe it'd be clearer to store ->change_subject_level, and put the explanation into properties_disco_cb, the point where the disco reply is actually interpreted?

Otherwise looks good, these are all nit-picks. Did you test it against real servers?

(In reply to comment #2)
> muc#roominfo_changesubject isn't actually implemented anywhere as far as I can
> tell.

Google, and Google Code Search, only turn up hits from Telepathy or the XEP.

(In reply to comment #4)
> Do no servers implement muc#roominfo_subject?

That's orthogonal. I believe servers support it, and in principle we could use it for state-recovery of the initial subject; would you like me to expand the scope of this branch to do that too?

> Why do you join a different room for the second test in 50c1556? Surely it
> should work just as well if you re-join the same room.

If I remember correctly, there was something weird involving re-disco'ing the same room. I'll investigate whether the current code is in fact subtly broken, or whether I just used different rooms to make the assertions more specific...

>      for change_subject in (None, True, False):
>          for send_first in (True, False):
> -            test_subject(q, bus, conn, stream, change_subject, send_first)
> +            for moderator in (True, False):
> +                test_subject(q, bus, conn, stream, change_subject, send_first,
> +                        moderator)
> 
> One problem with this is that python backtraces don't show the values of
> arguments, so it's harder to see which test is failing. While 3×2×2
> duplicated calls to test_subject offend a small part of me, it'd just be 10
> lines rather than 5.

A fair point, I'll change that.

> +  if (self->priv->self_role >=
> +      (self->priv->mortals_can_change_subject
> +       ? ROLE_PARTICIPANT
> +       : ROLE_MODERATOR))
> +    {
> 
> Maybe it'd be clearer to store ->change_subject_level, and put the explanation
> into properties_disco_cb, the point where the disco reply is actually
> interpreted?

You're right, I'll do that.

> Otherwise looks good, these are all nit-picks. Did you test it against real
> servers?

Not yet.

(In reply to comment #4)
> Use assertEquals please.

Done in my branch.

> assertContains.

Done in my branch.

> ns.py

Done in my branch.

> One problem with this is that python backtraces don't show the values of
> arguments, so it's harder to see which test is failing. While 3×2×2
> duplicated calls to test_subject offend a small part of me, it'd just be 10
> lines rather than 5.

Done in my branch.

> Maybe it'd be clearer to store ->change_subject_level, and put the explanation
> into properties_disco_cb, the point where the disco reply is actually
> interpreted?

Done in my branch.

> Did you test it against real servers?

Unfortunately, now that I have, it appears that despite XEP-0045 suggesting that roomconfig_changesubject could (should?) be in the disco#info, nobody actually does that.

Meanwhile, we can't just poll the room config, because unless we're an owner, the server MUST (as per the XEP) refuse to let us read the room config (I haven't actually checked to see whether any of the servers let unprivileged people read it anyway, but even if they do violate that requirement, it seems a bad idea for us to rely on that). Also, polling is rubbish.

So, I think the best we can do for this is to file bugs on various servers asking them to put roomconfig_changesubject and friends in their disco#info; until they do, we'll assume that all MUC servers will let us change the subject.

This probably means that this branch shouldn't be merged to 0.8.x, and perhaps it should go on hold in 0.9.x too until we get some sort of clarification of the XEP.

As I understand it, the second patch in your branch fixes things so Gabble actually does assume that you can change the subject. Maybe we should merge that one to 0.8 (so that a well-behaved UI will at least let you try to change the subject) having checked that the resulting error is reported in some useful way?

Merging the first two patches to 0.8 and 0.9 would be good (the first one fixes an uninitialized self->priv).

Mitigated in both 0.8 and 0.9 by merging the beginning of my branch; I've updated the branch for 0.9 (there's no point in targeting 0.8 for something that requires future server support anyway) and renamed it to change-the-subject.

Remaining things in change-the-subject:

* Harvest the initial subject from the disco#info if available. Should work (untested so far) with current Openfire, but is unsupported with current ejabberd, Prosody or Tigase; I think it may be worth splitting this into a separate bug/branch and testing that it at least works with Openfire.

* Try to determine who can change the subject from muc#roomconfig_changesubject. Unsupported in all servers I tried, and somewhat vague in the XEP too.

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/telepathy/telepathy-gabble/issues/3.