#328 Unable to create users or rooms using smack library

Reporter stevepugh99
Owner MattJ
Created
Updated
Stars ★★ (3)
Tags
  • Status-Fixed
  • Milestone-0.11
  • Type-Defect
  • MUC
  • Priority-Medium
  1. stevepugh99 on

    *What steps will reproduce the problem?* 1. Create a fresh prosody install 2. Create a simple program using the "smack" xmpp library e.g: ConnectionConfiguration config = new ConnectionConfiguration(HOST, PORT); connection.connect(); connection.login(username, password); then create new user with: connection.getAccountManager().createAccount(newuser, newpass); or create new room with: MultiUserChat muc = new MultiUserChat(connection, "room@conference.server.org"); muc.create("creator"); *What is the expected output? What do you see instead?* Expected: room / user create Actually see: When attempting to create user: bad-request(-1) When attempt to create room: Creation failed - Missing acknowledge of room creation. *What version of the product are you using? On what operating system?* Using prosody 0.8.2 on Ubuntu. Notes (Important!): 1. The connection for the above test is done with an admin user created using the "prosodyctl register" command. This login is successful, the point of failure happens at the user/room creation step. 2. I have tested the above smack code with ejabberd and it works fine. 3. Doing the same operations to create users and rooms via pidgin work fine on both prosody and ejabberd. So it appears to be a problem between smack and prosody. But the fact that smack works fine with ejabberd makes me thing maybe it's a prosody problem?

  2. cbs228 on

    This appears to be a standards-compliance issue with the multiuser chat (XEP-0045). As per Example 154 in http://xmpp.org/extensions/xep-0045.html#createroom-general, when a new room is created the muc#user list should include a <status code='201'/>. This status code indicates to the client that the room is new (i.e., has just been created) and that the user is the sole occupant. Prosody's mod_muc does not do this, for whatever reason, and Smack's MUC client appears to require it. In pidgin, a "room has been created" stanza from prosody looks like this: <presence to='myusername@myserver.com/pidgin' from='theroomname@muc.myserver.com/myusername'> <x xmlns='http://jabber.org/protocol/muc#user'> <item jid='myusername@myserver.com/pidgin' affiliation='owner' role='moderator'/> <status code='110'/> </x> </presence> it should have a <status code='201'/> stanza immediately following the status code 110. I believe this behavior should be fixed in prosody since it doesn't meet the XEP standard. A patch for this behavior probably wouldn't be that difficult. Can any developers comment on this ticket?

  3. cbs228 on

    Enclosed is a patch against version 1-1~nightly114 (the prosody-0.9 package from http://packages.prosody.im/debian/dists/stable/main/). This patch adds a new flag for newly-created rooms and sends the 201 status code like it should. DO NOT TRY this patch on production servers! While it seems to work, my test cases aren't the most exhaustive, and I'm not a regular prosody dev. Apply the patch with cd /usr/lib/prosody/modules/muc # or wherever mod_muc.lua is patch -p1 < /wherever/you/put/muc_diff_1-1-114.patch Restart prosody. Does it fix your smack problems? I have been trying to get gtalksms's MUC functionality to work (it uses smack). In the process, I have also noticed that the MUC room configuration form (see http://xmpp.org/extensions/xep-0045.html#createroom) does not include all the parameters that prosody supports. For example, muc#roomconfig_passwordprotectedroom (a boolean flag which is true for password-protected rooms), muc#roomconfig_roomowners, and muc#roomconfig_roomadmins are not exposed in the form. gtalksms assumes the first two parameters are available and errors when it cannot find them. I'm not sure if this should be fixed in prosody or in gtalksms. Any opinions?

    Attachments
  4. MattJ on

    Hi there, sorry - only just catching up with mail and saw your comments. At the time this issue first came to our attention we had some discussion about it in the chatroom. I'll give a summary here. Prosody's MUC component acts, as far as the protocol is concerned, as if a room already existed when you created it. As far as a client is concerned, it just happened to join an empty room and just happens to be the owner of it. One reason for this is to avoid the room locking behaviour required by XEP-0045. I'm not personally a fan of this, and I wish that it were an optional part of the specification (it was later made possible for a client to bypass it, but the default remains to lock). Since we first implemented MUC no user has complained about this missing functionality, nor requested it. Except for this Smack problem. The problem is, if we start returning 201 then we have to also implement room locking (and all the complexity it entails), and make it the default behaviour for room creation. Otherwise we *will* be breaking the spec, and many more clients will get very confused. Looking at it from the other side, a library having an API to create a room is somewhat silly - from the client side the protocol for creating and joining a room is exactly the same. This means that if you try to create a room in Smack that already exists, you will actually end up joining it anyway. So the mess is partly caused by XEP-0045 (which is too widespread now to receive significant changes), partly Smack's API (which is not maintained?), and partly our resistance to implementing any of the messy parts of the spec in Prosody. About the latter I could *possibly* be persuaded, but it's not going to be a high priority thing (I have a stack of MUC stuff on my todo, and I would probably add it when I started on that).

  5. cbs228 on

    The MUC XEP definitely has its shortcomings—the status codes themselves are neither self-documenting nor particularly XML-like. I do agree that the locking behavior is silly and that most users will never have cause to notice or care. gtalksms has a particularly unusual use-case: The MUC is really just a one-on-one chat. The sole purpose of the MUC is to separate out messages based on JID. gtalksms relays everything sent to the room to an SMS contact, and vice-versa. Ergo, it's really necessary to keep non-participants out of the room. There are plenty of alternate flows they could use to achieve this, however, and I'll see about getting them to fix it.

  6. MattJ on

    If you aren't familiar with the section already, take a look at: http://xmpp.org/extensions/xep-0045.html#continue There used to be a way to request a unique room name from the server (Prosody still implements it I believe). It was removed because it didn't seem much better than a client just using a UUID (and that's exactly what Prosody does anyway). But now I see it would be a handy way to implement on-demand locking :)

  7. cbs228 on

    The continue protocol does not appear to automatically configure an appropriate room for the task—that is a job for the client. Interestingly enough, Example 61 in the spec results in different output from the server than Example 153/154; the document isn't internally consistent. (Example 61 is missing the 201 creation acknowledgement despite having the exact same initial join request.) The big problem with automated applications such as gtalksms is ensuring that the room is "clean:" i.e., no other users in the room, no other users in the owner list, no other users in the admin list, no other users in the member list, and that all of this does not change in the middle of the configuration process. Checking all of this would require lots of queries. I suppose gtalksms could just use a UUID and assume everything is fine, and gtalksms does include a random int in every room name. The locking behavior, while cumbersome, would also help ensure that the room is secure. Not all clients support the locking behavior, however. This is a problem since deliberate action is required on the client's part to unlock the room. Pidgin and Empathy seem to support it, but sleekxmpp does not. The lock would need a timeout, and a short one at that. This is allowed by the spec. Still, it's a change that would require testing. I've started a separate ticket in gtalksms which has received some attention by the aSmack devs. It is here: http://code.google.com/p/gtalksms/issues/detail?id=323

  8. fschmaus on

    > So the mess is partly caused ... Smack's API (which is not maintained?) ... I'm sorry, but I fail to see what Smack is doing wrong besides sticking to the specifications of XEP-45. Or to put it in other words, what do you want me change while complying to the specification? > Otherwise we *will* be breaking the spec Am I mistaken or isn't prosody's MUC component already breaking the spec? E.g. XEP-45 10.1.1 requires '201' to be send. The room looking behavior has its use-cases, as #4 explained. Although I wouldn't mind if the looking behavior would be the optional part and the instant room the default. But frankly, even if the behavior has no use-case at all, intentionally providing an implementation that doesn't stick to an ~10 year old specification is a bad idea, as it hurts the interoperability between the various XMPP implementations. @Matthew I hope I could persuade you a bit to reconsider fixing this bug in prosody. PS.: Smack and aSmack are both well maintained. I would be happy to fix any bug, if I'm wrong and it's in fact Smack who doesn't stick to the spec.

  9. Waqas on

    One thing has changed regarding this issue: Prosody optionally implements room locking (it's a configuration option). On the Smack side of things, one thing can be fixed (it's Smack in particular which has this issue, other XMPP libraries and clients work fine). At the XMPP protocol level, joining a room and creating a room cause the exact same stanza to be sent to the server. It is then the server which decides whether the room already exists (in which case you join), or if the room doesn't exist yet (in which case it's created, you join, get a 201 response, and so on). Smack is unique in that (IIRC) it simply broke if you tried to create a room that already existed. The server happily allows you to join that room (since it already exists, and the server has no way of knowing you are creating and not joining, since they are the exact same thing at the XMPP layer). I consider that a Smack bug: It shouldn't break in this case. In the MUC XEP, the client doesn't get to choose whether it's creating a new room or joining an existing room. They are the same operation. The server gets to decide that. Smack pretends that they are different operations.

  10. fschmaus on

    > Smack is unique in that (IIRC) it simply broke if you tried to create a room that already existed. Smack expects that the MUC service will send a 201 if the room got created as specified in XEP-45 and assumes (correctly, IMHO) that the room already existed if the result presence stanza contained no 201 status code. This is all strictly holding on the spec. > and the server has no way of knowing you are creating and not joining And why would he need to know that? Nobody is expecting this, all I would expect is that a server sends 201 status code when the room got newly created. > I consider that a Smack bug: It shouldn't break in this case. I guess you are still having a scenario in mind where a user wants to join a room, regardless if it's a newly created one or an existing one. But there is a valid use case where the user wants to assure that the room got newly created and that there must be no time-frame between the creation and the submission of the configuration where some other user could join the room, read some details or does other stuff that the room configuration would prevent. This is why Smack's create() aborts if the room wasn't created in a locked state. The locking mechanism in described in XEP-45 for a good reason. Anyway, since it appears that prosody now has room locking support as least optionally the situation seems to have improved. I am also going to make some changes to Smack in order to implemented a "create or join" method for MultiUserChat (SMACK-557, http://issues.igniterealtime.org/browse/SMACK-557, https://github.com/Flowdalic/Smack/tree/muc) to support the user for the "I just want to join the room, not matter if it exists already or not" use case.

  11. quae@daurnimator.com on

    Support for room locking and the 201 code are now in prosody-trunk. Though now Smack has another issue (see #438)

  12. Zash on

    0.10 has room locking as well IIRC. It needs some testing and reconsideration of the defaults.

    Changes
    • owner MattJ
    • tags Status-Started Milestone-0.10
  13. Zash on

    Changes
    • tags MUC
  14. Valentine on

    Still same issue , I just patched the module to send 201.. and Yes I am using on quite a massive production server.

  15. Zash on

    You should try 0.10 with muc_room_locking = true and report any issues or if it works as expected.

  16. MattJ on

    Not enough feedback from anyone who has enabled muc_room_locking, pushing this to 0.11

    Changes
    • tags Milestone-0.11
  17. Zash on

    Enabled by default in https://hg.prosody.im/trunk/rev/2c60ae791bdc Please report any issues that pops up

    Changes
    • tags Status-Fixed

New comment

Not published. Used for spam prevention and optional update notifications.