Closed
Bug 954960
Opened 11 years ago
Closed 10 years ago
It should be possible to create a new XMPP MUC from Instantbird
Categories
(Chat Core :: XMPP, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
1.6
People
(Reporter: florian, Assigned: abdelrahman)
References
()
Details
Attachments
(1 file, 4 obsolete files)
10.58 KB,
patch
|
aleth
:
review+
|
Details | Diff | Splinter Review |
*** Original post on bio 1528 at 2012-06-19 23:01:00 UTC *** I think joining a non existing MUC should create it if the server supports that. Here's a log of the stanzas exchanged by Psi with the server while creating a testib@conference.jabber.org MUC (and the account is instantbird@jabber.org with the resource ebeil): <presence to="testib@conference.jabber.org/testib" > <priority>5</priority> <c xmlns="http://jabber.org/protocol/caps" node="http://psi-im.org/caps" ver="caps-b75d8d2b25" ext="ca cs ep-notify html" /> <x xmlns="http://jabber.org/protocol/muc"/> </presence> <presence from="testib@conference.jabber.org/testib" to="instantbird@jabber.org/ebeil" > <priority>5</priority> <c xmlns="http://jabber.org/protocol/caps" node="http://psi-im.org/caps" ver="caps-b75d8d2b25" ext="ca cs ep-notify html" /> <x xmlns="http://jabber.org/protocol/muc#user"> <item affiliation="owner" role="moderator" /> <status code="201" /> </x> </presence> <iq type="set" to="testib@conference.jabber.org" id="aad2a" > <query xmlns="http://jabber.org/protocol/muc#owner"> <x xmlns="jabber:x:data" type="submit" /> </query> </iq> <iq from="testib@conference.jabber.org" type="result" to="instantbird@jabber.org/ebeil" id="aad2a" /> <presence from="testib@conference.jabber.org/instantbird" to="instantbird@jabber.org/ebeil" > <x xmlns="http://jabber.org/protocol/muc#user"> <item affiliation="owner" role="moderator" /> </x> </presence> I think the most interesting subset of this log is <x xmlns="jabber:x:data" type="submit" />
Comment 1•10 years ago
|
||
It's possible this already works, or at least works sometimes, for JS-XMPP.
Assignee | ||
Comment 2•10 years ago
|
||
Comment 3•10 years ago
|
||
Comment on attachment 8577794 [details] [diff] [review] rev 1 - creating a new instant XMPP MUC room Review of attachment 8577794 [details] [diff] [review]: ----------------------------------------------------------------- Missing: handling room creation errors (e.g. example 152) ::: chat/protocols/xmpp/xmpp.jsm @@ +675,5 @@ > > return rv; > }, > + > + // XEP-0045: Requests joining room if it is exist or nit: ...if it exists @@ +676,5 @@ > return rv; > }, > + > + // XEP-0045: Requests joining room if it is exist or > + // creating room if it is not exist. nit: ...if it does not exist. @@ +704,5 @@ > Stanza.node("password", null, null, password)); > } > + else { > + x = Stanza.node("x", Stanza.NS.muc, null); > + } You can get rid of the if clause: let x = Stanza.node("x", Stanza.NS.muc, null, password? Stanza.node("password", null, null, password) : null); @@ +1017,5 @@ > } > muc.writeMessage(muc.name, message, {system: true, error: true}); > return; > } > + else if (muc.left && x && x.uri == Stanza.NS.muc_user) { Shouldn't the namespace check be earlier in the code? It doesn't seem to have anything to do with muc.left. Don't other muc presence stanzas also have this namespace? @@ +1019,5 @@ > return; > } > + else if (muc.left && x && x.uri == Stanza.NS.muc_user) { > + // XEP-0045 (10.1) Creating room. > + // Service Acknowledges Room Creation. Should this comment go under the if (code == 201)? i.e. are there any presence stanzas with that namespace that have something to do with room creation but don't have that code? @@ +1024,5 @@ > + let status = x.getElements(["status"]); > + let code; > + for (let i in status) { > + code = status[i].attributes["code"]; > + if (code == 201) { You can simplify this a lot. let codes = x.getElements(["status"]).map(elt => elt.attributes["code"]); if (codes.indexOf("201") != -1) ... @@ +1027,5 @@ > + code = status[i].attributes["code"]; > + if (code == 201) { > + // Room is created and awaiting configuration. > + // XEP-0045 (10.1.2): Instant room. > + let x = Stanza.node("x", Stanza.NS.xdata, {type: "submit"}); Nit: Don't override an existing variable. @@ +1031,5 @@ > + let x = Stanza.node("x", Stanza.NS.xdata, {type: "submit"}); > + let query = Stanza.node("query", Stanza.NS.muc_owner, null, x); > + let s = Stanza.iq("set", null, jid, query); > + this.sendStanza(s, aStanzaReceived => { > + if (aStanzaReceived.attributes["type"] == "result") { Use ! to return early and reduce indentation. Does the spec mention any other possible responses to the iq stanza we sent? e.g. type==error? @@ +1035,5 @@ > + if (aStanzaReceived.attributes["type"] == "result") { > + // XEP-0045: Service Informs New Room Owner of Success > + // for instant and reserved rooms. > + muc.left = false; > + muc.joining = false; That's OK, but it means you should move the earlier joining=false in the same function. @@ +1036,5 @@ > + // XEP-0045: Service Informs New Room Owner of Success > + // for instant and reserved rooms. > + muc.left = false; > + muc.joining = false; > + muc.onPresenceStanza(aStanza); This isn't a presence stanza. @@ +1044,5 @@ > + }); > + break; > + } > + } > + return; So if the room wasn't created we return and ignore the stanza? That seems wrong.
Attachment #8577794 -
Flags: review?(aleth) → review-
Assignee | ||
Comment 4•10 years ago
|
||
Attachment #8577794 -
Attachment is obsolete: true
Attachment #8579479 -
Flags: review?(aleth)
Comment 5•10 years ago
|
||
Comment on attachment 8579479 [details] [diff] [review] rev 2 - creating a new instant XMPP MUC room Review of attachment 8579479 [details] [diff] [review]: ----------------------------------------------------------------- ::: chat/locales/en-US/xmpp.properties @@ +44,5 @@ > # fails. > # %S is the name of the MUC. > conversation.error.joinFailed=Could not join: %S > conversation.error.joinFailedNotAuthorized=Registration required: You are not authorized to join this room. > +conversation.error.creationFailedNotAllowed=Access Restricted: You are not allowed to create room. nit: ...create rooms. ::: chat/protocols/xmpp/xmpp.jsm @@ +991,5 @@ > } > else if (this._buddies.has(jid)) > this._buddies.get(jid).onPresenceStanza(aStanza); > else if (this._mucs.has(jid)) { > let muc = this._mucs.get(jid); Let's move all this muc presence stanza handling into one place, i.e. like for buddies do this._mucs.get(jid).onPresenceStanza(aStanza); I don't see a good reason to keep it split into two parts. I would also encourage you again to get an idea of the different kinds of presence stanza a muc can receive according to the spec, as this will help you organise the code better. That includes the user joining and leaving and other participants joining and leaving... Not all of this has to be taken care of in this bug, but if you know what is needed in principle you can write the code so it can grow easily later. @@ +1010,5 @@ > message = _("conversation.error.joinFailed", muc.name); > this.ERROR("Failed to join MUC: " + aStanza.convertToString()); > break; > } > muc.writeMessage(muc.name, message, {system: true, error: true}); You have to keep the joining=false here as if we receive this kind of error, the joining process is over. @@ +1013,5 @@ > } > muc.writeMessage(muc.name, message, {system: true, error: true}); > return; > } > + if (x && x.uri == Stanza.NS.muc_user) { Are there any possible MUC presence stanzas that *don't* have an x element or *don't* have this namespace? i.e. should you this.ERROR if this if clause is not true? @@ +1014,5 @@ > muc.writeMessage(muc.name, message, {system: true, error: true}); > return; > } > + if (x && x.uri == Stanza.NS.muc_user) { > + if (muc.left) { Wouldn't it be better to structure this the other way? if (201) { if (!muc.left) {this.WARN("That's funny, received a 201 for a room we are already in");} ... } @@ +1040,5 @@ > + } > + } > + } > + if (setLeft) > + muc.left = false; We don't have to change left unless the stanza adds or removes the user from the room participant list. If the room already exists, I think that's code 110 for joining a room?
Attachment #8579479 -
Flags: review?(aleth) → review-
Assignee | ||
Comment 6•10 years ago
|
||
(In reply to aleth [:aleth] from comment #5) > Are there any possible MUC presence stanzas that *don't* have an x element > or *don't* have this namespace? i.e. should you this.ERROR if this if clause > is not true? After checking spec, it seems no for x element and namespace. > Wouldn't it be better to structure this the other way? > > if (201) { if (!muc.left) {this.WARN("That's funny, received a 201 for a > room we are already in");} ... } I think no need for checking that because this won't happen according to spec exmaples. > We don't have to change left unless the stanza adds or removes the user from > the room participant list. > > If the room already exists, I think that's code 110 for joining a room? yes, code 110 is for joining an existing room.
Attachment #8579479 -
Attachment is obsolete: true
Attachment #8580290 -
Flags: review?(aleth)
Comment 7•10 years ago
|
||
Comment on attachment 8580290 [details] [diff] [review] rev 3 - creating a new instant XMPP MUC room Review of attachment 8580290 [details] [diff] [review]: ----------------------------------------------------------------- This is a huge improvement! ::: chat/protocols/xmpp/xmpp.jsm @@ +113,5 @@ > let nick = this._account._parseJID(from).resource; > + let jid = this._account.normalize(from); > + let x = aStanza.getElement(["x"]); > + > + // The join failed. "Check if the join failed." @@ +133,5 @@ > + this.writeMessage(this.name, message, {system: true, error: true}); > + this.joining = false; > + return; > + } > + if (!x) { Nit: insert an empty line above this as this check is not related to the join error handling. Did you also want to check the namespace here? @@ +134,5 @@ > + this.joining = false; > + return; > + } > + if (!x) { > + this.ERROR("x element does not exist in received aStanza."); Let's make this a WARN("Received a MUC presence stanza without an x element.") as it's not an error in our code or something we don't know how to recover from. @@ +138,5 @@ > + this.ERROR("x element does not exist in received aStanza."); > + return; > + } > + > + let codes = x.getElements(["status"]).map(elt => elt.attributes["code"]); Nit: Let's move the empty line from above this line to below this line, as then it's clear why we need an x element. And the codes = doesn't belong only to the 'unavailable' block. @@ +153,5 @@ > this.notifyObservers(new nsSimpleEnumerator([nickString]), > "chat-buddy-remove"); > + if ((codes.indexOf("301") != -1 || codes.indexOf("307") != -1 || > + codes.indexOf("321") != -1 || codes.indexOf("322") != -1 || > + codes.indexOf("332") != -1) && nick == this._account._jid.node) { Move the nick == check to the beginning of the if clause, as that means all the code checks won't have to be evaluated if it is false. Is there a situation where the user goes unavailable, none of those codes appear, and we *don't* want to mark the room as left? @@ +155,5 @@ > + if ((codes.indexOf("301") != -1 || codes.indexOf("307") != -1 || > + codes.indexOf("321") != -1 || codes.indexOf("322") != -1 || > + codes.indexOf("332") != -1) && nick == this._account._jid.node) { > + // XEP-0045: User is informed that he or she is being > + // banned or removed or kicked from MUC room. Please add a TODO here to add an appropriate system message telling the user what happened when it is needed, in a followup bug. @@ +162,3 @@ > return; > } > + if (codes.indexOf("201") != -1) { Empty line before this block. @@ +1057,2 @@ > else if (jid != this.normalize(this._connection._jid.jid)) > this.WARN("received presence stanza for unknown buddy " + from); Can we add an "else this.WARN("Unhandled presence stanza.")" at the end here?
Attachment #8580290 -
Flags: review?(aleth) → review-
Assignee | ||
Comment 8•10 years ago
|
||
(In reply to aleth [:aleth] from comment #7) > Did you also want to check the namespace here? Actually, most of MUC presence stanzas use Stanza.NS.muc_user namespace but some cases do not use this namespace in x element so I didn't add it to check because it may break cases like adding/updating participants with namespace (vcard-temp:x:update) and through checks in code we determine which case we will work on away from namespaces. > Is there a situation where the user goes unavailable, none of those codes > appear, and we *don't* want to mark the room as left? with these 3 statements together, I think no. but there is a case of unavailable has code 303 and this for updating nickname of user, so I added that in this patch. also, I used different way from previous patch to determine user is no longer an occupant using role attribute in item element (see example 82 and 2 lines after it in spec). > the 'role' attribute MUST be set to a value of "none" to denote that the > individual is no longer an occupant. > Can we add an "else this.WARN("Unhandled presence stanza.")" at the end here? I think no problem and this will help us later know missing features.
Attachment #8580290 -
Attachment is obsolete: true
Attachment #8580688 -
Flags: review?(aleth)
Comment 9•10 years ago
|
||
Comment on attachment 8580688 [details] [diff] [review] rev 4 - creating a new instant XMPP MUC room Review of attachment 8580688 [details] [diff] [review]: ----------------------------------------------------------------- Looks like it's almost ready :-) >Actually, most of MUC presence stanzas use Stanza.NS.muc_user namespace but >some cases do not use this namespace in x element so I didn't add it to check >because it may break cases like adding/updating participants with namespace >(vcard-temp:x:update) This sounds like you should check for Stanza.NS.muc_user and if it's not, this.warn("Received a MUC presence stanza with a namespace we don't handle yet: " + namespace) and return. You're right that vcard-temp:x:update presence stanzas exist, but I don't think we actually handle them for MUCs so far? Do they even get sent for MUC participants? ::: chat/protocols/xmpp/xmpp.jsm @@ +112,5 @@ > let from = aStanza.attributes["from"]; > let nick = this._account._parseJID(from).resource; > + let jid = this._account.normalize(from); > + let x = aStanza.getElement(["x"]); > + let item = x.getElement(["item"]); This has to go under the if (!x) check. @@ +151,5 @@ > + if (codes.indexOf("303") != -1) { > + // XEP-0045 (7.6): Changing Nickname. > + // Service Updates Nick for user. > + if (!item || !item.attributes["nick"]) { > + this.WARN("Received a MUC presence stanza without an item element or a nick attribute."); ...MUC presence code 303 stanza... @@ +168,5 @@ > + this.notifyObservers(new nsSimpleEnumerator([nickString]), > + "chat-buddy-remove"); > + if (nick == this._account._jid.node) { > + this.left = true; > + } Nit: no need for {...} any more @@ +171,5 @@ > + this.left = true; > + } > + // TODO: Add an appropriate system message telling the > + // user what happened (banned or removed or kicked from > + // MUC room) using codes. Don't forget to file a bug for this ;) @@ +172,5 @@ > + } > + // TODO: Add an appropriate system message telling the > + // user what happened (banned or removed or kicked from > + // MUC room) using codes. > + } This looks like it needs an else WARN("Unhandled type==unavailable MUC presence stanza") ?
Attachment #8580688 -
Flags: review?(aleth) → review-
Comment 10•10 years ago
|
||
Comment on attachment 8580688 [details] [diff] [review] rev 4 - creating a new instant XMPP MUC room Review of attachment 8580688 [details] [diff] [review]: ----------------------------------------------------------------- ::: chat/protocols/xmpp/xmpp.jsm @@ +166,5 @@ > + .createInstance(Ci.nsISupportsString); > + nickString.data = nick; > + this.notifyObservers(new nsSimpleEnumerator([nickString]), > + "chat-buddy-remove"); > + if (nick == this._account._jid.node) { Shouldn't we use code==110 for this (section 7.14)? The nick of the user in the chatroom does not have to match the account username, iirc.
Assignee | ||
Comment 11•10 years ago
|
||
(In reply to aleth [:aleth] from comment #9) > This sounds like you should check for Stanza.NS.muc_user and if it's not, > this.warn("Received a MUC presence stanza with a namespace we don't handle > yet: " + namespace) and return. You're right that vcard-temp:x:update > presence stanzas exist, but I don't think we actually handle them for MUCs > so far? Do they even get sent for MUC participants? After checking messages, it seems some clients add multiple x elements in presence stanza like this case > <x xmlns="vcard-temp:x:update"> > <photo xmlns="vcard-temp:x:update"/> > </x> > <x xmlns="http://jabber.org/protocol/muc#user"> > <item xmlns="http://jabber.org/protocol/muc#user" affiliation="owner" role="moderator"/> > </x> previous patch catches first match and that was wrong but in this patch is updated to catch x element with required namespace. (In reply to aleth [:aleth] from comment #10) > Shouldn't we use code==110 for this (section 7.14)? The nick of the user in > the chatroom does not have to match the account username, iirc. yes, you are right.
Attachment #8580688 -
Attachment is obsolete: true
Attachment #8581247 -
Flags: review?(aleth)
Comment 12•10 years ago
|
||
Comment on attachment 8581247 [details] [diff] [review] rev 5 - creating a new instant XMPP MUC room Review of attachment 8581247 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for this big improvement of the MUC code! ::: chat/protocols/xmpp/xmpp.jsm @@ +180,5 @@ > + // MUC room) using codes. > + } > + else { > + this.WARN("Unhandled type==unavailable MUC presence stanza."); > + } Nit: no {...} needed I can fix that on checkin.
Attachment #8581247 -
Flags: review?(aleth) → review+
Updated•10 years ago
|
Severity: enhancement → normal
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.6
You need to log in
before you can comment on or make changes to this bug.
Description
•