diff --git a/pom.xml b/pom.xml index 70b35fe913..c9d58f39d5 100644 --- a/pom.xml +++ b/pom.xml @@ -156,7 +156,7 @@ ${project.groupId} jitsi-xmpp-extensions - 1.0-6-g009420d + 1.0-13-ga40f9c3 org.osgi diff --git a/src/main/java/org/jitsi/impl/protocol/xmpp/DefaultJingleRequestHandler.java b/src/main/java/org/jitsi/impl/protocol/xmpp/DefaultJingleRequestHandler.java index c72018e656..625d821c9a 100644 --- a/src/main/java/org/jitsi/impl/protocol/xmpp/DefaultJingleRequestHandler.java +++ b/src/main/java/org/jitsi/impl/protocol/xmpp/DefaultJingleRequestHandler.java @@ -66,6 +66,14 @@ public XMPPError onSessionAccept(JingleSession jingleSession, return null; } + @Override + public XMPPError onSessionTerminate(JingleSession jingleSession, JingleIQ iq) + { + logger.warn("Ignored Jingle 'session-terminate'"); + + return null; + } + @Override public XMPPError onSessionInfo(JingleSession session, JingleIQ iq) { diff --git a/src/main/java/org/jitsi/jicofo/JitsiMeetConferenceImpl.java b/src/main/java/org/jitsi/jicofo/JitsiMeetConferenceImpl.java index 6a455f77a3..0a6dc8cdb4 100644 --- a/src/main/java/org/jitsi/jicofo/JitsiMeetConferenceImpl.java +++ b/src/main/java/org/jitsi/jicofo/JitsiMeetConferenceImpl.java @@ -1286,7 +1286,11 @@ protected void onMemberLeft(ChatRoomMember chatRoomMember) = findParticipantForChatMember(chatRoomMember); if (leftParticipant != null) { - terminateParticipant(leftParticipant, Reason.GONE, null); + terminateParticipant( + leftParticipant, + Reason.GONE, + null, + /* no need to send session-terminate - gone */ false); } else { @@ -1308,8 +1312,15 @@ else if (participants.size() == 0) private void terminateParticipant(Participant participant, Reason reason, - String message) + String message, + boolean sendSessionTerminate) { + logger.info(String.format( + "Terminating %s, reason: %s, send st: %s", + participant, + reason, + sendSessionTerminate)); + BridgeSession bridgeSession; synchronized (participantLock) { @@ -1317,22 +1328,22 @@ private void terminateParticipant(Participant participant, if (participant.isSessionEstablished()) { JingleSession jingleSession = participant.getJingleSession(); - logger.info("Terminating: " + contactAddress); - jingle.terminateSession(jingleSession, reason, message); + jingle.terminateSession(jingleSession, reason, message, sendSessionTerminate); removeSources( jingleSession, participant.getSourcesCopy(), participant.getSourceGroupsCopy(), false /* no JVB update - will expire */); + + participant.setJingleSession(null); } bridgeSession = participant.terminateBridgeSession(); boolean removed = participants.remove(participant); - logger.info( - "Removed participant: " + removed + ", " + contactAddress); + logger.info("Removed participant: " + removed + ", " + contactAddress); } if (bridgeSession != null) @@ -1523,7 +1534,7 @@ public XMPPError onSessionInfo(JingleSession session, JingleIQ iq) String bridgeSessionId = bsPE != null ? bsPE.getId() : null; BridgeSession bridgeSession = findBridgeSession(participant); - if (bridgeSession != null) + if (bridgeSession != null && bridgeSession.id.equals(bridgeSessionId)) { logger.info(String.format( "Received ICE failed notification from %s, session: %s", @@ -1543,6 +1554,75 @@ public XMPPError onSessionInfo(JingleSession session, JingleIQ iq) return null; } + /** + * Handles 'session-terminate' received from the client. + * + * {@inheritDoc} + */ + @Override + public XMPPError onSessionTerminate(JingleSession session, JingleIQ iq) + { + Participant participant = findParticipantForJingleSession(session); + + // FIXME: (duplicate) there's very similar logic in onSessionAccept/onSessionInfo + if (participant == null) + { + String errorMsg = "No participant for " + session.getAddress(); + + logger.warn("onSessionTerminate: " + errorMsg); + + return XMPPError.from( + XMPPError.Condition.item_not_found, errorMsg).build(); + } + + BridgeSessionPacketExtension bsPE + = iq.getExtension( + BridgeSessionPacketExtension.ELEMENT_NAME, + BridgeSessionPacketExtension.NAMESPACE); + String bridgeSessionId = bsPE != null ? bsPE.getId() : null; + BridgeSession bridgeSession = findBridgeSession(participant); + boolean restartRequested = bsPE != null ? bsPE.isRestart() : false; + + if (bridgeSession == null || !bridgeSession.id.equals(bridgeSessionId)) + { + logger.info(String.format( + "Ignored session-terminate for invalid session: %s, bridge session ID: %s restart: %s", + participant, + bridgeSessionId, + restartRequested)); + + return XMPPError.from(XMPPError.Condition.item_not_found, "invalid bridge session ID").build(); + } + + logger.info(String.format( + "Received session-terminate from %s, session: %s, restart: %s", + participant, + bridgeSession, + restartRequested)); + + synchronized (participantLock) + { + terminateParticipant(participant, null, null, /* do not send session-terminate */ false); + + if (restartRequested) + { + if (participant.incrementAndCheckRestartRequests()) + { + participants.add(participant); + inviteParticipant(participant, false, hasToStartMuted(participant, false)); + } + else + { + logger.warn(String.format("Rate limiting %s for restart requests", participant)); + + return XMPPError.from(XMPPError.Condition.resource_constraint, "rate-limited").build(); + } + } + } + + return null; + } + /** * Advertises new sources across all conference participants by using * 'source-add' Jingle notification. @@ -2507,7 +2587,8 @@ void onInviteFailed(ParticipantChannelAllocator channelAllocator) terminateParticipant( channelAllocator.getParticipant(), Reason.GENERAL_ERROR, - "jingle session failed"); + "jingle session failed", + /* send session-terminate */ true); } /** @@ -2712,19 +2793,20 @@ public void run() if (participants.size() == 1) { Participant p = participants.get(0); - logger.info( - "Timing out single participant: " + p.getMucJid()); + + logger.info("Timing out single participant: " + p.getMucJid()); terminateParticipant( - p, Reason.EXPIRED, "Idle session timeout"); + p, + Reason.EXPIRED, + "Idle session timeout", + /* send session-terminate */ true); disposeConference(); } else { - logger.error( - "Should never execute if more than 1 participant? " - + getRoomName()); + logger.error("Should never execute if more than 1 participant? " + getRoomName()); } singleParticipantTout = null; } diff --git a/src/main/java/org/jitsi/jicofo/LipSyncHack.java b/src/main/java/org/jitsi/jicofo/LipSyncHack.java index a252ce7895..a5b3d100d2 100644 --- a/src/main/java/org/jitsi/jicofo/LipSyncHack.java +++ b/src/main/java/org/jitsi/jicofo/LipSyncHack.java @@ -366,9 +366,10 @@ public void sendRemoveSourceIQ( @Override public void terminateSession(JingleSession session, Reason reason, - String msg) + String msg, + boolean sendTerminate) { - jingleImpl.terminateSession(session, reason, msg); + jingleImpl.terminateSession(session, reason, msg, sendTerminate); } /** diff --git a/src/main/java/org/jitsi/jicofo/Participant.java b/src/main/java/org/jitsi/jicofo/Participant.java index 7ab12ef50a..7ee76b4853 100644 --- a/src/main/java/org/jitsi/jicofo/Participant.java +++ b/src/main/java/org/jitsi/jicofo/Participant.java @@ -26,8 +26,11 @@ import org.jitsi.utils.logging.*; import org.jxmpp.jid.*; +import java.time.*; import java.util.*; +import static java.time.temporal.ChronoUnit.SECONDS; + /** * Class represent Jitsi Meet conference participant. Stores information about * Colibri channels allocated, Jingle session and media sources. @@ -66,6 +69,17 @@ public static String getEndpointId(XmppChatMember chatRoomMember) */ private JitsiMeetConferenceImpl.BridgeSession bridgeSession; + /** + * The {@link Clock} used by this participant. + */ + private Clock clock = Clock.systemUTC(); + + /** + * The list stored the timestamp when the last restart requests have been received for this participant and is used + * for rate limiting. See {@link #incrementAndCheckRestartRequests()} for more details. + */ + private final Deque restartRequests = new LinkedList<>(); + /** * MUC chat member of this participant. */ @@ -152,6 +166,15 @@ void setBridgeSession(JitsiMeetConferenceImpl.BridgeSession bridgeSession) this.bridgeSession = bridgeSession; } + /** + * Sets the new clock instance to be used by this participant. Meant for testing. + * @param newClock - the new {@link Clock} + */ + public void setClock(Clock newClock) + { + this.clock = newClock; + } + /** * Sets {@link JingleSession} established with this peer. * @param jingleSession the new Jingle session to be assigned to this peer. @@ -170,6 +193,14 @@ public XmppChatMember getChatMember() return roomMember; } + /** + * @return {@link Clock} used by this participant instance. + */ + public Clock getClock() + { + return clock; + } + /** * Returns true if this participant supports RTP bundle and RTCP * mux. @@ -205,6 +236,44 @@ public boolean hasRtxSupport() return supportedFeatures.contains(DiscoveryUtil.FEATURE_RTX); } + /** + * Rate limiting mechanism for session restart requests received from participants. + * The rules ar as follows: + * - must be at least 10 second gap between the requests + * - no more than 3 requests within the last minute + * + * @return {@code true} if it's okay to process the request, as in it doesn't violate the current rate limiting + * policy, or {@code false} if the request should be denied. + */ + public boolean incrementAndCheckRestartRequests() + { + final Instant now = Instant.now(clock); + Instant previousRequest = this.restartRequests.peekLast(); + + if (previousRequest == null) + { + this.restartRequests.add(now); + + return true; + } + + if (previousRequest.until(now, SECONDS) < 10) + { + return false; + } + + // Allow only 3 requests within the last minute + this.restartRequests.removeIf(requestTime -> requestTime.until(now, SECONDS) > 60); + if (this.restartRequests.size() > 2) + { + return false; + } + + this.restartRequests.add(now); + + return true; + } + /** * FIXME: we need to remove "is SIP gateway code", but there are still * situations where we need to know whether given peer is a human or not. diff --git a/src/main/java/org/jitsi/protocol/xmpp/AbstractOperationSetJingle.java b/src/main/java/org/jitsi/protocol/xmpp/AbstractOperationSetJingle.java index f0ebd6b5b6..628bd1bd0b 100644 --- a/src/main/java/org/jitsi/protocol/xmpp/AbstractOperationSetJingle.java +++ b/src/main/java/org/jitsi/protocol/xmpp/AbstractOperationSetJingle.java @@ -307,6 +307,9 @@ protected IQ processJingleIQ(JingleIQ iq) case SESSION_INFO: error = requestHandler.onSessionInfo(session, iq); break; + case SESSION_TERMINATE: + error = requestHandler.onSessionTerminate(session, iq); + break; case TRANSPORT_ACCEPT: error = requestHandler.onTransportAccept(session, iq.getContentList()); break; @@ -558,39 +561,47 @@ public void terminateHandlersSessions(JingleRequestHandler requestHandler) { if (session.getRequestHandler() == requestHandler) { - terminateSession(session, Reason.GONE, null); + terminateSession(session, Reason.GONE, null, true); } } } /** - * Terminates given Jingle session by sending 'session-terminate' with some - * {@link Reason} if provided. + * Terminates given Jingle session. This method is to be called either to send 'session-terminate' or to inform + * this operation set that the session has been terminated as a result of 'session-terminate' received from + * the other peer in which case {@code sendTerminate} should be set to {@code false}. * * @param session the JingleSession to terminate. * @param reason one of {@link Reason} enum that indicates why the session * is being ended or null to omit. + * @param sendTerminate when {@code true} it means that a 'session-terminate' is to be sent, otherwise it means + * the session is being ended on the remote peer's request. * {@inheritDoc} */ @Override public void terminateSession(JingleSession session, Reason reason, - String message) + String message, + boolean sendTerminate) { - logger.info("Terminate session: " + session.getAddress()); + logger.info(String.format( + "Terminate session: %s, reason: %s, send terminate: %s", + session.getAddress(), + reason, + sendTerminate)); - // we do not send session-terminate as muc addresses are invalid at this - // point - // FIXME: but there is also connection address available - JingleIQ terminate - = JinglePacketFactory.createSessionTerminate( + if (sendTerminate) + { + JingleIQ terminate + = JinglePacketFactory.createSessionTerminate( getOurJID(), session.getAddress(), session.getSessionID(), reason, message); - getConnection().sendStanza(terminate); + getConnection().sendStanza(terminate); + } sessions.remove(session.getSessionID()); } diff --git a/src/main/java/org/jitsi/protocol/xmpp/JingleRequestHandler.java b/src/main/java/org/jitsi/protocol/xmpp/JingleRequestHandler.java index 43e74b1d6b..ed6be71e65 100644 --- a/src/main/java/org/jitsi/protocol/xmpp/JingleRequestHandler.java +++ b/src/main/java/org/jitsi/protocol/xmpp/JingleRequestHandler.java @@ -87,6 +87,17 @@ XMPPError onSessionAccept(JingleSession jingleSession, */ XMPPError onSessionInfo(JingleSession jingleSession, JingleIQ iq); + /** + * Callback fired when 'session-terminate' is received from the client. + * + * @param jingleSession the session that has received the notification. + * @param iq the full message sent by the client. + * + * @return XMPPError if an error should be returned as response to the original request or null + * to reply with RESULT. + */ + XMPPError onSessionTerminate(JingleSession jingleSession, JingleIQ iq); + /** * Callback fired when 'transport-info' is received from the client. * diff --git a/src/main/java/org/jitsi/protocol/xmpp/OperationSetJingle.java b/src/main/java/org/jitsi/protocol/xmpp/OperationSetJingle.java index c41c80a347..e531617c43 100644 --- a/src/main/java/org/jitsi/protocol/xmpp/OperationSetJingle.java +++ b/src/main/java/org/jitsi/protocol/xmpp/OperationSetJingle.java @@ -125,16 +125,17 @@ void sendRemoveSourceIQ(MediaSourceMap ssrcMap, JingleSession session); /** - * Terminates given session by sending 'session-terminate' IQ which will - * optionally include the Reason supplied. + * Terminates given Jingle session. This method is to be called either to send 'session-terminate' or to inform + * this operation set that the session has been terminated as a result of 'session-terminate' received from + * the other peer in which case {@code sendTerminate} should be set to {@code false}. * - * @param session the JingleSession to be terminated. - * @param reason optional Reason specifying the reason of session - * termination. - * @param message optional text message providing more details about - * the reason for terminating the session. + * @param session the JingleSession to terminate. + * @param reason one of {@link Reason} enum that indicates why the session + * is being ended or null to omit. + * @param sendTerminate when {@code true} it means that a 'session-terminate' is to be sent, otherwise it means + * the session is being ended on the remote peer's request. */ - void terminateSession(JingleSession session, Reason reason, String message); + void terminateSession(JingleSession session, Reason reason, String message, boolean sendTerminate); /** * Terminates all active Jingle Sessions associated with given diff --git a/src/test/java/mock/muc/MockRoomMember.java b/src/test/java/mock/muc/MockRoomMember.java index 00817162f7..e955e7f9ed 100644 --- a/src/test/java/mock/muc/MockRoomMember.java +++ b/src/test/java/mock/muc/MockRoomMember.java @@ -44,7 +44,7 @@ public class MockRoomMember private ChatRoomMemberRole role = ChatRoomMemberRole.MEMBER; - MockRoomMember(EntityFullJid address, MockMultiUserChat chatRoom) + public MockRoomMember(EntityFullJid address, MockMultiUserChat chatRoom) { this.address = address; this.name = address.getResourceOrThrow(); diff --git a/src/test/java/org/jitsi/jicofo/ParticipantTest.java b/src/test/java/org/jitsi/jicofo/ParticipantTest.java new file mode 100644 index 0000000000..78a752e357 --- /dev/null +++ b/src/test/java/org/jitsi/jicofo/ParticipantTest.java @@ -0,0 +1,75 @@ +/* + * Jicofo, the Jitsi Conference Focus. + * + * Copyright @ 2018 - present 8x8, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.jitsi.jicofo; + +import mock.muc.*; +import org.junit.*; +import org.junit.runner.*; +import org.junit.runners.*; +import org.jxmpp.jid.impl.*; +import org.jxmpp.stringprep.*; + +import java.time.*; + +import static org.junit.Assert.*; + +@RunWith(JUnit4.class) +public class ParticipantTest +{ + @Test + public void testRestartReqRateLimiting() + throws XmppStringprepException + { + Participant p = new Participant( + new MockJitsiMeetConference(), + new MockRoomMember(JidCreate.entityFullFrom("something@server.com/1234"), null), + 0); + + p.setClock(Clock.fixed(Instant.now(), ZoneId.systemDefault())); + + assertTrue("should allow 1st request", p.incrementAndCheckRestartRequests()); + assertFalse("should not allow next request immediately", p.incrementAndCheckRestartRequests()); + + p.setClock(Clock.offset(p.getClock(), Duration.ofSeconds(5))); + assertFalse("should not allow next request after 5 seconds", p.incrementAndCheckRestartRequests()); + + p.setClock(Clock.offset(p.getClock(), Duration.ofSeconds(6))); + assertTrue("should allow 2nd request after 11 seconds", p.incrementAndCheckRestartRequests()); + + assertFalse("should not allow 3rd request after 11 seconds", p.incrementAndCheckRestartRequests()); + + p.setClock(Clock.offset(p.getClock(), Duration.ofSeconds(10))); + assertTrue("should allow 3rd request after 21 seconds", p.incrementAndCheckRestartRequests()); + + p.setClock(Clock.offset(p.getClock(), Duration.ofSeconds(11))); + assertFalse("should not allow more than 3 request within the last minute (31 second)", p.incrementAndCheckRestartRequests()); + p.setClock(Clock.offset(p.getClock(), Duration.ofSeconds(10))); + assertFalse("should not allow more than 3 request within the last minute (41 second)", p.incrementAndCheckRestartRequests()); + p.setClock(Clock.offset(p.getClock(), Duration.ofSeconds(10))); + assertFalse("should not allow more than 3 request within the last minute (51 second)", p.incrementAndCheckRestartRequests()); + + p.setClock(Clock.offset(p.getClock(), Duration.ofSeconds(10))); + assertTrue("should allow the 4th request after 60 seconds have passed since the 1st (61 second)", p.incrementAndCheckRestartRequests()); + + p.setClock(Clock.offset(p.getClock(), Duration.ofSeconds(5))); + assertFalse("should not allow the 5th request in 66th second", p.incrementAndCheckRestartRequests()); + + p.setClock(Clock.offset(p.getClock(), Duration.ofSeconds(5))); + assertTrue("should allow the 5th request in 71st second", p.incrementAndCheckRestartRequests()); + } +}