Fix RecordRoute not added in case no special processings from the NatHelper...

Fix RecordRoute not added in case no special processings from the NatHelper are required (FlowTokenStrategy).

src/nat/flow-token-strategy.cc

@@ -83,7 +83,11 @@ void FlowTokenStrategy::preProcessOnRequestNatHelper(const std::shared_ptr<Reque
 }
 
 void FlowTokenStrategy::addRecordRouteNatHelper(const std::shared_ptr<RequestSipEvent>& ev) const {
-	if (!mHelper.requestMeetsRequirements(ev)) return;
+	// If request does not meet requirements to add a flow-token, add a simple record-route.
+	if (!mHelper.requestMeetsRequirements(ev)) {
+		ModuleToolbox::addRecordRouteIncoming(mAgent, ev);
+		return;
+	};
 
 	const auto* sip = ev->getSip();
 

tester/tests/nat/flow-token-strategy-tester.cc

@@ -28,7 +28,6 @@
 #include "flexisip-tester-config.hh"
 #include "nat/contact-correction-strategy.hh"
 #include "utils/nat-test-helper.hh"
-#include "utils/proxy-server.hh"
 #include "utils/string-formatter.hh"
 #include "utils/test-patterns/test.hh"
 #include "utils/test-suite.hh"
@@ -145,6 +144,23 @@ void addRecordRouteNatHelperNoVia() {
 	BC_ASSERT_CPP_EQUAL(recordRouteUrlStr, "sip:127.0.0.1:" + helper.mProxyPort + ";transport=tcp;lr");
 }
 
+/*
+ * Test successful "record-route" addition and url matches the address of the server.
+ * Test it does not contain a flow-token since the "Contact" header does not contain the "ob" parameter.
+ */
+void addRecordRouteNatHelperNoObParameter() {
+	const Helper helper{"false"};
+	const auto event = make_shared<RqSipEv>(helper.mAgent, Helper::getRegister(false), helper.mTport);
+
+	helper.mStrategy.addRecordRouteNatHelper(event);
+
+	const auto* recordRoute = event->getSip()->sip_record_route;
+	BC_HARD_ASSERT(&recordRoute[0] != nullptr);
+
+	const auto* recordRouteUrlStr = url_as_string(event->getHome(), recordRoute[0].r_url);
+	BC_ASSERT_CPP_EQUAL(recordRouteUrlStr, "sip:127.0.0.1:" + helper.mProxyPort + ";transport=tcp;lr");
+}
+
 /*
  * Test return value matches lastRoute with host:port corrected with information present in the flow-token.
  */
@@ -281,15 +297,16 @@ void addPathOnRegisterNotFirstHopWithUniq() {
 
 TestSuite _("NatTraversalStrategy::FlowToken",
             {
-                TEST_NO_TAG_AUTO_NAMED(addRecordRouteNatHelper),
-                TEST_NO_TAG_AUTO_NAMED(addRecordRouteNatHelperNoVia),
-                TEST_NO_TAG_AUTO_NAMED(getTportDestFromLastRoute),
-                TEST_NO_TAG_AUTO_NAMED(getTportDestFromLastRouteWithFalsifiedFlowToken),
-                TEST_NO_TAG_AUTO_NAMED(addRecordRouteForwardModule),
-                TEST_NO_TAG_AUTO_NAMED(addRecordRouteForwardModuleNoRouteUrl),
-                TEST_NO_TAG_AUTO_NAMED(addPathOnRegister),
-                TEST_NO_TAG_AUTO_NAMED(addPathOnRegisterNotFirstHop),
-                TEST_NO_TAG_AUTO_NAMED(addPathOnRegisterNotFirstHopWithUniq),
+                CLASSY_TEST(addRecordRouteNatHelper),
+                CLASSY_TEST(addRecordRouteNatHelperNoVia),
+                CLASSY_TEST(addRecordRouteNatHelperNoObParameter),
+                CLASSY_TEST(getTportDestFromLastRoute),
+                CLASSY_TEST(getTportDestFromLastRouteWithFalsifiedFlowToken),
+                CLASSY_TEST(addRecordRouteForwardModule),
+                CLASSY_TEST(addRecordRouteForwardModuleNoRouteUrl),
+                CLASSY_TEST(addPathOnRegister),
+                CLASSY_TEST(addPathOnRegisterNotFirstHop),
+                CLASSY_TEST(addPathOnRegisterNotFirstHopWithUniq),
             });
 
 } // namespace

tester/tests/nat/nat-traversal-feature-tester.cc

@@ -242,20 +242,94 @@ void registerUser() {
 
 /*
  * Test call establishment when flow-token feature is enabled in module::NatHelper.
+ * Hint: we force the use of a flow-token to simulate the need of processing from the NatHelper module.
  */
 void makeCall() {
 	// Initialization --------------------------------------------------------------------------------------------------
+	url_t recordRoute{};
+	string callerTcpPort{"unexpected"};
+
+	bool firstInvite{true};
+	const string proxyHost = "127.0.0.1";
+	// Inject custom module to get information from processed requests.
+	// Here, we want to make sure the NatHelper module correctly added the record-route.
+	InjectedHooks injectedModuleHooks{
+	    .injectAfterModule = "NatHelper",
+	    .onRequest =
+	        [&firstInvite, &recordRoute, &callerTcpPort](std::shared_ptr<RequestSipEvent>& ev) {
+		        if (firstInvite and ev->getMsgSip()->getSipMethod() == sip_method_invite) {
+			        auto const* rr = ev->getMsgSip()->getSip()->sip_record_route;
+			        auto const* rrUrl = rr->r_url;
+			        recordRoute.url_user = rrUrl ? rrUrl->url_user : "expected";
+			        callerTcpPort = ev->getMsgAddress()->getPortStr();
+			        firstInvite = false;
+		        }
+	        },
+	};
+	Server proxy{{
+	                 {"global/aliases", "localhost"},
+	                 {"global/transports", "sip:" + proxyHost + ":0;transport=tcp"},
+	                 {"module::MediaRelay/enabled", "false"},
+	                 {"module::Registrar/enabled", "true"},
+	                 {"module::Registrar/reg-domains", "localhost"},
+	                 {"module::NatHelper/enabled", "true"},
+	                 {"module::NatHelper/nat-traversal-strategy", "flow-token"},
+	                 {"module::NatHelper/force-flow-token", "true"},
+	             },
+	             &injectedModuleHooks};
+	proxy.start();
+
+	auto builder = ClientBuilder(*proxy.getAgent());
+	builder.setIce(OnOff::Off);
+	auto caller = builder.build("sip:caller@localhost");
+	auto callee = builder.build("sip:callee@localhost");
+	CoreAssert asserter{caller.getCore(), proxy, callee.getCore()};
+	// -----------------------------------------------------------------------------------------------------------------
+	const auto call = caller.call(callee);
+	BC_HARD_ASSERT(call != nullptr);
+	call->terminate();
+
+	FlowTestHelper helper;
+	auto flowToken = helper.mFactory.create(recordRoute.url_user);
+	BC_ASSERT_CPP_EQUAL(flowToken.getData().getLocalAddress()->str(), "127.0.0.1:"s + proxy.getFirstPort());
+	BC_ASSERT_CPP_EQUAL(flowToken.getData().getRemoteAddress()->str(), "127.0.0.1:" + callerTcpPort);
+}
+
+/*
+ * Test call establishment when no processing from the NatHelper is requested (user not behind a NAT).
+ * Hint: here we do not force to use flow-tokens and the Contact header do not contain the "ob" parameter, so it will
+ * fall back on the case where we do not need the NatHelper module to do something special.
+ */
+void makeCallNoNeedForNatHelper() {
+	// Initialization --------------------------------------------------------------------------------------------------
+	string recordRouteUrl{};
+
+	bool firstInvite{true};
 	const string proxyHost = "127.0.0.1";
+	// Inject custom module to get information from processed requests.
+	// Here, we want to make sure the NatHelper module correctly added the record-route.
+	InjectedHooks injectedModuleHooks{
+	    .injectAfterModule = "NatHelper",
+	    .onRequest =
+	        [&firstInvite, &recordRouteUrl](std::shared_ptr<RequestSipEvent>& ev) {
+		        if (firstInvite and ev->getMsgSip()->getSipMethod() == sip_method_invite) {
+			        auto const* rr = ev->getMsgSip()->getSip()->sip_record_route;
+			        recordRouteUrl = rr ? url_as_string(ev->getHome(), rr->r_url) : "expected";
+			        firstInvite = false;
+		        }
+	        },
+	};
 	Server proxy{{
-	    {"global/aliases", "localhost"},
-	    {"global/transports", "sip:" + proxyHost + ":0;transport=tcp"},
-	    {"module::MediaRelay/enabled", "false"},
-	    {"module::Registrar/enabled", "true"},
-	    {"module::Registrar/reg-domains", "localhost"},
-	    {"module::NatHelper/enabled", "true"},
-	    {"module::NatHelper/nat-traversal-strategy", "flow-token"},
-	    {"module::NatHelper/force-flow-token", "true"},
-	}};
+	                 {"global/aliases", "localhost"},
+	                 {"global/transports", "sip:" + proxyHost + ":0;transport=tcp"},
+	                 {"module::MediaRelay/enabled", "false"},
+	                 {"module::Registrar/enabled", "true"},
+	                 {"module::Registrar/reg-domains", "localhost"},
+	                 {"module::NatHelper/enabled", "true"},
+	                 {"module::NatHelper/nat-traversal-strategy", "flow-token"},
+	                 {"module::NatHelper/force-flow-token", "false"},
+	             },
+	             &injectedModuleHooks};
 	proxy.start();
 
 	auto builder = ClientBuilder(*proxy.getAgent());
@@ -267,6 +341,8 @@ void makeCall() {
 	const auto call = caller.call(callee);
 	BC_HARD_ASSERT(call != nullptr);
 	call->terminate();
+
+	BC_ASSERT_CPP_EQUAL(recordRouteUrl, "sip:127.0.0.1:"s + proxy.getFirstPort() + ";transport=tcp;lr");
 }
 
 } // namespace FlowToken
@@ -277,6 +353,7 @@ TestSuite _("NatTraversal",
                 CLASSY_TEST(ContactCorrection::makeCall),
                 CLASSY_TEST(FlowToken::registerUser),
                 CLASSY_TEST(FlowToken::makeCall),
+                CLASSY_TEST(FlowToken::makeCallNoNeedForNatHelper),
             });
 
 } // namespace