Skip to content

Commit 4b5cd13

Browse files
bgrozevbgrozev
authored
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 <error>. The correct way to check for an error is with IQ.getError() instead of the instance type.
1 parent 012e063 commit 4b5cd13

File tree

4 files changed

+88
-83
lines changed

4 files changed

+88
-83
lines changed

‎jicofo-selector/src/main/kotlin/org/jitsi/jicofo/bridge/colibri/Colibri2Session.kt‎

Lines changed: 35 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,6 @@ import org.jitsi.xmpp.extensions.jingle.ExtmapAllowMixedPacketExtension
4545
import org.jitsi.xmpp.extensions.jingle.IceUdpTransportPacketExtension
4646
import org.jitsi.xmpp.util.XmlStringBuilderUtil.Companion.toStringOpt
4747
import org.jivesoftware.smack.StanzaCollector
48-
import org.jivesoftware.smack.packet.ErrorIQ
4948
import org.jivesoftware.smack.packet.IQ
5049
import org.jivesoftware.smackx.muc.MUCRole
5150
import java.util.Collections.singletonList
@@ -294,36 +293,43 @@ class Colibri2Session(
294293
*/
295294
private fun sendRequest(iq: IQ, name: String) {
296295
logger.debug { "Sending $name request: ${iq.toStringOpt()}" }
297-
xmppConnection.sendIqAndHandleResponseAsync(iq) {
298-
when (it) {
299-
is ConferenceModifiedIQ -> logger.debug { "Received $name response: ${it.toStringOpt()}" }
300-
null -> logger.info("$name request timed out. Ignoring.")
301-
else -> {
302-
if (it is ErrorIQ) {
303-
val reason = it.error?.getExtension<Colibri2Error>(
304-
Colibri2Error.ELEMENT,
305-
Colibri2Error.NAMESPACE
306-
)?.reason
307-
val endpointId = it.error?.getExtension<Colibri2Endpoint>(
308-
Colibri2Endpoint.ELEMENT,
309-
Colibri2Endpoint.NAMESPACE
310-
)?.id
311-
// If colibri2 error extension is present then the message came from
312-
// a jitsi-videobridge instance. Otherwise, it might come from another component
313-
// (e.g. the XMPP server or MUC component).
314-
val reInvite = reason == Colibri2Error.Reason.UNKNOWN_ENDPOINT && endpointId != null
315-
if (reInvite) {
316-
logger.warn(
317-
"Endpoint [$endpointId] is not found, session failed: ${it.toStringOpt()}, " +
318-
"request was: ${iq.toStringOpt()}"
319-
)
320-
colibriSessionManager.endpointFailed(endpointId!!)
321-
return@sendIqAndHandleResponseAsync
322-
}
323-
}
324-
logger.error("Received error response for $name, session failed: ${it.toStringOpt()}")
296+
xmppConnection.sendIqAndHandleResponseAsync(iq) { response ->
297+
if (response == null) {
298+
logger.info("$name request timed out. Ignoring.")
299+
return@sendIqAndHandleResponseAsync
300+
}
301+
302+
response.error?.let { error ->
303+
val reason = error.getExtension<Colibri2Error>(
304+
Colibri2Error.ELEMENT,
305+
Colibri2Error.NAMESPACE
306+
)?.reason
307+
val endpointId = error.getExtension<Colibri2Endpoint>(
308+
Colibri2Endpoint.ELEMENT,
309+
Colibri2Endpoint.NAMESPACE
310+
)?.id
311+
// If colibri2 error extension is present then the message came from
312+
// a jitsi-videobridge instance. Otherwise, it might come from another component
313+
// (e.g. the XMPP server or MUC component).
314+
val reInvite = reason == Colibri2Error.Reason.UNKNOWN_ENDPOINT && endpointId != null
315+
if (reInvite) {
316+
logger.warn(
317+
"Endpoint [$endpointId] is not found, session failed: ${error.toStringOpt()}, " +
318+
"request was: ${iq.toStringOpt()}"
319+
)
320+
colibriSessionManager.endpointFailed(endpointId!!)
321+
} else {
322+
logger.error("Received error response for $name, session failed: ${error.toStringOpt()}")
325323
colibriSessionManager.sessionFailed(this@Colibri2Session)
326324
}
325+
return@sendIqAndHandleResponseAsync
326+
}
327+
328+
if (response !is ConferenceModifiedIQ) {
329+
logger.error("Received response with unexpected type ${response.javaClass.name}")
330+
colibriSessionManager.sessionFailed(this@Colibri2Session)
331+
} else {
332+
logger.debug { "Received $name response: ${response.toStringOpt()}" }
327333
}
328334
}
329335
}

‎jicofo-selector/src/main/kotlin/org/jitsi/jicofo/bridge/colibri/ColibriV2SessionManager.kt‎

Lines changed: 12 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,6 @@ import org.jitsi.xmpp.extensions.jingle.IceUdpTransportPacketExtension
4545
import org.jitsi.xmpp.util.XmlStringBuilderUtil.Companion.toStringOpt
4646
import org.jivesoftware.smack.AbstractXMPPConnection
4747
import org.jivesoftware.smack.StanzaCollector
48-
import org.jivesoftware.smack.packet.ErrorIQ
4948
import org.jivesoftware.smack.packet.IQ
5049
import org.jivesoftware.smack.packet.StanzaError.Condition.bad_request
5150
import org.jivesoftware.smack.packet.StanzaError.Condition.conflict
@@ -434,21 +433,22 @@ class ColibriV2SessionManager(
434433
if (response == null) {
435434
session.bridge.isOperational = false
436435
throw ColibriAllocationFailedException("Timeout", true)
437-
} else if (response is ErrorIQ) {
436+
}
437+
response.error?.let { error ->
438438
// The reason in a colibri2 error extension, if one is present. If a reason is present we know the response
439439
// comes from a jitsi-videobridge instance. Otherwise, it might come from another component (e.g. the
440440
// XMPP server or MUC component).
441-
val reason = response.error?.getExtension<Colibri2Error>(
441+
val reason = error.getExtension<Colibri2Error>(
442442
Colibri2Error.ELEMENT,
443443
Colibri2Error.NAMESPACE
444444
)?.reason
445445
logger.info("Received error response: ${response.toStringOpt()}")
446-
when (response.error?.condition) {
446+
when (error.condition) {
447447
bad_request -> {
448448
// Most probably we sent a bad request.
449449
// If we flag the bridge as non-operational we may disrupt other conferences.
450450
// If we trigger a re-invite we may cause the same error repeating.
451-
throw ColibriAllocationFailedException("Bad request: ${response.error?.toStringOpt()}", false)
451+
throw ColibriAllocationFailedException("Bad request: ${error.toStringOpt()}", false)
452452
}
453453
item_not_found -> {
454454
if (reason == Colibri2Error.Reason.CONFERENCE_NOT_FOUND) {
@@ -467,7 +467,7 @@ class ColibriV2SessionManager(
467467
if (reason == null) {
468468
// An error NOT coming from the bridge.
469469
throw ColibriAllocationFailedException(
470-
"XMPP error: ${response.error?.toStringOpt()}",
470+
"XMPP error: ${error.toStringOpt()}",
471471
true
472472
)
473473
} else if (reason == Colibri2Error.Reason.CONFERENCE_ALREADY_EXISTS) {
@@ -480,7 +480,7 @@ class ColibriV2SessionManager(
480480
// we can't expire a conference without listing its individual endpoints and we think there
481481
// were none.
482482
// We remove the bridge from the conference (expiring it) and re-invite the participants.
483-
throw ColibriAllocationFailedException("Colibri error: ${response.error?.toStringOpt()}", true)
483+
throw ColibriAllocationFailedException("Colibri error: ${error.toStringOpt()}", true)
484484
}
485485
}
486486
service_unavailable -> {
@@ -496,14 +496,17 @@ class ColibriV2SessionManager(
496496
}
497497
else -> {
498498
session.bridge.isOperational = false
499-
throw ColibriAllocationFailedException("Error: ${response.error?.toStringOpt()}", true)
499+
throw ColibriAllocationFailedException("Error: ${error.toStringOpt()}", true)
500500
}
501501
}
502502
}
503503

504504
if (response !is ConferenceModifiedIQ) {
505505
session.bridge.isOperational = false
506-
throw ColibriAllocationFailedException("Response of wrong type: ${response::class.java.name}", false)
506+
throw ColibriAllocationFailedException(
507+
"Response of wrong type: ${response::class.java.name}: ${response.toXML()}",
508+
false
509+
)
507510
}
508511

509512
if (created) {

‎jicofo/src/main/kotlin/org/jitsi/jicofo/ktor/Application.kt‎

Lines changed: 8 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,6 @@ import org.jitsi.jicofo.xmpp.XmppCapsStats
5353
import org.jitsi.utils.OrderedJsonObject
5454
import org.jitsi.utils.logging2.createLogger
5555
import org.jitsi.xmpp.extensions.jitsimeet.ConferenceIq
56-
import org.jivesoftware.smack.packet.ErrorIQ
5756
import org.jivesoftware.smack.packet.IQ
5857
import org.jivesoftware.smack.packet.StanzaError
5958
import org.json.simple.JSONArray
@@ -164,17 +163,16 @@ class Application(
164163
throw BadRequest(e.message)
165164
}
166165

167-
if (response !is ConferenceIq) {
168-
if (response is ErrorIQ) {
169-
throw when (response.error.condition) {
170-
StanzaError.Condition.not_authorized -> Forbidden()
171-
StanzaError.Condition.not_acceptable -> BadRequest("invalid-session")
172-
else -> BadRequest(response.error.toString())
173-
}
174-
} else {
175-
throw InternalError()
166+
response.error?.let {
167+
throw when (it.condition) {
168+
StanzaError.Condition.not_authorized -> Forbidden()
169+
StanzaError.Condition.not_acceptable -> BadRequest("invalid-session")
170+
else -> BadRequest(it.toString())
176171
}
177172
}
173+
if (response !is ConferenceIq) {
174+
throw InternalError()
175+
}
178176
call.respond(ConferenceRequest.fromConferenceIq(response))
179177
}
180178
}

‎jicofo/src/main/kotlin/org/jitsi/jicofo/xmpp/JigasiIqHandler.kt‎

Lines changed: 33 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,6 @@ import org.jitsi.xmpp.extensions.rayo.DialIq
3030
import org.jitsi.xmpp.util.XmlStringBuilderUtil.Companion.toStringOpt
3131
import org.jivesoftware.smack.AbstractXMPPConnection
3232
import org.jivesoftware.smack.SmackException
33-
import org.jivesoftware.smack.packet.ErrorIQ
3433
import org.jivesoftware.smack.packet.IQ
3534
import org.jivesoftware.smack.packet.StanzaError
3635
import org.jivesoftware.smack.packet.id.StandardStanzaIdSource
@@ -157,47 +156,46 @@ class JigasiIqHandler(
157156
return
158157
}
159158

160-
when (responseFromJigasi) {
161-
null, is ErrorIQ -> {
162-
if (responseFromJigasi == null) {
163-
logger.warn("Jigasi instance timed out: $jigasiJid")
164-
Stats.singleInstanceTimeouts.inc()
165-
} else {
166-
logger.warn("Jigasi instance returned error ($jigasiJid): ${responseFromJigasi.toStringOpt()}")
167-
Stats.singleInstanceErrors.inc()
168-
}
159+
if (responseFromJigasi == null || responseFromJigasi.error != null) {
160+
// Timeout or error.
161+
if (responseFromJigasi == null) {
162+
logger.warn("Jigasi instance timed out: $jigasiJid")
163+
Stats.singleInstanceTimeouts.inc()
164+
} else {
165+
logger.warn("Jigasi instance returned error ($jigasiJid): ${responseFromJigasi.toStringOpt()}")
166+
Stats.singleInstanceErrors.inc()
167+
}
169168

170-
if (retryCount > 0) {
171-
logger.info("Will retry up to $retryCount more times.")
172-
Stats.retries.inc()
173-
// Do not try the same instance again.
174-
inviteJigasi(request, conference, retryCount - 1, exclude + jigasiJid)
169+
if (retryCount > 0) {
170+
logger.info("Will retry up to $retryCount more times.")
171+
Stats.retries.inc()
172+
// Do not try the same instance again.
173+
inviteJigasi(request, conference, retryCount - 1, exclude + jigasiJid)
174+
} else {
175+
val condition = if (responseFromJigasi == null) {
176+
StanzaError.Condition.remote_server_timeout
175177
} else {
176-
val condition = if (responseFromJigasi == null) {
177-
StanzaError.Condition.remote_server_timeout
178-
} else {
179-
StanzaError.Condition.undefined_condition
180-
}
181-
logger.warn("Request failed, all instances failed.")
182-
stats.allInstancesFailed()
183-
request.connection.tryToSendStanza(
184-
IQ.createErrorResponse(request.iq, StanzaError.getBuilder(condition).build())
185-
)
178+
StanzaError.Condition.undefined_condition
186179
}
187-
}
188-
else -> {
189-
logger.info("response from jigasi: ${responseFromJigasi.toStringOpt()}")
190-
// Successful response from Jigasi, forward it as the response to the client.
180+
logger.warn("Request failed, all instances failed.")
181+
stats.allInstancesFailed()
191182
request.connection.tryToSendStanza(
192-
responseFromJigasi.apply {
193-
from = null
194-
to = request.iq.from
195-
stanzaId = request.iq.stanzaId
196-
}
183+
IQ.createErrorResponse(request.iq, StanzaError.getBuilder(condition).build())
197184
)
198-
return
199185
}
186+
187+
return
200188
}
189+
190+
logger.info("Response from jigasi: ${responseFromJigasi.toStringOpt()}")
191+
// Successful response from Jigasi, forward it as the response to the client.
192+
request.connection.tryToSendStanza(
193+
responseFromJigasi.apply {
194+
from = null
195+
to = request.iq.from
196+
stanzaId = request.iq.stanzaId
197+
}
198+
)
201199
}
202200

203201
companion object {

0 commit comments

Comments
 (0)