#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
  1. 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>

  2. 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
  3. 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!

  4. Zash on

    Fixed in https://hg.prosody.im/trunk/rev/a6574fdf8734 Thanks

    Changes
    • tags Status-Fixed
  5. Zash on

    Changes
    • tags Milestone-0.11
  6. Jonas Wielicki on

    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.

  7. 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
  8. 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.

  9. 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.

  10. MattJ on

    Changes
    • tags Status-Accepted
  11. Zash on

    I remain undecided.

    Changes
    • owner Zash MattJ
  12. pep. on

    Small 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.

  13. Zash on

    Does #1087 / https://hg.prosody.im/trunk/rev/0a3dced117f7 mean this can be closed?

  14. MattJ on

    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.

  15. 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.

  16. 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.

  17. 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?

  18. MattJ on

    Fixed in a474c94d0b0a

    Changes
    • tags Status-Fixed

New comment

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