Bug 13157 - support flag to indicate whether MUC subject is writable
Summary: support flag to indicate whether MUC subject is writable
Status: NEW
Alias: None
Product: Telepathy
Classification: Unclassified
Component: gabble (show other bugs)
Version: unspecified
Hardware: Other All
: low enhancement
Assignee: Simon McVittie
QA Contact: Telepathy bugs list
URL: http://git.collabora.co.uk/?p=user/sm...
Whiteboard: partial
Keywords:
Depends on:
Blocks:
 
Reported: 2007-11-09 05:05 UTC by Robert McQueen
Modified: 2009-11-12 07:20 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Robert McQueen 2007-11-09 05:05:04 UTC
We should support this; mail from Peter Saint-Andre:

Sometimes it takes me a long time to fix things, but I get around to
things eventually. See the penultimate paragraph here:

http://www.xmpp.org/extensions/tmp/xep-0045-1.23.html#disco-roominfo

Peter Saint-Andre wrote:
> > Jabber IM with Robert McQueen <robert.mcqueen@collabora.co.uk>.
> > 1:43 PM
> > Robert McQueen: hey, got a moment?
> > Peter Saint-Andre: hi
> > Robert McQueen: I'm just trying to double check in the JEP, but it seems
> > to me that there's no way for a MUC occupant to know if they may change
> > the topic in a given room without just trying it
> > 1:45 PM
> > Peter Saint-Andre: maybe
> > Peter Saint-Andre: I'd have to look at it more
> > Robert McQueen: our testers are filing bugs that the field is modifiable
> > by the user, and then they get an error
> > Peter Saint-Andre: ok, I will look into that
> > Robert McQueen: cool, thanks
> > 2:40 PM
> > Robert McQueen has gone offline.
> > 3:35 PM
> > Peter Saint-Andre: the idea is that any of the configuration settings at
> > http://www.jabber.org/jeps/jep-0045.html#registrar-formtype-owner could
> > be included in the extended room information as described at
> > http://www.jabber.org/jeps/tmp/jep-0045-1.21.html#disco-roominfo but
> > that is not fully explained
Comment 1 Will Thompson 2009-10-19 05:00:54 UTC
The relevant section is now at <http://xmpp.org/extensions/xep-0045.html#disco-roominfo>.
Comment 2 Simon McVittie 2009-10-22 10:33:35 UTC
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.
Comment 3 Will Thompson 2009-10-23 10:24:39 UTC
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.
Comment 4 Will Thompson 2009-11-09 11:00:37 UTC
+            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?
Comment 5 Simon McVittie 2009-11-11 06:20:23 UTC
(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.
Comment 6 Simon McVittie 2009-11-11 10:18:42 UTC
(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.
Comment 7 Will Thompson 2009-11-11 10:21:16 UTC
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?
Comment 8 Simon McVittie 2009-11-11 10:26:17 UTC
Merging the first two patches to 0.8 and 0.9 would be good (the first one fixes an uninitialized self->priv).
Comment 9 Simon McVittie 2009-11-12 07:20:34 UTC
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.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.