1011226 - Support setting the topic in XMPP MUCs

Status: RESOLVED FIXED

(Chat Core :: XMPP, defect)

Description

[Saurabh Anand [:sawrubh]]

We need to implement support for setting the subject/topic in a MUC in JS-XMPP using both the UI and the '/topic' command. I'll first take a look at doing this using the UI and then via '/topic' (otherwise the latter might be fixed in a separate bug, if that makes more sense).

Comment 1

[Saurabh Anand [:sawrubh]]

Attachment: Patch(WIP)

What works : Everything works in a room where the message stanza containing a subject doesn't have a body.
What doesn't work : In a room where the message stanza containing the subject element also has a body element, everything goes berzerk. The errors being :
1) participant is not defined on line 107
2) muc.incomingMessage is not a function on line 935

I'm not clear why the participant is not defined when there is a body element.

Comment 2

[aleth [:aleth]]

(In reply to Saurabh Anand [:sawrubh] from comment #1)
> I'm not clear why the participant is not defined when there is a body
> element.

If you look, participants are set here http://mxr.mozilla.org/comm-central/source/chat/protocols/xmpp/xmpp.jsm#100, so you should investigate, by adding dump calls or using the debugger: Does that method get called when you join the MUC? If not, why not? My guess is something goes wrong with the parsing (you have error messages after all) and it never gets called because of the errors. If so, fix the parsing and things should work?

Comment 3

[Patrick Cloke [:clokep]]

Comment on attachment 8427142 [details] [diff] [review]
Patch(WIP)

Review of attachment 8427142 [details] [diff] [review]:
-----------------------------------------------------------------

Overall it looks like it's on the right path. The setter the topic certainly looks good -- just a couple of nits!

::: chat/protocols/xmpp/xmpp.jsm
@@ +92,5 @@
>  
> +  get topic() this._topic,
> +  set topic(aTopic) {
> +    // Create a subject element with the topic to be set
> +    let child = Stanza.node("subject", null, null, aTopic);

I think naming this subject would make more sense!

@@ +100,5 @@
> +  },
> +  get topicSettable() {
> +    // By default, only moderators (or higher) should be allowed to change the subject
> +    // http://xmpp.org/extensions/xep-0045.html#subject-mod
> +    // TODO : Check the muc#roomconfig_changesubject for finding out the actual config

Can you re-word this comment to make a bit more sense. (Also is there a standard way we've been including references to specs in the XMPP code? In the IRC code I usually do it by RFC # & section instead of the whole link. E.g. "According to XEP-0045 8.1: by default, only moderators (or higher) are allowed to change the subject.")

@@ +890,5 @@
>      let b = aStanza.getElement(["body"]);
> +    let muc = this._mucs[norm];
> +    let s = aStanza.getElement(["subject"]);
> +    let nick = this._parseJID(aStanza.attributes["from"]).resource;
> +

I think it makes sense to declare all this code before it's used, i.e. right before we use the body, let's declare b, right before we want the subject, declare s.

@@ +912,5 @@
> +    if (s) {
> +      if (!body) {
> +        muc.setTopic(s.innerText, nick);
> +      }
> +    }

Why the !body check here? I think returning after calling setTopic might make everything magically "work".

@@ +933,2 @@
>          if (s)
> +          muc.setTopic(s.innerText, nick);

Why do we need the second call to setTopic here?

Comment 4

[Saurabh Anand [:sawrubh]]

I added the early return and it has fixed things. However I see the message of the form 'The topic for <channel name> is %s'. I wanted it to be of the form '%s has set the topic to %s'.

The problems I'm facing right now :
* I was relying on the server sending a message stanza with the 'from' attribute having the nick of the person who set the topic and thereby being able to show the message of the form '%s has set the topic to %s'. But while testing with [e-mail], which sends a message stanza of this form (having a body element) :
<message xmlns="jabber:client" from="[e-mail]" to="[e-mail]/Instantbir8394BD17" type="groupchat">
 <subject xmlns="jabber:client">
  Pidgin, Finch, and libpurple development headquarters
 </subject>
 <body xmlns="jabber:client">
  rekkanoryo has set the subject to: Pidgin, Finch, and libpurple development headquarters
 </body>
</message>

I tried to get the nick from the body element, but I'm intermittently getting the muc.incomingMessage and participant errors. In fact when I remove the code to get the nick from the body and simply add dumps I see those errors. When I remove both the dumps and the code to get the nick from the body, it works fine.

* I need to find a better way to test this since joining [e-mail] again and again will probably irritate them and cause them to kick me eventually. Also I'm using my gtalk account (because I'm not able to connect to conference.pidgin.im with my ch3kr.de and zsim.de accounts) which has a lot of messages in the debug log from Gtalk connections and is making debugging this a bit tricky.
So the question is a) are there any tests suites that I can use to test this instead of joining real life MUC channels ?

Comment 5

[Patrick Cloke [:clokep]]

(In reply to Saurabh Anand [:sawrubh] from comment #4)
> I added the early return and it has fixed things. However I see the message
> of the form 'The topic for <channel name> is %s'. I wanted it to be of the
> form '%s has set the topic to %s'.
If you call setTopic [1] with both a new topic and a new setter then this should all be handled magically for you (the matching strings are at [2]). If you call both setTopic AND display the body you'll end up with two messages.

> The problems I'm facing right now :
> * I was relying on the server sending a message stanza with the 'from'
> attribute having the nick of the person who set the topic and thereby being
> able to show the message of the form '%s has set the topic to %s'.
Is this expected? It looks like the stanza from [e-mail] has the from as the conference itself.

> I tried to get the nick from the body element, but I'm intermittently
> getting the muc.incomingMessage and participant errors. In fact when I
> remove the code to get the nick from the body and simply add dumps I see
> those errors. When I remove both the dumps and the code to get the nick from
> the body, it works fine.
Are you trying to pull the nick out of the first word of the body or something? This sounds dangerous to me and we shouldn't do it. Can you show us a WIP that attempts to do this? And tell us the errors you're getting?

> * I need to find a better way to test this since joining
> [e-mail] again and again will probably irritate them and
> cause them to kick me eventually.
You can write an xpcshell test to test this pretty easily.

> Also I'm using my gtalk account (because
> I'm not able to connect to conference.pidgin.im with my ch3kr.de and zsim.de
> accounts) which has a lot of messages in the debug log from Gtalk
> connections and is making debugging this a bit tricky.
Don't auto-join the room and clear the Error Console before joining. If you mean the "Copy Debug Log" feature, yes...that's missing a couple of features.

I hope that this clears things up and you have a way forward now.

[1] https://mxr.mozilla.org/comm-central/source/chat/modules/jsProtoHelper.jsm#539
[2] https://mxr.mozilla.org/comm-central/source/chat/locales/en-US/conversations.properties#46

Comment 6

[Saurabh Anand [:sawrubh]]

I will not be able to take a look at this before December so unassigning myself.

Comment 7

[aleth [:aleth]]

This spec is relevant: http://xmpp.org/extensions/xep-0045.html

Comment 8

[Harshit [:harshit]]

I should have unassigned myself. Sorry about that. I got busy with the mid-semester exams. I will resume once they get over.

Comment 9

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: rev 1 - Implement setting the topic

Comment 10

[aleth [:aleth]]

Comment on attachment 8616383 [details] [diff] [review]
rev 1 - Implement setting the topic

Review of attachment 8616383 [details] [diff] [review]:
-----------------------------------------------------------------

::: chat/locales/en-US/xmpp.properties
@@ +56,5 @@
>  conversation.error.joinFailedRemoteServerNotFound=Could not join the room %S as the server the room is hosted on could not be reached.
> +#   This is displayed in a conversation as an error message when user changes
> +#   topic to a room that he is not authorized.
> +#   %S is the text of the topic that failed to change to.
> +conversation.error.changeTopicFailedNotAuthorized=Could not set topic of this room to %S as you are not authorized.

I don't think %S needs to be repeated here:
"You are not authorized to set the topic of this room."

::: chat/protocols/xmpp/xmpp-commands.jsm
@@ +69,5 @@
> +    usageContext: Ci.imICommand.CMD_CONTEXT_CHAT,
> +    run: function(aMsg, aConv) {
> +      let conv = getConv(aConv);
> +      if (!conv.left)
> +        conv.topic = aMsg.trim();

No need to trim() here as the topic setter already does that.

::: chat/protocols/xmpp/xmpp.jsm
@@ +109,5 @@
> +  get topic() this._topic,
> +  set topic(aTopic) {
> +    let notAuthorized = (aError) => {
> +      // XEP-0045 (8.1): Unauthorized subject change.
> +      let from = aError.stanza.attributes["from"];

Who is "from" in this case? The user's JID? The room JID?

@@ +127,5 @@
> +    this._account.sendStanza(s, errorHandler);
> +  },
> +  get topicSettable() {
> +    if (this.left)
> +      return false;

I think you can probably ignore this.left here. If the user does try to change the topic while left, he'll get an error message just like when sending a message. Also, you'd have to notify the UI that the topicSettable value has changed.

If you do want to do the fully correct thing, the right place to take account of this.left would be in the UI, in conversation.xml (i.e. that way making the topic unsettable for *all* protocols while left=true).

@@ +128,5 @@
> +  },
> +  get topicSettable() {
> +    if (this.left)
> +      return false;
> +    return true;

Is it possible to discover if the user is authorized to set the topic?

@@ +246,5 @@
>          // XEP-0045: Service Informs New Room Owner of Success
>          // for instant and reserved rooms.
>          this.left = false;
>          this.joining = false;
> +        this.notifyObservers(this, "chat-update-topic");

Why is this needed?

@@ +254,5 @@
>      else if (codes.indexOf("110") != -1) {
>        // XEP-0045: Room exists and joined successfully.
>        this.left = false;
>        this.joining = false;
> +      this.notifyObservers(this, "chat-update-topic");

and here?

@@ +1267,5 @@
> +
> +    // Check for a subject element in the stanza and update the topic if
> +    // it exists.
> +    let subject = aStanza.getElement(["subject"]);
> +    if (subject && type == "groupchat") {

Section 7.2.16: "Note: In accordance with the core definition of XML stanzas, any message can contain a <subject/> element; only a message that contains a <subject/> but no <body/> element shall be considered a subject change for MUC purposes."

Is there a reason you don't check for this? (e.g. are there servers out there that include a body even though they shouldn't?)

@@ +1271,5 @@
> +    if (subject && type == "groupchat") {
> +      // TODO There can be multiple subject elements with different xml:lang
> +      // attributes.
> +      let muc = this._mucs.get(norm);
> +      muc.setTopic(subject.innerText);

Can you pass the second parameter too (topic setter)? See e.g. example 85.

Comment 11

[Abdelrahman Ahmed [:abdelrahman]]

(In reply to aleth [:aleth] from comment #10)
> Who is "from" in this case? The user's JID? The room JID?
The room JID. the room responds to user to inform him/her that he/she is not authorized.

> Is it possible to discover if the user is authorized to set the topic?
This is related to service discovery XEP-0128 (http://xmpp.org/extensions/xep-0128.html).
I think if I start implementation of it here (in next patch), it will help for next bugs related to that. OK?

> Why is this needed?
> and here?
Because changing left flag will affect the result of topicSettable, so I notify the UI about that.

> Is there a reason you don't check for this? (e.g. are there servers out
> there that include a body even though they shouldn't?)
Yes, that happens while testing, but I think we should ignore that because these servers does not follow the specs.
so I will add this condition if (subject) after checking of body as else if case, right?

Comment 12

[aleth [:aleth]]

(In reply to Abdelrhman Ahmed [:abdelrhman] from comment #11)
> (In reply to aleth [:aleth] from comment #10)
> > Who is "from" in this case? The user's JID? The room JID?
> The room JID. the room responds to user to inform him/her that he/she is not
> authorized.

OK, so you can just use this.name instead of having to get it from the stanza.

> > Is it possible to discover if the user is authorized to set the topic?
> This is related to service discovery XEP-0128
> (http://xmpp.org/extensions/xep-0128.html).
> I think if I start implementation of it here (in next patch), it will help
> for next bugs related to that. OK?

That's fine, just put a TODO comment there and file a followup bug. (By the way, please don't forget to go through the comments of the join/part bug and file the followups that were discussed there, so they won't be forgotten!)

> > Why is this needed?
> > and here?
> Because changing left flag will affect the result of topicSettable, so I
> notify the UI about that.

OK, so you can remove these notifications and (if you like) take care of it in conversation.xml, as discussed in the previous comment.
 
> > Is there a reason you don't check for this? (e.g. are there servers out
> > there that include a body even though they shouldn't?)
> Yes, that happens while testing, but I think we should ignore that because
> these servers does not follow the specs.
> so I will add this condition if (subject) after checking of body as else if
> case, right?

If we follow the spec precisley, does the topic get displayed at all on those servers? (i.e. is there still always a stanza that follows the spec and sets the topic?) Can you give an example room/server?

Comment 13

[Abdelrahman Ahmed [:abdelrahman]]

(In reply to aleth [:aleth] from comment #12)
> That's fine, just put a TODO comment there and file a followup bug. (By the
> way, please don't forget to go through the comments of the join/part bug and
> file the followups that were discussed there, so they won't be forgotten!)
OK, I will.

> If we follow the spec precisley, does the topic get displayed at all on
> those servers? (i.e. is there still always a stanza that follows the spec
> and sets the topic?) Can you give an example room/server?
No, there is no follow stanza fixes that, just one message with something like that
> <message xmlns="jabber:client" from="[e-mail]" to="[e-mail]/Instantbird" 
> type="groupchat">
>  <subject xmlns="jabber:client">
>   Lobby for XMPP.JP users
>  </subject>
>  <body xmlns="jabber:client">
>   xmppmaster は題を設定しました: Lobby for XMPP.JP users
>  </body>
> </message>
but to be accurate, not all rooms do that on the same server (seems that setting body for subject is optional).

Comment 14

[aleth [:aleth]]

(In reply to Abdelrhman Ahmed [:abdelrhman] from comment #13)
> No, there is no follow stanza fixes that, just one message with something
> like that
> > <message xmlns="jabber:client" from="[e-mail]" to="[e-mail]/Instantbird" 

A quick look shows xmpp.jp uses ejabberd, and it turns out they have a bug for this that was fixed 4 days ago:
https://github.com/processone/ejabberd/pull/592

Looking at the code, it seems a body was included originally to try to ensure there is a topic change message for clients that aren't clever enough to show one themselves.

But ejabberd is very widely used and we can't assume servers running it will be updated immediately when the next version is released. So I think unfortunately the right thing for us to do here is to also change the topic if there is a <body>, so as not to break topics on these servers. Please add a comment mentioning that we are breaking the spec here and this is needed for ejabberd versions before 15.06.

Needinfo'ing clokep to see if he agrees ;)

Comment 15

[Patrick Cloke [:clokep]]

(In reply to aleth [:aleth] from comment #14)
> (In reply to Abdelrhman Ahmed [:abdelrhman] from comment #13)
> > No, there is no follow stanza fixes that, just one message with something
> > like that
> > > <message xmlns="jabber:client" from="[e-mail]" to="[e-mail]/Instantbird" 
> 
> A quick look shows xmpp.jp uses ejabberd, and it turns out they have a bug
> for this that was fixed 4 days ago:
> https://github.com/processone/ejabberd/pull/592
> 
> Looking at the code, it seems a body was included originally to try to
> ensure there is a topic change message for clients that aren't clever enough
> to show one themselves.

If it was just fixed, we definitely need to be compatible with old versions.

Comment 16

[Patrick Cloke [:clokep]]

Comment on attachment 8616383 [details] [diff] [review]
rev 1 - Implement setting the topic

Review of attachment 8616383 [details] [diff] [review]:
-----------------------------------------------------------------

::: chat/locales/en-US/xmpp.properties
@@ +130,5 @@
>  # LOCALZIATION NOTE (command.*):
>  #  These are the help messages for each command.
>  command.join=join [<room@server>][/<nick>] [<password>]: Join a room, optionally providing a different nickname, or the room password.
>  command.part=part [<message>]: Leave the current room with an optional message.
> +command.topic=topic [<new topic>]: Set this room's topic.

Usually we don't include the command names here, see irc.properties.

Comment 17

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: rev 2 - Implement setting the topic

Comment 18

[aleth [:aleth]]

Comment on attachment 8616503 [details] [diff] [review]
rev 2 - Implement setting the topic

Review of attachment 8616503 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good - only a few nits left!

::: chat/protocols/xmpp/xmpp.jsm
@@ +111,5 @@
> +    let notAuthorized = (aError) => {
> +      // XEP-0045 (8.1): Unauthorized subject change.
> +      let s = aError.stanza.getElement(["subject"]);
> +      if (!s)
> +        return false;

While in general it's a good idea to check things, in this case we can do without as we don't need s for anything, and as this code is in a callback for the change topic stanza, so we already know the notAuthorized error refers to topics.

@@ +249,5 @@
>      else if (codes.indexOf("110") != -1) {
>        // XEP-0045: Room exists and joined successfully.
>        this.left = false;
>        this.joining = false;
> +      // Bug 1172350: Implement Service Discovery Extensions (XEP-0128) to obtain

add the word TODO here to make it clear this refers to an open bug ;)

@@ +1267,5 @@
> +      // XEP-0045 (7.2.16): Check for a subject element in the stanza and update
> +      // the topic if it exists.
> +      // We are breaking the spec because only a message that contains a
> +      // <subject/> but no <body/> element shall be considered a subject change
> +      // for MUC, but we do that to be compatible with is ejabberd versions

typo: "is"
we do that -> we ignore that

Comment 19

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: rev 3 - Implement setting the topic

Comment 20

[aleth [:aleth]]

Comment on attachment 8616511 [details] [diff] [review]
rev 3 - Implement setting the topic

Review of attachment 8616511 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

Comment 21

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/9615c8997cba