tst2005googlecode2 / lxmppd

Automatically exported from code.google.com/p/lxmppd
0 stars 0 forks source link

Unable to create users or rooms using smack library #328

Open GoogleCodeExporter opened 9 years ago

GoogleCodeExporter commented 9 years ago
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?

Original issue reported on code.google.com by stevepug...@gmail.com on 3 Feb 2013 at 2:53

GoogleCodeExporter commented 9 years ago
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?

Original comment by cbs...@gmail.com on 16 Apr 2013 at 1:18

GoogleCodeExporter commented 9 years ago
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?

Original comment by cbs...@gmail.com on 17 Apr 2013 at 2:08

Attachments:

GoogleCodeExporter commented 9 years ago
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).

Original comment by MWild1 on 18 Apr 2013 at 12:27

GoogleCodeExporter commented 9 years ago
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.

Original comment by cbs...@gmail.com on 18 Apr 2013 at 6:25

GoogleCodeExporter commented 9 years ago
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 :)

Original comment by MWild1 on 18 Apr 2013 at 7:53

GoogleCodeExporter commented 9 years ago
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

Original comment by cbs...@gmail.com on 20 Apr 2013 at 6:33

GoogleCodeExporter commented 9 years ago
> 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.

Original comment by fschm...@gmail.com on 15 Apr 2014 at 2:49

GoogleCodeExporter commented 9 years ago
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.

Original comment by waqas20 on 15 Apr 2014 at 8:53

GoogleCodeExporter commented 9 years ago
> 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.

Original comment by fschm...@gmail.com on 16 Apr 2014 at 8:31

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

Original comment by q...@daurnimator.com on 6 Sep 2014 at 10:34