*** Original post on bio 1874 by Mikael Nordfeldth <mmn AT hethane.se> at 2013-02-08 09:39:00 UTC *** Copy-paste from my entry on the Mozilla bug tracker, https://bugzilla.mozilla.org/show_bug.cgi?id=839421 User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:18.0) Gecko/20100101 Firefox/18.0 Build ID: 20130201185534 Steps to reproduce: 1. Start two instances of Thunderbird (I did it on separate computers) 2. Add the same XMPP account on both installations. 3. See the clients battle over which default XMPP resource "Thunderbird" should be the active one. Actual results: Thunderbird logs in to the XMPP server with identical resource names from two connections, which in XMPP means that the users wishes to replace the previous connection for that resource (in this case "Thunderbird"). Since both clients think they're the active one, an eternal battle of the resource ensues. Expected results: This is the proper behaviour whenever assigning a specific resource for one's client. However, the default behaviour should be to generate a random string to use as Resource (long enough to avoid collisions). * The resource field should be empty by default. * Whenever the resource field is empty, a new resource name should be generated (preferrably on client startup, NOT mere network reconnects, preferrably NOT only on software installation) * The resource field should be put under some sort of "Advanced" setting form. Resources are possibly a security risk (defaulting to "Thunderbird" can let people look for Thunderbird-specific vulnerabilities for example). A known resource ID for a specific client may also be a security risk if it never changes. Thus a resource should be [pseudo]randomly generated for every session (like when Thunderbird starts). The resource is there to reconnect conversations on temporary network failures. On bigger breaks, like turning the computer off or closing Thunderbird, the client may assume the user considers the current instant messaging conversations over.

*** Original post on bio 1874 at 2013-02-08 10:58:45 UTC *** http://xmpp.org/rfcs/rfc3921.html#session describes what should be done in case of resource collision. We need to handle either: <stream:error> <conflict xmlns='urn:ietf:params:xml:ns:xmpp-streams'/> </stream:error> </stream:stream> If the server decides to close the first connection, or: <iq from='example.com' type='error' id='sess_1'> <session xmlns='urn:ietf:params:xml:ns:xmpp-session'/> <error type='cancel'> <conflict xmlns='urn:ietf:params:xml:ns:xmpp-stanzas'/> </error> </iq> or the server refuses the second condition during resource binding.

*** Original post on bio 1874 by Mikael Nordfeldth <mmn AT hethane.se> at 2013-02-08 11:06:24 UTC *** Just to drag my comments insisting on random resources to a more static forum than IRC: "I think this is most easily, and true to spec, resolved by not replacing an empty Resource and then accepting the new resource name from the server" (current procedure is to replace empty Resource with "Thunderbird" in the Thunderbird case) I believe only people who know what a Resource is (aware you can set it to "Home", "Work" or whatever) should have a static resource. So if the string is empty by default, no collision problems will occur - and a user who desires specific resources can configure their clients appropriately. (XMPP spec says server will generate resource name if empty from client, http://xmpp.org/rfcs/rfc3920.html#bind - so forget that about the client generating an ID by session etc.)

*** Original post on bio 1874 at 2013-02-08 11:52:32 UTC *** (In reply to comment #2) > "I think this is most easily, and true to spec, resolved by not replacing an > empty Resource and then accepting the new resource name from the server" I understand your point (ie I understand that this behavior would make sense from your point of view), but I prefer keeping the client name as the default resource. Knowing which client is used is useful when talking to someone (for example if I talk to friends who frequently used gmail, Adium, Instantbird, and other clients for the same account, if I see a bug in our interactions, it's interesting for me to know if the problematic message came from Instantbird without having to ask the contact about which client is used).

*** Original post on bio 1874 by Mikael Nordfeldth <mmn AT hethane.se> at 2013-02-08 14:28:55 UTC *** (In reply to comment #3) > I understand your point (ie I understand that this behavior would make sense > from your point of view), but I prefer keeping the client name as the default > resource. Knowing which client is used is useful when talking to someone (for > example if I talk to friends who frequently used gmail, Adium, Instantbird, and > other clients for the same account, if I see a bug in our interactions, it's > interesting for me to know if the problematic message came from Instantbird > without having to ask the contact about which client is used). I'm afraid I couldn't find it immediatly on a search engine, but I'm reasonably certain there's a specification/XEP for how to ask a client itself through stanzas what software version it is running (in case it wants to share that info) ;) But I assume we're on the same page regarding what the actual issue is here (conflicting resource names causing for-average-Joe-confusing disconnects) and won't further argue about things irrelevant to that. I'll help out anyway I can with testing whatever patches come up here and will be in touch on IRC regarding learning how to perhaps patch it myself.

*** Original post on bio 1874 by Kim Alvefur <zash AT zash.se> at 2013-02-08 19:43:37 UTC *** (In reply to comment #1) > http://xmpp.org/rfcs/rfc3921.html#session describes what should be done in case > of resource collision. You should be reading RFC 6120 and 6121. > We need to handle either: > <stream:error> > <conflict xmlns='urn:ietf:params:xml:ns:xmpp-streams'/> > </stream:error> > </stream:stream> > > If the server decides to close the first connection, or: > > <iq from='example.com' type='error' id='sess_1'> > <session xmlns='urn:ietf:params:xml:ns:xmpp-session'/> > <error type='cancel'> > <conflict xmlns='urn:ietf:params:xml:ns:xmpp-stanzas'/> > </error> > </iq> > > or the server refuses the second condition during resource binding. <session/> can be skipped on modern servers, and it was removed in RFC 6121. http://tools.ietf.org/html/rfc6120#section-7 is what is relevant here. IMO, resources are not really something that normal users should need to ever see. It can be hidden away under 'Advanced' somewhere for power-users. The behavior that I recommend as default is: On first connect, let the server assign a resource (See RFC 6120 section 7.6). If the connection is lost and a reconnect is initiated, request the same resource. As most servers would terminate the old (already dead) session in this case, this helps avoid ghost sessions. (See RFC 6120 Section 7.7.2.2) (In reply to comment #4) > I'm afraid I couldn't find it immediatly on a search engine, but I'm reasonably > certain there's a specification/XEP for how to ask a client itself through > stanzas what software version it is running (in case it wants to share that > info) ;) This: http://xmpp.org/extensions/xep-0092.html

According to IRC discussion (http://log.bezut.info/instantbird/160628/#m319) > After reading these sections, I think the behavior should be like that if we do > not have the resourcepart, we will ask the server to generate it, otherwise try > to bind it and if we failed ask server to generate it (in retry), also of > course handle error cases The RFC is 6120 and sections are 7.5-7.7

(In reply to Florian Quèze [:florian] [:flo] from comment #3) > > "I think this is most easily, and true to spec, resolved by not replacing an > > empty Resource and then accepting the new resource name from the server" > > I understand your point (ie I understand that this behavior would make sense > from your point of view), but I prefer keeping the client name as the > default resource. Knowing which client is used is useful when talking to > someone (for example if I talk to friends who frequently used gmail, Adium, > Instantbird, and other clients for the same account, if I see a bug in our > interactions, it's interesting for me to know if the problematic message > came from Instantbird without having to ask the contact about which client > is used). For this we can add a /version command now js-xmpp supports sending version data.

Comment on attachment 8767319 [details] [diff] [review] v1 - Handle resource collision Review of attachment 8767319 [details] [diff] [review]: ----------------------------------------------------------------- Doesn't this new logic also require changes to the connection code so we don't try to set the resource if the user hasn't set the pref? ::: chat/protocols/xmpp/xmpp-session.jsm @@ +485,5 @@ > }, > bindResult: function(aStanza) { > + if (aStanza.attributes["type"] == "error") { > + let error = this._account.parseError(aStanza); > + if (error.condition == "conflict") { This could be inside the switch statement @@ +490,5 @@ > + // RFC 6120 (7.7.2.2): Conflict. > + // The provided resourcepart is already in use and the server > + // disallowed the resource binding attempt. > + > + // Request a server-generated resourcepart. Move this comment to _bindRequest.

Comment on attachment 8768089 [details] [diff] [review] v2 - Handle resource collision Review of attachment 8768089 [details] [diff] [review]: ----------------------------------------------------------------- ::: chat/locales/en-US/xmpp.properties @@ +33,5 @@ > connection.error.notSendingPasswordInClear=The server only supports authentication by sending the password in cleartext > connection.error.authenticationFailure=Authentication failure > connection.error.notAuthorized=Not authorized (Did you enter the wrong password?) > connection.error.failedToGetAResource=Failed to get a resource > +connection.error.failedMaxResourceLimit=This account has reached a limit on the number of simultaneous connected resources allowed. Does this just mean "This account is connected from too many places"? If so, can we simplify the wording? @@ +34,5 @@ > connection.error.authenticationFailure=Authentication failure > connection.error.notAuthorized=Not authorized (Did you enter the wrong password?) > connection.error.failedToGetAResource=Failed to get a resource > +connection.error.failedMaxResourceLimit=This account has reached a limit on the number of simultaneous connected resources allowed. > +connection.error.failedResourceBindNotAllowed=This server does not allow to bind a resource to the stream. I don't understand this message. Do we know in which case this can happen?

Comment on attachment 8768089 [details] [diff] [review] v2 - Handle resource collision Review of attachment 8768089 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, but I haven't tested it yet. gtalk.js will also require changes. ::: chat/protocols/xmpp/xmpp-session.jsm @@ +43,5 @@ > this._domain = aJID.domain; > this._password = aPassword; > this._account = aAccount; > > + this._resource = aJID.resource; Move this up to below this._domain @@ +586,5 @@ > + > + // If the resource is empty, we will fallback to XMPPDefaultResource > + // (set to brandShortName) as resource is REQUIRED. > + Stanza.node("resource", null, null, > + this._resource ? this._resource : XMPPDefaultResource) Shouldn't you actually set this._resource here if it's empty/null? Or does it get set later?

(In reply to aleth [:aleth] from comment #12) > Comment on attachment 8768089 [details] [diff] [review] > v2 - Handle resource collision > > gtalk.js will also require changes. and odoklassniki (and others?) > Shouldn't you actually set this._resource here if it's empty/null? Or does > it get set later? Also check _jid is set correctly.

(In reply to Florian Quèze [:florian] [:flo] from comment #11) > Comment on attachment 8768089 [details] [diff] [review] > v2 - Handle resource collision > > Review of attachment 8768089 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: chat/locales/en-US/xmpp.properties > @@ +33,5 @@ > > connection.error.notSendingPasswordInClear=The server only supports authentication by sending the password in cleartext > > connection.error.authenticationFailure=Authentication failure > > connection.error.notAuthorized=Not authorized (Did you enter the wrong password?) > > connection.error.failedToGetAResource=Failed to get a resource > > +connection.error.failedMaxResourceLimit=This account has reached a limit on the number of simultaneous connected resources allowed. > > Does this just mean "This account is connected from too many places"? If so, > can we simplify the wording? OK > @@ +34,5 @@ > > connection.error.authenticationFailure=Authentication failure > > connection.error.notAuthorized=Not authorized (Did you enter the wrong password?) > > connection.error.failedToGetAResource=Failed to get a resource > > +connection.error.failedMaxResourceLimit=This account has reached a limit on the number of simultaneous connected resources allowed. > > +connection.error.failedResourceBindNotAllowed=This server does not allow to bind a resource to the stream. > > I don't understand this message. Do we know in which case this can happen? RFC 6120 section 7.6.2 (Error Cases), but I'm not sure how this can happen.

(In reply to aleth [:aleth] from comment #12) > Shouldn't you actually set this._resource here if it's empty/null? Or does > it get set later? Yes, you are right.

(In reply to aleth [:aleth] from comment #13) > (In reply to aleth [:aleth] from comment #12) > > Comment on attachment 8768089 [details] [diff] [review] > > v2 - Handle resource collision > > > > gtalk.js will also require changes. > > and odoklassniki (and others?) It does not need (https://dxr.mozilla.org/comm-central/search?q=resource%3A+%7Bget+label()&redirect=false) I removed |failedResourceBindNotAllowed| message according to this IRC discussion (http://log.bezut.info/instantbird/160705/#m199)

Comment on attachment 8768385 [details] [diff] [review] v3 - Handle resource collision Review of attachment 8768385 [details] [diff] [review]: ----------------------------------------------------------------- This may need changes https://dxr.mozilla.org/comm-central/source/chat/protocols/gtalk/gtalk.js#43 similarly for odoklassniki ::: chat/locales/en-US/xmpp.properties @@ +33,5 @@ > connection.error.notSendingPasswordInClear=The server only supports authentication by sending the password in cleartext > connection.error.authenticationFailure=Authentication failure > connection.error.notAuthorized=Not authorized (Did you enter the wrong password?) > connection.error.failedToGetAResource=Failed to get a resource > +connection.error.failedMaxResourceLimit=This account is connected from too many places. ...too many places at the same time. ::: chat/protocols/xmpp/xmpp-session.jsm @@ +519,5 @@ > return; > } > jid = jid.innerText; > this.DEBUG("jid = " + jid); > this._jid = this._account._parseJID(jid); Should update this._resource here too ::: chat/protocols/xmpp/xmpp.jsm @@ +1239,5 @@ > + if (this._jid.node) > + jid = this._jid.node + "@" + jid; > + if (this._jid.resource) > + jid += "/" + this._jid.resource; > + this._jid.jid = jid; Whenever you copypaste code, it should probably be a helper function. Like _parseJid (_setJID(aNode =..., aDomain =..., aResource=...))

Comment on attachment 8769362 [details] [diff] [review] v4 - Handle resource collision Review of attachment 8769362 [details] [diff] [review]: ----------------------------------------------------------------- ::: chat/protocols/xmpp/xmpp.jsm @@ +2088,5 @@ > }, > > + // Constructs jid as a string from domain, node and resource parts. > + // aDomain is required, but aNode and aResource are optional. > + _setJID: function(aDomain, aNode = null, aResource = null) { _parseJID should also use this method for consistency and deduplication. @@ +2097,5 @@ > + if (aNode) > + jid = aNode.toLowerCase() + "@" + jid; > + if (aResource) > + jid += "/" + aResource; > + return jid; It might simplify the code if this returned a jid object instead of just the string. It has all the data needed to make one.

Comment on attachment 8769385 [details] [diff] [review] v5 - Handle resource collision Review of attachment 8769385 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, thanks! Two things to test before landing. ::: chat/protocols/gtalk/gtalk.js @@ +62,1 @@ > } Here too, need to check gtalk's servers actually support resource generation. (They should, but gtalk has been nonstandard before...) ::: chat/protocols/odnoklassniki/odnoklassniki.js @@ +23,5 @@ > if (!this.name.includes("@")) { > + let resource; > + if (this.prefs.prefHasUserValue("resource")) > + resource = this.getString("resource"); > + this._jid = this._setJID("odnoklassniki.ru", this.name, resource); Please check if this server supports resource generation (it should, but lets not break things)

(In reply to aleth [:aleth] from comment #21) > Comment on attachment 8769385 [details] [diff] [review] > v5 - Handle resource collision > > Review of attachment 8769385 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks good, thanks! > > Two things to test before landing. > > ::: chat/protocols/gtalk/gtalk.js > @@ +62,1 @@ > > } > > Here too, need to check gtalk's servers actually support resource > generation. (They should, but gtalk has been nonstandard before...) Could you confirm if this patch is working with gtalk?

(In reply to Abdelrhman Ahmed [:abdelrhman] from comment #22) > (In reply to aleth [:aleth] from comment #21) > > Comment on attachment 8769385 [details] [diff] [review] > > v5 - Handle resource collision > > > > Review of attachment 8769385 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > Looks good, thanks! > > > > Two things to test before landing. > > > > ::: chat/protocols/gtalk/gtalk.js > > @@ +62,1 @@ > > > } > > > > Here too, need to check gtalk's servers actually support resource > > generation. (They should, but gtalk has been nonstandard before...) > > Could you confirm if this patch is working with gtalk? I didn't notice anything broken. There were a few warnings, but they seemed unrelated.

(In reply to aleth [:aleth] from comment #21) > ::: chat/protocols/odnoklassniki/odnoklassniki.js > @@ +23,5 @@ > > if (!this.name.includes("@")) { > > + let resource; > > + if (this.prefs.prefHasUserValue("resource")) > > + resource = this.getString("resource"); > > + this._jid = this._setJID("odnoklassniki.ru", this.name, resource); > > Please check if this server supports resource generation (it should, but > lets not break things) I couldn't test it, so I added a todo instead.