From 4b5cd13b76c925e7b85cd05338d25f413c8a75e5 Mon Sep 17 00:00:00 2001 From: bgrozev Date: Thu, 12 Dec 2024 14:50:18 -0600 Subject: [PATCH] fix: Handling of IQ error responses. (#1204) Error responses to IQs may be parsed as a specific IQ type (e.g. ConferenceModifyIQ) when they contain a child other than . The correct way to check for an error is with IQ.getError() instead of the instance type. --- .../jicofo/bridge/colibri/Colibri2Session.kt | 64 +++++++++-------- .../bridge/colibri/ColibriV2SessionManager.kt | 21 +++--- .../org/jitsi/jicofo/ktor/Application.kt | 18 +++-- .../org/jitsi/jicofo/xmpp/JigasiIqHandler.kt | 68 +++++++++---------- 4 files changed, 88 insertions(+), 83 deletions(-) diff --git a/jicofo-selector/src/main/kotlin/org/jitsi/jicofo/bridge/colibri/Colibri2Session.kt b/jicofo-selector/src/main/kotlin/org/jitsi/jicofo/bridge/colibri/Colibri2Session.kt index 3633b4e0d0..40670e449c 100644 --- a/jicofo-selector/src/main/kotlin/org/jitsi/jicofo/bridge/colibri/Colibri2Session.kt +++ b/jicofo-selector/src/main/kotlin/org/jitsi/jicofo/bridge/colibri/Colibri2Session.kt @@ -45,7 +45,6 @@ import org.jitsi.xmpp.extensions.jingle.ExtmapAllowMixedPacketExtension import org.jitsi.xmpp.extensions.jingle.IceUdpTransportPacketExtension import org.jitsi.xmpp.util.XmlStringBuilderUtil.Companion.toStringOpt import org.jivesoftware.smack.StanzaCollector -import org.jivesoftware.smack.packet.ErrorIQ import org.jivesoftware.smack.packet.IQ import org.jivesoftware.smackx.muc.MUCRole import java.util.Collections.singletonList @@ -294,36 +293,43 @@ class Colibri2Session( */ private fun sendRequest(iq: IQ, name: String) { logger.debug { "Sending $name request: ${iq.toStringOpt()}" } - xmppConnection.sendIqAndHandleResponseAsync(iq) { - when (it) { - is ConferenceModifiedIQ -> logger.debug { "Received $name response: ${it.toStringOpt()}" } - null -> logger.info("$name request timed out. Ignoring.") - else -> { - if (it is ErrorIQ) { - val reason = it.error?.getExtension( - Colibri2Error.ELEMENT, - Colibri2Error.NAMESPACE - )?.reason - val endpointId = it.error?.getExtension( - Colibri2Endpoint.ELEMENT, - Colibri2Endpoint.NAMESPACE - )?.id - // If colibri2 error extension is present then the message came from - // a jitsi-videobridge instance. Otherwise, it might come from another component - // (e.g. the XMPP server or MUC component). - val reInvite = reason == Colibri2Error.Reason.UNKNOWN_ENDPOINT && endpointId != null - if (reInvite) { - logger.warn( - "Endpoint [$endpointId] is not found, session failed: ${it.toStringOpt()}, " + - "request was: ${iq.toStringOpt()}" - ) - colibriSessionManager.endpointFailed(endpointId!!) - return@sendIqAndHandleResponseAsync - } - } - logger.error("Received error response for $name, session failed: ${it.toStringOpt()}") + xmppConnection.sendIqAndHandleResponseAsync(iq) { response -> + if (response == null) { + logger.info("$name request timed out. Ignoring.") + return@sendIqAndHandleResponseAsync + } + + response.error?.let { error -> + val reason = error.getExtension( + Colibri2Error.ELEMENT, + Colibri2Error.NAMESPACE + )?.reason + val endpointId = error.getExtension( + Colibri2Endpoint.ELEMENT, + Colibri2Endpoint.NAMESPACE + )?.id + // If colibri2 error extension is present then the message came from + // a jitsi-videobridge instance. Otherwise, it might come from another component + // (e.g. the XMPP server or MUC component). + val reInvite = reason == Colibri2Error.Reason.UNKNOWN_ENDPOINT && endpointId != null + if (reInvite) { + logger.warn( + "Endpoint [$endpointId] is not found, session failed: ${error.toStringOpt()}, " + + "request was: ${iq.toStringOpt()}" + ) + colibriSessionManager.endpointFailed(endpointId!!) + } else { + logger.error("Received error response for $name, session failed: ${error.toStringOpt()}") colibriSessionManager.sessionFailed(this@Colibri2Session) } + return@sendIqAndHandleResponseAsync + } + + if (response !is ConferenceModifiedIQ) { + logger.error("Received response with unexpected type ${response.javaClass.name}") + colibriSessionManager.sessionFailed(this@Colibri2Session) + } else { + logger.debug { "Received $name response: ${response.toStringOpt()}" } } } } diff --git a/jicofo-selector/src/main/kotlin/org/jitsi/jicofo/bridge/colibri/ColibriV2SessionManager.kt b/jicofo-selector/src/main/kotlin/org/jitsi/jicofo/bridge/colibri/ColibriV2SessionManager.kt index c4df29f694..f151f5b0c8 100644 --- a/jicofo-selector/src/main/kotlin/org/jitsi/jicofo/bridge/colibri/ColibriV2SessionManager.kt +++ b/jicofo-selector/src/main/kotlin/org/jitsi/jicofo/bridge/colibri/ColibriV2SessionManager.kt @@ -45,7 +45,6 @@ import org.jitsi.xmpp.extensions.jingle.IceUdpTransportPacketExtension import org.jitsi.xmpp.util.XmlStringBuilderUtil.Companion.toStringOpt import org.jivesoftware.smack.AbstractXMPPConnection import org.jivesoftware.smack.StanzaCollector -import org.jivesoftware.smack.packet.ErrorIQ import org.jivesoftware.smack.packet.IQ import org.jivesoftware.smack.packet.StanzaError.Condition.bad_request import org.jivesoftware.smack.packet.StanzaError.Condition.conflict @@ -434,21 +433,22 @@ class ColibriV2SessionManager( if (response == null) { session.bridge.isOperational = false throw ColibriAllocationFailedException("Timeout", true) - } else if (response is ErrorIQ) { + } + response.error?.let { error -> // The reason in a colibri2 error extension, if one is present. If a reason is present we know the response // comes from a jitsi-videobridge instance. Otherwise, it might come from another component (e.g. the // XMPP server or MUC component). - val reason = response.error?.getExtension( + val reason = error.getExtension( Colibri2Error.ELEMENT, Colibri2Error.NAMESPACE )?.reason logger.info("Received error response: ${response.toStringOpt()}") - when (response.error?.condition) { + when (error.condition) { bad_request -> { // Most probably we sent a bad request. // If we flag the bridge as non-operational we may disrupt other conferences. // If we trigger a re-invite we may cause the same error repeating. - throw ColibriAllocationFailedException("Bad request: ${response.error?.toStringOpt()}", false) + throw ColibriAllocationFailedException("Bad request: ${error.toStringOpt()}", false) } item_not_found -> { if (reason == Colibri2Error.Reason.CONFERENCE_NOT_FOUND) { @@ -467,7 +467,7 @@ class ColibriV2SessionManager( if (reason == null) { // An error NOT coming from the bridge. throw ColibriAllocationFailedException( - "XMPP error: ${response.error?.toStringOpt()}", + "XMPP error: ${error.toStringOpt()}", true ) } else if (reason == Colibri2Error.Reason.CONFERENCE_ALREADY_EXISTS) { @@ -480,7 +480,7 @@ class ColibriV2SessionManager( // we can't expire a conference without listing its individual endpoints and we think there // were none. // We remove the bridge from the conference (expiring it) and re-invite the participants. - throw ColibriAllocationFailedException("Colibri error: ${response.error?.toStringOpt()}", true) + throw ColibriAllocationFailedException("Colibri error: ${error.toStringOpt()}", true) } } service_unavailable -> { @@ -496,14 +496,17 @@ class ColibriV2SessionManager( } else -> { session.bridge.isOperational = false - throw ColibriAllocationFailedException("Error: ${response.error?.toStringOpt()}", true) + throw ColibriAllocationFailedException("Error: ${error.toStringOpt()}", true) } } } if (response !is ConferenceModifiedIQ) { session.bridge.isOperational = false - throw ColibriAllocationFailedException("Response of wrong type: ${response::class.java.name}", false) + throw ColibriAllocationFailedException( + "Response of wrong type: ${response::class.java.name}: ${response.toXML()}", + false + ) } if (created) { diff --git a/jicofo/src/main/kotlin/org/jitsi/jicofo/ktor/Application.kt b/jicofo/src/main/kotlin/org/jitsi/jicofo/ktor/Application.kt index 907938f3e9..0354069382 100644 --- a/jicofo/src/main/kotlin/org/jitsi/jicofo/ktor/Application.kt +++ b/jicofo/src/main/kotlin/org/jitsi/jicofo/ktor/Application.kt @@ -53,7 +53,6 @@ import org.jitsi.jicofo.xmpp.XmppCapsStats import org.jitsi.utils.OrderedJsonObject import org.jitsi.utils.logging2.createLogger import org.jitsi.xmpp.extensions.jitsimeet.ConferenceIq -import org.jivesoftware.smack.packet.ErrorIQ import org.jivesoftware.smack.packet.IQ import org.jivesoftware.smack.packet.StanzaError import org.json.simple.JSONArray @@ -164,17 +163,16 @@ class Application( throw BadRequest(e.message) } - if (response !is ConferenceIq) { - if (response is ErrorIQ) { - throw when (response.error.condition) { - StanzaError.Condition.not_authorized -> Forbidden() - StanzaError.Condition.not_acceptable -> BadRequest("invalid-session") - else -> BadRequest(response.error.toString()) - } - } else { - throw InternalError() + response.error?.let { + throw when (it.condition) { + StanzaError.Condition.not_authorized -> Forbidden() + StanzaError.Condition.not_acceptable -> BadRequest("invalid-session") + else -> BadRequest(it.toString()) } } + if (response !is ConferenceIq) { + throw InternalError() + } call.respond(ConferenceRequest.fromConferenceIq(response)) } } diff --git a/jicofo/src/main/kotlin/org/jitsi/jicofo/xmpp/JigasiIqHandler.kt b/jicofo/src/main/kotlin/org/jitsi/jicofo/xmpp/JigasiIqHandler.kt index c77df821da..88f8130fe4 100644 --- a/jicofo/src/main/kotlin/org/jitsi/jicofo/xmpp/JigasiIqHandler.kt +++ b/jicofo/src/main/kotlin/org/jitsi/jicofo/xmpp/JigasiIqHandler.kt @@ -30,7 +30,6 @@ import org.jitsi.xmpp.extensions.rayo.DialIq import org.jitsi.xmpp.util.XmlStringBuilderUtil.Companion.toStringOpt import org.jivesoftware.smack.AbstractXMPPConnection import org.jivesoftware.smack.SmackException -import org.jivesoftware.smack.packet.ErrorIQ import org.jivesoftware.smack.packet.IQ import org.jivesoftware.smack.packet.StanzaError import org.jivesoftware.smack.packet.id.StandardStanzaIdSource @@ -157,47 +156,46 @@ class JigasiIqHandler( return } - when (responseFromJigasi) { - null, is ErrorIQ -> { - if (responseFromJigasi == null) { - logger.warn("Jigasi instance timed out: $jigasiJid") - Stats.singleInstanceTimeouts.inc() - } else { - logger.warn("Jigasi instance returned error ($jigasiJid): ${responseFromJigasi.toStringOpt()}") - Stats.singleInstanceErrors.inc() - } + if (responseFromJigasi == null || responseFromJigasi.error != null) { + // Timeout or error. + if (responseFromJigasi == null) { + logger.warn("Jigasi instance timed out: $jigasiJid") + Stats.singleInstanceTimeouts.inc() + } else { + logger.warn("Jigasi instance returned error ($jigasiJid): ${responseFromJigasi.toStringOpt()}") + Stats.singleInstanceErrors.inc() + } - if (retryCount > 0) { - logger.info("Will retry up to $retryCount more times.") - Stats.retries.inc() - // Do not try the same instance again. - inviteJigasi(request, conference, retryCount - 1, exclude + jigasiJid) + if (retryCount > 0) { + logger.info("Will retry up to $retryCount more times.") + Stats.retries.inc() + // Do not try the same instance again. + inviteJigasi(request, conference, retryCount - 1, exclude + jigasiJid) + } else { + val condition = if (responseFromJigasi == null) { + StanzaError.Condition.remote_server_timeout } else { - val condition = if (responseFromJigasi == null) { - StanzaError.Condition.remote_server_timeout - } else { - StanzaError.Condition.undefined_condition - } - logger.warn("Request failed, all instances failed.") - stats.allInstancesFailed() - request.connection.tryToSendStanza( - IQ.createErrorResponse(request.iq, StanzaError.getBuilder(condition).build()) - ) + StanzaError.Condition.undefined_condition } - } - else -> { - logger.info("response from jigasi: ${responseFromJigasi.toStringOpt()}") - // Successful response from Jigasi, forward it as the response to the client. + logger.warn("Request failed, all instances failed.") + stats.allInstancesFailed() request.connection.tryToSendStanza( - responseFromJigasi.apply { - from = null - to = request.iq.from - stanzaId = request.iq.stanzaId - } + IQ.createErrorResponse(request.iq, StanzaError.getBuilder(condition).build()) ) - return } + + return } + + logger.info("Response from jigasi: ${responseFromJigasi.toStringOpt()}") + // Successful response from Jigasi, forward it as the response to the client. + request.connection.tryToSendStanza( + responseFromJigasi.apply { + from = null + to = request.iq.from + stanzaId = request.iq.stanzaId + } + ) } companion object {