#939 Trunk includes a 307 status on error presences
Reporter
Link Mauve
Owner
MattJ
Created
Updated
Stars
★★★ (4)
Tags
Type-Defect
Priority-Medium
Status-Fixed
Milestone-0.11
Link Mauve
on
What steps will reproduce the problem?
1. Join a MUC running hg:5abb6bc45edd.
2. Send <presence type="error" to="muc@muc-server"><error xmlns="jabber:client" type="cancel"><undefined-condition xmlns="urn:ietf:params:xml:ns:xmpp-stanzas" /></error></presence> or any other valid error presence.
What is the expected output? What do you see instead?
I would like to see a normal part with a kicked reason, like that:
23:10:59 <--- Link Mauve has left the room (Kicked: undefined condition)
Instead I see an actual kick:
23:10:59 -!- You have been kicked.
What version of the product are you using? On what operating system?
Prosody trunk version hg:5abb6bc45edd, on ArchLinux.
Please provide any additional information below.
The only differences are the 307 status, the inclusion of the real JID, and the preserved role.
On 0.10:
<presence type="unavailable" to="me@server/poezio" from="muc@muc-server/Link Mauve"><status>Kicked: undefined condition</status><x xmlns="http://jabber.org/protocol/muc#user"><item affiliation="owner" role="none" /><status code="110" /></x></presence>
On trunk:
<presence type="unavailable" to="me@server/poezio" from="muc@muc-server/Link Mauve"><status>Kicked: undefined condition</status><x xmlns="http://jabber.org/protocol/muc#user"><status code="307" /><item jid="me@server/poezio" affiliation="owner" role="moderator" /><status code="110" /></x></presence>
Zash
on
Could you test this patch?
diff -r 45be94611593 plugins/muc/muc.lib.lua
--- a/plugins/muc/muc.lib.lua Tue Jun 13 20:14:06 2017 +0200
+++ b/plugins/muc/muc.lib.lua Fri Jun 23 02:44:16 2017 +0200
@@ -349,8 +349,11 @@ function room_mt:handle_kickable(origin,
occupant:set_session(real_jid, st.presence({type="unavailable"})
:tag('status'):text(error_message));
self:save_occupant(occupant);
- local x = st.stanza("x", {xmlns = "http://jabber.org/protocol/muc#user";})
- :tag("status", {code = "307"})
+ local x = {
+ base = st.stanza("x", {xmlns = "http://jabber.org/protocol/muc#user";});
+ self = st.stanza("x", {xmlns = "http://jabber.org/protocol/muc#user";})
+ :tag("status", {code = "307"});
+ };
self:publicise_occupant_status(occupant, x);
if occupant.jid == real_jid then -- Was last session
module:fire_event("muc-occupant-left", {room = self; nick = occupant.nick; occupant = occupant;});
@@ -1298,7 +1301,11 @@ function room_mt:set_role(actor, occupan
local x = st.stanza("x", {xmlns = "http://jabber.org/protocol/muc#user"});
if not role then
- x:tag("status", {code = "307"}):up();
+ x = {
+ base = x;
+ self = st.stanza("x", {xmlns = "http://jabber.org/protocol/muc#user"})
+ :tag("status", {code = "307"}):up();
+ };
end
occupant.role = role;
self:save_occupant(occupant);
Changes
owner Zash
tags Status-Started
pep.
on
IN: <presence type="unavailable" to="user@domain" from="muc@muc-server"><status>Kicked: undefined condition</status><x xmlns="http://jabber.org/protocol/muc#user"><item jid="user@domain" affiliation="owner" role="moderator" /><status code="110" /></x><nick xmlns="http://jabber.org/protocol/nick" /></presence>
Looks good to me!
I see the intent of this, but I cannot really see how this would happen in reality (error presences are, as far as I know, only generated on S2S issues, so you would not see your own kick anyways).
On the other hand, this violates XEP-0045.
1. §5.1.3 Changing Roles <https://xmpp.org/extensions/xep-0045.html#roles-change> states:
> The ways in which an occupant's role changes are well-defined.
and the table lists "exiting a room" as a change to the "none" role. I hope we can agree that if an unavailable presence is sent for that participant, they are outside the room now.
2. §8.2 Kicking an Occupant <https://xmpp.org/extensions/xep-0045.html#kick> states that the 307 status code is to be included in the <presence/> stanzas emitted for kicks.
Now we can argue that this only holds for moderator-initiated kicks. However, a service-initiated kick should, in my opinion, be clearly marked. If the additional information that the kick was due to an error is needed (and I think that would be valuable to know for clients, to allow for automatic re-join in those cases), a new status code could be defined which can be used *in addition* to 307.
Zash
on
Looks like Jonas is correct, everyone should see the 307 status code and the one being kicked should also get status code 110, "Inform user that presence refers to itself".
Reverted in https://hg.prosody.im/trunk/rev/627689c058aa
Changes
tags Status-Invalid
Link Mauve
on
The reason the fix for this issue wasn’t correct is that you removed the 307 status for every kick, which is obviously not something you want (when a participant kicks another participant, you want everyone to know about that).
Sending a 307 status in answer to a participant sending an error message on the other hand is a lot less “important” for everyone, and does lead people to ask why this specific user who didn’t do anything wrong has been kicked. It may make sense to give back that 307 status only to the one user who sent the error message, although a simple unavailable presence could make more sense for the same reason.
pep.
on
18:45:11 OUT <presence type="error" to="room@muc/pep." id="coucou"><error><recipient-unavailable xmlns="urn:ietf:params:xml:ns:xmpp-stanzas" /><reason>Coucou.</reason></error></presence>
18:45:11 IN <presence type="unavailable" to="pep@foo/bar1" from="room@muc/pep."><status>Kicked: recipient unavailable</status><x xmlns="http://jabber.org/protocol/muc#user"><status code="307" /><item jid="pep@foo/bar1" affiliation="owner" role="moderator" /><item jid="pep@foo/bar2" affiliation="owner" role="moderator" /><item jid="pep@foo/bar3" affiliation="owner" role="moderator" /><status code="110" /></x></presence>
This is what I get at the moment. The combination of <status/> and <status code="307"/> is confusing.
I would say either use <presence><x><status code="307"/><reason/></x>, or <presence><status/><x/>.
Apparently the XEP doesn't specify what should happen in this case (kick or not), but I agree with him that 307 on error sent by the user might not be the best course of action. In any case I think the status/reason should be adjusted.
I think the only open question here now is whether an error is a leave or a kick, though we have the new status code.
In my mind it should be normal leave + new status code.
Jonas Wielicki
on
Ge0rG observed this the other day:
22:26:10 <--- Ge0rG (georg@yax.im/poezio-IS8H) has left the room (It is not allowed to send error messages to the room. The participant (Ge0rG) has sent an error message (remote-server-timeout) and got kicked from the room)
Maybe a fancy error like this + possibly i18n + leave + new status code.
MattJ
on
That example is a bit wordy. I'm in favour of keeping the error text as is (though I'm considering "Kicked:" -> "Error:").
And I am marginally in favour of removing 307, after some thought.
My main reason against it, is that I would rather clients implement the 333 status code. Which is less likely if we revert to normal leaves.
pep.
on
Implemented in poezio in https://git.louiz.org/poezio/commit/?id=ed0be7b
I am ignoring the kick, just displaying the provided status. The 333 is there just to ignore that kick really, unless I should display a generic error when there is nothing provided by the server?
What steps will reproduce the problem? 1. Join a MUC running hg:5abb6bc45edd. 2. Send <presence type="error" to="muc@muc-server"><error xmlns="jabber:client" type="cancel"><undefined-condition xmlns="urn:ietf:params:xml:ns:xmpp-stanzas" /></error></presence> or any other valid error presence. What is the expected output? What do you see instead? I would like to see a normal part with a kicked reason, like that: 23:10:59 <--- Link Mauve has left the room (Kicked: undefined condition) Instead I see an actual kick: 23:10:59 -!- You have been kicked. What version of the product are you using? On what operating system? Prosody trunk version hg:5abb6bc45edd, on ArchLinux. Please provide any additional information below. The only differences are the 307 status, the inclusion of the real JID, and the preserved role. On 0.10: <presence type="unavailable" to="me@server/poezio" from="muc@muc-server/Link Mauve"><status>Kicked: undefined condition</status><x xmlns="http://jabber.org/protocol/muc#user"><item affiliation="owner" role="none" /><status code="110" /></x></presence> On trunk: <presence type="unavailable" to="me@server/poezio" from="muc@muc-server/Link Mauve"><status>Kicked: undefined condition</status><x xmlns="http://jabber.org/protocol/muc#user"><status code="307" /><item jid="me@server/poezio" affiliation="owner" role="moderator" /><status code="110" /></x></presence>
Could you test this patch? diff -r 45be94611593 plugins/muc/muc.lib.lua --- a/plugins/muc/muc.lib.lua Tue Jun 13 20:14:06 2017 +0200 +++ b/plugins/muc/muc.lib.lua Fri Jun 23 02:44:16 2017 +0200 @@ -349,8 +349,11 @@ function room_mt:handle_kickable(origin, occupant:set_session(real_jid, st.presence({type="unavailable"}) :tag('status'):text(error_message)); self:save_occupant(occupant); - local x = st.stanza("x", {xmlns = "http://jabber.org/protocol/muc#user";}) - :tag("status", {code = "307"}) + local x = { + base = st.stanza("x", {xmlns = "http://jabber.org/protocol/muc#user";}); + self = st.stanza("x", {xmlns = "http://jabber.org/protocol/muc#user";}) + :tag("status", {code = "307"}); + }; self:publicise_occupant_status(occupant, x); if occupant.jid == real_jid then -- Was last session module:fire_event("muc-occupant-left", {room = self; nick = occupant.nick; occupant = occupant;}); @@ -1298,7 +1301,11 @@ function room_mt:set_role(actor, occupan local x = st.stanza("x", {xmlns = "http://jabber.org/protocol/muc#user"}); if not role then - x:tag("status", {code = "307"}):up(); + x = { + base = x; + self = st.stanza("x", {xmlns = "http://jabber.org/protocol/muc#user"}) + :tag("status", {code = "307"}):up(); + }; end occupant.role = role; self:save_occupant(occupant);
ChangesIN: <presence type="unavailable" to="user@domain" from="muc@muc-server"><status>Kicked: undefined condition</status><x xmlns="http://jabber.org/protocol/muc#user"><item jid="user@domain" affiliation="owner" role="moderator" /><status code="110" /></x><nick xmlns="http://jabber.org/protocol/nick" /></presence> Looks good to me!
Fixed in https://hg.prosody.im/trunk/rev/a6574fdf8734 Thanks
ChangesI see the intent of this, but I cannot really see how this would happen in reality (error presences are, as far as I know, only generated on S2S issues, so you would not see your own kick anyways). On the other hand, this violates XEP-0045. 1. §5.1.3 Changing Roles <https://xmpp.org/extensions/xep-0045.html#roles-change> states: > The ways in which an occupant's role changes are well-defined. and the table lists "exiting a room" as a change to the "none" role. I hope we can agree that if an unavailable presence is sent for that participant, they are outside the room now. 2. §8.2 Kicking an Occupant <https://xmpp.org/extensions/xep-0045.html#kick> states that the 307 status code is to be included in the <presence/> stanzas emitted for kicks. Now we can argue that this only holds for moderator-initiated kicks. However, a service-initiated kick should, in my opinion, be clearly marked. If the additional information that the kick was due to an error is needed (and I think that would be valuable to know for clients, to allow for automatic re-join in those cases), a new status code could be defined which can be used *in addition* to 307.
Looks like Jonas is correct, everyone should see the 307 status code and the one being kicked should also get status code 110, "Inform user that presence refers to itself". Reverted in https://hg.prosody.im/trunk/rev/627689c058aa
ChangesThe reason the fix for this issue wasn’t correct is that you removed the 307 status for every kick, which is obviously not something you want (when a participant kicks another participant, you want everyone to know about that). Sending a 307 status in answer to a participant sending an error message on the other hand is a lot less “important” for everyone, and does lead people to ask why this specific user who didn’t do anything wrong has been kicked. It may make sense to give back that 307 status only to the one user who sent the error message, although a simple unavailable presence could make more sense for the same reason.
18:45:11 OUT <presence type="error" to="room@muc/pep." id="coucou"><error><recipient-unavailable xmlns="urn:ietf:params:xml:ns:xmpp-stanzas" /><reason>Coucou.</reason></error></presence> 18:45:11 IN <presence type="unavailable" to="pep@foo/bar1" from="room@muc/pep."><status>Kicked: recipient unavailable</status><x xmlns="http://jabber.org/protocol/muc#user"><status code="307" /><item jid="pep@foo/bar1" affiliation="owner" role="moderator" /><item jid="pep@foo/bar2" affiliation="owner" role="moderator" /><item jid="pep@foo/bar3" affiliation="owner" role="moderator" /><status code="110" /></x></presence> This is what I get at the moment. The combination of <status/> and <status code="307"/> is confusing. I would say either use <presence><x><status code="307"/><reason/></x>, or <presence><status/><x/>. Apparently the XEP doesn't specify what should happen in this case (kick or not), but I agree with him that 307 on error sent by the user might not be the best course of action. In any case I think the status/reason should be adjusted.
I remain undecided.
ChangesZashMattJSmall update: https://mail.jabber.org/pipermail/standards/2017-December/034057.html https://github.com/xsf/xeps/pull/559 I don't know if this is the correct way to go but at least the discussion started.
Does #1087 / https://hg.prosody.im/trunk/rev/0a3dced117f7 mean this can be closed?
I think the only open question here now is whether an error is a leave or a kick, though we have the new status code. In my mind it should be normal leave + new status code.
Ge0rG observed this the other day: 22:26:10 <--- Ge0rG (georg@yax.im/poezio-IS8H) has left the room (It is not allowed to send error messages to the room. The participant (Ge0rG) has sent an error message (remote-server-timeout) and got kicked from the room) Maybe a fancy error like this + possibly i18n + leave + new status code.
That example is a bit wordy. I'm in favour of keeping the error text as is (though I'm considering "Kicked:" -> "Error:"). And I am marginally in favour of removing 307, after some thought. My main reason against it, is that I would rather clients implement the 333 status code. Which is less likely if we revert to normal leaves.
Implemented in poezio in https://git.louiz.org/poezio/commit/?id=ed0be7b I am ignoring the kick, just displaying the provided status. The 333 is there just to ignore that kick really, unless I should display a generic error when there is nothing provided by the server?
Fixed in a474c94d0b0a
Changes